Re: [Qemu-devel] [PATCH 07/14] nbd/client: Refactor nbd_negotiate_simple_meta_context()

[Eric Blake]

On 12/6/18 7:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 01.12.2018 1:03, Eric Blake wrote:
> > Change the signature to make it easier for a future patch to
> > reuse this function for calling NBD_OPT_LIST_META_CONTEXT with
> > 0 or 1 queries.  Also, always allocate space for the received
> > name, even if it doesn't match expected lengths (no point
> > trying to optimize the unlikely error case, and tracing the
> > received rather than expected name can help debug a server
> > implementation).
> >
> > While there are now slightly different traces, and the error
> > message for a server replying with too many contexts is
> > different, there are no runtime-observable changes in behavior
> > for the more common case of the lone caller interacting with a
> > compliant server.
> >
> > Signed-off-by: Eric Blake <address@hidden>
> > ---
> >    nbd/client.c     | 105 +++++++++++++++++++++++++++--------------------
> >    nbd/trace-events |   2 +-
> >    2 files changed, 61 insertions(+), 46 deletions(-)
> >
> > diff --git a/nbd/client.c b/nbd/client.c
> > index b5818a99d21..1dc8f83e19a 100644
> > --- a/nbd/client.c
> > +++ b/nbd/client.c
> > @@ -603,49 +603,57 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
> >    }
> >
> >    /* nbd_negotiate_simple_meta_context:
> > - * Set one meta context. Simple means that reply must contain zero (not
> > - * negotiated) or one (negotiated) contexts. More contexts would be considered
> > - * as a protocol error. It's also implied that meta-data query equals queried
> > - * context name, so, if server replies with something different than @context,
> > - * it is considered an error too.
> > - * return 1 for successful negotiation, context_id is set
> > - *        0 if operation is unsupported,
> > + * List or set meta context data for export @info->name, based on @opt.
>
> hm, just list or set meta context? What is "data" about?

Okay, that's two different reviewers complaining. I'll do a bit more 
refactoring and comments for v2 to make it more obvious that I'm writing 
a common routine that can handle the bulk of the commonality between 
list and set; but maybe keep two wrappers so that the existing callers 
don't have to change their calls as drastically.

> > + * For list, leave @context NULL for 0 queries, or supplied for a single
> > + * query; all replies are ignored, and the call merely traces server behavior.
> > + * For set, @context must result in at most one matching server reply, in
> > + * which case @info->meta_base_allocation_id is set to the resulting id.
>
> Hmm, looks too cheating. Then it should be renamed to
> nbd_negotiate_base_allocation
>
> and parameter @context should be renamed to @x_dirty_bitmap,
>
> and if it is unset, we'll use "base:allocation" here.

No, the next patch uses "qemu:" as the context for LIST when recursing 
to work around qemu-nbd 3.0 not advertising the dirty-bitmap context 
under list with 0 queries.

> but in this case, it still weird about opt=list case.. So, it should be named
> like nbd_negotiation_helper, as it is doing several different things, which
> I can't describe in one word.

Yes, I think a function rename will help.

> > + * return 1 for successful negotiation,
> > + *        0 if operation is unsupported or context unavailable,
> >     *        -1 with errp set for any other error
>
> this return value description looks not very related to opt=list case

It's related - but list returns 1 for a lot more cases than set (a 
successful negotiation for list meant that all server replies were 
processed, while a successful negotiation for set requires that the 
server replies with exactly the one context we requested).

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org