Re: [PATCH v2 0/3] Support fd-based KVM stats

[Paolo Bonzini]

On 11/19/21 20:51, Mark Kanda wrote:
> v2: [Paolo]
> - generalize the interface
> - add support for querying stat schema and instances
> - add additional HMP semantic processing for a few exponent/unit
>    combinations (related to seconds and bytes)
>
> This patchset adds QEMU support for querying fd-based KVM stats. The
> kernel support was introduced by:
>
> cb082bfab59a ("KVM: stats: Add fd-based API to read binary stats data")

Hi Mark,

sorry for the late review.  Fortunately there's very little that I'd change.

In particular:

* please change the callbacks to accept a NULL name and type, instead of 
having the "bool"/"const char *" pair.  You can probably benefit from a 
function to cutils.c like

    bool qemu_match_string(const char *value, const char *request) {
        return !request || g_str_equal(value, request);
    }

* please pass a single const struct to add_stats_callbacks, using GList 
so that the struct can be const.

Putting both together it would be something like:

typedef struct StatsCallbacks {
    char *name;
    StatsList *(*get_values)(StatsList *list, const char *name,
                           const char *type, Error **errp);
    StatsSchemaList *(*get_schemas)(StatsSchemaList *list,
                                    const char *name, Error **errp);
    StatsInstanceList *(*get_instances)(StatsInstanceList *list,
                                        Error **errp);
} StatsCallbacks;

Finally, please put everything in a new header include/monitor/stats.h, 
so that we can document everything and please it in docs/devel.  I can 
take care of that though.

Thanks,

Paolo

> Mark Kanda (3):
>    qmp: Support for querying stats
>    hmp: Support for querying stats
>    kvm: Support for querying fd-based stats
>
>   accel/kvm/kvm-all.c       | 399 ++++++++++++++++++++++++++++++++++++++
>   hmp-commands-info.hx      |  40 ++++
>   include/monitor/hmp.h     |   3 +
>   include/monitor/monitor.h |  27 +++
>   monitor/hmp-cmds.c        | 125 ++++++++++++
>   monitor/qmp-cmds.c        |  71 +++++++
>   qapi/misc.json            | 142 ++++++++++++++
>   7 files changed, 807 insertions(+)