Re: [PATCH 08/20] qga: conditionalize schema for commands unsupported on Windows

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Jun 11, 2024 at 03:55:37PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Rather than creating stubs for every command that just return
> > QERR_UNSUPPORTED, use 'if' conditions in the QAPI schema to
> > fully exclude generation of the commands on Windows.
> >
> > The command will be rejected at QMP dispatch time instead,
> > avoiding reimplementing rejection by blocking the stub commands.
> >
> > This fixes inconsistency where some commands are implemented
> > as stubs, yet not added to the blockedrpc list.
> >
> > This has the additional benefit that the QGA protocol reference
> > now documents what conditions enable use of the command.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  qga/commands-win32.c | 56 +-------------------------------------------
> >  qga/qapi-schema.json | 45 +++++++++++++++++++++++------------
> >  2 files changed, 31 insertions(+), 70 deletions(-)
> >
> > diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> > index 9fe670d5b4..2533e4c748 100644
> > --- a/qga/commands-win32.c
> > +++ b/qga/commands-win32.c
> 
> [...]
> 
> >  /* add unsupported commands to the list of blocked RPCs */
> >  GList *ga_command_init_blockedrpcs(GList *blockedrpcs)
> >  {
> > -    const char *list_unsupported[] = {
> > -        "guest-suspend-hybrid",
> > -        "guest-set-vcpus",
> > -        "guest-get-memory-blocks", "guest-set-memory-blocks",
> > -        "guest-get-memory-block-info",
> > -        NULL};
> > -    char **p = (char **)list_unsupported;
> > -
> > -    while (*p) {
> > -        blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
> > -    }
> > -
> >      if (!vss_init(true)) {
> >          g_debug("vss_init failed, vss commands are going to be disabled");
> >          const char *list[] = {
> >              "guest-get-fsinfo", "guest-fsfreeze-status",
> >              "guest-fsfreeze-freeze", "guest-fsfreeze-thaw", NULL};
> > -        p = (char **)list;
> > +        char **p = (char **)list;
> >  
> >          while (*p) {
> >              blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
>            }
>        }
> 
>        return blockedrpcs;
>    }
> 
> Four commands get disabled when vss_init() fails, i.e. when qga-vss.dll
> can't be loaded and initialized.
> 
> Three of the four commands do this first:
> 
>         if (!vss_initialized()) {
>             error_setg(errp, QERR_UNSUPPORTED);
>             return 0;
>         }
> 
> The execption is qmp_guest_get_fsinfo().
> 
> vss_initialized() returns true between successful vss_init() and
> vss_deinit().
> 
> Aside: we call vss_init() in three places.  Two of them init, call
> something, then deinit.  Weird.  Moving on.

The two odd balls are a special case for the Windows only --service
argument, which installs a Windows system service for VSS. In that
case, the QGA immediately exits after (un)installing the service.
So the vss_init+vss_deinit pairly makes sense there.

> If these commands are meant to be only available when the DLL is, then
> having them check vss_initialized() is useless.

The DLL should always exist unless the install is broken, but versions
of Windows prior to win2k3 don't support the required APIs, so vss_init
could fail.

> Conversely, if the check isn't useless, then the "make it available
> only" business is.

The 'make it available only" business is bad practice, as it does not
allow a caller to distinguish between the admin manually disabling
these commands, vs the commands being unavailable due to the OS not
supporting the feature. This distinction  is important to preserve
to benefit those triaging bugs about why this might fail.

I'm going to get rid of the runtime blocking in ga_command_init_blockedrpcs,
and also replace QERR_UNSUPPORTED with a more targetted error message to
clearly articulate why the commands are failing.

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