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
