On 09/15/2014 09:42 AM, Peter Krempa wrote:
> From: Francesco Romani <fromani@xxxxxxxxxx>
>
> This patch implements the VIR_DOMAIN_STATS_VCPU group of statistics. To
> do so, this patch also extracts a helper to gather the vCPU information.
>
> Signed-off-by: Francesco Romani <fromani@xxxxxxxxxx>
> Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> ---
>
> Notes:
> Version 6:
> - changed indexing of the returned parameters from the retuned array that
> is not filled if the guest is offline to the local index and stopped
> returning the CPU time once the guest is offline.
>
> include/libvirt/libvirt.h.in | 1 +
> src/libvirt.c | 12 +++
> src/qemu/qemu_driver.c | 205 +++++++++++++++++++++++++++++--------------
> 3 files changed, 154 insertions(+), 64 deletions(-)
>
I might have split this into two patches - one for the function
extraction (mostly code motion, no real new code - with the first use
being the existing API) and the other for the new code (libvirt.h.in
addition, new code also calling into the function extracted in part 1).
But it's not worth making you respin it now, so I can live with it as
one patch.
> +++ b/src/libvirt.c
> @@ -21609,6 +21609,18 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
> * "balloon.maximum" - the maximum memory in kiB allowed
> * as unsigned long long.
> *
> + * VIR_DOMAIN_STATS_VCPU: Return virtual CPU statistics.
> + * Due to VCPU hotplug, the vcpu.<num>.* array could be sparse.
> + * The actual size of the array correspond to "vcpu.current".
s/correspond/corresponds/
> + /* Clamp to actual number of vcpus */
> + if (maxinfo > priv->nvcpupids)
> + maxinfo = priv->nvcpupids;
> +
> + if (maxinfo >= 1) {
> + if (info != NULL) {
Weak style preference for 'if (info)' in this file, but not strong
enough to require that it be changed.
> + memset(info, 0, sizeof(*info) * maxinfo);
> + for (i = 0; i < maxinfo; i++) {
> + info[i].number = i;
> + info[i].state = VIR_VCPU_RUNNING;
> +
> + if (priv->vcpupids != NULL &&
Another verbose comparison to NULL.
> +
> + if (cpumaps != NULL) {
> + memset(cpumaps, 0, maplen * maxinfo);
> + if (priv->vcpupids != NULL) {
and a couple more
> -
> - if (maxinfo >= 1) {
> - if (info != NULL) {
Then again, you're just doing code motion, so keep this patch as-is, and
save any comparison to NULL cleanups to something separate.
> +
> + if (VIR_ALLOC_N(cpuinfo, dom->def->vcpus) < 0)
> + return -1;
Hmm. cpuinfo is guaranteed to be zero-initialized here; can we avoid the
memset() to zero in the helper function? But that could be a followup
patch.
> +
> + if (qemuDomainHelperGetVcpus(dom, cpuinfo, dom->def->vcpus,
> + NULL, 0) < 0) {
> + virResetLastError();
> + ret = 0; /* it's ok to be silent and go ahead */
Technically, the error still gets listed in the logs; we'll probably get
people complaining about "why is my log showing an error even though
nothing went wrong". So in the future, we may need to teach
qemuDomainHelperGetVcpus to be silent to begin with, rather than logging
failures that we later ignore; but that can be a followup patch.
> + goto cleanup;
> + }
> +
> + for (i = 0; i < dom->def->vcpus; i++) {
> + snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH,
> + "vcpu.%zu.state", i);
Coverity might complain that the result is unchecked, if so, adding an
ignore_value() will be sufficient (I'm satisfied that
sizeof("vcpu..state") plus the longest possible 64-bit unsigned
stringized number fits comfortably within the field length, so I agree
with your choice of not bothering to check for failure).
So, although I was rather chatty, I don't see anything preventing me
from saying:
ACK
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list