Re: iotest failure -- test possibly not using sufficiently unique temp filename?

[Thomas Huth]

On 18/10/2019 10.42, Max Reitz wrote:
> On 18.10.19 08:20, Thomas Huth wrote:
>> On 17/10/2019 18.41, Peter Maydell wrote:
>>> On Fri, 27 Sep 2019 at 17:44, Max Reitz  wrote:

 On 27.09.19 18:39, Peter Maydell wrote:
> Hi; I just saw this iotest failure (on an s390x box, as it happens):
>
>   TESTiotest-qcow2: 130 [fail]
> QEMU  --
> "/home/linux1/qemu/build/all/tests/qemu-iotests/../../s390x-softmmu/qemu-system-s390x"
> -nodefaults -display none -machine accel=qtest
> QEMU_IMG  -- 
> "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-img"
> QEMU_IO   --
> "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-io"
> --cache writeback -f qcow2
> QEMU_NBD  -- 
> "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-nbd"
> IMGFMT-- qcow2 (compat=1.1)
> IMGPROTO  -- file
> PLATFORM  -- Linux/s390x lxub05 4.15.0-58-generic
> TEST_DIR  -- /home/linux1/qemu/build/all/tests/qemu-iotests/scratch
> SOCKET_SCM_HELPER --
> /home/linux1/qemu/build/all/tests/qemu-iotests/socket_scm_helper
>
> --- /home/linux1/qemu/tests/qemu-iotests/130.out2019-05-10
> 12:27:16.948075733 -0400
> +++ /home/linux1/qemu/build/all/tests/qemu-iotests/130.out.bad
> 2019-09-27 12:01:23.649722655 -0400
> @@ -18,20 +18,22 @@
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) commit testdisk
>  (qemu)
> -image: TEST_DIR/t.IMGFMT
> -file format: IMGFMT
> -virtual size: 64 MiB (67108864 bytes)
> -backing file: TEST_DIR/t.IMGFMT.orig
> -backing file format: raw
> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared 
> "write" lock
> +Is another process using the image [TEST_DIR/t.IMGFMT]?
>
>  === Marking image dirty (lazy refcounts) ===
>
> +qemu-img: TEST_DIR/t.IMGFMT: Failed to get "write" lock
> +Is another process using the image [TEST_DIR/t.IMGFMT]?
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> -wrote 4096/4096 bytes at offset 0
> -4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +qemu-io: can't open device
> /home/linux1/qemu/build/all/tests/qemu-iotests/scratch/t.qcow2: Failed
> to get "write" lock
> +Is another process using the image
> [/home/linux1/qemu/build/all/tests/qemu-iotests/scratch/t.qcow2]?
> +no file open, try 'help open'
>  image: TEST_DIR/t.IMGFMT
>  file format: IMGFMT
>  virtual size: 64 MiB (67108864 bytes)
> +backing file: TEST_DIR/t.IMGFMT.orig
> +backing file format: raw
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
> backing_file=TEST_DIR/t.IMGFMT.orig backing_fmt=raw
>  wrote 4096/4096 bytes at offset 0
>  4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>
>
>
> This looks suspiciously like the test isn't using a unique
> filename for its disk image: "qemu-iotests/scratch/t.qcow2"
> in the build directory, and so perhaps it has collided with
> another iotest ?
>
> If we run 'make check' with a -j option do the
> iotests all get run serially anyway, or do they run in
> parallel against each other ?

 As far as I know, all iotests are executed serially.  Anything else
 would not work with the same scratch directory.

 The only thing I suspect is that some tool has been accidentally left
 running by some previous test that still accesses its own image.  But I
 don’t know.
>>>
>>> Just saw this one again with the same iotest 130 on the same
>>> s390 box; only difference is that the log this time around
>>> has the first part where qemu-img fails, but not the second part
>>> where qemu-io fails:
>>>
>>> --- /home/linux1/qemu/tests/qemu-iotests/130.out2019-05-10
>>> 12:27:16.948075733 -0400
>>> +++ /home/linux1/qemu/build/all/tests/qemu-iotests/130.out.bad
>>> 2019-10-17 11:56:43.450750873 -0400
>>> @@ -18,11 +18,8 @@
>>>  QEMU X.Y.Z monitor - type 'help' for more information
>>>  (qemu) commit testdisk
>>>  (qemu)
>>> -image: TEST_DIR/t.IMGFMT
>>> -file format: IMGFMT
>>> -virtual size: 64 MiB (67108864 bytes)
>>> -backing file: TEST_DIR/t.IMGFMT.orig
>>> -backing file format: raw
>>> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared "write" 
>>> lock
>>> +Is another process using the image [TEST_DIR/t.IMGFMT]?
>>>
>>>  === Marking image dirty (lazy refcounts) ===
>>>
>>> On the host machine there don't seem to be any stray
>>> processes which might have held the file open, and
>>> indeed the file doesn't exist at all, so it got removed
>>> by some cleanup or other.
>>
>> Ok, so unless someone has a clue what might be going on here (is there a
>> race in the test?), I'd suggest that we simply remove 130 from the auto
>> group again. Shall I send a patch?
> 
> I don’t have much of an idea.  It looks like maybe the qemu process
> (which dos the 

Re: [PATCH v8 3/3] docs: qcow2: introduce compression type feature

[Eric Blake]

On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:

The patch add new additional field to qcow2 header: compression_type,
which specifies compression type. If field is absent or zero, default
compression type is set: ZLIB, which corresponds to current behavior.

New compression type (ZSTD) is to be added in further commit.

Suggested-by: Denis Plotnikov 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  docs/interop/qcow2.txt | 19 ++-
  1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index b971e59b1a..4eabd81363 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -109,6 +109,12 @@ least next five fields, up to the @header_length field.
  An External Data File Name header extension 
may
  be present if this bit is set.
  
+Bit 3:  Compression type bit.  If this bit is set,

+non-default compression is used for compressed
+clusters. In this case, @header_length must
+be at least 105 and @compression_type field
+must be non-zero.


I want to insist on 8-byte alignment, so this should be at least 112.

Also, as pointed out in v7, use of the decoration '@header_length' and 
'@compression_type' is not consistent with the rest of the document. 
Drop the @.



+
  Bits 3-63:  Reserved (set to 0)
  
   80 -  87:  compatible_features

@@ -183,7 +189,18 @@ It's allowed for the header end to cut some field in the 
middle (in this case
  the field is considered as absent), but in this case the part of the field
  which is covered by @header_length must be zeroed.
  
-< ... No additional fields in the header currently ... >

+  104:  compression_type
+Defines the compression method used for compressed 
clusters.
+A single compression type is applied to all compressed 
image
+clusters.
+If incompatible compression type bit is set: the field must
+exist (i.e. @header_length >= 105) and must be non-zero (
+which means non-zlib compression type)
+If incompatible compression type bit is unset: the field
+may not exist (if @header_length < 105) or it must be zero
+(which means zlib).
+Available compression type values:
+0: zlib 
  


One byte for the field is fine, but I'd still explicitly document 7 
bytes of unused padding, since I want to insist on an 8-byte multiple.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: iotest failure -- test possibly not using sufficiently unique temp filename?

[Max Reitz]

On 18.10.19 08:20, Thomas Huth wrote:
> On 17/10/2019 18.41, Peter Maydell wrote:
>> On Fri, 27 Sep 2019 at 17:44, Max Reitz  wrote:
>>>
>>> On 27.09.19 18:39, Peter Maydell wrote:
 Hi; I just saw this iotest failure (on an s390x box, as it happens):

   TESTiotest-qcow2: 130 [fail]
 QEMU  --
 "/home/linux1/qemu/build/all/tests/qemu-iotests/../../s390x-softmmu/qemu-system-s390x"
 -nodefaults -display none -machine accel=qtest
 QEMU_IMG  -- 
 "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-img"
 QEMU_IO   --
 "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-io"
 --cache writeback -f qcow2
 QEMU_NBD  -- 
 "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-nbd"
 IMGFMT-- qcow2 (compat=1.1)
 IMGPROTO  -- file
 PLATFORM  -- Linux/s390x lxub05 4.15.0-58-generic
 TEST_DIR  -- /home/linux1/qemu/build/all/tests/qemu-iotests/scratch
 SOCKET_SCM_HELPER --
 /home/linux1/qemu/build/all/tests/qemu-iotests/socket_scm_helper

 --- /home/linux1/qemu/tests/qemu-iotests/130.out2019-05-10
 12:27:16.948075733 -0400
 +++ /home/linux1/qemu/build/all/tests/qemu-iotests/130.out.bad
 2019-09-27 12:01:23.649722655 -0400
 @@ -18,20 +18,22 @@
  QEMU X.Y.Z monitor - type 'help' for more information
  (qemu) commit testdisk
  (qemu)
 -image: TEST_DIR/t.IMGFMT
 -file format: IMGFMT
 -virtual size: 64 MiB (67108864 bytes)
 -backing file: TEST_DIR/t.IMGFMT.orig
 -backing file format: raw
 +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared 
 "write" lock
 +Is another process using the image [TEST_DIR/t.IMGFMT]?

  === Marking image dirty (lazy refcounts) ===

 +qemu-img: TEST_DIR/t.IMGFMT: Failed to get "write" lock
 +Is another process using the image [TEST_DIR/t.IMGFMT]?
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 -wrote 4096/4096 bytes at offset 0
 -4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 +qemu-io: can't open device
 /home/linux1/qemu/build/all/tests/qemu-iotests/scratch/t.qcow2: Failed
 to get "write" lock
 +Is another process using the image
 [/home/linux1/qemu/build/all/tests/qemu-iotests/scratch/t.qcow2]?
 +no file open, try 'help open'
  image: TEST_DIR/t.IMGFMT
  file format: IMGFMT
  virtual size: 64 MiB (67108864 bytes)
 +backing file: TEST_DIR/t.IMGFMT.orig
 +backing file format: raw
  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 backing_file=TEST_DIR/t.IMGFMT.orig backing_fmt=raw
  wrote 4096/4096 bytes at offset 0
  4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)



 This looks suspiciously like the test isn't using a unique
 filename for its disk image: "qemu-iotests/scratch/t.qcow2"
 in the build directory, and so perhaps it has collided with
 another iotest ?

 If we run 'make check' with a -j option do the
 iotests all get run serially anyway, or do they run in
 parallel against each other ?
>>>
>>> As far as I know, all iotests are executed serially.  Anything else
>>> would not work with the same scratch directory.
>>>
>>> The only thing I suspect is that some tool has been accidentally left
>>> running by some previous test that still accesses its own image.  But I
>>> don’t know.
>>
>> Just saw this one again with the same iotest 130 on the same
>> s390 box; only difference is that the log this time around
>> has the first part where qemu-img fails, but not the second part
>> where qemu-io fails:
>>
>> --- /home/linux1/qemu/tests/qemu-iotests/130.out2019-05-10
>> 12:27:16.948075733 -0400
>> +++ /home/linux1/qemu/build/all/tests/qemu-iotests/130.out.bad
>> 2019-10-17 11:56:43.450750873 -0400
>> @@ -18,11 +18,8 @@
>>  QEMU X.Y.Z monitor - type 'help' for more information
>>  (qemu) commit testdisk
>>  (qemu)
>> -image: TEST_DIR/t.IMGFMT
>> -file format: IMGFMT
>> -virtual size: 64 MiB (67108864 bytes)
>> -backing file: TEST_DIR/t.IMGFMT.orig
>> -backing file format: raw
>> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared "write" 
>> lock
>> +Is another process using the image [TEST_DIR/t.IMGFMT]?
>>
>>  === Marking image dirty (lazy refcounts) ===
>>
>> On the host machine there don't seem to be any stray
>> processes which might have held the file open, and
>> indeed the file doesn't exist at all, so it got removed
>> by some cleanup or other.
> 
> Ok, so unless someone has a clue what might be going on here (is there a
> race in the test?), I'd suggest that we simply remove 130 from the auto
> group again. Shall I send a patch?

I don’t have much of an idea.  It looks like maybe the qemu process
(which dos the commit) is lingering, but that shouldn’t be because
_cleanup_qemu always waits for it.  (Also, I can’t reproduce the problem
on my system.)

The only hunch that I 

Re: iotest failure -- test possibly not using sufficiently unique temp filename?

[Thomas Huth]

