From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43866) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDNhs-0000Wx-4s for qemu-devel@nongnu.org; Wed, 06 Mar 2013 18:25:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UDNhp-0005si-Gn for qemu-devel@nongnu.org; Wed, 06 Mar 2013 18:25:08 -0500 Received: from mail-ie0-x231.google.com ([2607:f8b0:4001:c03::231]:37546) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UDNhp-0005sC-CH for qemu-devel@nongnu.org; Wed, 06 Mar 2013 18:25:05 -0500 Received: by mail-ie0-f177.google.com with SMTP id 16so10255742iea.22 for ; Wed, 06 Mar 2013 15:25:04 -0800 (PST) Sender: fluxion Date: Wed, 6 Mar 2013 17:24:48 -0600 From: mdroth Message-ID: <20130306232448.GB4005@vm> References: <1362607171-24668-1-git-send-email-lersek@redhat.com> <1362607171-24668-2-git-send-email-lersek@redhat.com> <5137C3E0.3060600@redhat.com> <5137C7AE.10309@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5137C7AE.10309@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 1/3] qga: introduce guest-get-vcpus / guest-set-vcpus with stubs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: qemu-devel@nongnu.org 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 > > Thanks much! > Laszlo >