Re: [PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands

[Markus Armbruster]

Kevin Wolf  writes:

> This patch adds a new 'coroutine' flag to QMP command definitions that
> tells the QMP dispatcher that the command handler is safe to be run in a
> coroutine.

I'm afraid I missed this question in my review of v3: when is a handler
*not* safe to be run in a coroutine?

> The documentation of the new flag pretends that this flag is already
> used as intended, which it isn't yet after this patch. We'll implement
> this in another patch in this series.
>
> Signed-off-by: Kevin Wolf 
> Reviewed-by: Marc-André Lureau 

Re: [PATCH 2/5] docs/sphinx: Add new hxtool Sphinx extension

[Richard Henderson]

On 1/21/20 12:22 PM, Peter Maydell wrote:
>> This doesn't seem to work for me.
>>
>> make[1]: Leaving directory '/home/rth/qemu/qemu/slirp'
>> CONFDIR="/home/rth/qemu/run/etc/qemu" sphinx-build  -W -b html -D
>> version=4.2.50 -D release="4.2.50 (rth)" -d .doctrees/devel-html
>> /home/rth/qemu/qemu/docs/devel docs/devel
>> Running Sphinx v1.8.5
>>
>> Extension error:
>> Could not import extension hxtool (exception: cannot import name 
>> ExtensionError)
>> make: *** [Makefile:1022: docs/devel/index.html] Error 2
> 
> I suspect this is an incompatibility (or possibly just a
> dropped back-compatibility I was accidentally relying on)
> between Sphinx 1.7 and 1.8. (I tested with a 1.6 and a 1.7.)
> 
> It looks like ExtensionError is now in sphinx.errors, so if you
> change
> +from sphinx.application import ExtensionError
> 
> to "from sphinx.errors import ExtensionError" does that help?

Yep, that's it.  Thanks,


r~

Re: [PATCH 0/2] qemu-img: Fix convert -n -B for backing-less targets

[John Snow]

On 1/21/20 10:59 AM, Max Reitz wrote:
> Hi,
> 
> When reviewing David’s series to add --target-is-zero convert, I looked
> for a case to show that the current implementation will crash if
> -n --target-is-zero is used together with -B.  It then turned out that
> -B will always crash when combined with -n and the target image does not
> have a backing file set in its image header.
> 
> This series fixes that.
> 
> 
> Max Reitz (2):
>   qemu-img: Fix convert -n -B for backing-less targets
>   iotests: Test convert -n -B to backing-less target
> 
>  qemu-img.c |  2 +-
>  tests/qemu-iotests/122 | 14 ++
>  tests/qemu-iotests/122.out |  5 +
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 

Hello.
Makes sense to me.

Reviewed-by: John Snow 

(My brain had an awfully tumultuous 35 seconds comprehending that
"is_new" was not a synonym for "-n was provided", but actually means the
opposite.)

Re: [PATCH 2/5] docs/sphinx: Add new hxtool Sphinx extension

[Peter Maydell]

On Tue, 21 Jan 2020 at 21:54, Richard Henderson
 wrote:
>
> On 1/21/20 9:10 AM, Peter Maydell wrote:
> > Some of our documentation includes sections which are created
> > by assembling fragments of texinfo from a .hx source file into
> > a .texi file, which is then included from qemu-doc.texi or
> > qemu-img.texi.
> >
> > For Sphinx, rather than creating a file to include, the most natural
> > way to handle this is to have a small custom Sphinx extension which
> > reads the .hx file and process it.  So instead of:
> >  * makefile produces foo.texi from foo.hx
> >  * qemu-doc.texi says '@include foo.texi'
> > we have:
> >  * qemu-doc.rst says 'hxtool-doc:: foo.hx'
> >  * the Sphinx extension for hxtool has code that runs to handle that
> >Sphinx directive which reads the .hx file and emits the appropriate
> >documentation contents
> >
> > This is pretty much the same way the kerneldoc extension works right
> > now. It also has the advantage that it should work for third-party
> > services like readthedocs that expect to build the docs directly with
> > sphinx rather than by invoking our makefiles.
> >
> > In this commit we implement the hxtool extension.
> >
> > Note that syntax errors in the rST fragments will be correctly
> > reported to the user with the filename and line number within the
> > hx file.
> >
> > Signed-off-by: Peter Maydell 
>
> This doesn't seem to work for me.
>
> make[1]: Leaving directory '/home/rth/qemu/qemu/slirp'
> CONFDIR="/home/rth/qemu/run/etc/qemu" sphinx-build  -W -b html -D
> version=4.2.50 -D release="4.2.50 (rth)" -d .doctrees/devel-html
> /home/rth/qemu/qemu/docs/devel docs/devel
> Running Sphinx v1.8.5
>
> Extension error:
> Could not import extension hxtool (exception: cannot import name 
> ExtensionError)
> make: *** [Makefile:1022: docs/devel/index.html] Error 2

I suspect this is an incompatibility (or possibly just a
dropped back-compatibility I was accidentally relying on)
between Sphinx 1.7 and 1.8. (I tested with a 1.6 and a 1.7.)

It looks like ExtensionError is now in sphinx.errors, so if you
change
+from sphinx.application import ExtensionError

to "from sphinx.errors import ExtensionError" does that help?

If so then I'll test later this week whether that works also
for 1.7/1.6 or if we need to do some version-specific stuff.

thanks
-- PMM

Re: [PATCH 3/5] qemu-img-cmds.hx: Add rST documentation fragments

[Richard Henderson]

On 1/21/20 9:10 AM, Peter Maydell wrote:
> Add the rST versions of the documentation fragments.
> Once we've converted qemu-img.texi to rST we can delete
> the texi fragments; for the moment we leave them in place.
> 
> (Commit created with the aid of emacs query-replace-regexp
> from "@var{\([^}]*\)}" to "\,(upcase \1))".)
> 
> Signed-off-by: Peter Maydell 
> ---
>  qemu-img-cmds.hx | 43 ++-
>  1 file changed, 42 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 


r~

Re: [PATCH 2/5] docs/sphinx: Add new hxtool Sphinx extension

[Richard Henderson]

On 1/21/20 9:10 AM, Peter Maydell wrote:
> Some of our documentation includes sections which are created
> by assembling fragments of texinfo from a .hx source file into
> a .texi file, which is then included from qemu-doc.texi or
> qemu-img.texi.
> 
> For Sphinx, rather than creating a file to include, the most natural
> way to handle this is to have a small custom Sphinx extension which
> reads the .hx file and process it.  So instead of:
>  * makefile produces foo.texi from foo.hx
>  * qemu-doc.texi says '@include foo.texi'
> we have:
>  * qemu-doc.rst says 'hxtool-doc:: foo.hx'
>  * the Sphinx extension for hxtool has code that runs to handle that
>Sphinx directive which reads the .hx file and emits the appropriate
>documentation contents
> 
> This is pretty much the same way the kerneldoc extension works right
> now. It also has the advantage that it should work for third-party
> services like readthedocs that expect to build the docs directly with
> sphinx rather than by invoking our makefiles.
> 
> In this commit we implement the hxtool extension.
> 
> Note that syntax errors in the rST fragments will be correctly
> reported to the user with the filename and line number within the
> hx file.
> 
> Signed-off-by: Peter Maydell 

This doesn't seem to work for me.

make[1]: Leaving directory '/home/rth/qemu/qemu/slirp'
CONFDIR="/home/rth/qemu/run/etc/qemu" sphinx-build  -W -b html -D
version=4.2.50 -D release="4.2.50 (rth)" -d .doctrees/devel-html
/home/rth/qemu/qemu/docs/devel docs/devel
Running Sphinx v1.8.5

Extension error:
Could not import extension hxtool (exception: cannot import name ExtensionError)
make: *** [Makefile:1022: docs/devel/index.html] Error 2

> @@ -221,3 +221,4 @@ texinfo_documents = [
>  # find everything.
>  kerneldoc_bin = os.path.join(qemu_docdir, '../scripts/kernel-doc')
>  kerneldoc_srctree = os.path.join(qemu_docdir, '..')
> +hxtool_srctree = os.path.join(qemu_docdir, '..')

I wondered if there was something more needed here?

> diff --git a/docs/sphinx/hxtool.py b/docs/sphinx/hxtool.py

The actual code looks fine.


r~

Re: [PATCH 0/5] qemu-img: convert to rST

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200121191033.28195-1-peter.mayd...@linaro.org/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/tmp/qemu-test/install --python=/usr/bin/python3 
--cross-prefix=x86_64-w64-mingw32- --enable-trace-backends=simple 
--enable-gnutls --enable-nettle --enable-curl --enable-vnc --enable-bzip2 
--enable-guest-agent --enable-docs

ERROR: User requested feature docs
   configure was not able to find it.
   Install texinfo, Perl/perl-podlators and python-sphinx

---
funcs: do_compiler do_cc compile_object check_define main
lines: 93 123 620 637 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o 
config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __linux__ not defined
 #error __linux__ not defined
  ^

---
funcs: do_compiler do_cc compile_object check_define main
lines: 93 123 620 689 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o 
config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __i386__ not defined
 #error __i386__ not defined
  ^

---
funcs: do_compiler do_cc compile_object check_define main
lines: 93 123 620 692 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o 
config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __ILP32__ not defined
 #error __ILP32__ not defined
  ^

---
lines: 93 129 932 0
x86_64-w64-mingw32-gcc -mthreads -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 
-D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef 
-Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv 
-std=gnu99 -o config-temp/qemu-conf.exe config-temp/qemu-conf.c -g -liberty
/usr/lib/gcc/x86_64-w64-mingw32/8.3.0/../../../../x86_64-w64-mingw32/bin/ld: 
cannot find -liberty
collect2: error: ld returned 1 exit status

funcs: do_compiler do_cc compile_object main
lines: 93 123 1861 0
---
funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
lines: 93 129 1942 1946 0
x86_64-w64-mingw32-gcc -m64 -mcx16 -mthreads -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Werror -Wstring-plus-int 
-o config-temp/qemu-conf.exe config-temp/qemu-conf.c -m64 -g
x86_64-w64-mingw32-gcc: error: unrecognized command line option 
'-Wstring-plus-int'; did you mean '-Wstrict-aliasing'?

funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
lines: 93 129 1942 1946 0
x86_64-w64-mingw32-gcc -m64 -mcx16 -mthreads -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Werror 
-Wtypedef-redefinition -o config-temp/qemu-conf.exe config-temp/qemu-conf.c 
-m64 -g
x86_64-w64-mingw32-gcc: error: unrecognized command line option 
'-Wtypedef-redefinition'; did you mean '-Wold-style-definition'?

funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
lines: 93 129 1942 1946 0
x86_64-w64-mingw32-gcc -m64 -mcx16 -mthreads -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Werror 
-Winitializer-overrides -o config-temp/qemu-conf.exe config-temp/qemu-conf.c 
-m64 -g
x86_64-w64-mingw32-gcc: error: unrecognized command line option 
'-Winitializer-overrides'; did you mean '-Wno-suggest-override'?

funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
lines: 93 129 1942 1946 0
---
lines: 93 123 2219 0
x86_64-w64-mingw32-gcc -m64 -mcx16 -mthreads -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Wexpansion-to-defined 
-Wendif-labels -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body 
-Wnested-externs 

Re: [PATCH 1/5] hxtool: Support SRST/ERST directives

[Richard Henderson]

On 1/21/20 9:10 AM, Peter Maydell wrote:
> We want to add support for including rST document fragments
> in our .hx files, in the same way we currently have texinfo
> fragments. These will be delimited by SRST and ERST directives,
> in the same way the texinfo is delimited by STEXI/ETEXI.
> The rST fragments will not be extracted by the hxtool
> script, but by a different mechanism, so all we need to
> do in hxtool is have it ignore all the text inside a
> SRST/ERST section, with suitable error-checking for
> mismatched rST-vs-texi fragment delimiters.
> 
> The resulting effective state machine has only three states:
>  * flag = 0, rstflag = 0 : reading section for C output
>  * flag = 1, rstflag = 0 : reading texi fragment
>  * flag = 0, rstflag = 1 : reading rST fragment
> and flag = 1, rstflag = 1 is not possible. Using two
> variables makes the parallel between the rST handling and
> the texi handling clearer; in any case all this code will
> be deleted once we've converted entirely to rST.
> 
> Signed-off-by: Peter Maydell 
> ---
>  scripts/hxtool | 33 -
>  1 file changed, 32 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 


r~

Re: [PATCH 0/5] qemu-img: convert to rST

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200121191033.28195-1-peter.mayd...@linaro.org/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

Configure options:
--enable-werror --target-list=x86_64-softmmu,aarch64-softmmu 
--prefix=/tmp/qemu-test/install --python=/usr/bin/python3 
--cross-prefix=x86_64-w64-mingw32- --enable-trace-backends=simple 
--enable-gnutls --enable-nettle --enable-curl --enable-vnc --enable-bzip2 
--enable-guest-agent --enable-docs

ERROR: User requested feature docs
   configure was not able to find it.
   Install texinfo, Perl/perl-podlators and python-sphinx

---
funcs: do_compiler do_cc compile_object check_define main
lines: 93 123 620 637 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o 
config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __linux__ not defined
 #error __linux__ not defined
  ^

---
funcs: do_compiler do_cc compile_object check_define main
lines: 93 123 620 689 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o 
config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __i386__ not defined
 #error __i386__ not defined
  ^

---
funcs: do_compiler do_cc compile_object check_define main
lines: 93 123 620 692 0
x86_64-w64-mingw32-gcc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE 
-Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings 
-Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -c -o 
config-temp/qemu-conf.o config-temp/qemu-conf.c
config-temp/qemu-conf.c:2:2: error: #error __ILP32__ not defined
 #error __ILP32__ not defined
  ^

---
lines: 93 129 932 0
x86_64-w64-mingw32-gcc -mthreads -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 
-D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef 
-Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv 
-std=gnu99 -o config-temp/qemu-conf.exe config-temp/qemu-conf.c -g -liberty
/usr/lib/gcc/x86_64-w64-mingw32/8.3.0/../../../../x86_64-w64-mingw32/bin/ld: 
cannot find -liberty
collect2: error: ld returned 1 exit status

funcs: do_compiler do_cc compile_object main
lines: 93 123 1861 0
---
funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
lines: 93 129 1942 1946 0
x86_64-w64-mingw32-gcc -m64 -mcx16 -mthreads -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Werror -Wstring-plus-int 
-o config-temp/qemu-conf.exe config-temp/qemu-conf.c -m64 -g
x86_64-w64-mingw32-gcc: error: unrecognized command line option 
'-Wstring-plus-int'; did you mean '-Wstrict-aliasing'?

funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
lines: 93 129 1942 1946 0
x86_64-w64-mingw32-gcc -m64 -mcx16 -mthreads -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Werror 
-Wtypedef-redefinition -o config-temp/qemu-conf.exe config-temp/qemu-conf.c 
-m64 -g
x86_64-w64-mingw32-gcc: error: unrecognized command line option 
'-Wtypedef-redefinition'; did you mean '-Wold-style-definition'?

funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
lines: 93 129 1942 1946 0
x86_64-w64-mingw32-gcc -m64 -mcx16 -mthreads -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Werror 
-Winitializer-overrides -o config-temp/qemu-conf.exe config-temp/qemu-conf.c 
-m64 -g
x86_64-w64-mingw32-gcc: error: unrecognized command line option 
'-Winitializer-overrides'; did you mean '-Wno-suggest-override'?

funcs: do_compiler do_cc compile_prog cc_has_warning_flag main
lines: 93 129 1942 1946 0
---
lines: 93 123 2219 0
x86_64-w64-mingw32-gcc -m64 -mcx16 -mthreads -D_GNU_SOURCE 
-D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes 
-Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes 
-fno-strict-aliasing -fno-common -fwrapv -std=gnu99 -Wexpansion-to-defined 
-Wendif-labels -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body 
-Wnested-externs 

[PATCH 4/5] qemu-img: Convert invocation documentation to rST

[Peter Maydell]

The qemu-img documentation is currently in qemu-nbd.texi in Texinfo
format, which we present to the user as:
 * a qemu-img manpage
 * a section of the main qemu-doc HTML documentation

Convert the documentation to rST format, and present it to the user as:
 * a qemu-img manpage
 * part of the interop/ Sphinx manual

The qemu-img rST document uses the new hxtool extension
to handle pulling rST fragments out of qemu-img-cmds.hx.

The documentation of the various options and commands is rather
muddled, with some options being described inside the relevant
command description and some in a more general section near the start
of the manual.  All the command synopses are replicated in the .hx
file and then again in the manual.  A lot of text is also duplicated
in the qemu-img.c code for the help text.  I have not attempted to
deal with any of this, but have simply transposed the existing
structure into rST.

As usual, there are some minor formatting changes but no
textual changes, except that as with one or two other conversions
I have dropped the 'see also' section since it's not very
informative and looks odd in the HTML.

Signed-off-by: Peter Maydell 
---
The eagle-eyed will notice that I have not attempted to
create a real linked reference to the 'QEMU block drivers
reference documentation'; this is a bit tricky since it's
(at least sometimes) in a different Sphinx manual to this
document. I opted to leave this sort of nicety for later
rather than hold up the process of conversion.
---
 Makefile  |  19 +-
 MAINTAINERS   |   1 +
 docs/interop/conf.py  |   2 +
 docs/interop/index.rst|   1 +
 docs/interop/qemu-img.rst | 822 ++
 qemu-doc.texi |  10 +-
 qemu-img.texi | 795 
 7 files changed, 839 insertions(+), 811 deletions(-)
 create mode 100644 docs/interop/qemu-img.rst
 delete mode 100644 qemu-img.texi

diff --git a/Makefile b/Makefile
index 318d1046c6c..6d508345f36 100644
--- a/Makefile
+++ b/Makefile
@@ -339,7 +339,8 @@ MANUAL_BUILDDIR := docs
 endif
 
 ifdef BUILD_DOCS
-DOCS=qemu-doc.html qemu-doc.txt qemu.1 qemu-img.1
+DOCS=qemu-doc.html qemu-doc.txt qemu.1
+DOCS+=$(MANUAL_BUILDDIR)/interop/qemu-img.1
 DOCS+=$(MANUAL_BUILDDIR)/interop/qemu-nbd.8
 DOCS+=$(MANUAL_BUILDDIR)/interop/qemu-ga.8
 DOCS+=$(MANUAL_BUILDDIR)/system/qemu-block-drivers.7
@@ -732,7 +733,7 @@ rm -f $(MANUAL_BUILDDIR)/$1/objects.inv 
$(MANUAL_BUILDDIR)/$1/searchindex.js $(M
 endef
 
 distclean: clean
-   rm -f config-host.mak config-host.h* config-host.ld $(DOCS) 
qemu-options.texi qemu-img-cmds.texi qemu-monitor.texi qemu-monitor-info.texi
+   rm -f config-host.mak config-host.h* config-host.ld $(DOCS) 
qemu-options.texi qemu-monitor.texi qemu-monitor-info.texi
rm -f tests/tcg/config-*.mak
rm -f config-all-devices.mak config-all-disas.mak config.status
rm -f $(SUBDIR_DEVICES_MAK)
@@ -830,7 +831,7 @@ ifdef CONFIG_POSIX
$(INSTALL_DATA) $(MANUAL_BUILDDIR)/system/qemu-block-drivers.7 
"$(DESTDIR)$(mandir)/man7"
$(INSTALL_DATA) docs/qemu-cpu-models.7 "$(DESTDIR)$(mandir)/man7"
 ifeq ($(CONFIG_TOOLS),y)
-   $(INSTALL_DATA) qemu-img.1 "$(DESTDIR)$(mandir)/man1"
+   $(INSTALL_DATA) $(MANUAL_BUILDDIR)/interop/qemu-img.1 
"$(DESTDIR)$(mandir)/man1"
$(INSTALL_DIR) "$(DESTDIR)$(mandir)/man8"
$(INSTALL_DATA) $(MANUAL_BUILDDIR)/interop/qemu-nbd.8 
"$(DESTDIR)$(mandir)/man8"
 endif
@@ -1032,6 +1033,10 @@ $(MANUAL_BUILDDIR)/system/index.html: $(call 
manual-deps,system)
 $(MANUAL_BUILDDIR)/interop/qemu-ga.8: $(call manual-deps,interop)
$(call build-manual,interop,man)
 
+$(MANUAL_BUILDDIR)/interop/qemu-img.1: $(call manual-deps,interop) \
+  $(SRC_PATH)/qemu-img-cmds.hx
+   $(call build-manual,interop,man)
+
 $(MANUAL_BUILDDIR)/interop/qemu-nbd.8: $(call manual-deps,interop)
$(call build-manual,interop,man)
 
@@ -1051,9 +1056,6 @@ qemu-monitor.texi: $(SRC_PATH)/hmp-commands.hx 
$(SRC_PATH)/scripts/hxtool
 qemu-monitor-info.texi: $(SRC_PATH)/hmp-commands-info.hx 
$(SRC_PATH)/scripts/hxtool
$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -t < $< > 
$@,"GEN","$@")
 
-qemu-img-cmds.texi: $(SRC_PATH)/qemu-img-cmds.hx $(SRC_PATH)/scripts/hxtool
-   $(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -t < $< > 
$@,"GEN","$@")
-
 docs/interop/qemu-qmp-qapi.texi: qapi/qapi-doc.texi
@cp -p $< $@
 
@@ -1062,7 +1064,6 @@ docs/interop/qemu-ga-qapi.texi: 
qga/qapi-generated/qga-qapi-doc.texi
 
 qemu.1: qemu-doc.texi qemu-options.texi qemu-monitor.texi 
qemu-monitor-info.texi
 qemu.1: qemu-option-trace.texi
-qemu-img.1: qemu-img.texi qemu-option-trace.texi qemu-img-cmds.texi
 fsdev/virtfs-proxy-helper.1: fsdev/virtfs-proxy-helper.texi
 docs/qemu-cpu-models.7: docs/qemu-cpu-models.texi
 scripts/qemu-trace-stap.1: scripts/qemu-trace-stap.texi
@@ -1073,9 +1074,9 @@ pdf: qemu-doc.pdf 

[PATCH 0/5] qemu-img: convert to rST

[Peter Maydell]

This patchset converts the qemu-img documentation to rST format.
It includes a new Sphinx extension which handles parsing the .hx
files which provide documentation fragments for this manual.

The general approach follows the outline in the email I
sent the other day:
https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg03786.html

The new Sphinx extension implements the hxtool-doc::
directive, which indicates where the assembled rST
document fragments should be inserted into the manual.
qemu-img-cmds.hx doesn't use the DEFHEADING or ARCHHEADING
directives, but the extension implements them (tested
with some local modifications to the .hx file to check
that they do the right thing).

As noted in the commit message for the qemu-img.texi conversion,
I have not attempted to tackle any of the muddle in the
current documentation structure or the repetition between
the manual document, the fragments in the .hx file and
the C code; this is a "simplest thing that works"
like-for-like conversion.

Another deliberate omission is that I have not attempted
to get links between our various Sphinx manuals (system,
interop, etc) working yet, as this is not totally trivial
and the odd minor missed hyperlink doesn't seem to me
to be a deal-breaker.

Sorry about the size of the main 'convert qemu-img'
patch, but it's unavoidable when converting a big
document between formats.

thanks
-- PMM

Based-on: 20200116141511.16849-1-peter.mayd...@linaro.org
("convert qemu-nbd, qemu-block-drivers to rST";
dependencies are mostly textual and in a few bits of
the makefile machinery)

Peter Maydell (5):
  hxtool: Support SRST/ERST directives
  docs/sphinx: Add new hxtool Sphinx extension
  qemu-img-cmds.hx: Add rST documentation fragments
  qemu-img: Convert invocation documentation to rST
  qemu-img-cmds.hx: Remove texinfo document fragments

 Makefile  |  19 +-
 MAINTAINERS   |   1 +
 docs/conf.py  |   3 +-
 docs/interop/conf.py  |   2 +
 docs/interop/index.rst|   1 +
 docs/interop/qemu-img.rst | 822 ++
 docs/sphinx/hxtool.py | 210 ++
 qemu-doc.texi |  10 +-
 qemu-img-cmds.hx  |  99 +++--
 qemu-img.texi | 795 
 scripts/hxtool|  33 +-
 11 files changed, 1128 insertions(+), 867 deletions(-)
 create mode 100644 docs/interop/qemu-img.rst
 create mode 100644 docs/sphinx/hxtool.py
 delete mode 100644 qemu-img.texi

-- 
2.20.1

[PATCH 1/5] hxtool: Support SRST/ERST directives

[Peter Maydell]

We want to add support for including rST document fragments
in our .hx files, in the same way we currently have texinfo
fragments. These will be delimited by SRST and ERST directives,
in the same way the texinfo is delimited by STEXI/ETEXI.
The rST fragments will not be extracted by the hxtool
script, but by a different mechanism, so all we need to
do in hxtool is have it ignore all the text inside a
SRST/ERST section, with suitable error-checking for
mismatched rST-vs-texi fragment delimiters.

The resulting effective state machine has only three states:
 * flag = 0, rstflag = 0 : reading section for C output
 * flag = 1, rstflag = 0 : reading texi fragment
 * flag = 0, rstflag = 1 : reading rST fragment
and flag = 1, rstflag = 1 is not possible. Using two
variables makes the parallel between the rST handling and
the texi handling clearer; in any case all this code will
be deleted once we've converted entirely to rST.

Signed-off-by: Peter Maydell 
---
 scripts/hxtool | 33 -
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/scripts/hxtool b/scripts/hxtool
index 7d7c4289e32..0003e7b673d 100644
--- a/scripts/hxtool
+++ b/scripts/hxtool
@@ -7,7 +7,7 @@ hxtoh()
 case $str in
 HXCOMM*)
 ;;
-STEXI*|ETEXI*) flag=$(($flag^1))
+STEXI*|ETEXI*|SRST*|ERST*) flag=$(($flag^1))
 ;;
 *)
 test $flag -eq 1 && printf "%s\n" "$str"
