Re: [Qemu-devel] [PATCH v2 13/22] nbd/client: Split out nbd_send_one_meta_context()

[Eric Blake]

On 12/15/18 9:22 AM, Richard W.M. Jones wrote:
> On Sat, Dec 15, 2018 at 07:53:15AM -0600, Eric Blake wrote:
> > Refactor nbd_negotiate_simple_meta_context() to pull out the
> > code that can be reused to send a LIST request for 0 or 1 query.
> > No semantic change.
> >
> > Signed-off-by: Eric Blake <address@hidden>
> >
> > ---
> > v2: split patch into easier-to-review pieces [Rich, Vladimir]
> > ---
> >   nbd/client.c     | 64 ++++++++++++++++++++++++++++++++++--------------
> >   nbd/trace-events |  2 +-
> >   2 files changed, 46 insertions(+), 20 deletions(-)

> > +    uint32_t queries = !!query;

This initialization...


> > +    uint32_t context_len = 0;
> > +    uint32_t data_len;
> > +    char *data;
> > +    char *p;
> > +
> > +    data_len = sizeof(export_len) + export_len + sizeof(queries);

...plus the fact that it is now sizeof(variable) instead of sizeof(type)...

> > @@ -651,26 +693,10 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
> > *ioc,
> >       NBDOptionReply reply;
> >       const char *context = info->x_dirty_bitmap ?: "base:allocation";
> >       bool received = false;
> > -    uint32_t export_len = strlen(info->name);
> > -    uint32_t context_len = strlen(context);
> > -    uint32_t data_len = sizeof(export_len) + export_len +
> > -                        sizeof(uint32_t) + /* number of queries */

...was the reason that I dropped the comment here.  The comment made 
sense for explaining why a sizeof(type) was being injected into data_len,

> > -                        sizeof(context_len) + context_len;
> > -    char *data = g_malloc(data_len);
> > -    char *p = data;
> >
> > -    trace_nbd_opt_meta_request(context, info->name);
> > -    stl_be_p(p, export_len);
> > -    memcpy(p += sizeof(export_len), info->name, export_len);
> > -    stl_be_p(p += export_len, 1);

...because the old code was hard-coding 1 query, while the new code uses 
the variable (whose value is the number of queries) rather than a 
hard-coding.


> Although a straightforward refactoring, we still lost the comment
> /* number of queries */.  I'd still perhaps like to see a bit more
> explanation of the layout and reasoning behind the data buffer.

Perhaps a possible improvement would be to introduce a packed struct 
that matches the protocol layout, instead of piece-meal constructing the 
struct.  But I'm not sure how much effort to spend on this code.

> But anyway ..
>
> Reviewed-by: Richard W.M. Jones <address@hidden>

Thanks. I'm glad you forced me to split the v1 patch into more 
manageable pieces; I like how it turned out.

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