[PATCH 1/1] qcow2: do not try to clear the dirty bit on a read-only node

[PATCH 1/1] qcow2: do not try to clear the dirty bit on a read-only node

[Denis V. Lunev]

qcow2_do_close() -> qcow2_inactivate() clears the dirty bit with a
plain write to bs->file, unconditionally. A read-only node can still
be dirty, inherited from an earlier writable session, and that write
then hits a missing BLK_PERM_WRITE and asserts in
bdrv_co_write_req_prepare() (block/io.c) on an entirely ordinary
close -- closing is expected, the dirty bit on a read-only node
is not.

Skip the clear for read-only nodes, same as read access already does.
Any other still-dirty node keeps the unguarded write: it is expected
to hold write permission, and a missing one there is a bug worth
seeing.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Hanna Reitz <hreitz@redhat.com>
---
 block/qcow2.c              |  6 +++++-
 tests/qemu-iotests/039     | 11 +++++++++++
 tests/qemu-iotests/039.out |  3 +++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2ab6ddd7b0..adb177e6b6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2870,7 +2870,11 @@ static int GRAPH_RDLOCK qcow2_inactivate(BlockDriverState *bs)
                      strerror(-ret));
     }
 
-    if (result == 0) {
+    /*
+     * A read-only node cannot resolve an inherited dirty bit here;
+     * leave it dirty, same as plain read access already does.
+     */
+    if (result == 0 && !bdrv_is_read_only(bs)) {
         qcow2_mark_clean(bs);
     }
 
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index e43e7026ce..94a8bfe754 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -84,6 +84,17 @@ $QEMU_IO -r -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
 # The dirty bit must be set
 _qcow2_dump_header | grep incompatible_features
 
+echo
+echo "== Read-only open must not crash on close =="
+
+# We must not try to write the QCOW2 header to a read-only image.
+$QEMU_IMG info --image-opts \
+    "driver=$IMGFMT,read-only=on,file.driver=file,file.filename=$TEST_IMG,file.read-only=off" \
+    > /dev/null
+
+# The dirty bit must still be set: this open never wrote any guest data
+_qcow2_dump_header | grep incompatible_features
+
 echo
 echo "== Repairing the image file must succeed =="
 
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index 8fdbcc528a..c66361128f 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -24,6 +24,9 @@ read 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 incompatible_features     [0]
 
+== Read-only open must not crash on close ==
+incompatible_features     [0]
+
 == Repairing the image file must succeed ==
 ERROR cluster 5 refcount=0 reference=1
 Rebuilding refcount structure
-- 
2.53.0

Re: [PATCH 1/1] qcow2: do not try to clear the dirty bit on a read-only node

[Denis V. Lunev]

On 7/16/26 17:35, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain it was sent from.
> Do not click links or open attachments unless it is an email you expected to receive.
> qcow2_do_close() -> qcow2_inactivate() clears the dirty bit with a
> plain write to bs->file, unconditionally. A read-only node can still
> be dirty, inherited from an earlier writable session, and that write
> then hits a missing BLK_PERM_WRITE and asserts in
> bdrv_co_write_req_prepare() (block/io.c) on an entirely ordinary
> close -- closing is expected, the dirty bit on a read-only node
> is not.
>
> Skip the clear for read-only nodes, same as read access already does.
> Any other still-dirty node keeps the unguarded write: it is expected
> to hold write permission, and a missing one there is a bug worth
> seeing.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> ---
>  block/qcow2.c              |  6 +++++-
>  tests/qemu-iotests/039     | 11 +++++++++++
>  tests/qemu-iotests/039.out |  3 +++
>  3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2ab6ddd7b0..adb177e6b6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2870,7 +2870,11 @@ static int GRAPH_RDLOCK qcow2_inactivate(BlockDriverState *bs)
>                       strerror(-ret));
>      }
>  
> -    if (result == 0) {
> +    /*
> +     * A read-only node cannot resolve an inherited dirty bit here;
> +     * leave it dirty, same as plain read access already does.
> +     */
> +    if (result == 0 && !bdrv_is_read_only(bs)) {
>          qcow2_mark_clean(bs);
>      }
>  
> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index e43e7026ce..94a8bfe754 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -84,6 +84,17 @@ $QEMU_IO -r -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
>  # The dirty bit must be set
>  _qcow2_dump_header | grep incompatible_features
>  
> +echo
> +echo "== Read-only open must not crash on close =="
> +
> +# We must not try to write the QCOW2 header to a read-only image.
> +$QEMU_IMG info --image-opts \
> +    "driver=$IMGFMT,read-only=on,file.driver=file,file.filename=$TEST_IMG,file.read-only=off" \
> +    > /dev/null
> +
> +# The dirty bit must still be set: this open never wrote any guest data
> +_qcow2_dump_header | grep incompatible_features
> +
>  echo
>  echo "== Repairing the image file must succeed =="
>  
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index 8fdbcc528a..c66361128f 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -24,6 +24,9 @@ read 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  incompatible_features     [0]
>  
> +== Read-only open must not crash on close ==
> +incompatible_features     [0]
> +
>  == Repairing the image file must succeed ==
>  ERROR cluster 5 refcount=0 reference=1
>  Rebuilding refcount structure
Actually the problem is more subtle. The following command line results in

qemu-storage-daemon \                                                   
                                                                       
                                                    
  --blockdev
'{"driver":"file","filename":"dirty.qcow2","aio":"threads","node-name":"file0","cache":{"direct":false,"no-flush":false},"auto-read-only":true,"discard":"unmap"}'
\                    
  --blockdev
'{"node-name":"fmt0","read-only":true,"discard":"unmap","cache":{"direct":false,"no-flush":false},"driver":"qcow2","file":"file0"}'
\                                                   
  --chardev socket,id=qmp,path=master.sock,server=on,wait=off \         
                                                                       
                                                    
  --monitor chardev=qmp                                                 
                                                                       
                                                    
                                                                       
                                                                       
                                                         
via query-named-block-nodes:

  node-name=fmt0     drv=qcow2  ro=True
  node-name=file0    drv=file   ro=False

which would come to exactly configuration being asserted in the test
and this setup is pretty legal at my opinion. In production we have
had something lengthier with more snapshots.

File descriptor in turn is opened in a correct way - with O_RDONLY
that is why the problem was hidden for a lot of time.

Would it be more reasonable to disallow to set dirty rather than
to ignore is open question to me. Playing with permissions and
changing bdrv_is_read_only() seems more dangerous.

Also worth to note that bdrv_co_write_req_prepare() has wrong check.
It should check file descriptor rather than administrative permission.

This looks toooooooooo complex to me :(

Den

Re: [PATCH 1/1] qcow2: do not try to clear the dirty bit on a read-only node

[Denis V. Lunev]

On 7/16/26 17:35, Denis V. Lunev wrote:
> This email originated from an IP that might not be authorized by the domain it was sent from.
> Do not click links or open attachments unless it is an email you expected to receive.
> qcow2_do_close() -> qcow2_inactivate() clears the dirty bit with a
> plain write to bs->file, unconditionally. A read-only node can still
> be dirty, inherited from an earlier writable session, and that write
> then hits a missing BLK_PERM_WRITE and asserts in
> bdrv_co_write_req_prepare() (block/io.c) on an entirely ordinary
> close -- closing is expected, the dirty bit on a read-only node
> is not.
>
> Skip the clear for read-only nodes, same as read access already does.
> Any other still-dirty node keeps the unguarded write: it is expected
> to hold write permission, and a missing one there is a bug worth
> seeing.
>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Hanna Reitz <hreitz@redhat.com>
> ---
>  block/qcow2.c              |  6 +++++-
>  tests/qemu-iotests/039     | 11 +++++++++++
>  tests/qemu-iotests/039.out |  3 +++
>  3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 2ab6ddd7b0..adb177e6b6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2870,7 +2870,11 @@ static int GRAPH_RDLOCK qcow2_inactivate(BlockDriverState *bs)
>                       strerror(-ret));
>      }
>  
> -    if (result == 0) {
> +    /*
> +     * A read-only node cannot resolve an inherited dirty bit here;
> +     * leave it dirty, same as plain read access already does.
> +     */
> +    if (result == 0 && !bdrv_is_read_only(bs)) {
>          qcow2_mark_clean(bs);
>      }
>  
> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index e43e7026ce..94a8bfe754 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -84,6 +84,17 @@ $QEMU_IO -r -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
>  # The dirty bit must be set
>  _qcow2_dump_header | grep incompatible_features
>  
> +echo
> +echo "== Read-only open must not crash on close =="
> +
> +# We must not try to write the QCOW2 header to a read-only image.
> +$QEMU_IMG info --image-opts \
> +    "driver=$IMGFMT,read-only=on,file.driver=file,file.filename=$TEST_IMG,file.read-only=off" \
> +    > /dev/null
> +
> +# The dirty bit must still be set: this open never wrote any guest data
> +_qcow2_dump_header | grep incompatible_features
> +
>  echo
>  echo "== Repairing the image file must succeed =="
>  
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index 8fdbcc528a..c66361128f 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -24,6 +24,9 @@ read 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  incompatible_features     [0]
>  
> +== Read-only open must not crash on close ==
> +incompatible_features     [0]
> +
>  == Repairing the image file must succeed ==
>  ERROR cluster 5 refcount=0 reference=1
>  Rebuilding refcount structure
ping

Re: [PATCH 1/1] qcow2: do not try to clear the dirty bit on a read-only node

[Kevin Wolf]

Am 16.07.2026 um 19:54 hat Denis V. Lunev geschrieben:
> On 7/16/26 17:35, Denis V. Lunev wrote:
> > This email originated from an IP that might not be authorized by the domain it was sent from.
> > Do not click links or open attachments unless it is an email you expected to receive.
> > qcow2_do_close() -> qcow2_inactivate() clears the dirty bit with a
> > plain write to bs->file, unconditionally. A read-only node can still
> > be dirty, inherited from an earlier writable session, and that write
> > then hits a missing BLK_PERM_WRITE and asserts in
> > bdrv_co_write_req_prepare() (block/io.c) on an entirely ordinary
> > close -- closing is expected, the dirty bit on a read-only node
> > is not.
> >
> > Skip the clear for read-only nodes, same as read access already does.
> > Any other still-dirty node keeps the unguarded write: it is expected
> > to hold write permission, and a missing one there is a bug worth
> > seeing.
> >
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Kevin Wolf <kwolf@redhat.com>
> > CC: Hanna Reitz <hreitz@redhat.com>
> > ---
> >  block/qcow2.c              |  6 +++++-
> >  tests/qemu-iotests/039     | 11 +++++++++++
> >  tests/qemu-iotests/039.out |  3 +++
> >  3 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 2ab6ddd7b0..adb177e6b6 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -2870,7 +2870,11 @@ static int GRAPH_RDLOCK qcow2_inactivate(BlockDriverState *bs)
> >                       strerror(-ret));
> >      }
> >  
> > -    if (result == 0) {
> > +    /*
> > +     * A read-only node cannot resolve an inherited dirty bit here;
> > +     * leave it dirty, same as plain read access already does.
> > +     */
> > +    if (result == 0 && !bdrv_is_read_only(bs)) {
> >          qcow2_mark_clean(bs);
> >      }
> >  
> > diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> > index e43e7026ce..94a8bfe754 100755
> > --- a/tests/qemu-iotests/039
> > +++ b/tests/qemu-iotests/039
> > @@ -84,6 +84,17 @@ $QEMU_IO -r -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
> >  # The dirty bit must be set
> >  _qcow2_dump_header | grep incompatible_features
> >  
> > +echo
> > +echo "== Read-only open must not crash on close =="
> > +
> > +# We must not try to write the QCOW2 header to a read-only image.
> > +$QEMU_IMG info --image-opts \
> > +    "driver=$IMGFMT,read-only=on,file.driver=file,file.filename=$TEST_IMG,file.read-only=off" \
> > +    > /dev/null
> > +
> > +# The dirty bit must still be set: this open never wrote any guest data
> > +_qcow2_dump_header | grep incompatible_features
> > +
> >  echo
> >  echo "== Repairing the image file must succeed =="
> >  
> > diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> > index 8fdbcc528a..c66361128f 100644
> > --- a/tests/qemu-iotests/039.out
> > +++ b/tests/qemu-iotests/039.out
> > @@ -24,6 +24,9 @@ read 512/512 bytes at offset 0
> >  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >  incompatible_features     [0]
> >  
> > +== Read-only open must not crash on close ==
> > +incompatible_features     [0]
> > +
> >  == Repairing the image file must succeed ==
> >  ERROR cluster 5 refcount=0 reference=1
> >  Rebuilding refcount structure
> Actually the problem is more subtle. The following command line results in
> 
> qemu-storage-daemon \                                                   
>   --blockdev
> '{"driver":"file","filename":"dirty.qcow2","aio":"threads","node-name":"file0","cache":{"direct":false,"no-flush":false},"auto-read-only":true,"discard":"unmap"}'
> \                    
>   --blockdev
> '{"node-name":"fmt0","read-only":true,"discard":"unmap","cache":{"direct":false,"no-flush":false},"driver":"qcow2","file":"file0"}'
> \                                                   
>   --chardev socket,id=qmp,path=master.sock,server=on,wait=off \         
>   --monitor chardev=qmp                                                 
>                                                          
> via query-named-block-nodes:
> 
>   node-name=fmt0     drv=qcow2  ro=True
>   node-name=file0    drv=file   ro=False
> 
> which would come to exactly configuration being asserted in the test
> and this setup is pretty legal at my opinion. In production we have
> had something lengthier with more snapshots.

Yes, I don't see a problem with it.

However, I also don't get a crash or anything with your QSD command line
and a dirty image with this patch applied, so what is the part that is
still incorrect?

What is the final result that you're seeing here?

> File descriptor in turn is opened in a correct way - with O_RDONLY
> that is why the problem was hidden for a lot of time.
> 
> Would it be more reasonable to disallow to set dirty rather than
> to ignore is open question to me. Playing with permissions and
> changing bdrv_is_read_only() seems more dangerous.

I don't follow, sorry. The patch fixes a code path that marks the image
as clean. Setting the dirty flag is the opposite operation and happens
here before we even opened the image. How would you disallow that?

> Also worth to note that bdrv_co_write_req_prepare() has wrong check.
> It should check file descriptor rather than administrative permission.

That doesn't sounds right. bdrv_co_write_req_prepare() is a generic
block layer function, the file descriptor is a file-posix implementation
detail.

> This looks toooooooooo complex to me :(
> 
> Den

Kevin

Re: [PATCH 1/1] qcow2: do not try to clear the dirty bit on a read-only node

[Denis V. Lunev]

On 7/24/26 18:54, Kevin Wolf wrote:
> Am 16.07.2026 um 19:54 hat Denis V. Lunev geschrieben:
>> On 7/16/26 17:35, Denis V. Lunev wrote:
>>> This email originated from an IP that might not be authorized by the domain it was sent from.
>>> Do not click links or open attachments unless it is an email you expected to receive.
>>> qcow2_do_close() -> qcow2_inactivate() clears the dirty bit with a
>>> plain write to bs->file, unconditionally. A read-only node can still
>>> be dirty, inherited from an earlier writable session, and that write
>>> then hits a missing BLK_PERM_WRITE and asserts in
>>> bdrv_co_write_req_prepare() (block/io.c) on an entirely ordinary
>>> close -- closing is expected, the dirty bit on a read-only node
>>> is not.
>>>
>>> Skip the clear for read-only nodes, same as read access already does.
>>> Any other still-dirty node keeps the unguarded write: it is expected
>>> to hold write permission, and a missing one there is a bug worth
>>> seeing.
>>>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>>> CC: Hanna Reitz <hreitz@redhat.com>
>>> ---
>>>  block/qcow2.c              |  6 +++++-
>>>  tests/qemu-iotests/039     | 11 +++++++++++
>>>  tests/qemu-iotests/039.out |  3 +++
>>>  3 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index 2ab6ddd7b0..adb177e6b6 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -2870,7 +2870,11 @@ static int GRAPH_RDLOCK qcow2_inactivate(BlockDriverState *bs)
>>>                       strerror(-ret));
>>>      }
>>>  
>>> -    if (result == 0) {
>>> +    /*
>>> +     * A read-only node cannot resolve an inherited dirty bit here;
>>> +     * leave it dirty, same as plain read access already does.
>>> +     */
>>> +    if (result == 0 && !bdrv_is_read_only(bs)) {
>>>          qcow2_mark_clean(bs);
>>>      }
>>>  
>>> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
>>> index e43e7026ce..94a8bfe754 100755
>>> --- a/tests/qemu-iotests/039
>>> +++ b/tests/qemu-iotests/039
>>> @@ -84,6 +84,17 @@ $QEMU_IO -r -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
>>>  # The dirty bit must be set
>>>  _qcow2_dump_header | grep incompatible_features
>>>  
>>> +echo
>>> +echo "== Read-only open must not crash on close =="
>>> +
>>> +# We must not try to write the QCOW2 header to a read-only image.
>>> +$QEMU_IMG info --image-opts \
>>> +    "driver=$IMGFMT,read-only=on,file.driver=file,file.filename=$TEST_IMG,file.read-only=off" \
>>> +    > /dev/null
>>> +
>>> +# The dirty bit must still be set: this open never wrote any guest data
>>> +_qcow2_dump_header | grep incompatible_features
>>> +
>>>  echo
>>>  echo "== Repairing the image file must succeed =="
>>>  
>>> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
>>> index 8fdbcc528a..c66361128f 100644
>>> --- a/tests/qemu-iotests/039.out
>>> +++ b/tests/qemu-iotests/039.out
>>> @@ -24,6 +24,9 @@ read 512/512 bytes at offset 0
>>>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>  incompatible_features     [0]
>>>  
>>> +== Read-only open must not crash on close ==
>>> +incompatible_features     [0]
>>> +
>>>  == Repairing the image file must succeed ==
>>>  ERROR cluster 5 refcount=0 reference=1
>>>  Rebuilding refcount structure
>> Actually the problem is more subtle. The following command line results in
>>
>> qemu-storage-daemon \                                                   
>>   --blockdev
>> '{"driver":"file","filename":"dirty.qcow2","aio":"threads","node-name":"file0","cache":{"direct":false,"no-flush":false},"auto-read-only":true,"discard":"unmap"}'
>> \                    
>>   --blockdev
>> '{"node-name":"fmt0","read-only":true,"discard":"unmap","cache":{"direct":false,"no-flush":false},"driver":"qcow2","file":"file0"}'
>> \                                                   
>>   --chardev socket,id=qmp,path=master.sock,server=on,wait=off \         
>>   --monitor chardev=qmp                                                 
>>                                                          
>> via query-named-block-nodes:
>>
>>   node-name=fmt0     drv=qcow2  ro=True
>>   node-name=file0    drv=file   ro=False
>>
>> which would come to exactly configuration being asserted in the test
>> and this setup is pretty legal at my opinion. In production we have
>> had something lengthier with more snapshots.
> Yes, I don't see a problem with it.
>
> However, I also don't get a crash or anything with your QSD command line
> and a dirty image with this patch applied, so what is the part that is
> still incorrect?
>
> What is the final result that you're seeing here?

  === Image setup ===
  qemu-img create -f qcow2 -o compat=1.1,lazy_refcounts=on dirty.qcow2 64M
  qemu-io -c "write -P 0x5a 0 512" -c "sigraise 9" dirty.qcow2
  # expect: "wrote 512/512 bytes..." followed by "Killed" from the shell

  qemu-img info --output=json dirty.qcow2 | grep dirty-flag
  # expect: "dirty-flag": true

  === Launch daemon ===
  ./build/storage-daemon/qemu-storage-daemon \
    --blockdev
'{"driver":"file","filename":"dirty.qcow2","aio":"threads","node-name":"file0","cache":{"direct":false,"no-flush":false},"auto-read-only":true,"discard":"unmap"}'
\
    --blockdev
'{"node-name":"fmt0","read-only":true,"discard":"unmap","cache":{"direct":false,"no-flush":false},"driver":"qcow2","file":"file0"}'
\
    --chardev socket,id=qmp,path=master.sock,server=on,wait=off \
    --monitor chardev=qmp \
    > master.log 2>&1 &

  === Connect and drive it ===
  ./build/run qmp-shell master.sock

  (QEMU) quit

Expected result:
[1]+  Aborted (core dumped) ./build/storage-daemon/qemu-storage-daemon

Double checked this on current QEMU master.

Here I have just stated that the case inside the test I have added
in the thread starter is not artificial and this is not a mistake.
Such a command line has been generated by libvirt for me.

>> File descriptor in turn is opened in a correct way - with O_RDONLY
>> that is why the problem was hidden for a lot of time.
>>
>> Would it be more reasonable to disallow to set dirty rather than
>> to ignore is open question to me. Playing with permissions and
>> changing bdrv_is_read_only() seems more dangerous.
> I don't follow, sorry. The patch fixes a code path that marks the image
> as clean. Setting the dirty flag is the opposite operation and happens
> here before we even opened the image. How would you disallow that?
>
>> Also worth to note that bdrv_co_write_req_prepare() has wrong check.
>> It should check file descriptor rather than administrative permission.
> That doesn't sounds right. bdrv_co_write_req_prepare() is a generic
> block layer function, the file descriptor is a file-posix implementation
> detail.
OK. You say that at the bdrv_co_write_req_prepare() we check
only possible permission and that is ok. That was really
unexpected to me and that is why I have wonders. Designed this
way, no changes going in.

I can live with that :-)

Den