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 <ebl...@redhat.com> >>> Message-Id: <20181215135324.152629-14-ebl...@redhat.com> >>> Reviewed-by: Richard W.M. Jones <rjo...@redhat.com> >>> >>> --- >>> 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 <vsement...@virtuozzo.com> >> >>> 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