On 17/10/2019 18.41, Peter Maydell wrote:
> On Fri, 27 Sep 2019 at 17:44, Max Reitz  wrote:
>>
>> On 27.09.19 18:39, Peter Maydell wrote:
>>> Hi; I just saw this iotest failure (on an s390x box, as it happens):
>>>
>>>   TESTiotest-qcow2: 130 [fail]
>>> QEMU  --
>>> "/home/linux1/qemu/build/all/tests/qemu-iotests/../../s390x-softmmu/qemu-system-s390x"
>>> -nodefaults -display none -machine accel=qtest
>>> QEMU_IMG  -- 
>>> "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-img"
>>> QEMU_IO   --
>>> "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-io"
>>> --cache writeback -f qcow2
>>> QEMU_NBD  -- 
>>> "/home/linux1/qemu/build/all/tests/qemu-iotests/../../qemu-nbd"
>>> IMGFMT-- qcow2 (compat=1.1)
>>> IMGPROTO  -- file
>>> PLATFORM  -- Linux/s390x lxub05 4.15.0-58-generic
>>> TEST_DIR  -- /home/linux1/qemu/build/all/tests/qemu-iotests/scratch
>>> SOCKET_SCM_HELPER --
>>> /home/linux1/qemu/build/all/tests/qemu-iotests/socket_scm_helper
>>>
>>> --- /home/linux1/qemu/tests/qemu-iotests/130.out2019-05-10
>>> 12:27:16.948075733 -0400
>>> +++ /home/linux1/qemu/build/all/tests/qemu-iotests/130.out.bad
>>> 2019-09-27 12:01:23.649722655 -0400
>>> @@ -18,20 +18,22 @@
>>>  QEMU X.Y.Z monitor - type 'help' for more information
>>>  (qemu) commit testdisk
>>>  (qemu)
>>> -image: TEST_DIR/t.IMGFMT
>>> -file format: IMGFMT
>>> -virtual size: 64 MiB (67108864 bytes)
>>> -backing file: TEST_DIR/t.IMGFMT.orig
>>> -backing file format: raw
>>> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared "write" 
>>> lock
>>> +Is another process using the image [TEST_DIR/t.IMGFMT]?
>>>
>>>  === Marking image dirty (lazy refcounts) ===
>>>
>>> +qemu-img: TEST_DIR/t.IMGFMT: Failed to get "write" lock
>>> +Is another process using the image [TEST_DIR/t.IMGFMT]?
>>>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>>> -wrote 4096/4096 bytes at offset 0
>>> -4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>> +qemu-io: can't open device
>>> /home/linux1/qemu/build/all/tests/qemu-iotests/scratch/t.qcow2: Failed
>>> to get "write" lock
>>> +Is another process using the image
>>> [/home/linux1/qemu/build/all/tests/qemu-iotests/scratch/t.qcow2]?
>>> +no file open, try 'help open'
>>>  image: TEST_DIR/t.IMGFMT
>>>  file format: IMGFMT
>>>  virtual size: 64 MiB (67108864 bytes)
>>> +backing file: TEST_DIR/t.IMGFMT.orig
>>> +backing file format: raw
>>>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>>> backing_file=TEST_DIR/t.IMGFMT.orig backing_fmt=raw
>>>  wrote 4096/4096 bytes at offset 0
>>>  4 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>
>>>
>>>
>>> This looks suspiciously like the test isn't using a unique
>>> filename for its disk image: "qemu-iotests/scratch/t.qcow2"
>>> in the build directory, and so perhaps it has collided with
>>> another iotest ?
>>>
>>> If we run 'make check' with a -j option do the
>>> iotests all get run serially anyway, or do they run in
>>> parallel against each other ?
>>
>> As far as I know, all iotests are executed serially.  Anything else
>> would not work with the same scratch directory.
>>
>> The only thing I suspect is that some tool has been accidentally left
>> running by some previous test that still accesses its own image.  But I
>> don’t know.
> 
> Just saw this one again with the same iotest 130 on the same
> s390 box; only difference is that the log this time around
> has the first part where qemu-img fails, but not the second part
> where qemu-io fails:
> 
> --- /home/linux1/qemu/tests/qemu-iotests/130.out2019-05-10
> 12:27:16.948075733 -0400
> +++ /home/linux1/qemu/build/all/tests/qemu-iotests/130.out.bad
> 2019-10-17 11:56:43.450750873 -0400
> @@ -18,11 +18,8 @@
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) commit testdisk
>  (qemu)
> -image: TEST_DIR/t.IMGFMT
> -file format: IMGFMT
> -virtual size: 64 MiB (67108864 bytes)
> -backing file: TEST_DIR/t.IMGFMT.orig
> -backing file format: raw
> +qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared "write" 
> lock
> +Is another process using the image [TEST_DIR/t.IMGFMT]?
> 
>  === Marking image dirty (lazy refcounts) ===
> 
> On the host machine there don't seem to be any stray
> processes which might have held the file open, and
> indeed the file doesn't exist at all, so it got removed
> by some cleanup or other.

Ok, so unless someone has a clue what might be going on here (is there a
race in the test?), I'd suggest that we simply remove 130 from the auto
group again. Shall I send a patch?

 Thomas

Re: [PATCH v2 01/23] iotests: Introduce $SOCK_DIR

[Max Reitz]

On 17.10.19 16:52, Eric Blake wrote:
> On 10/17/19 8:31 AM, Max Reitz wrote:
>> Unix sockets generally have a maximum path length.  Depending on your
>> $TEST_DIR, it may be exceeded and then all tests that create and use
>> Unix sockets there may fail.
>>
>> Circumvent this by adding a new scratch directory specifically for
>> Unix socket files.  It defaults to a temporary directory (mktemp -d)
>> that is completely removed after the iotests are done.
>>
>> (By default, mktemp -d creates a /tmp/tmp.XX directory, which
>> should be short enough for our use cases.)
>>
>> Use mkdir -p to create the directory (because it seems right), and do
>> the same for $TEST_DIR (because there is no reason for that to be
>> created in any different way).
>>
>> Signed-off-by: Max Reitz 
>> ---
>>   tests/qemu-iotests/check | 15 +--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
>> @@ -116,10 +117,14 @@ set_prog_path()
>>   if [ -z "$TEST_DIR" ]; then
>>   TEST_DIR=$PWD/scratch
>>   fi
>> +mkdir -p "$TEST_DIR" || _init_error 'Failed to create TEST_DIR'
> 
> This one seems fine. We are either using the user's name (and if it is
> pre-existing, not fail) or using a well-known name (if someone else
> slams in files into that directory in parallel with our test run, oh
> well).  But at least the well-known name is a directory that is probably
> already accessible only to the current user, not world-writable.
> 
>>   -if [ ! -e "$TEST_DIR" ]; then
>> -    mkdir "$TEST_DIR"
>> +tmp_sock_dir=false
>> +if [ -z "$SOCK_DIR" ]; then
>> +    SOCK_DIR=$(mktemp -d)
>> +    tmp_sock_dir=true
>>   fi
>> +mkdir -p "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
> 
> Thinking about this again: if the user passed in a name, we probably
> want to use it no matter whether the directory already exists (mkdir -p
> makes sense: either the directory did not exist, or the user is in
> charge of passing us a directory that they already secured).  But if we
> generate our own name in a world-writable location in /tmp, using mkdir
> -p means someone else can race us to the creation of the directory, and
> potentially populate it in a way to cause us a security hole while we
> execute our tests.

I don’t quite see how this is a security hole.  mktemp -d creates the
directory, so noone can race us.

Max

> I would be a bit more comfortable with:
> 
> tmp_sock_dir=false
> tmp_sock_opt=-p
> if [ -z "$SOCK_DIR" ]; then
>     SOCK_DIR=$(mktemp -d)
>     tmp_sock_dir=true
>     tmp_sock_opt=  # disable -p for our generated name
> fi
> mkdir $tmp_sock_opt "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

08.10.2019 12:05, Vladimir Sementsov-Ogievskiy wrote:
> 07.10.2019 23:21, Eric Blake wrote:
>> On 10/7/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Make it more obvious how to add new fields to the version 3 header and
>>> how to interpret them.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   docs/interop/qcow2.txt | 26 +++---
>>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>>> index af5711e533..3f2855593f 100644
>>> --- a/docs/interop/qcow2.txt
>>> +++ b/docs/interop/qcow2.txt
>>> @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file 
>>> header:
>>>   Offset into the image file at which the snapshot table
>>>   starts. Must be aligned to a cluster boundary.
>>> -If the version is 3 or higher, the header has the following additional 
>>> fields.
>>> -For version 2, the values are assumed to be zero, unless specified 
>>> otherwise
>>> -in the description of a field.
>>> +For version 2, header is always 72 bytes length and finishes here.
>>> +For version 3 or higher the header length is at least 104 bytes and has at
>>> +least next five fields, up to the @header_length field.
>>
>> This hunk seems okay.
>>
>>>    72 -  79:  incompatible_features
>>>   Bitmask of incompatible features. An implementation 
>>> must
>>> @@ -165,6 +165,26 @@ in the description of a field.
>>>   Length of the header structure in bytes. For version 2
>>>   images, the length is always assumed to be 72 bytes.
>>> +Additional fields (version 3 and higher)
>>> +
>>> +The following fields of the header are optional: if software don't know 
>>> how to
>>> +interpret the field, it may safely ignore it. Still the field must be kept 
>>> as is
>>> +when rewriting the image.
>>
>> if software doesn't know how to interpret the field, it may be safely 
>> ignored, other than preserving the field unchanged when rewriting the image 
>> header.
>>
>> Missing:
>>
>> If header_length excludes an optional field, the value of 0 should be used 
>> for that field.
> 
> This is what I dislike in old wording. Why do we need this default-zero 
> thing[*]? What is the default?
> 
> Default is absence of the feature, we don't have these future features now 
> and don't care of them.
> What is this default 0 for us now? Nothing.
> 
> Consider some future version: if it sees that header_length excludes some 
> fields, it understands,
> that there is no such feature here. That's all. Work without it. The feature 
> itself should declare
> behavior without this feature, which should correspond to behavior before 
> this feature introduction..
> 
> So at least, I don't like "the value of 0 should be used for that field", as 
> instances of Qemu which
> don't know about the feature will ignore this requirement, as they don't need 
> any value of that
> field at all.
> 
> What you actually mean, IMHO, is: for all optional field 0 value must be 
> equal to absence of the feature,
> like when header_length excludes this field. I don't see, do we really need 
> this requirement, but
> seems it was mentioned before this patch and we'd better keep it.. I just 
> don't like concept of
> "default" value keeping in mind valid Qemu instances which don't know about 
> field at all.
> 
>>
>>> @header_length must be bound to the end of one of
>>> +these fields (or to @header_length field end itself, to be 104 bytes).
>>
>> We don't use the @header_length markup anywhere else in this file, starting 
>> to do so here is odd.
>>
>> I would suggest a stronger requirement:
>>
>> header_length must be a multiple of 4, and must not land in the middle of 
>> any optional 8-byte field.
>>
>> Or maybe even add our compression type extension with 4 bytes of padding, so 
>> that we could go even stronger:
>>
>> header_length must be a multiple of 8.
> 
> Hmm, if we imply that software will have to add some padding, than 
> requirement above about zero === feature-absence
> becomes necessary. [*]
> 
> Still I have two questions:
> 1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for 
> compression?
> 2. What is the benefit of padding, which you propose?

Hmm, now I think, that we should align header to multiply of 8, as header 
extensions are already have
"""
Directly after the image header, optional sections called header extensions can
be stored. Each extension has a structure like the following:

[...]

   n -  m:   Padding to round up the header extension size to the next
 multiple of 8.
"""

So, it looks inconsistent, if we pad all header extensions to  8 bytes except 
for the start of the first extension.

I'll resend with padding soon.

> 
>>
>>> +This definition implies the following:
>>> +1. Software may support some of these optional fields and ignore the 
>>> others,
>>> +   which 

Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

18.10.2019 11:29, Vladimir Sementsov-Ogievskiy wrote:
> 08.10.2019 12:05, Vladimir Sementsov-Ogievskiy wrote:
>> 07.10.2019 23:21, Eric Blake wrote:
>>> On 10/7/19 11:04 AM, Vladimir Sementsov-Ogievskiy wrote:
 Make it more obvious how to add new fields to the version 3 header and
 how to interpret them.

 Signed-off-by: Vladimir Sementsov-Ogievskiy 
 ---
   docs/interop/qcow2.txt | 26 +++---
   1 file changed, 23 insertions(+), 3 deletions(-)

 diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
 index af5711e533..3f2855593f 100644
 --- a/docs/interop/qcow2.txt
 +++ b/docs/interop/qcow2.txt
 @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file 
 header:
   Offset into the image file at which the snapshot 
 table
   starts. Must be aligned to a cluster boundary.
 -If the version is 3 or higher, the header has the following additional 
 fields.
 -For version 2, the values are assumed to be zero, unless specified 
 otherwise
 -in the description of a field.
 +For version 2, header is always 72 bytes length and finishes here.
 +For version 3 or higher the header length is at least 104 bytes and has at
 +least next five fields, up to the @header_length field.
>>>
>>> This hunk seems okay.
>>>
    72 -  79:  incompatible_features
   Bitmask of incompatible features. An implementation 
 must
 @@ -165,6 +165,26 @@ in the description of a field.
   Length of the header structure in bytes. For version 
 2
   images, the length is always assumed to be 72 bytes.
 +Additional fields (version 3 and higher)
 +
 +The following fields of the header are optional: if software don't know 
 how to
 +interpret the field, it may safely ignore it. Still the field must be 
 kept as is
 +when rewriting the image.
>>>
>>> if software doesn't know how to interpret the field, it may be safely 
>>> ignored, other than preserving the field unchanged when rewriting the image 
>>> header.
>>>
>>> Missing:
>>>
>>> If header_length excludes an optional field, the value of 0 should be used 
>>> for that field.
>>
>> This is what I dislike in old wording. Why do we need this default-zero 
>> thing[*]? What is the default?
>>
>> Default is absence of the feature, we don't have these future features now 
>> and don't care of them.
>> What is this default 0 for us now? Nothing.
>>
>> Consider some future version: if it sees that header_length excludes some 
>> fields, it understands,
>> that there is no such feature here. That's all. Work without it. The feature 
>> itself should declare
>> behavior without this feature, which should correspond to behavior before 
>> this feature introduction..
>>
>> So at least, I don't like "the value of 0 should be used for that field", as 
>> instances of Qemu which
>> don't know about the feature will ignore this requirement, as they don't 
>> need any value of that
>> field at all.
>>
>> What you actually mean, IMHO, is: for all optional field 0 value must be 
>> equal to absence of the feature,
>> like when header_length excludes this field. I don't see, do we really need 
>> this requirement, but
>> seems it was mentioned before this patch and we'd better keep it.. I just 
>> don't like concept of
>> "default" value keeping in mind valid Qemu instances which don't know about 
>> field at all.
>>
>>>
 @header_length must be bound to the end of one of
 +these fields (or to @header_length field end itself, to be 104 bytes).
>>>
>>> We don't use the @header_length markup anywhere else in this file, starting 
>>> to do so here is odd.
>>>
>>> I would suggest a stronger requirement:
>>>
>>> header_length must be a multiple of 4, and must not land in the middle of 
>>> any optional 8-byte field.
>>>
>>> Or maybe even add our compression type extension with 4 bytes of padding, 
>>> so that we could go even stronger:
>>>
>>> header_length must be a multiple of 8.
>>
>> Hmm, if we imply that software will have to add some padding, than 
>> requirement above about zero === feature-absence
>> becomes necessary. [*]
>>
>> Still I have two questions:
>> 1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for 
>> compression?
>> 2. What is the benefit of padding, which you propose?
> 
> Hmm, now I think, that we should align header to multiply of 8, as header 
> extensions are already have
> """
> Directly after the image header, optional sections called header extensions 
> can
> be stored. Each extension has a structure like the following:
> 
> [...]
> 
>    n -  m:   Padding to round up the header extension size to the next
>      multiple of 8.
> """
> 
> So, it looks inconsistent, if we pad all header extensions to  8 bytes except 
> for the start of the 

Re: [PATCH] block/backup: drop dead code from backup_job_create

[Stefano Garzarella]

Hi Vladimir,

On Thu, Oct 17, 2019 at 05:21:22PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> After commit 00e30f05de1d195, there is no more "goto error" points
> after job creation, so after "error:" @job is always NULL and we don't
> need roll-back job creation.

I don't know this code very well, but IIUC only block_job_add_bdrv() could
fail after the job creation, but this shouldn't happen because "Required
permissions are already taken by backup-top target", so it seems safe
for me:

Acked-by: Stefano Garzarella 

Thanks,
Stefano

