Re: [Qemu-devel] [PATCH 10/11] block: Allow omitting the file name when using driver-specific options

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4028 bytes --]

On 03/18/2013 11:24 AM, Kevin Wolf wrote:
> After this patch, using -drive with an empty file name continues to open
> the file if driver-specific options are used. If no driver-specific
> options are specified, the semantics stay as it was: It defines a drive
> without an inserted medium.
> 
> In order to achieve this, bdrv_open() must be made safe to work with a
> NULL filename parameter. The assumption that is made is that only block
> drivers which implement bdrv_parse_filename() support using driver
> specific options and could therefore work without a filename. These
> drivers must make sure to cope with NULL in their implementation of
> .bdrv_open() (this is only NBD for now). For all other drivers, the
> block layer code will make sure to error out before calling into their
> code - they can't possibly work without a filename.
> 
> Now an NBD connection can be opened like this:
> 
>   qemu-system-x86_64 -drive file.driver=nbd,file.port=1234,file.host=::1

Slick.

> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c                   | 49 +++++++++++++++++++++++++++++++++++++++--------
>  block/nbd.c               |  1 -
>  blockdev.c                | 10 +++++++---
>  include/block/block_int.h |  3 +++
>  4 files changed, 51 insertions(+), 12 deletions(-)

> +++ b/block/nbd.c
> @@ -174,7 +174,6 @@ static void nbd_parse_filename(const char *filename, QDict *options,
>  
>      /* extract the host_spec - fail if it's not nbd:... */
>      if (!strstart(file, "nbd:", &host_spec)) {
> -        error_setg(errp, "File name string for NBD must start with 'nbd:'");
>          goto out;
>      }

Is this really right?  The code allows me to pass both file= and
file.driver= at once; so what if I pass -drive file.driver=nbd,file=xyz.
 Prior to this patch, I'd get a nice message about file=xyz not starting
with nbd:, but now there is a silent failure.

I think it might be better if you keep the error_setg(), and instead
rewrite the if on the previous line:

if (file && !strstart(file, "nbd:", &host_spec)) {

Also, since it looks like the code allows me to pass both file.driver=
and file= at once, if I pass both pieces of information, is there any
sanity checking that the two are identical, and/or should we error out
and declare that if driver options are used then nbd requires a NULL
filename?


> @@ -697,10 +701,10 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>      if (ret < 0) {
>          if (ret == -EMEDIUMTYPE) {
>              error_report("could not open disk image %s: not in %s format",
> -                         file, drv->format_name);
> +                         file ?: dinfo->id, drv->format_name);

You're not the first person to use this gcc extension, but the more
instances we add, the harder it will be to scrub them back out if
someone ever insists on portability to another compiler.

>          } else {
>              error_report("could not open disk image %s: %s",
> -                         file, strerror(-ret));
> +                         file ?: dinfo->id, strerror(-ret));
>          }
>          goto err;
>      }
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 1b06a11..0986a2d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -75,6 +75,9 @@ struct BlockDriver {
>      int instance_size;
>      int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
>      int (*bdrv_probe_device)(const char *filename);
> +
> +    /* Any driver implementing this callback is expected to be able to handle
> +     * NULL file names in its .bdrv_open() implementation */
>      void (*bdrv_parse_filename)(const char *filename, QDict *options, Error **errp);
>  
>      /* For handling image reopen for split or non-split files */
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]