docs/interop/qemu-ga.rst | 19 + meson.build | 16 + qga/commands-bsd.c | 24 - qga/commands-common.h | 9 - qga/commands-linux.c | 1805 +++++++++++++++++++++++++++++ qga/commands-posix.c | 2373 +++----------------------------------- qga/commands-win32.c | 78 +- qga/main.c | 216 ++-- qga/qapi-schema.json | 153 ++- 9 files changed, 2234 insertions(+), 2459 deletions(-)
This series is a side effect of other work I started, to attempt to make the QGA safe to use in confidential VMs by automatically restricting the permitted commands. Since this cleanup stands on its own, I'm sending it now. The QGA codebase has a very complicated maze of #ifdefs to create stubs for the various commands that cannot be implemented on certain platforms. It then has further logic to dynamically disable the stub commands at runtime, except this is not consistently applied, so some commands remain enabled despite being merely stubs. The resulting code is hard to follow, when trying to understand exactly what commands are available under what circumstances, and when changing impls it is easy to get the #ifdefs wrong, resulting in stubs getting missed on platforms without a real impl. In some cases, we have multiple stubs for the same command, due to the maze of #ifdefs. The QAPI schema language has support for many years for expressing conditions against commands when declaring them. This results in the QAPI code generator omitting their implementation entirely at build time. This has mutliple benefits * The unsupported commands are guaranteed to not exist at runtime * No stubs need ever be defined in the code * The generated QAPI reference manual documents the build conditions This series is broadly split into three parts * Moving tonnes of Linux only commands out of commands-posix.c into commands-linux.c to remove many #ifdefs. * Adding 'if' conditions in the QAPI schema to reflect the build conditions, removing many more #ifdefs * Sanitizing the logic for disabling/enabling commands at runtime to guarantee consistency Changed in v2: - Make FSFreeze error reporting distinguish inability to enable VSS from user config choice - Fully remove ga_command_init_blockedrpcs() methods. No more special case disabling of commands. Either they're disabled at build time, or disabled by user config, or by well defined rule ie not permitted during FS freeze. - Apply rules later in startup to avoid crash from NULL config pointer - Document changed error messages in commit messages - Add -c / --config command line parameter - Fix mistaken enabling of fsfreeze hooks on win32 - Remove pointless 'blockrpcs_key' variable - Allow concurrent setting of allow and block lists for RPC commands Daniel P. Berrangé (22): qga: drop blocking of guest-get-memory-block-size command qga: move linux vcpu command impls to commands-linux.c qga: move linux suspend command impls to commands-linux.c qga: move linux fs/disk command impls to commands-linux.c qga: move linux disk/cpu stats command impls to commands-linux.c qga: move linux memory block command impls to commands-linux.c qga: move CONFIG_FSFREEZE/TRIM to be meson defined options qga: conditionalize schema for commands unsupported on Windows qga: conditionalize schema for commands unsupported on non-Linux POSIX qga: conditionalize schema for commands requiring getifaddrs qga: conditionalize schema for commands requiring linux/win32 qga: conditionalize schema for commands only supported on Windows qga: conditionalize schema for commands requiring fsfreeze qga: conditionalize schema for commands requiring fstrim qga: conditionalize schema for commands requiring libudev qga: conditionalize schema for commands requiring utmpx qga: conditionalize schema for commands not supported on other UNIX qga: don't disable fsfreeze commands if vss_init fails qga: move declare of QGAConfig struct to top of file qga: remove pointless 'blockrpcs_key' variable qga: allow configuration file path via the cli qga: centralize logic for disabling/enabling commands docs/interop/qemu-ga.rst | 19 + meson.build | 16 + qga/commands-bsd.c | 24 - qga/commands-common.h | 9 - qga/commands-linux.c | 1805 +++++++++++++++++++++++++++++ qga/commands-posix.c | 2373 +++----------------------------------- qga/commands-win32.c | 78 +- qga/main.c | 216 ++-- qga/qapi-schema.json | 153 ++- 9 files changed, 2234 insertions(+), 2459 deletions(-) -- 2.45.1
Ping: for any review comments from QGA maintainers ? On Thu, Jun 13, 2024 at 04:01:05PM +0100, Daniel P. Berrangé wrote: > This series is a side effect of other work I started, to attempt to > make the QGA safe to use in confidential VMs by automatically > restricting the permitted commands. Since this cleanup stands on > its own, I'm sending it now. > > The QGA codebase has a very complicated maze of #ifdefs to create > stubs for the various commands that cannot be implemented on certain > platforms. It then has further logic to dynamically disable the stub > commands at runtime, except this is not consistently applied, so > some commands remain enabled despite being merely stubs. > > The resulting code is hard to follow, when trying to understand exactly > what commands are available under what circumstances, and when changing > impls it is easy to get the #ifdefs wrong, resulting in stubs getting > missed on platforms without a real impl. In some cases, we have multiple > stubs for the same command, due to the maze of #ifdefs. > > The QAPI schema language has support for many years for expressing > conditions against commands when declaring them. This results in the > QAPI code generator omitting their implementation entirely at build > time. This has mutliple benefits > > * The unsupported commands are guaranteed to not exist at runtime > * No stubs need ever be defined in the code > * The generated QAPI reference manual documents the build conditions > > This series is broadly split into three parts > > * Moving tonnes of Linux only commands out of commands-posix.c > into commands-linux.c to remove many #ifdefs. > * Adding 'if' conditions in the QAPI schema to reflect the > build conditions, removing many more #ifdefs > * Sanitizing the logic for disabling/enabling commands at > runtime to guarantee consistency > > Changed in v2: > > - Make FSFreeze error reporting distinguish inability to enable > VSS from user config choice > > - Fully remove ga_command_init_blockedrpcs() methods. No more > special case disabling of commands. Either they're disabled > at build time, or disabled by user config, or by well defined > rule ie not permitted during FS freeze. > > - Apply rules later in startup to avoid crash from NULL config > pointer > > - Document changed error messages in commit messages > > - Add -c / --config command line parameter > > - Fix mistaken enabling of fsfreeze hooks on win32 > > - Remove pointless 'blockrpcs_key' variable > > - Allow concurrent setting of allow and block lists for > RPC commands > > Daniel P. Berrangé (22): > qga: drop blocking of guest-get-memory-block-size command > qga: move linux vcpu command impls to commands-linux.c > qga: move linux suspend command impls to commands-linux.c > qga: move linux fs/disk command impls to commands-linux.c > qga: move linux disk/cpu stats command impls to commands-linux.c > qga: move linux memory block command impls to commands-linux.c > qga: move CONFIG_FSFREEZE/TRIM to be meson defined options > qga: conditionalize schema for commands unsupported on Windows > qga: conditionalize schema for commands unsupported on non-Linux POSIX > qga: conditionalize schema for commands requiring getifaddrs > qga: conditionalize schema for commands requiring linux/win32 > qga: conditionalize schema for commands only supported on Windows > qga: conditionalize schema for commands requiring fsfreeze > qga: conditionalize schema for commands requiring fstrim > qga: conditionalize schema for commands requiring libudev > qga: conditionalize schema for commands requiring utmpx > qga: conditionalize schema for commands not supported on other UNIX > qga: don't disable fsfreeze commands if vss_init fails > qga: move declare of QGAConfig struct to top of file > qga: remove pointless 'blockrpcs_key' variable > qga: allow configuration file path via the cli > qga: centralize logic for disabling/enabling commands > > docs/interop/qemu-ga.rst | 19 + > meson.build | 16 + > qga/commands-bsd.c | 24 - > qga/commands-common.h | 9 - > qga/commands-linux.c | 1805 +++++++++++++++++++++++++++++ > qga/commands-posix.c | 2373 +++----------------------------------- > qga/commands-win32.c | 78 +- > qga/main.c | 216 ++-- > qga/qapi-schema.json | 153 ++- > 9 files changed, 2234 insertions(+), 2459 deletions(-) > > -- > 2.45.1 > With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hi Daniel On Tue, Jul 2, 2024 at 10:00 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > Ping: for any review comments from QGA maintainers ? Maybe you could resend for patchew to correctly handle the series. thanks > > On Thu, Jun 13, 2024 at 04:01:05PM +0100, Daniel P. Berrangé wrote: > > This series is a side effect of other work I started, to attempt to > > make the QGA safe to use in confidential VMs by automatically > > restricting the permitted commands. Since this cleanup stands on > > its own, I'm sending it now. > > > > The QGA codebase has a very complicated maze of #ifdefs to create > > stubs for the various commands that cannot be implemented on certain > > platforms. It then has further logic to dynamically disable the stub > > commands at runtime, except this is not consistently applied, so > > some commands remain enabled despite being merely stubs. > > > > The resulting code is hard to follow, when trying to understand exactly > > what commands are available under what circumstances, and when changing > > impls it is easy to get the #ifdefs wrong, resulting in stubs getting > > missed on platforms without a real impl. In some cases, we have multiple > > stubs for the same command, due to the maze of #ifdefs. > > > > The QAPI schema language has support for many years for expressing > > conditions against commands when declaring them. This results in the > > QAPI code generator omitting their implementation entirely at build > > time. This has mutliple benefits > > > > * The unsupported commands are guaranteed to not exist at runtime > > * No stubs need ever be defined in the code > > * The generated QAPI reference manual documents the build conditions > > > > This series is broadly split into three parts > > > > * Moving tonnes of Linux only commands out of commands-posix.c > > into commands-linux.c to remove many #ifdefs. > > * Adding 'if' conditions in the QAPI schema to reflect the > > build conditions, removing many more #ifdefs > > * Sanitizing the logic for disabling/enabling commands at > > runtime to guarantee consistency > > > > Changed in v2: > > > > - Make FSFreeze error reporting distinguish inability to enable > > VSS from user config choice > > > > - Fully remove ga_command_init_blockedrpcs() methods. No more > > special case disabling of commands. Either they're disabled > > at build time, or disabled by user config, or by well defined > > rule ie not permitted during FS freeze. > > > > - Apply rules later in startup to avoid crash from NULL config > > pointer > > > > - Document changed error messages in commit messages > > > > - Add -c / --config command line parameter > > > > - Fix mistaken enabling of fsfreeze hooks on win32 > > > > - Remove pointless 'blockrpcs_key' variable > > > > - Allow concurrent setting of allow and block lists for > > RPC commands > > > > Daniel P. Berrangé (22): > > qga: drop blocking of guest-get-memory-block-size command > > qga: move linux vcpu command impls to commands-linux.c > > qga: move linux suspend command impls to commands-linux.c > > qga: move linux fs/disk command impls to commands-linux.c > > qga: move linux disk/cpu stats command impls to commands-linux.c > > qga: move linux memory block command impls to commands-linux.c > > qga: move CONFIG_FSFREEZE/TRIM to be meson defined options > > qga: conditionalize schema for commands unsupported on Windows > > qga: conditionalize schema for commands unsupported on non-Linux POSIX > > qga: conditionalize schema for commands requiring getifaddrs > > qga: conditionalize schema for commands requiring linux/win32 > > qga: conditionalize schema for commands only supported on Windows > > qga: conditionalize schema for commands requiring fsfreeze > > qga: conditionalize schema for commands requiring fstrim > > qga: conditionalize schema for commands requiring libudev > > qga: conditionalize schema for commands requiring utmpx > > qga: conditionalize schema for commands not supported on other UNIX > > qga: don't disable fsfreeze commands if vss_init fails > > qga: move declare of QGAConfig struct to top of file > > qga: remove pointless 'blockrpcs_key' variable > > qga: allow configuration file path via the cli > > qga: centralize logic for disabling/enabling commands > > > > docs/interop/qemu-ga.rst | 19 + > > meson.build | 16 + > > qga/commands-bsd.c | 24 - > > qga/commands-common.h | 9 - > > qga/commands-linux.c | 1805 +++++++++++++++++++++++++++++ > > qga/commands-posix.c | 2373 +++----------------------------------- > > qga/commands-win32.c | 78 +- > > qga/main.c | 216 ++-- > > qga/qapi-schema.json | 153 ++- > > 9 files changed, 2234 insertions(+), 2459 deletions(-) > > > > -- > > 2.45.1 > > > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Wed, Jul 03, 2024 at 10:15:44AM +0400, Marc-André Lureau wrote: > Hi Daniel > > On Tue, Jul 2, 2024 at 10:00 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > Ping: for any review comments from QGA maintainers ? > > Maybe you could resend for patchew to correctly handle the series. I don't want to spam the list again just because patchew didn't handle it. If anyone can't just pull the messages from mail though, they are also at https://gitlab.com/berrange/qemu/-/tags/qga-conditions-v2 With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Hi On Wed, Jul 3, 2024 at 12:06 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Jul 03, 2024 at 10:15:44AM +0400, Marc-André Lureau wrote: > > Hi Daniel > > > > On Tue, Jul 2, 2024 at 10:00 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > Ping: for any review comments from QGA maintainers ? > > > > Maybe you could resend for patchew to correctly handle the series. > > I don't want to spam the list again just because patchew didn't > handle it. If anyone can't just pull the messages from mail > though, they are also at https://gitlab.com/berrange/qemu/-/tags/qga-conditions-v2 patchew also handles R-b/A-b/T-b tags, mail ref, versioning etc..
Hi On Thu, Jun 13, 2024 at 7:02 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > This series is a side effect of other work I started, to attempt to > make the QGA safe to use in confidential VMs by automatically > restricting the permitted commands. Since this cleanup stands on > its own, I'm sending it now. > > The QGA codebase has a very complicated maze of #ifdefs to create > stubs for the various commands that cannot be implemented on certain > platforms. It then has further logic to dynamically disable the stub > commands at runtime, except this is not consistently applied, so > some commands remain enabled despite being merely stubs. > > The resulting code is hard to follow, when trying to understand exactly > what commands are available under what circumstances, and when changing > impls it is easy to get the #ifdefs wrong, resulting in stubs getting > missed on platforms without a real impl. In some cases, we have multiple > stubs for the same command, due to the maze of #ifdefs. > > The QAPI schema language has support for many years for expressing > conditions against commands when declaring them. This results in the > QAPI code generator omitting their implementation entirely at build > time. This has mutliple benefits > > * The unsupported commands are guaranteed to not exist at runtime > * No stubs need ever be defined in the code > * The generated QAPI reference manual documents the build conditions > > This series is broadly split into three parts > > * Moving tonnes of Linux only commands out of commands-posix.c > into commands-linux.c to remove many #ifdefs. > * Adding 'if' conditions in the QAPI schema to reflect the > build conditions, removing many more #ifdefs > * Sanitizing the logic for disabling/enabling commands at > runtime to guarantee consistency > > Changed in v2: > > - Make FSFreeze error reporting distinguish inability to enable > VSS from user config choice > > - Fully remove ga_command_init_blockedrpcs() methods. No more > special case disabling of commands. Either they're disabled > at build time, or disabled by user config, or by well defined > rule ie not permitted during FS freeze. > > - Apply rules later in startup to avoid crash from NULL config > pointer > > - Document changed error messages in commit messages > > - Add -c / --config command line parameter > > - Fix mistaken enabling of fsfreeze hooks on win32 > > - Remove pointless 'blockrpcs_key' variable > > - Allow concurrent setting of allow and block lists for > RPC commands > > Daniel P. Berrangé (22): > qga: drop blocking of guest-get-memory-block-size command > qga: move linux vcpu command impls to commands-linux.c > qga: move linux suspend command impls to commands-linux.c > qga: move linux fs/disk command impls to commands-linux.c > qga: move linux disk/cpu stats command impls to commands-linux.c > qga: move linux memory block command impls to commands-linux.c > qga: move CONFIG_FSFREEZE/TRIM to be meson defined options > qga: conditionalize schema for commands unsupported on Windows > qga: conditionalize schema for commands unsupported on non-Linux POSIX > qga: conditionalize schema for commands requiring getifaddrs > qga: conditionalize schema for commands requiring linux/win32 > qga: conditionalize schema for commands only supported on Windows > qga: conditionalize schema for commands requiring fsfreeze > qga: conditionalize schema for commands requiring fstrim > qga: conditionalize schema for commands requiring libudev > qga: conditionalize schema for commands requiring utmpx > qga: conditionalize schema for commands not supported on other UNIX > qga: don't disable fsfreeze commands if vss_init fails > qga: move declare of QGAConfig struct to top of file > qga: remove pointless 'blockrpcs_key' variable > qga: allow configuration file path via the cli > qga: centralize logic for disabling/enabling commands > > Something broke patchew handling: https://patchew.org/QEMU/20240613150127.1361931-1-berrange@redhat.com/20240613154406.1365469-1-berrange@redhat.com/ -- Marc-André Lureau
On Fri, Jun 14, 2024 at 12:34:52PM +0400, Marc-André Lureau wrote: > Hi > > On Thu, Jun 13, 2024 at 7:02 PM Daniel P. Berrangé <berrange@redhat.com> > wrote: > > > This series is a side effect of other work I started, to attempt to > > make the QGA safe to use in confidential VMs by automatically > > restricting the permitted commands. Since this cleanup stands on > > its own, I'm sending it now. > > > > The QGA codebase has a very complicated maze of #ifdefs to create > > stubs for the various commands that cannot be implemented on certain > > platforms. It then has further logic to dynamically disable the stub > > commands at runtime, except this is not consistently applied, so > > some commands remain enabled despite being merely stubs. > > > > The resulting code is hard to follow, when trying to understand exactly > > what commands are available under what circumstances, and when changing > > impls it is easy to get the #ifdefs wrong, resulting in stubs getting > > missed on platforms without a real impl. In some cases, we have multiple > > stubs for the same command, due to the maze of #ifdefs. > > > > The QAPI schema language has support for many years for expressing > > conditions against commands when declaring them. This results in the > > QAPI code generator omitting their implementation entirely at build > > time. This has mutliple benefits > > > > * The unsupported commands are guaranteed to not exist at runtime > > * No stubs need ever be defined in the code > > * The generated QAPI reference manual documents the build conditions > > > > This series is broadly split into three parts > > > > * Moving tonnes of Linux only commands out of commands-posix.c > > into commands-linux.c to remove many #ifdefs. > > * Adding 'if' conditions in the QAPI schema to reflect the > > build conditions, removing many more #ifdefs > > * Sanitizing the logic for disabling/enabling commands at > > runtime to guarantee consistency > > > > Changed in v2: > > > > - Make FSFreeze error reporting distinguish inability to enable > > VSS from user config choice > > > > - Fully remove ga_command_init_blockedrpcs() methods. No more > > special case disabling of commands. Either they're disabled > > at build time, or disabled by user config, or by well defined > > rule ie not permitted during FS freeze. > > > > - Apply rules later in startup to avoid crash from NULL config > > pointer > > > > - Document changed error messages in commit messages > > > > - Add -c / --config command line parameter > > > > - Fix mistaken enabling of fsfreeze hooks on win32 > > > > - Remove pointless 'blockrpcs_key' variable > > > > - Allow concurrent setting of allow and block lists for > > RPC commands > > > > Daniel P. Berrangé (22): > > qga: drop blocking of guest-get-memory-block-size command > > qga: move linux vcpu command impls to commands-linux.c > > qga: move linux suspend command impls to commands-linux.c > > qga: move linux fs/disk command impls to commands-linux.c > > qga: move linux disk/cpu stats command impls to commands-linux.c > > qga: move linux memory block command impls to commands-linux.c > > qga: move CONFIG_FSFREEZE/TRIM to be meson defined options > > qga: conditionalize schema for commands unsupported on Windows > > qga: conditionalize schema for commands unsupported on non-Linux POSIX > > qga: conditionalize schema for commands requiring getifaddrs > > qga: conditionalize schema for commands requiring linux/win32 > > qga: conditionalize schema for commands only supported on Windows > > qga: conditionalize schema for commands requiring fsfreeze > > qga: conditionalize schema for commands requiring fstrim > > qga: conditionalize schema for commands requiring libudev > > qga: conditionalize schema for commands requiring utmpx > > qga: conditionalize schema for commands not supported on other UNIX > > qga: don't disable fsfreeze commands if vss_init fails > > qga: move declare of QGAConfig struct to top of file > > qga: remove pointless 'blockrpcs_key' variable > > qga: allow configuration file path via the cli > > qga: centralize logic for disabling/enabling commands > > > > > Something broke patchew handling: > https://patchew.org/QEMU/20240613150127.1361931-1-berrange@redhat.com/20240613154406.1365469-1-berrange@redhat.com/ gmail refused further mail delivery from me part way through sending the series, so I had to send the 2nd half of it separately. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2024 Red Hat, Inc.