From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55460) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YsWcD-0004tH-Co for qemu-devel@nongnu.org; Wed, 13 May 2015 09:22:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YsWcC-0000zp-3e for qemu-devel@nongnu.org; Wed, 13 May 2015 09:22:25 -0400 Message-ID: <55535003.2030000@redhat.com> Date: Wed, 13 May 2015 15:22:11 +0200 From: Max Reitz MIME-Version: 1.0 References: <1431105726-3682-1-git-send-email-kwolf@redhat.com> <1431105726-3682-25-git-send-email-kwolf@redhat.com> In-Reply-To: <1431105726-3682-25-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=iso-8859-15; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 24/34] block: Keep "driver" in bs->options List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: armbru@redhat.com, qemu-devel@nongnu.org On 08.05.2015 19:21, Kevin Wolf wrote: > Instead of passing a separate drv argument to bdrv_open_common(), just > make sure that a "driver" option is set in the QDict. This also means > that a "driver" entry is consistently present in bs->options now. > > This is another step towards keeping all options in the QDict (which is > the represenation of the blockdev-add QMP command). > > Signed-off-by: Kevin Wolf > --- > block.c | 54 ++++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 38 insertions(+), 16 deletions(-) > > diff --git a/block.c b/block.c > index 7c0c9db..ef39c74 100644 > --- a/block.c > +++ b/block.c > @@ -795,6 +795,11 @@ static QemuOptsList bdrv_runtime_opts = { > .type = QEMU_OPT_STRING, > .help = "Node name of the block device node", > }, > + { > + .name = "driver", > + .type = QEMU_OPT_STRING, > + .help = "Block driver to use for the node", > + }, > { /* end of list */ } > }, > }; > @@ -805,18 +810,31 @@ static QemuOptsList bdrv_runtime_opts = { > * Removes all processed options from *options. > */ > static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, > - QDict *options, int flags, BlockDriver *drv, Error **errp) > + QDict *options, int flags, Error **errp) > { > int ret, open_flags; > const char *filename; > + const char *driver_name = NULL; > const char *node_name = NULL; > QemuOpts *opts; > + BlockDriver *drv; > Error *local_err = NULL; > > - assert(drv != NULL); > assert(bs->file == NULL); > assert(options != NULL && bs->options != options); > > + opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort); > + qemu_opts_absorb_qdict(opts, options, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + ret = -EINVAL; > + goto fail_opts; > + } > + > + driver_name = qemu_opt_get(opts, "driver"); > + drv = bdrv_find_format(driver_name); > + assert(drv != NULL); > + > if (file != NULL) { > filename = file->filename; > } else { The "return -EINVAL" in "if (drv->bdrv_needs_filename && !filename) {" should be changed to "ret = -EINVAL; goto fail_opts;" now. With that changed: Reviewed-by: Max Reitz > @@ -831,14 +849,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, > > trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name); > > - opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort); > - qemu_opts_absorb_qdict(opts, options, &local_err); > - if (local_err) { > - error_propagate(errp, local_err); > - ret = -EINVAL; > - goto fail_opts; > - } > - > node_name = qemu_opt_get(opts, "node-name"); > bdrv_assign_node_name(bs, node_name, &local_err); > if (local_err) { > @@ -1405,12 +1415,15 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, > goto fail; > } > > + bs->open_flags = flags; > + bs->options = options; > + options = qdict_clone_shallow(options); > + > /* Find the right image format driver */ > drv = NULL; > drvname = qdict_get_try_str(options, "driver"); > if (drvname) { > drv = bdrv_find_format(drvname); > - qdict_del(options, "driver"); > if (!drv) { > error_setg(errp, "Unknown driver: '%s'", drvname); > ret = -EINVAL; > @@ -1425,10 +1438,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, > flags &= ~BDRV_O_PROTOCOL; > } > > - bs->open_flags = flags; > - bs->options = options; > - options = qdict_clone_shallow(options); > - > /* Open image file without format layer */ > if ((flags & BDRV_O_PROTOCOL) == 0) { > if (flags & BDRV_O_RDWR) { > @@ -1455,6 +1464,19 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, > if (ret < 0) { > goto fail; > } > + /* > + * This option update would logically belong in bdrv_fill_options(), > + * but we first need to open bs->file for the probing to work, while > + * opening bs->file already requires the (mostly) final set of options > + * so that cache mode etc. can be inherited. > + * > + * Adding the driver later is somewhat ugly, but it's not an option > + * that would ever be inherited, so it's correct. We just need to make > + * sure to update both bs->options (which has the full effective > + * options for bs) and options (which has file.* already removed). > + */ > + qdict_put(bs->options, "driver", qstring_from_str(drv->format_name)); > + qdict_put(options, "driver", qstring_from_str(drv->format_name)); > } else if (!drv) { > error_setg(errp, "Must specify either driver or file"); > ret = -EINVAL; > @@ -1462,7 +1484,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, > } > > /* Open the image */ > - ret = bdrv_open_common(bs, file, options, flags, drv, &local_err); > + ret = bdrv_open_common(bs, file, options, flags, &local_err); > if (ret < 0) { > goto fail; > }