Message ID | 20190112175812.27068-15-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Series | nbd: add qemu-nbd --list | expand |
12.01.2019 20:58, Eric Blake wrote: > Another refactoring creating nbd_negotiate_finish_oldstyle() > for further reuse during 'qemu-nbd --list'. > > Signed-off-by: Eric Blake <eblake@redhat.com> > Message-Id: <20181215135324.152629-18-eblake@redhat.com> > Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Hmm, looking at this, I found that nbd_start_negotiate() has very unusual semantics for @zeroes field, without any comment about it: It must be set to true before calling, and nbd_start_negotiate may set it to true. I think better is not to rely on caller initialization and set zeroes to true at the beginning of nbd_start_negotiate(). > --- > nbd/client.c | 49 ++++++++++++++++++++++++++++++++----------------- > 1 file changed, 32 insertions(+), 17 deletions(-) > > diff --git a/nbd/client.c b/nbd/client.c > index 5053433ea5e..620fbb5ef01 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -818,7 +818,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, > * Start the handshake to the server. After a positive return, the server > * is ready to accept additional NBD_OPT requests. > * Returns: negative errno: failure talking to server > - * 0: server is oldstyle, client must still parse export size > + * 0: server is oldstyle, must call nbd_negotiate_finish_oldstyle > * 1: server is newstyle, but can only accept EXPORT_NAME > * 2: server is newstyle, but lacks structured replies > * 3: server is newstyle and set up for structured replies > @@ -923,6 +923,36 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, > } > } > > +/* > + * nbd_negotiate_finish_oldstyle: > + * Populate @info with the size and export flags from an oldstyle server, > + * but does not consume 124 bytes of reserved zero padding. > + * Returns 0 on success, -1 with @errp set on failure > + */ > +static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo *info, > + Error **errp) > +{ > + uint32_t oldflags; > + > + if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) { > + error_prepend(errp, "Failed to read export length: "); > + return -EINVAL; > + } > + info->size = be64_to_cpu(info->size); > + > + if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) { > + error_prepend(errp, "Failed to read export flags: "); > + return -EINVAL; > + } > + oldflags = be32_to_cpu(oldflags); > + if (oldflags & ~0xffff) { > + error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags); > + return -EINVAL; > + } > + info->flags = oldflags; > + return 0; > +} > + > /* > * nbd_receive_negotiate: > * Connect to server, complete negotiation, and move into transmission phase. > @@ -936,7 +966,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, > int result; > bool zeroes = true; > bool base_allocation = info->base_allocation; > - uint32_t oldflags; > > assert(info->name); > trace_nbd_receive_negotiate_name(info->name); > @@ -1009,23 +1038,9 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, > error_setg(errp, "Server does not support non-empty export names"); > return -EINVAL; > } > - > - if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) { > - error_prepend(errp, "Failed to read export length: "); > + if (nbd_negotiate_finish_oldstyle(ioc, info, errp) < 0) { > return -EINVAL; > } > - info->size = be64_to_cpu(info->size); > - > - if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) { > - error_prepend(errp, "Failed to read export flags: "); > - return -EINVAL; > - } > - oldflags = be32_to_cpu(oldflags); > - if (oldflags & ~0xffff) { > - error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags); > - return -EINVAL; > - } > - info->flags = oldflags; > break; > default: > return result; >
15.01.2019 18:35, Vladimir Sementsov-Ogievskiy wrote:
> It must be set to true before calling, and nbd_start_negotiate may set it to true.
set it to false, I wanted to say
On 1/15/19 9:35 AM, Vladimir Sementsov-Ogievskiy wrote: > 12.01.2019 20:58, Eric Blake wrote: >> Another refactoring creating nbd_negotiate_finish_oldstyle() >> for further reuse during 'qemu-nbd --list'. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> Message-Id: <20181215135324.152629-18-eblake@redhat.com> >> Reviewed-by: Richard W.M. Jones <rjones@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > Hmm, looking at this, I found that nbd_start_negotiate() has very unusual semantics > for @zeroes field, without any comment about it: > > It must be set to true before calling, and nbd_start_negotiate may set it to true. > > I think better is not to rely on caller initialization and set zeroes to true at the > beginning of nbd_start_negotiate(). Makes sense; done in my local tree.
diff --git a/nbd/client.c b/nbd/client.c index 5053433ea5e..620fbb5ef01 100644 --- a/nbd/client.c +++ b/nbd/client.c @@ -818,7 +818,7 @@ static int nbd_negotiate_simple_meta_context(QIOChannel *ioc, * Start the handshake to the server. After a positive return, the server * is ready to accept additional NBD_OPT requests. * Returns: negative errno: failure talking to server - * 0: server is oldstyle, client must still parse export size + * 0: server is oldstyle, must call nbd_negotiate_finish_oldstyle * 1: server is newstyle, but can only accept EXPORT_NAME * 2: server is newstyle, but lacks structured replies * 3: server is newstyle and set up for structured replies @@ -923,6 +923,36 @@ static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, } } +/* + * nbd_negotiate_finish_oldstyle: + * Populate @info with the size and export flags from an oldstyle server, + * but does not consume 124 bytes of reserved zero padding. + * Returns 0 on success, -1 with @errp set on failure + */ +static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo *info, + Error **errp) +{ + uint32_t oldflags; + + if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) { + error_prepend(errp, "Failed to read export length: "); + return -EINVAL; + } + info->size = be64_to_cpu(info->size); + + if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) { + error_prepend(errp, "Failed to read export flags: "); + return -EINVAL; + } + oldflags = be32_to_cpu(oldflags); + if (oldflags & ~0xffff) { + error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags); + return -EINVAL; + } + info->flags = oldflags; + return 0; +} + /* * nbd_receive_negotiate: * Connect to server, complete negotiation, and move into transmission phase. @@ -936,7 +966,6 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, int result; bool zeroes = true; bool base_allocation = info->base_allocation; - uint32_t oldflags; assert(info->name); trace_nbd_receive_negotiate_name(info->name); @@ -1009,23 +1038,9 @@ int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds, error_setg(errp, "Server does not support non-empty export names"); return -EINVAL; } - - if (nbd_read(ioc, &info->size, sizeof(info->size), errp) < 0) { - error_prepend(errp, "Failed to read export length: "); + if (nbd_negotiate_finish_oldstyle(ioc, info, errp) < 0) { return -EINVAL; } - info->size = be64_to_cpu(info->size); - - if (nbd_read(ioc, &oldflags, sizeof(oldflags), errp) < 0) { - error_prepend(errp, "Failed to read export flags: "); - return -EINVAL; - } - oldflags = be32_to_cpu(oldflags); - if (oldflags & ~0xffff) { - error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags); - return -EINVAL; - } - info->flags = oldflags; break; default: return result;