Re: [PATCH 3/4] qemu_driver: add check for qemu capabilities requirements

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

On 10/15/21 11:49 AM, Hiroki Narukawa wrote:
> query-dirty-rate command is used for virsh domstats by default, but this
> is available only on qemu >=5.2.0.
> 
> By this commit, qemu domain stats will check capabilities requirements before issuing actual query.
> 
> Signed-off-by: Hiroki Narukawa <hnarukaw@xxxxxxxxxxxxx>
> ---
>  src/qemu/qemu_driver.c | 40 +++++++++++++++++++++++++++++-----------
>  1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ac5eaf139e..9cfd0a6ca5 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -18714,13 +18714,28 @@ static struct qemuDomainGetStatsWorker qemuDomainGetStatsWorkers[] = {
>  
>  static int
>  qemuDomainGetStatsCheckSupport(unsigned int *stats,
> -                               bool enforce)
> +                               bool enforce,
> +                               virDomainObj *vm)
>  {
> +    qemuDomainObjPrivate *priv = vm->privateData;
>      unsigned int supportedstats = 0;
>      size_t i;
> +    virQEMUCapsFlags* flagCursor;

We like to declare variables inside loops when possible. A variable can
be declared outside if it's immutable (i.e. its value isn't changed
inside loop). So @priv can stay, but @flagCursor should go inside the
for() loop below.

>  
> -    for (i = 0; qemuDomainGetStatsWorkers[i].func; i++)
> -        supportedstats |= qemuDomainGetStatsWorkers[i].stats;
> +    for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) {
> +        bool supportedByQemu = true;
> +
> +        for (flagCursor = qemuDomainGetStatsWorkers[i].requiredCaps; *flagCursor != QEMU_CAPS_LAST;
> +             ++flagCursor) {
> +            if (!virQEMUCapsGet(priv->qemuCaps, *flagCursor)) {
> +                supportedByQemu = false;
> +                break;
> +            }
> +        }

This body will look slightly different if we allow NULL. I suggest:

    for (i = 0; qemuDomainGetStatsWorkers[i].func; i++) {
        bool supportedByQemu = true;
        virQEMUCapsFlags *requiredCaps = qemuDomainGetStatsWorkers[i].requiredCaps;

        while (requiredCaps && *requiredCaps != QEMU_CAPS_LAST) {
            if (!virQEMUCapsGet(priv->qemuCaps, *requiredCaps)) {
                supportedByQemu = false;
                break;
            }

            requiredCaps++;
        }

        if (supportedByQemu) {
            supportedstats |= qemuDomainGetStatsWorkers[i].stats;
        }
    }


> +        if (supportedByQemu) {
> +            supportedstats |= qemuDomainGetStatsWorkers[i].stats;
> +        }
> +    }
>  
>      if (*stats == 0) {
>          *stats = supportedstats;

Later in this function there's an error message that deserves to be
updated:

        virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED,
                       _("Stats types bits 0x%x are not supported by this daemon or QEMU"),
                       *stats & ~supportedstats);


> @@ -18791,7 +18806,7 @@ static int
>  qemuConnectGetAllDomainStats(virConnectPtr conn,
>                               virDomainPtr *doms,
>                               unsigned int ndoms,
> -                             unsigned int stats,
> +                             unsigned int requestedStats,

I'd rather not rename this variable (so that its name is the same as in
the public API) and introduce new variable later.

>                               virDomainStatsRecordPtr **retStats,
>                               unsigned int flags)
>  {
> @@ -18805,11 +18820,12 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
>      int nstats = 0;
>      size_t i;
>      int ret = -1;
> -    unsigned int privflags = 0;
> +    unsigned int privflags;
>      unsigned int domflags = 0;
>      unsigned int lflags = flags & (VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
>                                     VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
>                                     VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE);
> +    unsigned int stats;

Again, should be moved into the big for() loop below.

>  
>      virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
>                    VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
> @@ -18821,9 +18837,6 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
>      if (virConnectGetAllDomainStatsEnsureACL(conn) < 0)
>          return -1;
>  
> -    if (qemuDomainGetStatsCheckSupport(&stats, enforce) < 0)
> -        return -1;
> -
>      if (ndoms) {
>          if (virDomainObjListConvert(driver->domains, conn, doms, ndoms, &vms,
>                                      &nvms, virConnectGetAllDomainStatsCheckACL,
> @@ -18838,14 +18851,19 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
>  
>      tmpstats = g_new0(virDomainStatsRecordPtr, nvms + 1);
>  
> -    if (qemuDomainGetStatsNeedMonitor(stats))
> -        privflags |= QEMU_DOMAIN_STATS_HAVE_JOB;
> -
>      for (i = 0; i < nvms; i++) {
>          virDomainStatsRecordPtr tmp = NULL;
>          domflags = 0;
> +        privflags = 0;
>          vm = vms[i];
>  
> +        stats = requestedStats;
> +        if (qemuDomainGetStatsCheckSupport(&stats, enforce, vm) < 0)
> +            return -1;
> +
> +        if (qemuDomainGetStatsNeedMonitor(stats))
> +            privflags |= QEMU_DOMAIN_STATS_HAVE_JOB;
> +

How about the following instead?

    for (i = 0; i < nvms; i++) {
        virDomainStatsRecordPtr tmp = NULL;
        unsigned int privflags = 0;
        unsigned int requestedStats = stats;

        domflags = 0;
        vm = vms[i];

        if (qemuDomainGetStatsCheckSupport(&requestedStats, enforce, vm) < 0)
            return -1;

        if (qemuDomainGetStatsNeedMonitor(requestedStats))
            privflags |= QEMU_DOMAIN_STATS_HAVE_JOB;


There's one more line where @stats is used (when calling
qemuDomainGetStats()) and that would also need similar treatment:

        if (qemuDomainGetStats(conn, vm, requestedStats, &tmp, domflags) < 0) {



Michal