Re: [PATCH v3 6/8] hmp: add filtering of statistics by provider

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Allow the user to request statistics for a single provider of interest.
> Extracted from a patch by Mark Kanda.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hmp-commands-info.hx |  7 ++++---
>  monitor/hmp-cmds.c   | 41 ++++++++++++++++++++++++++++++++++-------
>  2 files changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 221feab8c0..d4d8a1e618 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -897,9 +897,10 @@ ERST
>  
>      {
>          .name       = "stats",
> -        .args_type  = "target:s",
> -        .params     = "target",
> -        .help       = "show statistics; target is either vm or vcpu",
> +        .args_type  = "target:s,provider:s?",
> +        .params     = "target [provider]",
> +        .help       = "show statistics for the given target (vm or vcpu); optionally filter by "
> +                      "provider",
>          .cmd        = hmp_info_stats,
>      },
>  
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index 15f1743d8c..4fa671fe0a 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -2291,6 +2291,7 @@ static StatsSchemaValueList *find_schema_value_list(
>  }
>  
>  static void print_stats_results(Monitor *mon, StatsTarget target,
> +                                bool show_provider,
>                                  StatsResult *result,
>                                  StatsSchemaList *schema)
>  {
> @@ -2305,8 +2306,10 @@ static void print_stats_results(Monitor *mon, StatsTarget target,
>          return;
>      }
>  
> -    monitor_printf(mon, "provider: %s\n",
> -                   StatsProvider_str(result->provider));
> +    if (show_provider) {
> +        monitor_printf(mon, "provider: %s\n",
> +                       StatsProvider_str(result->provider));
> +    }
>  
>      for (stats_list = result->stats; stats_list;
>               stats_list = stats_list->next,
> @@ -2347,7 +2350,8 @@ static void print_stats_results(Monitor *mon, StatsTarget target,
>  }
>  
>  /* Create the StatsFilter that is needed for an "info stats" invocation.  */
> -static StatsFilter *stats_filter(StatsTarget target, int cpu_index)
> +static StatsFilter *stats_filter(StatsTarget target, int cpu_index,
> +                                 StatsProvider provider)
>  {
>      StatsFilter *filter = g_malloc0(sizeof(*filter));
>  
> @@ -2369,12 +2373,27 @@ static StatsFilter *stats_filter(StatsTarget target, int cpu_index)
>      default:
>          break;
>      }
> +
> +    if (provider == STATS_PROVIDER__MAX) {
> +        return filter;
> +    }
> +
> +    /* "info stats" can only query either one or all the providers.  */
> +    StatsRequest *request = g_new0(StatsRequest, 1);
> +    request->provider = provider;
> +    StatsRequestList *request_list = g_new0(StatsRequestList, 1);

Why that g_new0 there? isn't that request_list = NULL and let the
PREPEND below do the alloc?

> +    QAPI_LIST_PREPEND(request_list, request);
> +    filter->has_providers = true;
> +    filter->providers = request_list;
>      return filter;
>  }
>  
>  void hmp_info_stats(Monitor *mon, const QDict *qdict)
>  {
>      const char *target_str = qdict_get_str(qdict, "target");
> +    const char *provider_str = qdict_get_try_str(qdict, "provider");
> +
> +    StatsProvider provider = STATS_PROVIDER__MAX;
>      StatsTarget target;
>      Error *err = NULL;
>      g_autoptr(StatsSchemaList) schema = NULL;
> @@ -2387,19 +2406,27 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
>          monitor_printf(mon, "invalid stats target %s\n", target_str);
>          goto exit_no_print;
>      }
> +    if (provider_str) {
> +        provider = qapi_enum_parse(&StatsProvider_lookup, provider_str, -1, &err);
> +        if (err) {
> +            monitor_printf(mon, "invalid stats provider %s\n", provider_str);
> +            goto exit_no_print;
> +        }
> +    }
>  
> -    schema = qmp_query_stats_schemas(false, 0, &err);
> +    schema = qmp_query_stats_schemas(provider_str ? true : false,
> +                                     provider, &err);
>      if (err) {
>          goto exit;
>      }
>  
>      switch (target) {
>      case STATS_TARGET_VM:
> -        filter = stats_filter(target, -1);
> +        filter = stats_filter(target, -1, provider);
>          break;
>      case STATS_TARGET_VCPU: {}
>          int cpu_index = monitor_get_cpu_index(mon);
> -        filter = stats_filter(target, cpu_index);
> +        filter = stats_filter(target, cpu_index, provider);
>          break;
>      default:
>          abort();
> @@ -2410,7 +2437,7 @@ void hmp_info_stats(Monitor *mon, const QDict *qdict)
>          goto exit;
>      }
>      for (entry = stats; entry; entry = entry->next) {
> -        print_stats_results(mon, target, entry->value, schema);
> +        print_stats_results(mon, target, provider_str == NULL, entry->value, schema);
>      }
>  
>  exit:
> -- 
> 2.36.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK