Re: [Qemu-block] [PATCH 2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

[Kevin Wolf]

Am 15.09.2016 um 14:31 hat Alberto Garcia geschrieben:
> On Thu 15 Sep 2016 02:19:36 PM CEST, Kevin Wolf wrote:
> 
> >> >> +    if (flags & BDRV_O_RDWR) {
> >> >> +        flags |= BDRV_O_ALLOW_RDWR;
> >> >> +    }
> >> >> +
> >> >> +    if (flags & BDRV_O_SNAPSHOT) {
> >> >> +        snapshot_options = qdict_new();
> >> >> +        bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> >> >> +                                   flags, options);
> >> >> +        bdrv_backing_options(&flags, options, flags, options);
> >> >> +    }
> >> >> +
> >> >>      bs->open_flags = flags;
> >> >>      bs->options = options;
> >> >>      options = qdict_clone_shallow(options);
> >> >
> >> > Here I think we get different semantics now: bdrv_backing_options()
> >> > only affected the cloned QDict before, and now it affects bs->options,
> >> > too.
> >> 
> >> The thing is that bdrv_backing_options() doesn't change the options in
> >> general, and in the case where it does (BDRV_OPT_READ_ONLY after the
> >> next patch) I think it makes sense that the options are changed.
> >> "snapshot=on" is a bit of a special case after all.
> >
> > Fair enough, it is correct (as I suspected), but it's not a bug fix
> > because so far the options weren't changed, so there's no observable
> > difference.
> 
> The idea of this change is to remove the ' bs->open_flags = flags ' line
> in the original code, as explained in the commit message.
> 
> That's one thing. The other (and most important one) is to prepare for
> the introduction of BDRV_OPT_READ_ONLY.
> 
> Without this patch we'd need to need to update not just bs->open_flags
> but bs->options as well, and it would be something like this:
> 
>    bs->open_flags = flags;
>    bs->options = options;
> 
>       /* ... */
> 
>    if ((flags & BDRV_O_PROTOCOL) == 0) {
> 
>       /* ... */
> 
>       if (flags & BDRV_O_SNAPSHOT) {
>           snapshot_options = qdict_new();
>           bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
>                                      flags, options);
> +         qdict_del(bs->options, BDRV_OPT_READ_ONLY);
> +         qdict_del(options, BDRV_OPT_READ_ONLY);
>           bdrv_backing_options(&flags, options, flags, options);
>       }
> 
>       bs->open_flags = flags;
> +     qdict_copy_default(bs->options, options, BDRV_OPT_READ_ONLY);
> 
>       /* ... */
>     }
> 
> Moving this chunk of code saves us from having to update bs->open_flags
> and bs->options and makes things more readable.

I fully understand understand what the patch is for, and I don't want
you to change any code. I was asking whether there was a hidden bug fix
in here (which should have been moved into a separate patch then), but
you explained to me why there isn't, so we're good. The only thing I'm
looking for now is putting this explanation into the commit message.

So just include in the commit message that moving across the clone of
bs->options doesn't make an observable difference (at the point of this
patch) because the bdrv_backing_options() call doesn't actually change
options yet, and that applying any changes made by it in the future
to bs->options makes more sense anyway.

Kevin