| Message ID | 1402495503-4722-2-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <kwolf@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)
The Wednesday 11 Jun 2014 à 16:04:55 (+0200), 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 <kwolf@redhat.com> > --- > block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------------------ > 1 file changed, 73 insertions(+), 43 deletions(-) > > diff --git a/block.c b/block.c > index 17f763d..c5707e8 100644 > --- a/block.c > +++ b/block.c > @@ -1007,77 +1007,107 @@ free_and_fail: > } > > /* > - * Opens a file using a protocol (file, host_device, nbd, ...) > - * > - * options is an indirect pointer to a QDict of options to pass to the block > - * drivers, or pointer to NULL for an empty set of options. If this function > - * takes ownership of the QDict reference, it will set *options to NULL; > - * otherwise, it will contain unused/unrecognized options after this function > - * returns. Then, the caller is responsible for freeing it. If it intends to > - * reuse the QDict, QINCREF() should be called beforehand. > + * 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) > { > - 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 inner filename testing 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); > + 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"); Isn't it "Must specify either driver or _filename_ ? > + return -EINVAL; > } > - } else { > - error_setg(errp, "Must specify either driver or file"); > - drv = NULL; > } > > + drv = bdrv_find_format(drvname); > if (!drv) { > - /* errp has been set already */ > - ret = -ENOENT; > - goto fail; > + error_setg(errp, "Unknown driver '%s'", drvname); > + return -ENOENT; > } > > - /* Parse the filename and open it */ > + /* Driver-specific filename parsing */ > if (drv->bdrv_parse_filename && parse_filename) { > drv->bdrv_parse_filename(filename, *options, &local_err); > if (local_err) { > error_propagate(errp, local_err); > - ret = -EINVAL; > - goto fail; > + return -EINVAL; > } > > if (!drv->bdrv_needs_filename) { > qdict_del(*options, "filename"); > - } else { > - filename = qdict_get_str(*options, "filename"); > } > } > > + return 0; > +} > + > +/* > + * Opens a file using a protocol (file, host_device, nbd, ...) > + * > + * options is an indirect pointer to a QDict of options to pass to the block > + * drivers, or pointer to NULL for an empty set of options. If this function > + * takes ownership of the QDict reference, it will set *options to NULL; > + * otherwise, it will contain unused/unrecognized options after this function > + * returns. Then, the caller is responsible for freeing it. If it intends to > + * reuse the QDict, QINCREF() should be called beforehand. > + */ > +static int bdrv_file_open(BlockDriverState *bs, const char *filename, > + QDict **options, int flags, Error **errp) > +{ > + BlockDriver *drv; > + const char *drvname; > + Error *local_err = NULL; > + int ret; > + > + ret = bdrv_fill_options(options, filename, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return ret; Why not goto fail for consistency with the rest of the function ? > + } > + > + filename = qdict_get_try_str(*options, "filename"); > + drvname = qdict_get_str(*options, "driver"); > + > + drv = bdrv_find_format(drvname); > + if (!drv) { > + error_setg(errp, "Unknown driver '%s'", drvname); > + ret = -ENOENT; > + goto fail; > + } > + qdict_del(*options, "driver"); > + > + /* Open the file */ > if (!drv->bdrv_file_open) { Don't we need: qdict_del(*options, "filename"); Here so bdrv_open don't see an extra filename option ? > ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err); > *options = NULL; > -- > 1.8.3.1 > >
Am 12.06.2014 um 14:08 hat Benoît Canet geschrieben: > The Wednesday 11 Jun 2014 à 16:04:55 (+0200), 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 <kwolf@redhat.com> > > --- > > block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------------------ > > 1 file changed, 73 insertions(+), 43 deletions(-) > > > > diff --git a/block.c b/block.c > > index 17f763d..c5707e8 100644 > > --- a/block.c > > +++ b/block.c > > @@ -1007,77 +1007,107 @@ free_and_fail: > > } > > > > /* > > - * Opens a file using a protocol (file, host_device, nbd, ...) > > - * > > - * options is an indirect pointer to a QDict of options to pass to the block > > - * drivers, or pointer to NULL for an empty set of options. If this function > > - * takes ownership of the QDict reference, it will set *options to NULL; > > - * otherwise, it will contain unused/unrecognized options after this function > > - * returns. Then, the caller is responsible for freeing it. If it intends to > > - * reuse the QDict, QINCREF() should be called beforehand. > > + * 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) > > { > > - 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 inner filename testing is redundant. Will fix. > > + 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); > > + 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"); > > Isn't it "Must specify either driver or _filename_ ? This is only code motion, but changing it in a separate patch would make sense, I guess. > > + return -EINVAL; > > } > > - } else { > > - error_setg(errp, "Must specify either driver or file"); > > - drv = NULL; > > } > > > > + drv = bdrv_find_format(drvname); > > if (!drv) { > > - /* errp has been set already */ > > - ret = -ENOENT; > > - goto fail; > > + error_setg(errp, "Unknown driver '%s'", drvname); > > + return -ENOENT; > > } > > > > - /* Parse the filename and open it */ > > + /* Driver-specific filename parsing */ > > if (drv->bdrv_parse_filename && parse_filename) { > > drv->bdrv_parse_filename(filename, *options, &local_err); > > if (local_err) { > > error_propagate(errp, local_err); > > - ret = -EINVAL; > > - goto fail; > > + return -EINVAL; > > } > > > > if (!drv->bdrv_needs_filename) { > > qdict_del(*options, "filename"); > > - } else { > > - filename = qdict_get_str(*options, "filename"); > > } > > } > > > > + return 0; > > +} > > + > > +/* > > + * Opens a file using a protocol (file, host_device, nbd, ...) > > + * > > + * options is an indirect pointer to a QDict of options to pass to the block > > + * drivers, or pointer to NULL for an empty set of options. If this function > > + * takes ownership of the QDict reference, it will set *options to NULL; > > + * otherwise, it will contain unused/unrecognized options after this function > > + * returns. Then, the caller is responsible for freeing it. If it intends to > > + * reuse the QDict, QINCREF() should be called beforehand. > > + */ > > +static int bdrv_file_open(BlockDriverState *bs, const char *filename, > > + QDict **options, int flags, Error **errp) > > +{ > > + BlockDriver *drv; > > + const char *drvname; > > + Error *local_err = NULL; > > + int ret; > > + > > + ret = bdrv_fill_options(options, filename, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return ret; > Why not goto fail for consistency with the rest of the function ? Okay. Rather pointless and the function will be gone at the end of the series, but more consistent indeed. > > + } > > + > > + filename = qdict_get_try_str(*options, "filename"); > > + drvname = qdict_get_str(*options, "driver"); > > + > > + drv = bdrv_find_format(drvname); > > + if (!drv) { > > + error_setg(errp, "Unknown driver '%s'", drvname); > > + ret = -ENOENT; > > + goto fail; > > + } > > + qdict_del(*options, "driver"); > > + > > + /* Open the file */ > > if (!drv->bdrv_file_open) { > > Don't we need: > qdict_del(*options, "filename"); > Here so bdrv_open don't see an extra filename option ? I think it doesn't matter because bdrv_open() would put it back again. Do you have an example where it makes a difference? > > ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err); > > *options = NULL; Kevin
The Monday 23 Jun 2014 à 17:30:45 (+0200), Kevin Wolf wrote : > Am 12.06.2014 um 14:08 hat Benoît Canet geschrieben: > > The Wednesday 11 Jun 2014 à 16:04:55 (+0200), 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 <kwolf@redhat.com> > > > --- > > > block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------------------ > > > 1 file changed, 73 insertions(+), 43 deletions(-) > > > > > > diff --git a/block.c b/block.c > > > index 17f763d..c5707e8 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -1007,77 +1007,107 @@ free_and_fail: > > > } > > > > > > /* > > > - * Opens a file using a protocol (file, host_device, nbd, ...) > > > - * > > > - * options is an indirect pointer to a QDict of options to pass to the block > > > - * drivers, or pointer to NULL for an empty set of options. If this function > > > - * takes ownership of the QDict reference, it will set *options to NULL; > > > - * otherwise, it will contain unused/unrecognized options after this function > > > - * returns. Then, the caller is responsible for freeing it. If it intends to > > > - * reuse the QDict, QINCREF() should be called beforehand. > > > + * 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) > > > { > > > - 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 inner filename testing is redundant. > > Will fix. > > > > + 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); > > > + 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"); > > > > Isn't it "Must specify either driver or _filename_ ? > > This is only code motion, but changing it in a separate patch would make > sense, I guess. > > > > + return -EINVAL; > > > } > > > - } else { > > > - error_setg(errp, "Must specify either driver or file"); > > > - drv = NULL; > > > } > > > > > > + drv = bdrv_find_format(drvname); > > > if (!drv) { > > > - /* errp has been set already */ > > > - ret = -ENOENT; > > > - goto fail; > > > + error_setg(errp, "Unknown driver '%s'", drvname); > > > + return -ENOENT; > > > } > > > > > > - /* Parse the filename and open it */ > > > + /* Driver-specific filename parsing */ > > > if (drv->bdrv_parse_filename && parse_filename) { > > > drv->bdrv_parse_filename(filename, *options, &local_err); > > > if (local_err) { > > > error_propagate(errp, local_err); > > > - ret = -EINVAL; > > > - goto fail; > > > + return -EINVAL; > > > } > > > > > > if (!drv->bdrv_needs_filename) { > > > qdict_del(*options, "filename"); > > > - } else { > > > - filename = qdict_get_str(*options, "filename"); > > > } > > > } > > > > > > + return 0; > > > +} > > > + > > > +/* > > > + * Opens a file using a protocol (file, host_device, nbd, ...) > > > + * > > > + * options is an indirect pointer to a QDict of options to pass to the block > > > + * drivers, or pointer to NULL for an empty set of options. If this function > > > + * takes ownership of the QDict reference, it will set *options to NULL; > > > + * otherwise, it will contain unused/unrecognized options after this function > > > + * returns. Then, the caller is responsible for freeing it. If it intends to > > > + * reuse the QDict, QINCREF() should be called beforehand. > > > + */ > > > +static int bdrv_file_open(BlockDriverState *bs, const char *filename, > > > + QDict **options, int flags, Error **errp) > > > +{ > > > + BlockDriver *drv; > > > + const char *drvname; > > > + Error *local_err = NULL; > > > + int ret; > > > + > > > + ret = bdrv_fill_options(options, filename, &local_err); > > > + if (local_err) { > > > + error_propagate(errp, local_err); > > > + return ret; > > Why not goto fail for consistency with the rest of the function ? > > Okay. Rather pointless and the function will be gone at the end of the > series, but more consistent indeed. > > > > + } > > > + > > > + filename = qdict_get_try_str(*options, "filename"); > > > + drvname = qdict_get_str(*options, "driver"); > > > + > > > + drv = bdrv_find_format(drvname); > > > + if (!drv) { > > > + error_setg(errp, "Unknown driver '%s'", drvname); > > > + ret = -ENOENT; > > > + goto fail; > > > + } > > > + qdict_del(*options, "driver"); > > > + > > > + /* Open the file */ > > > if (!drv->bdrv_file_open) { > > > > Don't we need: > > qdict_del(*options, "filename"); > > Here so bdrv_open don't see an extra filename option ? > > I think it doesn't matter because bdrv_open() would put it back again. > Do you have an example where it makes a difference? No, I don't understand the code that much. I was just thinking outloud betting it would point to something. It's not :) > > > > ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err); > > > *options = NULL; > > Kevin
diff --git a/block.c b/block.c index 17f763d..c5707e8 100644 --- a/block.c +++ b/block.c @@ -1007,77 +1007,107 @@ free_and_fail: } /* - * Opens a file using a protocol (file, host_device, nbd, ...) - * - * options is an indirect pointer to a QDict of options to pass to the block - * drivers, or pointer to NULL for an empty set of options. If this function - * takes ownership of the QDict reference, it will set *options to NULL; - * otherwise, it will contain unused/unrecognized options after this function - * returns. Then, the caller is responsible for freeing it. If it intends to - * reuse the QDict, QINCREF() should be called beforehand. + * 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) { - 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")) { + 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); + 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); if (!drv) { - /* errp has been set already */ - ret = -ENOENT; - goto fail; + error_setg(errp, "Unknown driver '%s'", drvname); + return -ENOENT; } - /* Parse the filename and open it */ + /* Driver-specific filename parsing */ if (drv->bdrv_parse_filename && parse_filename) { drv->bdrv_parse_filename(filename, *options, &local_err); if (local_err) { error_propagate(errp, local_err); - ret = -EINVAL; - goto fail; + return -EINVAL; } if (!drv->bdrv_needs_filename) { qdict_del(*options, "filename"); - } else { - filename = qdict_get_str(*options, "filename"); } } + return 0; +} + +/* + * Opens a file using a protocol (file, host_device, nbd, ...) + * + * options is an indirect pointer to a QDict of options to pass to the block + * drivers, or pointer to NULL for an empty set of options. If this function + * takes ownership of the QDict reference, it will set *options to NULL; + * otherwise, it will contain unused/unrecognized options after this function + * returns. Then, the caller is responsible for freeing it. If it intends to + * reuse the QDict, QINCREF() should be called beforehand. + */ +static int bdrv_file_open(BlockDriverState *bs, const char *filename, + QDict **options, int flags, Error **errp) +{ + BlockDriver *drv; + const char *drvname; + Error *local_err = NULL; + int ret; + + ret = bdrv_fill_options(options, filename, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return ret; + } + + filename = qdict_get_try_str(*options, "filename"); + drvname = qdict_get_str(*options, "driver"); + + drv = bdrv_find_format(drvname); + if (!drv) { + error_setg(errp, "Unknown driver '%s'", drvname); + ret = -ENOENT; + goto fail; + } + qdict_del(*options, "driver"); + + /* Open the file */ if (!drv->bdrv_file_open) { ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err); *options = NULL;
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 <kwolf@redhat.com> --- block.c | 116 ++++++++++++++++++++++++++++++++++++++++------------------------ 1 file changed, 73 insertions(+), 43 deletions(-)