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.
> 
> If these commands are meant to be only available when the DLL is, then
> having them check vss_initialized() is useless.
> 
> Conversely, if the check isn't useless, then the "make it available
> only" business is.
> 
> Opportunity for further cleanup?

If we eliminate the "make it available" check in ga_command_init_blockedrpcs,
that would be a nice cleanup IMHO, as these few commands are the only
special case where that's needed now.

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