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

[Laszlo Ersek]

On 03/06/13 23:32, Eric Blake wrote:
> On 03/06/2013 02:59 PM, Laszlo Ersek wrote:

>> +##
>> +# @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.

I'll fix this in v3 if I'll have to respin, otherwise I'd prefer a
followup patch.

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

I see what you mean. In my mind, "unsupported" beats everything else, as
if there was a big banner on top of the schema file: "you'll get
QERR_UNSUPPORTED from any interface that's not supported".

I'd like to leave this as-is even if I have to respin; distinguishing
between zero-length-list and "unsupported" seems awkward, plus I'd also
like to accept an empty list without error (in the supported case).

> 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 <address@hidden>

Thanks much!
Laszlo