| Message ID | 1363627441-8297-11-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 */ >
Am 20.03.2013 um 03:27 hat Eric Blake geschrieben: > > +++ 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)) { In fact, the additional check isn't even necessary because the function is only called for file != NULL in the first place. I've put the error back. > 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? I've added another patch that checks that only one of host/port/filename is specified. Kevin
diff --git a/block.c b/block.c index abe6bd5..ae46ca2 100644 --- a/block.c +++ b/block.c @@ -688,7 +688,11 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, bdrv_enable_copy_on_read(bs); } - pstrcpy(bs->filename, sizeof(bs->filename), filename); + if (filename != NULL) { + pstrcpy(bs->filename, sizeof(bs->filename), filename); + } else { + bs->filename[0] = '\0'; + } if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv)) { return -ENOTSUP; @@ -708,6 +712,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, bdrv_swap(file, bs); ret = 0; } else { + assert(drv->bdrv_parse_filename || filename != NULL); ret = drv->bdrv_file_open(bs, filename, options, open_flags); } } else { @@ -727,6 +732,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, #ifndef _WIN32 if (bs->is_temporary) { + assert(filename != NULL); unlink(filename); } #endif @@ -753,14 +759,9 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, { BlockDriverState *bs; BlockDriver *drv; + const char *drvname; int ret; - drv = bdrv_find_protocol(filename); - if (!drv) { - QDECREF(options); - return -ENOENT; - } - /* NULL means an empty set of options */ if (options == NULL) { options = qdict_new(); @@ -770,7 +771,26 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, bs->options = options; options = qdict_clone_shallow(options); - if (drv->bdrv_parse_filename) { + /* Find the right block driver */ + drvname = qdict_get_try_str(options, "driver"); + if (drvname) { + drv = bdrv_find_whitelisted_format(drvname); + qdict_del(options, "driver"); + } else if (filename) { + drv = bdrv_find_protocol(filename); + } else { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "Must specify either driver or file"); + drv = NULL; + } + + if (!drv) { + ret = -ENOENT; + goto fail; + } + + /* Parse the filename and open it */ + if (drv->bdrv_parse_filename && filename) { Error *local_err = NULL; drv->bdrv_parse_filename(filename, options, &local_err); if (error_is_set(&local_err)) { @@ -779,6 +799,12 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename, ret = -EINVAL; goto fail; } + } else if (!drv->bdrv_parse_filename && !filename) { + qerror_report(ERROR_CLASS_GENERIC_ERROR, + "The '%s' block driver requires a file name", + drv->format_name); + ret = -EINVAL; + goto fail; } ret = bdrv_open_common(bs, NULL, filename, options, flags, drv); @@ -899,6 +925,13 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options, QEMUOptionParameter *create_options; char backing_filename[PATH_MAX]; + if (qdict_size(options) != 0) { + error_report("Can't use snapshot=on with driver-specific options"); + ret = -EINVAL; + goto fail; + } + assert(filename != NULL); + /* if snapshot, we create a temporary backing file and open it instead of opening 'filename' directly */ diff --git a/block/nbd.c b/block/nbd.c index 9858f06..218df6a 100644 --- a/block/nbd.c +++ 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; } diff --git a/blockdev.c b/blockdev.c index 09f76b7..a0b2fb4 100644 --- a/blockdev.c +++ b/blockdev.c @@ -658,7 +658,11 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type) abort(); } if (!file || !*file) { - return dinfo; + if (qdict_size(bs_opts)) { + file = NULL; + } else { + return dinfo; + } } if (snapshot) { /* always use cache=unsafe with snapshot */ @@ -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); } 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 */
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 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(-)