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

[Osier Yang]

On 03/07/2012 02:46 PM, Osier Yang wrote:
> 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.

Perhaps something like:

   * whole).  Otherwise, @start_cpu represents which cpu to start
   * with, and @ncpus represents how many consecutive processors to
   * query, with statistics attributable per processor (such as
- * per-cpu usage).
+ * per-cpu usage). If @ncpus is larger than the number of host
+ * CPUs, the exceeded one(s) will be just ignored.


>
> Osier
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list