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

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

15.01.2019 18:50, Eric Blake wrote:
> On 1/15/19 9:05 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 12.01.2019 20:58, Eric Blake wrote:
>>> Extract portions of nbd_negotiate_simple_meta_context() to
>>> a new function nbd_receive_one_meta_context() that copies the
>>> pattern of nbd_receive_list() for performing the argument
>>> validation of one reply.  The error message when the server
>>> replies with more than one context changes slightly, but
>>> that shouldn't happen in the common case.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> Message-Id: <20181215135324.152629-15-eblake@redhat.com>
>>>
>>> ---
>>> v3: rebase, without changing into a loop
>>> ---
> 
>>>
>>> +static int nbd_receive_one_meta_context(QIOChannel *ioc,
>>> +                                        uint32_t opt,
>>> +                                        char **name,
>>> +                                        uint32_t *id,
>>> +                                        Error **errp)
>>> +{
> 
>>> +    reply.length -= sizeof(local_id);
>>> +    local_name = g_malloc(reply.length + 1);
>>> +    if (nbd_read(ioc, local_name, reply.length, errp) < 0) {
>>> +        g_free(local_name);
>>> +        return -1;
>>> +    }
>>> +    local_name[reply.length] = '\0';
>>> +    trace_nbd_opt_meta_reply(nbd_opt_lookup(opt), local_name, local_id);
>>> +
> 
> [1]
> 
>>> @@ -746,36 +796,20 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc,
>>>                return -1;
>>>            }
>>>            g_free(name);
>>> -
>>> -        trace_nbd_opt_meta_reply(context, info->context_id);
>>>            received = true;
>>>
>>> -        /* receive NBD_REP_ACK */
>>> -        if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, &reply,
>>> -                                     errp) < 0)
>>> -        {
>>> +        ret = nbd_receive_one_meta_context(ioc, NBD_OPT_SET_META_CONTEXT,
>>> +                                       &name, &info->context_id, errp);
>>
>> indent, and, no reasons to use variables instead of NULL, NULL, as success here is an error
>> path anyway
> 
> Not sure how I missed the indent, and good point about not needing &name
> or &info->context_id on this second call.
> 
>>> +++ b/nbd/trace-events
>>> @@ -13,7 +13,7 @@ nbd_receive_query_exports_success(const char *wantname) "Found desired export na
>>>    nbd_receive_starttls_new_client(void) "Setting up TLS"
>>>    nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
>>>    nbd_opt_meta_request(const char *optname, const char *context, const char *export) "Requesting %s %s for export %s"
>>> -nbd_opt_meta_reply(const char *context, uint32_t id) "Received mapping of context %s to id %" PRIu32
>>> +nbd_opt_meta_reply(const char *optname, const char *context, uint32_t id) "Received %s mapping of %s to id %" PRIu32
>>
>> nbd_opt_lookup
> 
> Added at [1], so I'm not sure what you are still wanting here.

Yes, my fault, it's OK.

> 
>>
>>>    nbd_receive_negotiate(void *tlscreds, const char *hostname) "Receiving negotiation tlscreds=%p hostname=%s"
>>>    nbd_receive_negotiate_magic(uint64_t magic) "Magic is 0x%" PRIx64
>>>    nbd_receive_negotiate_server_flags(uint32_t globalflags) "Global flags are 0x%" PRIx32
>>>
>>
>> With at least nbd_opt_lookup:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
> 


-- 
Best regards,
Vladimir