> 
> Reported-by: Coverity (CID 1406402)
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 46978c1785..6e1497f7bb 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -474,10 +474,7 @@ BlockJob *backup_job_create(const char *job_id, 
> BlockDriverState *bs,
>  if (sync_bitmap) {
>  bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
>  }
> -if (job) {
> -backup_clean(>common.job);
> -job_early_fail(>common.job);
> -} else if (backup_top) {
> +if (backup_top) {
>  bdrv_backup_top_drop(backup_top);
>  }
>  
> -- 
> 2.21.0
> 
> 

Re: [PATCH] iotests: Skip read-only cases in 118 when run as root

[Philippe Mathieu-Daudé]

Hi Kevin,

On 10/18/19 1:51 PM, Kevin Wolf wrote:

Some tests in 118 use chmod to remove write permissions from the file
and assume that the image can indeed not be opened read-write
afterwards. This doesn't work when the test is run as root, because root
can still open the file as writable even when the permission bit isn't
set.

Introduce a @skip_if_root decorator and use it in 118 to skip the tests
in question when the script is run as root.

Signed-off-by: Kevin Wolf 
---
  tests/qemu-iotests/118|  3 +++
  tests/qemu-iotests/iotests.py | 10 ++
  2 files changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index ea0b326ae0..9eff46d189 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -446,6 +446,7 @@ class TestChangeReadOnly(ChangeBaseClass):
  self.assert_qmp(result, 'return[0]/inserted/ro', True)
  self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
  
+@iotests.skip_if_root


Why not have case_notrun() return 'reason' and use:

@unittest.skipIf(os.getuid() == 0, case_notrun("cannot be run as root"))



  def test_rw_ro_retain(self):
  os.chmod(new_img, 0o444)
  self.vm.add_drive(old_img, 'media=disk', 'none')
@@ -530,6 +531,7 @@ class TestChangeReadOnly(ChangeBaseClass):
  self.assert_qmp(result, 'return[0]/inserted/ro', True)
  self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
  
+@iotests.skip_if_root

  def test_make_ro_rw(self):
  os.chmod(new_img, 0o444)
  self.vm.add_drive(old_img, 'media=disk', 'none')
@@ -571,6 +573,7 @@ class TestChangeReadOnly(ChangeBaseClass):
  self.assert_qmp(result, 'return[0]/inserted/ro', True)
  self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
  
+@iotests.skip_if_root

  def test_make_ro_rw_by_retain(self):
  os.chmod(new_img, 0o444)
  self.vm.add_drive(old_img, 'media=disk', 'none')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3a8f378f90..9c66db613e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -920,6 +920,16 @@ def skip_if_unsupported(required_formats=[], 
read_only=False):
  return func_wrapper
  return skip_test_decorator
  
+def skip_if_root(func):


skip_if_user_is_root() is slightly less confuse.


+'''Skip Test Decorator
+   Runs the test only without root permissions'''
+def func_wrapper(*args, **kwargs):
+if os.getuid() == 0:
+case_notrun('{}: cannot be run as root'.format(args[0]))
+else:
+return func(*args, **kwargs)
+return func_wrapper
+
  def execute_unittest(output, verbosity, debug):
  runner = unittest.TextTestRunner(stream=output, descriptions=True,
   verbosity=verbosity)

[PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

Make it more obvious how to add new fields to the version 3 header and
how to interpret them.

The specification is adjusted so for new defined optional fields:

1. Software may support some of these optional fields and ignore the
   others, which means that features may be backported to downstream
   Qemu independently.
3. If @header_length is higher than the highest field end that software
   knows, it should assume that topmost unknown additional fields are
   correct, and keep additional unknown fields as is on rewriting the
   image.
3. If we want to add incompatible field (or a field, for which some its
   values would be incompatible), it must be accompanied by
   incompatible feature bit.

Also the concept of "default is zero" is clarified, as it's strange to
say that the value of the field is assumed to be zero for the software
version which don't know about the field at all and don't know how to
treat it be it zero or not.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 docs/interop/qcow2.txt | 26 +++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index af5711e533..4709f3bb30 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
 Offset into the image file at which the snapshot table
 starts. Must be aligned to a cluster boundary.
 
-If the version is 3 or higher, the header has the following additional fields.
-For version 2, the values are assumed to be zero, unless specified otherwise
-in the description of a field.
+For version 2, header is always 72 bytes length and finishes here.
+For version 3 or higher the header length is at least 104 bytes and has at
+least next five fields, up to the @header_length field.
 
  72 -  79:  incompatible_features
 Bitmask of incompatible features. An implementation must
@@ -164,6 +164,26 @@ in the description of a field.
 100 - 103:  header_length
 Length of the header structure in bytes. For version 2
 images, the length is always assumed to be 72 bytes.
+For version 3 it's at least 104 bytes.
+
+Additional fields (version 3 and higher)
+
+The following fields of the header are optional: if software doesn't know how
+to interpret the field, it may be safely ignored, other than preserving the
+field unchanged when rewriting the image header.
+
+For all additional fields zero value equals to absence of field (absence is
+when field.offset + field.size > @header_length). This implies
+that if software want's to set fields up to some field not aligned to multiply
+of 8 it must align header up by zeroes. And on the other hand, if software
+need some optional field which is absent it should assume that it's value is
+zero.
+
+It's allowed for the header end to cut some field in the middle (in this case
+the field is considered as absent), but in this case the part of the field
+which is covered by @header_length must be zeroed.
+
+< ... No additional fields in the header currently ... >
 
 Directly after the image header, optional sections called header extensions can
 be stored. Each extension has a structure like the following:
-- 
2.21.0

Re: [PATCH v2 01/23] iotests: Introduce $SOCK_DIR

[Eric Blake]

On 10/18/19 4:03 AM, Max Reitz wrote:


   -if [ ! -e "$TEST_DIR" ]; then
-    mkdir "$TEST_DIR"
+tmp_sock_dir=false
+if [ -z "$SOCK_DIR" ]; then
+    SOCK_DIR=$(mktemp -d)
+    tmp_sock_dir=true
   fi
+mkdir -p "$SOCK_DIR" || _init_error 'Failed to create SOCK_DIR'


Thinking about this again: if the user passed in a name, we probably
want to use it no matter whether the directory already exists (mkdir -p
makes sense: either the directory did not exist, or the user is in
charge of passing us a directory that they already secured).  But if we
generate our own name in a world-writable location in /tmp, using mkdir
-p means someone else can race us to the creation of the directory, and
potentially populate it in a way to cause us a security hole while we
execute our tests.


I don’t quite see how this is a security hole.  mktemp -d creates the
directory, so noone can race us.


Aha - I confused 'mktemp -u' (necessary for creating a socket name) and 
'mktemp -d' (for directories).  With that confusion cleared up, yes, the 
directory is safely created (or else the burden is on the caller), so:


Reviewed-by: Eric Blake 

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v8 2/3] docs: define padding for qcow2 header

[Eric Blake]

On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:

Header extensions ends are already defined to be multiply of 8. Let's
gently ask for header length to be a multiply of 8 too, when we have
some additional fields. Requiring this may be considered as an
incompatible change, so the padding is optional. Actually, padding is
allowed before this patch (due to definition of additional fields),
the only actual change is "SHOULD" word.


Too weak. I've already argued that this should be mandatory, and that we 
are not breaking backwards compatibility, but merely clarifying what has 
already been implicit by the fact that header extensions are required to 
be 8-byte size multiple (which makes no sense unless they are also 
8-byte aligned).




Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  docs/interop/qcow2.txt | 5 +
  1 file changed, 5 insertions(+)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 4709f3bb30..b971e59b1a 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -185,6 +185,11 @@ which is covered by @header_length must be zeroed.
  
  < ... No additional fields in the header currently ... >
  
+Header padding

+If @header_length is larger than 104, software SHOULD make it a
+multiply of 8, adding zero-padding after additional fields. Still the
+padding is optional and may be absent in the image.
+
  Directly after the image header, optional sections called header extensions 
can
  be stored. Each extension has a structure like the following:
  



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

[PATCH] iotests: Skip read-only cases in 118 when run as root

[Kevin Wolf]

Some tests in 118 use chmod to remove write permissions from the file
and assume that the image can indeed not be opened read-write
afterwards. This doesn't work when the test is run as root, because root
can still open the file as writable even when the permission bit isn't
set.

Introduce a @skip_if_root decorator and use it in 118 to skip the tests
in question when the script is run as root.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/118|  3 +++
 tests/qemu-iotests/iotests.py | 10 ++
 2 files changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index ea0b326ae0..9eff46d189 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -446,6 +446,7 @@ class TestChangeReadOnly(ChangeBaseClass):
 self.assert_qmp(result, 'return[0]/inserted/ro', True)
 self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
+@iotests.skip_if_root
 def test_rw_ro_retain(self):
 os.chmod(new_img, 0o444)
 self.vm.add_drive(old_img, 'media=disk', 'none')
@@ -530,6 +531,7 @@ class TestChangeReadOnly(ChangeBaseClass):
 self.assert_qmp(result, 'return[0]/inserted/ro', True)
 self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
+@iotests.skip_if_root
 def test_make_ro_rw(self):
 os.chmod(new_img, 0o444)
 self.vm.add_drive(old_img, 'media=disk', 'none')
@@ -571,6 +573,7 @@ class TestChangeReadOnly(ChangeBaseClass):
 self.assert_qmp(result, 'return[0]/inserted/ro', True)
 self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
 
+@iotests.skip_if_root
 def test_make_ro_rw_by_retain(self):
 os.chmod(new_img, 0o444)
 self.vm.add_drive(old_img, 'media=disk', 'none')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3a8f378f90..9c66db613e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -920,6 +920,16 @@ def skip_if_unsupported(required_formats=[], 
read_only=False):
 return func_wrapper
 return skip_test_decorator
 
+def skip_if_root(func):
+'''Skip Test Decorator
+   Runs the test only without root permissions'''
+def func_wrapper(*args, **kwargs):
+if os.getuid() == 0:
+case_notrun('{}: cannot be run as root'.format(args[0]))
+else:
+return func(*args, **kwargs)
+return func_wrapper
+
 def execute_unittest(output, verbosity, debug):
 runner = unittest.TextTestRunner(stream=output, descriptions=True,
  verbosity=verbosity)
-- 
2.20.1

[PATCH] blockdev: modify blockdev-change-medium to change non-removable device

[Denis Plotnikov]

The modification is useful to workaround exclusive file access restrictions,
e.g. to implement VM migration with shared disk stored on a storage with
the exclusive file opening model: a destination VM is started waiting for
incomming migration with a fake image drive, and later, on the last migration
phase, the fake image file is replaced with the real one.

Signed-off-by: Denis Plotnikov 
---
 blockdev.c   | 69 +++-
 hmp.c|  2 ++
 qapi/block-core.json |  7 +++--
 qmp.c|  3 +-
 4 files changed, 57 insertions(+), 24 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index d358169995..23f3465cfc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2609,6 +2609,8 @@ void qmp_blockdev_change_medium(bool has_device, const 
char *device,
 bool has_format, const char *format,
 bool has_read_only,
 BlockdevChangeReadOnlyMode read_only,
+bool has_medium_name,
+const char *medium_name,
 Error **errp)
 {
 BlockBackend *blk;
@@ -2667,29 +2669,56 @@ void qmp_blockdev_change_medium(bool has_device, const 
char *device,
 goto fail;
 }
 
-rc = do_open_tray(has_device ? device : NULL,
-  has_id ? id : NULL,
-  false, );
-if (rc && rc != -ENOSYS) {
-error_propagate(errp, err);
-goto fail;
-}
-error_free(err);
-err = NULL;
+if (blk_dev_has_removable_media(blk)) {
+rc = do_open_tray(has_device ? device : NULL,
+  has_id ? id : NULL,
+  false, );
+if (rc && rc != -ENOSYS) {
+error_propagate(errp, err);
+goto fail;
+}
+error_free(err);
+err = NULL;
 
-blockdev_remove_medium(has_device, device, has_id, id, );
-if (err) {
-error_propagate(errp, err);
-goto fail;
-}
+blockdev_remove_medium(has_device, device, has_id, id, );
+if (err) {
+error_propagate(errp, err);
+goto fail;
+}
 
-qmp_blockdev_insert_anon_medium(blk, medium_bs, );
-if (err) {
-error_propagate(errp, err);
-goto fail;
-}
+qmp_blockdev_insert_anon_medium(blk, medium_bs, );
+if (err) {
+error_propagate(errp, err);
+goto fail;
+}
+
+qmp_blockdev_close_tray(has_device, device, has_id, id, errp);
+} else {
+if (!medium_name) {
+error_setg(errp, "A medium name should be given");
+goto fail;
+}
 
-qmp_blockdev_close_tray(has_device, device, has_id, id, errp);
+if (runstate_is_running()) {
+error_setg(errp, "Can't set a medium for non-removable device "
+"in a running VM");
+goto fail;
+}
+
+if (strlen(blk_name(blk))) {
+error_setg(errp, "The device already has a medium");
+goto fail;
+}
+
+if (blk_insert_bs(blk, medium_bs, ) < 0) {
+error_propagate(errp, err);
+goto fail;
+}
+
+if (!monitor_add_blk(blk, medium_name, )) {
+error_propagate(errp, err);
+}
+}
 
 fail:
 /* If the medium has been inserted, the device has its own reference, so
diff --git a/hmp.c b/hmp.c
index 8eec768088..fc7bac5b4b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1948,6 +1948,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
 const char *target = qdict_get_str(qdict, "target");
 const char *arg = qdict_get_try_str(qdict, "arg");
 const char *read_only = qdict_get_try_str(qdict, "read-only-mode");
+const char *target_name = qdict_get_try_str(qdict, "target-name");
 BlockdevChangeReadOnlyMode read_only_mode = 0;
 Error *err = NULL;
 
@@ -1982,6 +1983,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
 
 qmp_blockdev_change_medium(true, device, false, NULL, target,
!!arg, arg, !!read_only, read_only_mode,
+   !!target_name, target_name,
);
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..f493a7c737 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4769,6 +4769,8 @@
 # @read-only-mode:  change the read-only mode of the device; defaults
 #   to 'retain'
 #
+# @medium-name: drive-name when changing the media in non-removable devices
+#   ignored when changing media in removable devices
 # Since: 2.5
 #
 # Examples:
@@ -4807,9 +4809,8 @@
 '*id': 'str',
 'filename': 'str',
 '*format': 'str',
-'*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
-
-
+'*read-only-mode': 

[PATCH v8 2/3] docs: define padding for qcow2 header

[Vladimir Sementsov-Ogievskiy]

Header extensions ends are already defined to be multiply of 8. Let's
gently ask for header length to be a multiply of 8 too, when we have
some additional fields. Requiring this may be considered as an
incompatible change, so the padding is optional. Actually, padding is
allowed before this patch (due to definition of additional fields),
the only actual change is "SHOULD" word.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 docs/interop/qcow2.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 4709f3bb30..b971e59b1a 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -185,6 +185,11 @@ which is covered by @header_length must be zeroed.
 
 < ... No additional fields in the header currently ... >
 
+Header padding
+If @header_length is larger than 104, software SHOULD make it a
+multiply of 8, adding zero-padding after additional fields. Still the
+padding is optional and may be absent in the image.
+
 Directly after the image header, optional sections called header extensions can
 be stored. Each extension has a structure like the following:
 
-- 
2.21.0

Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Eric Blake]

On 10/18/19 4:27 AM, Vladimir Sementsov-Ogievskiy wrote:


I would suggest a stronger requirement:

header_length must be a multiple of 4, and must not land in the middle of any 
optional 8-byte field.

Or maybe even add our compression type extension with 4 bytes of padding, so 
that we could go even stronger:

header_length must be a multiple of 8.


Hmm, if we imply that software will have to add some padding, than requirement 
above about zero === feature-absence
becomes necessary. [*]

Still I have two questions:
1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for 
compression?


No, fields can be smaller if that makes sense; but I still think it's 
important that fields don't straddle natural alignment boundaries. When 
adding just one field at a time, it's easier to add a wide field and 
less padding than a narrow field and lots of padding, if we're still 
striving for alignment.



2. What is the benefit of padding, which you propose?


The biggest benefit to keeping large fields from straddling alignment 
boundaries is that you can mmap() a qcow2 file and do naturally-aligned 
reads of those large fields, rather than byte-by-byte reads, without 
risking SIGBUS on some architectures.  (You still have to worry about 
endianness, but that's true regardless of alignment)




So, it looks inconsistent, if we pad all header extensions to  8 bytes except 
for the start of the first extension.

I'll resend with padding soon.



Still, we have to make an exception at least for header_length = 104, which is 
not a multiply of 8.


Huh?  104 == 8 * 13, so our current v3 header size is 8-byte aligned. 
Likewise for 72 == 8 * 9 for v2 header size.




Also, is requiring alignment is an incompatible change of specification?


No. I think it is just clarifying what was implicitly already  the case. 
 Remember, immediately after the header comes header extensions, and 
THOSE are explicitly required to be multiple-of-8 in size.  That 
requirement makes no sense unless the header itself ends on an 8-byte 
alignment (which it does for all existing v2 and v3 images prior to our 
first official v3 header extension).


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] block/backup: drop dead code from backup_job_create

[Kevin Wolf]

Am 17.10.2019 um 16:21 hat Vladimir Sementsov-Ogievskiy geschrieben:
> After commit 00e30f05de1d195, there is no more "goto error" points
> after job creation, so after "error:" @job is always NULL and we don't
> need roll-back job creation.
> 
> Reported-by: Coverity (CID 1406402)
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Thanks, applied to the block branch.

Kevin

Re: [PATCH v2] doc: Describe missing generic -blockdev options

[Peter Maydell]

On Fri, 18 Oct 2019 at 13:02, Kevin Wolf  wrote:
>
> We added more generic options after introducing -blockdev and forgot to
> update the documentation (man page and --help output) accordingly. Do
> that now.
>
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-options.hx | 22 +-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 793d70ff93..2e6ba5ef1f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -849,7 +849,8 @@ ETEXI
>  DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,
>  "-blockdev [driver=]driver[,node-name=N][,discard=ignore|unmap]\n"
>  "  [,cache.direct=on|off][,cache.no-flush=on|off]\n"
> -"  [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
> +"  [,read-only=on|off][,auto-read-only=on|off]\n"
> +"  [,force-share=on|off][,detect-zeroes=on|off|unmap]\n"
>  "  [,driver specific parameters...]\n"
>  "configure a block backend\n", QEMU_ARCH_ALL)
>  STEXI
> @@ -885,6 +886,25 @@ name is not intended to be predictable and changes 
> between QEMU invocations.
>  For the top level, an explicit node name must be specified.
>  @item read-only
>  Open the node read-only. Guest write attempts will fail.
> +
> +Note that some block drivers support only read-only access, either generally 
> or
> +in certain configurations. In this case, the default value
> +@option{read-only=off} does not work and the option must be specified
> +explicitly.
> +@item auto-read-only
> +If @option{auto-read-only=on} is set, QEMU may fall back to read-only usage
> +even when @option{read-only=off} is requested, or even switch between modes 
> as
> +needed, e.g. depending on whether the image file is writable or whether a
> +writing user is attached to the node.
> +@item force-share
> +Override the image locking system of QEMU by forcing the node to utilize
> +weaker shared access for permissions where it would normally request 
> exclusive
> +access.  When there is the potential for multiple instances to have the same
> +file open (whether this invocation of qemu is the first or the second

"QEMU" should be all-upper-case.

> +instance), both instances must permit shared access for the second instance 
> to
> +succeed at opening the file.
> +
> +Enabling @option{force-share=on} requires @option{read-only=on}.
>  @item cache.direct
>  The host page cache can be avoided with @option{cache.direct=on}. This will
>  attempt to do disk IO directly to the guest's memory. QEMU may still perform 
> an
> --
> 2.20.1

Otherwise
Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PULL v3 00/19] Bitmaps patches

[Peter Maydell]

On Thu, 17 Oct 2019 at 22:54, John Snow  wrote:
>
> The following changes since commit f22f553efffd083ff624be116726f843a39f1148:
>
>   Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20191013' into 
> staging (2019-10-17 16:48:56 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/bitmaps-pull-request
>
> for you to fetch changes up to 3264ffced3d087bbe72d759639ef84fd5bd924cc:
>
>   dirty-bitmaps: remove deprecated autoload parameter (2019-10-17 17:53:28 
> -0400)
>
> 
> pull request
>
> 
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2
for any user-visible changes.

-- PMM

Re: [PATCH] qemu-img.texi: Describe data_file and data_file_raw

[Eric Blake]

[cc qemu-block]

On 10/18/19 4:59 AM, Han Han wrote:

https://bugzilla.redhat.com/show_bug.cgi?id=1763105

Signed-off-by: Han Han 
---
  qemu-img.texi | 10 ++
  1 file changed, 10 insertions(+)



Reviewed-by: Eric Blake 


diff --git a/qemu-img.texi b/qemu-img.texi
index b5156d6316..44596c2d93 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -763,6 +763,16 @@ file which is COW and has data blocks already, it couldn't 
be changed to NOCOW
  by setting @code{nocow=on}. One can issue @code{lsattr filename} to check if
  the NOCOW flag is set or not (Capital 'C' is NOCOW flag).
  
+@item data_file

+File name of data file that is stored in the image and used as a default for
+opening the image. If the option is used, qcow2 file only stores the metadata
+of the image.
+
+@item data_file_raw
+This option requires @option{data_file}. If this option is set to @code{on},
+qemu will always keep the external data file consistent as a standalone
+read-only raw image. Default value is @code{off}.
+
  @end table
  
  @item Other




--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] iotests: Skip read-only cases in 118 when run as root

[Kevin Wolf]

Am 18.10.2019 um 14:59 hat Philippe Mathieu-Daudé geschrieben:
> Hi Kevin,
> 
> On 10/18/19 1:51 PM, Kevin Wolf wrote:
> > Some tests in 118 use chmod to remove write permissions from the file
> > and assume that the image can indeed not be opened read-write
> > afterwards. This doesn't work when the test is run as root, because root
> > can still open the file as writable even when the permission bit isn't
> > set.
> > 
> > Introduce a @skip_if_root decorator and use it in 118 to skip the tests
> > in question when the script is run as root.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >   tests/qemu-iotests/118|  3 +++
> >   tests/qemu-iotests/iotests.py | 10 ++
> >   2 files changed, 13 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
> > index ea0b326ae0..9eff46d189 100755
> > --- a/tests/qemu-iotests/118
> > +++ b/tests/qemu-iotests/118
> > @@ -446,6 +446,7 @@ class TestChangeReadOnly(ChangeBaseClass):
> >   self.assert_qmp(result, 'return[0]/inserted/ro', True)
> >   self.assert_qmp(result, 'return[0]/inserted/image/filename', 
> > new_img)
> > +@iotests.skip_if_root
> 
> Why not have case_notrun() return 'reason' and use:
> 
> @unittest.skipIf(os.getuid() == 0, case_notrun("cannot be run as root"))

Because we can't skip test cases using unittest functionality, it
results in different output (the test is marked as 's' instead of '.'
and a message '(skipped=n)' is added), which means failure for
qemu-iotests.

Apart from that, it would duplicate the logic and the error message in
every place, which wouldn't be very nice anyway. With the necessary
iotests.case_notrun() the line becomes > 80 characters, too.

> >   def test_rw_ro_retain(self):
> >   os.chmod(new_img, 0o444)
> >   self.vm.add_drive(old_img, 'media=disk', 'none')
> > @@ -530,6 +531,7 @@ class TestChangeReadOnly(ChangeBaseClass):
> >   self.assert_qmp(result, 'return[0]/inserted/ro', True)
> >   self.assert_qmp(result, 'return[0]/inserted/image/filename', 
> > new_img)
> > +@iotests.skip_if_root
> >   def test_make_ro_rw(self):
> >   os.chmod(new_img, 0o444)
> >   self.vm.add_drive(old_img, 'media=disk', 'none')
> > @@ -571,6 +573,7 @@ class TestChangeReadOnly(ChangeBaseClass):
> >   self.assert_qmp(result, 'return[0]/inserted/ro', True)
> >   self.assert_qmp(result, 'return[0]/inserted/image/filename', 
> > new_img)
> > +@iotests.skip_if_root
> >   def test_make_ro_rw_by_retain(self):
> >   os.chmod(new_img, 0o444)
> >   self.vm.add_drive(old_img, 'media=disk', 'none')
> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > index 3a8f378f90..9c66db613e 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -920,6 +920,16 @@ def skip_if_unsupported(required_formats=[], 
> > read_only=False):
> >   return func_wrapper
> >   return skip_test_decorator
> > +def skip_if_root(func):
> 
> skip_if_user_is_root() is slightly less confuse.

Ok, I can make this change.

Kevin

Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Eric Blake]

On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:

Make it more obvious how to add new fields to the version 3 header and
how to interpret them.

The specification is adjusted so for new defined optional fields:


The specification is adjusted to make it clear that future fields are 
optional:




1. Software may support some of these optional fields and ignore the
others, which means that features may be backported to downstream
Qemu independently.
3. If @header_length is higher than the highest field end that software
knows, it should assume that topmost unknown additional fields are
correct, and keep additional unknown fields as is on rewriting the
image.
3. If we want to add incompatible field (or a field, for which some its
values would be incompatible), it must be accompanied by
incompatible feature bit.

Also the concept of "default is zero" is clarified, as it's strange to
say that the value of the field is assumed to be zero for the software
version which don't know about the field at all and don't know how to
treat it be it zero or not.



I'd also mention that we want to enforce 8-byte alignment in this cover 
letter.



Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  docs/interop/qcow2.txt | 26 +++---
  1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index af5711e533..4709f3bb30 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file header:
  Offset into the image file at which the snapshot table
  starts. Must be aligned to a cluster boundary.
  
-If the version is 3 or higher, the header has the following additional fields.

-For version 2, the values are assumed to be zero, unless specified otherwise
-in the description of a field.
+For version 2, header is always 72 bytes length and finishes here.
+For version 3 or higher the header length is at least 104 bytes and has at
+least next five fields, up to the @header_length field.


For version 2, the header is exactly 72 bytes in length, and finishes here.
For version 3 or higher, the header length is at least 104 bytes, 
including the next fields through header_length.


  
   72 -  79:  incompatible_features

  Bitmask of incompatible features. An implementation must
@@ -164,6 +164,26 @@ in the description of a field.
  100 - 103:  header_length
  Length of the header structure in bytes. For version 2
  images, the length is always assumed to be 72 bytes.
+For version 3 it's at least 104 bytes.


I'd also add a sentence that this field must be a multiple of 8.


+
+Additional fields (version 3 and higher)
+
+The following fields of the header are optional: if software doesn't know how
+to interpret the field, it may be safely ignored, other than preserving the
+field unchanged when rewriting the image header.


Maybe:

if software doesn't know how to interpret the field, it may be safely 
ignored unless a corresponding incompatible feature flag bit is set; 
however, the field should be preserved unchanged when rewriting the 
image header.



+
+For all additional fields zero value equals to absence of field (absence is
+when field.offset + field.size > @header_length). This implies
+that if software want's to set fields up to some field not aligned to multiply
+of 8 it must align header up by zeroes. And on the other hand, if software
+need some optional field which is absent it should assume that it's value is
+zero.


Maybe:

Each optional field that does not have a corresponding incompatible 
feature bit must support the value 0 that gives the same default 
behavior as when the optional field is omitted.



+
+It's allowed for the header end to cut some field in the middle (in this case
+the field is considered as absent), but in this case the part of the field
+which is covered by @header_length must be zeroed.
+
+< ... No additional fields in the header currently ... >


Do we even still need this if we require 8-byte alignment?  We'd never 
be able to cut a single field in the middle, but I suppose you are 
worried about cutting a 2-field 16-byte addition tied to a single 
feature in the middle.  But that's not going to happen in practice.  The 
only time the header will be longer than 104 bytes is if at least one 
documented optional feature has been implemented/backported, and that 
feature will be implemented in its entirety.  If you backport a later 
feature but not the earlier, you're still going to set header_length to 
the boundary of the feature that you ARE backporting.  Thus, I argue 
that blindly setting header_length to 120 prior to the standard ever 
defining optional field(s) at 112-120 is premature, and that if we ever 
add a feature requiring bytes 112-128 for a new feature, 

[PATCH v8 3/3] docs: qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

The patch add new additional field to qcow2 header: compression_type,
which specifies compression type. If field is absent or zero, default
compression type is set: ZLIB, which corresponds to current behavior.

New compression type (ZSTD) is to be added in further commit.

Suggested-by: Denis Plotnikov 
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 docs/interop/qcow2.txt | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index b971e59b1a..4eabd81363 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -109,6 +109,12 @@ least next five fields, up to the @header_length field.
 An External Data File Name header extension may
 be present if this bit is set.
 
+Bit 3:  Compression type bit.  If this bit is set,
+non-default compression is used for compressed
+clusters. In this case, @header_length must
+be at least 105 and @compression_type field
+must be non-zero.
+
 Bits 3-63:  Reserved (set to 0)
 
  80 -  87:  compatible_features
@@ -183,7 +189,18 @@ It's allowed for the header end to cut some field in the 
middle (in this case
 the field is considered as absent), but in this case the part of the field
 which is covered by @header_length must be zeroed.
 
-< ... No additional fields in the header currently ... >
+  104:  compression_type
+Defines the compression method used for compressed 
clusters.
+A single compression type is applied to all compressed 
image
+clusters.
+If incompatible compression type bit is set: the field must
+exist (i.e. @header_length >= 105) and must be non-zero (
+which means non-zlib compression type)
+If incompatible compression type bit is unset: the field
+may not exist (if @header_length < 105) or it must be zero
+(which means zlib).
+Available compression type values:
+0: zlib 
 
 Header padding
 If @header_length is larger than 104, software SHOULD make it a
-- 
2.21.0

[PATCH v8 0/3] qcow2: add zstd cluster compression

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is my proposal, about how to correctly update qcow2 specification
to introduce new field, keeping in mind currently existing images and
downstream Qemu instances.

v8: Add padding, and clarify "zero equals absence" concept.
Move some points to commit message from the spec itself.
Fix s/108/105 in 02

Vladimij Sementsov-Ogievskiy (3):
  docs: improve qcow2 spec about extending image header
  docs: define padding for qcow2 header
  docs: qcow2: introduce compression type feature

 docs/interop/qcow2.txt | 48 +++---
 1 file changed, 45 insertions(+), 3 deletions(-)

-- 
2.21.0

[PATCH v2] doc: Describe missing generic -blockdev options

[Kevin Wolf]

We added more generic options after introducing -blockdev and forgot to
update the documentation (man page and --help output) accordingly. Do
that now.

Signed-off-by: Kevin Wolf 
---
 qemu-options.hx | 22 +-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 793d70ff93..2e6ba5ef1f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -849,7 +849,8 @@ ETEXI
 DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,
 "-blockdev [driver=]driver[,node-name=N][,discard=ignore|unmap]\n"
 "  [,cache.direct=on|off][,cache.no-flush=on|off]\n"
-"  [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
+"  [,read-only=on|off][,auto-read-only=on|off]\n"
+"  [,force-share=on|off][,detect-zeroes=on|off|unmap]\n"
 "  [,driver specific parameters...]\n"
 "configure a block backend\n", QEMU_ARCH_ALL)
 STEXI
@@ -885,6 +886,25 @@ name is not intended to be predictable and changes between 
QEMU invocations.
 For the top level, an explicit node name must be specified.
 @item read-only
 Open the node read-only. Guest write attempts will fail.
+
+Note that some block drivers support only read-only access, either generally or
+in certain configurations. In this case, the default value
+@option{read-only=off} does not work and the option must be specified
+explicitly.
+@item auto-read-only
+If @option{auto-read-only=on} is set, QEMU may fall back to read-only usage
+even when @option{read-only=off} is requested, or even switch between modes as
+needed, e.g. depending on whether the image file is writable or whether a
+writing user is attached to the node.
+@item force-share
+Override the image locking system of QEMU by forcing the node to utilize
+weaker shared access for permissions where it would normally request exclusive
+access.  When there is the potential for multiple instances to have the same
+file open (whether this invocation of qemu is the first or the second
+instance), both instances must permit shared access for the second instance to
+succeed at opening the file.
+
+Enabling @option{force-share=on} requires @option{read-only=on}.
 @item cache.direct
 The host page cache can be avoided with @option{cache.direct=on}. This will
 attempt to do disk IO directly to the guest's memory. QEMU may still perform an
-- 
2.20.1

Re: [PATCH v7 1/2] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

18.10.2019 16:39, Eric Blake wrote:
> On 10/18/19 4:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> I would suggest a stronger requirement:
>
> header_length must be a multiple of 4, and must not land in the middle of 
> any optional 8-byte field.
>
> Or maybe even add our compression type extension with 4 bytes of padding, 
> so that we could go even stronger:
>
> header_length must be a multiple of 8.

 Hmm, if we imply that software will have to add some padding, than 
 requirement above about zero === feature-absence
 becomes necessary. [*]

 Still I have two questions:
 1. Do we really need all fields to be 4 or 8 bytes? Why not use 1 byte for 
 compression?
> 
> No, fields can be smaller if that makes sense; but I still think it's 
> important that fields don't straddle natural alignment boundaries. When 
> adding just one field at a time, it's easier to add a wide field and less 
> padding than a narrow field and lots of padding, if we're still striving for 
> alignment.
> 
 2. What is the benefit of padding, which you propose?
> 
> The biggest benefit to keeping large fields from straddling alignment 
> boundaries is that you can mmap() a qcow2 file and do naturally-aligned reads 
> of those large fields, rather than byte-by-byte reads, without risking SIGBUS 
> on some architectures.  (You still have to worry about endianness, but that's 
> true regardless of alignment)
> 
> 
>>> So, it looks inconsistent, if we pad all header extensions to  8 bytes 
>>> except for the start of the first extension.
>>>
>>> I'll resend with padding soon.
>>
>>
>> Still, we have to make an exception at least for header_length = 104, which 
>> is not a multiply of 8.
> 
> Huh?  104 == 8 * 13, so our current v3 header size is 8-byte aligned. 
> Likewise for 72 == 8 * 9 for v2 header size.

Ahahahaha, shame on my head:) I should go to school again.

> 
>>
>> Also, is requiring alignment is an incompatible change of specification?
> 
> No. I think it is just clarifying what was implicitly already  the case.  
> Remember, immediately after the header comes header extensions, and THOSE are 
> explicitly required to be multiple-of-8 in size.  That requirement makes no 
> sense unless the header itself ends on an 8-byte alignment (which it does for 
> all existing v2 and v3 images prior to our first official v3 header 
> extension).
> 

OK, let's go with it, I'll resend again.


-- 
Best regards,
Vladimir

Re: [PATCH] iotests: Skip read-only cases in 118 when run as root

[Max Reitz]

On 18.10.19 16:27, Kevin Wolf wrote:
> Am 18.10.2019 um 14:59 hat Philippe Mathieu-Daudé geschrieben:
>> Hi Kevin,
>>
>> On 10/18/19 1:51 PM, Kevin Wolf wrote:
>>> Some tests in 118 use chmod to remove write permissions from the file
>>> and assume that the image can indeed not be opened read-write
>>> afterwards. This doesn't work when the test is run as root, because root
>>> can still open the file as writable even when the permission bit isn't
>>> set.
>>>
>>> Introduce a @skip_if_root decorator and use it in 118 to skip the tests
>>> in question when the script is run as root.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>   tests/qemu-iotests/118|  3 +++
>>>   tests/qemu-iotests/iotests.py | 10 ++
>>>   2 files changed, 13 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
>>> index ea0b326ae0..9eff46d189 100755
>>> --- a/tests/qemu-iotests/118
>>> +++ b/tests/qemu-iotests/118
>>> @@ -446,6 +446,7 @@ class TestChangeReadOnly(ChangeBaseClass):
>>>   self.assert_qmp(result, 'return[0]/inserted/ro', True)
>>>   self.assert_qmp(result, 'return[0]/inserted/image/filename', 
>>> new_img)
>>> +@iotests.skip_if_root
>>
>> Why not have case_notrun() return 'reason' and use:
>>
>> @unittest.skipIf(os.getuid() == 0, case_notrun("cannot be run as root"))
> 
> Because we can't skip test cases using unittest functionality, it
> results in different output (the test is marked as 's' instead of '.'
> and a message '(skipped=n)' is added), which means failure for
> qemu-iotests.

Not arguing that we should use unittest skipping here, but my “Selfish
patches” series allows it:

https://lists.nongnu.org/archive/html/qemu-devel/2019-09/msg03423.html

The advantage is that using unittest skipping works in setUp, too.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 08/10] nbd/server: introduce NBDExtentArray

[Vladimir Sementsov-Ogievskiy]

09.10.2019 20:02, Eric Blake wrote:
> On 9/30/19 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Introduce NBDExtentArray class, to handle extents list creation in more
>> controlled way and with less OUT parameters in functions.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   nbd/server.c | 184 +--
>>   1 file changed, 90 insertions(+), 94 deletions(-)
>>
> 
>> +static void nbd_extent_array_free(NBDExtentArray *ea)
>> +{
>> +    g_free(ea->extents);
>> +    g_free(ea);
>> +}
>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
> 
> Nice to see this getting more popular :)
> 
>> +
>> +static int nbd_extent_array_add(NBDExtentArray *ea,
>> +    uint32_t length, uint32_t flags)
>>   {
> 
>> -    assert(*nb_extents);
>> -    while (remaining_bytes) {
>> +    if (ea->count >= ea->nb_alloc) {
>> +    return -1;
>> +    }
> 
> Returning -1 is not a failure in the protocol, just failure to add any more 
> information to the reply.  A function comment might help, but this looks like 
> a good helper function.

Something like
/*
  * Add extent to NBDExtentArray. If extent can't be added (no available space),
  * return -1.
  */
above the function?

> 
>> +static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
>> +  uint64_t bytes, NBDExtentArray *ea)
>> +{
>> +    while (bytes) {
>>   uint32_t flags;
>>   int64_t num;
>> -    int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
>> -  , NULL, NULL);
>> +    int ret = bdrv_block_status_above(bs, NULL, offset, bytes, ,
>> +  NULL, NULL);
> 
>> +    if (nbd_extent_array_add(ea, num, flags) < 0) {
>> +    return 0;
>>   }
>> -    offset += num;
>> -    remaining_bytes -= num;
>> -    }
>> -    extents_end = extent + 1;
>> -
>> -    for (extent = extents; extent < extents_end; extent++) {
>> -    extent->flags = cpu_to_be32(extent->flags);
>> -    extent->length = cpu_to_be32(extent->length);
>> +    offset += num;
>> +    bytes -= num;
>>   }
>> -    *bytes -= remaining_bytes;
>> -    *nb_extents = extents_end - extents;
>> -
>>   return 0;
> 
> Also looks good (return 0 if we populated until we either ran out of reply 
> space or out of bytes to report on).
> 
>>   static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
>> -   NBDExtent *extents, unsigned int nb_extents,
>> -   uint64_t length, bool last,
>> -   uint32_t context_id, Error **errp)
>> +   NBDExtentArray *ea,
>> +   bool last, uint32_t context_id, Error **errp)
>>   {
>>   NBDStructuredMeta chunk;
>> -
>> +    size_t len = ea->count * sizeof(ea->extents[0]);
>> +    g_autofree NBDExtent *extents = g_memdup(ea->extents, len);
> 
> Why do we need memdup here?  What's wrong with modifying ea->extents in 
> place?...

To not make ea to be IN-OUT parameter.. I don't like functions with side 
effects.
It will break the code if at some point we call nbd_co_send_extents twice on 
same
ea object.

What is the true way? To not memdup, nbd_co_send_extents should consume the 
whole
ea object..

Seems, g_autoptr attribute can't be used for function parameter, gcc complains:
nbd/server.c:1983:32: error: ‘cleanup’ attribute ignored [-Werror=attributes]
  1983 |g_autoptr(NBDExtentArray) ea,
   |^

so, is it better
to call nbd_co_send_external(... g_steal_pointer() ...)

and than in nbd_co_send_external do

g_autoptr(NBDExtentArray) local_ea = ea;
NBDExtent *extents = local_ea->extents;

?


> 
>> +    NBDExtent *extent, *extents_end = extents + ea->count;
>>   struct iovec iov[] = {
>>   {.iov_base = , .iov_len = sizeof(chunk)},
>> -    {.iov_base = extents, .iov_len = nb_extents * sizeof(extents[0])}
>> +    {.iov_base = extents, .iov_len = len}
>>   };
>> -    trace_nbd_co_send_extents(handle, nb_extents, context_id, length, last);
>> +    for (extent = extents; extent < extents_end; extent++) {
>> +    extent->flags = cpu_to_be32(extent->flags);
>> +    extent->length = cpu_to_be32(extent->length);
>> +    }
>> +
>> +    trace_nbd_co_send_extents(handle, ea->count, context_id, 
>> ea->total_length,
>> +  last);
>>   set_be_chunk(, last ? NBD_REPLY_FLAG_DONE : 0,
>>    NBD_REPLY_TYPE_BLOCK_STATUS,
>>    handle, sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
>> @@ -1994,39 +2012,27 @@ static int nbd_co_send_block_status(NBDClient 
>> *client, uint64_t handle,
>>   {
>>   int ret;
>>   unsigned int nb_extents = dont_fragment ? 1 : 
>> 

Re: [PATCH 08/10] nbd/server: introduce NBDExtentArray

[Eric Blake]

On 10/18/19 11:07 AM, Vladimir Sementsov-Ogievskiy wrote:


   static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
-   NBDExtent *extents, unsigned int nb_extents,
-   uint64_t length, bool last,
-   uint32_t context_id, Error **errp)
+   NBDExtentArray *ea,
+   bool last, uint32_t context_id, Error **errp)
   {
   NBDStructuredMeta chunk;
-
+    size_t len = ea->count * sizeof(ea->extents[0]);
+    g_autofree NBDExtent *extents = g_memdup(ea->extents, len);


Why do we need memdup here?  What's wrong with modifying ea->extents in 
place?...


To not make ea to be IN-OUT parameter.. I don't like functions with side 
effects.
It will break the code if at some point we call nbd_co_send_extents twice on 
same
ea object.

What is the true way? To not memdup, nbd_co_send_extents should consume the 
whole
ea object..

Seems, g_autoptr attribute can't be used for function parameter, gcc complains:
nbd/server.c:1983:32: error: ‘cleanup’ attribute ignored [-Werror=attributes]
   1983 |g_autoptr(NBDExtentArray) ea,
|^

so, is it better
to call nbd_co_send_external(... g_steal_pointer() ...)

and than in nbd_co_send_external do

g_autoptr(NBDExtentArray) local_ea = ea;
NBDExtent *extents = local_ea->extents;

?



No, that makes it worse.  It's that much more confusing to track who is 
allocating what and where it gets cleaned up.


I personally don't see the need to avoid jumping through hoops to avoid 
an in-out parameter (if we're going to rework code later, we'll notice 
that we documented how things are supposed to be used), but if in-out 
parameters bother you, then the approach you used, even with an extra 
memdup(), is the simplest way to maintain, even if it is not the most 
efficient.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] iotests: Skip read-only cases in 118 when run as root

[Kevin Wolf]

Am 18.10.2019 um 17:00 hat Max Reitz geschrieben:
> On 18.10.19 16:27, Kevin Wolf wrote:
> > Am 18.10.2019 um 14:59 hat Philippe Mathieu-Daudé geschrieben:
> >> Hi Kevin,
> >>
> >> On 10/18/19 1:51 PM, Kevin Wolf wrote:
> >>> Some tests in 118 use chmod to remove write permissions from the file
> >>> and assume that the image can indeed not be opened read-write
> >>> afterwards. This doesn't work when the test is run as root, because root
> >>> can still open the file as writable even when the permission bit isn't
> >>> set.
> >>>
> >>> Introduce a @skip_if_root decorator and use it in 118 to skip the tests
> >>> in question when the script is run as root.
> >>>
> >>> Signed-off-by: Kevin Wolf 
> >>> ---
> >>>   tests/qemu-iotests/118|  3 +++
> >>>   tests/qemu-iotests/iotests.py | 10 ++
> >>>   2 files changed, 13 insertions(+)
> >>>
> >>> diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
> >>> index ea0b326ae0..9eff46d189 100755
> >>> --- a/tests/qemu-iotests/118
> >>> +++ b/tests/qemu-iotests/118
> >>> @@ -446,6 +446,7 @@ class TestChangeReadOnly(ChangeBaseClass):
> >>>   self.assert_qmp(result, 'return[0]/inserted/ro', True)
> >>>   self.assert_qmp(result, 'return[0]/inserted/image/filename', 
> >>> new_img)
> >>> +@iotests.skip_if_root
> >>
> >> Why not have case_notrun() return 'reason' and use:
> >>
> >> @unittest.skipIf(os.getuid() == 0, case_notrun("cannot be run as root"))
> > 
> > Because we can't skip test cases using unittest functionality, it
> > results in different output (the test is marked as 's' instead of '.'
> > and a message '(skipped=n)' is added), which means failure for
> > qemu-iotests.
> 
> Not arguing that we should use unittest skipping here, but my “Selfish
> patches” series allows it:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2019-09/msg03423.html
> 
> The advantage is that using unittest skipping works in setUp, too.

Ah, good to know. If this had already been in master, I might have
chosen a simple function call iotests.skip_if_root() inside the test
function instead of using a decorator. But in the end, I don't think it
makes a big difference in this case.

Kevin


signature.asc
Description: PGP signature

Re: [PATCH] iotests: Remove 130 from the "auto" group

[Bruce Rogers]

On Fri, 2019-10-18 at 18:10 +0200, Thomas Huth wrote:
> Peter hit a "Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared
> 'write' lock - Is another process using the image
> [TEST_DIR/t.IMGFMT]?"
> error with 130 already twice. Looks like this test is a little bit
> shaky, and currently nobody has a real clue what could be causing
> this
> issue, so for the time being, let's disable it from the "auto" group
> so
> that it does not gate the pull requests.
> 

For some time I've also needed to work around issues running 130. I
either disabled it, or I found a few properly placed sleeps got it to
reliably pass. Last week I finally got around to investigating it a bit
more and discovered that the failure was related to my using --enable-
membarrier in my configure.

I didn't investigate whether the block io tests' _cleanup_qemu using
kill -KILL was being relied on in some way by some tests, or if that is
simply a way to speed the testing along, or what, but I've gotten test
130 to reliably pass by changing the test to quit properly via the
monitor, and by adding a wait=1 so that _cleanup_qemu doesn't simply
kill qemu.

I believe 153 and 161 also suffer in a similar way.

I haven't gotten around to fully understanding how qemu's using the
kernel sys_membarrier is adversly affected by killing qemu in this way,
but it seems there's an issue with that.

Hopefully someone who is more familiar with qemu's use of membarrier's
can add more details here.

Bruce

Re: [PATCH v8 1/3] docs: improve qcow2 spec about extending image header

[Vladimir Sementsov-Ogievskiy]

18.10.2019 17:00, Eric Blake wrote:
> On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Make it more obvious how to add new fields to the version 3 header and
>> how to interpret them.
>>
>> The specification is adjusted so for new defined optional fields:
> 
> The specification is adjusted to make it clear that future fields are 
> optional:
> 
>>
>> 1. Software may support some of these optional fields and ignore the
>>     others, which means that features may be backported to downstream
>>     Qemu independently.
>> 3. If @header_length is higher than the highest field end that software
>>     knows, it should assume that topmost unknown additional fields are
>>     correct, and keep additional unknown fields as is on rewriting the
>>     image.
>> 3. If we want to add incompatible field (or a field, for which some its
>>     values would be incompatible), it must be accompanied by
>>     incompatible feature bit.
>>
>> Also the concept of "default is zero" is clarified, as it's strange to
>> say that the value of the field is assumed to be zero for the software
>> version which don't know about the field at all and don't know how to
>> treat it be it zero or not.
>>
> 
> I'd also mention that we want to enforce 8-byte alignment in this cover 
> letter.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   docs/interop/qcow2.txt | 26 +++---
>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index af5711e533..4709f3bb30 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -79,9 +79,9 @@ The first cluster of a qcow2 image contains the file 
>> header:
>>   Offset into the image file at which the snapshot table
>>   starts. Must be aligned to a cluster boundary.
>> -If the version is 3 or higher, the header has the following additional 
>> fields.
>> -For version 2, the values are assumed to be zero, unless specified otherwise
>> -in the description of a field.
>> +For version 2, header is always 72 bytes length and finishes here.
>> +For version 3 or higher the header length is at least 104 bytes and has at
>> +least next five fields, up to the @header_length field.
> 
> For version 2, the header is exactly 72 bytes in length, and finishes here.
> For version 3 or higher, the header length is at least 104 bytes, including 
> the next fields through header_length.
> 
>>    72 -  79:  incompatible_features
>>   Bitmask of incompatible features. An implementation 
>> must
>> @@ -164,6 +164,26 @@ in the description of a field.
>>   100 - 103:  header_length
>>   Length of the header structure in bytes. For version 2
>>   images, the length is always assumed to be 72 bytes.
>> +    For version 3 it's at least 104 bytes.
> 
> I'd also add a sentence that this field must be a multiple of 8.
> 
>> +
>> +Additional fields (version 3 and higher)
>> +
>> +The following fields of the header are optional: if software doesn't know 
>> how
>> +to interpret the field, it may be safely ignored, other than preserving the
>> +field unchanged when rewriting the image header.
> 
> Maybe:
> 
> if software doesn't know how to interpret the field, it may be safely ignored 
> unless a corresponding incompatible feature flag bit is set; however, the 
> field should be preserved unchanged when rewriting the image header.
> 
>> +
>> +For all additional fields zero value equals to absence of field (absence is
>> +when field.offset + field.size > @header_length). This implies
>> +that if software want's to set fields up to some field not aligned to 
>> multiply
>> +of 8 it must align header up by zeroes. And on the other hand, if software
>> +need some optional field which is absent it should assume that it's value is
>> +zero.
> 
> Maybe:
> 
> Each optional field that does not have a corresponding incompatible feature 
> bit must support the value 0 that gives the same default behavior as when the 
> optional field is omitted.

Hmmm. That doesn't work, as "corresponding" is something not actually defined. 
Consider our zstd extension.

It has corresponding incompatible bit, therefore, this sentence doesn't apply 
to it. But still, if incompatible bit is unset we can have this field. And it's 
zero value must correspond
to the absence of the field.

So, additional field may use incomaptible bit only for subset of its values.

But, I see, that you want to allow 0 value to not match field-absence if 
incompatible bit is set?

So, may be

Additional fields has the following compatible behavior by default:

1. If software doesn't know how to interpret the field, it may be safely 
ignored, other than preserving the field unchanged when rewriting the image 
header.
2. Zeroed additional field gives the same behavior as when this field is 
omitted.

This default behavior may be 

Re: [PATCH] iotests: Skip read-only cases in 118 when run as root

[Philippe Mathieu-Daudé]

On 10/18/19 4:27 PM, Kevin Wolf wrote:

Am 18.10.2019 um 14:59 hat Philippe Mathieu-Daudé geschrieben:

Hi Kevin,

On 10/18/19 1:51 PM, Kevin Wolf wrote:

Some tests in 118 use chmod to remove write permissions from the file
and assume that the image can indeed not be opened read-write
afterwards. This doesn't work when the test is run as root, because root
can still open the file as writable even when the permission bit isn't
set.

Introduce a @skip_if_root decorator and use it in 118 to skip the tests
in question when the script is run as root.

Signed-off-by: Kevin Wolf 
---
   tests/qemu-iotests/118|  3 +++
   tests/qemu-iotests/iotests.py | 10 ++
   2 files changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
index ea0b326ae0..9eff46d189 100755
--- a/tests/qemu-iotests/118
+++ b/tests/qemu-iotests/118
@@ -446,6 +446,7 @@ class TestChangeReadOnly(ChangeBaseClass):
   self.assert_qmp(result, 'return[0]/inserted/ro', True)
   self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+@iotests.skip_if_root


Why not have case_notrun() return 'reason' and use:

@unittest.skipIf(os.getuid() == 0, case_notrun("cannot be run as root"))


Because we can't skip test cases using unittest functionality, it
results in different output (the test is marked as 's' instead of '.'
and a message '(skipped=n)' is added), which means failure for
qemu-iotests.


Ah, I see.



Apart from that, it would duplicate the logic and the error message in
every place, which wouldn't be very nice anyway. With the necessary
iotests.case_notrun() the line becomes > 80 characters, too.


OK.




   def test_rw_ro_retain(self):
   os.chmod(new_img, 0o444)
   self.vm.add_drive(old_img, 'media=disk', 'none')
@@ -530,6 +531,7 @@ class TestChangeReadOnly(ChangeBaseClass):
   self.assert_qmp(result, 'return[0]/inserted/ro', True)
   self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+@iotests.skip_if_root
   def test_make_ro_rw(self):
   os.chmod(new_img, 0o444)
   self.vm.add_drive(old_img, 'media=disk', 'none')
@@ -571,6 +573,7 @@ class TestChangeReadOnly(ChangeBaseClass):
   self.assert_qmp(result, 'return[0]/inserted/ro', True)
   self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+@iotests.skip_if_root
   def test_make_ro_rw_by_retain(self):
   os.chmod(new_img, 0o444)
   self.vm.add_drive(old_img, 'media=disk', 'none')
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3a8f378f90..9c66db613e 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -920,6 +920,16 @@ def skip_if_unsupported(required_formats=[], 
read_only=False):
   return func_wrapper
   return skip_test_decorator
+def skip_if_root(func):


skip_if_user_is_root() is slightly less confuse.


Ok, I can make this change.


Thanks.

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 1/5] hbitmap: handle set/reset with zero length

[Max Reitz]

On 11.10.19 11:07, Vladimir Sementsov-Ogievskiy wrote:
> Passing zero length to these functions leads to unpredicted results.
> Zero-length set/reset may occur in active-mirror, on zero-length write
> (which is unlikely, but not guaranteed to never happen).
> 
> Let's just do nothing on zero-length request.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  util/hbitmap.c | 8 
>  1 file changed, 8 insertions(+)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 4/5] block/mirror: support unaligned write in active mirror

