diff mbox

[2/7] block: Set BDRV_O_ALLOW_RDWR and snapshot_options before storing the flags

Message ID 5fd02facca962ec5c64ef45a43413d4c5571d42c.1473867966.git.berto@igalia.com
State New
Headers show

Commit Message

Alberto Garcia Sept. 14, 2016, 3:52 p.m. UTC 
If an image is opened with snapshot=on, its flags are modified by
bdrv_backing_options() and then bs->open_flags is updated accordingly.
This last step is unnecessary if we calculate the new flags before
setting bs->open_flags.

Soon we'll introduce the "read-only" option, and then we'll need to be
able to modify its value in the QDict when snapshot=on. This is more
cumbersome if bs->options is already set. This patch simplifies that.

The code that sets BDRV_O_ALLOW_RDWR is also moved for the same
reason.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

Comments

Kevin Wolf Sept. 14, 2016, 4:40 p.m. UTC | #1 
Am 14.09.2016 um 17:52 hat Alberto Garcia geschrieben:
> If an image is opened with snapshot=on, its flags are modified by
> bdrv_backing_options() and then bs->open_flags is updated accordingly.
> This last step is unnecessary if we calculate the new flags before
> setting bs->open_flags.
> 
> Soon we'll introduce the "read-only" option, and then we'll need to be
> able to modify its value in the QDict when snapshot=on. This is more
> cumbersome if bs->options is already set. This patch simplifies that.
> 
> The code that sets BDRV_O_ALLOW_RDWR is also moved for the same
> reason.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1c75a6f..7cae841 100644
> --- a/block.c
> +++ b/block.c
> @@ -1627,6 +1627,17 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>          goto fail;
>      }
>  
> +    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.

Given that the flags are already updated and don't contain
BDRV_O_SNAPSHOT any more, I suspect that this is a silent bug fix. If
so, it might be better to split it out into a separate patch before this
one, or at the very least should be mentioned in the commit message.

If it's not a fix, however, it's probably a bug. :-)

> @@ -1651,18 +1662,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>  
>      /* Open image file without format layer */
>      if ((flags & BDRV_O_PROTOCOL) == 0) {
> -        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;
> -
>          file = bdrv_open_child(filename, options, "file", bs,
>                                 &child_file, true, &local_err);
>          if (local_err) {

Kevin
Jeff Cody Sept. 14, 2016, 6:54 p.m. UTC | #2 
On Wed, Sep 14, 2016 at 06:52:15PM +0300, Alberto Garcia wrote:
> If an image is opened with snapshot=on, its flags are modified by
> bdrv_backing_options() and then bs->open_flags is updated accordingly.
> This last step is unnecessary if we calculate the new flags before
> setting bs->open_flags.
> 
> Soon we'll introduce the "read-only" option, and then we'll need to be
> able to modify its value in the QDict when snapshot=on. This is more
> cumbersome if bs->options is already set. This patch simplifies that.
> 
> The code that sets BDRV_O_ALLOW_RDWR is also moved for the same
> reason.
> 

Before, we would not set BDRV_O_ALLOW_RDWR for protocols, but this will
change that.  Is that side-affect intentional?

> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1c75a6f..7cae841 100644
> --- a/block.c
> +++ b/block.c
> @@ -1627,6 +1627,17 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>          goto fail;
>      }
>  
> +    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);
> @@ -1651,18 +1662,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>  
>      /* Open image file without format layer */
>      if ((flags & BDRV_O_PROTOCOL) == 0) {
> -        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;
> -
>          file = bdrv_open_child(filename, options, "file", bs,
>                                 &child_file, true, &local_err);
>          if (local_err) {
Alberto Garcia Sept. 15, 2016, 9:10 a.m. UTC | #3 
On Wed 14 Sep 2016 08:54:19 PM CEST, Jeff Cody wrote:
>> If an image is opened with snapshot=on, its flags are modified by
>> bdrv_backing_options() and then bs->open_flags is updated accordingly.
>> This last step is unnecessary if we calculate the new flags before
>> setting bs->open_flags.
>> 
>> Soon we'll introduce the "read-only" option, and then we'll need to be
>> able to modify its value in the QDict when snapshot=on. This is more
>> cumbersome if bs->options is already set. This patch simplifies that.
>> 
>> The code that sets BDRV_O_ALLOW_RDWR is also moved for the same
>> reason.
>
> Before, we would not set BDRV_O_ALLOW_RDWR for protocols, but this
> will change that.  Is that side-affect intentional?

BDRV_O_ALLOW_RDWR is set in the root BDS, and then all children inherit
that flag (both bdrv_inherited_options() and bdrv_backing_options() copy
it), so I don't think the ((flags & BDRV_O_PROTOCOL) == 0) check makes
any difference in practice.

Berto
Alberto Garcia Sept. 15, 2016, 11:24 a.m. UTC | #4 
On Wed 14 Sep 2016 06:40:29 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.

Berto
Kevin Wolf Sept. 15, 2016, 12:19 p.m. UTC | #5 
Am 15.09.2016 um 13:24 hat Alberto Garcia geschrieben:
> On Wed 14 Sep 2016 06:40:29 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.

I think mentioning this in the commit message wouldn't hurt, though.

Kevin
Alberto Garcia Sept. 15, 2016, 12:31 p.m. UTC | #6 
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
Kevin Wolf Sept. 15, 2016, 12:50 p.m. UTC | #7 
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
Alberto Garcia Sept. 15, 2016, 12:55 p.m. UTC | #8 
On Thu 15 Sep 2016 02:50:53 PM CEST, Kevin Wolf wrote:

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

I see, I thought it wasn't clear, I'll update the message then :)

Berto
diff mbox

Patch

diff --git a/block.c b/block.c
index 1c75a6f..7cae841 100644
--- a/block.c
+++ b/block.c
@@ -1627,6 +1627,17 @@  static BlockDriverState *bdrv_open_inherit(const char *filename,
         goto fail;
     }
 
+    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);
@@ -1651,18 +1662,6 @@  static BlockDriverState *bdrv_open_inherit(const char *filename,
 
     /* Open image file without format layer */
     if ((flags & BDRV_O_PROTOCOL) == 0) {
-        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;
-
         file = bdrv_open_child(filename, options, "file", bs,
                                &child_file, true, &local_err);
         if (local_err) {