[Qemu-devel] [PATCH v1.5] nbd: Fix filename generation

[Qemu-devel] [PATCH v1.5] nbd: Fix filename generation

[Max Reitz]

Export names may be used with nbd+unix, too, fix nbd_refresh_filename()
accordingly. Also, for nbd+tcp, the documented path schema is
"nbd://host[:port]/export", so use it. Furthermore, as can be seen from
that schema, the port is optional.

That makes six single cases for how the filename can be formatted; it is
not easy to generalize these cases without the resulting statement being
completely unreadable, thus there is simply one snprintf() per case.

Finally, taking the options from BDRVNBDState::socket_opts is wrong,
because those will not contain the export name. Just use
BlockDriverState::options instead.

Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
This is exactly the same patch as patch 1 from my series
"[PATCH 0/3] block: Fix filename generation for blkdebug and nbd",
except I added Paolo's R-b. The reason I split it off from the series is
that is an actual bug fix and therefore I want to get it merged
regardless of when the blkdebug patches are getting reviewed.
---
 block/nbd.c | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 89775e1..04cc845 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -342,30 +342,44 @@ static void nbd_attach_aio_context(BlockDriverState *bs,
 
 static void nbd_refresh_filename(BlockDriverState *bs)
 {
-    BDRVNBDState *s = bs->opaque;
     QDict *opts = qdict_new();
-    const char *path   = qemu_opt_get(s->socket_opts, "path");
-    const char *host   = qemu_opt_get(s->socket_opts, "host");
-    const char *port   = qemu_opt_get(s->socket_opts, "port");
-    const char *export = qemu_opt_get(s->socket_opts, "export");
+    const char *path   = qdict_get_try_str(bs->options, "path");
+    const char *host   = qdict_get_try_str(bs->options, "host");
+    const char *port   = qdict_get_try_str(bs->options, "port");
+    const char *export = qdict_get_try_str(bs->options, "export");
 
     qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nbd")));
 
-    if (path) {
+    if (path && export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd+unix:%s", path);
-        qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(path)));
-    } else if (export) {
+                 "nbd+unix:///%s?socket=%s", export, path);
+    } else if (path && !export) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd:%s:%s/%s", host, port, export);
-        qdict_put_obj(opts, "host",   QOBJECT(qstring_from_str(host)));
-        qdict_put_obj(opts, "port",   QOBJECT(qstring_from_str(port)));
-        qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(export)));
-    } else {
+                 "nbd+unix://?socket=%s", path);
+    } else if (!path && export && port) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nbd://%s:%s/%s", host, port, export);
+    } else if (!path && export && !port) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nbd://%s/%s", host, export);
+    } else if (!path && !export && port) {
+        snprintf(bs->exact_filename, sizeof(bs->exact_filename),
+                 "nbd://%s:%s", host, port);
+    } else if (!path && !export && !port) {
         snprintf(bs->exact_filename, sizeof(bs->exact_filename),
-                 "nbd:%s:%s", host, port);
+                 "nbd://%s", host);
+    }
+
+    if (path) {
+        qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(path)));
+    } else if (port) {
         qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(host)));
         qdict_put_obj(opts, "port", QOBJECT(qstring_from_str(port)));
+    } else {
+        qdict_put_obj(opts, "host", QOBJECT(qstring_from_str(host)));
+    }
+    if (export) {
+        qdict_put_obj(opts, "export", QOBJECT(qstring_from_str(export)));
     }
 
     bs->full_open_options = opts;
-- 
2.1.2

Re: [Qemu-devel] [PATCH v1.5] nbd: Fix filename generation

[Kevin Wolf]

Am 08.10.2014 um 19:55 hat Max Reitz geschrieben:
> Export names may be used with nbd+unix, too, fix nbd_refresh_filename()
> accordingly. Also, for nbd+tcp, the documented path schema is
> "nbd://host[:port]/export", so use it. Furthermore, as can be seen from
> that schema, the port is optional.
> 
> That makes six single cases for how the filename can be formatted; it is
> not easy to generalize these cases without the resulting statement being
> completely unreadable, thus there is simply one snprintf() per case.
> 
> Finally, taking the options from BDRVNBDState::socket_opts is wrong,
> because those will not contain the export name. Just use
> BlockDriverState::options instead.
> 
> Reported-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> This is exactly the same patch as patch 1 from my series
> "[PATCH 0/3] block: Fix filename generation for blkdebug and nbd",
> except I added Paolo's R-b. The reason I split it off from the series is
> that is an actual bug fix and therefore I want to get it merged
> regardless of when the blkdebug patches are getting reviewed.
> ---
>  block/nbd.c | 44 +++++++++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 89775e1..04cc845 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -342,30 +342,44 @@ static void nbd_attach_aio_context(BlockDriverState *bs,
>  
>  static void nbd_refresh_filename(BlockDriverState *bs)
>  {
> -    BDRVNBDState *s = bs->opaque;
>      QDict *opts = qdict_new();
> -    const char *path   = qemu_opt_get(s->socket_opts, "path");
> -    const char *host   = qemu_opt_get(s->socket_opts, "host");
> -    const char *port   = qemu_opt_get(s->socket_opts, "port");
> -    const char *export = qemu_opt_get(s->socket_opts, "export");
> +    const char *path   = qdict_get_try_str(bs->options, "path");
> +    const char *host   = qdict_get_try_str(bs->options, "host");
> +    const char *port   = qdict_get_try_str(bs->options, "port");
> +    const char *export = qdict_get_try_str(bs->options, "export");
>  
>      qdict_put_obj(opts, "driver", QOBJECT(qstring_from_str("nbd")));
>  
> -    if (path) {
> +    if (path && export) {
>          snprintf(bs->exact_filename, sizeof(bs->exact_filename),
> -                 "nbd+unix:%s", path);
> -        qdict_put_obj(opts, "path", QOBJECT(qstring_from_str(path)));
> -    } else if (export) {
> +                 "nbd+unix:///%s?socket=%s", export, path);

We really need URL encoding for all of the strings we're putting into
the URL here and in the other cases in this function. It's a preexisting,
independent problem, though, so this patch is still a strict
improvement.

Thanks, applied to the block branch.

Kevin