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

[mdroth]

On Wed, Mar 06, 2013 at 11:48:14PM +0100, Laszlo Ersek wrote:
> 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).
> 

That's the general understanding for the current interfaces: "unsupported"
is a higher-level error than the errors that individual commands might
document. So I think we should keep this as-is for consistency, and if
it does need to be documented better then a patch adding the big banner
at the top of the schema is probably the best approach actually.

> > 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
>