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

[Eric Blake]

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 <address@hidden>
>> Message-Id: <address@hidden>
>>
>> ---
>> 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.

> 
>>   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 <address@hidden>
> 

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

signature.asc
Description: OpenPGP digital signature