[Max Reitz]

On 11.10.19 11:07, Vladimir Sementsov-Ogievskiy wrote:
> Prior 9adc1cb49af8d do_sync_target_write had a bug: it reset aligned-up
> region in the dirty bitmap, which means that we may not copy some bytes
> and assume them copied, which actually leads to producing corrupted
> target.
> 
> So 9adc1cb49af8d forced dirty bitmap granularity to be
> request_alignment for mirror-top filter, so we are not working with
> unaligned requests. However forcing large alignment obviously decreases
> performance of unaligned requests.
> 
> This commit provides another solution for the problem: if unaligned
> padding is already dirty, we can safely ignore it, as
> 1. It's dirty, it will be copied by mirror_iteration anyway
> 2. It's dirty, so skipping it now we don't increase dirtiness of the
>bitmap and therefore don't damage "synchronicity" of the
>write-blocking mirror.
> 
> If unaligned padding is not dirty, we just write it, no reason to touch
> dirty bitmap if we succeed (on failure we'll set the whole region
> ofcourse, but we loss "synchronicity" on failure anyway).
> 
> Note: we need to disable dirty_bitmap, otherwise we will not be able to
> see in do_sync_target_write bitmap state before current operation. We
> may of course check dirty bitmap before the operation in
> bdrv_mirror_top_do_write and remember it, but we don't need active
> dirty bitmap for write-blocking mirror anyway.
> 
> New code-path is unused until the following commit reverts
> 9adc1cb49af8d.
> 
> Suggested-by: Denis V. Lunev 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/mirror.c | 71 +++---
>  1 file changed, 68 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

