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

[Eric Blake <eblake@redhat.com>]

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 <eblake@redhat.com>
>>
>> ---
>> 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 <rjones@redhat.com>

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