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: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-block] [PATCH v3 10/19] nbd/client: Split out nbd_send_one_meta_context()
Date: Tue, 15 Jan 2019 15:52:11 +0000

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 <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.

Your morning is my evening)

Yes, it's ok, and in the next patch too. But you usually trace both number + 
lookup-string,
and here only string. Is there a reason to?

> 
>>
>> 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
>>>
>>
>>
> 


-- 
Best regards,
Vladimir

reply via email to

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