@@ -27,12 +27,17 @@ print_texi_heading()
 hxtotexi()
 {
 flag=0
+rstflag=0
 line=1
 while read -r str; do
 case "$str" in
 HXCOMM*)
 ;;
 STEXI*)
+if test $rstflag -eq 1 ; then
+printf "line %d: syntax error: expected ERST, found '%s'\n" 
"$line" "$str" >&2
+exit 1
+fi
 if test $flag -eq 1 ; then
 printf "line %d: syntax error: expected ETEXI, found '%s'\n" 
"$line" "$str" >&2
 exit 1
@@ -40,12 +45,38 @@ hxtotexi()
 flag=1
 ;;
 ETEXI*)
+if test $rstflag -eq 1 ; then
+printf "line %d: syntax error: expected ERST, found '%s'\n" 
"$line" "$str" >&2
+exit 1
+fi
 if test $flag -ne 1 ; then
 printf "line %d: syntax error: expected STEXI, found '%s'\n" 
"$line" "$str" >&2
 exit 1
 fi
 flag=0
 ;;
+SRST*)
+if test $rstflag -eq 1 ; then
+printf "line %d: syntax error: expected ERST, found '%s'\n" 
"$line" "$str" >&2
+exit 1
+fi
+if test $flag -eq 1 ; then
+printf "line %d: syntax error: expected ETEXI, found '%s'\n" 
"$line" "$str" >&2
+exit 1
+fi
+rstflag=1
+;;
+ERST*)
+if test $flag -eq 1 ; then
+printf "line %d: syntax error: expected ETEXI, found '%s'\n" 
"$line" "$str" >&2
+exit 1
+fi
+if test $rstflag -ne 1 ; then
+printf "line %d: syntax error: expected SRST, found '%s'\n" 
"$line" "$str" >&2
+exit 1
+fi
+rstflag=0
+;;
 DEFHEADING*)
 print_texi_heading "$(expr "$str" : "DEFHEADING(\(.*\))")"
 ;;
-- 
2.20.1

[PATCH 2/5] docs/sphinx: Add new hxtool Sphinx extension

[Peter Maydell]

Some of our documentation includes sections which are created
by assembling fragments of texinfo from a .hx source file into
a .texi file, which is then included from qemu-doc.texi or
qemu-img.texi.

For Sphinx, rather than creating a file to include, the most natural
way to handle this is to have a small custom Sphinx extension which
reads the .hx file and process it.  So instead of:
 * makefile produces foo.texi from foo.hx
 * qemu-doc.texi says '@include foo.texi'
we have:
 * qemu-doc.rst says 'hxtool-doc:: foo.hx'
 * the Sphinx extension for hxtool has code that runs to handle that
   Sphinx directive which reads the .hx file and emits the appropriate
   documentation contents

This is pretty much the same way the kerneldoc extension works right
now. It also has the advantage that it should work for third-party
services like readthedocs that expect to build the docs directly with
sphinx rather than by invoking our makefiles.

In this commit we implement the hxtool extension.

Note that syntax errors in the rST fragments will be correctly
reported to the user with the filename and line number within the
hx file.

Signed-off-by: Peter Maydell 
---
 docs/conf.py  |   3 +-
 docs/sphinx/hxtool.py | 210 ++
 2 files changed, 212 insertions(+), 1 deletion(-)
 create mode 100644 docs/sphinx/hxtool.py

diff --git a/docs/conf.py b/docs/conf.py
index 259c6049da7..ee7faa6b4e7 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -54,7 +54,7 @@ needs_sphinx = '1.3'
 # Add any Sphinx extension module names here, as strings. They can be
 # extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
 # ones.
-extensions = ['kerneldoc', 'qmp_lexer']
+extensions = ['kerneldoc', 'qmp_lexer', 'hxtool']
 
 # Add any paths that contain templates here, relative to this directory.
 templates_path = ['_templates']
@@ -221,3 +221,4 @@ texinfo_documents = [
 # find everything.
 kerneldoc_bin = os.path.join(qemu_docdir, '../scripts/kernel-doc')
 kerneldoc_srctree = os.path.join(qemu_docdir, '..')
+hxtool_srctree = os.path.join(qemu_docdir, '..')
diff --git a/docs/sphinx/hxtool.py b/docs/sphinx/hxtool.py
new file mode 100644
index 000..417cb10ece7
--- /dev/null
+++ b/docs/sphinx/hxtool.py
@@ -0,0 +1,210 @@
+# coding=utf-8
+#
+# QEMU hxtool .hx file parsing extension
+#
+# Copyright (c) 2020 Linaro
+#
+# This work is licensed under the terms of the GNU GPLv2 or later.
+# See the COPYING file in the top-level directory.
+"""hxtool is a Sphinx extension that implements the hxtool-doc directive"""
+
+# The purpose of this extension is to read fragments of rST
+# from .hx files, and insert them all into the current document.
+# The rST fragments are delimited by SRST/ERST lines.
+# The conf.py file must set the hxtool_srctree config value to
+# the root of the QEMU source tree.
+# Each hxtool-doc:: directive takes one argument which is the
+# path of the .hx file to process, relative to the source tree.
+
+import os
+import re
+from enum import Enum
+
+from docutils import nodes
+from docutils.statemachine import ViewList
+from docutils.parsers.rst import directives, Directive
+from sphinx.application import ExtensionError
+from sphinx.util.nodes import nested_parse_with_titles
+import sphinx
+
+# Sphinx up to 1.6 uses AutodocReporter; 1.7 and later
+# use switch_source_input. Check borrowed from kerneldoc.py.
+Use_SSI = sphinx.__version__[:3] >= '1.7'
+if Use_SSI:
+from sphinx.util.docutils import switch_source_input
+else:
+from sphinx.ext.autodoc import AutodocReporter
+
+__version__ = '1.0'
+
+# We parse hx files with a state machine which may be in one of three
+# states: reading the C code fragment, inside a texi fragment,
+# or inside a rST fragment.
+class HxState(Enum):
+CTEXT = 1
+TEXI = 2
+RST = 3
+
+def serror(file, lnum, errtext):
+"""Raise an exception giving a user-friendly syntax error message"""
+raise ExtensionError('%s line %d: syntax error: %s' % (file, lnum, 
errtext))
+
+def parse_directive(line):
+"""Return first word of line, if any"""
+return re.split('\W', line)[0]
+
+def parse_defheading(file, lnum, line):
+"""Handle a DEFHEADING directive"""
+# The input should be "DEFHEADING(some string)", though note that
+# the 'some string' could be the empty string. If the string is
+# empty we ignore the directive -- these are used only to add
+# blank lines in the plain-text content of the --help output.
+#
+# Return the heading text
+match = re.match(r'DEFHEADING\((.*)\)', line)
+if match is None:
+serror(file, lnum, "Invalid DEFHEADING line")
+return match.group(1)
+
+def parse_archheading(file, lnum, line):
+"""Handle an ARCHHEADING directive"""
+# The input should be "ARCHHEADING(some string, other arg)",
+# though note that the 'some string' could be the empty string.
+# As with DEFHEADING, empty string ARCHHEADINGs will be ignored.
+#
+# Return the 

[PATCH 5/5] qemu-img-cmds.hx: Remove texinfo document fragments

[Peter Maydell]

Now the qemu-img documentation has been converted to rST, we can
remove the texinfo document fragments from qemu-img-cmds.hx, as
they are no longer used.

Signed-off-by: Peter Maydell 
---
 qemu-img-cmds.hx | 56 +++-
 1 file changed, 3 insertions(+), 53 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 0c8b210b3c3..32e999d0965 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -1,143 +1,93 @@
 HXCOMM Keep the list of subcommands sorted by name.
 HXCOMM Use DEFHEADING() to define headings in both help text and texi
-HXCOMM Text between STEXI and ETEXI are copied to texi version and
+HXCOMM Text between SRST and ERST are copied to rST version and
 HXCOMM discarded from C version
 HXCOMM DEF(command, callback, arg_string) is used to construct
 HXCOMM command structures and help message.
-HXCOMM HXCOMM can be used for comments, discarded from both texi and C
+HXCOMM HXCOMM can be used for comments, discarded from both rST and C
 
-HXCOMM When amending the TEXI sections, please remember to copy the usage
+HXCOMM When amending the rST sections, please remember to copy the usage
 HXCOMM over to the per-command sections in qemu-img.texi.
 
-STEXI
-@table @option
-ETEXI
-
 DEF("amend", img_amend,
 "amend [--object objectdef] [--image-opts] [-p] [-q] [-f fmt] [-t cache] 
-o options filename")
-STEXI
-@item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}] 
[-t @var{cache}] -o @var{options} @var{filename}
-ETEXI
 SRST
 .. option:: amend [--object OBJECTDEF] [--image-opts] [-p] [-q] [-f FMT] [-t 
CACHE] -o OPTIONS FILENAME
 ERST
 
 DEF("bench", img_bench,
 "bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval] 
[-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S 
step_size] [-t cache] [-w] [-U] filename")
-STEXI
-@item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}] 
[--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}] 
[--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t 
@var{cache}] [-w] [-U] @var{filename}
-ETEXI
 SRST
 .. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] 
[--flush-interval=FLUSH_INTERVAL] [-n] [--no-drain] [-o OFFSET] 
[--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] 
FILENAME
 ERST
 DEF("check", img_check,
 "check [--object objectdef] [--image-opts] [-q] [-f fmt] [--output=ofmt] 
[-r [leaks | all]] [-T src_cache] [-U] filename")
-STEXI
-@item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] 
[--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] [-U] 
@var{filename}
-ETEXI
 SRST
 .. option:: check [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] 
[--output=OFMT] [-r [leaks | all]] [-T SRC_CACHE] [-U] FILENAME
 ERST
 
 DEF("commit", img_commit,
 "commit [--object objectdef] [--image-opts] [-q] [-f fmt] [-t cache] [-b 
base] [-d] [-p] filename")
-STEXI
-@item commit [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-t 
@var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
-ETEXI
 SRST
 .. option:: commit [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] [-t 
CACHE] [-b BASE] [-d] [-p] FILENAME
 ERST
 
 DEF("compare", img_compare,
 "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T 
src_cache] [-p] [-q] [-s] [-U] filename1 filename2")
-STEXI
-@item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F 
@var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] [-U] @var{filename1} 
@var{filename2}
-ETEXI
 SRST
 .. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] FILENAME1 FILENAME2
 ERST
 
 DEF("convert", img_convert,
 "convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] 
[-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B 
backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m 
num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
-STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] 
[-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T 
@var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o 
@var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m 
@var{num_coroutines}] [-W] [--salvage] @var{filename} [@var{filename2} [...]] 
@var{output_filename}
-ETEXI
 SRST
 .. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] 
