Re: [libvirt] [PATCH] rpc: allow truncated return for virDomainGetCPUStats

On 03/07/2012 12:48 PM, Eric Blake wrote: > The RPC code assumed that the array returned by the driver would be > fully populated; that is, ncpus on entry resulted in ncpus * return > value on exit. However, while we don't support holes in the middle > of ncpus, we do want to permit the case of ncpus on entry being > longer than the array returned by the driver (that is, it should be > safe for the caller to pass ncpus=128 on entry, and the driver will > stop populating the array when it hits max_id). > > There are now three cases: > server 0.9.10 and client 0.9.10 or newer: No impact - there were no > hypervisor drivers that supported cpu stats > > server 0.9.11 or newer and client 0.9.10: if the client calls with > ncpus beyond the max, then the rpc call will fail on the client side > and disconnect the client, but the server is no worse for the wear > > server 0.9.11 or newer and client 0.9.11: the server can return a > truncated array and the client will do just fine > > I reproduced the problem by using a host with 2 CPUs, and doing: > virsh cpu-stats $dom --start 1 --count 2 > > * daemon/remote.c (remoteDispatchDomainGetCPUStats): Allow driver > to omit tail of array. > * src/remote/remote_driver.c (remoteDomainGetCPUStats): > Accommodate driver that omits tail of array. > --- > daemon/remote.c | 10 ++++++++-- > src/remote/remote_driver.c | 6 ++++-- > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/daemon/remote.c b/daemon/remote.c > index 74a5f16..39302cc 100644 > --- a/daemon/remote.c > +++ b/daemon/remote.c > @@ -3574,11 +3574,17 @@ > remoteDispatchDomainGetCPUStats(virNetServerPtr server ATTRIBUTE_UNUSED, > args->flags)< 0) > goto cleanup; > > - percpu_len = ret->params.params_len / args->ncpus; > - > success: > rv = 0; > ret->nparams = percpu_len; > + if (args->nparams&& !(args->flags& VIR_TYPED_PARAM_STRING_OKAY)) { > + int i; > + > + for (i = 0; i< percpu_len; i++) { > + if (params[i].type == VIR_TYPED_PARAM_STRING) > + ret->nparams--; > + } > + } > > cleanup: > if (rv< 0) > diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c > index 9e74cea..031167a 100644 > --- a/src/remote/remote_driver.c > +++ b/src/remote/remote_driver.c > @@ -2382,7 +2382,7 @@ static int remoteDomainGetCPUStats(virDomainPtr > domain, > /* Check the length of the returned list carefully. */ > if (ret.params.params_len> nparams * ncpus || > (ret.params.params_len&& > - ret.nparams * ncpus != ret.params.params_len)) { > + ((ret.params.params_len % ret.nparams) || ret.nparams> nparams))) { > remoteError(VIR_ERR_RPC, "%s", > _("remoteDomainGetCPUStats: " > "returned number of stats exceeds limit")); > @@ -2399,9 +2399,11 @@ static int remoteDomainGetCPUStats(virDomainPtr > domain, > } > > /* The remote side did not send back any zero entries, so we have > - * to expand things back into a possibly sparse array. > + * to expand things back into a possibly sparse array, where the > + * tail of the array may be omitted. > */ > memset(params, 0, sizeof(*params) * nparams * ncpus); > + ncpus = ret.params.params_len / ret.nparams; > for (cpu = 0; cpu< ncpus; cpu++) { > int tmp = nparams; > remote_typed_param *stride =&ret.params.params_val[cpu * ret.nparams]; Make sense, and ACK. But do we want to add document to declare the returned array will be truncated among the API implementation. Not neccessary though.

Osier -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list