[PATCH] iotests: Remove 130 from the "auto" group

[Thomas Huth]

Peter hit a "Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared
'write' lock - Is another process using the image [TEST_DIR/t.IMGFMT]?"
error with 130 already twice. Looks like this test is a little bit
shaky, and currently nobody has a real clue what could be causing this
issue, so for the time being, let's disable it from the "auto" group so
that it does not gate the pull requests.

Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/group | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 7dac79a783..6aa4b8d098 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -151,7 +151,7 @@
 127 rw backing quick
 128 rw quick
 129 rw quick
-130 rw auto quick
+130 rw quick
 131 rw quick
 132 rw quick
 133 auto quick
-- 
2.18.1

Re: [PATCH 4/5] iotests: Skip "make check-block" if QEMU does not support virtio-blk

[Max Reitz]

On 11.10.19 16:50, Thomas Huth wrote:
> The next patch is going to add some python-based tests to the "auto"
> group, and these tests require virtio-blk to work properly. Running
> iotests without virtio-blk likely does not make too much sense anyway,
> so instead of adding a check for the availability of virtio-blk to each
> and every test (which does not sound very appealing), let's rather add
> a check for this at the top level in the check-block.sh script instead
> (so that it is possible to run "make check" without the "check-block"
> part for qemu-system-tricore for example).
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/check-block.sh | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/check-block.sh b/tests/check-block.sh
> index 679aedec50..7582347ec2 100755
> --- a/tests/check-block.sh
> +++ b/tests/check-block.sh
> @@ -26,10 +26,24 @@ if grep -q "CFLAGS.*-fsanitize" config-host.mak 
> 2>/dev/null ; then
>  exit 0
>  fi
>  
> -if [ -z "$(find . -name 'qemu-system-*' -print)" ]; then
> +if [ -n "$QEMU_PROG" ]; then
> +qemu_prog="$QEMU_PROG"
> +else
> +for binary in *-softmmu/qemu-system-* ; do
> +if [ -x "$binary" ]; then
> +qemu_prog="$binary"
> +break
> +fi
> +done
> +fi
> +if [ -z "$qemu_prog" ]; then
>  echo "No qemu-system binary available ==> Not running the qemu-iotests."
>  exit 0
>  fi
> +if ! "$qemu_prog" -M none -device help | grep virtio-blk >/dev/null 2>&1 ; 
> then

