blockdev: Unset temporary flag when changing medium.

Commit Message

Alyssa Milburn Feb. 4, 2016, 5:36 p.m. UTC

This avoids a 'change' command from the monitor unlink()ing the new
file if the bdrv was previously snapshotted.

Signed-off-by: Alyssa Milburn <[e-mail]>
---
 blockdev.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Max Reitz Feb. 6, 2016, 1:04 p.m. UTC | #1

On 04.02.2016 18:36, Alyssa Milburn wrote:
> This avoids a 'change' command from the monitor unlink()ing the new
> file if the bdrv was previously snapshotted.
> 
> Signed-off-by: Alyssa Milburn <[e-mail]>
> ---
>  blockdev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index be4ca44..d39c2e6 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2514,6 +2514,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
>      }
>  
>      bdrv_flags = blk_get_open_flags_from_root_state(blk);
> +    bdrv_flags &= ~BDRV_O_TEMPORARY;
>  
>      if (!has_read_only) {
>          read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;
> 

This patch is correct (thanks!), but I think we want to unset even more
flags, namely BDRV_O_SNAPSHOT, BDRV_O_NO_BACKING, and BDRV_O_PROTOCOL.

Max

Alyssa Milburn Feb. 6, 2016, 1:47 p.m. UTC | #2

On Sat, Feb 06, 2016 at 02:04:23PM +0100, Max Reitz wrote:
> On 04.02.2016 18:36, Alyssa Milburn wrote:
> > This avoids a 'change' command from the monitor unlink()ing the new
> > file if the bdrv was previously snapshotted.
> > 
> > Signed-off-by: Alyssa Milburn <[e-mail]>
> > ---
> >  blockdev.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index be4ca44..d39c2e6 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -2514,6 +2514,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
> >      }
> >  
> >      bdrv_flags = blk_get_open_flags_from_root_state(blk);
> > +    bdrv_flags &= ~BDRV_O_TEMPORARY;
> >  
> >      if (!has_read_only) {
> >          read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;
> > 
> 
> This patch is correct (thanks!), but I think we want to unset even more
> flags, namely BDRV_O_SNAPSHOT, BDRV_O_NO_BACKING, and BDRV_O_PROTOCOL.

Thanks. I posted an updated patch with this change (but of course, this is a
trivial patch anyway). It's not entirely clear to me which flags should(n't)
be preserved and where to do that. Since the snapshotted file is part of a
chain (and the original flags are "correct") I wondered if this might not be
the ideal fix, but it seems safe/sane to do it here.

- Alyssa

Max Reitz Feb. 6, 2016, 2:15 p.m. UTC | #3

On 06.02.2016 14:47, Alyssa Milburn wrote:
> On Sat, Feb 06, 2016 at 02:04:23PM +0100, Max Reitz wrote:
>> On 04.02.2016 18:36, Alyssa Milburn wrote:
>>> This avoids a 'change' command from the monitor unlink()ing the new
>>> file if the bdrv was previously snapshotted.
>>>
>>> Signed-off-by: Alyssa Milburn <[e-mail]>
>>> ---
>>>  blockdev.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index be4ca44..d39c2e6 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>> @@ -2514,6 +2514,7 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
>>>      }
>>>  
>>>      bdrv_flags = blk_get_open_flags_from_root_state(blk);
>>> +    bdrv_flags &= ~BDRV_O_TEMPORARY;
>>>  
>>>      if (!has_read_only) {
>>>          read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;
>>>
>>
>> This patch is correct (thanks!), but I think we want to unset even more
>> flags, namely BDRV_O_SNAPSHOT, BDRV_O_NO_BACKING, and BDRV_O_PROTOCOL.
> 
> Thanks. I posted an updated patch with this change (but of course, this is a
> trivial patch anyway). It's not entirely clear to me which flags should(n't)
> be preserved and where to do that.

Ideally, none would be. You'd use blockdev-add and
x-blockdev-{remove,insert}-medium to do the change manually, and specify
all the options for the new BlockDriverState using blockdev-add.

The BlockBackend's root state is just a legacy hack because the 'change'
command preserved some flags and other options of the old medium for the
new one that is inserted.

So I think we should preserve as many flags as possible, unless
something weird might happen which one doesn't want, and so I guessed
for each of the flags if they'd do something I'd not expect when issuing
a change command:

- O_TEMPORARY: Would delete the new medium's file.
- O_SNAPSHOT: Would silently create a snapshot over the new medium
  (actually, apparently it does not, but this is a bug...).
- O_NO_BACKING: Would not open the backing chain for the new image.
- O_PROTOCOL: Would ignore the format of the new file and just open it
  raw.

While I personally wouldn't like any flag to be inherited from the
previous medium, the rest doesn't look like it would do much harm. If
one doesn't like that the cache is fully inherited, they just have to
use blockdev-add and x-blockdev-{remove,insert}-medium.

>                                    Since the snapshotted file is part of a
> chain (and the original flags are "correct") I wondered if this might not be
> the ideal fix, but it seems safe/sane to do it here.

Well, another place to do it is in blk_update_root_state() or inside of
blk_get_open_flags_from_root_state(). But I don't think it matters
because this is the only place that calls that function (and this is not
supposed to change, because it is just legacy handling anyway).

Max

Patch

diff --git a/blockdev.c b/blockdev.c
index be4ca44..d39c2e6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2514,6 +2514,7 @@  void qmp_blockdev_change_medium(const char *device, const char *filename,
     }
 
     bdrv_flags = blk_get_open_flags_from_root_state(blk);
+    bdrv_flags &= ~BDRV_O_TEMPORARY;
 
     if (!has_read_only) {
         read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;