| Message ID | 1390762963-25538-10-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Jan 26, 2014 at 08:02:42PM +0100, Max Reitz wrote: > 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 | 33 ++++++++++++--------------------- > 1 file changed, 12 insertions(+), 21 deletions(-) > > diff --git a/block.c b/block.c > index f847c4b..b1bae23 100644 > --- a/block.c > +++ b/block.c > @@ -943,9 +943,7 @@ 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. > + * empty set of options. > */ > static int bdrv_file_open(BlockDriverState *bs, const char *filename, > QDict *options, int flags, Error **errp) > @@ -1010,8 +1008,8 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, > } > > if (!drv->bdrv_file_open) { > + QINCREF(options); > 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); > } > @@ -1020,21 +1018,10 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, > 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; > } > > @@ -1238,7 +1225,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > assert(!drv); > ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, > &local_err); > - options = NULL; > if (ret) { > if (bs->drv) { > goto close_and_fail; > @@ -1246,8 +1232,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > goto fail; > } > } > - *pbs = bs; > - return 0; > + goto done; > } > > /* For snapshot=on, create a temporary qcow2 overlay */ > @@ -1377,12 +1362,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, > } > } > > +done: > /* Check if any unknown options were used */ > if (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); Tests 071 and 072 segfault, and gdb shows that it occurs here. More investigation shows that entry is NULL, although qdict_size() is returning 1. > + } 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.3 > >
On 27.01.2014 18:44, Jeff Cody wrote: > On Sun, Jan 26, 2014 at 08:02:42PM +0100, Max Reitz wrote: >> 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 | 33 ++++++++++++--------------------- >> 1 file changed, 12 insertions(+), 21 deletions(-) >> >> diff --git a/block.c b/block.c >> index f847c4b..b1bae23 100644 >> --- a/block.c >> +++ b/block.c >> @@ -943,9 +943,7 @@ 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. >> + * empty set of options. >> */ >> static int bdrv_file_open(BlockDriverState *bs, const char *filename, >> QDict *options, int flags, Error **errp) >> @@ -1010,8 +1008,8 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, >> } >> >> if (!drv->bdrv_file_open) { >> + QINCREF(options); >> 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); >> } >> @@ -1020,21 +1018,10 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, >> 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; >> } >> >> @@ -1238,7 +1225,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, >> assert(!drv); >> ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, >> &local_err); >> - options = NULL; >> if (ret) { >> if (bs->drv) { >> goto close_and_fail; >> @@ -1246,8 +1232,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, >> goto fail; >> } >> } >> - *pbs = bs; >> - return 0; >> + goto done; >> } >> >> /* For snapshot=on, create a temporary qcow2 overlay */ >> @@ -1377,12 +1362,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, >> } >> } >> >> +done: >> /* Check if any unknown options were used */ >> if (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); > Tests 071 and 072 segfault, and gdb shows that it occurs here. More > investigation shows that entry is NULL, although qdict_size() is > returning 1. Hm, and I thought I'd run the tests, but obviously I did not for this final version… The problem is probably the QINCREF(). Hm, I really want to reuse this test; the only way I see then is to make the options parameter of bdrv_file_open() an indirect pointer so it can be set to NULL after bdrv_open() (in bdrv_file_open()). That would not be very pretty, but it would work and there wouldn't be two tests whether all options could be handled. Thanks for catching this, Max >> + } 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.3 >> >>
diff --git a/block.c b/block.c index f847c4b..b1bae23 100644 --- a/block.c +++ b/block.c @@ -943,9 +943,7 @@ 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. + * empty set of options. */ static int bdrv_file_open(BlockDriverState *bs, const char *filename, QDict *options, int flags, Error **errp) @@ -1010,8 +1008,8 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, } if (!drv->bdrv_file_open) { + QINCREF(options); 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); } @@ -1020,21 +1018,10 @@ static int bdrv_file_open(BlockDriverState *bs, const char *filename, 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; } @@ -1238,7 +1225,6 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, assert(!drv); ret = bdrv_file_open(bs, filename, options, flags & ~BDRV_O_PROTOCOL, &local_err); - options = NULL; if (ret) { if (bs->drv) { goto close_and_fail; @@ -1246,8 +1232,7 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, goto fail; } } - *pbs = bs; - return 0; + goto done; } /* For snapshot=on, create a temporary qcow2 overlay */ @@ -1377,12 +1362,18 @@ int bdrv_open(BlockDriverState **pbs, const char *filename, } } +done: /* Check if any unknown options were used */ if (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 | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-)