Maybe grep -q?

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] blockdev: modify blockdev-change-medium to change non-removable device

[Eric Blake]

On 10/18/19 7:09 AM, Denis Plotnikov wrote:

The modification is useful to workaround exclusive file access restrictions,
e.g. to implement VM migration with shared disk stored on a storage with
the exclusive file opening model: a destination VM is started waiting for
incomming migration with a fake image drive, and later, on the last migration
phase, the fake image file is replaced with the real one.

Signed-off-by: Denis Plotnikov 
---



+++ b/qapi/block-core.json
@@ -4769,6 +4769,8 @@
  # @read-only-mode:  change the read-only mode of the device; defaults
  #   to 'retain'
  #
+# @medium-name: drive-name when changing the media in non-removable devices
+#   ignored when changing media in removable devices


s/devices ignored/devices. Ignored/

Missing a '(since 4.2)' tag.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v2 00/23] iotests: Add and use $SOCK_DIR

[Max Reitz]

On 17.10.19 15:31, Max Reitz wrote:
> Hi,
> 
> Perhaps the main reason we cannot run important tests such as 041 in CI
> is that when they care Unix sockets in $TEST_DIR, the path may become
> too long to connect to them.
> 
> To get by this problem, this series lets the check script create a new
> temporary directory (mktemp -d) and then makes the iotests use it for
> all Unix sockets.
> 
> 
> v2:
> - Patch 1: Use mkdir -p
> - Patches 4/23: Only add the $SOCK_DIR replacement line in patch 4 and
> only drop the $TEST_DIR line in patch 23
>   (Took Eric’s R-b on both because that’s how I interpreted his
>   comments)

