Re: [Qemu-devel] [PATCH v2 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2977 bytes --]

On 03/06/2013 02:59 PM, Laszlo Ersek wrote:
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  qga/qapi-schema.json |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  qga/commands-posix.c |   12 ++++++++
>  qga/commands-win32.c |   12 ++++++++
>  3 files changed, 96 insertions(+), 0 deletions(-)
> 
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index d91d903..cba881c 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -515,3 +515,75 @@
>  ##
>  { 'command': 'guest-network-get-interfaces',
>    'returns': ['GuestNetworkInterface'] }
> +
> +##
> +# @GuestLogicalProcessor:
> +#
> +# @logical-id: Arbitrary guest-specific unique identifier of the VCPU.
> +#
> +# @online: Whether the VCPU is enabled.
> +#
> +# @can-offline: Whether offlining the VCPU is possible. This member is always
> +#               filled in by the guest agent when the structure is returned,
> +#               and always ignored on input (hence it can be omitted then).

Other places have used the notation '#optional' when documenting a
parameter that need not be present on input; although we don't have
anything that strictly requires/enforces that notation.

> +#
> +# Since: 1.5
> +##
> +{ 'type': 'GuestLogicalProcessor',
> +  'data': {'logical-id': 'int',

Should logical-id be 'str' instead of 'int', since we said it is
arbitrary what the guest names its vcpus?  Then again, integers can be
made to work (even if the guest OS prefers to name cpus via strings, the
agent can track a 1:1 lookup table between OS string and integer number
handed over qga, perhaps even by returning an invariant pointer address
of the OS string as the integer identifier), so I won't insist.

> +# Returns: The length of the initial sublist that has been successfully
> +#          processed. The guest agent maximizes this value. Possible cases:
> +#
> +#          0:                if the @vcpus list was empty on input. Guest state
> +#                            has not been changed. Otherwise,
> +#
> +#          Error:            processing the first node of @vcpus failed for the
> +#                            reason returned. Guest state has not been changed.
> +#                            Otherwise,
> +#

> +
> +int64_t qmp_guest_set_vcpus(GuestLogicalProcessorList *vcpus, Error **errp)
> +{
> +    error_set(errp, QERR_UNSUPPORTED);
> +    return -1;

This returns an error even on an empty input @vcpus, while the docs said
that returning 0 takes priority.  But it's so much of a corner case that
I don't care; always returning an error seems fine.

Thus, although there are things you might change if you have to respin
the series for later review comments, I'm perfectly fine leaving this
as-is and you can use:

Reviewed-by: Eric Blake <eblake@redhat.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]