| Message ID | 20181215135324.152629-12-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
On Sat, Dec 15, 2018 at 07:53:13AM -0600, Eric Blake wrote: > Pass 'info' instead of three separate parameters related to info, > when requesting the server to set the meta context. Update the > NBDExportInfo struct to rename the received id field to match the > fact that we are currently overloading the field to match whatever > context the user supplied through the x-dirty-bitmap hack, as well > as adding a TODO comment to remind future patches about a desire > to request two contexts at once. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > v2: split patch into easier-to-review pieces [Rich, Vladimir] > rename NBDExportInfo meta_base_allocation_id [Vladimir] > --- > include/block/nbd.h | 2 +- > block/nbd-client.c | 4 ++-- > nbd/client.c | 53 +++++++++++++++++++++------------------------ > 3 files changed, 28 insertions(+), 31 deletions(-) > > diff --git a/include/block/nbd.h b/include/block/nbd.h > index 65feff8ba96..ae5fe28f486 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -276,7 +276,7 @@ struct NBDExportInfo { > uint32_t opt_block; > uint32_t max_block; > > - uint32_t meta_base_allocation_id; > + uint32_t context_id; > }; > typedef struct NBDExportInfo NBDExportInfo; > > diff --git a/block/nbd-client.c b/block/nbd-client.c > index 417971d8b05..608b578e1d3 100644 > --- a/block/nbd-client.c > +++ b/block/nbd-client.c > @@ -246,11 +246,11 @@ static int nbd_parse_blockstatus_payload(NBDClientSession *client, > } > > context_id = payload_advance32(&payload); > - if (client->info.meta_base_allocation_id != context_id) { > + if (client->info.context_id != context_id) { > error_setg(errp, "Protocol error: unexpected context id %d for " > "NBD_REPLY_TYPE_BLOCK_STATUS, when negotiated context " > "id is %d", context_id, > - client->info.meta_base_allocation_id); > + client->info.context_id); > return -EINVAL; > } > > diff --git a/nbd/client.c b/nbd/client.c > index 7462fa5ae0e..bcccd5f555e 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -628,26 +628,30 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc, > } > > /* nbd_negotiate_simple_meta_context: > - * Set one meta context. Simple means that reply must contain zero (not > - * negotiated) or one (negotiated) contexts. More contexts would be considered > - * as a protocol error. It's also implied that meta-data query equals queried > - * context name, so, if server replies with something different than @context, > - * it is considered an error too. > - * return 1 for successful negotiation, context_id is set > + * Request the server to set the meta context for export @info->name > + * using @info->x_dirty_bitmap with a fallback to "base:allocation", > + * setting @info->context_id to the resulting id. Fail if the server > + * responds with more than one context or with a context different > + * than the query. > + * return 1 for successful negotiation, > * 0 if operation is unsupported, > * -1 with errp set for any other error > */ > static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > - const char *export, > - const char *context, > - uint32_t *context_id, > + NBDExportInfo *info, > Error **errp) > { > + /* > + * TODO: Removing the x_dirty_bitmap hack will mean refactoring > + * this function to request and store ids for multiple contexts > + * (both base:allocation and a dirty bitmap), at which point this > + * function should lose the term _simple. > + */ > int ret; > NBDOptionReply reply; > - uint32_t received_id = 0; > + const char *context = info->x_dirty_bitmap ?: "base:allocation"; > bool received = false; > - uint32_t export_len = strlen(export); > + 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 */ > @@ -655,9 +659,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > char *data = g_malloc(data_len); > char *p = data; > > - trace_nbd_opt_meta_request(context, export); > + trace_nbd_opt_meta_request(context, info->name); > stl_be_p(p, export_len); > - memcpy(p += sizeof(export_len), export, 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); > @@ -683,7 +687,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > if (reply.type == NBD_REP_META_CONTEXT) { > char *name; > > - if (reply.length != sizeof(received_id) + context_len) { > + if (reply.length != sizeof(info->context_id) + context_len) { > error_setg(errp, "Failed to negotiate meta context '%s', server " > "answered with unexpected length %" PRIu32, context, > reply.length); > @@ -691,12 +695,13 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > return -1; > } > > - if (nbd_read(ioc, &received_id, sizeof(received_id), errp) < 0) { > + if (nbd_read(ioc, &info->context_id, sizeof(info->context_id), > + errp) < 0) { > return -1; > } > - received_id = be32_to_cpu(received_id); > + info->context_id = be32_to_cpu(info->context_id); > > - reply.length -= sizeof(received_id); > + reply.length -= sizeof(info->context_id); > name = g_malloc(reply.length + 1); > if (nbd_read(ioc, name, reply.length, errp) < 0) { > g_free(name); > @@ -713,7 +718,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > } > g_free(name); > > - trace_nbd_opt_meta_reply(context, received_id); > + trace_nbd_opt_meta_reply(context, info->context_id); > received = true; > > /* receive NBD_REP_ACK */ > @@ -742,12 +747,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > return -1; > } > > - if (received) { > - *context_id = received_id; > - return 1; > - } > - > - return 0; > + return received; > } > > int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, > @@ -846,10 +846,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, > } > > if (info->structured_reply && base_allocation) { > - result = nbd_negotiate_simple_meta_context( > - ioc, info->name, > - info->x_dirty_bitmap ?: "base:allocation", > - &info->meta_base_allocation_id, errp); > + result = nbd_negotiate_simple_meta_context(ioc, info, errp); > if (result < 0) { > goto fail; > } This is much clearer and smaller refactoring so: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich.
15.12.2018 16:53, Eric Blake wrote: > Pass 'info' instead of three separate parameters related to info, > when requesting the server to set the meta context. Update the > NBDExportInfo struct to rename the received id field to match the > fact that we are currently overloading the field to match whatever > context the user supplied through the x-dirty-bitmap hack, as well > as adding a TODO comment to remind future patches about a desire > to request two contexts at once. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > v2: split patch into easier-to-review pieces [Rich, Vladimir] > rename NBDExportInfo meta_base_allocation_id [Vladimir] > --- [..] > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -628,26 +628,30 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc, > } > > /* nbd_negotiate_simple_meta_context: > - * Set one meta context. Simple means that reply must contain zero (not > - * negotiated) or one (negotiated) contexts. More contexts would be considered > - * as a protocol error. It's also implied that meta-data query equals queried > - * context name, so, if server replies with something different than @context, > - * it is considered an error too. > - * return 1 for successful negotiation, context_id is set > + * Request the server to set the meta context for export @info->name > + * using @info->x_dirty_bitmap with a fallback to "base:allocation", > + * setting @info->context_id to the resulting id. Fail if the server > + * responds with more than one context or with a context different > + * than the query. > + * return 1 for successful negotiation, > * 0 if operation is unsupported, > * -1 with errp set for any other error > */ > static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > - const char *export, > - const char *context, > - uint32_t *context_id, > + NBDExportInfo *info, > Error **errp) > { > + /* > + * TODO: Removing the x_dirty_bitmap hack will mean refactoring > + * this function to request and store ids for multiple contexts > + * (both base:allocation and a dirty bitmap), at which point this > + * function should lose the term _simple. > + */ > int ret; > NBDOptionReply reply; > - uint32_t received_id = 0; > + const char *context = info->x_dirty_bitmap ?: "base:allocation"; > bool received = false; > - uint32_t export_len = strlen(export); > + 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 */ > @@ -655,9 +659,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > char *data = g_malloc(data_len); > char *p = data; > > - trace_nbd_opt_meta_request(context, export); > + trace_nbd_opt_meta_request(context, info->name); > stl_be_p(p, export_len); > - memcpy(p += sizeof(export_len), export, 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); > @@ -683,7 +687,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > if (reply.type == NBD_REP_META_CONTEXT) { > char *name; > > - if (reply.length != sizeof(received_id) + context_len) { > + if (reply.length != sizeof(info->context_id) + context_len) { > error_setg(errp, "Failed to negotiate meta context '%s', server " > "answered with unexpected length %" PRIu32, context, > reply.length); > @@ -691,12 +695,13 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > return -1; > } > > - if (nbd_read(ioc, &received_id, sizeof(received_id), errp) < 0) { here you may set info->context_id on failure path. Looks like no harm in it (we use info->base_allocation and on the other hand, zero is possible id), so I'm not against. I just try to keep "no change on fail" semantics whenever possible, and don't care, is it really needed or not. Do you have an opinion on this topic, or may be some general agreements exists on it? ok, in this case it looks like a kind of overlogic, and, anyway, agree with Richard that now the refactoring is very clear: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
diff --git a/include/block/nbd.h b/include/block/nbd.h index 65feff8ba96..ae5fe28f486 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -276,7 +276,7 @@ struct NBDExportInfo { uint32_t opt_block; uint32_t max_block; - uint32_t meta_base_allocation_id; + uint32_t context_id; }; typedef struct NBDExportInfo NBDExportInfo; diff --git a/block/nbd-client.c b/block/nbd-client.c index 417971d8b05..608b578e1d3 100644 --- a/block/nbd-client.c +++ b/block/nbd-client.c @@ -246,11 +246,11 @@ static int nbd_parse_blockstatus_payload(NBDClientSession *client, } context_id = payload_advance32(&payload); - if (client->info.meta_base_allocation_id != context_id) { + if (client->info.context_id != context_id) { error_setg(errp, "Protocol error: unexpected context id %d for " "NBD_REPLY_TYPE_BLOCK_STATUS, when negotiated context " "id is %d", context_id, - client->info.meta_base_allocation_id); + client->info.context_id); return -EINVAL; } diff --git a/nbd/client.c b/nbd/client.c index 7462fa5ae0e..bcccd5f555e 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -628,26 +628,30 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc, } /* nbd_negotiate_simple_meta_context: - * Set one meta context. Simple means that reply must contain zero (not - * negotiated) or one (negotiated) contexts. More contexts would be considered - * as a protocol error. It's also implied that meta-data query equals queried - * context name, so, if server replies with something different than @context, - * it is considered an error too. - * return 1 for successful negotiation, context_id is set + * Request the server to set the meta context for export @info->name + * using @info->x_dirty_bitmap with a fallback to "base:allocation", + * setting @info->context_id to the resulting id. Fail if the server + * responds with more than one context or with a context different + * than the query. + * return 1 for successful negotiation, * 0 if operation is unsupported, * -1 with errp set for any other error */ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, - const char *export, - const char *context, - uint32_t *context_id, + NBDExportInfo *info, Error **errp) { + /* + * TODO: Removing the x_dirty_bitmap hack will mean refactoring + * this function to request and store ids for multiple contexts + * (both base:allocation and a dirty bitmap), at which point this + * function should lose the term _simple. + */ int ret; NBDOptionReply reply; - uint32_t received_id = 0; + const char *context = info->x_dirty_bitmap ?: "base:allocation"; bool received = false; - uint32_t export_len = strlen(export); + 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 */ @@ -655,9 +659,9 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, char *data = g_malloc(data_len); char *p = data; - trace_nbd_opt_meta_request(context, export); + trace_nbd_opt_meta_request(context, info->name); stl_be_p(p, export_len); - memcpy(p += sizeof(export_len), export, 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); @@ -683,7 +687,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, if (reply.type == NBD_REP_META_CONTEXT) { char *name; - if (reply.length != sizeof(received_id) + context_len) { + if (reply.length != sizeof(info->context_id) + context_len) { error_setg(errp, "Failed to negotiate meta context '%s', server " "answered with unexpected length %" PRIu32, context, reply.length); @@ -691,12 +695,13 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, return -1; } - if (nbd_read(ioc, &received_id, sizeof(received_id), errp) < 0) { + if (nbd_read(ioc, &info->context_id, sizeof(info->context_id), + errp) < 0) { return -1; } - received_id = be32_to_cpu(received_id); + info->context_id = be32_to_cpu(info->context_id); - reply.length -= sizeof(received_id); + reply.length -= sizeof(info->context_id); name = g_malloc(reply.length + 1); if (nbd_read(ioc, name, reply.length, errp) < 0) { g_free(name); @@ -713,7 +718,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, } g_free(name); - trace_nbd_opt_meta_reply(context, received_id); + trace_nbd_opt_meta_reply(context, info->context_id); received = true; /* receive NBD_REP_ACK */ @@ -742,12 +747,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, return -1; } - if (received) { - *context_id = received_id; - return 1; - } - - return 0; + return received; } int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, @@ -846,10 +846,7 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, } if (info->structured_reply && base_allocation) { - result = nbd_negotiate_simple_meta_context( - ioc, info->name, - info->x_dirty_bitmap ?: "base:allocation", - &info->meta_base_allocation_id, errp); + result = nbd_negotiate_simple_meta_context(ioc, info, errp); if (result < 0) { goto fail; }
Pass 'info' instead of three separate parameters related to info, when requesting the server to set the meta context. Update the NBDExportInfo struct to rename the received id field to match the fact that we are currently overloading the field to match whatever context the user supplied through the x-dirty-bitmap hack, as well as adding a TODO comment to remind future patches about a desire to request two contexts at once. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: split patch into easier-to-review pieces [Rich, Vladimir] rename NBDExportInfo meta_base_allocation_id [Vladimir] --- include/block/nbd.h | 2 +- block/nbd-client.c | 4 ++-- nbd/client.c | 53 +++++++++++++++++++++------------------------ 3 files changed, 28 insertions(+), 31 deletions(-)