Re: [PATCH v4 17/22] qapi: introduce x-query-cmma QMP command

[Philippe Mathieu-Daudé]

On 10/28/21 17:54, Daniel P. Berrangé wrote:
> This is a counterpart to the HMP "info cmma" command. It is being
> added with an "x-" prefix because this QMP command is intended as an
> adhoc debugging tool and will thus not be modelled in QAPI as fully
> structured data, nor will it have long term guaranteed stability.
> The existing HMP command is rewritten to call the QMP command.
> 
> This command is unable to use the pre-existing HumanReadableText,
> because if 'common.json' is included into 'machine-target.json'
> the static marshalling method for HumanReadableText will be reported
> as unused by the compiler on all architectures except s390x.
> 
> Possible options were
> 
>  1 Support 'if' conditionals on 'include' statements in QAPI
>  2 Add further commands to 'machine-target.json' that use
>    HumanReadableText, such that it has at least one usage
>    on all architecture targets.
>  3 Duplicate HumanReadableText as TargetHumanReadableText
>    adding conditions
> 
> This patch takes option (3) in the belief that we will eventually
> get to a point where option (2) happens, and TargetHumanReadableText
> can be removed again.

Aren't you using (1) here? TargetHumanReadableText was added in v2
but v3 diverged and doesn't add it anymore.

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  hw/s390x/s390-stattrib.c | 56 +++++++++++++++++++++++++++-------------
>  qapi/machine-target.json | 14 ++++++++++
>  2 files changed, 52 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
> index 9eda1c3b2a..d8f5867a5f 100644
> --- a/hw/s390x/s390-stattrib.c
> +++ b/hw/s390x/s390-stattrib.c
> @@ -17,6 +17,8 @@
>  #include "qemu/error-report.h"
>  #include "exec/ram_addr.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-commands-machine-target.h"
> +#include "qapi/type-helpers.h"
>  #include "qapi/qmp/qdict.h"
>  
>  /* 512KiB cover 2GB of guest memory */
> @@ -67,41 +69,59 @@ void hmp_migrationmode(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> -void hmp_info_cmma(Monitor *mon, const QDict *qdict)
> +HumanReadableText *qmp_x_query_cmma(int64_t addr,
> +                                    bool has_count,
> +                                    int64_t count,
> +                                    Error **errp)
>  {
> +    g_autoptr(GString) buf = g_string_new("");
>      S390StAttribState *sas = s390_get_stattrib_device();
>      S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
> -    uint64_t addr = qdict_get_int(qdict, "addr");
> -    uint64_t buflen = qdict_get_try_int(qdict, "count", 8);
> -    uint8_t *vals;
> +    g_autofree uint8_t *vals = NULL;
>      int cx, len;
>  
> -    vals = g_try_malloc(buflen);
> +    vals = g_try_malloc(count);
>      if (!vals) {
> -        monitor_printf(mon, "Error: %s\n", strerror(errno));
> -        return;
> +        error_setg_errno(errp, errno, "cannot allocate CMMA attribute 
> values");
> +        return NULL;
>      }
>  
> -    len = sac->peek_stattr(sas, addr / TARGET_PAGE_SIZE, buflen, vals);
> +    len = sac->peek_stattr(sas, addr / TARGET_PAGE_SIZE, count, vals);
>      if (len < 0) {
> -        monitor_printf(mon, "Error: %s", strerror(-len));
> -        goto out;
> +        error_setg_errno(errp, -len, "cannot peek at CMMA attribute values");
> +        return NULL;
>      }
>  
> -    monitor_printf(mon, "  CMMA attributes, "
> -                   "pages %" PRIu64 "+%d (0x%" PRIx64 "):\n",
> -                   addr / TARGET_PAGE_SIZE, len, addr & ~TARGET_PAGE_MASK);
> +    g_string_append_printf(buf, "  CMMA attributes, "
> +                           "pages %" PRIu64 "+%d (0x%" PRIx64 "):\n",
> +                           addr / TARGET_PAGE_SIZE, len,
> +                           addr & ~TARGET_PAGE_MASK);
>      for (cx = 0; cx < len; cx++) {
>          if (cx % 8 == 7) {
> -            monitor_printf(mon, "%02x\n", vals[cx]);
> +            g_string_append_printf(buf, "%02x\n", vals[cx]);
>          } else {
> -            monitor_printf(mon, "%02x", vals[cx]);
> +            g_string_append_printf(buf, "%02x", vals[cx]);
>          }
>      }
> -    monitor_printf(mon, "\n");
> +    g_string_append_printf(buf, "\n");
> +
> +    return human_readable_text_from_str(buf);
> +}
> +
> +void hmp_info_cmma(Monitor *mon, const QDict *qdict)
> +{
> +    Error *err = NULL;
> +    g_autoptr(HumanReadableText) info = NULL;
> +    uint64_t addr = qdict_get_int(qdict, "addr");
> +    uint64_t count = qdict_get_try_int(qdict, "count", 8);
> +
> +    info = qmp_x_query_cmma(addr, true, count, &err);
> +    if (err) {
> +        error_report_err(err);
> +        return;
> +    }
>  
> -out:
> -    g_free(vals);
> +    monitor_printf(mon, "%s", info->human_readable_text);
>  }
>  
>  /* Migration support: */
> diff --git a/qapi/machine-target.json b/qapi/machine-target.json
> index 7d4e73462f..19e44987af 100644
> --- a/qapi/machine-target.json
> +++ b/qapi/machine-target.json
> @@ -345,6 +345,20 @@
>                     'TARGET_MIPS' ] } }
>  
>  
> +##
> +# @x-query-cmma:
> +#
> +# Query the values of the CMMA storage attributes for a range of pages
> +#
> +# Returns: CMMA storage attribute values
> +#
> +# Since: 6.2
> +##
> +{ 'command': 'x-query-cmma',
> +  'data': { 'addr': 'int', '*count': 'int' },
> +  'returns': 'HumanReadableText',
> +  'if': 'TARGET_S390X' }
> +
>  ##
>  # @x-query-skeys:
>  #
>