Re: [Qemu-devel] [PATCH v3 10/19] nbd/client: Split out nbd_send_one_meta_context()

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

15.01.2019 18:44, Eric Blake wrote:
> On 1/15/19 7:18 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.01.2019 20:58, 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.  The old comment about 'sizeof(uint32_t)'
>>> being equivalent to '/* number of queries */' is no longer
>>> needed, now that we are computing 'sizeof(queries)' instead.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> Message-Id: <20181215135324.152629-14-eblake@redhat.com>
>>> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
>>>
>>> ---
>>> v3: Improve commit message [Rich], formatting tweak [checkpatch],
>>> rebase to dropped patch
>>> ---
> 
>>> +++ b/nbd/client.c
>>> @@ -629,6 +629,49 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
>>>        return QIO_CHANNEL(tioc);
>>>    }
>>>
>>> +/*
>>> + * nbd_send_one_meta_context:
>>> + * Send 0 or 1 set/list meta context queries.
>>> + * Return 0 on success, -1 with errp set for any error
>>> + */
>>> +static int nbd_send_one_meta_context(QIOChannel *ioc,
>>> +                                     uint32_t opt,
>>> +                                     const char *export,
>>> +                                     const char *query,
>>> +                                     Error **errp)
>>> +{
>>> +    int ret;
>>> +    uint32_t export_len = strlen(export);
>>> +    uint32_t queries = !!query;
>>
>> n_ or nb_ prefix may make it more clear
>>
>>> +    uint32_t context_len = 0;
>>> +    uint32_t data_len;
>>> +    char *data;
>>> +    char *p;
>>> +
>>> +    data_len = sizeof(export_len) + export_len + sizeof(queries);
> 
> [1]
> 
>>> +    if (query) {
>>> +        context_len = strlen(query);
>>
>> looks like it then should be query_len
> 
> Sure, an alternative name may make things easier to read (I think this
> is somewhat fallout from my rebase churn, where earlier versions of the
> patch shared as much code with NBD_OPT_SET_META_CONTEXT, and that code
> used the name 'context' rather than 'query'; but now that I've split
> things to add a new function, it doesn't have to maintain the old naming).

and if you are going to fix it, note, that function name may be fixed too, as we
don't send context, we send query. Especially in the case when query=NULL,
we send empty query, but what is empty context?


-- 
Best regards,
Vladimir