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