| Message ID | 1391881159-13585-8-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 08.02.2014 um 18:39 hat Max Reitz geschrieben: > The fail and success paths of bdrv_file_open() may be further shortened > by reusing code already existent in bdrv_open(). This includes > bdrv_file_open() not taking the reference to options which allows the > removal of QDECREF(options) in that function. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, > > /* Parse the filename and open it */ > if (drv->bdrv_parse_filename && filename) { > - drv->bdrv_parse_filename(filename, options, &local_err); > + drv->bdrv_parse_filename(filename, *options, &local_err); > if (error_is_set(&local_err)) { > error_propagate(errp, local_err); > ret = -EINVAL; > goto fail; > } > - qdict_del(options, "filename"); > + qdict_del(*options, "filename"); > + } else if (drv->bdrv_needs_filename && !filename) { > + error_setg(errp, "The '%s' block driver requires a file name", > + drv->format_name); > + ret = -EINVAL; > + goto fail; > } How did this part end up in this patch? It doesn't look wrong, though I think bdrv_open_common() should already catch it. In any case it's an addition that the commit message didn't mention. Kevin
Le Saturday 08 Feb 2014 à 18:39:18 (+0100), Max Reitz a écrit : > The fail and success paths of bdrv_file_open() may be further shortened > by reusing code already existent in bdrv_open(). This includes > bdrv_file_open() not taking the reference to options which allows the > removal of QDECREF(options) in that function. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block.c | 68 ++++++++++++++++++++++++++++++++--------------------------------- > 1 file changed, 34 insertions(+), 34 deletions(-) > > diff --git a/block.c b/block.c > index 1fcd7e0..22e5a44 100644 > --- a/block.c > +++ b/block.c > @@ -948,13 +948,15 @@ free_and_fail: > /* > * Opens a file using a protocol (file, host_device, nbd, ...) > * > - * options is a QDict of options to pass to the block drivers, or NULL for an > - * empty set of options. The reference to the QDict belongs to the block layer > - * after the call (even on failure), so if the caller intends to reuse the > - * dictionary, it needs to use QINCREF() before calling bdrv_file_open. > + * 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) > + QDict **options, int flags, Error **errp) > { > BlockDriver *drv; > const char *drvname; > @@ -964,9 +966,9 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, > > /* 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)); > + filename = qdict_get_try_str(*options, "filename"); > + } else if (filename && !qdict_haskey(*options, "filename")) { > + qdict_put(*options, "filename", qstring_from_str(filename)); > allow_protocol_prefix = true; > } else { > error_setg(errp, "Can't specify 'file' and 'filename' options at the " > @@ -976,13 +978,13 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, > } > > /* Find the right block driver */ > - drvname = qdict_get_try_str(options, "driver"); > + 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"); > + qdict_del(*options, "driver"); > } else if (filename) { > drv = bdrv_find_protocol(filename, allow_protocol_prefix); > if (!drv) { > @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, > > /* Parse the filename and open it */ > if (drv->bdrv_parse_filename && filename) { > - drv->bdrv_parse_filename(filename, options, &local_err); > + drv->bdrv_parse_filename(filename, *options, &local_err); > if (error_is_set(&local_err)) { > error_propagate(errp, local_err); > ret = -EINVAL; > goto fail; > } > - qdict_del(options, "filename"); > + qdict_del(*options, "filename"); > + } else if (drv->bdrv_needs_filename && !filename) { > + error_setg(errp, "The '%s' block driver requires a file name", > + drv->format_name); > + ret = -EINVAL; > + goto fail; > } > > if (!drv->bdrv_file_open) { > - ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err); > - options = NULL; > + ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err); > + *options = NULL; > } else { > - ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err); > + ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err); > } > if (ret < 0) { > error_propagate(errp, local_err); > goto fail; > } > > - /* Check if any unknown options were used */ > - if (options && (qdict_size(options) != 0)) { > - const QDictEntry *entry = qdict_first(options); > - error_setg(errp, "Block protocol '%s' doesn't support the option '%s'", > - drv->format_name, entry->key); > - ret = -EINVAL; > - goto fail; > - } > - QDECREF(options); > - > bs->growable = 1; > return 0; > > fail: > - QDECREF(options); > return ret; > } > > @@ -1242,12 +1238,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > > if (flags & BDRV_O_PROTOCOL) { > assert(!drv); > - ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, > + ret = bdrv_file_open(bs, filename, &options, flags & ~BDRV_O_PROTOCOL, > &local_err); > - options = NULL; > if (!ret) { > - *pbs = bs; > - return 0; > + goto done; > } else if (bs->drv) { > goto close_and_fail; > } else { > @@ -1384,12 +1378,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > } > } > > +done: > /* Check if any unknown options were used */ > - if (qdict_size(options) != 0) { > + if (options && (qdict_size(options) != 0)) { > const QDictEntry *entry = qdict_first(options); > - error_setg(errp, "Block format '%s' used by device '%s' doesn't " > - "support the option '%s'", drv->format_name, bs->device_name, > - entry->key); > + if (flags & BDRV_O_PROTOCOL) { > + error_setg(errp, "Block protocol '%s' doesn't support the option " > + "'%s'", drv->format_name, entry->key); > + } else { > + error_setg(errp, "Block format '%s' used by device '%s' doesn't " > + "support the option '%s'", drv->format_name, > + bs->device_name, entry->key); > + } > > ret = -EINVAL; > goto close_and_fail; > -- > 1.8.5.4 > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
On 10.02.2014 15:56, Kevin Wolf wrote: > Am 08.02.2014 um 18:39 hat Max Reitz geschrieben: >> The fail and success paths of bdrv_file_open() may be further shortened >> by reusing code already existent in bdrv_open(). This includes >> bdrv_file_open() not taking the reference to options which allows the >> removal of QDECREF(options) in that function. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, >> >> /* Parse the filename and open it */ >> if (drv->bdrv_parse_filename && filename) { >> - drv->bdrv_parse_filename(filename, options, &local_err); >> + drv->bdrv_parse_filename(filename, *options, &local_err); >> if (error_is_set(&local_err)) { >> error_propagate(errp, local_err); >> ret = -EINVAL; >> goto fail; >> } >> - qdict_del(options, "filename"); >> + qdict_del(*options, "filename"); >> + } else if (drv->bdrv_needs_filename && !filename) { >> + error_setg(errp, "The '%s' block driver requires a file name", >> + drv->format_name); >> + ret = -EINVAL; >> + goto fail; >> } > How did this part end up in this patch? It doesn't look wrong, though I > think bdrv_open_common() should already catch it. In any case it's an > addition that the commit message didn't mention. I wonder. It definitely doesn't belong here, since as of your commit "block: Fail gracefully with missing filename" this check should be in bdrv_open_common() and not here. I guess, I somehow ended up reverting it to the old state here. I just hope there aren't any more such reverts; I'll take a look. I remember some rebase conflict here (for obvious reasons), so it's probably just in this case, though. Max
diff --git a/block.c b/block.c index 1fcd7e0..22e5a44 100644 --- a/block.c +++ b/block.c @@ -948,13 +948,15 @@ free_and_fail: /* * Opens a file using a protocol (file, host_device, nbd, ...) * - * options is a QDict of options to pass to the block drivers, or NULL for an - * empty set of options. The reference to the QDict belongs to the block layer - * after the call (even on failure), so if the caller intends to reuse the - * dictionary, it needs to use QINCREF() before calling bdrv_file_open. + * 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) + QDict **options, int flags, Error **errp) { BlockDriver *drv; const char *drvname; @@ -964,9 +966,9 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, /* 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)); + filename = qdict_get_try_str(*options, "filename"); + } else if (filename && !qdict_haskey(*options, "filename")) { + qdict_put(*options, "filename", qstring_from_str(filename)); allow_protocol_prefix = true; } else { error_setg(errp, "Can't specify 'file' and 'filename' options at the " @@ -976,13 +978,13 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, } /* Find the right block driver */ - drvname = qdict_get_try_str(options, "driver"); + 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"); + qdict_del(*options, "driver"); } else if (filename) { drv = bdrv_find_protocol(filename, allow_protocol_prefix); if (!drv) { @@ -1001,41 +1003,35 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, /* Parse the filename and open it */ if (drv->bdrv_parse_filename && filename) { - drv->bdrv_parse_filename(filename, options, &local_err); + drv->bdrv_parse_filename(filename, *options, &local_err); if (error_is_set(&local_err)) { error_propagate(errp, local_err); ret = -EINVAL; goto fail; } - qdict_del(options, "filename"); + qdict_del(*options, "filename"); + } else if (drv->bdrv_needs_filename && !filename) { + error_setg(errp, "The '%s' block driver requires a file name", + drv->format_name); + ret = -EINVAL; + goto fail; } if (!drv->bdrv_file_open) { - ret = bdrv_open(&bs, filename, NULL, options, flags, drv, &local_err); - options = NULL; + ret = bdrv_open(&bs, filename, NULL, *options, flags, drv, &local_err); + *options = NULL; } else { - ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err); + ret = bdrv_open_common(bs, NULL, *options, flags, drv, &local_err); } if (ret < 0) { error_propagate(errp, local_err); goto fail; } - /* Check if any unknown options were used */ - if (options && (qdict_size(options) != 0)) { - const QDictEntry *entry = qdict_first(options); - error_setg(errp, "Block protocol '%s' doesn't support the option '%s'", - drv->format_name, entry->key); - ret = -EINVAL; - goto fail; - } - QDECREF(options); - bs->growable = 1; return 0; fail: - QDECREF(options); return ret; } @@ -1242,12 +1238,10 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, if (flags & BDRV_O_PROTOCOL) { assert(!drv); - ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, + ret = bdrv_file_open(bs, filename, &options, flags & ~BDRV_O_PROTOCOL, &local_err); - options = NULL; if (!ret) { - *pbs = bs; - return 0; + goto done; } else if (bs->drv) { goto close_and_fail; } else { @@ -1384,12 +1378,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, } } +done: /* Check if any unknown options were used */ - if (qdict_size(options) != 0) { + if (options && (qdict_size(options) != 0)) { const QDictEntry *entry = qdict_first(options); - error_setg(errp, "Block format '%s' used by device '%s' doesn't " - "support the option '%s'", drv->format_name, bs->device_name, - entry->key); + if (flags & BDRV_O_PROTOCOL) { + error_setg(errp, "Block protocol '%s' doesn't support the option " + "'%s'", drv->format_name, entry->key); + } else { + error_setg(errp, "Block format '%s' used by device '%s' doesn't " + "support the option '%s'", drv->format_name, + bs->device_name, entry->key); + } ret = -EINVAL; goto close_and_fail;
The fail and success paths of bdrv_file_open() may be further shortened by reusing code already existent in bdrv_open(). This includes bdrv_file_open() not taking the reference to options which allows the removal of QDECREF(options) in that function. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 68 ++++++++++++++++++++++++++++++++--------------------------------- 1 file changed, 34 insertions(+), 34 deletions(-)