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
@@ -1494,11 +1494,6 @@ out:
     }
 }
 
-void qmp_guest_suspend_hybrid(Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-}
-
 static IP_ADAPTER_ADDRESSES *guest_get_adapters_addresses(Error **errp)
 {
     IP_ADAPTER_ADDRESSES *adptr_addrs = NULL;
@@ -1862,12 +1857,6 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
     return NULL;
 }
 
-int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return -1;
-}
-
 static gchar *
 get_net_error_message(gint error)
 {
@@ -1969,46 +1958,15 @@ done:
     g_free(rawpasswddata);
 }
 
-GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
-
-GuestMemoryBlockResponseList *
-qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
-
-GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
-
 /* 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++));
@@ -2505,15 +2463,3 @@ char *qga_get_host_name(Error **errp)
 
     return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
 }
-
-GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
-
-GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
-{
-    error_setg(errp, QERR_UNSUPPORTED);
-    return NULL;
-}
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index b3de1fb6b3..b91456e9ad 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -636,7 +636,8 @@
 #
 # Since: 1.1
 ##
-{ 'command': 'guest-suspend-hybrid', 'success-response': false }
+{ 'command': 'guest-suspend-hybrid', 'success-response': false,
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @GuestIpAddressType:
@@ -806,7 +807,8 @@
 ##
 { 'command': 'guest-set-vcpus',
   'data':    {'vcpus': ['GuestLogicalProcessor'] },
-  'returns': 'int' }
+  'returns': 'int',
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @GuestDiskBusType:
@@ -1099,7 +1101,8 @@
 { 'struct': 'GuestMemoryBlock',
   'data': {'phys-index': 'uint64',
            'online': 'bool',
-           '*can-offline': 'bool'} }
+           '*can-offline': 'bool'},
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @guest-get-memory-blocks:
@@ -1115,7 +1118,8 @@
 # Since: 2.3
 ##
 { 'command': 'guest-get-memory-blocks',
-  'returns': ['GuestMemoryBlock'] }
+  'returns': ['GuestMemoryBlock'],
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @GuestMemoryBlockResponseType:
@@ -1138,7 +1142,8 @@
 ##
 { 'enum': 'GuestMemoryBlockResponseType',
   'data': ['success', 'not-found', 'operation-not-supported',
-           'operation-failed'] }
+           'operation-failed'],
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @GuestMemoryBlockResponse:
@@ -1156,7 +1161,8 @@
 { 'struct': 'GuestMemoryBlockResponse',
   'data': { 'phys-index': 'uint64',
             'response': 'GuestMemoryBlockResponseType',
-            '*error-code': 'int' }}
+            '*error-code': 'int' },
+  'if': 'CONFIG_POSIX'}
 
 ##
 # @guest-set-memory-blocks:
@@ -1187,7 +1193,8 @@
 ##
 { 'command': 'guest-set-memory-blocks',
   'data':    {'mem-blks': ['GuestMemoryBlock'] },
-  'returns': ['GuestMemoryBlockResponse'] }
+  'returns': ['GuestMemoryBlockResponse'],
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @GuestMemoryBlockInfo:
@@ -1199,7 +1206,8 @@
 # Since: 2.3
 ##
 { 'struct': 'GuestMemoryBlockInfo',
-  'data': {'size': 'uint64'} }
+  'data': {'size': 'uint64'},
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @guest-get-memory-block-info:
@@ -1211,7 +1219,8 @@
 # Since: 2.3
 ##
 { 'command': 'guest-get-memory-block-info',
-  'returns': 'GuestMemoryBlockInfo' }
+  'returns': 'GuestMemoryBlockInfo',
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @GuestExecStatus:
@@ -1702,7 +1711,8 @@
   'data': {'name': 'str',
            'major': 'uint64',
            'minor': 'uint64',
-           'stats': 'GuestDiskStats' } }
+           'stats': 'GuestDiskStats' },
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @guest-get-diskstats:
@@ -1714,7 +1724,8 @@
 # Since: 7.1
 ##
 { 'command': 'guest-get-diskstats',
-  'returns': ['GuestDiskStatsInfo']
+  'returns': ['GuestDiskStatsInfo'],
+  'if': 'CONFIG_POSIX'
 }
 
 ##
@@ -1727,7 +1738,8 @@
 # Since: 7.1
 ##
 { 'enum': 'GuestCpuStatsType',
-  'data': [ 'linux' ] }
+  'data': [ 'linux' ],
+  'if': 'CONFIG_POSIX' }
 
 
 ##
@@ -1772,7 +1784,8 @@
            '*steal': 'uint64',
            '*guest': 'uint64',
            '*guestnice': 'uint64'
-           } }
+           },
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @GuestCpuStats:
@@ -1786,7 +1799,8 @@
 { 'union': 'GuestCpuStats',
   'base': { 'type': 'GuestCpuStatsType' },
   'discriminator': 'type',
-  'data': { 'linux': 'GuestLinuxCpuStats' } }
+  'data': { 'linux': 'GuestLinuxCpuStats' },
+  'if': 'CONFIG_POSIX' }
 
 ##
 # @guest-get-cpustats:
@@ -1798,5 +1812,6 @@
 # Since: 7.1
 ##
 { 'command': 'guest-get-cpustats',
-  'returns': ['GuestCpuStats']
+  'returns': ['GuestCpuStats'],
+  'if': 'CONFIG_POSIX'
 }
-- 
2.45.1

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?

[...]

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

On Thu, Jun 13, 2024 at 2:43 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

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

If Windows VSS is disabled vss_init could fail too. Also, one person
reported
an error that VSS failed to initialize after the first boot. I can't
reproduce this
so I have not details but we should check in runtime that QGA-VSS is
initialized
before trying to freeze fs, in case to be more sure where is error.


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

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

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.

The commit message should mention that the value of "error" in the error
response changes from

    {"class": "GenericError, "desc": "this feature or command is not currently supported"}

to

    {"class": "CommandNotFound", "desc": "The command FOO has not been found"}

> This fixes inconsistency where some commands are implemented
> as stubs, yet not added to the blockedrpc list.

Example?

> This has the additional benefit that the QGA protocol reference
> now documents what conditions enable use of the command.

Yes!

> 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
> @@ -1494,11 +1494,6 @@ out:
>      }
>  }
>  
> -void qmp_guest_suspend_hybrid(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -}
> -
>  static IP_ADAPTER_ADDRESSES *guest_get_adapters_addresses(Error **errp)
>  {
>      IP_ADAPTER_ADDRESSES *adptr_addrs = NULL;
> @@ -1862,12 +1857,6 @@ GuestLogicalProcessorList *qmp_guest_get_vcpus(Error **errp)
>      return NULL;
>  }
>  
> -int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return -1;
> -}
> -
>  static gchar *
>  get_net_error_message(gint error)
>  {
> @@ -1969,46 +1958,15 @@ done:
>      g_free(rawpasswddata);
>  }
>  
> -GuestMemoryBlockList *qmp_guest_get_memory_blocks(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> -
> -GuestMemoryBlockResponseList *
> -qmp_guest_set_memory_blocks(GuestMemoryBlockList *mem_blks, Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> -
> -GuestMemoryBlockInfo *qmp_guest_get_memory_block_info(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> -
>  /* 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;

Can you make @p const and drop the cast?

>  
>          while (*p) {
>              blockedrpcs = g_list_append(blockedrpcs, g_strdup(*p++));
> @@ -2505,15 +2463,3 @@ char *qga_get_host_name(Error **errp)
>  
>      return g_utf16_to_utf8(tmp, size, NULL, NULL, NULL);
>  }
> -
> -GuestDiskStatsInfoList *qmp_guest_get_diskstats(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}
> -
> -GuestCpuStatsList *qmp_guest_get_cpustats(Error **errp)
> -{
> -    error_setg(errp, QERR_UNSUPPORTED);
> -    return NULL;
> -}

Reducing use of QERR_UNSUPPORTED is lovely.


[Schema patch snipped; it looks good to me...]

On Tue, Jun 11, 2024 at 11:13:00AM +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.
> 
> The commit message should mention that the value of "error" in the error
> response changes from
> 
>     {"class": "GenericError, "desc": "this feature or command is not currently supported"}
> 
> to
> 
>     {"class": "CommandNotFound", "desc": "The command FOO has not been found"}

Actually it doesn't change like this, because the runtime disablement
means the stub never runs. So the actual differenceis this:

    {"class": "CommandNotFound", "desc": "Command FOO has been disabled"}
    
to
    
    {"class": "CommandNotFound", "desc": "The command FOO has not been found"}

still better, because it more accurately describes the sitution
where a command is not implemented on a platform configuration.

I'll add this to the commit msg.

> 
> > This fixes inconsistency where some commands are implemented
> > as stubs, yet not added to the blockedrpc list.
> 
> Example?

guest-get-cpustats & guest-get-diskstats. These two do actally
have their error message change in the way you describe. I'll
document this too.

> 
> > This has the additional benefit that the QGA protocol reference
> > now documents what conditions enable use of the command.
> 
> Yes!
> 

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