Thanks for the rewies, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 5/5] iotests: Enable more tests in the 'auto' group to improve test coverage

[Max Reitz]

On 11.10.19 16:50, Thomas Huth wrote:
> According to Kevin, tests 030, 040 and 041 are among the most valuable
> tests that we have, so we should always run them if possible, even if
> they take a little bit longer.
> 
> According to Max, it would be good to have a test for iothreads and
> migration. 127 and 256 seem to be good candidates for iothreads. For
> migration, let's enable 091, 181, 183, and 203 (which also tests
> iothreads).
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/qemu-iotests/group | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)

I’m not sure whether I’ve recently seen intermittent failures in one of
91, 181, or 183, so I’ll have to look into that again.

In the meantime:

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v8 2/3] docs: define padding for qcow2 header

[Vladimir Sementsov-Ogievskiy]

18.10.2019 17:02, Eric Blake wrote:
> On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Header extensions ends are already defined to be multiply of 8. Let's
>> gently ask for header length to be a multiply of 8 too, when we have
>> some additional fields. Requiring this may be considered as an
>> incompatible change, so the padding is optional. Actually, padding is
>> allowed before this patch (due to definition of additional fields),
>> the only actual change is "SHOULD" word.
> 
> Too weak. I've already argued that this should be mandatory, and that we are 
> not breaking backwards compatibility, but merely clarifying what has already 
> been implicit by the fact that header extensions are required to be 8-byte 
> size multiple (which makes no sense unless they are also 8-byte aligned).

OK

> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   docs/interop/qcow2.txt | 5 +
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index 4709f3bb30..b971e59b1a 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -185,6 +185,11 @@ which is covered by @header_length must be zeroed.
>>   < ... No additional fields in the header currently ... >
>> +Header padding
>> +    If @header_length is larger than 104, software SHOULD make it a
>> +    multiply of 8, adding zero-padding after additional fields. Still 
>> the
>> +    padding is optional and may be absent in the image.
>> +
>>   Directly after the image header, optional sections called header 
>> extensions can
>>   be stored. Each extension has a structure like the following:
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v8 3/3] docs: qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

18.10.2019 17:04, Eric Blake wrote:
> On 10/18/19 4:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The patch add new additional field to qcow2 header: compression_type,
>> which specifies compression type. If field is absent or zero, default
>> compression type is set: ZLIB, which corresponds to current behavior.
>>
>> New compression type (ZSTD) is to be added in further commit.
>>
>> Suggested-by: Denis Plotnikov 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   docs/interop/qcow2.txt | 19 ++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index b971e59b1a..4eabd81363 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -109,6 +109,12 @@ least next five fields, up to the @header_length field.
>>   An External Data File Name header 
>> extension may
>>   be present if this bit is set.
>> +    Bit 3:  Compression type bit.  If this bit is set,
>> +    non-default compression is used for 
>> compressed
>> +    clusters. In this case, @header_length must
>> +    be at least 105 and @compression_type field
>> +    must be non-zero.
> 
> I want to insist on 8-byte alignment, so this should be at least 112.
> 
> Also, as pointed out in v7, use of the decoration '@header_length' and 
> '@compression_type' is not consistent with the rest of the document. Drop the 
> @.

Missed this, sorry(

> 
>> +
>>   Bits 3-63:  Reserved (set to 0)
>>    80 -  87:  compatible_features
>> @@ -183,7 +189,18 @@ It's allowed for the header end to cut some field in 
>> the middle (in this case
>>   the field is considered as absent), but in this case the part of the field
>>   which is covered by @header_length must be zeroed.
>> -    < ... No additional fields in the header currently ... >
>> +  104:  compression_type
>> +    Defines the compression method used for compressed 
>> clusters.
>> +    A single compression type is applied to all compressed 
>> image
>> +    clusters.
>> +    If incompatible compression type bit is set: the field 
>> must
>> +    exist (i.e. @header_length >= 105) and must be non-zero 
>> (
>> +    which means non-zlib compression type)
>> +    If incompatible compression type bit is unset: the field
>> +    may not exist (if @header_length < 105) or it must be 
>> zero
>> +    (which means zlib).
>> +    Available compression type values:
>> +    0: zlib 
> 
> One byte for the field is fine, but I'd still explicitly document 7 bytes of 
> unused padding, since I want to insist on an 8-byte multiple.
> 

OK

-- 
Best regards,
Vladimir

Re: [PATCH] blockdev: modify blockdev-change-medium to change non-removable device

[Max Reitz]

On 18.10.19 14:09, Denis Plotnikov wrote:
> The modification is useful to workaround exclusive file access restrictions,
> e.g. to implement VM migration with shared disk stored on a storage with
> the exclusive file opening model: a destination VM is started waiting for
> incomming migration with a fake image drive, and later, on the last migration
> phase, the fake image file is replaced with the real one.
> 
> Signed-off-by: Denis Plotnikov 

Isn’t this what we would want to use reopen for?

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 2/5] iotests: Test 041 does not work on macOS

[Max Reitz]

On 11.10.19 16:50, Thomas Huth wrote:
> 041 works fine on Linux, FreeBSD and OpenBSD, so let's mark it as
> only supported on these systems.
> 
> Signed-off-by: Thomas Huth 
> ---
>  tests/qemu-iotests/041 | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 8568426311..7211f1950a 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041
> @@ -1123,4 +1123,5 @@ class TestOrphanedSource(iotests.QMPTestCase):
>  
>  if __name__ == '__main__':
>  iotests.main(supported_fmts=['qcow2', 'qed'],
> - supported_protocols=['file'])
> + supported_protocols=['file'],
> + supported_platforms=['linux', 'freebsd', 'openbsd6'])

Hmm, why not just “openbsd”, or single out macOS as unsupported?  (I
suppose the latter would require an additional patch.)

There’s also the fact that I maybe wanted to let make check skip all
tests on macOS and Windows – though that doesn’t make this patch
unnecessary, because that would only fix make check, not running the
iotests explicitly.

And I suppose maybe we even want to run the tests in macOS if it’s part
of the CI anyway?

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 0/5] active-mirror: support unaligned guest operations

[Max Reitz]

On 11.10.19 11:07, Vladimir Sementsov-Ogievskiy wrote:
> Commit 9adc1cb49af8d fixed a bug about unaligned (to dirty bitmap
> granularity) guest writes (and discards) by simply requesting
> corresponding alignment on mirror-top filter. However forcing large
> alignment obviously decreases performance of unaligned requests.
> 
> So it's time for a new solution which is in 04. And 05 reverts
> 9adc1cb49af8d.
> 
> v2:
> 01: new fix (do we need it for stable?)

I don’t know? :-)

I’ll just add the stable tag for good measure, I suppose it can’t hurt.

> 02,03,05: add Max's r-b
> 04: fix bitmap handling
> improve comments
> 
> Vladimir Sementsov-Ogievskiy (5):
>   hbitmap: handle set/reset with zero length
>   block/mirror: simplify do_sync_target_write
>   block/block-backend: add blk_co_pwritev_part
>   block/mirror: support unaligned write in active mirror
>   Revert "mirror: Only mirror granularity-aligned chunks"
> 
>  include/sysemu/block-backend.h |   4 +
>  block/block-backend.c  |  17 +++-
>  block/mirror.c | 181 -
>  util/hbitmap.c |   8 ++
>  4 files changed, 114 insertions(+), 96 deletions(-)

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 0/5] active-mirror: support unaligned guest operations

[Vladimir Sementsov-Ogievskiy]

18.10.2019 18:54, Max Reitz wrote:
> On 11.10.19 11:07, Vladimir Sementsov-Ogievskiy wrote:
>> Commit 9adc1cb49af8d fixed a bug about unaligned (to dirty bitmap
>> granularity) guest writes (and discards) by simply requesting
>> corresponding alignment on mirror-top filter. However forcing large
>> alignment obviously decreases performance of unaligned requests.
>>
>> So it's time for a new solution which is in 04. And 05 reverts
>> 9adc1cb49af8d.
>>
>> v2:
>> 01: new fix (do we need it for stable?)
> 
> I don’t know? :-)
> 
> I’ll just add the stable tag for good measure, I suppose it can’t hurt.
> 
>> 02,03,05: add Max's r-b
>> 04: fix bitmap handling
>>  improve comments
>>
>> Vladimir Sementsov-Ogievskiy (5):
>>hbitmap: handle set/reset with zero length
>>block/mirror: simplify do_sync_target_write
>>block/block-backend: add blk_co_pwritev_part
>>block/mirror: support unaligned write in active mirror
>>Revert "mirror: Only mirror granularity-aligned chunks"
>>
>>   include/sysemu/block-backend.h |   4 +
>>   block/block-backend.c  |  17 +++-
>>   block/mirror.c | 181 -
>>   util/hbitmap.c |   8 ++
>>   4 files changed, 114 insertions(+), 96 deletions(-)
> 
> Thanks, applied to my block branch:
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
> 
> Max
> 

Thank you!

-- 
Best regards,
Vladimir

Re: [PATCH 1/5] iotests: remove 'linux' from default supported platforms

[Max Reitz]

On 11.10.19 16:50, Thomas Huth wrote:
> From: John Snow 
> 
> verify_platform will check an explicit whitelist and blacklist instead.
> The default will now be assumed to be allowed to run anywhere.
> 
> For tests that do not specify their platforms explicitly, this has the effect 
> of
> enabling these tests on non-linux platforms. For tests that always specified
> linux explicitly, there is no change.
> 
> For Python tests on FreeBSD at least; only seven python tests fail:
> 045 147 149 169 194 199 211
> 
> 045 and 149 appear to be misconfigurations,
> 147 and 194 are the AF_UNIX path too long error,
> 169 and 199 are bitmap migration bugs, and
> 211 is a bug that shows up on Linux platforms, too.
> 
> This is at least good evidence that these tests are not Linux-only. If
> they aren't suitable for other platforms, they should be disabled on a
> per-platform basis as appropriate.
> 
> Therefore, let's switch these on and deal with the failures.
> 
> Signed-off-by: John Snow 
> ---
>  tests/qemu-iotests/iotests.py | 16 +++-
>  1 file changed, 11 insertions(+), 5 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 1/3] iotests: Fix 173

[Eric Blake]

On 10/17/19 7:17 AM, Max Reitz wrote:


Why haven't we noticed the failure? Because the test rarely gets run:
'./check -qcow2 173' is insufficient (that defaults to using file protocol)
'./check -nfs 173' is insufficient (that defaults to using raw format)
so the test is only run with:
./check -qcow2 -nfs 173

Note that we already have a number of other problems with -nfs:
./check -nfs (fails 18/30)
./check -qcow2 -nfs (fails 45/76 after this patch)
and it's not on my priority list to fix those.  Rather, I found this
because of my next patch's work on tests using _send_qemu_cmd.

Fixes: 655ae6b
Signed-off-by: Eric Blake 
---
  tests/qemu-iotests/173 | 4 ++--
  tests/qemu-iotests/173.out | 6 +-
  2 files changed, 7 insertions(+), 3 deletions(-)


On second thought, I wonder whether this test actually does anything
with NFS.  It doesn’t look like it to me.

I wonder because for some reason I can’t get NFS to work with qemu at
all.  I don’t think the iotests are at fault why so many tests fail,
actually.


OK, I was just missing an “insecure” in my exports.  I hate debugging NFS.


Hmm, that probably explains some of my failures as well.  It is not 
obvious what has to be done to a system prior to being able to even run 
'./check -nfs'; and it would be nice if ./check could give better 
heads-up warnings about an insufficient setup.


You may indeed have a point that the test may work with other non-local 
setups.  But with this a quick hack:


diff --git i/tests/qemu-iotests/173 w/tests/qemu-iotests/173
index 29dcaa1960df..c01d0186c6ba 100755
--- i/tests/qemu-iotests/173
+++ w/tests/qemu-iotests/173
@@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.qemu

 _supported_fmt qcow2
-_supported_proto nfs
+_supported_proto nbd nfs

 size=100M

./check -qcow2 -nfs 173   # still passes
./check -qcow2 -nbd 173   # fails:

+{"error": {"class": "GenericError", "desc": "Failed to get "write" lock"}}
+Timeout waiting for return on handle 0

there may be more I can do to let this test work with nbd as well, but 
for the sake of getting this email sent, that's as far as I've gotten 
for now.




Now I’m down to 16/76 for qcow2, and most of those look benign.  (As in,
they simply don’t support nfs.)

Max



--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] iotests: Remove 130 from the "auto" group

[John Snow]

On 10/18/19 12:10 PM, Thomas Huth wrote:
> Peter hit a "Could not open 'TEST_DIR/t.IMGFMT': Failed to get shared
> 'write' lock - Is another process using the image [TEST_DIR/t.IMGFMT]?"
> error with 130 already twice. Looks like this test is a little bit
> shaky, and currently nobody has a real clue what could be causing this
> issue, so for the time being, let's disable it from the "auto" group so
> that it does not gate the pull requests.
> 
> Signed-off-by: Thomas Huth 

Reviewed-by: John Snow 

> ---
>  tests/qemu-iotests/group | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 7dac79a783..6aa4b8d098 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -151,7 +151,7 @@
>  127 rw backing quick
>  128 rw quick
>  129 rw quick
> -130 rw auto quick
> +130 rw quick
>  131 rw quick
>  132 rw quick
>  133 auto quick
> 

