Re: [Qemu-devel] [PATCH v3 14/19] nbd/client: Pull out oldstyle size determination
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 >> Message-Id: <20181215135324.152629-18-ebl...@redhat.com> >> Reviewed-by: Richard W.M. Jones > > Reviewed-by: Vladimir Sementsov-Ogievskiy > > 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 14/19] nbd/client: Pull out oldstyle size determination
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 -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH v3 14/19] nbd/client: Pull out oldstyle size determination
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
> Message-Id: <20181215135324.152629-18-ebl...@redhat.com>
> Reviewed-by: Richard W.M. Jones
Reviewed-by: Vladimir Sementsov-Ogievskiy
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, >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, , sizeof(oldflags), errp) < 0) {
> +error_prepend(errp, "Failed to read export flags: ");
> +return -EINVAL;
> +}
> +oldflags = be32_to_cpu(oldflags);
> +if (oldflags & ~0x) {
> +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, >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, , sizeof(oldflags), errp) < 0) {
> -error_prepend(errp, "Failed to read export flags: ");
> -return -EINVAL;
> -}
> -oldflags = be32_to_cpu(oldflags);
> -if (oldflags & ~0x) {
> -error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
> -return -EINVAL;
> -}
> -info->flags = oldflags;
> break;
> default:
> return result;
>
--
Best regards,
Vladimir
[Qemu-devel] [PATCH v3 14/19] nbd/client: Pull out oldstyle size determination
Another refactoring creating nbd_negotiate_finish_oldstyle()
for further reuse during 'qemu-nbd --list'.
Signed-off-by: Eric Blake
Message-Id: <20181215135324.152629-18-ebl...@redhat.com>
Reviewed-by: Richard W.M. Jones
---
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, >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, , sizeof(oldflags), errp) < 0) {
+error_prepend(errp, "Failed to read export flags: ");
+return -EINVAL;
+}
+oldflags = be32_to_cpu(oldflags);
+if (oldflags & ~0x) {
+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, >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, , sizeof(oldflags), errp) < 0) {
-error_prepend(errp, "Failed to read export flags: ");
-return -EINVAL;
-}
-oldflags = be32_to_cpu(oldflags);
-if (oldflags & ~0x) {
-error_setg(errp, "Unexpected export flags %0x" PRIx32, oldflags);
-return -EINVAL;
-}
-info->flags = oldflags;
break;
default:
return result;
--
2.20.1
