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 <ebl...@redhat.com>
Thanks much!
Laszlo
