Re: [PATCH v2 00/22] qga: clean up command source locations and conditionals

[Marc-André Lureau]

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 :|
>