[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
[Qemu-block] [PATCH v3 17/19] qemu-nbd: Add --list option, Eric Blake, 2019/01/12
[Qemu-block] [PATCH v3 16/19] nbd/client: Add meta contexts to nbd_receive_export_list(), Eric Blake, 2019/01/12
[Qemu-block] [PATCH v3 13/19] nbd/client: Split handshake into two functions, Eric Blake, 2019/01/12