[-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O 
OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] 
[-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
 ERST
 
 DEF("create", img_create,
 "create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F 
backing_fmt] [-u] [-o options] filename [size]")
-STEXI
-@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b 

[PATCH 3/5] qemu-img-cmds.hx: Add rST documentation fragments

[Peter Maydell]

Add the rST versions of the documentation fragments.
Once we've converted qemu-img.texi to rST we can delete
the texi fragments; for the moment we leave them in place.

(Commit created with the aid of emacs query-replace-regexp
from "@var{\([^}]*\)}" to "\,(upcase \1))".)

Signed-off-by: Peter Maydell 
---
 qemu-img-cmds.hx | 43 ++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1c93e6d1855..0c8b210b3c3 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -18,84 +18,125 @@ DEF("amend", img_amend,
 STEXI
 @item amend [--object @var{objectdef}] [--image-opts] [-p] [-q] [-f @var{fmt}] 
[-t @var{cache}] -o @var{options} @var{filename}
 ETEXI
+SRST
+.. option:: amend [--object OBJECTDEF] [--image-opts] [-p] [-q] [-f FMT] [-t 
CACHE] -o OPTIONS FILENAME
+ERST
 
 DEF("bench", img_bench,
 "bench [-c count] [-d depth] [-f fmt] [--flush-interval=flush_interval] 
[-n] [--no-drain] [-o offset] [--pattern=pattern] [-q] [-s buffer_size] [-S 
step_size] [-t cache] [-w] [-U] filename")
 STEXI
 @item bench [-c @var{count}] [-d @var{depth}] [-f @var{fmt}] 
[--flush-interval=@var{flush_interval}] [-n] [--no-drain] [-o @var{offset}] 
[--pattern=@var{pattern}] [-q] [-s @var{buffer_size}] [-S @var{step_size}] [-t 
@var{cache}] [-w] [-U] @var{filename}
 ETEXI
-
+SRST
+.. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] 
[--flush-interval=FLUSH_INTERVAL] [-n] [--no-drain] [-o OFFSET] 
[--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] 
FILENAME
+ERST
 DEF("check", img_check,
 "check [--object objectdef] [--image-opts] [-q] [-f fmt] [--output=ofmt] 
[-r [leaks | all]] [-T src_cache] [-U] filename")
 STEXI
 @item check [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] 
[--output=@var{ofmt}] [-r [leaks | all]] [-T @var{src_cache}] [-U] 
@var{filename}
 ETEXI
+SRST
+.. option:: check [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] 
[--output=OFMT] [-r [leaks | all]] [-T SRC_CACHE] [-U] FILENAME
+ERST
 
 DEF("commit", img_commit,
 "commit [--object objectdef] [--image-opts] [-q] [-f fmt] [-t cache] [-b 
base] [-d] [-p] filename")
 STEXI
 @item commit [--object @var{objectdef}] [--image-opts] [-q] [-f @var{fmt}] [-t 
@var{cache}] [-b @var{base}] [-d] [-p] @var{filename}
 ETEXI
+SRST
+.. option:: commit [--object OBJECTDEF] [--image-opts] [-q] [-f FMT] [-t 
CACHE] [-b BASE] [-d] [-p] FILENAME
+ERST
 
 DEF("compare", img_compare,
 "compare [--object objectdef] [--image-opts] [-f fmt] [-F fmt] [-T 
src_cache] [-p] [-q] [-s] [-U] filename1 filename2")
 STEXI
 @item compare [--object @var{objectdef}] [--image-opts] [-f @var{fmt}] [-F 
@var{fmt}] [-T @var{src_cache}] [-p] [-q] [-s] [-U] @var{filename1} 
@var{filename2}
 ETEXI
+SRST
+.. option:: compare [--object OBJECTDEF] [--image-opts] [-f FMT] [-F FMT] [-T 
SRC_CACHE] [-p] [-q] [-s] [-U] FILENAME1 FILENAME2
+ERST
 
 DEF("convert", img_convert,
 "convert [--object objectdef] [--image-opts] [--target-image-opts] [-U] 
[-C] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B 
backing_file] [-o options] [-l snapshot_param] [-S sparse_size] [-m 
num_coroutines] [-W] [--salvage] filename [filename2 [...]] output_filename")
 STEXI
 @item convert [--object @var{objectdef}] [--image-opts] [--target-image-opts] 
[-U] [-C] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T 
@var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o 
@var{options}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m 
@var{num_coroutines}] [-W] [--salvage] @var{filename} [@var{filename2} [...]] 
@var{output_filename}
 ETEXI
+SRST
+.. option:: convert [--object OBJECTDEF] [--image-opts] [--target-image-opts] 
[-U] [-C] [-c] [-p] [-q] [-n] [-f FMT] [-t CACHE] [-T SRC_CACHE] [-O 
OUTPUT_FMT] [-B BACKING_FILE] [-o OPTIONS] [-l SNAPSHOT_PARAM] [-S SPARSE_SIZE] 
[-m NUM_COROUTINES] [-W] [--salvage] FILENAME [FILENAME2 [...]] OUTPUT_FILENAME
+ERST
 
 DEF("create", img_create,
 "create [--object objectdef] [-q] [-f fmt] [-b backing_file] [-F 
backing_fmt] [-u] [-o options] filename [size]")
 STEXI
 @item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b 
@var{backing_file}] [-F @var{backing_fmt}] [-u] [-o @var{options}] 
@var{filename} [@var{size}]
 ETEXI
+SRST
+.. option:: create [--object OBJECTDEF] [-q] [-f FMT] [-b BACKING_FILE] [-F 
BACKING_FMT] [-u] [-o OPTIONS] FILENAME [SIZE]
+ERST
 
 DEF("dd", img_dd,
 "dd [--image-opts] [-U] [-f fmt] [-O output_fmt] [bs=block_size] 
[count=blocks] [skip=blocks] if=input of=output")
 STEXI
 @item dd [--image-opts] [-U] [-f @var{fmt}] [-O @var{output_fmt}] 
[bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} 
of=@var{output}
 ETEXI
+SRST
+.. option:: dd [--image-opts] [-U] [-f FMT] [-O OUTPUT_FMT] [bs=BLOCK_SIZE] 
[count=BLOCKS] [skip=BLOCKS] if=INPUT of=OUTPUT
+ERST
 
 DEF("info", img_info,
 "info [--object objectdef] [--image-opts] [-f fmt] 

[PATCH v4 2/4] vl: Initialise main loop earlier

[Kevin Wolf]

We want to be able to use qemu_aio_context in the monitor
initialisation.

Signed-off-by: Kevin Wolf 
Reviewed-by: Marc-André Lureau 
Reviewed-by: Markus Armbruster 
---
 vl.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/vl.c b/vl.c
index 751401214c..8064fef8b3 100644
--- a/vl.c
+++ b/vl.c
@@ -2909,6 +2909,11 @@ int main(int argc, char **argv, char **envp)
 runstate_init();
 precopy_infrastructure_init();
 postcopy_infrastructure_init();
+
+if (qemu_init_main_loop(_loop_err)) {
+error_report_err(main_loop_err);
+exit(1);
+}
 monitor_init_globals();
 
 if (qcrypto_init() < 0) {
@@ -3823,11 +3828,6 @@ int main(int argc, char **argv, char **envp)
 qemu_unlink_pidfile_notifier.notify = qemu_unlink_pidfile;
 qemu_add_exit_notifier(_unlink_pidfile_notifier);
 
-if (qemu_init_main_loop(_loop_err)) {
-error_report_err(main_loop_err);
-exit(1);
-}
-
 #ifdef CONFIG_SECCOMP
 olist = qemu_find_opts_err("sandbox", NULL);
 if (olist) {
-- 
2.20.1

[PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands

[Kevin Wolf]

This patch adds a new 'coroutine' flag to QMP command definitions that
tells the QMP dispatcher that the command handler is safe to be run in a
coroutine.

The documentation of the new flag pretends that this flag is already
used as intended, which it isn't yet after this patch. We'll implement
this in another patch in this series.

Signed-off-by: Kevin Wolf 
Reviewed-by: Marc-André Lureau 
---
 docs/devel/qapi-code-gen.txt| 10 ++
 include/qapi/qmp/dispatch.h |  1 +
 tests/test-qmp-cmds.c   |  4 
 scripts/qapi/commands.py| 10 +++---
 scripts/qapi/doc.py |  2 +-
 scripts/qapi/expr.py|  7 +--
 scripts/qapi/introspect.py  |  2 +-
 scripts/qapi/schema.py  |  9 ++---
 tests/qapi-schema/test-qapi.py  |  7 ---
 tests/Makefile.include  |  1 +
 tests/qapi-schema/oob-coroutine.err |  2 ++
 tests/qapi-schema/oob-coroutine.json|  2 ++
 tests/qapi-schema/oob-coroutine.out |  0
 tests/qapi-schema/qapi-schema-test.json |  1 +
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 15 files changed, 47 insertions(+), 13 deletions(-)
 create mode 100644 tests/qapi-schema/oob-coroutine.err
 create mode 100644 tests/qapi-schema/oob-coroutine.json
 create mode 100644 tests/qapi-schema/oob-coroutine.out

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 59d6973e1e..9b65cd3ab3 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -457,6 +457,7 @@ Syntax:
 '*gen': false,
 '*allow-oob': true,
 '*allow-preconfig': true,
+'*coroutine': true,
 '*if': COND,
 '*features': FEATURES }
 
@@ -581,6 +582,15 @@ before the machine is built.  It defaults to false.  For 
example:
 QMP is available before the machine is built only when QEMU was
 started with --preconfig.
 
+Member 'coroutine' tells the QMP dispatcher whether the command handler
+is safe to be run in a coroutine.  It defaults to false.  If it is true,
+the command handler is called from coroutine context and may yield while
+waiting for an external event (such as I/O completion) in order to avoid
+blocking the guest and other background operations.
+
+It is an error to specify both 'coroutine': true and 'allow-oob': true
+for a command.
+
 The optional 'if' member specifies a conditional.  See "Configuring
 the schema" below for more on this.
 
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 9aa426a398..d6ce9efc8e 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -24,6 +24,7 @@ typedef enum QmpCommandOptions
 QCO_NO_SUCCESS_RESP   =  (1U << 0),
 QCO_ALLOW_OOB =  (1U << 1),
 QCO_ALLOW_PRECONFIG   =  (1U << 2),
+QCO_COROUTINE =  (1U << 3),
 } QmpCommandOptions;
 
 typedef struct QmpCommand
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 79507d9e54..6359cc28c7 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -35,6 +35,10 @@ void qmp_cmd_success_response(Error **errp)
 {
 }
 
+void qmp_coroutine_cmd(Error **errp)
+{
+}
+
 Empty2 *qmp_user_def_cmd0(Error **errp)
 {
 return g_new0(Empty2, 1);
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index afa55b055c..f2f2f8948d 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -194,7 +194,8 @@ out:
 return ret
 
 
-def gen_register_command(name, success_response, allow_oob, allow_preconfig):
+def gen_register_command(name, success_response, allow_oob, allow_preconfig,
+ coroutine):
 options = []
 
 if not success_response:
@@ -203,6 +204,8 @@ def gen_register_command(name, success_response, allow_oob, 
allow_preconfig):
 options += ['QCO_ALLOW_OOB']
 if allow_preconfig:
 options += ['QCO_ALLOW_PRECONFIG']
+if coroutine:
+options += ['QCO_COROUTINE']
 
 if not options:
 options = ['QCO_NO_OPTIONS']
@@ -285,7 +288,7 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
 
 def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
   success_response, boxed, allow_oob, allow_preconfig,
-  features):
+  coroutine, features):
 if not gen:
 return
 # FIXME: If T is a user-defined type, the user is responsible
@@ -303,7 +306,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
 self._genh.add(gen_marshal_decl(name))
 self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
 self._regy.add(gen_register_command(name, success_response,
-allow_oob, allow_preconfig))
+allow_oob, allow_preconfig,
+

[PATCH v4 4/4] block: Mark 'block_resize' as coroutine

[Kevin Wolf]

block_resize is safe to run in a coroutine, and it does some I/O that
could potentially take quite some time, so use it as an example for the
new 'coroutine': true annotation in the QAPI schema.

Signed-off-by: Kevin Wolf 
Reviewed-by: Marc-André Lureau 
Reviewed-by: Markus Armbruster 
---
 qapi/block-core.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ff5e5edaf..1dbb2a9901 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1341,7 +1341,8 @@
 { 'command': 'block_resize',
   'data': { '*device': 'str',
 '*node-name': 'str',
-'size': 'int' } }
+'size': 'int' },
+  'coroutine': true }
 
 ##
 # @NewImageMode:
-- 
2.20.1

[PATCH v4 3/4] qmp: Move dispatcher to a coroutine

[Kevin Wolf]

This moves the QMP dispatcher to a coroutine and runs all QMP command
handlers that declare 'coroutine': true in coroutine context so they
can avoid blocking the main loop while doing I/O or waiting for other
events.

For commands that are not declared safe to run in a coroutine, the
dispatcher drops out of coroutine context by calling the QMP command
handler from a bottom half.

Signed-off-by: Kevin Wolf 
---
 include/qapi/qmp/dispatch.h |   1 +
 monitor/monitor-internal.h  |   6 +-
 monitor/monitor.c   |  33 ---
 monitor/qmp.c   | 110 ++--
 qapi/qmp-dispatch.c |  44 ++-
 qapi/qmp-registry.c |   3 +
 util/aio-posix.c|   7 ++-
 7 files changed, 162 insertions(+), 42 deletions(-)

diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index d6ce9efc8e..6812e49b5f 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -30,6 +30,7 @@ typedef enum QmpCommandOptions
 typedef struct QmpCommand
 {
 const char *name;
+/* Runs in coroutine context if QCO_COROUTINE is set */
 QmpCommandFunc *fn;
 QmpCommandOptions options;
 QTAILQ_ENTRY(QmpCommand) node;
diff --git a/monitor/monitor-internal.h b/monitor/monitor-internal.h
index d78f5ca190..f180d03368 100644
--- a/monitor/monitor-internal.h
+++ b/monitor/monitor-internal.h
@@ -154,7 +154,9 @@ static inline bool monitor_is_qmp(const Monitor *mon)
 
 typedef QTAILQ_HEAD(MonitorList, Monitor) MonitorList;
 extern IOThread *mon_iothread;
-extern QEMUBH *qmp_dispatcher_bh;
+extern Coroutine *qmp_dispatcher_co;
+extern bool qmp_dispatcher_co_shutdown;
+extern bool qmp_dispatcher_co_busy;
 extern QmpCommandList qmp_commands, qmp_cap_negotiation_commands;
 extern QemuMutex monitor_lock;
 extern MonitorList mon_list;
@@ -172,7 +174,7 @@ void monitor_fdsets_cleanup(void);
 
 void qmp_send_response(MonitorQMP *mon, const QDict *rsp);
 void monitor_data_destroy_qmp(MonitorQMP *mon);
-void monitor_qmp_bh_dispatcher(void *data);
+void coroutine_fn monitor_qmp_dispatcher_co(void *data);
 
 int get_monitor_def(int64_t *pval, const char *name);
 void help_cmd(Monitor *mon, const char *name);
diff --git a/monitor/monitor.c b/monitor/monitor.c
index 12898b6448..e753fa435d 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -53,8 +53,18 @@ typedef struct {
 /* Shared monitor I/O thread */
 IOThread *mon_iothread;
 
-/* Bottom half to dispatch the requests received from I/O thread */
-QEMUBH *qmp_dispatcher_bh;
+/* Coroutine to dispatch the requests received from I/O thread */
+Coroutine *qmp_dispatcher_co;
+
+/* Set to true when the dispatcher coroutine should terminate */
+bool qmp_dispatcher_co_shutdown;
+
+/*
+ * true if the coroutine is active and processing requests. The coroutine may
+ * only be woken up externally (e.g. from the monitor thread) after changing
+ * qmp_dispatcher_co_busy from false to true (e.g. using atomic_xchg).
+ */
+bool qmp_dispatcher_co_busy;
 
 /* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
 QemuMutex monitor_lock;
@@ -579,9 +589,16 @@ void monitor_cleanup(void)
 }
 qemu_mutex_unlock(_lock);
 
-/* QEMUBHs needs to be deleted before destroying the I/O thread */
-qemu_bh_delete(qmp_dispatcher_bh);
-qmp_dispatcher_bh = NULL;
+/* The dispatcher needs to stop before destroying the I/O thread */
+qmp_dispatcher_co_shutdown = true;
+if (!atomic_xchg(_dispatcher_co_busy, true)) {
+aio_co_wake(qmp_dispatcher_co);
+}
+
+AIO_WAIT_WHILE(qemu_get_aio_context(),
+   (aio_poll(iohandler_get_aio_context(), false),
+atomic_mb_read(_dispatcher_co_busy)));
+
 if (mon_iothread) {
 iothread_destroy(mon_iothread);
 mon_iothread = NULL;
@@ -604,9 +621,9 @@ void monitor_init_globals_core(void)
  * have commands assuming that context.  It would be nice to get
  * rid of those assumptions.
  */
-qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
-   monitor_qmp_bh_dispatcher,
-   NULL);
+qmp_dispatcher_co = qemu_coroutine_create(monitor_qmp_dispatcher_co, NULL);
+atomic_mb_set(_dispatcher_co_busy, true);
+aio_co_schedule(iohandler_get_aio_context(), qmp_dispatcher_co);
 }
 
 QemuOptsList qemu_mon_opts = {
diff --git a/monitor/qmp.c b/monitor/qmp.c
index 54c06ba824..9444de9fcf 100644
--- a/monitor/qmp.c
+++ b/monitor/qmp.c
@@ -133,6 +133,10 @@ static void monitor_qmp_respond(MonitorQMP *mon, QDict 
*rsp)
 }
 }
 
+/*
+ * Runs outside of coroutine context for OOB commands, but in coroutine context
+ * for everything else.
+ */
 static void monitor_qmp_dispatch(MonitorQMP *mon, QObject *req)
 {
 Monitor *old_mon;
@@ -211,43 +215,87 @@ static QMPRequest 
*monitor_qmp_requests_pop_any_with_lock(void)
 return req_obj;
 }
 
-void monitor_qmp_bh_dispatcher(void *data)
+void coroutine_fn 

[PATCH v4 0/4] qmp: Optionally run handlers in coroutines

[Kevin Wolf]

Some QMP command handlers can block the main loop for a relatively long
time, for example because they perform some I/O. This is quite nasty.
Allowing such handlers to run in a coroutine where they can yield (and
therefore release the BQL) while waiting for an event such as I/O
completion solves the problem.

This series adds the infrastructure to allow this and switches
block_resize to run in a coroutine as a first example.

This is an alternative solution to Marc-André's "monitor: add
asynchronous command type" series.

v4:
- Forbid 'oob': true, 'coroutine': true [Markus]
- Removed Python type hints [Markus]
- Introduced separate bool qmp_dispatcher_co_shutdown to make it clearer
  how a shutdown request is signalled to the dispatcher [Markus]
- Allow using aio_poll() with iohandler_ctx and use that instead of
  aio_bh_poll() [Markus]
- Removed coroutine_fn from qmp_block_resize() again because at least
  one caller (HMP) calls it outside of coroutine context [Markus]
- Tried to make the synchronisation between dispatcher and the monitor
  thread clearer, and fixed a race condition
- Improved documentation and comments

v3:
- Fix race between monitor thread and dispatcher that could schedule the
  dispatcher coroutine twice if a second requests comes in before the
  dispatcher can wake up [Patchew]

v2:
- Fix typo in a commit message [Eric]
- Use hyphen instead of underscore for the test command [Eric]
- Mark qmp_block_resize() as coroutine_fn [Stefan]

Kevin Wolf (4):
  qapi: Add a 'coroutine' flag for commands
  vl: Initialise main loop earlier
  qmp: Move dispatcher to a coroutine
  block: Mark 'block_resize' as coroutine

 qapi/block-core.json|   3 +-
 docs/devel/qapi-code-gen.txt|  10 +++
 include/qapi/qmp/dispatch.h |   2 +
 monitor/monitor-internal.h  |   6 +-
 monitor/monitor.c   |  33 +--
 monitor/qmp.c   | 110 +---
 qapi/qmp-dispatch.c |  44 +-
 qapi/qmp-registry.c |   3 +
 tests/test-qmp-cmds.c   |   4 +
 util/aio-posix.c|   7 +-
 vl.c|  10 +--
 scripts/qapi/commands.py|  10 ++-
 scripts/qapi/doc.py |   2 +-
 scripts/qapi/expr.py|   7 +-
 scripts/qapi/introspect.py  |   2 +-
 scripts/qapi/schema.py  |   9 +-
 tests/qapi-schema/test-qapi.py  |   7 +-
 tests/Makefile.include  |   1 +
 tests/qapi-schema/oob-coroutine.err |   2 +
 tests/qapi-schema/oob-coroutine.json|   2 +
 tests/qapi-schema/oob-coroutine.out |   0
 tests/qapi-schema/qapi-schema-test.json |   1 +
 tests/qapi-schema/qapi-schema-test.out  |   2 +
 23 files changed, 216 insertions(+), 61 deletions(-)
 create mode 100644 tests/qapi-schema/oob-coroutine.err
 create mode 100644 tests/qapi-schema/oob-coroutine.json
 create mode 100644 tests/qapi-schema/oob-coroutine.out

-- 
2.20.1

Re: [PATCH 2/3] docs: Create stub system manual

[Peter Maydell]

On Thu, 16 Jan 2020 at 14:15, Peter Maydell  wrote:
>
> We want a user-facing manual which contains system emulation
> documentation. Create an empty one which we can populate.
>
> Signed-off-by: Peter Maydell 
> ---
>  Makefile  | 10 +-
>  docs/system/conf.py   | 15 +++
>  docs/system/index.rst | 16 
>  3 files changed, 40 insertions(+), 1 deletion(-)
>  create mode 100644 docs/system/conf.py
>  create mode 100644 docs/system/index.rst

I think this may have crossed in the post with the
commit adding index.html.in. Anyway, here's the obvious
fixup, which I plan to squash into this patch without
doing a respin unless there's some other respin needed:

diff --git a/docs/index.html.in b/docs/index.html.in
index 94eb782cf7e..573c543c02b 100644
--- a/docs/index.html.in
+++ b/docs/index.html.in
@@ -11,6 +11,7 @@
 QMP Reference Manual
 Guest Agent Protocol
Reference
 System Emulation
Management and Interoperability Guide
+System Emulation User's
Guide
 System Emulation Guest
Hardware Specifications
 
 

thanks
-- PMM

Re: [PATCH v2 0/2] backup-top failure path fix

[Vladimir Sementsov-Ogievskiy]

21.01.2020 19:32, Max Reitz wrote:
> On 21.01.20 15:28, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> Here is a small crash fix.
> 
> Thanks, applied to my block branch:
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
> 

Thank you Max!


-- 
Best regards,
Vladimir

Re: [PATCH v2 0/2] backup-top failure path fix

[Max Reitz]

On 21.01.20 15:28, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> Here is a small crash fix.

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 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

On 21.01.20 15:28, Vladimir Sementsov-Ogievskiy wrote:
> This test checks that bug is really fixed by previous commit.
> 
> Cc: qemu-sta...@nongnu.org # v4.2.0
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/283 | 92 ++
>  tests/qemu-iotests/283.out |  8 
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 101 insertions(+)
>  create mode 100644 tests/qemu-iotests/283
>  create mode 100644 tests/qemu-iotests/283.out
> 
> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
> new file mode 100644
> index 00..293e557bd9
> --- /dev/null
> +++ b/tests/qemu-iotests/283
> @@ -0,0 +1,92 @@

[...]

> +""" Test description
> +
> +When performing a backup, all writes on the source subtree must go through 
> the
> +backup-top filter so it can copy all data to the target before it is changed.
> +backup-top filter is appended above source node, to achieve this thing, so 
> all
> +parents of source node are handled. A configuration with side parents of 
> source
> +sub-tree with write permission is unsupported (we'd have append several
> +backup-top filter like nodes to handle such parents). The test create an
> +example of such configuration and checks that a backup is then not allowed
> +(blockdev-backup command should fail).
> +
> +The configuration:
> +
> +┌┐  target  ┌─┐
> +│ target │ ◀─── │ backup_top  │
> +└┘  └─┘
> +│
> +│ backing
> +▼
> +┌─┐
> +│   source│
> +└─┘
> +│
> +│ file
> +▼
> +┌─┐  write perm   ┌───┐
> +│base │ ◀ │ other │
> +└─┘   └───┘
> +
> +On activation (see .active field of backup-top state in block/backup-top.c),
> +backup-top is going to unshare write permission on its source child. Write
> +unsharing will be propagated to the "source->base" link and will conflict 
> with
> +other node write permission. So permission update will fail and backup job 
> will
> +not be started.
> +
> +Note, that the only thing which prevents backup of running on such
> +configuration is default permission propagation scheme. It may be altered by
> +different block drivers, so backup will run in invalid configuration. But
> +something is better than nothing. Also, before the previous commit (commit
> +preceding this test creation), starting backup on such configuration led to
> +crash, so current "something" is a lot better, and this test actual goal is
> +to check that crash is fixed :)

Thanks a lot for bearing with me!

I was wondering whether this is the first smiley in our code, but it
isn’t.  (Not unfortunately, I think. :-))  It’s also not the first
smiley in the iotests, but the second one!  (As far as I can tell.)

Max



signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

On 20.01.20 20:42, Eric Blake wrote:
> On 1/20/20 11:13 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:
> 
> s/so for/so that for/
> 
>>
>> 1. Software may support some of these optional fields and ignore the
>>     others, which means that features may be backported to downstream
>>     Qemu independently.
>> 2. 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 | 44 +++---
>>   1 file changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index af5711e533..355925c35e 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, 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,44 @@ 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 and must be
>> a multiple
>> +    of 8.
>> +
>> +
>> +=== Additional fields (version 3 and higher) ===
>> +
>> +In general, these fields are optional and may be safely ignored by
>> the software,
>> +as well as filled by zeros (which is equal to field absence), if
>> software needs
> 
> We're inconsistent on 'zeros' (git grep has 201 hits) vs. 'zeroes' (688
> hits); I prefer the latter, but won't object if you don't tweak it since
> this is the first use of either spelling in qcow2.txt.
> 
>> +to set field B, but does not care about field A, which precedes B. More
> 
> s/A, which/A which/

I’ve heard before that one should always use a comma before “which” and
never before “that” (in that a subordinate clause opened by “that” is a
mandatory description, whereas those that start with “why” are not
necessary for understanding).

So if this is a mandatory description (which I suppose it is), shouldn’t
it also be s/which/that/?

I suppose “field A that precedes B” sounds a bit weird because “A”
hasn’t been introduced before.  That is, “the field that precedes B”
would sound more natural.  Or is that precisely the kind of instance
where one would use “which” without comma? :-)

All in all, I was wondering whether there isn’t a more natural way to
rephrase the whole paragraph.  (No, I don’t have an excuse why I didn’t
say so in the last revision.)

Maybe:

In general, these fields are optional and may be safely ignored when
read and filled with zeroes when written.  For example, say software
wants to set field B but does not care about its preceding field A.  It
may then set A to zero, B to its desired value, and adjust header_length
to include A and B.

?

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v10 2/2] docs: qcow2: introduce compression type feature

[Max Reitz]

On 20.01.20 20:46, Eric Blake wrote:
> On 1/20/20 11:13 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The patch add new additional field to qcow2 header: compression_type,
> 
> s/add/adds a/
> s/to/to the/
> 
>> 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.
> 
> It would be nice to have that patch as part of the same series, but it
> has already been posted to the list separately, so I'm okay with this
> series as just doc word-smithing while we get that patch series in soon.
> 
>>
>> Suggested-by: Denis Plotnikov 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   docs/interop/qcow2.txt | 16 +++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index 355925c35e..4569f0dba3 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -109,6 +109,11 @@ the next fields through header_length.
>>   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,
>> +    a non-default compression is used for
>> compressed
>> +    clusters. The compression_type field
>> must be
>> +    present and not zero.
> 
> Why must the compression_type field be non-zero?  If this bit is set, an
> older qemu cannot use the image, regardless of the contents of the
> compression_type field, so for maximum back-compat, a sane app will
> never set this bit when compression_type is zero.  But there is nothing
> technically wrong with setting this bit even when compression_type is 0,
> and newer qemu would still manage to use the image correctly with zlib
> compression if it were not for this arbitrary restriction.

There is something technically wrong if the specification demands otherwise.

As you yourself say, no sane software would deviate from this behavior
anyway.  If that is so, I don’t see why we shouldn’t specify it this
way.  I see no benefit in allowing this bit be set for zlib images.

Max



signature.asc
Description: OpenPGP digital signature

[PATCH 2/2] iotests: Test convert -n -B to backing-less target

[Max Reitz]

This must not crash.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/122 | 14 ++
 tests/qemu-iotests/122.out |  5 +
 2 files changed, 19 insertions(+)

diff --git a/tests/qemu-iotests/122 b/tests/qemu-iotests/122
index dfa350936f..f7a3ae684a 100755
--- a/tests/qemu-iotests/122
+++ b/tests/qemu-iotests/122
@@ -276,6 +276,20 @@ $QEMU_IMG convert -O $IMGFMT -n "$TEST_IMG" 
"$TEST_IMG".orig
 
 $QEMU_IMG compare "$TEST_IMG" "$TEST_IMG".orig
 
+echo
+echo '=== -n -B to an image without a backing file ==='
+echo
+
+# Base for the output
+TEST_IMG="$TEST_IMG".base _make_test_img 64M
+
+# Output that does have $TEST_IMG.base set as its (implicit) backing file
+TEST_IMG="$TEST_IMG".orig _make_test_img 64M
+
+# Convert with -n, which should not confuse -B with "target BDS has a
+# backing file"
+$QEMU_IMG convert -O $IMGFMT -B "$TEST_IMG".base -n "$TEST_IMG" 
"$TEST_IMG".orig
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 849b6cc2ef..1a35951a80 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -228,4 +228,9 @@ Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT 
size=67108864
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Images are identical.
+
+=== -n -B to an image without a backing file ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=67108864
 *** done
-- 
2.24.1

[PATCH 0/2] qemu-img: Fix convert -n -B for backing-less targets

[Max Reitz]

Hi,

When reviewing David’s series to add --target-is-zero convert, I looked
for a case to show that the current implementation will crash if
-n --target-is-zero is used together with -B.  It then turned out that
-B will always crash when combined with -n and the target image does not
have a backing file set in its image header.

This series fixes that.


Max Reitz (2):
  qemu-img: Fix convert -n -B for backing-less targets
  iotests: Test convert -n -B to backing-less target

 qemu-img.c |  2 +-
 tests/qemu-iotests/122 | 14 ++
 tests/qemu-iotests/122.out |  5 +
 3 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.24.1

[PATCH 1/2] qemu-img: Fix convert -n -B for backing-less targets

[Max Reitz]

s.target_has_backing does not reflect whether the target BDS has a
backing file; it only tells whether we should use a backing file during
conversion (specified by -B).

As such, if you use convert -n, the target does not necessarily actually
have a backing file, and then dereferencing out_bs->backing fails here.

When converting to an existing file, we should set
target_backing_sectors to a negative value, because first, as the
comment explains, this value is only used for optimization, so it is
always fine to do that.

Second, we use this value to determine where the target must be
initialized to zeroes (overlays are initialized to zero after the end of
their backing file).  When converting to an existing file, we cannot
assume that to be true.

Cc: qemu-sta...@nongnu.org
Fixes: 351c8efff9ad809c822d55620df54d575d536f68
   ("qemu-img: Special post-backing convert handling")
Signed-off-by: Max Reitz 
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6233b8ca56..89a1d11781 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2503,7 +2503,7 @@ static int img_convert(int argc, char **argv)
 }
 }
 
-if (s.target_has_backing) {
+if (s.target_has_backing && s.target_is_new) {
 /* Errors are treated as "backing length unknown" (which means
  * s.target_backing_sectors has to be negative, which it will
  * be automatically).  The backing file length is used only
-- 
2.24.1

Re: [PATCH] qemu-img: Add --target-is-zero to convert

[Max Reitz]

Hi,

On 17.01.20 11:34, David Edmondson wrote:
> In many cases the target of a convert operation is a newly provisioned
> target that the user knows is blank (filled with zeroes). In this
> situation there is no requirement for qemu-img to wastefully zero out
> the entire device.
> 
> Add a new option, --target-is-zero, allowing the user to indicate that
> an existing target device is already zero filled.
> ---
>  qemu-img.c | 19 ---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 95a24b9762..56ca727e8c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -70,6 +70,7 @@ enum {
>  OPTION_PREALLOCATION = 265,
>  OPTION_SHRINK = 266,
>  OPTION_SALVAGE = 267,
> +OPTION_TARGET_IS_ZERO = 268,
>  };
>  
>  typedef enum OutputFormat {
> @@ -1593,6 +1594,7 @@ typedef struct ImgConvertState {
>  bool copy_range;
>  bool salvage;
>  bool quiet;
> +bool target_is_zero;

As you already said, we probably don’t need this and it’d be sufficient
to set the has_zero_init value directly.

>  int min_sparse;
>  int alignment;
>  size_t cluster_sectors;
> @@ -1984,10 +1986,11 @@ static int convert_do_copy(ImgConvertState *s)
>  int64_t sector_num = 0;
>  
>  /* Check whether we have zero initialisation or can get it efficiently */
> -if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
> +s->has_zero_init = s->target_is_zero;

We cannot has_zero_init to true if the target has a backing file,
because convert_co_write() asserts that the target must not have a
backing file if has_zero_init is true.  (It’s impossible for a file to
be initialized to zeroes if it has a backing file; or at least it
doesn’t make sense then to have a backing file.)

Case in point:

$ ./qemu-img create -f qcow2 src.qcow2 64M
Formatting 'src.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 backing.qcow2 64M
Formatting 'backing.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
lazy_refcounts=off refcount_bits=16
$ ./qemu-img create -f qcow2 -b backing.qcow2 dst.qcow2 64M

Formatting 'dst.qcow2', fmt=qcow2 size=67108864
backing_file=backing.qcow2 cluster_size=65536 lazy_refcounts=off
refcount_bits=16
$ ./qemu-img convert -n -B backing.qcow2 -f qcow2 -O qcow2
--target-is-zero src.qcow2 dst.qcow2
qemu-img: qemu-img.c:1812: convert_co_write: Assertion
`!s->target_has_backing' failed.
[1]80813 abort (core dumped)  ./qemu-img convert -n -B backing.qcow2
-f qcow2 -O qcow2 --target-is-zero

> +
> +if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
> +!s->target_has_backing) {

(This will be irrelevant after target_has_backing is gone, but because
has_zero_init and target_has_backing are equivalent here, there is no
need to check both.)

>  s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
> -} else {
> -s->has_zero_init = false;
>  }
>  
>  if (!s->has_zero_init && !s->target_has_backing &&
> @@ -2076,6 +2079,7 @@ static int img_convert(int argc, char **argv)
>  .buf_sectors= IO_BUF_SIZE / BDRV_SECTOR_SIZE,
>  .wr_in_order= true,
>  .num_coroutines = 8,
> +.target_is_zero = false,
>  };
>  
>  for(;;) {
> @@ -2086,6 +2090,7 @@ static int img_convert(int argc, char **argv)
>  {"force-share", no_argument, 0, 'U'},
>  {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
>  {"salvage", no_argument, 0, OPTION_SALVAGE},
> +{"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
>  {0, 0, 0, 0}
>  };
>  c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
> @@ -2209,6 +2214,9 @@ static int img_convert(int argc, char **argv)
>  case OPTION_TARGET_IMAGE_OPTS:
>  tgt_image_opts = true;
>  break;
> +case OPTION_TARGET_IS_ZERO:
> +s.target_is_zero = true;
> +break;
>  }
>  }
>  
> @@ -2247,6 +2255,11 @@ static int img_convert(int argc, char **argv)
>  warn_report("This will become an error in future QEMU versions.");
>  }
>  
> +if (s.target_is_zero && !skip_create) {
> +error_report("--target-is-zero requires use of -n flag");

Hm, I could imagine it being useful even without -n, but maybe it’s
safer to forbid this case for now and reconsider if someone were to ask.

> +goto fail_getopt;
> +}
> +
>  s.src_num = argc - optind - 1;
>  out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;

This patch should also add some documentation for the new option (in
qemu-img-cmds.hx and in qemu-img.texi for the man page).

Max



signature.asc
Description: OpenPGP digital signature

[PATCH v2 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

This test checks that bug is really fixed by previous commit.

Cc: qemu-sta...@nongnu.org # v4.2.0
Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/283 | 92 ++
 tests/qemu-iotests/283.out |  8 
 tests/qemu-iotests/group   |  1 +
 3 files changed, 101 insertions(+)
 create mode 100644 tests/qemu-iotests/283
 create mode 100644 tests/qemu-iotests/283.out

diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
new file mode 100644
index 00..293e557bd9
--- /dev/null
+++ b/tests/qemu-iotests/283
@@ -0,0 +1,92 @@
+#!/usr/bin/env python
+#
+# Test for backup-top filter permission activation failure
+#
+# Copyright (c) 2019 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+
+# The test is unrelated to formats, restrict it to qcow2 to avoid extra runs
+iotests.verify_image_format(supported_fmts=['qcow2'])
+
+size = 1024 * 1024
+
+""" Test description
+
+When performing a backup, all writes on the source subtree must go through the
+backup-top filter so it can copy all data to the target before it is changed.
+backup-top filter is appended above source node, to achieve this thing, so all
+parents of source node are handled. A configuration with side parents of source
+sub-tree with write permission is unsupported (we'd have append several
+backup-top filter like nodes to handle such parents). The test create an
+example of such configuration and checks that a backup is then not allowed
+(blockdev-backup command should fail).
+
+The configuration:
+
+┌┐  target  ┌─┐
+│ target │ ◀─── │ backup_top  │
+└┘  └─┘
+│
+│ backing
+▼
+┌─┐
+│   source│
+└─┘
+│
+│ file
+▼
+┌─┐  write perm   ┌───┐
+│base │ ◀ │ other │
+└─┘   └───┘
+
+On activation (see .active field of backup-top state in block/backup-top.c),
+backup-top is going to unshare write permission on its source child. Write
+unsharing will be propagated to the "source->base" link and will conflict with
+other node write permission. So permission update will fail and backup job will
+not be started.
+
+Note, that the only thing which prevents backup of running on such
+configuration is default permission propagation scheme. It may be altered by
+different block drivers, so backup will run in invalid configuration. But
+something is better than nothing. Also, before the previous commit (commit
+preceding this test creation), starting backup on such configuration led to
+crash, so current "something" is a lot better, and this test actual goal is
+to check that crash is fixed :)
+"""
+
+vm = iotests.VM()
+vm.launch()
+
+vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
+
+vm.qmp_log('blockdev-add', **{
+'node-name': 'source',
+'driver': 'blkdebug',
+'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
+})
+
+vm.qmp_log('blockdev-add', **{
+'node-name': 'other',
+'driver': 'blkdebug',
+'image': 'base',
+'take-child-perms': ['write']
+})
+
+vm.qmp_log('blockdev-backup', sync='full', device='source', target='target')
+
+vm.shutdown()
diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
new file mode 100644
index 00..daaf5828c1
--- /dev/null
+++ b/tests/qemu-iotests/283.out
@@ -0,0 +1,8 @@
+{"execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": 
"target"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": 
{"driver": "null-co", "node-name": "base", "size": 1048576}, "node-name": 
"source"}}
+{"return": {}}
+{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": 
"base", "node-name": "other", "take-child-perms": ["write"]}}
+{"return": {}}
+{"execute": "blockdev-backup", "arguments": {"device": "source", "sync": 
"full", "target": "target"}}
+{"error": {"class": "GenericError", "desc": "Cannot set permissions for 
backup-top filter: 

Re: [PATCH 10/13] block: add generic infrastructure for x-blockdev-amend qmp command

[Maxim Levitsky]

On Tue, 2020-01-21 at 08:59 +0100, Markus Armbruster wrote:
> Maxim Levitsky  writes:
> 
> > blockdev-amend will be used similiar to blockdev-create
> > to allow on the fly changes of the structure of the format based block 
> > devices.
> > 
> > Current plan is to first support encryption keyslot management for luks
> > based formats (raw and embedded in qcow2)
> > 
> > Signed-off-by: Maxim Levitsky 
> 
> [...]
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 7ff5e5edaf..601f7dc9a4 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -4743,6 +4743,48 @@
> >'data': { 'job-id': 'str',
> >  'options': 'BlockdevCreateOptions' } }
> >  
> > +##
> > +# @BlockdevAmendOptions:
> > +#
> > +# Options for amending an image format
> > +#
> > +# @driver   block driver that is suitable for the image
> > +#
> > +# Since: 5.0
> > +##
> > +{ 'union': 'BlockdevAmendOptions',
> > +  'base': {
> > +  'driver': 'BlockdevDriver' },
> > +  'discriminator': 'driver',
> > +  'data': {
> > +  } }
> > +
> > +##
> > +# @x-blockdev-amend:
> > +#
> > +# Starts a job to amend format specific options of an existing open block 
> > device
> > +# The job is automatically finalized, but a manual job-dismiss is required.
> > +#
> > +# @job-id:  Identifier for the newly created job.
> > +#
> > +# @node-name:   Name of the block node to work on
> > +#
> > +# @options: Options (driver specific)
> > +#
> > +# @force:   Allow unsafe operations, format specific
> > +#   For luks that allows erase of the last active keyslot
> > +#   (permanent loss of data),
> > +#   and replacement of an active keyslot
> > +#   (possible loss of data if IO error happens)
> 
> PATCH 2 appears to reject that.  What am I missing?

this parameter overrides the safety checks for both operations.
It allows to erase all the keyslots (to allow to destroy the data
in unrecoverable way very fast), and it allows to overwrite an active
keyslot, which is not as dramatic, but in case of IO failure can
also result in bad things happening.

> 
> > +#
> > +# Since: 5.0
> > +##
> > +{ 'command': 'x-blockdev-amend',
> > +  'data': { 'job-id': 'str',
> > +'node-name': 'str',
> > +'options': 'BlockdevAmendOptions',
> > +'*force': 'bool' } }
> > +
> >  ##
> >  # @blockdev-open-tray:
> >  #
> > diff --git a/qapi/job.json b/qapi/job.json
> > index a121b615fb..362b634ec1 100644
> > --- a/qapi/job.json
> > +++ b/qapi/job.json
> > @@ -19,10 +19,12 @@
> >  #
> >  # @create: image creation job type, see "blockdev-create" (since 3.0)
> >  #
> > +# @amend: image options amend job type, see "x-blockdev-amend" (since 5.0)
> > +#
> >  # Since: 1.7
> >  ##
> >  { 'enum': 'JobType',
> > -  'data': ['commit', 'stream', 'mirror', 'backup', 'create'] }
> > +  'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
> >  
> >  ##
> >  # @JobStatus:


Best regards,
Maxim Levitsky

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

On 21.01.20 14:48, Vladimir Sementsov-Ogievskiy wrote:
> 21.01.2020 15:39, Max Reitz wrote:
>> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
>>> 21.01.2020 12:41, Max Reitz wrote:
 On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
> 21.01.2020 12:14, Max Reitz wrote:
>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>> 20.01.2020 20:04, Max Reitz wrote:
 On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
> This test checks that bug is really fixed by previous commit.
>
> Cc: qemu-sta...@nongnu.org # v4.2.0
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  tests/qemu-iotests/283 | 75 
> ++
>  tests/qemu-iotests/283.out |  8 
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 84 insertions(+)
>  create mode 100644 tests/qemu-iotests/283
>  create mode 100644 tests/qemu-iotests/283.out

 The test looks good to me, I just have a comment nit and a note on the
 fact that this should probably be queued only after Thomas’s “Enable
 more iotests during "make check-block"” series.

> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
> new file mode 100644
> index 00..f0f216d109
> --- /dev/null
> +++ b/tests/qemu-iotests/283
> @@ -0,0 +1,75 @@
> +#!/usr/bin/env python
> +#
> +# Test for backup-top filter permission activation failure
> +#
> +# Copyright (c) 2019 Virtuozzo International GmbH.
> +#
> +# This program is free software; you can redistribute it and/or 
> modify
> +# it under the terms of the GNU General Public License as published 
> by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see 
> .
> +#
> +
> +import iotests
> +
> +# The test is unrelated to formats, restrict it to qcow2 to avoid 
> extra runs
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +
> +size = 1024 * 1024
> +
> +"""
> +On activation, backup-top is going to unshare write permission on its
> +source child. It will be impossible for the following configuration:

 “The following configuration will become impossible”?
