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

[Markus Armbruster]

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