| Message ID | 1259596471-14576-1-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 > >
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
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
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;
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(-)