| Message ID | 20240613150127.1361931-1-berrange@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | qga: clean up command source locations and conditionals | expand |
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/
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
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
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
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..