[libvirt] [PATCH 2/2] qemu: allow getting < max typed parameters

Daniel P. Berrange berrange at redhat.com
Wed Nov 2 10:17:41 UTC 2011 

On Mon, Oct 31, 2011 at 05:33:24PM -0600, Eric Blake wrote:
> Since all virTypedParameter APIs allow us to return the number
> of slots we actually populated, we should allow the user to
> call with nparams too small (without overrunning their array)
> or too large (ignoring the tail of the array that we can't fill),
> rather than requiring that they get things exactly right.
> 
> * src/qemu/qemu_driver.c (qemuDomainGetBlkioParameters)
> (qemuDomainGetMemoryParameters): Allow variable nparams on entry.
> (qemuGetSchedulerParametersFlags): Drop redundant check.
> (qemudDomainBlockStats, qemudDomainBlockStatsFlags): Rename...
> (qemuDomainBlockStats, qemuDomainBlockStatsFlags): ...to this.
> Don't return unavailable stats.
> ---
> 
> Making this change will make it easier to introduce VIR_TYPED_PARAM_STRING,
> with libvirt.c doing the filtering to further strip off string arguments,
> rather than having to teach every driver to honor a new flag.

There's a compile problem with this patch

  CC     libvirt_driver_qemu_la-qemu_driver.lo
qemu/qemu_driver.c: In function 'qemuDomainBlockStatsFlags':
qemu/qemu_driver.c:7325:24: error: 'param' may be used uninitialized in this function [-Werror=uninitialized]
cc1: all warnings being treated as errors

> @@ -7221,97 +7208,111 @@ qemudDomainBlockStatsFlags (virDomainPtr dom,
>      if (ret < 0)
>          goto endjob;
> 
> -    /* Field 'errs' is meaningless for QEMU, won't set it. */
> -    for (i = 0; i < *nparams; i++) {
> -        virTypedParameterPtr param = &params[i];
> -
> -        switch (i) {
> -        case 0: /* fill write_bytes here */
> -            if (virStrcpyStatic(param->field, VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES) == NULL) {
> -                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("Field write bytes too long for destination"));
> -                goto endjob;
> -            }
> -            param->type = VIR_TYPED_PARAM_LLONG;
> -            param->value.l = wr_bytes;
> -            break;
> +    tmp = 0;
> +    ret = -1;
> 
> -        case 1: /* fill wr_operations here */
> -            if (virStrcpyStatic(param->field, VIR_DOMAIN_BLOCK_STATS_WRITE_REQ) == NULL) {
> -                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("Field write requests too long for destination"));
> -                goto endjob;
> -            }
> -            param->type = VIR_TYPED_PARAM_LLONG;
> -            param->value.l = wr_req;
> -            break;
> +    if (tmp < *nparams && wr_bytes != -1) {
> +        param = &params[tmp];
> +        if (virStrcpyStatic(param->field,
> +                            VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES) == NULL) {
> +            qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                            _("Field write bytes too long for destination"));
> +            goto endjob;
> +        }
> +        param->type = VIR_TYPED_PARAM_LLONG;
> +        param->value.l = wr_bytes;
> +        tmp++;
> +    }
> 
> -        case 2: /* fill read_bytes here */
> -            if (virStrcpyStatic(param->field, VIR_DOMAIN_BLOCK_STATS_READ_BYTES) == NULL) {
> -                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("Field read bytes too long for destination"));
> -                goto endjob;
> -            }
> -            param->type = VIR_TYPED_PARAM_LLONG;
> -            param->value.l = rd_bytes;
> -            break;
> +    if (tmp < *nparams && wr_req != -1) {
> +        if (virStrcpyStatic(param->field,
> +                            VIR_DOMAIN_BLOCK_STATS_WRITE_REQ) == NULL) {
> +            qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                            _("Field write requests too long for destination"));
> +            goto endjob;
> +        }
> +        param->type = VIR_TYPED_PARAM_LLONG;
> +        param->value.l = wr_req;
> +        tmp++;
> +    }
> 
> -        case 3: /* fill rd_operations here */
> -            if (virStrcpyStatic(param->field, VIR_DOMAIN_BLOCK_STATS_READ_REQ) == NULL) {
> -                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("Field read requests too long for destination"));
> -                goto endjob;
> -            }
> -            param->type = VIR_TYPED_PARAM_LLONG;
> -            param->value.l = rd_req;
> -            break;
> +    if (tmp < *nparams && rd_bytes != -1) {
> +        if (virStrcpyStatic(param->field,
> +                            VIR_DOMAIN_BLOCK_STATS_READ_BYTES) == NULL) {
> +            qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                            _("Field read bytes too long for destination"));
> +            goto endjob;
> +        }
> +        param->type = VIR_TYPED_PARAM_LLONG;
> +        param->value.l = rd_bytes;
> +        tmp++;
> +    }
> 
> -        case 4: /* fill flush_operations here */
> -            if (virStrcpyStatic(param->field, VIR_DOMAIN_BLOCK_STATS_FLUSH_REQ) == NULL) {
> -                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("Field flush requests too long for destination"));
> -                goto endjob;
> -            }
> -            param->type = VIR_TYPED_PARAM_LLONG;
> -            param->value.l = flush_req;
> -            break;
> +    if (tmp < *nparams && rd_req != -1) {
> +        if (virStrcpyStatic(param->field,
> +                            VIR_DOMAIN_BLOCK_STATS_READ_REQ) == NULL) {
> +            qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                            _("Field read requests too long for destination"));
> +            goto endjob;
> +        }
> +        param->type = VIR_TYPED_PARAM_LLONG;
> +        param->value.l = rd_req;
> +        tmp++;
> +    }
> 
> -        case 5: /* fill wr_total_times_ns here */
> -            if (virStrcpyStatic(param->field, VIR_DOMAIN_BLOCK_STATS_WRITE_TOTAL_TIMES) == NULL) {
> -                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("Field write total times too long for destination"));
> -                goto endjob;
> -            }
> -            param->type = VIR_TYPED_PARAM_LLONG;
> -            param->value.l = wr_total_times;
> -            break;
> +    if (tmp < *nparams && flush_req != -1) {
> +        if (virStrcpyStatic(param->field,
> +                            VIR_DOMAIN_BLOCK_STATS_FLUSH_REQ) == NULL) {
> +            qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                            _("Field flush requests too long for destination"));
> +            goto endjob;
> +        }
> +        param->type = VIR_TYPED_PARAM_LLONG;
> +        param->value.l = flush_req;
> +        tmp++;
> +    }
> 
> -        case 6: /* fill rd_total_times_ns here */
> -            if (virStrcpyStatic(param->field, VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES) == NULL) {
> -                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("Field read total times too long for destination"));
> -                goto endjob;
> -            }
> -            param->type = VIR_TYPED_PARAM_LLONG;
> -            param->value.l = rd_total_times;
> -            break;
> +    if (tmp < *nparams && wr_total_times != -1) {
> +        if (virStrcpyStatic(param->field,
> +                            VIR_DOMAIN_BLOCK_STATS_WRITE_TOTAL_TIMES) == NULL) {
> +            qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                            _("Field write total times too long for destination"));
> +            goto endjob;
> +        }
> +        param->type = VIR_TYPED_PARAM_LLONG;
> +        param->value.l = wr_total_times;
> +        tmp++;
> +    }
> 
> -        case 7: /* fill flush_total_times_ns here */
> -            if (virStrcpyStatic(param->field, VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES) == NULL) {
> -                qemuReportError(VIR_ERR_INTERNAL_ERROR,
> -                                "%s", _("Field flush total times too long for destination"));
> -                goto endjob;
> -            }
> -            param->type = VIR_TYPED_PARAM_LLONG;
> -            param->value.l = flush_total_times;
> -            break;
> +    if (tmp < *nparams && rd_total_times != -1) {
> +        if (virStrcpyStatic(param->field,
> +                            VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES) == NULL) {
> +            qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                            _("Field read total times too long for destination"));
> +            goto endjob;
> +        }
> +        param->type = VIR_TYPED_PARAM_LLONG;
> +        param->value.l = rd_total_times;
> +        tmp++;
> +    }
> 
> -        default:
> -            break;
> -            /* should not hit here */
> +    if (tmp < *nparams && flush_total_times != -1) {
> +        if (virStrcpyStatic(param->field,
> +                            VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES) == NULL) {
> +            qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                            _("Field flush total times too long for destination"));
> +            goto endjob;
>          }
> +        param->type = VIR_TYPED_PARAM_LLONG;
> +        param->value.l = flush_total_times;
> +        tmp++;
>      }
> 
> +    /* Field 'errs' is meaningless for QEMU, won't set it. */
> +
> +    ret = 0;
> +    *nparams = tmp;
> +
>  endjob:
>      if (qemuDomainObjEndJob(driver, vm) == 0)
>          vm = NULL;