-- 
—js

Re: [PATCH v6 1/4] block/replication.c: Ignore requests after failover

[Lukas Straub]

On Sat, 5 Oct 2019 15:05:23 +0200
Lukas Straub  wrote:

> After failover the Secondary side of replication shouldn't change state, 
> because
> it now functions as our primary disk.
>
> In replication_start, replication_do_checkpoint, replication_stop, ignore
> the request if current state is BLOCK_REPLICATION_DONE (sucessful failover) or
> BLOCK_REPLICATION_FAILOVER (failover in progres i.e. currently merging active
> and hidden images into the base image).
>
> Signed-off-by: Lukas Straub 
> Reviewed-by: Zhang Chen 
> ---
>  block/replication.c | 38 +++---
>  1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/block/replication.c b/block/replication.c
> index 3d4dedddfc..97cc65c0cf 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -454,6 +454,17 @@ static void replication_start(ReplicationState *rs, 
> ReplicationMode mode,
>  aio_context_acquire(aio_context);
>  s = bs->opaque;
>
> +if (s->stage == BLOCK_REPLICATION_DONE ||
> +s->stage == BLOCK_REPLICATION_FAILOVER) {
> +/*
> + * This case happens when a secondary is promoted to primary.
> + * Ignore the request because the secondary side of replication
> + * doesn't have to do anything anymore.
> + */
> +aio_context_release(aio_context);
> +return;
> +}
> +
>  if (s->stage != BLOCK_REPLICATION_NONE) {
>  error_setg(errp, "Block replication is running or done");
>  aio_context_release(aio_context);
> @@ -529,8 +540,7 @@ static void replication_start(ReplicationState *rs, 
> ReplicationMode mode,
> "Block device is in use by internal backup job");
>
>  top_bs = bdrv_lookup_bs(s->top_id, s->top_id, NULL);
> -if (!top_bs || !bdrv_is_root_node(top_bs) ||
> -!check_top_bs(top_bs, bs)) {
> +if (!top_bs || !check_top_bs(top_bs, bs)) {
>  error_setg(errp, "No top_bs or it is invalid");
>  reopen_backing_file(bs, false, NULL);
>  aio_context_release(aio_context);
> @@ -577,6 +587,17 @@ static void replication_do_checkpoint(ReplicationState 
> *rs, Error **errp)
>  aio_context_acquire(aio_context);
>  s = bs->opaque;
>
> +if (s->stage == BLOCK_REPLICATION_DONE ||
> +s->stage == BLOCK_REPLICATION_FAILOVER) {
> +/*
> + * This case happens when a secondary was promoted to primary.
> + * Ignore the request because the secondary side of replication
> + * doesn't have to do anything anymore.
> + */
> +aio_context_release(aio_context);
> +return;
> +}
> +
>  if (s->mode == REPLICATION_MODE_SECONDARY) {
>  secondary_do_checkpoint(s, errp);
>  }
> @@ -593,7 +614,7 @@ static void replication_get_error(ReplicationState *rs, 
> Error **errp)
>  aio_context_acquire(aio_context);
>  s = bs->opaque;
>
> -if (s->stage != BLOCK_REPLICATION_RUNNING) {
> +if (s->stage == BLOCK_REPLICATION_NONE) {
>  error_setg(errp, "Block replication is not running");
>  aio_context_release(aio_context);
>  return;
> @@ -635,6 +656,17 @@ static void replication_stop(ReplicationState *rs, bool 
> failover, Error **errp)
>  aio_context_acquire(aio_context);
>  s = bs->opaque;
>
> +if (s->stage == BLOCK_REPLICATION_DONE ||
> +s->stage == BLOCK_REPLICATION_FAILOVER) {
> +/*
> + * This case happens when a secondary was promoted to primary.
> + * Ignore the request because the secondary side of replication
> + * doesn't have to do anything anymore.
> + */
> +aio_context_release(aio_context);
> +return;
> +}
> +
>  if (s->stage != BLOCK_REPLICATION_RUNNING) {
>  error_setg(errp, "Block replication is not running");
>  aio_context_release(aio_context);

Hello Everyone,
Could the block people have a look at this patch?

Regards,
Lukas Straub

Re: [PATCH] qemu-img.texi: Describe data_file and data_file_raw

[John Snow]

CC qemu-block

On 10/18/19 5:59 AM, Han Han wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1763105
> 
> Signed-off-by: Han Han 
> ---
>  qemu-img.texi | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/qemu-img.texi b/qemu-img.texi
> index b5156d6316..44596c2d93 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -763,6 +763,16 @@ file which is COW and has data blocks already, it 
> couldn't be changed to NOCOW
>  by setting @code{nocow=on}. One can issue @code{lsattr filename} to check if
>  the NOCOW flag is set or not (Capital 'C' is NOCOW flag).
>  
> +@item data_file
> +File name of data file that is stored in the image and used as a default for
> +opening the image. If the option is used, qcow2 file only stores the metadata
> +of the image.
> +

This is a little unclear, and seems to imply the data file is stored
*IN* the image.

"Optional filename to be used as a data store for this qcow2 file. If
this option is used, the qcow2 file only stores metadata for this image."

> +@item data_file_raw
> +This option requires @option{data_file}. If this option is set to @code{on},
> +qemu will always keep the external data file consistent as a standalone
> +read-only raw image. Default value is @code{off}.
> +
>  @end table
>  
>  @item Other
> 

qemu crashing when attaching an ISO file to a virtio-scsi CD-ROM device through libvirt

[Fernando Casas Schössow]

Hi,

Today while working with two different Windows Server 2012 R2 guests I found 
that when I try to attach an ISO file to a SCSI CD-ROM device through libvirt 
(virsh or virt-manager) while the guest is running, qemu crashes and the 
following message is logged:

Assertion failed: blk_get_aio_context(d->conf.blk) == s->ctx 
(/home/buildozer/aports/main/qemu/src/qemu-4.0.0/hw/scsi/virtio-scsi.c: 
virtio_scsi_ctx_check: 246)

I can repro this at will. All I have to do is to try to attach an ISO file to 
the SCSI CDROM while the guest is running.
The SCSI controller model is virtio-scsi with iothread enabled.
Please find below all the details about my setup that I considered relevant but 
I missed something please don't hesitate to let me know:

Host arch: x86_64
Distro: Alpine Linux 3.10.2
qemu version: 4.0
Linux kernel version: 4.19.67
libvirt: 5.5.0
Emulated SCSI controller: virtio-scsi (with iothread enabled)
Guest firmware: OVMF-EFI
Guest OS: Window Server 2012 R2
Guest virtio drivers version: 171 (current stable)

qemu command line:

/usr/bin/qemu-system-x86_64 -name guest=DCHOMENET01,debug-threads=on -S -object 
secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-78-DCHOMENET01/master-key.aes
 -machine pc-i440fx-4.0,accel=kvm,usb=off,dump-guest-core=on -cpu 
IvyBridge,ss=on,vmx=off,pcid=on,hypervisor=on,arat=on,tsc_adjust=on,umip=on,xsaveopt=on,hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0x1fff
 -drive 
file=/usr/share/edk2.git/ovmf-x64/OVMF_CODE-pure-efi.fd,if=pflash,format=raw,unit=0,readonly=on
 -drive 
file=/var/lib/libvirt/qemu/nvram/DCHOMENET01_VARS.fd,if=pflash,format=raw,unit=1
 -m 1536 -overcommit mem-lock=off -smp 1,sockets=1,cores=1,threads=1 -object 
iothread,id=iothread1 -uuid f06978ad-2734-44ab-a518-5dfcf71d625e 
-no-user-config -nodefaults -chardev socket,id=charmonitor,fd=33,server,nowait 
-mon chardev=charmonitor,id=monitor,mode=control -rtc 
base=localtime,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet 
-no-shutdown -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot 
strict=on -device qemu-xhci,id=usb,bus=pci.0,addr=0x4 -device 
virtio-scsi-pci,iothread=iothread1,id=scsi0,num_queues=1,bus=pci.0,addr=0x5 
-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x6 -drive 
file=/storage/storage-hdd-vms/virtual_machines_hdd/dchomenet01.qcow2,format=qcow2,if=none,id=drive-scsi0-0-0-0,cache=none,discard=unmap,aio=threads
 -device 
scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,device_id=drive-scsi0-0-0-0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1,write-cache=on
 -drive if=none,id=drive-scsi0-0-0-1,readonly=on -device 
scsi-cd,bus=scsi0.0,channel=0,scsi-id=0,lun=1,device_id=drive-scsi0-0-0-1,drive=drive-scsi0-0-0-1,id=scsi0-0-0-1
 -netdev tap,fd=41,id=hostnet0,vhost=on,vhostfd=43 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:99:b5:62,bus=pci.0,addr=0x3 
-chardev socket,id=charserial0,host=127.0.0.1,port=4900,telnet,server,nowait 
-device isa-serial,chardev=charserial0,id=serial0 -chardev 
spicevmc,id=charchannel0,name=vdagent -device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
 -chardev socket,id=charchannel1,fd=45,server,nowait -device 
virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,name=org.qemu.guest_agent.0
 -chardev spiceport,id=charchannel2,name=org.spice-space.webdav.0 -device 
virtserialport,bus=virtio-serial0.0,nr=3,chardev=charchannel2,id=channel2,name=org.spice-space.webdav.0
 -device virtio-tablet-pci,id=input2,bus=pci.0,addr=0x7 -spice 
port=5900,addr=127.0.0.1,disable-ticketing,seamless-migration=on -device 
qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2
 -chardev spicevmc,id=charredir0,name=usbredir -device 
usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=2 -chardev 
spicevmc,id=charredir1,name=usbredir -device 
usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=3 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 -sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny -msg 
timestamp=on

I can provide a core dump of the process if needed for debugging and the guest 
XML as well.

Thanks.

Fernando

qemu crashing when attaching an ISO file to a virtio-scsi CD-ROM device through libvirt

[Fernando Casas Schössow]

Hi,

Today while working with two different Windows Server 2012 R2 guests I 
found that when I try to attach an ISO file to a SCSI CD-ROM device 
through libvirt (virsh or virt-manager) while the guest is running, 
qemu crashes and the following message is logged:

Assertion failed: blk_get_aio_context(d->conf.blk) == s->ctx 
(/home/buildozer/aports/main/qemu/src/qemu-4.0.0/hw/scsi/virtio-scsi.c: 
virtio_scsi_ctx_check: 246)

I can repro this at will. All I have to do is to try to attach an ISO 
file to the SCSI CDROM while the guest is running.
The SCSI controller model is virtio-scsi with iothread enabled.
Please find below all the details about my setup that I considered 
relevant but I missed something please don't hesitate to let me know:

Host arch: x86_64
Distro: Alpine Linux 3.10.2
qemu version: 4.0
Linux kernel version: 4.19.67
libvirt: 5.5.0
Emulated SCSI controller: virtio-scsi (with iothread enabled)
Guest firmware: OVMF-EFI
Guest OS: Window Server 2012 R2
Guest virtio drivers version: 171 (current stable)

qemu command line:

/usr/bin/qemu-system-x86_64 -name guest=DCHOMENET01,debug-threads=on -S 
-object 
secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-78-DCHOMENET01/master-key.aes
 
-machine pc-i440fx-4.0,accel=kvm,usb=off,dump-guest-core=on -cpu 
IvyBridge,ss=on,vmx=off,pcid=on,hypervisor=on,arat=on,tsc_adjust=on,umip=on,xsaveopt=on,hv_time,hv_relaxed,hv_vapic,hv_spinlocks=0x1fff
 
-drive 
file=/usr/share/edk2.git/ovmf-x64/OVMF_CODE-pure-efi.fd,if=pflash,format=raw,unit=0,readonly=on
 
-drive 
file=/var/lib/libvirt/qemu/nvram/DCHOMENET01_VARS.fd,if=pflash,format=raw,unit=1
 
-m 1536 -overcommit mem-lock=off -smp 1,sockets=1,cores=1,threads=1 
-object iothread,id=iothread1 -uuid 
f06978ad-2734-44ab-a518-5dfcf71d625e -no-user-config -nodefaults 
-chardev socket,id=charmonitor,fd=33,server,nowait -mon 
chardev=charmonitor,id=monitor,mode=control -rtc 
base=localtime,driftfix=slew -global kvm-pit.lost_tick_policy=delay 
-no-hpet -no-shutdown -global PIIX4_PM.disable_s3=1 -global 
PIIX4_PM.disable_s4=1 -boot strict=on -device 
qemu-xhci,id=usb,bus=pci.0,addr=0x4 -device 
virtio-scsi-pci,iothread=iothread1,id=scsi0,num_queues=1,bus=pci.0,addr=0x5 
-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x6 -drive 
file=/storage/storage-hdd-vms/virtual_machines_hdd/dchomenet01.qcow2,format=qcow2,if=none,id=drive-scsi0-0-0-0,cache=none,discard=unmap,aio=threads
 
-device 
scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,device_id=drive-scsi0-0-0-0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1,write-cache=on
 
-drive if=none,id=drive-scsi0-0-0-1,readonly=on -device 
scsi-cd,bus=scsi0.0,channel=0,scsi-id=0,lun=1,device_id=drive-scsi0-0-0-1,drive=drive-scsi0-0-0-1,id=scsi0-0-0-1
 
-netdev tap,fd=41,id=hostnet0,vhost=on,vhostfd=43 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:99:b5:62,bus=pci.0,addr=0x3 
-chardev 
socket,id=charserial0,host=127.0.0.1,port=4900,telnet,server,nowait 
-device isa-serial,chardev=charserial0,id=serial0 -chardev 
spicevmc,id=charchannel0,name=vdagent -device 
virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=com.redhat.spice.0
 
-chardev socket,id=charchannel1,fd=45,server,nowait -device 
virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,name=org.qemu.guest_agent.0
 
-chardev spiceport,id=charchannel2,name=org.spice-space.webdav.0 
-device 
virtserialport,bus=virtio-serial0.0,nr=3,chardev=charchannel2,id=channel2,name=org.spice-space.webdav.0
 
-device virtio-tablet-pci,id=input2,bus=pci.0,addr=0x7 -spice 
port=5900,addr=127.0.0.1,disable-ticketing,seamless-migration=on 
-device 
qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2
 
-chardev spicevmc,id=charredir0,name=usbredir -device 
usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=2 -chardev 
spicevmc,id=charredir1,name=usbredir -device 
usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=3 -device 
virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 -sandbox 
on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny 
-msg timestamp=on

I can provide a core dump of the process if needed for debugging and 
the guest XML as well.

Thanks.

Fernando