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

[Kevin Wolf <kwolf@redhat.com>]

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