When signaling a corruption on a read-only image, qcow2 already makes
fatal events non-fatal (i.e., they will not result in the image being
closed, and the image header's corrupt flag will not be set).  This is
necessary because we cannot set the corrupt flag on read-only images,
and it is possible because further corruption of read-only images is
impossible.

Inactive images are effectively read-only, too, so we should do the same
for them.

(Otherwise, the assert(!(bs->open_flags & BDRV_O_INACTIVE)) in
bdrv_co_pwritev() will fail, crashing qemu.)

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 59a38b9cd3..8b5f7386f7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4402,7 +4402,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
     char *message;
     va_list ap;
 
-    fatal = fatal && !bs->read_only;
+    if ((bs->open_flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) != BDRV_O_RDWR) {
+        fatal = false;
+    }
 
     if (s->signaled_corruption &&
         (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT)))
-- 
2.17.0

On Mon, Jun 04, 2018 at 04:14:36PM +0200, Max Reitz wrote:
> When signaling a corruption on a read-only image, qcow2 already makes
> fatal events non-fatal (i.e., they will not result in the image being
> closed, and the image header's corrupt flag will not be set).  This is
> necessary because we cannot set the corrupt flag on read-only images,
> and it is possible because further corruption of read-only images is
> impossible.
> 
> Inactive images are effectively read-only, too, so we should do the same
> for them.
> 
> (Otherwise, the assert(!(bs->open_flags & BDRV_O_INACTIVE)) in
> bdrv_co_pwritev() will fail, crashing qemu.)
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 59a38b9cd3..8b5f7386f7 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4402,7 +4402,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
>      char *message;
>      va_list ap;
>  
> -    fatal = fatal && !bs->read_only;
> +    if ((bs->open_flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) != BDRV_O_RDWR) {

Hmm, this is pretty much exactly what the bdrv_is_writable() helper function
does in block.c; too bad it's scope is limited to block.c.  Maybe worth it
to make it a more widely available helper and use it here?

> +        fatal = false;
> +    }
>  
>      if (s->signaled_corruption &&
>          (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT)))
> -- 
> 2.17.0
> 
> 

On 2018-06-04 22:06, Jeff Cody wrote:
> On Mon, Jun 04, 2018 at 04:14:36PM +0200, Max Reitz wrote:
>> When signaling a corruption on a read-only image, qcow2 already makes
>> fatal events non-fatal (i.e., they will not result in the image being
>> closed, and the image header's corrupt flag will not be set).  This is
>> necessary because we cannot set the corrupt flag on read-only images,
>> and it is possible because further corruption of read-only images is
>> impossible.
>>
>> Inactive images are effectively read-only, too, so we should do the same
>> for them.
>>
>> (Otherwise, the assert(!(bs->open_flags & BDRV_O_INACTIVE)) in
>> bdrv_co_pwritev() will fail, crashing qemu.)
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/qcow2.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 59a38b9cd3..8b5f7386f7 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -4402,7 +4402,9 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
>>      char *message;
>>      va_list ap;
>>  
>> -    fatal = fatal && !bs->read_only;
>> +    if ((bs->open_flags & (BDRV_O_RDWR | BDRV_O_INACTIVE)) != BDRV_O_RDWR) {
> 
> Hmm, this is pretty much exactly what the bdrv_is_writable() helper function
> does in block.c; too bad it's scope is limited to block.c.  Maybe worth it
> to make it a more widely available helper and use it here?

You know what, I copied it from there but for some reason never thought
of making it global.

Will do.

Max

>> +        fatal = false;
>> +    }
>>  
>>      if (s->signaled_corruption &&
>>          (!fatal || (s->incompatible_features & QCOW2_INCOMPAT_CORRUPT)))
>> -- 
>> 2.17.0
>>
>>