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

[Eric Blake]

On 03/07/2012 12:18 AM, Osier Yang wrote:
> 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).
>>>

>>> /* 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.

I realized that qemu_driver.c also needs this memset to guarantee that
unused slots are returned empty on success.

>>
>> 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.

Good idea.  Here's what I squashed in before pushing:

diff --git i/src/libvirt.c w/src/libvirt.c
index d98741b..39ebb52 100644
--- i/src/libvirt.c
+++ w/src/libvirt.c
@@ -18534,7 +18534,9 @@ error:
  * 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 cpus
+ * available to query, then the trailing part of the array will
+ * be unpopulated.
  *
  * The remote driver imposes a limit of 128 @ncpus and 16 @nparams;
  * the number of parameters per cpu should not exceed 16, but if you
@@ -18579,8 +18581,10 @@ error:
  * number of populated @params, unless @ncpus was 1; and may be
  * less than @nparams).  The populated parameters start at each
  * stride of @nparams, which means the results may be discontiguous;
- * any unpopulated parameters will be zeroed on success.  The caller
- * is responsible for freeing any returned string parameters.
+ * any unpopulated parameters will be zeroed on success (this includes
+ * skipped elements if @nparams is too large, and tail elements if
+ * @ncpus is too large).  The caller is responsible for freeing any
+ * returned string parameters.
  */
 int virDomainGetCPUStats(virDomainPtr domain,
                          virTypedParameterPtr params,
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c
index 538a419..ddb381a 100644
--- i/src/qemu/qemu_driver.c
+++ w/src/qemu/qemu_driver.c
@@ -12162,6 +12162,7 @@ qemuDomainGetPercpuStats(virDomainPtr domain,
     if (virCgroupGetCpuacctPercpuUsage(group, &buf))
         goto cleanup;
     pos = buf;
+    memset(params, 0, nparams * ncpus);

     if (max_id - start_cpu > ncpus - 1)
         max_id = start_cpu + ncpus - 1;


-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120307/7821e552/attachment-0001.sig>