| Message ID | 5fd02facca962ec5c64ef45a43413d4c5571d42c.1473867966.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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) {
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
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
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
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
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
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 --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) {
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(-)