Re: [Qemu-devel] [PATCH 1/2] block: Enable BDRV_O_SNAPSHOT with driver-specific options

[Kevin Wolf]

Am 21.11.2013 um 15:33 hat Stefan Hajnoczi geschrieben:
> On Tue, Nov 19, 2013 at 04:37:27PM +0100, Kevin Wolf wrote:
> > @@ -1114,6 +1093,24 @@ int bdrv_open(BlockDriverState *bs, const char 
> > *filename, QDict *options,
> >              goto fail;
> >          }
> >  
> > +        /* Prepare a new options QDict for the temporary file, where user
> > +         * options refer to the backing file */
> > +        if (!options) {
> > +            options = qdict_new();
> > +        }
> 
> You can drop this because options is never NULL:
> 
>   options = qdict_clone_shallow(options);
> 
>   /* For snapshot=on, create a temporary qcow2 overlay */
>   if (flags & BDRV_O_SNAPSHOT) {
>       ...

You're right, I'll remove it in v2.

> > +        if (filename) {
> > +            qdict_put(options, "file.filename", 
> > qstring_from_str(filename));
> > +        }
> > +        if (drv) {
> > +            qdict_put(options, "driver", 
> > qstring_from_str(drv->format_name));
> > +        }
> > +
> > +        snapshot_options = qdict_new();
> > +        qdict_put(snapshot_options, "backing", options);
> > +        qdict_flatten(snapshot_options);
> > +
> > +        options = snapshot_options;
> 
> One thing I'm not sure about after these operations have been performed:
> 
>   bs->options = options;
>   ...
>   if (flags & BDRV_O_SNAPSHOT) {
>       ...
> 
> So bs->options does not reflect what we ended up with in the
> BDRV_O_SNAPSHOT case.
> 
> But git grep -- '->options' shows no users of this field.  Therefore it
> won't cause a problem yet.  But can you explain what's going on here?
> Either we should keep bs->options up-to-date or we should drop the
> field.

I think bs->options was meant to be used in cases where we reopen a BDS.
If it's currently unused, I guess this means we have some bugs. :-)

Should bs->options be the original options passed by the user or the
ones modified by BDRV_O_SNAPSHOT? I'm not completely sure, but I think
the modified ones make more sense; it would also make it easier to move
the whole BDRV_O_SNAPSHOT logic out to drive_init (which I plan to do in
a next step).

Kevin