>>>
>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write 
>>> permission",
>>> is impossible with such configuration..
>>
>> But backup_top always unshares the write permission on the source.
>
> Yes, and I just try to say, that this action will fail. And the test 
> checks that it
> fails (and it crashes with current master instead of fail).

 OK.  So what I was trying to say is that the comment currently only
 states that this will fail.  I’d prefer it to also reassure me that it’s
 correct that this fails (because all writes on the backup source must go
 through backup_top), and that this is exactly what we want to test here.

 On first reading, I was wondering why exactly this comment would tell me
 all these things, because I didn’t know what the test wants to test in
 the first place.

 Max
>>>
>>> Hmm, something like:
>>>
>>> Backup wants to copy a point-in-time state of the source node. So, it 
>>> catches all writes
>>> to the source node by appending backup-top filter above it. So we handle 
>>> all changes which
>>> comes from source node parents. To prevent appearing of new writing parents 
>>> during the
>>> progress, backup-top unshares write permission on its source child. This 
>>> has additional
>>> implication: as this "unsharing" is propagated by default by backing/file 
>>> children,
>>> backup-top conflicts with any side parents of source sub-tree with write 
>>> permission.
>>> And this is in good relation with the general idea: with such parents we 
>>> can't guarantee
>>> point-in-time backup.
>>
>> Works for me (thanks :-)), but a shorter “When performing a backup, all
>> writes on the source subtree must go through the backup-top filter so it
>> can copy all data to the target before it is changed.  Therefore,
>> backup-top cannot allow other nodes to change data on its source child.”
>> would work for me just as well.

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 16:51, Max Reitz wrote:
> On 21.01.20 14:48, Vladimir Sementsov-Ogievskiy wrote:
>> 21.01.2020 15:39, Max Reitz wrote:
>>> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
 21.01.2020 12:41, Max Reitz wrote:
> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>> 21.01.2020 12:14, Max Reitz wrote:
>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
 20.01.2020 20:04, Max Reitz wrote:
> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>> This test checks that bug is really fixed by previous commit.
>>
>> Cc: qemu-sta...@nongnu.org # v4.2.0
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> 
>> ---
>>   tests/qemu-iotests/283 | 75 
>> ++
>>   tests/qemu-iotests/283.out |  8 
>>   tests/qemu-iotests/group   |  1 +
>>   3 files changed, 84 insertions(+)
>>   create mode 100644 tests/qemu-iotests/283
>>   create mode 100644 tests/qemu-iotests/283.out
>
> The test looks good to me, I just have a comment nit and a note on the
> fact that this should probably be queued only after Thomas’s “Enable
> more iotests during "make check-block"” series.
>
>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>> new file mode 100644
>> index 00..f0f216d109
>> --- /dev/null
>> +++ b/tests/qemu-iotests/283
>> @@ -0,0 +1,75 @@
>> +#!/usr/bin/env python
>> +#
>> +# Test for backup-top filter permission activation failure
>> +#
>> +# Copyright (c) 2019 Virtuozzo International GmbH.
>> +#
>> +# This program is free software; you can redistribute it and/or 
>> modify
>> +# it under the terms of the GNU General Public License as published 
>> by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see 
>> .
>> +#
>> +
>> +import iotests
>> +
>> +# The test is unrelated to formats, restrict it to qcow2 to avoid 
>> extra runs
>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>> +
>> +size = 1024 * 1024
>> +
>> +"""
>> +On activation, backup-top is going to unshare write permission on 
>> its
>> +source child. It will be impossible for the following configuration:
>
> “The following configuration will become impossible”?

 Hmm, no, the configuration is possible. But "it", i.e. "unshare write 
 permission",
 is impossible with such configuration..
>>>
>>> But backup_top always unshares the write permission on the source.
>>
>> Yes, and I just try to say, that this action will fail. And the test 
>> checks that it
>> fails (and it crashes with current master instead of fail).
>
> OK.  So what I was trying to say is that the comment currently only
> states that this will fail.  I’d prefer it to also reassure me that it’s
> correct that this fails (because all writes on the backup source must go
> through backup_top), and that this is exactly what we want to test here.
>
> On first reading, I was wondering why exactly this comment would tell me
> all these things, because I didn’t know what the test wants to test in
> the first place.
>
> Max

 Hmm, something like:

 Backup wants to copy a point-in-time state of the source node. So, it 
 catches all writes
 to the source node by appending backup-top filter above it. So we handle 
 all changes which
 comes from source node parents. To prevent appearing of new writing 
 parents during the
 progress, backup-top unshares write permission on its source child. This 
 has additional
 implication: as this "unsharing" is propagated by default by backing/file 
 children,
 backup-top conflicts with any side parents of source sub-tree with write 
 permission.
 And this is in good relation with the general idea: with such parents we 
 can't guarantee
 point-in-time backup.
>>>
>>> Works for me (thanks :-)), but a shorter “When performing a backup, all
>>> writes on the source subtree must go through the backup-top filter so it
>>> 

[PATCH] gitlab-ci: Refresh the list of iotests

[Thomas Huth]

iotest 147 and 205 have recently been marked as "NBD-only", so they
are currently simply skipped and thus can be removed.

iotest 129 occasionally fails in the gitlab-CI, and according to Max,
there are some known issues with this test (see for example this URL:
https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg00499.html ),
so for the time being, let's disable it until the problems are fixed.

The iotests 040, 127, 203 and 256 are scheduled to become part of "make
check-block", so we also do not have to test them seperately here anymore.

On the other side, new iotests have been added to the QEMU repository
in the past months, so we can now add some new test > 256 instead.

Signed-off-by: Thomas Huth 
---
 .gitlab-ci.yml | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index d9a90f795d..52d73c1c72 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -51,12 +51,12 @@ build-tcg-disabled:
  - make check-qapi-schema
  - cd tests/qemu-iotests/
  - ./check -raw 001 002 003 004 005 008 009 010 011 012 021 025 032 033 048
-052 063 077 086 101 104 106 113 147 148 150 151 152 157 159 160
-163 170 171 183 184 192 194 197 205 208 215 221 222 226 227 236
- - ./check -qcow2 028 040 051 056 057 058 065 067 068 082 085 091 095 096 102
-122 124 127 129 132 139 142 144 145 147 151 152 155 157 165 194
-196 197 200 202 203 205 208 209 215 216 218 222 227 234 246 247
-248 250 254 255 256
+052 063 077 086 101 104 106 113 148 150 151 152 157 159 160 163
+170 171 183 184 192 194 197 208 215 221 222 226 227 236 253 277
+ - ./check -qcow2 028 051 056 057 058 065 067 068 082 085 091 095 096 102 122
+124 132 139 142 144 145 151 152 155 157 165 194 196 197 200 202
+208 209 215 216 218 222 227 234 246 247 248 250 254 255 257 258
+260 261 262 263 264 270 272 273 277 279
 
 build-user:
  script:
-- 
2.18.1

[PATCH v2 1/2] block/backup-top: fix failure path

[Vladimir Sementsov-Ogievskiy]

We can't access top after call bdrv_backup_top_drop, as it is already
freed at this time.

Also, no needs to unref target child by hand, it will be unrefed on
bdrv_close() automatically.

So, just do bdrv_backup_top_drop if append succeed and one bdrv_unref
otherwise.

Note, that in !appended case bdrv_unref(top) moved into drained section
on source. It doesn't really matter, but just for code simplicity.

Fixes: 7df7868b96404
Cc: qemu-sta...@nongnu.org # v4.2.0
Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Max Reitz 
---
 block/backup-top.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/block/backup-top.c b/block/backup-top.c
index b8d863ff08..cdfec782e2 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -190,6 +190,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState 
*source,
 BlockDriverState *top = bdrv_new_open_driver(_backup_top_filter,
  filter_node_name,
  BDRV_O_RDWR, errp);
+bool appended = false;
 
 if (!top) {
 return NULL;
@@ -212,8 +213,9 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState 
*source,
 bdrv_append(top, source, _err);
 if (local_err) {
 error_prepend(_err, "Cannot append backup-top filter: ");
-goto append_failed;
+goto fail;
 }
+appended = true;
 
 /*
  * bdrv_append() finished successfully, now we can require permissions
@@ -224,14 +226,14 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState 
*source,
 if (local_err) {
 error_prepend(_err,
   "Cannot set permissions for backup-top filter: ");
-goto failed_after_append;
+goto fail;
 }
 
 state->bcs = block_copy_state_new(top->backing, state->target,
   cluster_size, write_flags, _err);
 if (local_err) {
 error_prepend(_err, "Cannot create block-copy-state: ");
-goto failed_after_append;
+goto fail;
 }
 *bcs = state->bcs;
 
@@ -239,14 +241,15 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState 
*source,
 
 return top;
 
-failed_after_append:
-state->active = false;
-bdrv_backup_top_drop(top);
+fail:
+if (appended) {
+state->active = false;
+bdrv_backup_top_drop(top);
+} else {
+bdrv_unref(top);
+}
 
-append_failed:
 bdrv_drained_end(source);
-bdrv_unref_child(top, state->target);
-bdrv_unref(top);
 error_propagate(errp, local_err);
 
 return NULL;
-- 
2.21.0

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 15:39, Max Reitz wrote:
> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
>> 21.01.2020 12:41, Max Reitz wrote:
>>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
 21.01.2020 12:14, Max Reitz wrote:
> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>> 20.01.2020 20:04, Max Reitz wrote:
>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
 This test checks that bug is really fixed by previous commit.

 Cc: qemu-sta...@nongnu.org # v4.2.0
 Signed-off-by: Vladimir Sementsov-Ogievskiy 
 ---
  tests/qemu-iotests/283 | 75 
 ++
  tests/qemu-iotests/283.out |  8 
  tests/qemu-iotests/group   |  1 +
  3 files changed, 84 insertions(+)
  create mode 100644 tests/qemu-iotests/283
  create mode 100644 tests/qemu-iotests/283.out
>>>
>>> The test looks good to me, I just have a comment nit and a note on the
>>> fact that this should probably be queued only after Thomas’s “Enable
>>> more iotests during "make check-block"” series.
>>>
 diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
 new file mode 100644
 index 00..f0f216d109
 --- /dev/null
 +++ b/tests/qemu-iotests/283
 @@ -0,0 +1,75 @@
 +#!/usr/bin/env python
 +#
 +# Test for backup-top filter permission activation failure
 +#
 +# Copyright (c) 2019 Virtuozzo International GmbH.
 +#
 +# This program is free software; you can redistribute it and/or modify
 +# it under the terms of the GNU General Public License as published by
 +# the Free Software Foundation; either version 2 of the License, or
 +# (at your option) any later version.
 +#
 +# This program is distributed in the hope that it will be useful,
 +# but WITHOUT ANY WARRANTY; without even the implied warranty of
 +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 +# GNU General Public License for more details.
 +#
 +# You should have received a copy of the GNU General Public License
 +# along with this program.  If not, see 
 .
 +#
 +
 +import iotests
 +
 +# The test is unrelated to formats, restrict it to qcow2 to avoid 
 extra runs
 +iotests.verify_image_format(supported_fmts=['qcow2'])
 +
 +size = 1024 * 1024
 +
 +"""
 +On activation, backup-top is going to unshare write permission on its
 +source child. It will be impossible for the following configuration:
>>>
>>> “The following configuration will become impossible”?
>>
>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write 
>> permission",
>> is impossible with such configuration..
>
> But backup_top always unshares the write permission on the source.

 Yes, and I just try to say, that this action will fail. And the test 
 checks that it
 fails (and it crashes with current master instead of fail).
>>>
>>> OK.  So what I was trying to say is that the comment currently only
>>> states that this will fail.  I’d prefer it to also reassure me that it’s
>>> correct that this fails (because all writes on the backup source must go
>>> through backup_top), and that this is exactly what we want to test here.
>>>
>>> On first reading, I was wondering why exactly this comment would tell me
>>> all these things, because I didn’t know what the test wants to test in
>>> the first place.
>>>
>>> Max
>>
>> Hmm, something like:
>>
>> Backup wants to copy a point-in-time state of the source node. So, it 
>> catches all writes
>> to the source node by appending backup-top filter above it. So we handle all 
>> changes which
>> comes from source node parents. To prevent appearing of new writing parents 
>> during the
>> progress, backup-top unshares write permission on its source child. This has 
>> additional
>> implication: as this "unsharing" is propagated by default by backing/file 
>> children,
>> backup-top conflicts with any side parents of source sub-tree with write 
>> permission.
>> And this is in good relation with the general idea: with such parents we 
>> can't guarantee
>> point-in-time backup.
> 
> Works for me (thanks :-)), but a shorter “When performing a backup, all
> writes on the source subtree must go through the backup-top filter so it
> can copy all data to the target before it is changed.  Therefore,
> backup-top cannot allow other nodes to change data on its source child.”
> would work for me just as well.

Hmm, I don't like this "Therefore". For me the last statement
"cannot allow" doesn't looks like a consequence of the first
"all writes must go through", it more like rephrasing (still

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 15:39, Max Reitz wrote:
> On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
>> 21.01.2020 12:41, Max Reitz wrote:
>>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
 21.01.2020 12:14, Max Reitz wrote:
> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>> 20.01.2020 20:04, Max Reitz wrote:
>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
 This test checks that bug is really fixed by previous commit.

 Cc: qemu-sta...@nongnu.org # v4.2.0
 Signed-off-by: Vladimir Sementsov-Ogievskiy 
 ---
  tests/qemu-iotests/283 | 75 
 ++
  tests/qemu-iotests/283.out |  8 
  tests/qemu-iotests/group   |  1 +
  3 files changed, 84 insertions(+)
  create mode 100644 tests/qemu-iotests/283
  create mode 100644 tests/qemu-iotests/283.out
>>>
>>> The test looks good to me, I just have a comment nit and a note on the
>>> fact that this should probably be queued only after Thomas’s “Enable
>>> more iotests during "make check-block"” series.
>>>
 diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
 new file mode 100644
 index 00..f0f216d109
 --- /dev/null
 +++ b/tests/qemu-iotests/283
 @@ -0,0 +1,75 @@
 +#!/usr/bin/env python
 +#
 +# Test for backup-top filter permission activation failure
 +#
 +# Copyright (c) 2019 Virtuozzo International GmbH.
 +#
 +# This program is free software; you can redistribute it and/or modify
 +# it under the terms of the GNU General Public License as published by
 +# the Free Software Foundation; either version 2 of the License, or
 +# (at your option) any later version.
 +#
 +# This program is distributed in the hope that it will be useful,
 +# but WITHOUT ANY WARRANTY; without even the implied warranty of
 +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 +# GNU General Public License for more details.
 +#
 +# You should have received a copy of the GNU General Public License
 +# along with this program.  If not, see 
 .
 +#
 +
 +import iotests
 +
 +# The test is unrelated to formats, restrict it to qcow2 to avoid 
 extra runs
 +iotests.verify_image_format(supported_fmts=['qcow2'])
 +
 +size = 1024 * 1024
 +
 +"""
 +On activation, backup-top is going to unshare write permission on its
 +source child. It will be impossible for the following configuration:
>>>
>>> “The following configuration will become impossible”?
>>
>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write 
>> permission",
>> is impossible with such configuration..
>
> But backup_top always unshares the write permission on the source.

 Yes, and I just try to say, that this action will fail. And the test 
 checks that it
 fails (and it crashes with current master instead of fail).
>>>
>>> OK.  So what I was trying to say is that the comment currently only
>>> states that this will fail.  I’d prefer it to also reassure me that it’s
>>> correct that this fails (because all writes on the backup source must go
>>> through backup_top), and that this is exactly what we want to test here.
>>>
>>> On first reading, I was wondering why exactly this comment would tell me
>>> all these things, because I didn’t know what the test wants to test in
>>> the first place.
>>>
>>> Max
>>
>> Hmm, something like:
>>
>> Backup wants to copy a point-in-time state of the source node. So, it 
>> catches all writes
>> to the source node by appending backup-top filter above it. So we handle all 
>> changes which
>> comes from source node parents. To prevent appearing of new writing parents 
>> during the
>> progress, backup-top unshares write permission on its source child. This has 
>> additional
>> implication: as this "unsharing" is propagated by default by backing/file 
>> children,
>> backup-top conflicts with any side parents of source sub-tree with write 
>> permission.
>> And this is in good relation with the general idea: with such parents we 
>> can't guarantee
>> point-in-time backup.
> 
> Works for me (thanks :-)), but a shorter “When performing a backup, all
> writes on the source subtree must go through the backup-top filter so it
> can copy all data to the target before it is changed.  Therefore,
> backup-top cannot allow other nodes to change data on its source child.”
> would work for me just as well.
> 
>> So, trying to backup the configuration with writing side parents of
>> source sub-tree nodes should fail. Let's test it.

But than, we need somehow link part about appending 

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

On 21.01.20 11:40, Vladimir Sementsov-Ogievskiy wrote:
> 21.01.2020 12:41, Max Reitz wrote:
>> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>>> 21.01.2020 12:14, Max Reitz wrote:
 On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
> 20.01.2020 20:04, Max Reitz wrote:
>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>> This test checks that bug is really fixed by previous commit.
>>>
>>> Cc: qemu-sta...@nongnu.org # v4.2.0
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>> tests/qemu-iotests/283 | 75 
>>> ++
>>> tests/qemu-iotests/283.out |  8 
>>> tests/qemu-iotests/group   |  1 +
>>> 3 files changed, 84 insertions(+)
>>> create mode 100644 tests/qemu-iotests/283
>>> create mode 100644 tests/qemu-iotests/283.out
>>
>> The test looks good to me, I just have a comment nit and a note on the
>> fact that this should probably be queued only after Thomas’s “Enable
>> more iotests during "make check-block"” series.
>>
>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>> new file mode 100644
>>> index 00..f0f216d109
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/283
>>> @@ -0,0 +1,75 @@
>>> +#!/usr/bin/env python
>>> +#
>>> +# Test for backup-top filter permission activation failure
>>> +#
>>> +# Copyright (c) 2019 Virtuozzo International GmbH.
>>> +#
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 2 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see .
>>> +#
>>> +
>>> +import iotests
>>> +
>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid 
>>> extra runs
>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>> +
>>> +size = 1024 * 1024
>>> +
>>> +"""
>>> +On activation, backup-top is going to unshare write permission on its
>>> +source child. It will be impossible for the following configuration:
>>
>> “The following configuration will become impossible”?
>
> Hmm, no, the configuration is possible. But "it", i.e. "unshare write 
> permission",
> is impossible with such configuration..

 But backup_top always unshares the write permission on the source.
>>>
>>> Yes, and I just try to say, that this action will fail. And the test checks 
>>> that it
>>> fails (and it crashes with current master instead of fail).
>>
>> OK.  So what I was trying to say is that the comment currently only
>> states that this will fail.  I’d prefer it to also reassure me that it’s
>> correct that this fails (because all writes on the backup source must go
>> through backup_top), and that this is exactly what we want to test here.
>>
>> On first reading, I was wondering why exactly this comment would tell me
>> all these things, because I didn’t know what the test wants to test in
>> the first place.
>>
>> Max
> 
> Hmm, something like:
> 
> Backup wants to copy a point-in-time state of the source node. So, it catches 
> all writes
> to the source node by appending backup-top filter above it. So we handle all 
> changes which
> comes from source node parents. To prevent appearing of new writing parents 
> during the
> progress, backup-top unshares write permission on its source child. This has 
> additional
> implication: as this "unsharing" is propagated by default by backing/file 
> children,
> backup-top conflicts with any side parents of source sub-tree with write 
> permission.
> And this is in good relation with the general idea: with such parents we 
> can't guarantee
> point-in-time backup.

Works for me (thanks :-)), but a shorter “When performing a backup, all
writes on the source subtree must go through the backup-top filter so it
can copy all data to the target before it is changed.  Therefore,
backup-top cannot allow other nodes to change data on its source child.”
would work for me just as well.

