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.
Berto
