| Message ID | 20190112175812.27068-11-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
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 <eblake@redhat.com> > Message-Id: <20181215135324.152629-14-eblake@redhat.com> > Reviewed-by: Richard W.M. Jones <rjones@redhat.com> > > --- > v3: Improve commit message [Rich], formatting tweak [checkpatch], > rebase to dropped patch > --- > nbd/client.c | 67 +++++++++++++++++++++++++++++++++--------------- > nbd/trace-events | 2 +- > 2 files changed, 48 insertions(+), 21 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index 77993890f04..3c716be2719 100644 > --- a/nbd/client.c > +++ 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); > + if (query) { > + context_len = strlen(query); looks like it then should be query_len > + 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 > + > + trace_nbd_opt_meta_request(nbd_opt_lookup(opt), query ?: "(all)", export); > + 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 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. Interesting, troubles around "don't use be64_to_cpuS, use only be64_to_cpu", do they apply somehow to *_be_p functions family? > + 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() With this and s/uint32_t/queries (other tiny things up to you): Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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 >
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 <eblake@redhat.com> >> Message-Id: <20181215135324.152629-14-eblake@redhat.com> >> Reviewed-by: Richard W.M. Jones <rjones@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. > > With this and s/uint32_t/queries (other tiny things up to you): > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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 >> > >
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 <eblake@redhat.com> >>> Message-Id: <20181215135324.152629-14-eblake@redhat.com> >>> Reviewed-by: Richard W.M. Jones <rjones@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 <vsementsov@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 >>> >> >> >
On 1/15/19 9:52 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> -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? I didn't see any added value in tracing the number, since the trace point can only happen on one of two opt values (the number is more important when the opt value can be anything, including one that does not have a corresponding string name compiled in).
15.01.2019 18:55, Eric Blake wrote: > On 1/15/19 9:52 AM, Vladimir Sementsov-Ogievskiy wrote: > >>>>> -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? > > I didn't see any added value in tracing the number, since the trace > point can only happen on one of two opt values (the number is more > important when the opt value can be anything, including one that does > not have a corresponding string name compiled in). > OK, agreed.
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 <eblake@redhat.com> >>> Message-Id: <20181215135324.152629-14-eblake@redhat.com> >>> Reviewed-by: Richard W.M. Jones <rjones@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). and if you are going to fix it, note, that function name may be fixed too, as we don't send context, we send query. Especially in the case when query=NULL, we send empty query, but what is empty context?
diff --git a/nbd/client.c b/nbd/client.c index 77993890f04..3c716be2719 100644 --- a/nbd/client.c +++ 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; + uint32_t context_len = 0; + uint32_t data_len; + char *data; + char *p; + + data_len = sizeof(export_len) + export_len + sizeof(queries); + if (query) { + context_len = strlen(query); + data_len += sizeof(context_len) + context_len; + } else { + assert(opt == NBD_OPT_LIST_META_CONTEXT); + } + data = g_malloc(data_len); + p = data; + + trace_nbd_opt_meta_request(nbd_opt_lookup(opt), query ?: "(all)", export); + 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); + 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" 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