> So, trying to backup the configuration with writing side parents of
> source sub-tree nodes should fail. Let's test it.
Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 02/13] qcrypto-luks: implement encryption key management

[Maxim Levitsky]

On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote:



> > +##
> > +# @LUKSKeyslotUpdate:
> > +#
> > +# @keyslot: If specified, will update only keyslot with this index
> > +#
> > +# @old-secret:  If specified, will only update keyslots that
> > +#   can be opened with password which is contained in
> > +#   QCryptoSecret with @old-secret ID
> > +#
> > +#   If neither @keyslot nor @old-secret is specified,
> > +#   first empty keyslot is selected for the update
> > +#
> > +# @new-secret:  The ID of a QCryptoSecret object providing a new 
> > decryption
> > +#   key to place in all matching keyslots.
> > +#   null/empty string erases all matching keyslots
> 
> I hate making the empty string do something completely different than a
> non-empty string.
> 
> What about making @new-secret optional, and have absent @new-secret
> erase?

I don't remember already why I and Keven Wolf decided to do this this way, but 
I think that you are right here.
I don't mind personally to do this this way.
empty string though is my addition, since its not possible to pass null on 
command line.

> 
> > +#  unless
> > +#   last valid keyslot is erased.
> 
> Leaves me to wonder what happens when I try.  If I read your code
> correctly, it's an error.  Suggest "You cannot erase the last valid
> keyslot".
> 
> Not documented here: "Refusing to overwrite active slot".

In my current implementation, if all slots are selected for erase,
I just refuse the erase operation. In former versions of my patches,
I would instead erase all but the last one.
IMHO, its unlikely that more that one slot would have the same password,
thus anyway correct usage for replacing the password would be first add
a new slot, then erase all slots that match the old password.
If all slots are active and have the same password, then user still can
use 'force' option to overwrite one of them.

> 
> > +#
> > +# @iter-time:   number of milliseconds to spend in
> > +#   PBKDF passphrase processing
> 
> Default?
2000, as in QCryptoBlockCreateOptionsLUKS. I forgot to copy this here.

> 
> > +# Since: 5.0
> > +##
> > +{ 'struct': 'LUKSKeyslotUpdate',
> > +  'data': {
> > +   '*keyslot': 'int',
> > +   '*old-secret': 'str',
> > +   'new-secret' : 'StrOrNull',
> > +   '*iter-time' : 'int' } }
> > +
> > +
> > +##
> > +# @QCryptoBlockAmendOptionsLUKS:
> > +#
> > +# The options that can be changed on existing luks encrypted device
> > +# @keys:   list of keyslot updates to perform
> > +#  (updates are performed in order)
> > +# @unlock-secret:  use this secret to retrieve the current master key
> > +#  if not given will use the same secret as one
> 
> "as the one"?
Yea, this is wrong wording, I'll drop those words. Thanks.

> 
> > +#  that was used to open the image
> > +#
> > +# Since: 5.0
> > +##
> > +{ 'struct': 'QCryptoBlockAmendOptionsLUKS',
> > +  'data' : {
> > +'keys': ['LUKSKeyslotUpdate'],
> > + '*unlock-secret' : 'str' } }
> > +
> >   
> >   
> >   ##
> > @@ -324,4 +372,4 @@
> > 'base': 'QCryptoBlockOptionsBase',
> > 'discriminator': 'format',
> > 'data': {
> > -} }
> > +  'luks': 'QCryptoBlockAmendOptionsLUKS' } }


Thanks for review,
Best regards,
Maxim Levitsky

Re: [PATCH] qemu-img: Add --target-is-zero to convert

[David Edmondson]

On Monday, 2020-01-20 at 19:33:43 -05, John Snow wrote:

> CC qemu-block and block maintainers
>
> On 1/17/20 5:34 AM, David Edmondson wrote:
>> In many cases the target of a convert operation is a newly provisioned
>> target that the user knows is blank (filled with zeroes). In this
>> situation there is no requirement for qemu-img to wastefully zero out
>> the entire device.
>> 
>
> Is there no way to convince bdrv_has_zero_init to return what we want
> already in this case?

In the current HEAD code, bdrv_has_zero_init will never be called for
“convert -n” (skip target volume creation).

If -n is not specified the host_device block driver doesn't provide a
bdrv_has_zero_init function, so it's assumed not supported.

> I cannot recall off hand, but wonder if there's an advanced syntax
> method of specifying the target image that can set this flag already.

I couldn't see one, but I'd be happy to learn of its existence. My first
approach was to add a raw specific option and add it using
--target-image-opts, resulting in something like:

qemu-img convert -n --target-image-opts sparse.qcow2 \
driver=raw,file.filename=/dev/sdg,assume-blank=on

(assume-blank=on is the new bit). This worked, but is only useful for
raw targets.

The discussion here led me to switch to --target-is-zero.

Mark Kanda sent me some comments suggesting that I get rid of the new
target_is_zero boolean and simply set has_zero_init, which I will do
before sending out another patch if this overall approach is considered
appropriate.

>> Add a new option, --target-is-zero, allowing the user to indicate that
>> an existing target device is already zero filled.
>> ---
>>  qemu-img.c | 19 ---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>> 
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 95a24b9762..56ca727e8c 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -70,6 +70,7 @@ enum {
>>  OPTION_PREALLOCATION = 265,
>>  OPTION_SHRINK = 266,
>>  OPTION_SALVAGE = 267,
>> +OPTION_TARGET_IS_ZERO = 268,
>>  };
>>  
>>  typedef enum OutputFormat {
>> @@ -1593,6 +1594,7 @@ typedef struct ImgConvertState {
>>  bool copy_range;
>>  bool salvage;
>>  bool quiet;
>> +bool target_is_zero;
>>  int min_sparse;
>>  int alignment;
>>  size_t cluster_sectors;
>> @@ -1984,10 +1986,11 @@ static int convert_do_copy(ImgConvertState *s)
>>  int64_t sector_num = 0;
>>  
>>  /* Check whether we have zero initialisation or can get it efficiently 
>> */
>> -if (s->target_is_new && s->min_sparse && !s->target_has_backing) {
>> +s->has_zero_init = s->target_is_zero;
>> +
>> +if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
>> +!s->target_has_backing) {
>>  s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
>> -} else {
>> -s->has_zero_init = false;
>>  }
>>  
>>  if (!s->has_zero_init && !s->target_has_backing &&
>> @@ -2076,6 +2079,7 @@ static int img_convert(int argc, char **argv)
>>  .buf_sectors= IO_BUF_SIZE / BDRV_SECTOR_SIZE,
>>  .wr_in_order= true,
>>  .num_coroutines = 8,
>> +.target_is_zero = false,
>>  };
>>  
>>  for(;;) {
>> @@ -2086,6 +2090,7 @@ static int img_convert(int argc, char **argv)
>>  {"force-share", no_argument, 0, 'U'},
>>  {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
>>  {"salvage", no_argument, 0, OPTION_SALVAGE},
>> +{"target-is-zero", no_argument, 0, OPTION_TARGET_IS_ZERO},
>>  {0, 0, 0, 0}
>>  };
>>  c = getopt_long(argc, argv, ":hf:O:B:Cco:l:S:pt:T:qnm:WU",
>> @@ -2209,6 +2214,9 @@ static int img_convert(int argc, char **argv)
>>  case OPTION_TARGET_IMAGE_OPTS:
>>  tgt_image_opts = true;
>>  break;
>> +case OPTION_TARGET_IS_ZERO:
>> +s.target_is_zero = true;
>> +break;
>>  }
>>  }
>>  
>> @@ -2247,6 +2255,11 @@ static int img_convert(int argc, char **argv)
>>  warn_report("This will become an error in future QEMU versions.");
>>  }
>>  
>> +if (s.target_is_zero && !skip_create) {
>> +error_report("--target-is-zero requires use of -n flag");
>> +goto fail_getopt;
>> +}
>> +
>>  s.src_num = argc - optind - 1;
>>  out_filename = s.src_num >= 1 ? argv[argc - 1] : NULL;
>>  
>> 

dme.
-- 
Please forgive me if I act a little strange, for I know not what I do.

Re: [PATCH v3 0/5] Misc BDRV_SECTOR_SIZE updates

[Max Reitz]

On 18.01.20 20:09, Alberto Garcia wrote:
> This series gets rid of all the remaining instances of hardcoded
> sector sizes in the qcow2 code and adds a check for images whose
> virtual size is not a multiple of the sector size.
> 
> See the individual patches for details.

Thanks, applied to my block branch:

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

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 5/5] qcow2: Use BDRV_SECTOR_SIZE instead of the hardcoded value

[Max Reitz]

On 18.01.20 20:09, Alberto Garcia wrote:
> This replaces all remaining instances in the qcow2 code.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 4/5] qcow2: Don't require aligned offsets in qcow2_co_copy_range_from()

[Max Reitz]

On 18.01.20 20:09, Alberto Garcia wrote:
> qemu-img's convert_co_copy_range() operates at the sector level and
> block_copy() operates at the cluster level so this condition is always
> true, but it is not necessary to restrict this here, so let's leave it
> to the driver implementation return an error if there is any.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.c | 4 
>  1 file changed, 4 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 3/5] qcow2: Use bs->bl.request_alignment when updating an L1 entry

[Max Reitz]

On 18.01.20 20:09, Alberto Garcia wrote:
> When updating an L1 entry the qcow2 driver writes a (512-byte) sector
> worth of data to avoid a read-modify-write cycle. Instead of always
> writing 512 bytes we should follow the alignment requirements of the
> storage backend.
> 
> (the only exception is when the alignment is larger than the cluster
> size because then we could be overwriting data after the L1 table)
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2-cluster.c | 25 +++--
>  1 file changed, 15 insertions(+), 10 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v5 00/15] io_uring: add Linux io_uring AIO engine

[Stefan Hajnoczi]

On Mon, Jan 20, 2020 at 02:18:43PM +, Stefan Hajnoczi wrote:
> v5:
>  * Add back BDRV_O_IO_URING check that was dropped accidentally in v3
>[Kevin]
> 
> v4:
>  * Drop unnecessary changes in Patch 8 [Stefano]
> 
> v3:
>  * Reword BlockdevAioOptions QAPI schema commit description [Markus]
>  * Increase QAPI "Since: 4.2" to "Since: 5.0"
>  * Explain rationale for io_uring stubs in commit description [Kevin]
>  * Tried to use file.aio=io_uring instead of BDRV_O_IO_URING but it's really
>hard to make qemu-iotests work.  Tests build blkdebug: and other graphs so
>the syntax for io_uring is dependent on the test case.  I scrapped this
>approach and went back to a global flag.
> 
> v2:
>  * Drop fd registration because it breaks QEMU's file locking and will need to
>be resolved in a separate patch series
>  * Drop line-wrapping changes that accidentally broke several qemu-iotests
> 
> This patch series is based on Aarushi Mehta's v9 patch series written for
> Outreachy 2019:
> 
>   https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg00179.html
> 
> It adds a new AIO engine that uses the new Linux io_uring API.  This is the
> successor to Linux AIO with a number of improvements:
> 1. Both O_DIRECT and buffered I/O work
> 2. fdatasync(2) is supported (no need for a separate thread pool!)
> 3. True async behavior so the syscall doesn't block (Linux AIO got there to 
> some degree...)
> 4. Advanced performance optimizations are available (file registration, memory
>buffer registration, completion polling, submission polling).
> 
> Since Aarushi has been busy, I have taken up this patch series.  Booting a
> guest works with -drive aio=io_uring and -drive aio=io_uring,cache=none with a
> raw file on XFS.
> 
> I currently recommend using -drive aio=io_uring only with host block devices
> (like NVMe devices).  As of Linux v5.4-rc1 I still hit kernel bugs when using
> image files on ext4 or XFS.
> 
> Aarushi Mehta (15):
>   configure: permit use of io_uring
>   qapi/block-core: add option for io_uring
>   block/block: add BDRV flag for io_uring
>   block/io_uring: implements interfaces for io_uring
>   stubs: add stubs for io_uring interface
>   util/async: add aio interfaces for io_uring
>   blockdev: adds bdrv_parse_aio to use io_uring
>   block/file-posix.c: extend to use io_uring
>   block: add trace events for io_uring
>   block/io_uring: adds userspace completion polling
>   qemu-io: adds option to use aio engine
>   qemu-img: adds option to use aio engine for benchmarking
>   qemu-nbd: adds option for aio engines
>   tests/qemu-iotests: enable testing with aio options
>   tests/qemu-iotests: use AIOMODE with various tests
> 
>  MAINTAINERS   |   9 +
>  block.c   |  22 ++
>  block/Makefile.objs   |   3 +
>  block/file-posix.c|  98 ++--
>  block/io_uring.c  | 433 ++
>  block/trace-events|  12 +
>  blockdev.c|  12 +-
>  configure |  27 +++
>  include/block/aio.h   |  16 +-
>  include/block/block.h |   2 +
>  include/block/raw-aio.h   |  12 +
>  qapi/block-core.json  |   4 +-
>  qemu-img-cmds.hx  |   4 +-
>  qemu-img.c|  11 +-
>  qemu-img.texi |   5 +-
>  qemu-io.c |  25 +-
>  qemu-nbd.c|  12 +-
>  qemu-nbd.texi |   4 +-
>  stubs/Makefile.objs   |   1 +
>  stubs/io_uring.c  |  32 +++
>  tests/qemu-iotests/028|   2 +-
>  tests/qemu-iotests/058|   2 +-
>  tests/qemu-iotests/089|   4 +-
>  tests/qemu-iotests/091|   4 +-
>  tests/qemu-iotests/109|   2 +-
>  tests/qemu-iotests/147|   5 +-
>  tests/qemu-iotests/181|   8 +-
>  tests/qemu-iotests/183|   4 +-
>  tests/qemu-iotests/185|  10 +-
>  tests/qemu-iotests/200|   2 +-
>  tests/qemu-iotests/201|   8 +-
>  tests/qemu-iotests/check  |  15 +-
>  tests/qemu-iotests/common.rc  |  14 ++
>  tests/qemu-iotests/iotests.py |  12 +-
>  util/async.c  |  36 +++
>  35 files changed, 797 insertions(+), 75 deletions(-)
>  create mode 100644 block/io_uring.c
>  create mode 100644 stubs/io_uring.c
> 
> -- 
> 2.24.1
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan


signature.asc
Description: PGP signature

Re: [PATCH v4 00/15] io_uring: add Linux io_uring AIO engine

[Stefan Hajnoczi]

On Mon, Jan 20, 2020 at 10:35:33AM +, Stefan Hajnoczi wrote:
> On Tue, Jan 14, 2020 at 10:59:06AM +, Stefan Hajnoczi wrote:
> > v13:
> >  * Drop unnecessary changes in Patch 8 [Stefano]
> > 
> > v12:
> >  * Reword BlockdevAioOptions QAPI schema commit description [Markus]
> >  * Increase QAPI "Since: 4.2" to "Since: 5.0"
> >  * Explain rationale for io_uring stubs in commit description [Kevin]
> >  * Tried to use file.aio=io_uring instead of BDRV_O_IO_URING but it's really
> >hard to make qemu-iotests work.  Tests build blkdebug: and other graphs 
> > so
> >the syntax for io_uring is dependent on the test case.  I scrapped this
> >approach and went back to a global flag.
> > 
> > v11:
> >  * Drop fd registration because it breaks QEMU's file locking and will need 
> > to
> >be resolved in a separate patch series
> >  * Drop line-wrapping changes that accidentally broke several qemu-iotests
> > 
> > v10:
> >  * Dropped kernel submission queue polling, it requires root and has 
> > additional
> >limitations.  It should be benchmarked and considered for inclusion 
> > later,
> >maybe even together with kernel side changes.
> >  * Add io_uring_register_files() return value to trace_luring_fd_register()
> >  * Fix indentation in luring_fd_unregister()
> >  * Set s->fd_reg.fd_array to NULL after g_free() to avoid dangling pointers
> >  * Simplify fd registration code
> >  * Add luring_fd_unregister() and call it from file-posix.c to prevent
> >fd leaks
> >  * Add trace_luring_fd_unregister() trace event
> >  * Add missing space to qemu-img command-line documentation
> >  * Update MAINTAINERS file [Julia]
> >  * Rename MAX_EVENTS to MAX_ENTRIES [Julia]
> >  * Define ioq_submit() before callers so the prototype isn't necessary 
> > [Julia]
> >  * Declare variables at the beginning of the block in luring_init() [Julia]
> > 
> > This patch series is based on Aarushi Mehta's v9 patch series written for
> > Google Summer of Code 2019:
> > 
> >   https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg00179.html
> > 
> > It adds a new AIO engine that uses the new Linux io_uring API.  This is the
> > successor to Linux AIO with a number of improvements:
> > 1. Both O_DIRECT and buffered I/O work
> > 2. fdatasync(2) is supported (no need for a separate thread pool!)
> > 3. True async behavior so the syscall doesn't block (Linux AIO got there to 
> > some degree...)
> > 4. Advanced performance optimizations are available (file registration, 
> > memory
> >buffer registration, completion polling, submission polling).
> > 
> > Since Aarushi has been busy, I have taken up this patch series.  Booting a
> > guest works with -drive aio=io_uring and -drive aio=io_uring,cache=none 
> > with a
> > raw file on XFS.
> > 
> > I currently recommend using -drive aio=io_uring only with host block devices
> > (like NVMe devices).  As of Linux v5.4-rc1 I still hit kernel bugs when 
> > using
> > image files on ext4 or XFS.
> > 
> > Aarushi Mehta (15):
> >   configure: permit use of io_uring
> >   qapi/block-core: add option for io_uring
> >   block/block: add BDRV flag for io_uring
> >   block/io_uring: implements interfaces for io_uring
> >   stubs: add stubs for io_uring interface
> >   util/async: add aio interfaces for io_uring
> >   blockdev: adds bdrv_parse_aio to use io_uring
> >   block/file-posix.c: extend to use io_uring
> >   block: add trace events for io_uring
> >   block/io_uring: adds userspace completion polling
> >   qemu-io: adds option to use aio engine
> >   qemu-img: adds option to use aio engine for benchmarking
> >   qemu-nbd: adds option for aio engines
> >   tests/qemu-iotests: enable testing with aio options
> >   tests/qemu-iotests: use AIOMODE with various tests
> > 
> >  MAINTAINERS   |   9 +
> >  block.c   |  22 ++
> >  block/Makefile.objs   |   3 +
> >  block/file-posix.c|  85 +--
> >  block/io_uring.c  | 433 ++
> >  block/trace-events|  12 +
> >  blockdev.c|  12 +-
> >  configure |  27 +++
> >  include/block/aio.h   |  16 +-
> >  include/block/block.h |   2 +
> >  include/block/raw-aio.h   |  12 +
> >  qapi/block-core.json  |   4 +-
> >  qemu-img-cmds.hx  |   4 +-
> >  qemu-img.c|  11 +-
> >  qemu-img.texi |   5 +-
> >  qemu-io.c |  25 +-
> >  qemu-nbd.c|  12 +-
> >  qemu-nbd.texi |   4 +-
> >  stubs/Makefile.objs   |   1 +
> >  stubs/io_uring.c  |  32 +++
> >  tests/qemu-iotests/028|   2 +-
> >  tests/qemu-iotests/058|   2 +-
> >  tests/qemu-iotests/089|   4 +-
> >  tests/qemu-iotests/091|   4 +-
> >  tests/qemu-iotests/109|   2 +-
> >  tests/qemu-iotests/147|   5 +-
> >  tests/qemu-iotests/181|   8 +-
> >  

