qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v3 10/19] nbd/client: Split out nbd_send_one_met


From: Eric Blake
Subject: Re: [Qemu-block] [PATCH v3 10/19] nbd/client: Split out nbd_send_one_meta_context()
Date: Tue, 15 Jan 2019 09:44:56 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0

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 <address@hidden>
>> Message-Id: <address@hidden>
>> Reviewed-by: Richard W.M. Jones <address@hidden>
>>
>> ---
>> 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).

> 
>> +        data_len += sizeof(context_len) + context_len;
>> +    } else {
>> +        assert(opt == NBD_OPT_LIST_META_CONTEXT);
>> +    }
>> +    data = g_malloc(data_len);
>> +    p = data;
> 
> may use p = data = g_malloc

Will do.

> 
>> +
>> +    trace_nbd_opt_meta_request(nbd_opt_lookup(opt), query ?: "(all)", 
>> export);

[2]

>> +    stl_be_p(p, export_len);
>> +    memcpy(p += sizeof(export_len), export, export_len);
>> +    stl_be_p(p += export_len, queries);
>> +    if (query) {
>> +        stl_be_p(p += sizeof(uint32_t), context_len);
> 
> :), aha, please, s/uint32_t/queries, as you promised

I did up at [1], but should indeed do so again here.

> 
> Hmm, its my code. It's hard to read and not very comfortable to maintain..
> 
> In block/nbd-client.c we have
> payload_advance* functions, to read such formatted data, I think, it should be
> good to make something like this for server-part. Not about these series, of 
> course.

Yes, I wouldn't object to even more cleanups to make the code easier to
maintain, but not as part of this series.

> 
> Interesting, troubles around "don't use be64_to_cpuS, use only be64_to_cpu",
> do they apply somehow to *_be_p functions family?

The problem was in the in-place conversion routines where the
destination type was strongly typed to something wider than char*.  This
is not an inplace conversion, because st*_p takes a raw pointer
interpreted as char* as its destination.  So no, clang does not have
problems with this construct.

> 
>> +        memcpy(p += sizeof(context_len), query, context_len);
>> +    }
>> +
>> +    ret = nbd_send_option_request(ioc, opt, data_len, data, errp);
>> +    g_free(data);
>> +    return ret;
>> +}
>> +
>>   /* nbd_negotiate_simple_meta_context:
>>    * Request the server to set the meta context for export @info->name
>>    * using @info->x_dirty_bitmap with a fallback to "base:allocation",
>> @@ -653,26 +696,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 */
>> -                        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);
>> -    stl_be_p(p += sizeof(uint32_t), context_len);
>> -    memcpy(p += sizeof(context_len), context, context_len);
>> -
>> -    ret = nbd_send_option_request(ioc, NBD_OPT_SET_META_CONTEXT, data_len, 
>> data,
>> -                                  errp);
>> -    g_free(data);
>> -    if (ret < 0) {
>> -        return ret;
>> +    if (nbd_send_one_meta_context(ioc, NBD_OPT_SET_META_CONTEXT,
>> +                                  info->name, context, errp) < 0) {
>> +        return -1;
>>       }
>>
>>       if (nbd_receive_option_reply(ioc, NBD_OPT_SET_META_CONTEXT, &reply,
>> @@ -689,7 +716,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel 
>> *ioc,
>>       if (reply.type == NBD_REP_META_CONTEXT) {
>>           char *name;
>>
>> -        if (reply.length != sizeof(info->context_id) + context_len) {
>> +        if (reply.length != sizeof(info->context_id) + strlen(context)) {
>>               error_setg(errp, "Failed to negotiate meta context '%s', 
>> server "
>>                          "answered with unexpected length %" PRIu32, context,
>>                          reply.length);
>> diff --git a/nbd/trace-events b/nbd/trace-events
>> index c3966d2b653..59521e47a3d 100644
>> --- a/nbd/trace-events
>> +++ b/nbd/trace-events
>> @@ -12,7 +12,7 @@ nbd_receive_query_exports_start(const char *wantname) 
>> "Querying export list for
>>   nbd_receive_query_exports_success(const char *wantname) "Found desired 
>> export name '%s'"
>>   nbd_receive_starttls_new_client(void) "Setting up TLS"
>>   nbd_receive_starttls_tls_handshake(void) "Starting TLS handshake"
>> -nbd_opt_meta_request(const char *context, const char *export) "Requesting 
>> to set meta context %s for export %s"
>> +nbd_opt_meta_request(const char *optname, const char *context, const char 
>> *export) "Requesting %s %s for export %s"
> 
> Hmm, you forget nbd_opt_lookup()

Where? The updated trace point at [2] has nbd_opt_lookup() for
determining optname.

> 
> With this and s/uint32_t/queries (other tiny things up to you):
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
>>   nbd_opt_meta_reply(const char *context, uint32_t id) "Received mapping of 
>> context %s to id %" PRIu32
>>   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
>>
> 
> 

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

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]