It is easier to see without the diff, only the first if {}  block
initializes 'param':

    if (tmp < *nparams && wr_bytes != -1) {
        param = &params[tmp];
        if (virStrcpyStatic(param->field,
                            VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES) == NULL) {
            qemuReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Field name '%s' too long"),
                            VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES);
            goto endjob;
        }
        param->type = VIR_TYPED_PARAM_LLONG;
        param->value.l = wr_bytes;
        tmp++;
    }

    if (tmp < *nparams && wr_req != -1) {
        if (virStrcpyStatic(param->field,
                            VIR_DOMAIN_BLOCK_STATS_WRITE_REQ) == NULL) {
            qemuReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Field name '%s' too long"),
                            VIR_DOMAIN_BLOCK_STATS_WRITE_REQ);
            goto endjob;
        }
        param->type = VIR_TYPED_PARAM_LLONG;
        param->value.l = wr_req;
        tmp++;
    }

    if (tmp < *nparams && rd_bytes != -1) {
        if (virStrcpyStatic(param->field,
                            VIR_DOMAIN_BLOCK_STATS_READ_BYTES) == NULL) {
            qemuReportError(VIR_ERR_INTERNAL_ERROR,
                            _("Field name '%s' too long"),
                            VIR_DOMAIN_BLOCK_STATS_READ_BYTES);
            goto endjob;
        }
        param->type = VIR_TYPED_PARAM_LLONG;
        param->value.l = rd_bytes;
        tmp++;
    }

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

More information about the libvir-list mailing list