Re: [PATCH 0/6] Fix more GCC9 -O3 warnings

[Paolo Bonzini]

On 20/01/20 20:02, Philippe Mathieu-Daudé wrote:
> Hi Paolo,
> 
> On 1/18/20 8:24 PM, Paolo Bonzini wrote:
>> On 18/12/19 07:05, Markus Armbruster wrote:
>>> "Chubb, Peter (Data61, Kensington NSW)" 
>>> writes:
>>>
> "Philippe" == Philippe Mathieu-Daudé  writes:

 Philippe> Fix some trivial warnings when building with -O3.

 For compatibility with lint and other older checkers, it'd be good
 to keep
 this as /* FALLTHROUGH */ (which gcc should accept according to its
 manual).
>>>
>>> We have hundreds of /* fall through */ comments already.
>>>
 Fixing the comments' placement is a different matter, and should be
 done.  Seems to me that until gcc started warning for this, noone had
 actually run a checker, and the comments were just for human info.

 Peter C
>>>
>>
>> Queued, thanks.
> 
> Thanks, but I sent a v2 (20191218192526.13845-1-phi...@redhat.com) with:
> 
> - addressed Thomas and Aleksandar comments
> - dropped 'hw/scsi/megasas: Silent GCC9 duplicated-cond warning'
> - dropped 'qemu-io-cmds: Silent GCC9 format-overflow warning'
> 
> See:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg666280.html
> 
> Can you queue it instead?

Yes, I replaced it.

Paolo

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 12:41, Max Reitz wrote:
> On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
>> 21.01.2020 12:14, Max Reitz wrote:
>>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
 20.01.2020 20:04, Max Reitz wrote:
> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>> This test checks that bug is really fixed by previous commit.
>>
>> Cc: qemu-sta...@nongnu.org # v4.2.0
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>> tests/qemu-iotests/283 | 75 
>> ++
>> tests/qemu-iotests/283.out |  8 
>> tests/qemu-iotests/group   |  1 +
>> 3 files changed, 84 insertions(+)
>> create mode 100644 tests/qemu-iotests/283
>> create mode 100644 tests/qemu-iotests/283.out
>
> The test looks good to me, I just have a comment nit and a note on the
> fact that this should probably be queued only after Thomas’s “Enable
> more iotests during "make check-block"” series.
>
>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>> new file mode 100644
>> index 00..f0f216d109
>> --- /dev/null
>> +++ b/tests/qemu-iotests/283
>> @@ -0,0 +1,75 @@
>> +#!/usr/bin/env python
>> +#
>> +# Test for backup-top filter permission activation failure
>> +#
>> +# Copyright (c) 2019 Virtuozzo International GmbH.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see .
>> +#
>> +
>> +import iotests
>> +
>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra 
>> runs
>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>> +
>> +size = 1024 * 1024
>> +
>> +"""
>> +On activation, backup-top is going to unshare write permission on its
>> +source child. It will be impossible for the following configuration:
>
> “The following configuration will become impossible”?

 Hmm, no, the configuration is possible. But "it", i.e. "unshare write 
 permission",
 is impossible with such configuration..
>>>
>>> But backup_top always unshares the write permission on the source.
>>
>> Yes, and I just try to say, that this action will fail. And the test checks 
>> that it
>> fails (and it crashes with current master instead of fail).
> 
> OK.  So what I was trying to say is that the comment currently only
> states that this will fail.  I’d prefer it to also reassure me that it’s
> correct that this fails (because all writes on the backup source must go
> through backup_top), and that this is exactly what we want to test here.
> 
> On first reading, I was wondering why exactly this comment would tell me
> all these things, because I didn’t know what the test wants to test in
> the first place.
> 
> Max

Hmm, something like:

Backup wants to copy a point-in-time state of the source node. So, it catches 
all writes
to the source node by appending backup-top filter above it. So we handle all 
changes which
comes from source node parents. To prevent appearing of new writing parents 
during the
progress, backup-top unshares write permission on its source child. This has 
additional
implication: as this "unsharing" is propagated by default by backing/file 
children,
backup-top conflicts with any side parents of source sub-tree with write 
permission.
And this is in good relation with the general idea: with such parents we can't 
guarantee
point-in-time backup. So, trying to backup the configuration with writing side 
parents of
source sub-tree nodes should fail. Let's test it.

> 
> I think there should be some note that this is exactly what we want to
> test, i.e. what happens when this impossible configuration is attempted
> by starting a backup.  (And maybe why this isn’t allowed; namely because
> we couldn’t do CBW for such write accesses.)
>
>> +
>> +┌┐  target  ┌─┐
>> +│ target │ ◀─── │ backup_top  │
>> +└┘  └─┘
>> +│
>> +│ backing
>> +▼
>> +┌─┐
>> +│   source│
>> +└─┘
>> 

Re: [PATCH v5 0/6] Enable more iotests during "make check-block"

[Max Reitz]

On 21.01.20 10:51, Thomas Huth wrote:
> As discussed here:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg00697.html
> 
> and here:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01388.html
> 
> it would be good to have some more valuable iotests enabled in the
> "auto" group to get better iotest coverage during "make check".
> 
> Since these Python-based tests require a QEMU that features a 'virtio-blk'
> device, we can only run the Python tests if this device is available. With
> binaries like qemu-system-tricore, the Python-based tests will be skipped.

Thanks, applied to my block branch:

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

Max



signature.asc
Description: OpenPGP digital signature

[PATCH v5 5/6] iotests: Skip Python-based tests if QEMU does not support virtio-blk

[Thomas Huth]

We are going to enable some of the python-based tests in 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 a central spot in the "check" script instead (so that it is still
possible to run "make check" for qemu-system-tricore for example).

Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/check | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index 2890785a10..1629b6c914 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -642,7 +642,15 @@ fi
 python_usable=false
 if $PYTHON -c 'import sys; sys.exit(0 if sys.version_info >= (3,6) else 1)'
 then
-python_usable=true
+# Our python framework also requires virtio-blk
+if "$QEMU_PROG" -M none -device help | grep -q virtio-blk >/dev/null 2>&1
+then
+python_usable=true
+else
+python_unusable_because="Missing virtio-blk in QEMU binary"
+fi
+else
+python_unusable_because="Unsupported Python version"
 fi
 
 default_machine=$($QEMU_PROG -machine help | sed -n '/(default)/ s/ .*//p')
@@ -830,7 +838,7 @@ do
 run_command="$PYTHON $seq"
 else
 run_command="false"
-echo "Unsupported Python version" > $seq.notrun
+echo "$python_unusable_because" > $seq.notrun
 fi
 else
 run_command="./$seq"
-- 
2.18.1

[PATCH v5 4/6] iotests: Check for the availability of the required devices in 267 and 127

[Thomas Huth]

We are going to enable 127 in the "auto" group, but it only works if
virtio-scsi and scsi-hd are available - which is not the case with
QEMU binaries like qemu-system-tricore for example, so we need a
proper check for the availability of these devices here.

A very similar problem exists in iotest 267 - it has been added to
the "auto" group already, but requires virtio-blk and thus currently
fails with qemu-system-tricore for example. Let's also add aproper
check there.

Reviewed-by: Max Reitz 
Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/127   |  2 ++
 tests/qemu-iotests/267   |  2 ++
 tests/qemu-iotests/common.rc | 14 ++
 3 files changed, 18 insertions(+)

diff --git a/tests/qemu-iotests/127 b/tests/qemu-iotests/127
index b64926ab31..a4fc866038 100755
--- a/tests/qemu-iotests/127
+++ b/tests/qemu-iotests/127
@@ -43,6 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 
+_require_devices virtio-scsi scsi-hd
+
 IMG_SIZE=64K
 
 _make_test_img $IMG_SIZE
diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
index c296877168..3146273eef 100755
--- a/tests/qemu-iotests/267
+++ b/tests/qemu-iotests/267
@@ -46,6 +46,8 @@ _require_drivers copy-on-read
 # and generally impossible with external data files
 _unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
+_require_devices virtio-blk
+
 do_run_qemu()
 {
 echo Testing: "$@"
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index d088392ab6..4dfe92e6e3 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -699,5 +699,19 @@ _require_large_file()
 rm "$TEST_IMG"
 }
 
+# Check that a set of devices is available in the QEMU binary
+#
+_require_devices()
+{
+available=$($QEMU -M none -device help | \
+grep ^name | sed -e 's/^name "//' -e 's/".*$//')
+for device
+do
+if ! echo "$available" | grep -q "$device" ; then
+_notrun "$device not available"
+fi
+done
+}
+
 # make sure this script returns success
 true
-- 
2.18.1

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

[Thomas Huth]

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 181 and 203 (which also tests iothreads).
(091 would be a good candidate for migration, too, but Alex Bennée
reported that this test fails on ZFS file systems, so it can't be
included yet)

Reviewed-by: Max Reitz 
Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/group | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index cb2b789e44..15a005b467 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -51,7 +51,7 @@
 027 rw auto quick
 028 rw backing quick
 029 rw auto quick
-030 rw backing
+030 rw auto backing
 031 rw auto quick
 032 rw auto quick
 033 rw auto quick
@@ -61,8 +61,8 @@
 037 rw auto backing quick
 038 rw auto backing quick
 039 rw auto quick
-040 rw
-041 rw backing
+040 rw auto
+041 rw auto backing
 042 rw auto quick
 043 rw auto backing
 044 rw
@@ -148,7 +148,7 @@
 124 rw backing
 125 rw
 126 rw auto backing
-127 rw backing quick
+127 rw auto backing quick
 128 rw quick
 129 rw quick
 130 rw quick
@@ -197,7 +197,7 @@
 177 rw auto quick
 178 img
 179 rw auto quick
-181 rw migration
+181 rw auto migration
 182 rw quick
 183 rw migration
 184 rw auto quick
@@ -218,7 +218,7 @@
 200 rw
 201 rw migration
 202 rw quick
-203 rw migration
+203 rw auto migration
 204 rw quick
 205 rw quick
 206 rw
@@ -270,7 +270,7 @@
 253 rw quick
 254 rw backing quick
 255 rw quick
-256 rw quick
+256 rw auto quick
 257 rw
 258 rw quick
 260 rw quick
-- 
2.18.1

[PATCH v5 2/6] iotests: Test 041 only works on certain systems

[Thomas Huth]

041 works fine on Linux, FreeBSD, NetBSD and OpenBSD, but fails on macOS.
Let's mark it as only supported on the systems where we know that it is
working fine.

Reviewed-by: Max Reitz 
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 c07437fda1..0181f7a9b6 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1134,4 +1134,5 @@ class TestOrphanedSource(iotests.QMPTestCase):
 
 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2', 'qed'],
- supported_protocols=['file'])
+ supported_protocols=['file'],
+ supported_platforms=['linux', 'freebsd', 'netbsd', 'openbsd'])
-- 
2.18.1

[PATCH v5 3/6] iotests: Test 183 does not work on macOS and OpenBSD

[Thomas Huth]

In the long run, we might want to add test 183 to the "auto" group
(but it still fails occasionally, so we cannot do that yet). However,
when running 183 in Cirrus-CI on macOS, or with our vm-build-openbsd
target, it currently always fails with an "Timeout waiting for return
on handle 0" error.

Let's mark it as supported only on systems where the test is working
most of the time (i.e. Linux, FreeBSD and NetBSD).

Reviewed-by: Max Reitz 
Signed-off-by: Thomas Huth 
---
 tests/qemu-iotests/183 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
index 3f74b9f62d..a5e0d7e8b7 100755
--- a/tests/qemu-iotests/183
+++ b/tests/qemu-iotests/183
@@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 . ./common.qemu
 
+_supported_os Linux FreeBSD NetBSD
 _supported_fmt qcow2 raw qed quorum
 _supported_proto file
 
-- 
2.18.1

[PATCH v5 1/6] iotests: remove 'linux' from default supported platforms

[Thomas Huth]

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.

Reviewed-by: Max Reitz 
Signed-off-by: John Snow 
---
 tests/qemu-iotests/iotests.py | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 13fd8b5cd2..1f6d918d6d 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -925,9 +925,14 @@ def verify_protocol(supported=[], unsupported=[]):
 if not_sup or (imgproto in unsupported):
 notrun('not suitable for this protocol: %s' % imgproto)
 
-def verify_platform(supported_oses=['linux']):
-if True not in [sys.platform.startswith(x) for x in supported_oses]:
-notrun('not suitable for this OS: %s' % sys.platform)
+def verify_platform(supported=None, unsupported=None):
+if unsupported is not None:
+if any((sys.platform.startswith(x) for x in unsupported)):
+notrun('not suitable for this OS: %s' % sys.platform)
+
+if supported is not None:
+if not any((sys.platform.startswith(x) for x in supported)):
+notrun('not suitable for this OS: %s' % sys.platform)
 
 def verify_cache_mode(supported_cache_modes=[]):
 if supported_cache_modes and (cachemode not in supported_cache_modes):
@@ -1018,7 +1023,8 @@ def execute_unittest(output, verbosity, debug):
 sys.stderr.write(out)
 
 def execute_test(test_function=None,
- supported_fmts=[], supported_oses=['linux'],
+ supported_fmts=[],
+ supported_platforms=None,
  supported_cache_modes=[], unsupported_fmts=[],
  supported_protocols=[], unsupported_protocols=[]):
 """Run either unittest or script-style tests."""
@@ -1035,7 +1041,7 @@ def execute_test(test_function=None,
 verbosity = 1
 verify_image_format(supported_fmts, unsupported_fmts)
 verify_protocol(supported_protocols, unsupported_protocols)
-verify_platform(supported_oses)
+verify_platform(supported=supported_platforms)
 verify_cache_mode(supported_cache_modes)
 
 if debug:
-- 
2.18.1

[PATCH v5 0/6] Enable more iotests during "make check-block"

[Thomas Huth]

As discussed here:

 https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg00697.html

and here:

 https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg01388.html

it would be good to have some more valuable iotests enabled in the
"auto" group to get better iotest coverage during "make check".

Since these Python-based tests require a QEMU that features a 'virtio-blk'
device, we can only run the Python tests if this device is available. With
binaries like qemu-system-tricore, the Python-based tests will be skipped.

v5:
 - Added $python_unusable_because text in the fifth patch
 - Rebased to master and checked that everything still works fine on
   Travis-CI, Cirrus-CI, Gitlab-CI, OpenBSD and NetBSD.

v4:
 - The check for 'virtio-blk' is now done in the tests/qemu-iotests/check
   script instead of tests/check-block.sh (to avoid to duplicate the code
   that searches for the right QEMU binary - and we can also still run
   the shell-based tests this way).
 - Added the new patch to check for the availability of virtio devices in
   the iotests 127 and 267.
 - The patch that drops test 130 from the "auto" group has already been
   merged and thus been dropped from this series.

v3:
 - Test 183 fails on Patchew, so I removed it from the "auto" group
   again

v2:
 - Checked the iotests with NetBSD, too (now that Eduardo has
   re-activated Gerd's patches for creating NetBSD VM images)
 - Use 'openbsd' instead of 'openbsd6'
 - Use 'grep -q' instead of 'grep' for grep'ing silently
 - Added the patch to disable 130 from the "auto" group

John Snow (1):
  iotests: remove 'linux' from default supported platforms

Thomas Huth (5):
  iotests: Test 041 only works on certain systems
  iotests: Test 183 does not work on macOS and OpenBSD
  iotests: Check for the availability of the required devices in 267 and
127
  iotests: Skip Python-based tests if QEMU does not support virtio-blk
  iotests: Enable more tests in the 'auto' group to improve test
coverage

 tests/qemu-iotests/041|  3 ++-
 tests/qemu-iotests/127|  2 ++
 tests/qemu-iotests/183|  1 +
 tests/qemu-iotests/267|  2 ++
 tests/qemu-iotests/check  | 12 ++--
 tests/qemu-iotests/common.rc  | 14 ++
 tests/qemu-iotests/group  | 14 +++---
 tests/qemu-iotests/iotests.py | 16 +++-
 8 files changed, 49 insertions(+), 15 deletions(-)

-- 
2.18.1

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

On 21.01.20 10:23, Vladimir Sementsov-Ogievskiy wrote:
> 21.01.2020 12:14, Max Reitz wrote:
>> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>>> 20.01.2020 20:04, Max Reitz wrote:
 On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
> This test checks that bug is really fixed by previous commit.
>
> Cc: qemu-sta...@nongnu.org # v4.2.0
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>tests/qemu-iotests/283 | 75 ++
>tests/qemu-iotests/283.out |  8 
>tests/qemu-iotests/group   |  1 +
>3 files changed, 84 insertions(+)
>create mode 100644 tests/qemu-iotests/283
>create mode 100644 tests/qemu-iotests/283.out

 The test looks good to me, I just have a comment nit and a note on the
 fact that this should probably be queued only after Thomas’s “Enable
 more iotests during "make check-block"” series.

> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
> new file mode 100644
> index 00..f0f216d109
> --- /dev/null
> +++ b/tests/qemu-iotests/283
> @@ -0,0 +1,75 @@
> +#!/usr/bin/env python
> +#
> +# Test for backup-top filter permission activation failure
> +#
> +# Copyright (c) 2019 Virtuozzo International GmbH.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +#
> +
> +import iotests
> +
> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra 
> runs
> +iotests.verify_image_format(supported_fmts=['qcow2'])
> +
> +size = 1024 * 1024
> +
> +"""
> +On activation, backup-top is going to unshare write permission on its
> +source child. It will be impossible for the following configuration:

 “The following configuration will become impossible”?
>>>
>>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write 
>>> permission",
>>> is impossible with such configuration..
>>
>> But backup_top always unshares the write permission on the source.
> 
> Yes, and I just try to say, that this action will fail. And the test checks 
> that it
> fails (and it crashes with current master instead of fail).

