[Qemu-devel] [PATCH] raw: Use the right host device driver for open/create

[Qemu-devel] [PATCH] raw: Use the right host device driver for open/create

[Kevin Wolf]

Users don't expect that they need to specify host_device/cdrom/floppy when
"creating" an image on a block device or converting with an device as target.
Currently creating as raw leads to 'Error while formatting' whereas using as
raw just works.

With this patch raw is accepted for both files and host devices. For devices
the block driver is transparently changed to host_*.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c           |    2 +-
 block/raw-posix.c |   16 ++++++++++++++++
 block_int.h       |    1 +
 qemu-img.c        |    1 +
 4 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/block.c b/block.c
index 6fdabff..377747f 100644
--- a/block.c
+++ b/block.c
@@ -283,7 +283,7 @@ static BlockDriver *find_protocol(const char *filename)
  * Detect host devices. By convention, /dev/cdrom[N] is always
  * recognized as a host CDROM.
  */
-static BlockDriver *find_hdev_driver(const char *filename)
+BlockDriver *find_hdev_driver(const char *filename)
 {
     int score_max = 0, score;
     BlockDriver *drv = NULL, *d;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 3763d0c..54b127a 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -206,6 +206,14 @@ static int raw_open(BlockDriverState *bs, const char *filename, int flags)
 {
     BDRVRawState *s = bs->opaque;
     int open_flags = 0;
+    BlockDriver *drv;
+
+    /* If it's a block device, we want to use the right host_* driver */
+    drv = find_hdev_driver(filename);
+    if (drv) {
+        bs->drv = drv;
+        return drv->bdrv_open(bs, filename, flags);
+    }
 
     s->type = FTYPE_FILE;
     if (flags & BDRV_O_CREAT)
@@ -710,6 +718,14 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
     int fd;
     int result = 0;
     int64_t total_size = 0;
+    BlockDriver *drv;
+
+    /* If it's a block device, we want to use the right host_* driver */
+    drv = find_hdev_driver(filename);
+    if (drv) {
+        return drv->bdrv_create(filename, options);
+    }
+
 
     /* Read out options */
     while (options && options->name) {
diff --git a/block_int.h b/block_int.h
index a7ac1f6..d96f182 100644
--- a/block_int.h
+++ b/block_int.h
@@ -185,6 +185,7 @@ struct BlockDriverAIOCB {
     BlockDriverAIOCB *next;
 };
 
+BlockDriver *find_hdev_driver(const char *filename);
 void get_tmp_filename(char *filename, int size);
 
 void *qemu_aio_get(AIOPool *pool, BlockDriverState *bs,
diff --git a/qemu-img.c b/qemu-img.c
index f19c644..8284d96 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -651,6 +651,7 @@ static int img_convert(int argc, char **argv)
     }
 
     out_bs = bdrv_new_open(out_filename, out_fmt);
+    drv = out_bs->drv;
 
     bs_i = 0;
     bs_offset = 0;
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH] raw: Use the right host device driver for open/create

[Christoph Hellwig]

On Mon, Nov 30, 2009 at 04:54:31PM +0100, Kevin Wolf wrote:
> Users don't expect that they need to specify host_device/cdrom/floppy when
> "creating" an image on a block device or converting with an device as target.
> Currently creating as raw leads to 'Error while formatting' whereas using as
> raw just works.
> 
> With this patch raw is accepted for both files and host devices. For devices
> the block driver is transparently changed to host_*.

I agree that we should allow raw to also cover host devices, but I don't
like the implementation very much.  Beeing used to specify the format is
pretty much the only reason to have the name in the block driver anyway,
and looking it up in one is a bit of a layering violation.

I'd suggest to either allow multiple formats with the same name and
looping over them or some sort of alias property in the block driver to
also match the alias instead.

Re: [Qemu-devel] [PATCH] raw: Use the right host device driver for open/create

[Anthony Liguori]

Christoph Hellwig wrote:
> On Mon, Nov 30, 2009 at 04:54:31PM +0100, Kevin Wolf wrote:
>   
>> Users don't expect that they need to specify host_device/cdrom/floppy when
>> "creating" an image on a block device or converting with an device as target.
>> Currently creating as raw leads to 'Error while formatting' whereas using as
>> raw just works.
>>
>> With this patch raw is accepted for both files and host devices. For devices
>> the block driver is transparently changed to host_*.
>>     
>
> I agree that we should allow raw to also cover host devices, but I don't
> like the implementation very much.  Beeing used to specify the format is
> pretty much the only reason to have the name in the block driver anyway,
> and looking it up in one is a bit of a layering violation.
>   

I agree.

> I'd suggest to either allow multiple formats with the same name and
> looping over them or some sort of alias property in the block driver to
> also match the alias instead.
>   
Regards,

Anthony Liguori
>
>   

Re: [Qemu-devel] [PATCH] raw: Use the right host device driver for open/create

[Kevin Wolf]

Am 30.11.2009 21:51, schrieb Christoph Hellwig:
> On Mon, Nov 30, 2009 at 04:54:31PM +0100, Kevin Wolf wrote:
>> Users don't expect that they need to specify host_device/cdrom/floppy when
>> "creating" an image on a block device or converting with an device as target.
>> Currently creating as raw leads to 'Error while formatting' whereas using as
>> raw just works.
>>
>> With this patch raw is accepted for both files and host devices. For devices
>> the block driver is transparently changed to host_*.
> 
> I agree that we should allow raw to also cover host devices, but I don't
> like the implementation very much.  Beeing used to specify the format is
> pretty much the only reason to have the name in the block driver anyway,
> and looking it up in one is a bit of a layering violation.

Ok, if you start talking about layering, we can have a fundamental
discussion on this topic and why the layering is broken anyway.
Logically, we have image formats like qcow2, VMDK and raw, and they are
stored in files, on CD-ROMs or general block devices. From a layering
perspective, it is wrong to include the latter in the raw format driver
in the first place.

> I'd suggest to either allow multiple formats with the same name and
> looping over them or some sort of alias property in the block driver to
> also match the alias instead.

I don't like taking the same name for multiple formats very much. You
get a bug report with a "raw" image and need to start guessing which of
the many raws could be meant.

Adding some kind of a "parent driver" field to the driver definition and
probing for which of the children is used could work though (it would
basically mean moving the code of this patch into block.c). I'm not sure
if such an abstraction is needed when there is only one user anyway
(same argument as yours with AIO, even though there are two users), but
I'm not opposed to it. It just won't help much in getting the layering
right.

In the end, I don't care too much which way it is implemented, but I do
want to present the right user interface in 0.12.

Kevin

Re: [Qemu-devel] [PATCH] raw: Use the right host device driver for open/create

[Kevin Wolf]

So the proposed patch is too hacky, but nobody likes to discuss what the
right way is? What about 0.12? Are we just going to provide the current
bad user interface?

Kevin