Re: [Qemu-devel] [PATCH 09/10] block: Reuse success path from bdrv_open()

[Max Reitz]

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 <address@hidden>
> > ---
> >   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