Re: [Qemu-devel] [PATCH] block: introduce BlockDriver.bdrv_needs_filename to enable some drivers.

[Kevin Wolf]

Am 20.09.2013 um 22:24 hat Benoît Canet geschrieben:
> Some drivers will have driver specifics options but no filename.
> This new bool allow the block layer to treat them correctly.
> 
> The first driver of this type will be the quorum driver.
> 
> Signed-off-by: Benoit Canet <address@hidden>
> ---
>  block.c                   | 6 ++++--
>  block/bochs.c             | 1 +
>  block/cloop.c             | 1 +
>  block/cow.c               | 1 +
>  block/dmg.c               | 1 +
>  block/gluster.c           | 4 ++++
>  block/iscsi.c             | 1 +
>  block/parallels.c         | 1 +
>  block/qcow.c              | 1 +
>  block/qcow2.c             | 1 +
>  block/qed.c               | 1 +
>  block/raw-posix.c         | 5 +++++
>  block/raw-win32.c         | 2 ++
>  block/raw_bsd.c           | 1 +
>  block/rbd.c               | 1 +
>  block/sheepdog.c          | 3 +++
>  block/vdi.c               | 1 +
>  block/vhdx.c              | 1 +
>  block/vmdk.c              | 1 +
>  block/vpc.c               | 1 +
>  include/block/block_int.h | 1 +
>  21 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 827549c..88099a1 100644
> --- a/block.c
> +++ b/block.c
> @@ -760,7 +760,8 @@ static int bdrv_open_common(BlockDriverState *bs, 
> BlockDriverState *file,
>      /* Open the image, either directly or using a protocol */
>      if (drv->bdrv_file_open) {
>          assert(file == NULL);
> -        assert(drv->bdrv_parse_filename || filename != NULL);
> +        assert(drv->bdrv_parse_filename || !drv->bdrv_needs_filename ||
> +               filename != NULL);

Doesn't drv->bdrv_parse_filename imply !drv->bdrv_needs_filename? (If
I'm not mistaken, it is only checked here anyway because there was no
bdrv_needs_filename yet.) I think it's enough to check the latter.

>          ret = drv->bdrv_file_open(bs, options, open_flags);
>      } else {
>          if (file == NULL) {
> @@ -870,7 +871,8 @@ int bdrv_file_open(BlockDriverState **pbs, const char 
> *filename,
>              goto fail;
>          }
>          qdict_del(options, "filename");
> -    } else if (!drv->bdrv_parse_filename && !filename) {
> +    } else if (!drv->bdrv_parse_filename && drv->bdrv_needs_filename &&
> +               !filename) {
>          qerror_report(ERROR_CLASS_GENERIC_ERROR,
>                        "The '%s' block driver requires a file name",
>                        drv->format_name);
> diff --git a/block/bochs.c b/block/bochs.c
> index d7078c0..72a73aa 100644
> --- a/block/bochs.c
> +++ b/block/bochs.c
> @@ -237,6 +237,7 @@ static void bochs_close(BlockDriverState *bs)
>  static BlockDriver bdrv_bochs = {
>      .format_name     = "bochs",
>      .instance_size   = sizeof(BDRVBochsState),
> +    .bdrv_needs_filename = true,

This looks wrong.

Format drivers which only implement .bdrv_open() (and not
.bdrv_file_open()) aren't even passed a filename. It would also be wrong
because they can be used above non-file protocols.

Do you get failures if you remove this line from format drivers? If so,
we need to look into that and fix it.

Kevin