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

[Alberto Garcia]

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