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

[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