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

2012-03-07 Thread 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   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2012-03-06 Thread Eric Blake 
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];
-- 
1.7.7.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2012-03-06 Thread Osier Yang 

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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2012-03-06 Thread 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list