diff mbox

[2/2] block: assert no image modification under BDRV_O_INACTIVE

Message ID 1491405505-31620-3-git-send-email-den@openvz.org (mailing list archive)
State New, archived
Headers show

Commit Message

Denis V. Lunev April 5, 2017, 3:18 p.m. UTC
As long as BDRV_O_INACTIVE is set, the image file is only opened so we
have a file descriptor for it. We're definitely not supposed to modify
the image, it's still owned by the migration source.

This commit is an addition to 09e0c771 but the assert() is added to
bdrv_truncate().

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Eric Blake April 5, 2017, 3:37 p.m. UTC | #1
On 04/05/2017 10:18 AM, Denis V. Lunev wrote:
> As long as BDRV_O_INACTIVE is set, the image file is only opened so we
> have a file descriptor for it. We're definitely not supposed to modify
> the image, it's still owned by the migration source.
> 
> This commit is an addition to 09e0c771 but the assert() is added to
> bdrv_truncate().
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 2 ++
>  1 file changed, 2 insertions(+)

I don't feel comfortable adding an assert this late in the 2.9 phase,
but think it makes perfect sense as a 2.10 addition.

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block.c b/block.c
> index 927ba89..9273741 100644
> --- a/block.c
> +++ b/block.c
> @@ -3279,6 +3279,8 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
>      if (bs->read_only)
>          return -EACCES;
>  
> +    assert(!(bs->open_flags & BDRV_O_INACTIVE));
> +
>      ret = drv->bdrv_truncate(bs, offset);
>      if (ret == 0) {
>          ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>
Denis V. Lunev April 5, 2017, 3:47 p.m. UTC | #2
On 04/05/2017 06:37 PM, Eric Blake wrote:
> On 04/05/2017 10:18 AM, Denis V. Lunev wrote:
>> As long as BDRV_O_INACTIVE is set, the image file is only opened so we
>> have a file descriptor for it. We're definitely not supposed to modify
>> the image, it's still owned by the migration source.
>>
>> This commit is an addition to 09e0c771 but the assert() is added to
>> bdrv_truncate().
>>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c | 2 ++
>>  1 file changed, 2 insertions(+)
> I don't feel comfortable adding an assert this late in the 2.9 phase,
> but think it makes perfect sense as a 2.10 addition.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
no-no, this is for 2.10 for sure. No way for 2.9. I have stated that in
cover letter :(
Sorry for inconvenience.

Den
Max Reitz April 13, 2017, 7:09 p.m. UTC | #3
On 05.04.2017 17:18, Denis V. Lunev wrote:
> As long as BDRV_O_INACTIVE is set, the image file is only opened so we
> have a file descriptor for it. We're definitely not supposed to modify
> the image, it's still owned by the migration source.
> 
> This commit is an addition to 09e0c771 but the assert() is added to
> bdrv_truncate().
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block.c | 2 ++
>  1 file changed, 2 insertions(+)

Thanks to the new op blockers I'm not sure this is really necessary
anymore (that assertion should cover this, once it's reinstated, that
is...), but it won't hurt, I guess.

Max
diff mbox

Patch

diff --git a/block.c b/block.c
index 927ba89..9273741 100644
--- a/block.c
+++ b/block.c
@@ -3279,6 +3279,8 @@  int bdrv_truncate(BdrvChild *child, int64_t offset)
     if (bs->read_only)
         return -EACCES;
 
+    assert(!(bs->open_flags & BDRV_O_INACTIVE));
+
     ret = drv->bdrv_truncate(bs, offset);
     if (ret == 0) {
         ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);