OK.  So what I was trying to say is that the comment currently only
states that this will fail.  I’d prefer it to also reassure me that it’s
correct that this fails (because all writes on the backup source must go
through backup_top), and that this is exactly what we want to test here.

On first reading, I was wondering why exactly this comment would tell me
all these things, because I didn’t know what the test wants to test in
the first place.

Max

 I think there should be some note that this is exactly what we want to
 test, i.e. what happens when this impossible configuration is attempted
 by starting a backup.  (And maybe why this isn’t allowed; namely because
 we couldn’t do CBW for such write accesses.)

> +
> +┌┐  target  ┌─┐
> +│ target │ ◀─── │ backup_top  │
> +└┘  └─┘
> +│
> +│ backing
> +▼
> +┌─┐
> +│   source│
> +└─┘
> +│
> +│ file
> +▼
> +┌─┐  write perm   ┌───┐
> +│base │ ◀ │ other │
> +└─┘   └───┘

 Cool Unicode art. :-)
>>>
>>> I found the great tool: https://dot-to-ascii.ggerganov.com/
>>
>> Thanks!
>>
>> Max
>>
> +
> +Write unsharing will be propagated to the "source->base"link and will
> +conflict with other node write permission.
> +
> +(Note, that we can't just consider source to be direct child of other,
> +as in this case this link will be broken, when backup_top is appended)
> +"""
> +
> +vm = iotests.VM()
> +vm.launch()
> +
> +vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 
> 'null-co'})
> +
> 

Re: [PATCH v3 06/10] block/dirty-bitmap: add _next_dirty API

[Vladimir Sementsov-Ogievskiy]

20.01.2020 19:30, Vladimir Sementsov-Ogievskiy wrote:
> 20.01.2020 16:14, Max Reitz wrote:
>> On 19.12.19 11:03, Vladimir Sementsov-Ogievskiy wrote:
>>> We have bdrv_dirty_bitmap_next_zero, let's add corresponding
>>> bdrv_dirty_bitmap_next_dirty, which is more comfortable to use than
>>> bitmap iterators in some cases.
>>>
>>> For test modify test_hbitmap_next_zero_check_range to check both
>>> next_zero and next_dirty and add some new checks.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   include/block/dirty-bitmap.h |   2 +
>>>   include/qemu/hbitmap.h   |  13 
>>>   block/dirty-bitmap.c |   6 ++
>>>   tests/test-hbitmap.c | 130 ---
>>>   util/hbitmap.c   |  60 
>>>   5 files changed, 126 insertions(+), 85 deletions(-)
>>
>> [...]
>>
>>> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
>>> index b6e85f3d5d..a4b032b270 100644
>>> --- a/include/qemu/hbitmap.h
>>> +++ b/include/qemu/hbitmap.h
>>> @@ -297,6 +297,19 @@ void hbitmap_free(HBitmap *hb);
>>>    */
>>>   void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t 
>>> first);
>>> +/*
>>> + * hbitmap_next_dirty:
>>> + *
>>> + * Find next dirty bit within selected range. If not found, return -1.
>>> + *
>>> + * @hb: The HBitmap to operate on
>>> + * @start: The bit to start from.
>>> + * @count: Number of bits to proceed. If @start+@count > bitmap size, the 
>>> whole
>>> + * bitmap is looked through. You can use UINT64_MAX as @count to search up 
>>> to
>>
>> I would’ve said s/looked through/scanned/, but it matches
>> hbitmap_next_zero()’s documentation, so it’s fine.
>>
>> But definitely s/UINT64_MAX/INT64_MAX/.
>>
>>> + * the bitmap end.
>>> + */
>>> +int64_t hbitmap_next_dirty(const HBitmap *hb, int64_t start, int64_t 
>>> count);
>>> +
>>>   /* hbitmap_next_zero:
>>>    *
>>>    * Find next not dirty bit within selected range. If not found, return -1.
>>
>> [...]
>>
>>> diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
>>> index 0e1e5c64dd..e3f1b3f361 100644
>>> --- a/tests/test-hbitmap.c
>>> +++ b/tests/test-hbitmap.c
>>> @@ -816,92 +816,108 @@ static void 
>>> test_hbitmap_iter_and_reset(TestHBitmapData *data,
>>>   hbitmap_iter_next();
>>>   }
>>> -static void test_hbitmap_next_zero_check_range(TestHBitmapData *data,
>>> -   uint64_t start,
>>> -   uint64_t count)
>>> +static void test_hbitmap_next_x_check_range(TestHBitmapData *data,
>>> +    uint64_t start,
>>> +    uint64_t count)
>>
>> Why not change the parameters to be signed while we’re already here?

Now I think that better to do it in previous patch.

>>
>> [...]
>>
>>> diff --git a/util/hbitmap.c b/util/hbitmap.c
>>> index df22f06be6..d23f4b9678 100644
>>> --- a/util/hbitmap.c
>>> +++ b/util/hbitmap.c
>>> @@ -193,6 +193,30 @@ void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap 
>>> *hb, uint64_t first)
>>>   }
>>>   }
>>> +int64_t hbitmap_next_dirty(const HBitmap *hb, int64_t start, int64_t count)
>>> +{
>>> +    HBitmapIter hbi;
>>> +    int64_t firt_dirty_off;
>>
>> Pre-existing, but isn’t this just a typo that you could fix here?  (i.e.
>> s/firt/first/)
>>
>> Apart from this minor things:
> 
> Agree with them.
> 
>>
>> Reviewed-by: Max Reitz 
>>
>>> +    uint64_t end;
>>> +
>>> +    assert(start >= 0 && count >= 0);
>>> +
>>> +    if (start >= hb->orig_size || count == 0) {
>>> +    return -1;
>>> +    }
>>> +
>>> +    end = count > hb->orig_size - start ?
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Vladimir Sementsov-Ogievskiy]

21.01.2020 12:14, Max Reitz wrote:
> On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
>> 20.01.2020 20:04, Max Reitz wrote:
>>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
 This test checks that bug is really fixed by previous commit.

 Cc: qemu-sta...@nongnu.org # v4.2.0
 Signed-off-by: Vladimir Sementsov-Ogievskiy 
 ---
tests/qemu-iotests/283 | 75 ++
tests/qemu-iotests/283.out |  8 
tests/qemu-iotests/group   |  1 +
3 files changed, 84 insertions(+)
create mode 100644 tests/qemu-iotests/283
create mode 100644 tests/qemu-iotests/283.out
>>>
>>> The test looks good to me, I just have a comment nit and a note on the
>>> fact that this should probably be queued only after Thomas’s “Enable
>>> more iotests during "make check-block"” series.
>>>
 diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
 new file mode 100644
 index 00..f0f216d109
 --- /dev/null
 +++ b/tests/qemu-iotests/283
 @@ -0,0 +1,75 @@
 +#!/usr/bin/env python
 +#
 +# Test for backup-top filter permission activation failure
 +#
 +# Copyright (c) 2019 Virtuozzo International GmbH.
 +#
 +# This program is free software; you can redistribute it and/or modify
 +# it under the terms of the GNU General Public License as published by
 +# the Free Software Foundation; either version 2 of the License, or
 +# (at your option) any later version.
 +#
 +# This program is distributed in the hope that it will be useful,
 +# but WITHOUT ANY WARRANTY; without even the implied warranty of
 +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 +# GNU General Public License for more details.
 +#
 +# You should have received a copy of the GNU General Public License
 +# along with this program.  If not, see .
 +#
 +
 +import iotests
 +
 +# The test is unrelated to formats, restrict it to qcow2 to avoid extra 
 runs
 +iotests.verify_image_format(supported_fmts=['qcow2'])
 +
 +size = 1024 * 1024
 +
 +"""
 +On activation, backup-top is going to unshare write permission on its
 +source child. It will be impossible for the following configuration:
>>>
>>> “The following configuration will become impossible”?
>>
>> Hmm, no, the configuration is possible. But "it", i.e. "unshare write 
>> permission",
>> is impossible with such configuration..
> 
> But backup_top always unshares the write permission on the source.

Yes, and I just try to say, that this action will fail. And the test checks 
that it
fails (and it crashes with current master instead of fail).

> 
>>> I think there should be some note that this is exactly what we want to
>>> test, i.e. what happens when this impossible configuration is attempted
>>> by starting a backup.  (And maybe why this isn’t allowed; namely because
>>> we couldn’t do CBW for such write accesses.)
>>>
 +
 +┌┐  target  ┌─┐
 +│ target │ ◀─── │ backup_top  │
 +└┘  └─┘
 +│
 +│ backing
 +▼
 +┌─┐
 +│   source│
 +└─┘
 +│
 +│ file
 +▼
 +┌─┐  write perm   ┌───┐
 +│base │ ◀ │ other │
 +└─┘   └───┘
>>>
>>> Cool Unicode art. :-)
>>
>> I found the great tool: https://dot-to-ascii.ggerganov.com/
> 
> Thanks!
> 
> Max
> 
 +
 +Write unsharing will be propagated to the "source->base"link and will
 +conflict with other node write permission.
 +
 +(Note, that we can't just consider source to be direct child of other,
 +as in this case this link will be broken, when backup_top is appended)
 +"""
 +
 +vm = iotests.VM()
 +vm.launch()
 +
 +vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
 +
 +vm.qmp_log('blockdev-add', **{
 +'node-name': 'source',
 +'driver': 'blkdebug',
 +'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
 +})
 +
 +vm.qmp_log('blockdev-add', **{
 +'node-name': 'other',
 +'driver': 'blkdebug',
 +'image': 'base',
 +'take-child-perms': ['write']
 +})
 +
 +vm.qmp_log('blockdev-backup', sync='full', device='source', 
 target='target')
 +
 +vm.shutdown()
>>>
>>> [...]
>>>
 diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
 index cb2b789e44..d827e8c821 100644
 --- 

Re: [PATCH 2/2] iotests: add test for backup-top failure on permission activation

[Max Reitz]

On 20.01.20 18:20, Vladimir Sementsov-Ogievskiy wrote:
> 20.01.2020 20:04, Max Reitz wrote:
>> On 16.01.20 16:54, Vladimir Sementsov-Ogievskiy wrote:
>>> This test checks that bug is really fixed by previous commit.
>>>
>>> Cc: qemu-sta...@nongnu.org # v4.2.0
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>>> ---
>>>   tests/qemu-iotests/283 | 75 ++
>>>   tests/qemu-iotests/283.out |  8 
>>>   tests/qemu-iotests/group   |  1 +
>>>   3 files changed, 84 insertions(+)
>>>   create mode 100644 tests/qemu-iotests/283
>>>   create mode 100644 tests/qemu-iotests/283.out
>>
>> The test looks good to me, I just have a comment nit and a note on the
>> fact that this should probably be queued only after Thomas’s “Enable
>> more iotests during "make check-block"” series.
>>
>>> diff --git a/tests/qemu-iotests/283 b/tests/qemu-iotests/283
>>> new file mode 100644
>>> index 00..f0f216d109
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/283
>>> @@ -0,0 +1,75 @@
>>> +#!/usr/bin/env python
>>> +#
>>> +# Test for backup-top filter permission activation failure
>>> +#
>>> +# Copyright (c) 2019 Virtuozzo International GmbH.
>>> +#
>>> +# This program is free software; you can redistribute it and/or modify
>>> +# it under the terms of the GNU General Public License as published by
>>> +# the Free Software Foundation; either version 2 of the License, or
>>> +# (at your option) any later version.
>>> +#
>>> +# This program is distributed in the hope that it will be useful,
>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +# GNU General Public License for more details.
>>> +#
>>> +# You should have received a copy of the GNU General Public License
>>> +# along with this program.  If not, see .
>>> +#
>>> +
>>> +import iotests
>>> +
>>> +# The test is unrelated to formats, restrict it to qcow2 to avoid extra 
>>> runs
>>> +iotests.verify_image_format(supported_fmts=['qcow2'])
>>> +
>>> +size = 1024 * 1024
>>> +
>>> +"""
>>> +On activation, backup-top is going to unshare write permission on its
>>> +source child. It will be impossible for the following configuration:
>>
>> “The following configuration will become impossible”?
> 
> Hmm, no, the configuration is possible. But "it", i.e. "unshare write 
> permission",
> is impossible with such configuration..

But backup_top always unshares the write permission on the source.

>> I think there should be some note that this is exactly what we want to
>> test, i.e. what happens when this impossible configuration is attempted
>> by starting a backup.  (And maybe why this isn’t allowed; namely because
>> we couldn’t do CBW for such write accesses.)
>>
>>> +
>>> +┌┐  target  ┌─┐
>>> +│ target │ ◀─── │ backup_top  │
>>> +└┘  └─┘
>>> +│
>>> +│ backing
>>> +▼
>>> +┌─┐
>>> +│   source│
>>> +└─┘
>>> +│
>>> +│ file
>>> +▼
>>> +┌─┐  write perm   ┌───┐
>>> +│base │ ◀ │ other │
>>> +└─┘   └───┘
>>
>> Cool Unicode art. :-)
> 
> I found the great tool: https://dot-to-ascii.ggerganov.com/

Thanks!

Max

>>> +
>>> +Write unsharing will be propagated to the "source->base"link and will
>>> +conflict with other node write permission.
>>> +
>>> +(Note, that we can't just consider source to be direct child of other,
>>> +as in this case this link will be broken, when backup_top is appended)
>>> +"""
>>> +
>>> +vm = iotests.VM()
>>> +vm.launch()
>>> +
>>> +vm.qmp_log('blockdev-add', **{'node-name': 'target', 'driver': 'null-co'})
>>> +
>>> +vm.qmp_log('blockdev-add', **{
>>> +'node-name': 'source',
>>> +'driver': 'blkdebug',
>>> +'image': {'node-name': 'base', 'driver': 'null-co', 'size': size}
>>> +})
>>> +
>>> +vm.qmp_log('blockdev-add', **{
>>> +'node-name': 'other',
>>> +'driver': 'blkdebug',
>>> +'image': 'base',
>>> +'take-child-perms': ['write']
>>> +})
>>> +
>>> +vm.qmp_log('blockdev-backup', sync='full', device='source', 
>>> target='target')
>>> +
>>> +vm.shutdown()
>>
>> [...]
>>
>>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>>> index cb2b789e44..d827e8c821 100644
>>> --- a/tests/qemu-iotests/group
>>> +++ b/tests/qemu-iotests/group
>>> @@ -288,3 +288,4 @@
>>>   277 rw quick
>>>   279 rw backing quick
>>>   280 rw migration quick
>>> +283 auto quick
>>
>> Hm.  This would be the first Python test in auto.
> 
> Missed that. It's OK to define it just "quick" and update later.
> 
>>  Thomas’s series has
>> at least 

Re: [PATCH v10 2/2] docs: qcow2: introduce compression type feature

[Vladimir Sementsov-Ogievskiy]

20.01.2020 22:46, Eric Blake wrote:
> On 1/20/20 11:13 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The patch add new additional field to qcow2 header: compression_type,
> 
> s/add/adds a/
> s/to/to the/
> 
>> 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.
> 
> It would be nice to have that patch as part of the same series, but it has 
> already been posted to the list separately, so I'm okay with this series as 
> just doc word-smithing while we get that patch series in soon.
> 
>>
>> Suggested-by: Denis Plotnikov 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
>> ---
>>   docs/interop/qcow2.txt | 16 +++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index 355925c35e..4569f0dba3 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -109,6 +109,11 @@ the next fields through header_length.
>>   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,
>> +    a non-default compression is used for 
>> compressed
>> +    clusters. The compression_type field must be
>> +    present and not zero.
> 
> Why must the compression_type field be non-zero?  If this bit is set, an 
> older qemu cannot use the image, regardless of the contents of the 
> compression_type field, so for maximum back-compat, a sane app will never set 
> this bit when compression_type is zero.  But there is nothing technically 
> wrong with setting this bit even when compression_type is 0, and newer qemu 
> would still manage to use the image correctly with zlib compression if it 
> were not for this arbitrary restriction.

OK, I just made it stricter, no actual reason for it. Then:

If this bit is set, the compression type of the image is defined by 
compression_type additional field (which must present in this case).

> 
>> +
>>   Bits 3-63:  Reserved (set to 0)
>>    80 -  87:  compatible_features
>> @@ -189,7 +194,16 @@ present*, if not altered by a specific incompatible bit.
>>   *. A field is considered not present when header_length is less or equal 
>> to the
>>   field's offset. Also, all additional fields are not present for version 2.
>> -    < ... No additional fields in the header currently ... >
>> +  104:  compression_type
>> +    Defines the compression method used for compressed 
>> clusters.
>> +    All compressed clusters in an image use the same 
>> compression
>> +    type.
>> +    If the incompatible bit "Compression type" is set: the 
>> field
> 
> Ragged edge formatting looks awkward.  Either this is one paragraph ("type.  
> If") or there should be a blank line to make it obvious there are two 
> paragraphs.

OK, let it be additional empty line. Then we need one before "Defines" too?

> 
>> +    must be present and non-zero (which means non-zlib
>> +    compression type). Otherwise, this field must not be 
>> present
>> +    or must be zero (which means zlib).
>> +    Available compression type values:
>> +    0: zlib 
> 
> I'm still not sure I agree with the mandate that the field must be non-zero 
> when the incompatible feature bit is set.
> 

I don't care, so let's make it less strict.

-- 
Best regards,
Vladimir

Re: [PATCH 10/13] block: add generic infrastructure for x-blockdev-amend qmp command

[Markus Armbruster]

Maxim Levitsky  writes:

> blockdev-amend will be used similiar to blockdev-create
> to allow on the fly changes of the structure of the format based block 
> devices.
>
> Current plan is to first support encryption keyslot management for luks
> based formats (raw and embedded in qcow2)
>
> Signed-off-by: Maxim Levitsky 
[...]
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ff5e5edaf..601f7dc9a4 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4743,6 +4743,48 @@
>'data': { 'job-id': 'str',
>  'options': 'BlockdevCreateOptions' } }
>  
> +##
> +# @BlockdevAmendOptions:
> +#
> +# Options for amending an image format
> +#
> +# @driver   block driver that is suitable for the image
> +#
> +# Since: 5.0
> +##
> +{ 'union': 'BlockdevAmendOptions',
> +  'base': {
> +  'driver': 'BlockdevDriver' },
> +  'discriminator': 'driver',
> +  'data': {
> +  } }
> +
> +##
> +# @x-blockdev-amend:
> +#
> +# Starts a job to amend format specific options of an existing open block 
> device
> +# The job is automatically finalized, but a manual job-dismiss is required.
> +#
> +# @job-id:  Identifier for the newly created job.
> +#
> +# @node-name:   Name of the block node to work on
> +#
> +# @options: Options (driver specific)
> +#
> +# @force:   Allow unsafe operations, format specific
> +#   For luks that allows erase of the last active keyslot
> +#   (permanent loss of data),
> +#   and replacement of an active keyslot
> +#   (possible loss of data if IO error happens)

PATCH 2 appears to reject that.  What am I missing?

> +#
> +# Since: 5.0
> +##
> +{ 'command': 'x-blockdev-amend',
> +  'data': { 'job-id': 'str',
> +'node-name': 'str',
> +'options': 'BlockdevAmendOptions',
> +'*force': 'bool' } }
> +
>  ##
>  # @blockdev-open-tray:
>  #
> diff --git a/qapi/job.json b/qapi/job.json
> index a121b615fb..362b634ec1 100644
> --- a/qapi/job.json
> +++ b/qapi/job.json
> @@ -19,10 +19,12 @@
>  #
>  # @create: image creation job type, see "blockdev-create" (since 3.0)
>  #
> +# @amend: image options amend job type, see "x-blockdev-amend" (since 5.0)
> +#
>  # Since: 1.7
>  ##
>  { 'enum': 'JobType',
> -  'data': ['commit', 'stream', 'mirror', 'backup', 'create'] }
> +  'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
>  
>  ##
>  # @JobStatus: