On 06/11/2014 08:04 AM, Kevin Wolf wrote: > The idea of bdrv_fill_options() is to convert every parameter for > opening images, in particular the filename and flags, to entries in the > options QDict. > > This patch starts with moving the filename parsing and driver probing > part from bdrv_file_open() to the new function. > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > --- > block.c | 116 > ++++++++++++++++++++++++++++++++++++++++------------------------ > 1 file changed, 73 insertions(+), 43 deletions(-) > > + * Fills in default options for opening images and converts the legacy > + * filename/flags pair to option QDict entries. > */ > -static int bdrv_file_open(BlockDriverState *bs, const char *filename, > - QDict **options, int flags, Error **errp) > +static int bdrv_fill_options(QDict **options, const char *filename, > + Error **errp)
The comment mentions filename/flags, but I only see filename as a
parameter. Is this something changed later in the series?
> {
> - BlockDriver *drv;
> const char *drvname;
> bool parse_filename = false;
> Error *local_err = NULL;
> - int ret;
> + BlockDriver *drv;
>
> /* Fetch the file name from the options QDict if necessary */
> - if (!filename) {
> - filename = qdict_get_try_str(*options, "filename");
> - } else if (filename && !qdict_haskey(*options, "filename")) {
> - qdict_put(*options, "filename", qstring_from_str(filename));
> - parse_filename = true;
> - } else {
> - error_setg(errp, "Can't specify 'file' and 'filename' options at the
> "
> - "same time");
> - ret = -EINVAL;
> - goto fail;
> + if (filename) {
> + if (filename && !qdict_haskey(*options, "filename")) {
The 'filename &&' is redundant.
> + qdict_put(*options, "filename", qstring_from_str(filename));
> + parse_filename = true;
> + } else {
> + error_setg(errp, "Can't specify 'file' and 'filename' options at
> "
> + "the same time");
> + return -EINVAL;
> + }
> }
>
> /* Find the right block driver */
> + filename = qdict_get_try_str(*options, "filename");
> drvname = qdict_get_try_str(*options, "driver");
> - if (drvname) {
> - drv = bdrv_find_format(drvname);
> - if (!drv) {
> - error_setg(errp, "Unknown driver '%s'", drvname);
> - }
> - qdict_del(*options, "driver");
> - } else if (filename) {
> - drv = bdrv_find_protocol(filename, parse_filename);
> - if (!drv) {
> - error_setg(errp, "Unknown protocol");
> +
> + if (!drvname) {
> + if (filename) {
> + drv = bdrv_find_protocol(filename, parse_filename);
This assigns drv...
> + if (!drv) {
> + error_setg(errp, "Unknown protocol");
> + return -EINVAL;
> + }
> +
> + drvname = drv->format_name;
> + qdict_put(*options, "driver", qstring_from_str(drvname));
> + } else {
> + error_setg(errp, "Must specify either driver or file");
> + return -EINVAL;
> }
> - } else {
> - error_setg(errp, "Must specify either driver or file");
> - drv = NULL;
> }
>
> + drv = bdrv_find_format(drvname);
...and this does it again. Should this second assignment be inside an
else{}?
> +static int bdrv_file_open(BlockDriverState *bs, const char *filename,
> + QDict **options, int flags, Error **errp)
> +
> + drv = bdrv_find_format(drvname);
> + if (!drv) {
> + error_setg(errp, "Unknown driver '%s'", drvname);
> + ret = -ENOENT;
> + goto fail;
> + }
Isn't this error check redundant with the one already done in
bdrv_fill_options()? You could probably just use assert(drv)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
