[libvirt] [PATCH 1/2] docs: improve typed parameter documentation
virDomainBlockStatsFlags was missing a check that was present in virDomainGetMemoryParameters. Additionally, I found that the existing descriptions were a bit hard to read. A later patch will fix qemu to return fewer than max parameters if @nparams was too small on input, rather than outright fail. * src/libvirt.c (virDomainGetMemoryParameters) (virDomainGetBlkioParameters, virDomainGetSchedulerParameters) (virDomainGetSchedulerParametersFlags): Tweak documentation wording. (virDomainBlockStatsFlags): Likewise, and add sanity check. --- src/libvirt.c | 100 +++++++++++++++++++++++++++++++++++++-------------------- 1 files changed, 65 insertions(+), 35 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index e9d1a29..f9cddef 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3639,15 +3639,17 @@ error: * @domain: pointer to domain object * @params: pointer to memory parameter object * (return value, allocated by the caller) - * @nparams: pointer to number of memory parameters + * @nparams: pointer to number of memory parameters; input and output * @flags: one of virDomainModificationImpact * - * Get all memory parameters, the @params array will be filled with the values - * equal to the number of parameters suggested by @nparams + * Get all memory parameters. On input, @nparams gives the size of the + * @params array; on output, @nparams gives how many slots were filled + * with parameter information, which might be less but will not exceed + * the input value. * - * As the value of @nparams is dynamic, call the API setting @nparams to 0 and - * @params as NULL, the API returns the number of parameters supported by the - * HV by updating @nparams on SUCCESS. The caller should then allocate @params + * As a special case, calling with @params as NULL and @nparams as 0 on + * input will cause @nparams on output to contain the number of parameters + * supported by the hypervisor. The caller should then allocate @params * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API * again. * @@ -3765,12 +3767,21 @@ error: * @domain: pointer to domain object * @params: pointer to blkio parameter object * (return value, allocated by the caller) - * @nparams: pointer to number of blkio parameters + * @nparams: pointer to number of blkio parameters; input and output * @flags: an OR'ed set of virDomainModificationImpact * - * Get all blkio parameters, the @params array will be filled with the values - * equal to the number of parameters suggested by @nparams. - * See virDomainGetMemoryParameters for an equivalent usage example. + * Get all blkio parameters. On input, @nparams gives the size of the + * @params array; on output, @nparams gives how many slots were filled + * with parameter information, which might be less but will not exceed + * the input value. + * + * As a special case, calling with @params as NULL and @nparams as 0 on + * input will cause @nparams on output to contain the number of parameters + * supported by the hypervisor. The caller should then allocate @params + * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API + * again. + * + * See virDomainGetMemoryParameters() for an equivalent usage example. * * This function may require privileged access to the hypervisor. This function * expects the caller to allocate the @params. @@ -6335,14 +6346,17 @@ error: * @params: pointer to scheduler parameter objects * (return value) * @nparams: pointer to number of scheduler parameter objects - * (this value must be at least as large as the returned value - * nparams of virDomainGetSchedulerType) + * (this value should generally be as large as the returned value + * nparams of virDomainGetSchedulerType()); input and output + * + * Get all scheduler parameters. On input, @nparams gives the size of the + * @params array; on output, @nparams gives how many slots were filled + * with parameter information, which might be less but will not exceed + * the input value. @nparams cannot be 0. * - * Get all scheduler parameters, the @params array will be filled with the - * values and @nparams will be updated to the number of valid elements in - * @params. It is hypervisor specific whether this returns the live or + * It is hypervisor specific whether this returns the live or * persistent state; for more control, use - * virDomainGetSchedulerParametersFlags. + * virDomainGetSchedulerParametersFlags(). * * Returns -1 in case of error, 0 in case of success. */ @@ -6391,15 +6405,28 @@ error: * (return value) * @nparams: pointer to number of scheduler parameter * (this value should be same than the returned value - * nparams of virDomainGetSchedulerType) + * nparams of virDomainGetSchedulerType()); input and output * @flags: one of virDomainModificationImpact * - * Get the scheduler parameters, the @params array will be filled with the - * values. + * Get all scheduler parameters. On input, @nparams gives the size of the + * @params array; on output, @nparams gives how many slots were filled + * with parameter information, which might be less but will not exceed + * the input value. @nparams cannot be 0. * * The value of @flags can be exactly VIR_DOMAIN_AFFECT_CURRENT, * VIR_DOMAIN_AFFECT_LIVE, or VIR_DOMAIN_AFFECT_CONFIG. * + * Here is a sample code snippet: + * + * char *ret = virDomainGetSchedulerType(dom, &nparams); + * if (ret && nparams != 0) { + * if ((params = malloc(sizeof(*params) * nparams)) == NULL) + * goto error; + * memset(params, 0, sizeof(*params) * nparams); + * if (virDomainGetSchedulerParametersFlags(dom, params, &nparams, 0)) + * goto error; + * } + * * Returns -1 in case of error, 0 in case of success. */ int @@ -6633,8 +6660,8 @@ error: * @path: path to the block device * @params: pointer to block stats parameter object * (return value) - * @nparams: pointer to number of block stats - * @flags: unused, always passes 0 + * @nparams: pointer to number of block stats; input and output + * @flags: unused, always pass 0 * * This function is to get block stats parameters for block * devices attached to the domain. @@ -6646,24 +6673,26 @@ error: * Domains may have more than one block device. To get stats for * each you should make multiple calls to this function. * - * The @params array will be filled with the value equal to the number of - * parameters suggested by @nparams. + * On input, @nparams gives the size of the @params array; on output, + * @nparams gives how many slots were filled with parameter + * information, which might be less but will not exceed the input + * value. * - * As the value of @nparams is dynamic, call the API setting @nparams to 0 and - * @params as NULL, the API returns the number of parameters supported by the - * HV by updating @nparams on SUCCESS. (Note that block device of different type - * might support different parameters numbers, so it might be necessary to compute - * @nparams for each block device type). The caller should then allocate @params + * As a special case, calling with @params as NULL and @nparams as 0 on + * input will cause @nparams on output to contain the number of parameters + * supported by the hypervisor. (Note that block devices of different types + * might support different parameters, so it might be necessary to compute + * @nparams for each block device). The caller should then allocate @params * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API - * again. See virDomainGetMemoryParameters for more details. + * again. See virDomainGetMemoryParameters() for more details. * * Returns -1 in case of error, 0 in case of success. */ -int virDomainBlockStatsFlags (virDomainPtr dom, - const char *path, - virTypedParameterPtr params, - int *nparams, - unsigned int flags) +int virDomainBlockStatsFlags(virDomainPtr dom, + const char *path, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) { virConnectPtr conn; @@ -6677,7 +6706,8 @@ int virDomainBlockStatsFlags (virDomainPtr dom, virDispatchError(NULL); return -1; } - if (!path || (nparams == NULL) || (*nparams < 0)) { + if (!path || (nparams == NULL) || (*nparams < 0) || + (params == NULL && *nparams != 0)) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } -- 1.7.4.4
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. src/qemu/qemu_driver.c | 231 ++++++++++++++++++++++++------------------------ 1 files changed, 116 insertions(+), 115 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 02cbf2d..d70ab7a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6059,12 +6059,6 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; } - if ((*nparams) != QEMU_NB_BLKIO_PARAM) { - qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); - goto cleanup; - } - isActive = virDomainObjIsActive(vm); if (flags == VIR_DOMAIN_AFFECT_CURRENT) { @@ -6104,7 +6098,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - for (i = 0; i < *nparams; i++) { + for (i = 0; i < *nparams && i < QEMU_NB_BLKIO_PARAM; i++) { virTypedParameterPtr param = ¶ms[i]; val = 0; param->value.ui = 0; @@ -6132,7 +6126,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } } } else if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - for (i = 0; i < *nparams; i++) { + for (i = 0; i < *nparams && i < QEMU_NB_BLKIO_PARAM; i++) { virTypedParameterPtr param = ¶ms[i]; val = 0; param->value.ui = 0; @@ -6155,6 +6149,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } } + if (QEMU_NB_BLKIO_PARAM < *nparams) + *nparams = QEMU_NB_BLKIO_PARAM; ret = 0; cleanup: @@ -6395,14 +6391,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; } - if ((*nparams) < QEMU_NB_MEM_PARAM) { - qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); - goto cleanup; - } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - for (i = 0; i < *nparams; i++) { + for (i = 0; i < *nparams && i < QEMU_NB_MEM_PARAM; i++) { virMemoryParameterPtr param = ¶ms[i]; val = 0; param->value.ul = 0; @@ -6444,7 +6434,7 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, goto out; } - for (i = 0; i < QEMU_NB_MEM_PARAM; i++) { + for (i = 0; i < *nparams && i < QEMU_NB_MEM_PARAM; i++) { virTypedParameterPtr param = ¶ms[i]; val = 0; param->value.ul = 0; @@ -6506,7 +6496,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, } out: - *nparams = QEMU_NB_MEM_PARAM; + if (QEMU_NB_MEM_PARAM < *nparams) + *nparams = QEMU_NB_MEM_PARAM; ret = 0; cleanup: @@ -6894,12 +6885,6 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; } - if (*nparams < 1) { - qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); - goto cleanup; - } - if (*nparams > 1) { rc = qemuGetCpuBWStatus(driver->cgroup); if (rc < 0) @@ -7048,9 +7033,9 @@ qemuGetSchedulerParameters(virDomainPtr dom, * not supported we detect this and return the appropriate error. */ static int -qemudDomainBlockStats (virDomainPtr dom, - const char *path, - struct _virDomainBlockStats *stats) +qemuDomainBlockStats(virDomainPtr dom, + const char *path, + struct _virDomainBlockStats *stats) { struct qemud_driver *driver = dom->conn->privateData; int i, ret = -1; @@ -7129,11 +7114,11 @@ cleanup: } static int -qemudDomainBlockStatsFlags (virDomainPtr dom, - const char *path, - virTypedParameterPtr params, - int *nparams, - unsigned int flags) +qemuDomainBlockStatsFlags(virDomainPtr dom, + const char *path, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; int i, tmp, ret = -1; @@ -7142,6 +7127,7 @@ qemudDomainBlockStatsFlags (virDomainPtr dom, qemuDomainObjPrivatePtr priv; long long rd_req, rd_bytes, wr_req, wr_bytes, rd_total_times; long long wr_total_times, flush_req, flush_total_times, errs; + virTypedParameterPtr param; virCheckFlags(0, -1); @@ -7178,7 +7164,8 @@ qemudDomainBlockStatsFlags (virDomainPtr dom, if (!disk->info.alias) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("missing disk device alias name for %s"), disk->dst); + _("missing disk device alias name for %s"), + disk->dst); goto cleanup; } } @@ -7199,7 +7186,7 @@ qemudDomainBlockStatsFlags (virDomainPtr dom, tmp = *nparams; ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams); - if (tmp == 0) { + if (tmp == 0 || ret < 0) { qemuDomainObjExitMonitor(driver, vm); goto endjob; } @@ -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 = ¶ms[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 = ¶ms[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; @@ -10811,8 +10812,8 @@ static virDriver qemuDriver = { .domainSetSchedulerParameters = qemuSetSchedulerParameters, /* 0.7.0 */ .domainSetSchedulerParametersFlags = qemuSetSchedulerParametersFlags, /* 0.9.2 */ .domainMigratePerform = qemudDomainMigratePerform, /* 0.5.0 */ - .domainBlockStats = qemudDomainBlockStats, /* 0.4.1 */ - .domainBlockStatsFlags = qemudDomainBlockStatsFlags, /* 0.9.5 */ + .domainBlockStats = qemuDomainBlockStats, /* 0.4.1 */ + .domainBlockStatsFlags = qemuDomainBlockStatsFlags, /* 0.9.5 */ .domainInterfaceStats = qemudDomainInterfaceStats, /* 0.4.1 */ .domainMemoryStats = qemudDomainMemoryStats, /* 0.7.5 */ .domainBlockPeek = qemudDomainBlockPeek, /* 0.4.4 */ -- 1.7.4.4
On 10/31/2011 07:33 PM, 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.
src/qemu/qemu_driver.c | 231 ++++++++++++++++++++++++------------------------ 1 files changed, 116 insertions(+), 115 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 02cbf2d..d70ab7a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6059,12 +6059,6 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; }
- if ((*nparams) != QEMU_NB_BLKIO_PARAM) { - qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); - goto cleanup; - } - isActive = virDomainObjIsActive(vm);
if (flags == VIR_DOMAIN_AFFECT_CURRENT) { @@ -6104,7 +6098,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, }
if (flags& VIR_DOMAIN_AFFECT_LIVE) { - for (i = 0; i< *nparams; i++) { + for (i = 0; i< *nparams&& i< QEMU_NB_BLKIO_PARAM; i++) { virTypedParameterPtr param =¶ms[i]; val = 0; param->value.ui = 0; @@ -6132,7 +6126,7 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } } } else if (flags& VIR_DOMAIN_AFFECT_CONFIG) { - for (i = 0; i< *nparams; i++) { + for (i = 0; i< *nparams&& i< QEMU_NB_BLKIO_PARAM; i++) { virTypedParameterPtr param =¶ms[i]; val = 0; param->value.ui = 0; @@ -6155,6 +6149,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } }
+ if (QEMU_NB_BLKIO_PARAM< *nparams) + *nparams = QEMU_NB_BLKIO_PARAM; ret = 0;
cleanup: @@ -6395,14 +6391,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; }
- if ((*nparams)< QEMU_NB_MEM_PARAM) { - qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); - goto cleanup; - } - if (flags& VIR_DOMAIN_AFFECT_CONFIG) { - for (i = 0; i< *nparams; i++) { + for (i = 0; i< *nparams&& i< QEMU_NB_MEM_PARAM; i++) { virMemoryParameterPtr param =¶ms[i]; val = 0; param->value.ul = 0; @@ -6444,7 +6434,7 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, goto out; }
- for (i = 0; i< QEMU_NB_MEM_PARAM; i++) { + for (i = 0; i< *nparams&& i< QEMU_NB_MEM_PARAM; i++) { virTypedParameterPtr param =¶ms[i]; val = 0; param->value.ul = 0; @@ -6506,7 +6496,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, }
out: - *nparams = QEMU_NB_MEM_PARAM; + if (QEMU_NB_MEM_PARAM< *nparams) + *nparams = QEMU_NB_MEM_PARAM; ret = 0;
cleanup: @@ -6894,12 +6885,6 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, goto cleanup; }
- if (*nparams< 1) { - qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); - goto cleanup; - } - if (*nparams> 1) { rc = qemuGetCpuBWStatus(driver->cgroup); if (rc< 0) @@ -7048,9 +7033,9 @@ qemuGetSchedulerParameters(virDomainPtr dom, * not supported we detect this and return the appropriate error. */ static int -qemudDomainBlockStats (virDomainPtr dom, - const char *path, - struct _virDomainBlockStats *stats) +qemuDomainBlockStats(virDomainPtr dom, + const char *path, + struct _virDomainBlockStats *stats) { struct qemud_driver *driver = dom->conn->privateData; int i, ret = -1; @@ -7129,11 +7114,11 @@ cleanup: }
static int -qemudDomainBlockStatsFlags (virDomainPtr dom, - const char *path, - virTypedParameterPtr params, - int *nparams, - unsigned int flags) +qemuDomainBlockStatsFlags(virDomainPtr dom, + const char *path, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; int i, tmp, ret = -1; @@ -7142,6 +7127,7 @@ qemudDomainBlockStatsFlags (virDomainPtr dom, qemuDomainObjPrivatePtr priv; long long rd_req, rd_bytes, wr_req, wr_bytes, rd_total_times; long long wr_total_times, flush_req, flush_total_times, errs; + virTypedParameterPtr param;
virCheckFlags(0, -1);
@@ -7178,7 +7164,8 @@ qemudDomainBlockStatsFlags (virDomainPtr dom,
if (!disk->info.alias) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("missing disk device alias name for %s"), disk->dst); + _("missing disk device alias name for %s"), + disk->dst); goto cleanup; } } @@ -7199,7 +7186,7 @@ qemudDomainBlockStatsFlags (virDomainPtr dom, tmp = *nparams; ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams);
- if (tmp == 0) { + if (tmp == 0 || ret< 0) { qemuDomainObjExitMonitor(driver, vm); goto endjob; } @@ -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 =¶ms[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 =¶ms[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")); Existing but odd error message... "Field 'write bytes' is too long for ..." (?)
+ 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; @@ -10811,8 +10812,8 @@ static virDriver qemuDriver = { .domainSetSchedulerParameters = qemuSetSchedulerParameters, /* 0.7.0 */ .domainSetSchedulerParametersFlags = qemuSetSchedulerParametersFlags, /* 0.9.2 */ .domainMigratePerform = qemudDomainMigratePerform, /* 0.5.0 */ - .domainBlockStats = qemudDomainBlockStats, /* 0.4.1 */ - .domainBlockStatsFlags = qemudDomainBlockStatsFlags, /* 0.9.5 */ + .domainBlockStats = qemuDomainBlockStats, /* 0.4.1 */ + .domainBlockStatsFlags = qemuDomainBlockStatsFlags, /* 0.9.5 */ .domainInterfaceStats = qemudDomainInterfaceStats, /* 0.4.1 */ .domainMemoryStats = qemudDomainMemoryStats, /* 0.7.5 */ .domainBlockPeek = qemudDomainBlockPeek, /* 0.4.4 */ ACK
On 11/01/2011 04:47 AM, Stefan Berger wrote:
On 10/31/2011 07:33 PM, 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.
+ if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Field write bytes too long for destination")); Existing but odd error message... "Field 'write bytes' is too long for ..." (?)
Yeah, that helps.
ACK
Pushed the series with this squashed in. I'm also working a followup for the XXX comment added below: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index d70ab7a..f81cb88 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -6114,7 +6114,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } if (virStrcpyStatic(param->field, VIR_DOMAIN_BLKIO_WEIGHT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field blkio weight too long for destination")); + _("Field name '%s' too long"), + VIR_DOMAIN_BLKIO_WEIGHT); goto cleanup; } param->value.ui = val; @@ -6136,7 +6137,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, case 0: /* fill blkio weight here */ if (virStrcpyStatic(param->field, VIR_DOMAIN_BLKIO_WEIGHT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field blkio weight too long for destination")); + _("Field name '%s' too long"), + VIR_DOMAIN_BLKIO_WEIGHT); goto cleanup; } param->value.ui = persistentDef->blkio.weight; @@ -6402,7 +6404,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, case 0: /* fill memory hard limit here */ if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_HARD_LIMIT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field memory hard limit too long for destination")); + _("Field name '%s' too long"), + VIR_DOMAIN_MEMORY_HARD_LIMIT); goto cleanup; } param->value.ul = persistentDef->mem.hard_limit; @@ -6411,7 +6414,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, case 1: /* fill memory soft limit here */ if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_SOFT_LIMIT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field memory soft limit too long for destination")); + _("Field name '%s' too long"), + VIR_DOMAIN_MEMORY_SOFT_LIMIT); goto cleanup; } param->value.ul = persistentDef->mem.soft_limit; @@ -6420,7 +6424,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, case 2: /* fill swap hard limit here */ if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field swap hard limit too long for destination")); + _("Field name '%s' too long"), + VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT); goto cleanup; } param->value.ul = persistentDef->mem.swap_hard_limit; @@ -6453,7 +6458,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, } if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_HARD_LIMIT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field memory hard limit too long for destination")); + _("Field name '%s' too long"), + VIR_DOMAIN_MEMORY_HARD_LIMIT); goto cleanup; } param->value.ul = val; @@ -6468,7 +6474,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, } if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_SOFT_LIMIT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field memory soft limit too long for destination")); + _("Field name '%s' too long"), + VIR_DOMAIN_MEMORY_SOFT_LIMIT); goto cleanup; } param->value.ul = val; @@ -6483,7 +6490,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, } if (virStrcpyStatic(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT) == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field swap hard limit too long for destination")); + _("Field name '%s' too long"), + VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT); goto cleanup; } param->value.ul = val; @@ -6973,9 +6981,11 @@ qemuGetSchedulerParametersFlags(virDomainPtr dom, out: params[0].value.ul = shares; params[0].type = VIR_TYPED_PARAM_ULLONG; + /* XXX make these field names public in libvirt.h */ if (virStrcpyStatic(params[0].field, "cpu_shares") == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field cpu_shares too long for destination")); + _("Field name '%s' too long"), + "cpu_shares"); goto cleanup; } @@ -6987,8 +6997,8 @@ out: params[1].type = VIR_TYPED_PARAM_ULLONG; if (virStrcpyStatic(params[1].field, "vcpu_period") == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", - _("Field vcpu_period too long for destination")); + _("Field name '%s' too long"), + "vcpu_period"); goto cleanup; } saved_nparams++; @@ -6999,8 +7009,8 @@ out: params[2].type = VIR_TYPED_PARAM_LLONG; if (virStrcpyStatic(params[2].field, "vcpu_quota") == NULL) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", - _("Field vcpu_quota too long for destination")); + _("Field name '%s' too long"), + "vcpu_quota"); goto cleanup; } saved_nparams++; @@ -7215,8 +7225,9 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, param = ¶ms[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")); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_STATS_WRITE_BYTES); goto endjob; } param->type = VIR_TYPED_PARAM_LLONG; @@ -7227,8 +7238,9 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, 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")); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_STATS_WRITE_REQ); goto endjob; } param->type = VIR_TYPED_PARAM_LLONG; @@ -7239,8 +7251,9 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, 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")); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_STATS_READ_BYTES); goto endjob; } param->type = VIR_TYPED_PARAM_LLONG; @@ -7251,8 +7264,9 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, 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")); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_STATS_READ_REQ); goto endjob; } param->type = VIR_TYPED_PARAM_LLONG; @@ -7263,8 +7277,9 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, 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")); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_STATS_FLUSH_REQ); goto endjob; } param->type = VIR_TYPED_PARAM_LLONG; @@ -7275,8 +7290,9 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, 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")); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_STATS_WRITE_TOTAL_TIMES); goto endjob; } param->type = VIR_TYPED_PARAM_LLONG; @@ -7287,8 +7303,9 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, 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")); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES); goto endjob; } param->type = VIR_TYPED_PARAM_LLONG; @@ -7299,8 +7316,9 @@ qemuDomainBlockStatsFlags(virDomainPtr dom, 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")); + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_STATS_READ_TOTAL_TIMES); goto endjob; } param->type = VIR_TYPED_PARAM_LLONG; -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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 = ¶ms[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 = ¶ms[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 = ¶ms[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 :|
On 11/02/2011 04:17 AM, Daniel P. Berrange wrote:
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
Wonder why I didn't see it in my environment? F14 gcc 4.5.1; maybe you were using newer gcc? At any rate, thanks for fixing up my mess. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
2011/11/2 Eric Blake <eblake@redhat.com>:
On 11/02/2011 04:17 AM, Daniel P. Berrange wrote:
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
Wonder why I didn't see it in my environment? F14 gcc 4.5.1; maybe you were using newer gcc? At any rate, thanks for fixing up my mess.
Are you compiling with -O0? Because gcc reports/detects some uninitialized warnings only with -O2, while it doesn't warn with -O0. -- Matthias Bolte http://photron.blogspot.com
On 10/31/2011 07:33 PM, Eric Blake wrote:
virDomainBlockStatsFlags was missing a check that was present in virDomainGetMemoryParameters. Additionally, I found that the existing descriptions were a bit hard to read. A later patch will fix qemu to return fewer than max parameters if @nparams was too small on input, rather than outright fail.
* src/libvirt.c (virDomainGetMemoryParameters) (virDomainGetBlkioParameters, virDomainGetSchedulerParameters) (virDomainGetSchedulerParametersFlags): Tweak documentation wording. (virDomainBlockStatsFlags): Likewise, and add sanity check. --- src/libvirt.c | 100 +++++++++++++++++++++++++++++++++++++-------------------- 1 files changed, 65 insertions(+), 35 deletions(-)
diff --git a/src/libvirt.c b/src/libvirt.c index e9d1a29..f9cddef 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3639,15 +3639,17 @@ error: * @domain: pointer to domain object * @params: pointer to memory parameter object * (return value, allocated by the caller) - * @nparams: pointer to number of memory parameters + * @nparams: pointer to number of memory parameters; input and output * @flags: one of virDomainModificationImpact * - * Get all memory parameters, the @params array will be filled with the values - * equal to the number of parameters suggested by @nparams + * Get all memory parameters. On input, @nparams gives the size of the + * @params array; on output, @nparams gives how many slots were filled + * with parameter information, which might be less but will not exceed + * the input value. * - * As the value of @nparams is dynamic, call the API setting @nparams to 0 and - * @params as NULL, the API returns the number of parameters supported by the - * HV by updating @nparams on SUCCESS. The caller should then allocate @params + * As a special case, calling with @params as NULL and @nparams as 0 on + * input will cause @nparams on output to contain the number of parameters + * supported by the hypervisor. The caller should then allocate @params * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API * again. * @@ -3765,12 +3767,21 @@ error: * @domain: pointer to domain object * @params: pointer to blkio parameter object * (return value, allocated by the caller) - * @nparams: pointer to number of blkio parameters + * @nparams: pointer to number of blkio parameters; input and output * @flags: an OR'ed set of virDomainModificationImpact * - * Get all blkio parameters, the @params array will be filled with the values - * equal to the number of parameters suggested by @nparams. - * See virDomainGetMemoryParameters for an equivalent usage example. + * Get all blkio parameters. On input, @nparams gives the size of the + * @params array; on output, @nparams gives how many slots were filled + * with parameter information, which might be less but will not exceed + * the input value. + * + * As a special case, calling with @params as NULL and @nparams as 0 on + * input will cause @nparams on output to contain the number of parameters + * supported by the hypervisor. The caller should then allocate @params + * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API + * again. + * + * See virDomainGetMemoryParameters() for an equivalent usage example. * * This function may require privileged access to the hypervisor. This function * expects the caller to allocate the @params. @@ -6335,14 +6346,17 @@ error: * @params: pointer to scheduler parameter objects * (return value) * @nparams: pointer to number of scheduler parameter objects - * (this value must be at least as large as the returned value - * nparams of virDomainGetSchedulerType) + * (this value should generally be as large as the returned value + * nparams of virDomainGetSchedulerType()); input and output + * + * Get all scheduler parameters. On input, @nparams gives the size of the + * @params array; on output, @nparams gives how many slots were filled + * with parameter information, which might be less but will not exceed + * the input value. @nparams cannot be 0. * - * Get all scheduler parameters, the @params array will be filled with the - * values and @nparams will be updated to the number of valid elements in - * @params. It is hypervisor specific whether this returns the live or + * It is hypervisor specific whether this returns the live or * persistent state; for more control, use - * virDomainGetSchedulerParametersFlags. + * virDomainGetSchedulerParametersFlags(). * * Returns -1 in case of error, 0 in case of success. */ @@ -6391,15 +6405,28 @@ error: * (return value) * @nparams: pointer to number of scheduler parameter * (this value should be same than the returned value - * nparams of virDomainGetSchedulerType) + * nparams of virDomainGetSchedulerType()); input and output * @flags: one of virDomainModificationImpact * - * Get the scheduler parameters, the @params array will be filled with the - * values. + * Get all scheduler parameters. On input, @nparams gives the size of the + * @params array; on output, @nparams gives how many slots were filled + * with parameter information, which might be less but will not exceed + * the input value. @nparams cannot be 0. * * The value of @flags can be exactly VIR_DOMAIN_AFFECT_CURRENT, * VIR_DOMAIN_AFFECT_LIVE, or VIR_DOMAIN_AFFECT_CONFIG. * + * Here is a sample code snippet: + * + * char *ret = virDomainGetSchedulerType(dom,&nparams); + * if (ret&& nparams != 0) { + * if ((params = malloc(sizeof(*params) * nparams)) == NULL) + * goto error; + * memset(params, 0, sizeof(*params) * nparams); + * if (virDomainGetSchedulerParametersFlags(dom, params,&nparams, 0)) + * goto error; + * } + * * Returns -1 in case of error, 0 in case of success. */ int @@ -6633,8 +6660,8 @@ error: * @path: path to the block device * @params: pointer to block stats parameter object * (return value) - * @nparams: pointer to number of block stats - * @flags: unused, always passes 0 + * @nparams: pointer to number of block stats; input and output + * @flags: unused, always pass 0 * * This function is to get block stats parameters for block * devices attached to the domain. @@ -6646,24 +6673,26 @@ error: * Domains may have more than one block device. To get stats for * each you should make multiple calls to this function. * - * The @params array will be filled with the value equal to the number of - * parameters suggested by @nparams. + * On input, @nparams gives the size of the @params array; on output, + * @nparams gives how many slots were filled with parameter + * information, which might be less but will not exceed the input + * value. * - * As the value of @nparams is dynamic, call the API setting @nparams to 0 and - * @params as NULL, the API returns the number of parameters supported by the - * HV by updating @nparams on SUCCESS. (Note that block device of different type - * might support different parameters numbers, so it might be necessary to compute - * @nparams for each block device type). The caller should then allocate @params + * As a special case, calling with @params as NULL and @nparams as 0 on + * input will cause @nparams on output to contain the number of parameters + * supported by the hypervisor. (Note that block devices of different types + * might support different parameters, so it might be necessary to compute + * @nparams for each block device). The caller should then allocate @params * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API - * again. See virDomainGetMemoryParameters for more details. + * again. See virDomainGetMemoryParameters() for more details. * * Returns -1 in case of error, 0 in case of success. */ -int virDomainBlockStatsFlags (virDomainPtr dom, - const char *path, - virTypedParameterPtr params, - int *nparams, - unsigned int flags) +int virDomainBlockStatsFlags(virDomainPtr dom, + const char *path, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) { virConnectPtr conn;
@@ -6677,7 +6706,8 @@ int virDomainBlockStatsFlags (virDomainPtr dom, virDispatchError(NULL); return -1; } - if (!path || (nparams == NULL) || (*nparams< 0)) { + if (!path || (nparams == NULL) || (*nparams< 0) || + (params == NULL&& *nparams != 0)) { virLibConnError(VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } Looks good to me. ACK.
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Matthias Bolte -
Stefan Berger