Re: [PATCH v7 4/5] block/io: fix bdrv_is_allocated_above

2020-09-24 Thread Eric Blake

On 9/24/20 2:40 PM, Vladimir Sementsov-Ogievskiy wrote:

bdrv_is_allocated_above wrongly handles short backing files: it reports
after-EOF space as UNALLOCATED which is wrong, as on read the data is
generated on the level of short backing file (if all overlays has


s/has/have/


unallocated area at that place).

Reusing bdrv_common_block_status_above fixes the issue and unifies code
path.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
Reviewed-by: Alberto Garcia 
---
  block/io.c | 43 +--
  1 file changed, 5 insertions(+), 38 deletions(-)


Nice diffstat, R-b remains in force.

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




Re: [PATCH v7 2/5] block/io: bdrv_common_block_status_above: support include_base

2020-09-24 Thread Eric Blake

On 9/24/20 2:40 PM, Vladimir Sementsov-Ogievskiy wrote:

In order to reuse bdrv_common_block_status_above in
bdrv_is_allocated_above, let's support include_base parameter.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
  block/coroutines.h |  2 ++
  block/io.c | 21 ++---
  2 files changed, 16 insertions(+), 7 deletions(-)




+++ b/block/io.c
@@ -2343,6 +2343,7 @@ early_out:
  int coroutine_fn
  bdrv_co_common_block_status_above(BlockDriverState *bs,
BlockDriverState *base,
+  bool include_base,
bool want_zero,
int64_t offset,
int64_t bytes,
@@ -2354,10 +2355,11 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
  BlockDriverState *p;
  int64_t eof = 0;
  
-assert(bs != base);

+assert(include_base || bs != base);
+assert(!include_base || base); /* Can't include NULL base */


I wonder if this would be easier to read as:

if (include_base) {
assert(bs != base);
} else {
assert(base); /* Can't include NULL base */
}

but I won't insist.

Reviewed-by: Eric Blake 

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




Re: [PATCH v7 1/5] block/io: fix bdrv_co_block_status_above

2020-09-24 Thread Eric Blake

On 9/24/20 2:39 PM, Vladimir Sementsov-Ogievskiy wrote:

bdrv_co_block_status_above has several design problems with handling
short backing files:

1. With want_zeros=true, it may return ret with BDRV_BLOCK_ZERO but
without BDRV_BLOCK_ALLOCATED flag, when actually short backing file
which produces these after-EOF zeros is inside requested backing
sequence.

2. With want_zero=false, it may return pnum=0 prior to actual EOF,
because of EOF of short backing file.

Fix these things, making logic about short backing files clearer.

With fixed bdrv_block_status_above we also have to improve is_zero in
qcow2 code, otherwise iotest 154 will fail, because with this patch we
stop to merge zeros of different types (produced by fully unallocated
in the whole backing chain regions vs produced by short backing files).

Note also, that this patch leaves for another day the general problem
around block-status: misuse of BDRV_BLOCK_ALLOCATED as is-fs-allocated
vs go-to-backing.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Alberto Garcia 
---
  block/io.c| 68 ---
  block/qcow2.c | 16 ++--
  2 files changed, 68 insertions(+), 16 deletions(-)

diff --git a/block/io.c b/block/io.c
index 449b99b92c..4697e67a85 100644
--- a/block/io.c
+++ b/block/io.c
@@ -2350,34 +2350,74 @@ bdrv_co_common_block_status_above(BlockDriverState *bs,
int64_t *map,
BlockDriverState **file)
  {
+int ret;
  BlockDriverState *p;
-int ret = 0;


The shuffle of these lines is odd, but not a show-stopper.

The new flow is definitely easier to read.  Thanks for doing this!


+++ b/block/qcow2.c
@@ -3860,8 +3860,20 @@ static bool is_zero(BlockDriverState *bs, int64_t 
offset, int64_t bytes)
  if (!bytes) {
  return true;
  }
-res = bdrv_block_status_above(bs, NULL, offset, bytes, , NULL, NULL);
-return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == bytes;
+
+/*
+ * bdrv_block_status_above doesn't merge different types of zeros, for


We're inconsistent on whether we spell the noun 'zeros' or 'zeroes'; but 
I don't care enough to make you change this.



+ * example, zeros which come from the region which is unallocated in
+ * the whole backing chain, and zeros which comes because of a short


s/comes/come/


+ * backing file. So, we need a loop.
+ */
+do {
+res = bdrv_block_status_above(bs, NULL, offset, bytes, , NULL, 
NULL);
+offset += nr;
+bytes -= nr;
+} while (res >= 0 && (res & BDRV_BLOCK_ZERO) && nr && bytes);
+
+return res >= 0 && (res & BDRV_BLOCK_ZERO) && bytes == 0;
  }
  
  static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs,




Reviewed-by: Eric Blake 

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




Re: [PATCH v9 4/7] scripts: add block-coroutine-wrapper.py

2020-09-24 Thread Eric Blake

On 9/24/20 1:54 PM, Vladimir Sementsov-Ogievskiy wrote:

We have a very frequent pattern of creating a coroutine from a function
with several arguments:

   - create a structure to pack parameters
   - create _entry function to call original function taking parameters
 from struct
   - do different magic to handle completion: set ret to NOT_DONE or
 EINPROGRESS or use separate bool field
   - fill the struct and create coroutine from _entry function with this
 struct as a parameter
   - do coroutine enter and BDRV_POLL_WHILE loop

Let's reduce code duplication by generating coroutine wrappers.

This patch adds scripts/block-coroutine-wrapper.py together with some
friends, which will generate functions with declared prototypes marked
by the 'generated_co_wrapper' specifier.

The usage of new code generation is as follows:

 1. define the coroutine function somewhere

 int coroutine_fn bdrv_co_NAME(...) {...}

 2. declare in some header file

 int generated_co_wrapper bdrv_NAME(...);

with same list of parameters (generated_co_wrapper is
defined in "include/block/block.h").

 3. Make sure the block_gen_c delaration in block/meson.build


declaration


mentions the file with your marker function.

Still, no function is now marked, this work is for the following
commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---



+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -0,0 +1,54 @@
+===
+block-coroutine-wrapper
+===
+
+A lot of functions in QEMU block layer (see ``block/*``) can only be
+called in coroutine context. Such functions are normally marked by the
+coroutine_fn specifier. Still, sometimes we need to call them from
+non-coroutine context; for this we need to start a coroutine, run the
+needed function from it and wait for coroutine finish in


to finish in a


+BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
+void* argument. So for each coroutine_fn function which needs a
+non-coroutine interface, we should define a structure to pack the
+parameters, define a separate function to unpack the parameters and
+call the original function and finally define a new interface function
+with same list of arguments as original one, which will pack the
+parameters into a struct, create a coroutine, run it and wait in
+BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand,
+so we have a script to generate them.
+



+++ b/scripts/block-coroutine-wrapper.py

Reviewed-by: Eric Blake 

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




Re: [PATCH v9 0/7] coroutines: generate wrapper code

2020-09-24 Thread Eric Blake

On 9/24/20 1:54 PM, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

The aim of the series is to reduce code-duplication and writing
parameters structure-packing by hand around coroutine function wrappers.

Benefits:
  - no code duplication
  - less indirection

v9: Thanks to Eric, I used commit message tweaks and rebase-conflict solving 
from his git.
01: add Philippe's, Stefan's r-bs
02: - add Philippe's, Stefan's r-bs
 - commit message tweaks stolen from Eric's git :)


LOL: "stolen" makes it sound like a crime was committed ;)  I find it to 
be one of the joys of open source when my ideas show up in someone 
else's work when properly attributed, as you did here.  [Maybe the 
recent push towards a conscious language initiative has let me hone in 
on something that turned out humorous to me, but might not be as obvious 
to someone less fluent in English idioms]


At any rate, I'm glad my rebase efforts helped.


03: add Philippe's, Stefan's r-bs
04: - wording/grammar by Eric (partly, stolen from repo)
 - ref new file in docs/devel/index.rst
 - use 644 rights and recommended header for python script
 - call gen_header() once
 - rename gen_wrappers_file to gen_wrappers
05: add Stefan's r-b
06: add Philippe's, Stefan's r-bs
07: Stefan's r-b



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




Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

2020-09-24 Thread Eric Blake

On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:

We have a very frequent pattern of creating coroutine from function
with several arguments:




+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -0,0 +1,54 @@
+===
+block-coroutine-wrapper
+===
+
+A lot of functions in QEMJ block layer (see ``block/*``) can by called


My editor italicized everhting after block/*...


+only in coroutine context. Such functions are normally marked by
+coroutine_fn specifier. Still, sometimes we need to call them from
+non-coroutine context, for this we need to start a coroutine, run the
+needed function from it and wait for coroutine finish in
+BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
+void* argument. So for each coroutine_fn function, which needs


...through void*.  I wonder if you need to use \* to let .rst know that 
these are literal *, and not markup for a different font style. 
Although I did not check the actual generated docs to see how they look.


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




Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

2020-09-23 Thread Eric Blake

On 9/23/20 7:00 PM, Eric Blake wrote:



Tested-by: Eric Blake 

There's enough grammar fixes, and the fact that John is working on 
python cleanups, to make me wonder if we need a v9, or if I should just 
stage it where it is with any other cleanups as followups.  But I'm 
liking the reduced maintenance burden once it is in, and don't want to 
drag it out to the point that it needs more rebasing as other things 
land first.




Here's what I've squashed in and temporarily pushed to my tree if you 
want to double-check my rebase work:

https://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/master

diff --git a/docs/devel/block-coroutine-wrapper.rst 
b/docs/devel/block-coroutine-wrapper.rst

index f7050bbc8fa6..d09fff2cc539 100644
--- a/docs/devel/block-coroutine-wrapper.rst
+++ b/docs/devel/block-coroutine-wrapper.rst
@@ -2,43 +2,43 @@
 block-coroutine-wrapper
 ===

-A lot of functions in QEMJ block layer (see ``block/*``) can by called
-only in coroutine context. Such functions are normally marked by
+A lot of functions in QEMU block layer (see ``block/*``) can only be
+called in coroutine context. Such functions are normally marked by the
 coroutine_fn specifier. Still, sometimes we need to call them from
-non-coroutine context, for this we need to start a coroutine, run the
+non-coroutine context; for this we need to start a coroutine, run the
 needed function from it and wait for coroutine finish in
 BDRV_POLL_WHILE() loop. To run a coroutine we need a function with one
-void* argument. So for each coroutine_fn function, which needs
+void* argument. So for each coroutine_fn function which needs a
 non-coroutine interface, we should define a structure to pack the
 parameters, define a separate function to unpack the parameters and
 call the original function and finally define a new interface function
 with same list of arguments as original one, which will pack the
 parameters into a struct, create a coroutine, run it and wait in
-BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand, so
-we have a script to generate them.
+BDRV_POLL_WHILE() loop. It's boring to create such wrappers by hand,
+so we have a script to generate them.

 Usage
 =

-Assume we have defined ``coroutine_fn`` function
+Assume we have defined the ``coroutine_fn`` function
 ``bdrv_co_foo()`` and need a non-coroutine interface for it,
 called ``bdrv_foo()``. In this case the script can help. To
 trigger the generation:

-1. You need ``bdrv_foo`` declaration somewhere (for example in
-   ``block/coroutines.h`` with ``generated_co_wrapper`` mark,
+1. You need ``bdrv_foo`` declaration somewhere (for example, in
+   ``block/coroutines.h``) with the ``generated_co_wrapper`` mark,
like this:

 .. code-block:: c

-int generated_co_wrapper bdrv_foor();
+int generated_co_wrapper bdrv_foo();

 2. You need to feed this declaration to block-coroutine-wrapper script.
-   For this, add .h (or .c) file with the declaration to
+   For this, add the .h (or .c) file with the declaration to the
``input: files(...)`` list of ``block_gen_c`` target declaration in
``block/meson.build``

-You are done. On build, coroutine wrappers will be generated in
+You are done. During the build, coroutine wrappers will be generated in
 ``/block/block-gen.c``.

 Links
diff --git a/docs/devel/index.rst b/docs/devel/index.rst
index 04773ce076b3..cb0abe1e6988 100644
--- a/docs/devel/index.rst
+++ b/docs/devel/index.rst
@@ -31,3 +31,4 @@ Contents:
reset
s390-dasd-ipl
clocks
+   block-coroutine-wrapper
diff --git a/scripts/block-coroutine-wrapper.py 
b/scripts/block-coroutine-wrapper.py

index d859c07a5f55..8c0a08d9b020 100755
--- a/scripts/block-coroutine-wrapper.py
+++ b/scripts/block-coroutine-wrapper.py
@@ -2,7 +2,7 @@
 """Generate coroutine wrappers for block subsystem.

 The program parses one or several concatenated c files from stdin,
-searches for functions with 'generated_co_wrapper' specifier
+searches for functions with the 'generated_co_wrapper' specifier
 and generates corresponding wrappers on stdout.

 Usage: block-coroutine-wrapper.py generated-file.c FILE.[ch]...
@@ -39,7 +39,7 @@ def prettify(code: str) -> str:
 'BraceWrapping': {'AfterFunction': True},
 'BreakBeforeBraces': 'Custom',
 'SortIncludes': False,
-'MaxEmptyLinesToKeep': 2
+'MaxEmptyLinesToKeep': 2,
 })
 p = subprocess.run(['clang-format', f'-style={style}'], 
check=True,

encoding='utf-8', input=code,
@@ -168,7 +168,7 @@ int {func.name}({ func.gen_list('{decl}') })


 def gen_wrappers_file(input_code: str) -> str:
-res = gen_header()
+res = ''
 for func in func_decl_iter(input_code):
 res += '\n\n\n'
 res += gen_wrapper(func)
@@ -181,6 +181,7 @@ if __name__ == '__main__':
 exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')

 with open(sys.a

Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

2020-09-23 Thread Eric Blake

On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:

We have a very frequent pattern of creating coroutine from function
with several arguments:




+++ b/scripts/block-coroutine-wrapper.py
@@ -0,0 +1,187 @@
+#!/usr/bin/env python3
+"""Generate coroutine wrappers for block subsystem.


Looking at the generated file after patch 5 is applied,...



+
+def gen_header():
+copyright = re.sub('^.*Copyright', 'Copyright', __doc__, flags=re.DOTALL)
+copyright = re.sub('^(?=.)', ' * ', copyright.strip(), flags=re.MULTILINE)
+copyright = re.sub('^$', ' *', copyright, flags=re.MULTILINE)
+return f"""\


This generated comment...



+
+
+def gen_wrappers_file(input_code: str) -> str:
+res = gen_header()


...is getting inserted into the generated file...


+for func in func_decl_iter(input_code):
+res += '\n\n\n'
+res += gen_wrapper(func)
+
+return prettify(res)  # prettify to wrap long lines
+
+
+if __name__ == '__main__':
+if len(sys.argv) < 3:
+exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
+
+with open(sys.argv[1], 'w') as f_out:
+for fname in sys.argv[2:]:
+with open(fname) as f_in:
+f_out.write(gen_wrappers_file(f_in.read()))


multiple times.  You'll want to hoist the call to gen_header outside the 
loop over fname in sys.argv[2:].


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




Re: [PATCH v8 4/7] scripts: add block-coroutine-wrapper.py

2020-09-23 Thread Eric Blake
args=m.group('args'))
+
+
+def snake_to_camel(func_name: str) -> str:
+"""
+Convert underscore names like 'some_function_name' to camel-case like
+'SomeFunctionName'
+"""
+words = func_name.split('_')
+words = [w[0].upper() + w[1:] for w in words]
+return ''.join(words)
+
+
+def gen_wrapper(func: FuncDecl) -> str:
+assert func.name.startswith('bdrv_')
+assert not func.name.startswith('bdrv_co_')
+assert func.return_type == 'int'
+assert func.args[0].type in ['BlockDriverState *', 'BdrvChild *']
+
+name = 'bdrv_co_' + func.name[5:]
+bs = 'bs' if func.args[0].type == 'BlockDriverState *' else 'child->bs'
+struct_name = snake_to_camel(name)
+
+return f"""\
+/*
+ * Wrappers for {name}
+ */
+
+typedef struct {struct_name} {{
+BdrvPollCo poll_state;
+{ func.gen_block('{decl};') }
+}} {struct_name};
+
+static void coroutine_fn {name}_entry(void *opaque)
+{{
+{struct_name} *s = opaque;
+
+s->poll_state.ret = {name}({ func.gen_list('s->{name}') });


Excuse my lack of python style awareness, but why are we mixing 
{nospace} and { space } on the same line?



+s->poll_state.in_progress = false;
+
+aio_wait_kick();
+}}
+
+int {func.name}({ func.gen_list('{decl}') })
+{{
+if (qemu_in_coroutine()) {{
+return {name}({ func.gen_list('{name}') });
+}} else {{
+{struct_name} s = {{
+.poll_state.bs = {bs},
+.poll_state.in_progress = true,
+
+{ func.gen_block('.{name} = {name},') }
+}};
+
+s.poll_state.co = qemu_coroutine_create({name}_entry, );
+
+return bdrv_poll_co(_state);
+}}
+}}"""
+
+
+def gen_wrappers_file(input_code: str) -> str:
+res = gen_header()
+for func in func_decl_iter(input_code):
+res += '\n\n\n'
+res += gen_wrapper(func)
+
+return prettify(res)  # prettify to wrap long lines
+
+
+if __name__ == '__main__':
+if len(sys.argv) < 3:
+exit(f'Usage: {sys.argv[0]} OUT_FILE.c IN_FILE.[ch]...')
+
+with open(sys.argv[1], 'w') as f_out:
+for fname in sys.argv[2:]:
+with open(fname) as f_in:
+f_out.write(gen_wrappers_file(f_in.read()))
+f_out.write('\n')


Tested-by: Eric Blake 

There's enough grammar fixes, and the fact that John is working on 
python cleanups, to make me wonder if we need a v9, or if I should just 
stage it where it is with any other cleanups as followups.  But I'm 
liking the reduced maintenance burden once it is in, and don't want to 
drag it out to the point that it needs more rebasing as other things 
land first.


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




Re: [PATCH v8 3/7] block: declare some coroutine functions in block/coroutines.h

2020-09-23 Thread Eric Blake

On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:

We are going to keep coroutine-wrappers code (structure-packing
parameters, BDRV_POLL wrapper functions) in separate auto-generated
files. So, we'll need a header with declaration of original _co_
functions, for those which are static now. As well, we'll need
declarations for wrapper functions. Do these declarations now, as a
preparation step.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
---



+++ b/block/coroutines.h



+int coroutine_fn bdrv_co_check(BlockDriverState *bs,
+   BdrvCheckResult *res, BdrvCheckMode fix);
+int coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp);
+
+int coroutine_fn
+bdrv_co_prwv(BdrvChild *child, int64_t offset, QEMUIOVector *qiov,
+ bool is_write, BdrvRequestFlags flags);


Inconsistent on whether the function name is in column 1 or after the 
return type. But we aren't consistent elsewhre, so I won't bother 
changing it.


R-b still stands

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




[PATCH] docs: Better mention of qemu-img amend limitations

2020-09-23 Thread Eric Blake
Missed during merge resolution of commit bc5ee6da71.

Signed-off-by: Eric Blake 
---
 docs/tools/qemu-img.rst | 4 
 1 file changed, 4 insertions(+)

diff --git a/docs/tools/qemu-img.rst b/docs/tools/qemu-img.rst
index c35bd6482203..2b5891b54db7 100644
--- a/docs/tools/qemu-img.rst
+++ b/docs/tools/qemu-img.rst
@@ -265,6 +265,10 @@ Command description:
   --force allows some unsafe operations. Currently for -f luks, it allows to
   erase the last encryption key, and to overwrite an active encryption key.

+  The set of options that can be amended are dependent on the image
+  format, but note that amending the backing chain relationship should
+  instead be performed with ``qemu-img rebase``.
+
 .. option:: bench [-c COUNT] [-d DEPTH] [-f FMT] 
[--flush-interval=FLUSH_INTERVAL] [-i AIO] [-n] [--no-drain] [-o OFFSET] 
[--pattern=PATTERN] [-q] [-s BUFFER_SIZE] [-S STEP_SIZE] [-t CACHE] [-w] [-U] 
FILENAME

   Run a simple sequential I/O benchmark on the specified image. If ``-w`` is
-- 
2.28.0




Re: [PATCH v8 7/7] block/io: refactor save/load vmstate

2020-09-23 Thread Eric Blake

On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:

Like for read/write in a previous commit, drop extra indirection layer,
generate directly bdrv_readv_vmstate() and bdrv_writev_vmstate().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 
---
  block/coroutines.h| 10 +++
  include/block/block.h |  6 ++--
  block/io.c| 67 ++-
  3 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/block/coroutines.h b/block/coroutines.h



  int coroutine_fn
-bdrv_co_rw_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos,
-   bool is_read)
+bdrv_co_readv_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
  {
  BlockDriver *drv = bs->drv;
  int ret = -ENOTSUP;
  
+if (!drv) {

+return -ENOMEDIUM;
+}
+
  bdrv_inc_in_flight(bs);
  
-if (!drv) {

-ret = -ENOMEDIUM;
-} else if (drv->bdrv_load_vmstate) {
-if (is_read) {
-ret = drv->bdrv_load_vmstate(bs, qiov, pos);
-} else {
-ret = drv->bdrv_save_vmstate(bs, qiov, pos);
-}
+if (drv->bdrv_load_vmstate) {
+ret = drv->bdrv_load_vmstate(bs, qiov, pos);


This one makes sense;


  } else if (bs->file) {
-ret = bdrv_co_rw_vmstate(bs->file->bs, qiov, pos, is_read);
+ret = bdrv_co_readv_vmstate(bs->file->bs, qiov, pos);
  }
  
  bdrv_dec_in_flight(bs);

+
  return ret;
  }
  
-int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,

-  int64_t pos, int size)
+int coroutine_fn
+bdrv_co_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)
  {
-QEMUIOVector qiov = QEMU_IOVEC_INIT_BUF(qiov, buf, size);
-int ret;
+BlockDriver *drv = bs->drv;
+int ret = -ENOTSUP;
  
-ret = bdrv_writev_vmstate(bs, , pos);

-if (ret < 0) {
-return ret;
+if (!drv) {
+return -ENOMEDIUM;
  }
  
-return size;

-}
+bdrv_inc_in_flight(bs);
  
-int bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos)

-{
-return bdrv_rw_vmstate(bs, qiov, pos, false);
+if (drv->bdrv_load_vmstate) {
+ret = drv->bdrv_save_vmstate(bs, qiov, pos);


but this one looks awkward. It represents the pre-patch logic, but it 
would be nicer to check for bdrv_save_vmstate.  With that tweak, my R-b 
still stands.


I had an interesting time applying this patch due to merge conflicts 
with the new bdrv_primary_bs() changes that landed in the meantime.


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




Re: [PATCH v8 2/7] block/io: refactor coroutine wrappers

2020-09-23 Thread Eric Blake

On 9/15/20 11:44 AM, Vladimir Sementsov-Ogievskiy wrote:

Most of our coroutine wrappers already follow this convention:

We have 'coroutine_fn bdrv_co_()' as
the core function, and a wrapper 'bdrv_()' which does parameters packing and call bdrv_run_co().

The only outsiders are the bdrv_prwv_co and
bdrv_common_block_status_above wrappers. Let's refactor them to behave
as the others, it simplifies further conversion of coroutine wrappers.

This patch adds indirection layer, but it will be compensated by


adds an


further commit, which will drop bdrv_co_prwv together with is_write
logic, to keep read and write path separate.


keep the



Signed-off-by: Vladimir Sementsov-Ogievskiy 
Reviewed-by: Eric Blake 


R-b stands.

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




Re: [PATCH 3/4] block/nbd: fix reconnect-delay

2020-09-23 Thread Eric Blake

On 9/3/20 2:03 PM, Vladimir Sementsov-Ogievskiy wrote:

reconnect-delay has a design flaw: we handle it in the same loop where
we do connection attempt. So, reconnect-delay may be exceeded by
unpredictable time of connection attempt.

Let's instead use separate timer.

How to reproduce the bug:





This will make the connect() call of qemu-io at node2 take a long time.

And you'll see that read command in qemu-io will hang for a long time,
more than 15 seconds specified by reconnect-delay parameter. It's the
bug.

8. Don't forget to drop iptables rule on node1:

sudo iptables -D INPUT -p tcp --dport 10809 -j DROP

Important note: Step [5] is necessary to reproduce _this_ bug. If we
miss step [5], the read command (step 6) will hang for a long time and
this commit doesn't help, because there will be not long connect() to
unreachable host, but long sendmsg() to unreachable host, which should
be fixed by enabling and adjusting keep-alive on the socket, which is a
thing for further patch set.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/nbd.c | 59 +
  1 file changed, 50 insertions(+), 9 deletions(-)


Reviewed-by: Eric Blake 

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




Re: [PATCH 4/4] block/nbd: nbd_co_reconnect_loop(): don't connect if drained

2020-09-23 Thread Eric Blake

On 9/3/20 2:03 PM, Vladimir Sementsov-Ogievskiy wrote:

In a recent commit 12c75e20a269ac we've improved
nbd_co_reconnect_loop() to not make drain wait for additional sleep.
Similarly, we shouldn't try to connect, if previous sleep was
interrupted by drain begin, otherwise drain_begin will have to wait for
the whole connection attempt.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/nbd.c | 3 +++
  1 file changed, 3 insertions(+)


Reviewed-by: Eric Blake 



diff --git a/block/nbd.c b/block/nbd.c
index caae0e6d31..4548046cd7 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -661,6 +661,9 @@ static coroutine_fn void nbd_co_reconnect_loop(BDRVNBDState 
*s)
  } else {
  qemu_co_sleep_ns_wakeable(QEMU_CLOCK_REALTIME, timeout,
>connection_co_sleep_ns_state);
+if (s->drained) {
+continue;
+}
  if (timeout < max_timeout) {
  timeout *= 2;
  }



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




Re: [PATCH 2/4] block/nbd: correctly use qio_channel_detach_aio_context when needed

2020-09-23 Thread Eric Blake

On 9/3/20 2:02 PM, Vladimir Sementsov-Ogievskiy wrote:

Don't use nbd_client_detach_aio_context() driver handler where we want
to finalize the connection. We should directly use
qio_channel_detach_aio_context() in such cases. Driver handler may (and
will) contain another things, unrelated to the qio channel.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/nbd.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)


Reviewed-by: Eric Blake 

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




Re: [PATCH 1/4] block/nbd: fix drain dead-lock because of nbd reconnect-delay

2020-09-23 Thread Eric Blake

On 9/3/20 2:02 PM, Vladimir Sementsov-Ogievskiy wrote:

We pause reconnect process during drained section. So, if we have some
requests, waiting for reconnect we should cancel them, otherwise they
deadlock the drained section.

How to reproduce:




Now Qemu is trying to reconnect, and dd-generated requests are waiting
for the connection (they will wait up to 60 seconds (see
reconnect-delay option above) and than fail). But suddenly, vm may
totally hang in the deadlock. You may need to increase reconnect-delay
period to catch the dead-lock.

VM doesn't respond because drain dead-lock happens in cpu thread with
global mutex taken. That's not good thing by itself and is not fixed
by this commit (true way is using iothreads). Still this commit fixes
drain dead-lock itself.

Note: probably, we can instead continue to reconnect during drained
section. To achieve this, we may move negotiation to the connect thread
to make it independent of bs aio context. But expanding drained section
doesn't seem good anyway. So, let's now fix the bug the simplest way.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/nbd.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index 9daf003bea..912ea27be7 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -242,6 +242,11 @@ static void coroutine_fn 
nbd_client_co_drain_begin(BlockDriverState *bs)
  }
  
  nbd_co_establish_connection_cancel(bs, false);

+
+if (s->state == NBD_CLIENT_CONNECTING_WAIT) {
+s->state = NBD_CLIENT_CONNECTING_NOWAIT;
+qemu_co_queue_restart_all(>free_sema);
+}
  }
  


Reviewed-by: Eric Blake 

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




[PULL 1/1] qemu-img: Support bitmap --merge into backing image

2020-09-21 Thread Eric Blake
If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a
bitmap from top into base, qemu-img was failing with:

qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to 
get shared "write" lock
Is another process using the image [base.qcow2]?

The easiest fix is to not open the entire backing chain of either
image (source or destination); after all, the point of 'qemu-img
bitmap' is solely to manipulate bitmaps directly within a single qcow2
image, and this is made more precise if we don't pay attention to
other images in the chain that may happen to have a bitmap by the same
name.

However, note that on a case-by-case analysis, there _are_ times where
we treat it as a feature that we can access a bitmap from a backing
layer in association with an overlay BDS.  A demonstration of this is
using NBD to expose both an overlay BDS (for constant contents) and a
bitmap (for learning which blocks are interesting) during an
incremental backup:

Base <- Active <- Temporary
  \--block job ->/

where Temporary is being fed by a backup 'sync=none' job.  When
exposing Temporary over NBD, referring to a bitmap that lives only in
Active is less effort than having to copy a bitmap into Temporary [1].
So the testsuite additions in this patch check both where bitmaps get
allocated (the qemu-img info output), and that qemu-nbd is indeed able
to access a bitmap inherited from the backing chain since it is a
different use case than 'qemu-img bitmap'.

[1] Full disclosure: prior to the recent commit 374eedd1c4 and
friends, we were NOT able to see bitmaps through filters, which meant
that we actually did not have nice clean semantics for uniformly being
able to pick up bitmaps from anywhere in the backing chain (seen as a
change in behavior between qemu 4.1 and 4.2 at commit 00e30f05de, when
block-copy swapped from a one-off to a filter).  Which means libvirt
was already coded to copy bitmaps around for the sake of older qemu,
even though modern qemu no longer needs it.  Oh well.

Fixes: http://bugzilla.redhat.com/1877209
Reported-by: Eyal Shenitzky 
Signed-off-by: Eric Blake 
Message-Id: <20200914191009.644842-1-ebl...@redhat.com>
[eblake: more commit message tweaks, per Max Reitz review]
Reviewed-by: Max Reitz 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 qemu-img.c | 11 ++--
 tests/qemu-iotests/291 | 12 
 tests/qemu-iotests/291.out | 56 ++
 3 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 1d8c5cd778c0..d79c6ffa2e49 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4779,14 +4779,19 @@ static int img_bitmap(int argc, char **argv)
 filename = argv[optind];
 bitmap = argv[optind + 1];

-blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
-   false);
+/*
+ * No need to open backing chains; we will be manipulating bitmaps
+ * directly in this image without reference to image contents.
+ */
+blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR | BDRV_O_NO_BACKING,
+   false, false, false);
 if (!blk) {
 goto out;
 }
 bs = blk_bs(blk);
 if (src_filename) {
-src = img_open(false, src_filename, src_fmt, 0, false, false, false);
+src = img_open(false, src_filename, src_fmt, BDRV_O_NO_BACKING,
+   false, false, false);
 if (!src) {
 goto out;
 }
diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
index 1e0bb76959bb..4f837b205655 100755
--- a/tests/qemu-iotests/291
+++ b/tests/qemu-iotests/291
@@ -91,6 +91,15 @@ $QEMU_IMG bitmap --remove --image-opts \
 driver=$IMGFMT,file.driver=file,file.filename="$TEST_IMG" tmp
 _img_info --format-specific

+echo
+echo "=== Merge from top layer into backing image ==="
+echo
+
+$QEMU_IMG rebase -u -F qcow2 -b "$TEST_IMG.base" "$TEST_IMG"
+$QEMU_IMG bitmap --add --merge b2 -b "$TEST_IMG" -F $IMGFMT \
+ -f $IMGFMT "$TEST_IMG.base" b3
+_img_info --format-specific --backing-chain
+
 echo
 echo "=== Check bitmap contents ==="
 echo
@@ -107,6 +116,9 @@ $QEMU_IMG map --output=json --image-opts \
 nbd_server_start_unix_socket -r -f qcow2 -B b2 "$TEST_IMG"
 $QEMU_IMG map --output=json --image-opts \
 "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+nbd_server_start_unix_socket -r -f qcow2 -B b3 "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+"$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b3" | _filter_qemu_img_map

 # success, all done
 echo '*** done'
diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
index ee89a728855f..3990f7aacc7b 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/291.out
@@ -68,6 +68,59 @@ Format specific information:
 corrupt: false
 extended l2: fals

Re: [PATCH v2] qemu-img: Support bitmap --merge into backing image

2020-09-21 Thread Eric Blake

On 9/17/20 5:19 AM, Max Reitz wrote:


temporary over NBD, referring to a bitmap that lives only in Active is
less effort than having to copy a bitmap into temporary [1].  So the
testsuite additions in this patch check both where bitmaps get
allocated (the qemu-img info output), and, when NOT using 'qemu-img
bitmap', that bitmaps are indeed visible through a backing chain.


Well.  It is useful over NBD but I would doubt that it isn’t useful in
general.  For example, the QMP commands that refer to bitmaps always do
so through a node-name + bitmap-name combination, and they require that
the given bitmap is exactly on the given node.

So I think this is a very much a case-by-case question.  (And in
practice, NBD seems to be the outlier, not qemu-img bitmap.)



I'm happy to reword slightly to give that caveat.




The code looks good to me, but I wonder whether in the commit message it
should be noted that we don’t want to let bitmaps from deeper nodes
shine through by default everywhere, but just in specific cases where
that’s useful (i.e. only NBD so far AFAIA).


So is this a Reviewed-by?  I'm happy to queue it through my bitmaps
tree, if so.


It wasn’t meant as an R-b, because my R-b depends on how the commit
message addresses the question of when exactly bitmaps from the backing
chain should be visible on the top image.  Whether qemu-img bitmap is an
exception, or whether this is really a case-by-case question.

(I wanted to know whether you agree on including it.  Normally, I’m
happy to give an R-b on the basis of “with that done”, but in this case
I wasn’t entirely sure whether my request was reasonable, but I also
felt that in case it was, it wasn’t optional, given that you do have an
entire paragraph in the commit message dedicated to why the backing
image’s bitmap is visible on an image exported over NBD.)


Here's my rewording:

However, note that on a case-by-case analysis, there _are_ times where
we treat it as a feature that we can access a bitmap from a backing
layer in association with an overlay BDS.  A demonstration of this is
using NBD to expose both an overlay BDS (for constant contents) and a
bitmap (for learning which blocks are interesting) during an
incremental backup:

Base <- Active <- Temporary
  \--block job ->/

where Temporary is being fed by a backup 'sync=none' job.  When
exposing Temporary over NBD, referring to a bitmap that lives only in
Active is less effort than having to copy a bitmap into Temporary [1].
So the testsuite additions in this patch check both where bitmaps get
allocated (the qemu-img info output), and that qemu-nbd is indeed able
to access a bitmap inherited from the backing chain since it is a
different use case than 'qemu-img bitmap'.



I have to say I would like to see how you do phrase it in the end, but
given that you do agree on including it, I can give a

Reviewed-by: Max Reitz 

Now.


Okay, I think I've met your request, so I'll go ahead and send the pull 
request today.


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




Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions

2020-09-21 Thread Eric Blake

On 9/21/20 11:23 AM, Stefan Hajnoczi wrote:

clang's C11 atomic_fetch_*() functions only take a C11 atomic type
pointer argument. QEMU uses direct types (int, etc) and this causes a
compiler error when a QEMU code calls these functions in a source file
that also included  via a system header file:

   $ CC=clang CXX=clang++ ./configure ... && make
   ../util/async.c:79:17: error: address argument to atomic operation must be a 
pointer to _Atomic type ('unsigned int *' invalid)

Avoid using atomic_*() names in QEMU's atomic.h since that namespace is
used by . Prefix QEMU's APIs with qemu_ so that atomic.h
and  can co-exist.

This patch was generated using:

   $ git diff | grep -o '\/tmp/changed_identifiers


Missing a step in the recipe: namely, you probably modified 
include/qemu/atomic*.h prior to running 'git diff' (so that you actually 
had input to feed to grep -o).  But spelling it 'git diff HEAD^ 
include/qemu/atomic*.h | ...' does indeed give me a sane list of 
identifiers that looks like what you touched in the rest of the patch.



   $ for identifier in $(

Also not quite the right recipe, based on the file name used in the line 
above.



sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l 
"\<$identifier\>") \
 done



Fortunately, running "git grep -c '\pre-patch state of the tree gives me a list that is somewhat close to 
yours, where the obvious difference in line counts is explained by:



I manually fixed line-wrap issues and misaligned rST tables.

Signed-off-by: Stefan Hajnoczi 
---


First, focusing on the change summary:


  docs/devel/lockcnt.txt|  14 +-
  docs/devel/rcu.txt|  40 +--
  accel/tcg/atomic_template.h   |  20 +-
  include/block/aio-wait.h  |   4 +-
  include/block/aio.h   |   8 +-
  include/exec/cpu_ldst.h   |   2 +-
  include/exec/exec-all.h   |   6 +-
  include/exec/log.h|   6 +-
  include/exec/memory.h |   2 +-
  include/exec/ram_addr.h   |  27 +-
  include/exec/ramlist.h|   2 +-
  include/exec/tb-lookup.h  |   4 +-
  include/hw/core/cpu.h |   2 +-
  include/qemu/atomic.h | 258 +++---
  include/qemu/atomic128.h  |   6 +-


These two are the most important for the sake of this patch; perhaps 
it's worth a temporary override of your git orderfile if you have to 
respin, to list them first?



  include/qemu/bitops.h |   2 +-
  include/qemu/coroutine.h  |   2 +-
  include/qemu/log.h|   6 +-
  include/qemu/queue.h  |   8 +-
  include/qemu/rcu.h|  10 +-
  include/qemu/rcu_queue.h  | 103 +++---


Presumably, this and any other file with an odd number of changes was 
due to a difference in lines after reformatting long lines.



  include/qemu/seqlock.h|   8 +-

...


  util/stats64.c|  34 +-
  docs/devel/atomics.rst| 326 +-
  .../opensbi-riscv32-generic-fw_dynamic.elf| Bin 558668 -> 558698 bytes
  .../opensbi-riscv64-generic-fw_dynamic.elf| Bin 620424 -> 620454 bytes


Why are we regenerating .elf files in this patch?  Is your change even 
correct for those two files?



  scripts/kernel-doc|   2 +-
  tcg/aarch64/tcg-target.c.inc  |   2 +-
  tcg/mips/tcg-target.c.inc |   2 +-
  tcg/ppc/tcg-target.c.inc  |   6 +-
  tcg/sparc/tcg-target.c.inc|   5 +-
  135 files changed, 1195 insertions(+), 1130 deletions(-)


I don't spot accel/tcg/atomic_common.c.inc in the list (which declares 
functions such as atomic_trace_rmw_pre) - I guess that's intentional 
based on how you tried to edit only the identifiers you touched in 
include/qemu/atomic*.h.


For the rest of this patch, I only spot-checked in places, trusting the 
mechanical nature of this patch, and not spotting anything wrong in the 
places I checked.  But the two .elf files worry me enough to withhold 
R-b.  At the same time, because it's big, it will probably be a source 
of conflicts if we don't get it in soon, but can also be regenerated (if 
your recipe is corrected) without too much difficulty.  So I am in favor 
of the idea.



diff --git a/pc-bios/opensbi-riscv32-generic-fw_dynamic.elf 
b/pc-bios/opensbi-riscv32-generic-fw_dynamic.elf
index 
eb9ebf5674d3953ab25de6bf4db134abe904eb20..35b80512446dcf5c49424ae90caacf3c579be1b5
 100644
GIT binary patch
delta 98
zcmX@pqx7mrsiB3jg{g(Pg=Gt?!4uZP)ZEhe?LdZ@5QIJ5?Hg+mgxS918!HgAZQt>Y
ceSHN~X?i|K5fhYsvykI97nHrFhGPaN0Hp#a^8f$<

delta 62

Re: [PATCH v4 9/9] migration: introduce snapshot-{save, load, delete} QMP commands

2020-09-21 Thread Eric Blake

On 9/16/20 3:17 AM, Markus Armbruster wrote:

Daniel P. Berrangé  writes:


savevm, loadvm and delvm are some of the few HMP commands that have never
been converted to use QMP. The reasons for the lack of conversion are
that they blocked execution of the event thread, and the semantics
around choice of disks were ill-defined.

Despite this downside, however, libvirt and applications using libvirt
have used these commands for as long as QMP has existed, via the
"human-monitor-command" passthrough command. IOW, while it is clearly
desirable to be able to fix the problems, they are not a blocker to
all real world usage.

Meanwhile there is a need for other features which involve adding new
parameters to the commands. This is possible with HMP passthrough, but
it provides no reliable way for apps to introspect features, so using
QAPI modelling is highly desirable.

This patch thus introduces new snapshot-{load,save,delete} commands to
QMP that are intended to replace the old HMP counterparts. The new
commands are given different names, because they will be using the new
QEMU job framework and thus will have diverging behaviour from the HMP
originals. It would thus be misleading to keep the same name.

While this design uses the generic job framework, the current impl is
still blocking. The intention that the blocking problem is fixed later.
None the less applications using these new commands should assume that
they are asynchronous and thus wait for the job status change event to
indicate completion.

In addition to using the job framework, the new commands require the
caller to be explicit about all the block device nodes used in the
snapshot operations, with no built-in default heuristics in use.

Signed-off-by: Daniel P. Berrangé 

[...]

diff --git a/qapi/job.json b/qapi/job.json
index 280c2f76f1..b2cbb4fead 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -22,10 +22,17 @@
  #
  # @amend: image options amend job type, see "x-blockdev-amend" (since 5.1)
  #
+# @snapshot-load: snapshot load job type, see "snapshot-load" (since 5.2)
+#
+# @snapshot-save: snapshot save job type, see "snapshot-save" (since 5.2)
+#
+# @snapshot-delete: snapshot delete job type, see "snapshot-delete" (since 5.2)
+#
  # Since: 1.7
  ##
  { 'enum': 'JobType',
-  'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
+  'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend',
+   'snapshot-load', 'snapshot-save', 'snapshot-delete'] }
  
  ##

  # @JobStatus:
diff --git a/qapi/migration.json b/qapi/migration.json
index 675f70bb67..b584c0be31 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1720,3 +1720,123 @@
  ##
  { 'event': 'UNPLUG_PRIMARY',
'data': { 'device-id': 'str' } }
+
+##
+# @snapshot-save:
+#
+# Save a VM snapshot
+#
+# @job-id: identifier for the newly created job
+# @tag: name of the snapshot to create
+# @devices: list of block device node names to save a snapshot to


Looks like you dropped the idea to also accept drive IDs.  Is that for
good, or would you like to add it later?


Is it necessary?  Several of our newer block interfaces have required 
node names, rather than permitting alternation.  If we rewrite the 
existing HMP commands to operate on top of the new QMP command, it is 
still possible for HMP to support drive names even when QMP does not.  I 
don't think the complexity of worrying about drive names is worth it; 
after all, the QMP command is new enough that the only libvirt that will 
use it is also a libvirt that knows how to use -blockdev, and thus node 
names are sufficient.


Yes, we can add drive ids later if I turn out to be wrong, but for now, 
I'm hoping their exclusion is intentional.


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




Re: [PATCH v2] qemu-img: Support bitmap --merge into backing image

2020-09-16 Thread Eric Blake

On 9/15/20 3:57 AM, Max Reitz wrote:

On 14.09.20 21:10, Eric Blake wrote:

If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a
bitmap from top into base, qemu-img was failing with:

qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to get shared 
"write" lock
Is another process using the image [base.qcow2]?

The easiest fix is to not open the entire backing chain of either
image (source or destination); after all, the point of 'qemu-img
bitmap' is solely to manipulate bitmaps directly within a single qcow2
image, and this is made more precise if we don't pay attention to
other images in the chain that may happen to have a bitmap by the same
name.

However, note that during normal usage, it is a feature that qemu will
allow a bitmap from a backing image to be exposed by an overlay BDS;
doing so makes it easier to perform incremental backup, where we have:

Base <- Active <- temporrary


*temporary

(Also it’s a bit strange that “Base” and “Active” are capitalized, but
“temporary” isn’t)


   \--block job ->/

with temporary being fed by a block-copy 'sync' job; when exposing


s/block-copy 'sync'/backup 'sync=none'/?


Will fix both.




temporary over NBD, referring to a bitmap that lives only in Active is
less effort than having to copy a bitmap into temporary [1].  So the
testsuite additions in this patch check both where bitmaps get
allocated (the qemu-img info output), and, when NOT using 'qemu-img
bitmap', that bitmaps are indeed visible through a backing chain.


Well.  It is useful over NBD but I would doubt that it isn’t useful in
general.  For example, the QMP commands that refer to bitmaps always do
so through a node-name + bitmap-name combination, and they require that
the given bitmap is exactly on the given node.

So I think this is a very much a case-by-case question.  (And in
practice, NBD seems to be the outlier, not qemu-img bitmap.)



I'm happy to reword slightly to give that caveat.


[1] Full disclosure: prior to the recent commit 374eedd1c4 and
friends, we were NOT able to see bitmaps through filters, which meant
that we actually did not have nice clean semantics for uniformly being
able to pick up bitmaps from anywhere in the backing chain (seen as a
change in behavior between qemu 4.1 and 4.2 at commit 00e30f05de, when
block-copy swapped from a one-off to a filter).  Which means libvirt
was already coded to copy bitmaps around for the sake of older qemu,
even though modern qemu no longer needs it.  Oh well.

Fixes: http://bugzilla.redhat.com/1877209
Reported-by: Eyal Shenitzky 
Signed-off-by: Eric Blake 
---

In v2:
- also use BDRV_O_NO_BACKING on source [Max]
- improved commit message [Max]

  qemu-img.c | 11 ++--
  tests/qemu-iotests/291 | 12 
  tests/qemu-iotests/291.out | 56 ++
  3 files changed, 76 insertions(+), 3 deletions(-)


The code looks good to me, but I wonder whether in the commit message it
should be noted that we don’t want to let bitmaps from deeper nodes
shine through by default everywhere, but just in specific cases where
that’s useful (i.e. only NBD so far AFAIA).


So is this a Reviewed-by?  I'm happy to queue it through my bitmaps 
tree, if so.



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




[PATCH v2] qemu-img: Support bitmap --merge into backing image

2020-09-14 Thread Eric Blake
If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a
bitmap from top into base, qemu-img was failing with:

qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to 
get shared "write" lock
Is another process using the image [base.qcow2]?

The easiest fix is to not open the entire backing chain of either
image (source or destination); after all, the point of 'qemu-img
bitmap' is solely to manipulate bitmaps directly within a single qcow2
image, and this is made more precise if we don't pay attention to
other images in the chain that may happen to have a bitmap by the same
name.

However, note that during normal usage, it is a feature that qemu will
allow a bitmap from a backing image to be exposed by an overlay BDS;
doing so makes it easier to perform incremental backup, where we have:

Base <- Active <- temporrary
  \--block job ->/

with temporary being fed by a block-copy 'sync' job; when exposing
temporary over NBD, referring to a bitmap that lives only in Active is
less effort than having to copy a bitmap into temporary [1].  So the
testsuite additions in this patch check both where bitmaps get
allocated (the qemu-img info output), and, when NOT using 'qemu-img
bitmap', that bitmaps are indeed visible through a backing chain.

[1] Full disclosure: prior to the recent commit 374eedd1c4 and
friends, we were NOT able to see bitmaps through filters, which meant
that we actually did not have nice clean semantics for uniformly being
able to pick up bitmaps from anywhere in the backing chain (seen as a
change in behavior between qemu 4.1 and 4.2 at commit 00e30f05de, when
block-copy swapped from a one-off to a filter).  Which means libvirt
was already coded to copy bitmaps around for the sake of older qemu,
even though modern qemu no longer needs it.  Oh well.

Fixes: http://bugzilla.redhat.com/1877209
Reported-by: Eyal Shenitzky 
Signed-off-by: Eric Blake 
---

In v2:
- also use BDRV_O_NO_BACKING on source [Max]
- improved commit message [Max]

 qemu-img.c | 11 ++--
 tests/qemu-iotests/291 | 12 
 tests/qemu-iotests/291.out | 56 ++
 3 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index d0b1c97562e8..48d35a2f23a1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4757,14 +4757,19 @@ static int img_bitmap(int argc, char **argv)
 filename = argv[optind];
 bitmap = argv[optind + 1];

-blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR, false, false,
-   false);
+/*
+ * No need to open backing chains; we will be manipulating bitmaps
+ * directly in this image without reference to image contents.
+ */
+blk = img_open(image_opts, filename, fmt, BDRV_O_RDWR | BDRV_O_NO_BACKING,
+   false, false, false);
 if (!blk) {
 goto out;
 }
 bs = blk_bs(blk);
 if (src_filename) {
-src = img_open(false, src_filename, src_fmt, 0, false, false, false);
+src = img_open(false, src_filename, src_fmt, BDRV_O_NO_BACKING,
+   false, false, false);
 if (!src) {
 goto out;
 }
diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
index 1e0bb76959bb..4f837b205655 100755
--- a/tests/qemu-iotests/291
+++ b/tests/qemu-iotests/291
@@ -91,6 +91,15 @@ $QEMU_IMG bitmap --remove --image-opts \
 driver=$IMGFMT,file.driver=file,file.filename="$TEST_IMG" tmp
 _img_info --format-specific

+echo
+echo "=== Merge from top layer into backing image ==="
+echo
+
+$QEMU_IMG rebase -u -F qcow2 -b "$TEST_IMG.base" "$TEST_IMG"
+$QEMU_IMG bitmap --add --merge b2 -b "$TEST_IMG" -F $IMGFMT \
+ -f $IMGFMT "$TEST_IMG.base" b3
+_img_info --format-specific --backing-chain
+
 echo
 echo "=== Check bitmap contents ==="
 echo
@@ -107,6 +116,9 @@ $QEMU_IMG map --output=json --image-opts \
 nbd_server_start_unix_socket -r -f qcow2 -B b2 "$TEST_IMG"
 $QEMU_IMG map --output=json --image-opts \
 "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+nbd_server_start_unix_socket -r -f qcow2 -B b3 "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+"$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b3" | _filter_qemu_img_map

 # success, all done
 echo '*** done'
diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
index ee89a728855f..3990f7aacc7b 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/291.out
@@ -68,6 +68,59 @@ Format specific information:
 corrupt: false
 extended l2: false

+=== Merge from top layer into backing image ===
+
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.base
+backing file format: IMGFMT
+Format specific information:
+compat: 1.1
+compression type: zl

Re: [PATCH v3] iotests: Drop readlink -f

2020-09-14 Thread Eric Blake

On 9/14/20 9:56 AM, Max Reitz wrote:

On macOS, (out of the box) readlink does not have -f.  We do not really
need readlink here, though, it was just a replacement for realpath
(which is not available on our BSD test systems), which we needed to
make the $(dirname) into an absolute path.

Instead of using either, just use "cd; pwd" like is done for
$source_iotests.

Fixes: b1cbc33a3971b6bb005d5ac3569feae35a71de0f
("iotests: Allow running from different directory")
Suggested-by: Peter Maydell 
Reported-by: Claudio Fontana 
Reported-by: Thomas Huth 
Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/check | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
index e14a1f354d..678b6e4910 100755
--- a/tests/qemu-iotests/check
+++ b/tests/qemu-iotests/check
@@ -44,7 +44,7 @@ then
  _init_error "failed to obtain source tree name from check symlink"
  fi
  source_iotests=$(cd "$source_iotests"; pwd) || _init_error "failed to enter 
source tree"
-build_iotests=$(readlink -f $(dirname "$0"))
+build_iotests=$(cd "$(dirname "$0")"; pwd)


If CDPATH is set, this can produce wrong results.  Do we care?  Probably 
not, since (as you point out), $source_iotests has the same bug, and no 
one has complained yet.


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




Re: [PATCH] tests/check-block: Do not run the iotests with old versions of bash

2020-09-14 Thread Eric Blake

On 9/14/20 10:33 AM, Thomas Huth wrote:

On 14/09/2020 17.03, Eric Blake wrote:

On 9/12/20 7:14 AM, Thomas Huth wrote:

macOS is shipped with a very old version of the bash (3.2), which
is currently not suitable for running the iotests anymore. Add
a check to skip the iotests in this case - if someone still wants
to run the iotests on macOS, they can install a newer version from
homebrew, for example.

Signed-off-by: Thomas Huth 
---
   tests/check-block.sh | 5 +
   1 file changed, 5 insertions(+)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 8e29c868e5..bfe1630c1e 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -46,6 +46,11 @@ if ! command -v bash >/dev/null 2>&1 ; then
   exit 0
   fi
   +if bash --version | grep 'GNU bash, version [123]' > /dev/null 2>&1
; then


We're already running bash - why do we need to spawn another bash and a
grep, when we can just check $BASH_VERSION?


tests/check-block.sh uses "#!/bin/sh", so it could be running with a
different kind of shell, I think.


Ah, right; I'm so used to most of our testsuite demanding bash that I 
forgot to check that this script sticks to the smaller subset of /bin/sh 
portability.


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




Re: [PATCH v3] iotests: Drop readlink -f

2020-09-14 Thread Eric Blake

On 9/14/20 10:43 AM, Thomas Huth wrote:


+++ b/tests/qemu-iotests/check
@@ -44,7 +44,7 @@ then
  _init_error "failed to obtain source tree name from check symlink"
  fi
  source_iotests=$(cd "$source_iotests"; pwd) || _init_error "failed to enter 
source tree"
-build_iotests=$(readlink -f $(dirname "$0"))
+build_iotests=$(cd "$(dirname "$0")"; pwd)


I assume the nested quotes are ok here? ... shell scripts always confuse
me in that regard...


Yes.  Anything inside $() is a standalone script.  That's one of the 
reasons that $() is nicer than `` (where you did indeed have to worry 
about " inside `` interfering with "``" on the outside, in some shells).


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




Re: [PATCH] tests/check-block: Do not run the iotests with old versions of bash

2020-09-14 Thread Eric Blake

On 9/12/20 7:14 AM, Thomas Huth wrote:

macOS is shipped with a very old version of the bash (3.2), which
is currently not suitable for running the iotests anymore. Add
a check to skip the iotests in this case - if someone still wants
to run the iotests on macOS, they can install a newer version from
homebrew, for example.

Signed-off-by: Thomas Huth 
---
  tests/check-block.sh | 5 +
  1 file changed, 5 insertions(+)

diff --git a/tests/check-block.sh b/tests/check-block.sh
index 8e29c868e5..bfe1630c1e 100755
--- a/tests/check-block.sh
+++ b/tests/check-block.sh
@@ -46,6 +46,11 @@ if ! command -v bash >/dev/null 2>&1 ; then
  exit 0
  fi
  
+if bash --version | grep 'GNU bash, version [123]' > /dev/null 2>&1 ; then


We're already running bash - why do we need to spawn another bash and a 
grep, when we can just check $BASH_VERSION?


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




Re: [PATCH v8 11/27] meson: Use -b to ignore CR vs. CR-LF issues on Windows

2020-09-14 Thread Eric Blake

On 9/13/20 10:35 AM, Philippe Mathieu-Daudé wrote:

On 9/13/20 12:44 AM, Yonggang Luo wrote:

On windows, a difference in line endings causes testsuite failures
complaining that every single line in files such as
'tests/qapi-schemadoc-good.texi' is wrong.  Fix it by adding -b to diff.


Isn't '--strip-trailing-cr' more adapted?


No, BSD lacks that spelling.

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




Re: [PATCH] qemu-img: Support bitmap --merge into backing image

2020-09-11 Thread Eric Blake

On 9/11/20 3:31 AM, Max Reitz wrote:

On 09.09.20 14:33, Eric Blake wrote:

If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a
bitmap from top into base, qemu-img was failing with:

qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to get shared 
"write" lock
Is another process using the image [base.qcow2]?

The easiest fix is to not open the entire backing chain of the source
image, so that we aren't worrying about competing BDS visiting the
backing image as both a read-only backing of the source and the
writeable destination.

Fixes: http://bugzilla.redhat.com/1877209
Reported-by: Eyal Shenitzky 
Signed-off-by: Eric Blake 
---
  qemu-img.c |  3 +-
  tests/qemu-iotests/291 | 12 
  tests/qemu-iotests/291.out | 56 ++
  3 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index eb2fc1f86243..b15098a2f9b3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4755,7 +4755,8 @@ static int img_bitmap(int argc, char **argv)
  }
  bs = blk_bs(blk);
  if (src_filename) {
-src = img_open(false, src_filename, src_fmt, 0, false, false, false);
+src = img_open(false, src_filename, src_fmt, BDRV_O_NO_BACKING,
+   false, false, false);


Why not do the same for the destination BB?


Yeah, that should work, too.




  if (!src) {
  goto out;
  }
diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
index 1e0bb76959bb..4f837b205655 100755
--- a/tests/qemu-iotests/291
+++ b/tests/qemu-iotests/291


[...]


@@ -107,6 +116,9 @@ $QEMU_IMG map --output=json --image-opts \
  nbd_server_start_unix_socket -r -f qcow2 -B b2 "$TEST_IMG"
  $QEMU_IMG map --output=json --image-opts \
  "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+nbd_server_start_unix_socket -r -f qcow2 -B b3 "$TEST_IMG"


Why not look into $TEST_IMG.base to see specifically whether the bitmap
is there?


We did just that, several lines earlier, with the qemu-img info 
--backing-chain.




(I also am quite surprised that it’s even possible to export bitmaps
from backing nodes, but, well.)


I actually ought to call that out in the commit message.  It used to be 
that we were inconsistent on what we could see from the backing chain (a 
filter would make it so we can't), but as soon as your filter patches 
land, then we _do_ want to always be able to find a bitmap from the 
backing chain (incremental backup depends on that: we create an overlay 
disk to run the block-copy job as a filter, and _want_ to expose that 
overlay image with the bitmap it inherits from the original image).  So 
being able to export bitmaps from a backing node is normally a feature; 
and it is only in 'qemu-img bitmap' where we don't want accidental 
inheritance to get in the way from what we are actually merging.


I'll send a v2.

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




Re: [PULL v2] Block layer patches

2020-09-09 Thread Eric Blake

On 9/9/20 4:55 PM, Peter Maydell wrote:



This fails 'make check' on NetBSD and OpenBSD:

./check: line 47: realpath: command not found
./check: line 60: /common.env: No such file or directory
check: failed to source common.env (make sure the qemu-iotests are run
from tests/qemu-iotests in the build tree)
gmake: *** [/home/qemu/qemu-test.vcb7nz/src/tests/Makefile.include:144:
check-block] Error 1


BSD has 'readlink -f' (and so does coreutils on Linux), which does the 
same thing as the Linux-only realpath.


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




Re: [PATCH] qcow2: Return the original error code in qcow2_co_pwrite_zeroes()

2020-09-09 Thread Eric Blake

On 9/9/20 7:37 AM, Alberto Garcia wrote:

This function checks the current status of a (sub)cluster in order to
see if an unaligned 'write zeroes' request can be done efficiently by
simply updating the L2 metadata and without having to write actual
zeroes to disk.

If the situation does not allow using the fast path then the function
returns -ENOTSUP and the caller falls back to writing zeroes.

If can happen however that the aforementioned check returns an actual
error code so in this case we should pass it to the caller.

Signed-off-by: Alberto Garcia 
---
  block/qcow2.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Eric Blake 

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




[PATCH] qemu-img: Support bitmap --merge into backing image

2020-09-09 Thread Eric Blake
If you have the chain 'base.qcow2 <- top.qcow2' and want to merge a
bitmap from top into base, qemu-img was failing with:

qemu-img: Could not open 'top.qcow2': Could not open backing file: Failed to 
get shared "write" lock
Is another process using the image [base.qcow2]?

The easiest fix is to not open the entire backing chain of the source
image, so that we aren't worrying about competing BDS visiting the
backing image as both a read-only backing of the source and the
writeable destination.

Fixes: http://bugzilla.redhat.com/1877209
Reported-by: Eyal Shenitzky 
Signed-off-by: Eric Blake 
---
 qemu-img.c |  3 +-
 tests/qemu-iotests/291 | 12 
 tests/qemu-iotests/291.out | 56 ++
 3 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index eb2fc1f86243..b15098a2f9b3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4755,7 +4755,8 @@ static int img_bitmap(int argc, char **argv)
 }
 bs = blk_bs(blk);
 if (src_filename) {
-src = img_open(false, src_filename, src_fmt, 0, false, false, false);
+src = img_open(false, src_filename, src_fmt, BDRV_O_NO_BACKING,
+   false, false, false);
 if (!src) {
 goto out;
 }
diff --git a/tests/qemu-iotests/291 b/tests/qemu-iotests/291
index 1e0bb76959bb..4f837b205655 100755
--- a/tests/qemu-iotests/291
+++ b/tests/qemu-iotests/291
@@ -91,6 +91,15 @@ $QEMU_IMG bitmap --remove --image-opts \
 driver=$IMGFMT,file.driver=file,file.filename="$TEST_IMG" tmp
 _img_info --format-specific

+echo
+echo "=== Merge from top layer into backing image ==="
+echo
+
+$QEMU_IMG rebase -u -F qcow2 -b "$TEST_IMG.base" "$TEST_IMG"
+$QEMU_IMG bitmap --add --merge b2 -b "$TEST_IMG" -F $IMGFMT \
+ -f $IMGFMT "$TEST_IMG.base" b3
+_img_info --format-specific --backing-chain
+
 echo
 echo "=== Check bitmap contents ==="
 echo
@@ -107,6 +116,9 @@ $QEMU_IMG map --output=json --image-opts \
 nbd_server_start_unix_socket -r -f qcow2 -B b2 "$TEST_IMG"
 $QEMU_IMG map --output=json --image-opts \
 "$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b2" | _filter_qemu_img_map
+nbd_server_start_unix_socket -r -f qcow2 -B b3 "$TEST_IMG"
+$QEMU_IMG map --output=json --image-opts \
+"$IMG,x-dirty-bitmap=qemu:dirty-bitmap:b3" | _filter_qemu_img_map

 # success, all done
 echo '*** done'
diff --git a/tests/qemu-iotests/291.out b/tests/qemu-iotests/291.out
index ee89a728855f..3990f7aacc7b 100644
--- a/tests/qemu-iotests/291.out
+++ b/tests/qemu-iotests/291.out
@@ -68,6 +68,59 @@ Format specific information:
 corrupt: false
 extended l2: false

+=== Merge from top layer into backing image ===
+
+image: TEST_DIR/t.IMGFMT
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+cluster_size: 65536
+backing file: TEST_DIR/t.IMGFMT.base
+backing file format: IMGFMT
+Format specific information:
+compat: 1.1
+compression type: zlib
+lazy refcounts: false
+bitmaps:
+[0]:
+flags:
+name: b1
+granularity: 524288
+[1]:
+flags:
+[0]: auto
+name: b2
+granularity: 65536
+[2]:
+flags:
+name: b0
+granularity: 65536
+refcount bits: 16
+corrupt: false
+extended l2: false
+
+image: TEST_DIR/t.IMGFMT.base
+file format: IMGFMT
+virtual size: 10 MiB (10485760 bytes)
+cluster_size: 65536
+Format specific information:
+compat: 1.1
+compression type: zlib
+lazy refcounts: false
+bitmaps:
+[0]:
+flags:
+[0]: auto
+name: b0
+granularity: 65536
+[1]:
+flags:
+[0]: auto
+name: b3
+granularity: 65536
+refcount bits: 16
+corrupt: false
+extended l2: false
+
 === Check bitmap contents ===

 [{ "start": 0, "length": 3145728, "depth": 0, "zero": false, "data": true, 
"offset": OFFSET},
@@ -79,4 +132,7 @@ Format specific information:
 [{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, 
"offset": OFFSET},
 { "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": 
false},
 { "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": 
true, "offset": OFFSET}]
+[{ "start": 0, "length": 2097152, "depth": 0, "zero": false, "data": true, 
"offset": OFFSET},
+{ "start": 2097152, "length": 1048576, "depth": 0, "zero": false, "data": 
false},
+{ "start": 3145728, "length": 7340032, "depth": 0, "zero": false, "data": 
true, "offset": OFFSET}]
 *** done
-- 
2.28.0




Re: [PATCH 00/16] W32, W64 patches

2020-09-08 Thread Eric Blake

On 9/8/20 2:48 PM, Yonggang Luo wrote:

It first introduce msys2 CI on cirrus by fixes nfs, capstone, curses and
disable partial test-char tests.
And then fixes a number of unit tests failure on msys2/mingw


Please remember to include a version number (v2, v3, ...) if this is an 
improved posting of an earlier revision of the patch series.  'git 
send-email -v2' does that automatically, for example.


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




Re: [PULL 03/16] configure: Fixes ncursesw detection under msys2/mingw and enable curses

2020-09-08 Thread Eric Blake

On 9/8/20 1:49 PM, Yonggang Luo wrote:

The mingw pkg-config are showing following absolute path and contains : as the 
separator,
so we must handling : properly.

-D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L 
-IC:/CI-Tools/msys64/mingw64/include/ncursesw:-I/usr/include/ncursesw:
-DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC -pipe 
-lncursesw -lgnurx -ltre -lintl -liconv
-DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC -lncursesw
-DNCURSES_WIDECHAR -D_XOPEN_SOURCE=600 -D_POSIX_C_SOURCE=199506L -IC -lcursesw
-DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -pipe -lncursesw 
-lgnurx -ltre -lintl -liconv
-DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lncursesw
-DNCURSES_WIDECHAR /CI-Tools/msys64/mingw64/include/ncursesw -lcursesw
-DNCURSES_WIDECHAR -I/usr/include/ncursesw -pipe -lncursesw -lgnurx -ltre 
-lintl -liconv
-DNCURSES_WIDECHAR -I/usr/include/ncursesw -lncursesw
-DNCURSES_WIDECHAR -I/usr/include/ncursesw -lcursesw

MINGW doesn't have langinfo.h, only exist in glic and musl


Grammar and spelling are off; if I were keeping the sentence as-is, I 
would use:


MINGW lacks langinfo.h, which only exists in glibc and musl

But that statement is a lie, since POSIX requires  and we 
build on BSD systems which use neither glibc nor musl.  So better would be:


mingw lacks the POSIX-required langinfo.h.

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




Re: [PULL 02/16] ci: fixes msys2 build by upgrading capstone to 4.0.2

2020-09-08 Thread Eric Blake

On 9/8/20 1:49 PM, Yonggang Luo wrote:

Signed-off-by: Yonggang Luo 
---
  capstone  | 2 +-
  configure | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)


The commit message is sparse; it might be nice to give more details 
about what error is fixed, or possibly even mention of the fact of which 
capstone commit id that is in 4.0.2 but not the current version matters 
to qemu on msys.




diff --git a/capstone b/capstone
index 22ead3e0bf..1d23053284 16
--- a/capstone
+++ b/capstone
@@ -1 +1 @@
-Subproject commit 22ead3e0bfdb87516656453336160e0a37b066bf
+Subproject commit 1d230532840a37ac032c6ab80128238fc930c6c1
diff --git a/configure b/configure
index 4231d56bcc..f4f8bc3756 100755
--- a/configure
+++ b/configure
@@ -5156,7 +5156,7 @@ case "$capstone" in
LIBCAPSTONE=libcapstone.a
  fi
  capstone_libs="-Lcapstone -lcapstone"
-capstone_cflags="-I${source_path}/capstone/include"
+capstone_cflags="-I${source_path}/capstone/include 
-I${source_path}/capstone/include/capstone"


This change was not mentioned in the commit message.  Did capstone 4.0.2 
change where its include files live?  Or is it a separate bug, and you 
are fixing two things at once (in which case, doing two separate commits 
might be nicer)?


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




Re: [PULL 01/16] block: Fixes nfs on msys2/mingw

2020-09-08 Thread Eric Blake

On 9/8/20 1:49 PM, Yonggang Luo wrote:

Signed-off-by: Yonggang Luo 
---
  block/nfs.c | 26 +-
  1 file changed, 17 insertions(+), 9 deletions(-)


The commit message is too sparse.  You say that nfs is broken, but fail 
to give any details how, or why this patch improves the situation. 
Also, the patch has no 'Reviewed-by' tag from anyone, and accepting 
unreviewed code into mainline can be risky (we tolerate it from 
well-established contributors, but you are still working toward that point).


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




Re: [PULL 00/16] Msys2 patches patches

2020-09-08 Thread Eric Blake

On 9/8/20 1:49 PM, Yonggang Luo wrote:

The following changes since commit 6779038537360e957dbded830f76b08ef5070161:

   Merge remote-tracking branch 'remotes/armbru/tags/pull-qapi-2020-09-08' int=
o staging (2020-09-08 17:23:31 +0100)

are available in the Git repository at:

   http://github.com/lygstate/qemu tags/msys2-patches-pull-request

for you to fetch changes up to 1892e4360f55ac8cbeeeae0043e0a9dc05c50269:

   rcu: add uninit destructor for rcu (2020-09-09 02:34:59 +0800)


msys2 patch queue 2020-09-09


MAINTAINERS doesn't mention a category for msys2, and this patch series 
doesn't add one.  It is unusual to send a pull request without being a 
listed maintainer for the code in question.  I'm not objecting to you 
taking on a maintainership role, if we are ready for that, but I also 
worry that you have so few contributions currently in the main 
repository that you have not necessarily proven the quality of your 
work.  Maybe it's better to just send this as an ordinary patch series, 
and let an existing maintainer incorporate it?


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




Re: [PATCH v7 1/8] Introduce yank feature

2020-09-04 Thread Eric Blake

On 9/4/20 7:33 AM, Lukas Straub wrote:


+##
+# @YankInstances:
+#
+# @instances: List of yank instances.
+#
+# Yank instances are named after the following schema:
+# "blockdev:", "chardev:" and "migration"
+#
+# Since: 5.1
+##
+{ 'struct': 'YankInstances', 'data': {'instances': ['str'] } }


I'm afraid this is a problematic QMP interface.

By making YankInstances a struct, you keep the door open to adding more
members, which is good.

But by making its 'instances' member a ['str'], you close the door to
using anything but a single string for the individual instances.  Not so
good.

The single string encodes information which QMP client will need to
parse from the string.  We frown on that in QMP.  Use QAPI complex types
capabilities for structured data.

Could you use something like this instead?

{ 'enum': 'YankInstanceType',
   'data': { 'block-node', 'chardev', 'migration' } }

{ 'struct': 'YankInstanceBlockNode',
   'data': { 'node-name': 'str' } }

{ 'struct': 'YankInstanceChardev',
   'data' { 'label': 'str' } }

{ 'union': 'YankInstance',
   'base': { 'type': 'YankInstanceType' },
   'discriminator': 'type',
   'data': {
   'block-node': 'YankInstanceBlockNode',
   'chardev': 'YankInstanceChardev' } }

{ 'command': 'yank',
   'data': { 'instances': ['YankInstance'] },
   'allow-oob': true }


This proposal looks good to me. Does everyone agree?


Yes; this is also more introspectible, so that if we add more yank 
instances down the road, or even more optional features to existing yank 
instances, it becomes easier to detect whether a particular qemu has 
those additions.


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




Re: [PATCH v2 3/3] nbd: disable signals and forking on Windows builds

2020-09-02 Thread Eric Blake

On 9/2/20 5:07 PM, 罗勇刚(Yonggang Luo) wrote:

On Tue, Aug 25, 2020 at 6:40 PM Daniel P. Berrangé 
wrote:


Disabling these parts are sufficient to get the qemu-nbd program
compiling in a Windows build.

Signed-off-by: Daniel P. Berrangé 
---
  meson.build | 7 ++-
  qemu-nbd.c  | 5 +
  2 files changed, 7 insertions(+), 5 deletions(-)



+++ b/qemu-nbd.c
@@ -899,6 +899,7 @@ int main(int argc, char **argv)
  #endif

  if ((device && !verbose) || fork_process) {
+#ifndef WIN32
  int stderr_fd[2];
  pid_t pid;
  int ret;
@@ -962,6 +963,10 @@ int main(int argc, char **argv)
   */
  exit(errors);
  }
+#else /* WIN32 */
+error_report("Unable to fork into background on Windows hosts");
+exit(EXIT_FAILURE);
+#endif /* WIN32 */
  }


May us replace fork with alternative such as spawn?


You're certainly welcome to propose a patch along those lines, if 
spawning a task is a common Windows counterpart to the Unix notion of 
forking off a daemon.  But even requiring qemu-nbd to run in the 
foreground is already an improvement over what we had previously, so any 
change to use spawn will be a separate series, and will not hold up this 
one.


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




[PULL 2/6] iotests/259: Fix reference output

2020-09-02 Thread Eric Blake
From: Max Reitz 

The error message has changed recently, breaking the test.  Fix it.

Fixes: a2b333c01880f56056d50c238834d62e32001e54
   ("block: nbd: Fix convert qcow2 compressed to nbd")
Signed-off-by: Max Reitz 
Message-Id: <20200811080830.289136-1-mre...@redhat.com>
Reviewed-by: Eric Blake 
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/259.out | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/259.out b/tests/qemu-iotests/259.out
index e27b9ff38d75..1aaadfda4ead 100644
--- a/tests/qemu-iotests/259.out
+++ b/tests/qemu-iotests/259.out
@@ -10,5 +10,5 @@ disk size: unavailable

 --- Testing creation for which the node would need to grow ---
 Formatting 'TEST_DIR/t.IMGFMT', fmt=qcow2 size=67108864 preallocation=metadata
-qemu-img: TEST_DIR/t.IMGFMT: Could not resize image: Image format driver does 
not support resize
+qemu-img: TEST_DIR/t.IMGFMT: Could not resize image: Cannot grow NBD nodes
 *** done
-- 
2.28.0




[PULL 3/6] block/nbd: use non-blocking connect: fix vm hang on connect()

2020-09-02 Thread Eric Blake
From: Vladimir Sementsov-Ogievskiy 

This makes nbd's connection_co yield during reconnects, so that
reconnect doesn't block the main thread. This is very important in
case of an unavailable nbd server host: connect() call may take a long
time, blocking the main thread (and due to reconnect, it will hang
again and again with small gaps of working time during pauses between
connection attempts).

Realization notes:

 - We don't want to implement non-blocking connect() over non-blocking
 socket, because getaddrinfo() doesn't have portable non-blocking
 realization anyway, so let's just use a thread for both getaddrinfo()
 and connect().

 - We can't use qio_channel_socket_connect_async (which behaves
 similarly and starts a thread to execute connect() call), as it's relying
 on someone iterating main loop (g_main_loop_run() or something like
 this), which is not always the case.

 - We can't use thread_pool_submit_co API, as thread pool waits for all
 threads to finish (but we don't want to wait for blocking reconnect
 attempt on shutdown.

 So, we just create the thread by hand. Some additional difficulties
 are:

 - We want our connect to avoid blocking drained sections and aio context
 switches. To achieve this, we make it possible to "cancel" synchronous
 wait for the connect (which is a coroutine yield actually), still,
 the thread continues in background, and if successful, its result may be
 reused on next reconnect attempt.

 - We don't want to wait for reconnect on shutdown, so there is
 CONNECT_THREAD_RUNNING_DETACHED thread state, which means that the block
 layer is no longer interested in a result, and thread should close new
 connected socket on finish and free the state.

How to reproduce the bug, fixed with this commit:

1. Create an image on node1:
   qemu-img create -f qcow2 xx 100M

2. Start NBD server on node1:
   qemu-nbd xx

3. Start vm with second nbd disk on node2, like this:

  ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -drive \
file=/work/images/cent7.qcow2 -drive file=nbd+tcp://192.168.100.2 \
-vnc :0 -qmp stdio -m 2G -enable-kvm -vga std

4. Access the vm through vnc (or some other way?), and check that NBD
   drive works:

   dd if=/dev/sdb of=/dev/null bs=1M count=10

   - the command should succeed.

5. Now, let's trigger nbd-reconnect loop in Qemu process. For this:

5.1 Kill NBD server on node1

5.2 run "dd if=/dev/sdb of=/dev/null bs=1M count=10" in the guest
again. The command should fail and a lot of error messages about
failing disk may appear as well.

Now NBD client driver in Qemu tries to reconnect.
Still, VM works well.

6. Make node1 unavailable on NBD port, so connect() from node2 will
   last for a long time:

   On node1 (Note, that 10809 is just a default NBD port):

   sudo iptables -A INPUT -p tcp --dport 10809 -j DROP

   After some time the guest hangs, and you may check in gdb that Qemu
   hangs in connect() call, issued from the main thread. This is the
   BUG.

7. Don't forget to drop iptables rule from your node1:

   sudo iptables -D INPUT -p tcp --dport 10809 -j DROP

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20200812145237.4396-1-vsement...@virtuozzo.com>
Reviewed-by: Eric Blake 
[eblake: minor wording and formatting tweaks]
Signed-off-by: Eric Blake 
---
 block/nbd.c | 266 +++-
 1 file changed, 265 insertions(+), 1 deletion(-)

diff --git a/block/nbd.c b/block/nbd.c
index 7bb881fef49c..9daf003bea30 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -38,6 +38,7 @@

 #include "qapi/qapi-visit-sockets.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/clone-visitor.h"

 #include "block/qdict.h"
 #include "block/nbd.h"
@@ -62,6 +63,47 @@ typedef enum NBDClientState {
 NBD_CLIENT_QUIT
 } NBDClientState;

+typedef enum NBDConnectThreadState {
+/* No thread, no pending results */
+CONNECT_THREAD_NONE,
+
+/* Thread is running, no results for now */
+CONNECT_THREAD_RUNNING,
+
+/*
+ * Thread is running, but requestor exited. Thread should close
+ * the new socket and free the connect state on exit.
+ */
+CONNECT_THREAD_RUNNING_DETACHED,
+
+/* Thread finished, results are stored in a state */
+CONNECT_THREAD_FAIL,
+CONNECT_THREAD_SUCCESS
+} NBDConnectThreadState;
+
+typedef struct NBDConnectThread {
+/* Initialization constants */
+SocketAddress *saddr; /* address to connect to */
+/*
+ * Bottom half to schedule on completion. Scheduled only if bh_ctx is not
+ * NULL
+ */
+QEMUBHFunc *bh_func;
+void *bh_opaque;
+
+/*
+ * Result of last attempt. Valid in FAIL and SUCCESS states.
+ * If you want to steal error, don't forget to set pointer to NULL.
+ */
+QIOChannelSocket *sioc;
+Error *err;
+
+/* state and bh_ctx are protected by mutex */
+QemuMutex mutex;
+NBDConnectThreadStat

[PULL 6/6] nbd: disable signals and forking on Windows builds

2020-09-02 Thread Eric Blake
From: Daniel P. Berrangé 

Disabling these parts are sufficient to get the qemu-nbd program
compiling in a Windows build.

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20200825103850.119911-4-berra...@redhat.com>
Reviewed-by: Eric Blake 
Signed-off-by: Eric Blake 
---
 qemu-nbd.c  | 5 +
 meson.build | 7 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index dc6ef089afd5..33476a1000c8 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -899,6 +899,7 @@ int main(int argc, char **argv)
 #endif

 if ((device && !verbose) || fork_process) {
+#ifndef WIN32
 int stderr_fd[2];
 pid_t pid;
 int ret;
@@ -962,6 +963,10 @@ int main(int argc, char **argv)
  */
 exit(errors);
 }
+#else /* WIN32 */
+error_report("Unable to fork into background on Windows hosts");
+exit(EXIT_FAILURE);
+#endif /* WIN32 */
 }

 if (device != NULL && sockpath == NULL) {
diff --git a/meson.build b/meson.build
index 55c7d2318cdb..5aaa3647305d 100644
--- a/meson.build
+++ b/meson.build
@@ -1095,12 +1095,9 @@ if have_tools
  dependencies: [authz, block, crypto, io, qom, qemuutil], install: 
true)
   qemu_io = executable('qemu-io', files('qemu-io.c'),
  dependencies: [block, qemuutil], install: true)
-  qemu_block_tools += [qemu_img, qemu_io]
-  if targetos != 'windows'
-qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
+  qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
dependencies: [block, qemuutil], install: true)
-qemu_block_tools += [qemu_nbd]
-  endif
+  qemu_block_tools += [qemu_img, qemu_io, qemu_nbd]

   subdir('storage-daemon')
   subdir('contrib/rdmacm-mux')
-- 
2.28.0




[PULL 4/6] block: add missing socket_init() calls to tools

2020-09-02 Thread Eric Blake
From: Daniel P. Berrangé 

Any tool that uses sockets needs to call socket_init() in order to work
on the Windows platform.

Reviewed-by: Eric Blake 
Signed-off-by: Daniel P. Berrangé 
Message-Id: <20200825103850.119911-2-berra...@redhat.com>
Signed-off-by: Eric Blake 
---
 qemu-img.c | 2 ++
 qemu-io.c  | 2 ++
 qemu-nbd.c | 1 +
 3 files changed, 5 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 5308773811f3..eb2fc1f86243 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -41,6 +41,7 @@
 #include "qemu/log.h"
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
+#include "qemu/sockets.h"
 #include "qemu/units.h"
 #include "qom/object_interfaces.h"
 #include "sysemu/block-backend.h"
@@ -5410,6 +5411,7 @@ int main(int argc, char **argv)
 signal(SIGPIPE, SIG_IGN);
 #endif

+socket_init();
 error_init(argv[0]);
 module_call_init(MODULE_INIT_TRACE);
 qemu_init_exec_dir(argv[0]);
diff --git a/qemu-io.c b/qemu-io.c
index 3adc5a7d0d3f..7cc832b3d618 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -25,6 +25,7 @@
 #include "qemu/config-file.h"
 #include "qemu/readline.h"
 #include "qemu/log.h"
+#include "qemu/sockets.h"
 #include "qapi/qmp/qstring.h"
 #include "qapi/qmp/qdict.h"
 #include "qom/object_interfaces.h"
@@ -542,6 +543,7 @@ int main(int argc, char **argv)
 signal(SIGPIPE, SIG_IGN);
 #endif

+socket_init();
 error_init(argv[0]);
 module_call_init(MODULE_INIT_TRACE);
 qemu_init_exec_dir(argv[0]);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index d2657b8db50d..b102874f0f46 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -599,6 +599,7 @@ int main(int argc, char **argv)
 signal(SIGPIPE, SIG_IGN);
 #endif

+socket_init();
 error_init(argv[0]);
 module_call_init(MODULE_INIT_TRACE);
 qcrypto_init(_fatal);
-- 
2.28.0




[PULL 5/6] nbd: skip SIGTERM handler if NBD device support is not built

2020-09-02 Thread Eric Blake
From: Daniel P. Berrangé 

The termsig_handler function is used by the client thread handling the
host NBD device connection to do a graceful shutdown. IOW, if we have
disabled NBD device support at compile time, we don't need the SIGTERM
handler. This fixes a build issue for Windows.

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20200825103850.119911-3-berra...@redhat.com>
Reviewed-by: Eric Blake 
Signed-off-by: Eric Blake 
---
 qemu-nbd.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index b102874f0f46..dc6ef089afd5 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -155,12 +155,13 @@ QEMU_COPYRIGHT "\n"
 , name);
 }

+#if HAVE_NBD_DEVICE
 static void termsig_handler(int signum)
 {
 atomic_cmpxchg(, RUNNING, TERMINATE);
 qemu_notify_event();
 }
-
+#endif /* HAVE_NBD_DEVICE */

 static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
 const char *hostname)
@@ -587,6 +588,7 @@ int main(int argc, char **argv)
 unsigned socket_activation;
 const char *pid_file_name = NULL;

+#if HAVE_NBD_DEVICE
 /* The client thread uses SIGTERM to interrupt the server.  A signal
  * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
  */
@@ -594,6 +596,7 @@ int main(int argc, char **argv)
 memset(_sigterm, 0, sizeof(sa_sigterm));
 sa_sigterm.sa_handler = termsig_handler;
 sigaction(SIGTERM, _sigterm, NULL);
+#endif /* HAVE_NBD_DEVICE */

 #ifdef CONFIG_POSIX
 signal(SIGPIPE, SIG_IGN);
-- 
2.28.0




[PULL 1/6] iotests/059: Fix reference output

2020-09-02 Thread Eric Blake
From: Max Reitz 

As of the patch to flush qemu-img's "Formatting" message before the
error message, 059 has been broken for vmdk.  Fix it.

Fixes: 4e2f4418784da09cb106264340241856cd2846df
   ("qemu-img: Flush stdout before before potential stderr messages")
Signed-off-by: Max Reitz 
Message-Id: <20200811084150.326377-1-mre...@redhat.com>
Reviewed-by: Eric blake 
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/059.out | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/059.out b/tests/qemu-iotests/059.out
index 6d127e28d8a6..2b83c0c8b66b 100644
--- a/tests/qemu-iotests/059.out
+++ b/tests/qemu-iotests/059.out
@@ -19,8 +19,8 @@ file format: IMGFMT
 virtual size: 2 GiB (2147483648 bytes)

 === Testing monolithicFlat with zeroed_grain ===
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648
 qemu-img: TEST_DIR/t.IMGFMT: Flat image can't enable zeroed grain
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2147483648

 === Testing big twoGbMaxExtentFlat ===
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824000
-- 
2.28.0




Re: [PATCH v2 3/3] nbd: disable signals and forking on Windows builds

2020-09-02 Thread Eric Blake

On 8/25/20 5:38 AM, Daniel P. Berrangé wrote:

Disabling these parts are sufficient to get the qemu-nbd program
compiling in a Windows build.

Signed-off-by: Daniel P. Berrangé 
---
  meson.build | 7 ++-
  qemu-nbd.c  | 5 +
  2 files changed, 7 insertions(+), 5 deletions(-)


Reviewed-by: Eric Blake 

Queuing through my NBD tree.

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




Re: [PATCH v2 2/3] nbd: skip SIGTERM handler if NBD device support is not built

2020-09-02 Thread Eric Blake

On 8/25/20 5:38 AM, Daniel P. Berrangé wrote:

The termsig_handler function is used by the client thread handling the
host NBD device connection to do a graceful shutdown. IOW, if we have
disabled NBD device support at compile time, we don't need the SIGTERM
handler. This fixes a build issue for Windows.

Signed-off-by: Daniel P. Berrangé 
---
  qemu-nbd.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)


Reviewed-by: Eric Blake 

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




[PATCH v4 3/5] qmp: expose block-dirty-bitmap-populate

2020-09-02 Thread Eric Blake
From: John Snow 

This is a new job-creating command.

Signed-off-by: John Snow 
Signed-off-by: Eric Blake 
---
 qapi/block-core.json  | 18 +++
 qapi/transaction.json |  2 ++
 blockdev.c| 74 +++
 3 files changed, 94 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1cac9a9a8207..ed05c0bfa720 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2264,6 +2264,24 @@
 '*auto-finalize': 'bool',
 '*auto-dismiss': 'bool' } }

+##
+# @block-dirty-bitmap-populate:
+#
+# Creates a new job that writes a pattern into a dirty bitmap.
+#
+# Since: 5.2
+#
+# Example:
+#
+# -> { "execute": "block-dirty-bitmap-populate",
+#  "arguments": { "node": "drive0", "target": "bitmap0",
+# "job-id": "job0", "pattern": "allocate-top" } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'block-dirty-bitmap-populate', 'boxed': true,
+  'data': 'BlockDirtyBitmapPopulate' }
+
 ##
 # @BlockDirtyBitmapSha256:
 #
diff --git a/qapi/transaction.json b/qapi/transaction.json
index 15ddebdbc360..8302b824e4d5 100644
--- a/qapi/transaction.json
+++ b/qapi/transaction.json
@@ -51,6 +51,7 @@
 # - @block-dirty-bitmap-enable: since 4.0
 # - @block-dirty-bitmap-disable: since 4.0
 # - @block-dirty-bitmap-merge: since 4.0
+# - @block-dirty-bitmap-populate: since 5.2
 # - @blockdev-backup: since 2.3
 # - @blockdev-snapshot: since 2.5
 # - @blockdev-snapshot-internal-sync: since 1.7
@@ -68,6 +69,7 @@
'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
'block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge',
+   'block-dirty-bitmap-populate': 'BlockDirtyBitmapPopulate',
'blockdev-backup': 'BlockdevBackup',
'blockdev-snapshot': 'BlockdevSnapshot',
'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
diff --git a/blockdev.c b/blockdev.c
index f177048a4fe8..3657377d2aca 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2190,6 +2190,63 @@ static void 
block_dirty_bitmap_remove_commit(BlkActionState *common)
 bdrv_release_dirty_bitmap(state->bitmap);
 }

+static void block_dirty_bitmap_populate_prepare(BlkActionState *common,
+Error **errp)
+{
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
+BlockDirtyBitmapPopulate *bitpop;
+BlockDriverState *bs;
+AioContext *aio_context;
+BdrvDirtyBitmap *bmap = NULL;
+int job_flags = JOB_DEFAULT;
+
+assert(common->action->type ==
+   TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE);
+bitpop = common->action->u.block_dirty_bitmap_populate.data;
+
+bmap = block_dirty_bitmap_lookup(bitpop->node, bitpop->name, , errp);
+if (!bmap) {
+return;
+}
+
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
+state->bs = bs;
+
+/* Paired with .clean() */
+bdrv_drained_begin(state->bs);
+
+if (!bitpop->has_on_error) {
+bitpop->on_error = BLOCKDEV_ON_ERROR_REPORT;
+}
+if (!bitpop->has_auto_finalize) {
+bitpop->auto_finalize = true;
+}
+if (!bitpop->has_auto_dismiss) {
+bitpop->auto_dismiss = true;
+}
+
+if (!bitpop->auto_finalize) {
+job_flags |= JOB_MANUAL_FINALIZE;
+}
+if (!bitpop->auto_dismiss) {
+job_flags |= JOB_MANUAL_DISMISS;
+}
+
+state->job = bitpop_job_create(
+bitpop->job_id,
+bs,
+bmap,
+bitpop->pattern,
+bitpop->on_error,
+job_flags,
+NULL, NULL,
+common->block_job_txn,
+errp);
+
+aio_context_release(aio_context);
+}
+
 static void abort_prepare(BlkActionState *common, Error **errp)
 {
 error_setg(errp, "Transaction aborted using Abort action");
@@ -2273,6 +2330,13 @@ static const BlkActionOps actions[] = {
 .commit = block_dirty_bitmap_remove_commit,
 .abort = block_dirty_bitmap_remove_abort,
 },
+[TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_POPULATE] = {
+.instance_size = sizeof(BlockJobActionState),
+.prepare = block_dirty_bitmap_populate_prepare,
+.commit = blockdev_backup_commit,
+.abort = blockdev_backup_abort,
+.clean = blockdev_backup_clean,
+},
 /* Where are transactions for MIRROR, COMMIT and STREAM?
  * Although these blockjobs use transaction callbacks like the backup job,
  * these jobs do not necessarily adhere to transaction semantics.
@@ -2392,6 +2456,16 @@ void qmp_block_passwd(bool has_device, const char 
*device,
"Setting block passwords directly is no longer supported");
 }

+void qmp_block_d

[PATCH v4 1/5] block: add bitmap-populate job

2020-09-02 Thread Eric Blake
From: John Snow 

This job copies the allocation map into a bitmap. It's a job because
there's no guarantee that allocation interrogation will be quick (or
won't hang), so it cannot be retrofitted into block-dirty-bitmap-merge.

It was designed with different possible population patterns in mind,
but only top layer allocation was implemented for now.

Signed-off-by: John Snow 
Signed-off-by: Eric Blake 
---
 qapi/block-core.json  |  48 +
 qapi/job.json |   6 +-
 include/block/block.h |   1 +
 include/block/block_int.h |  21 
 block/bitmap-populate.c   | 207 ++
 blockjob.c|   3 +-
 MAINTAINERS   |   1 +
 block/meson.build |   1 +
 8 files changed, 286 insertions(+), 2 deletions(-)
 create mode 100644 block/bitmap-populate.c

diff --git a/qapi/block-core.json b/qapi/block-core.json
index db08c58d788c..1cac9a9a8207 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2216,6 +2216,54 @@
 { 'command': 'block-dirty-bitmap-merge',
   'data': 'BlockDirtyBitmapMerge' }

+##
+# @BitmapPattern:
+#
+# An enumeration of possible patterns that can be written into a bitmap.
+#
+# @allocation-top: The allocation status of the top layer
+#  of the attached storage node.
+#
+# Since: 5.2
+##
+{ 'enum': 'BitmapPattern',
+  'data': ['allocation-top'] }
+
+##
+# @BlockDirtyBitmapPopulate:
+#
+# @job-id: identifier for the newly-created block job.
+#
+# @pattern: What pattern should be written into the bitmap?
+#
+# @on-error: the action to take if an error is encountered on a bitmap's
+#attached node, default 'report'.
+#'stop' and 'enospc' can only be used if the block device supports
+#io-status (see BlockInfo).
+#
+# @auto-finalize: When false, this job will wait in a PENDING state after it
+# has finished its work, waiting for @block-job-finalize
+# before making any block graph changes.
+# When true, this job will automatically
+# perform its abort or commit actions.
+# Defaults to true.
+#
+# @auto-dismiss: When false, this job will wait in a CONCLUDED state after it
+#has completely ceased all work, and awaits @block-job-dismiss.
+#When true, this job will automatically disappear from the
+#query list without user intervention.
+#Defaults to true.
+#
+# Since: 5.2
+##
+{ 'struct': 'BlockDirtyBitmapPopulate',
+  'base': 'BlockDirtyBitmap',
+  'data': { 'job-id': 'str',
+'pattern': 'BitmapPattern',
+'*on-error': 'BlockdevOnError',
+'*auto-finalize': 'bool',
+'*auto-dismiss': 'bool' } }
+
 ##
 # @BlockDirtyBitmapSha256:
 #
diff --git a/qapi/job.json b/qapi/job.json
index 280c2f76f136..fb0b606e868d 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -22,10 +22,14 @@
 #
 # @amend: image options amend job type, see "x-blockdev-amend" (since 5.1)
 #
+# @bitmap-populate: drive bitmap population job type, see
+#   "block-dirty-bitmap-populate" (since 5.2)
+#
 # Since: 1.7
 ##
 { 'enum': 'JobType',
-  'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
+  'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend',
+   'bitmap-populate'] }

 ##
 # @JobStatus:
diff --git a/include/block/block.h b/include/block/block.h
index 6e36154061c1..f4b740857725 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -200,6 +200,7 @@ typedef struct BDRVReopenState {
 typedef enum BlockOpType {
 BLOCK_OP_TYPE_BACKUP_SOURCE,
 BLOCK_OP_TYPE_BACKUP_TARGET,
+BLOCK_OP_TYPE_BITMAP_POPULATE,
 BLOCK_OP_TYPE_CHANGE,
 BLOCK_OP_TYPE_COMMIT_SOURCE,
 BLOCK_OP_TYPE_COMMIT_TARGET,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9da7a42927eb..c85a79666524 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1257,6 +1257,27 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 BlockCompletionFunc *cb, void *opaque,
 JobTxn *txn, Error **errp);

+/*
+ * bitpop_job_create: Create a new bitmap population job.
+ *
+ * @job_id: The id of the newly-created job.
+ * @bs: Block device associated with the @target_bitmap.
+ * @target_bitmap: The bitmap to populate.
+ * @on_error: What to do if an error on @bs is encountered.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ *  See @BlockJobCreateFlags
+ * @cb: Completion function for the job.
+ * @opaque: Opaque pointer value passed to @cb.
+ * @txn: Transaction that this job is part of (may be NULL).
+ */
+BlockJob *bitpop_job_create(const char *job_id, BlockDriverState *bs,
+BdrvDirtyBitmap *target_bitmap,
+B

[PATCH v4 4/5] iotests: move bitmap helpers into their own file

2020-09-02 Thread Eric Blake
From: John Snow 

Signed-off-by: John Snow 
Message-Id: <20200514034922.24834-5-js...@redhat.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/257| 110 +---
 tests/qemu-iotests/bitmaps.py | 131 ++
 2 files changed, 132 insertions(+), 109 deletions(-)
 create mode 100644 tests/qemu-iotests/bitmaps.py

diff --git a/tests/qemu-iotests/257 b/tests/qemu-iotests/257
index e1e60772195e..c12191ec422f 100755
--- a/tests/qemu-iotests/257
+++ b/tests/qemu-iotests/257
@@ -24,120 +24,12 @@ import os

 import iotests
 from iotests import log, qemu_img
+from bitmaps import EmulatedBitmap, GROUPS

 SIZE = 64 * 1024 * 1024
 GRANULARITY = 64 * 1024


-class Pattern:
-def __init__(self, byte, offset, size=GRANULARITY):
-self.byte = byte
-self.offset = offset
-self.size = size
-
-def bits(self, granularity):
-lower = self.offset // granularity
-upper = (self.offset + self.size - 1) // granularity
-return set(range(lower, upper + 1))
-
-
-class PatternGroup:
-"""Grouping of Pattern objects. Initialize with an iterable of Patterns."""
-def __init__(self, patterns):
-self.patterns = patterns
-
-def bits(self, granularity):
-"""Calculate the unique bits dirtied by this pattern grouping"""
-res = set()
-for pattern in self.patterns:
-res |= pattern.bits(granularity)
-return res
-
-
-GROUPS = [
-PatternGroup([
-# Batch 0: 4 clusters
-Pattern('0x49', 0x000),
-Pattern('0x6c', 0x010),   # 1M
-Pattern('0x6f', 0x200),   # 32M
-Pattern('0x76', 0x3ff)]), # 64M - 64K
-PatternGroup([
-# Batch 1: 6 clusters (3 new)
-Pattern('0x65', 0x000),   # Full overwrite
-Pattern('0x77', 0x00f8000),   # Partial-left (1M-32K)
-Pattern('0x72', 0x2008000),   # Partial-right (32M+32K)
-Pattern('0x69', 0x3fe)]), # Adjacent-left (64M - 128K)
-PatternGroup([
-# Batch 2: 7 clusters (3 new)
-Pattern('0x74', 0x001),   # Adjacent-right
-Pattern('0x69', 0x00e8000),   # Partial-left  (1M-96K)
-Pattern('0x6e', 0x2018000),   # Partial-right (32M+96K)
-Pattern('0x67', 0x3fe,
-2*GRANULARITY)]), # Overwrite [(64M-128K)-64M)
-PatternGroup([
-# Batch 3: 8 clusters (5 new)
-# Carefully chosen such that nothing re-dirties the one cluster
-# that copies out successfully before failure in Group #1.
-Pattern('0xaa', 0x001,
-3*GRANULARITY),   # Overwrite and 2x Adjacent-right
-Pattern('0xbb', 0x00d8000),   # Partial-left (1M-160K)
-Pattern('0xcc', 0x2028000),   # Partial-right (32M+160K)
-Pattern('0xdd', 0x3fc)]), # New; leaving a gap to the right
-]
-
-
-class EmulatedBitmap:
-def __init__(self, granularity=GRANULARITY):
-self._bits = set()
-self.granularity = granularity
-
-def dirty_bits(self, bits):
-self._bits |= set(bits)
-
-def dirty_group(self, n):
-self.dirty_bits(GROUPS[n].bits(self.granularity))
-
-def clear(self):
-self._bits = set()
-
-def clear_bits(self, bits):
-self._bits -= set(bits)
-
-def clear_bit(self, bit):
-self.clear_bits({bit})
-
-def clear_group(self, n):
-self.clear_bits(GROUPS[n].bits(self.granularity))
-
-@property
-def first_bit(self):
-return sorted(self.bits)[0]
-
-@property
-def bits(self):
-return self._bits
-
-@property
-def count(self):
-return len(self.bits)
-
-def compare(self, qmp_bitmap):
-"""
-Print a nice human-readable message checking that a bitmap as reported
-by the QMP interface has as many bits set as we expect it to.
-"""
-
-name = qmp_bitmap.get('name', '(anonymous)')
-log("= Checking Bitmap {:s} =".format(name))
-
-want = self.count
-have = qmp_bitmap['count'] // qmp_bitmap['granularity']
-
-log("expecting {:d} dirty sectors; have {:d}. {:s}".format(
-want, have, "OK!" if want == have else "ERROR!"))
-log('')
-
-
 class Drive:
 """Represents, vaguely, a drive attached to a VM.
 Includes format, graph, and device information."""
diff --git a/tests/qemu-iotests/bitmaps.py b/tests/qemu-iotests/bitmaps.py
new file mode 100644
index ..522fc25171d1
--- /dev/null
+++ b/tests/qemu-iotests/bitmaps.py
@@ -0,0 +1,131 @@
+# Bitmap-related helper utilities
+#
+# Copyright (c) 2020 John Snow for Red Hat, Inc.
+#
+# 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 Fre

[PATCH v4 2/5] blockdev: combine DriveBackupState and BlockdevBackupState

2020-09-02 Thread Eric Blake
From: John Snow 

They have the same fields -- rename it BlockJobActionState.

Signed-off-by: John Snow 
Signed-off-by: Eric Blake 
---
 blockdev.c | 30 --
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3848a9c8ab86..f177048a4fe8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1668,11 +1668,11 @@ static void external_snapshot_clean(BlkActionState 
*common)
 aio_context_release(aio_context);
 }

-typedef struct DriveBackupState {
+typedef struct BlockJobActionState {
 BlkActionState common;
 BlockDriverState *bs;
 BlockJob *job;
-} DriveBackupState;
+} BlockJobActionState;

 static BlockJob *do_backup_common(BackupCommon *backup,
   BlockDriverState *bs,
@@ -1682,7 +1682,7 @@ static BlockJob *do_backup_common(BackupCommon *backup,

 static void drive_backup_prepare(BlkActionState *common, Error **errp)
 {
-DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 DriveBackup *backup;
 BlockDriverState *bs;
 BlockDriverState *target_bs;
@@ -1819,7 +1819,7 @@ out:

 static void drive_backup_commit(BlkActionState *common)
 {
-DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 AioContext *aio_context;

 aio_context = bdrv_get_aio_context(state->bs);
@@ -1833,7 +1833,7 @@ static void drive_backup_commit(BlkActionState *common)

 static void drive_backup_abort(BlkActionState *common)
 {
-DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);

 if (state->job) {
 AioContext *aio_context;
@@ -1849,7 +1849,7 @@ static void drive_backup_abort(BlkActionState *common)

 static void drive_backup_clean(BlkActionState *common)
 {
-DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 AioContext *aio_context;

 if (!state->bs) {
@@ -1864,15 +1864,9 @@ static void drive_backup_clean(BlkActionState *common)
 aio_context_release(aio_context);
 }

-typedef struct BlockdevBackupState {
-BlkActionState common;
-BlockDriverState *bs;
-BlockJob *job;
-} BlockdevBackupState;
-
 static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
 {
-BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 BlockdevBackup *backup;
 BlockDriverState *bs;
 BlockDriverState *target_bs;
@@ -1920,7 +1914,7 @@ static void blockdev_backup_prepare(BlkActionState 
*common, Error **errp)

 static void blockdev_backup_commit(BlkActionState *common)
 {
-BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 AioContext *aio_context;

 aio_context = bdrv_get_aio_context(state->bs);
@@ -1934,7 +1928,7 @@ static void blockdev_backup_commit(BlkActionState *common)

 static void blockdev_backup_abort(BlkActionState *common)
 {
-BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);

 if (state->job) {
 AioContext *aio_context;
@@ -1950,7 +1944,7 @@ static void blockdev_backup_abort(BlkActionState *common)

 static void blockdev_backup_clean(BlkActionState *common)
 {
-BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, 
common);
+BlockJobActionState *state = DO_UPCAST(BlockJobActionState, common, 
common);
 AioContext *aio_context;

 if (!state->bs) {
@@ -,14 +2216,14 @@ static const BlkActionOps actions[] = {
 .clean = external_snapshot_clean,
 },
 [TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = {
-.instance_size = sizeof(DriveBackupState),
+.instance_size = sizeof(BlockJobActionState),
 .prepare = drive_backup_prepare,
 .commit = drive_backup_commit,
 .abort = drive_backup_abort,
 .clean = drive_backup_clean,
 },
 [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
-.instance_size = sizeof(BlockdevBackupState),
+.instance_size = sizeof(BlockJobActionState),
 .prepare = blockdev_backup_prepare,
 .commit = blockdev_backup_commit,
 .abort = blockdev_backup_abort,
-- 
2.28.0




[PATCH v4 0/5] block: add block-dirty-bitmap-populate job

2020-09-02 Thread Eric Blake
This is NOT the final version of this patch series, but I'm posting it
to revive conversation on the topic while fixing it to compile on top
of meson changes.

v3 was: https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg06258.html

001/5:[0025] [FC] 'block: add bitmap-populate job'
002/5:[] [--] 'blockdev: combine DriveBackupState and BlockdevBackupState'
003/5:[0004] [FC] 'qmp: expose block-dirty-bitmap-populate'
004/5:[] [--] 'iotests: move bitmap helpers into their own file'
005/5:[] [-C] 'iotests: add 298 for block-dirty-bitmap-populate'

I'm still trying to find the right QAPI contract (affects patch 1 and
3 for the qapi, and 5 for invoking the command in iotests), but right
now, I'm leaning towards:

{ "execute": "block-dirty-bitmap-populate", "arguments": {
"job-id": "job0", "node": "target_node", "name": "target_bitmap",
"sources": [ { "pattern": "allocation", "node": "from_node" } ] } }

which allows expansion into multiple sources, so that we can combine a
populate action with a bitmap merge rather than having to do those as
separate commands, like:

{ "execute": "block-dirty-bitmap-populate", "arguments": {
"job-id": "job0", "node": "target_node", "name": "target_bitmap",
"sources": [ { "pattern": "allocation", "node": "from_node" },
 { "pattern": "bitmap", "node": "from_node",
 "bitmap": "from_bitmap" } ] } }

John Snow (5):
  block: add bitmap-populate job
  blockdev: combine DriveBackupState and BlockdevBackupState
  qmp: expose block-dirty-bitmap-populate
  iotests: move bitmap helpers into their own file
  iotests: add 298 for block-dirty-bitmap-populate

 qapi/block-core.json  |   66 +
 qapi/job.json |6 +-
 qapi/transaction.json |2 +
 include/block/block.h |1 +
 include/block/block_int.h |   21 +
 block/bitmap-populate.c   |  207 ++
 blockdev.c|  104 +-
 blockjob.c|3 +-
 MAINTAINERS   |1 +
 block/meson.build |1 +
 tests/qemu-iotests/257|  110 +-
 tests/qemu-iotests/298|  232 ++
 tests/qemu-iotests/298.out| 4544 +
 tests/qemu-iotests/bitmaps.py |  131 +
 tests/qemu-iotests/group  |1 +
 15 files changed, 5301 insertions(+), 129 deletions(-)
 create mode 100644 block/bitmap-populate.c
 create mode 100755 tests/qemu-iotests/298
 create mode 100644 tests/qemu-iotests/298.out
 create mode 100644 tests/qemu-iotests/bitmaps.py

-- 
2.28.0




Re: [PATCH v3 1/6] block: add bitmap-populate job

2020-09-02 Thread Eric Blake

[reviving an old thread]

On 6/22/20 4:44 PM, Eric Blake wrote:

On 6/19/20 11:16 PM, Vladimir Sementsov-Ogievskiy wrote:

19.06.2020 22:56, Eric Blake wrote:

From: John Snow 

This job copies the allocation map into a bitmap. It's a job because
there's no guarantee that allocation interrogation will be quick (or
won't hang), so it cannot be retrofitted into block-dirty-bitmap-merge.

It was designed with different possible population patterns in mind,
but only top layer allocation was implemented for now.

Signed-off-by: John Snow 
Signed-off-by: Eric Blake 
---



+{ 'struct': 'BlockDirtyBitmapPopulate',
+  'base': 'BlockDirtyBitmap',
+  'data': { 'job-id': 'str',
+    'pattern': 'BitmapPattern',


As written, "pattern":"allocate-top" is rather limited - it can only 
grab allocation from the top node.  Being able to grab the allocation 
from a specific node may indeed be more useful.  Another bitmap patterns 
that might be useful would be an all-one pattern (create a bitmap that 
treats the entire disk as dirty).  I also remember brainstorming with 
John the question of whether we want bitmap-populate to have different 
mask modes: does the population perform an overwrite (the bitmap now 
matches the source pattern exactly, even if some bits were set and 
others cleared), a union merge (any bits already set in the bitmap 
remain set, additional bits are set according to pattern), or even a 
difference (any bits already cleared in the bitmap remain clear, while 
bits in the pattern can also clear additional bits in the bitmap).


If I understand Peter's goals, the initial libvirt use is a union mode 
(keep bits in the bitmap that are already set, but set additional bits 
according to the population pattern).



+    '*on-error': 'BlockdevOnError',
+    '*auto-finalize': 'bool',
+    '*auto-dismiss': 'bool' } }
+


Peter said about a possibility of populating several target bitmaps 
simultaneously.


What about such a generalized semantics:

Merge all sources to each target

@targets: list of bitmaps to be populated by the job
{ 'struct': 'BlockDirtyBitmapPopulate',
   'data': { ,
 'targets': ['BlockDirtyBitmap'],
 'sources': ['BitmapPopulateSource'] } }


We still need the 'pattern' argument (the idea being that if we have: 
Base <- Active, we want to be able to merge in the allocation map of 
Active into bitmaps stored in Base as part of a commit operation, 
whether that is active commit of a live guest or offline commit while 
the guest is offline).  Having an array for 'targets' to merge into is 
fine, but for 'sources', it's less a concern about selecting from 
multiple sources, and more a concern about selecting the allocation 
pattern to be merged in (libvirt wants to merge the same allocation 
pattern into each bitmap in Base).  Generalizing things to allow the 
merge of more than one source at once might not hurt, but I'm not sure 
we need it yet.


But when it comes to multiple destinations or multiple sources, while it 
seems like it might be a convenience factor, I also worry that it is 
over-engineering.  See more below...




But there are other patterns that we may want to support: an all-ones 
pattern, or maybe a pattern that tracks known-zeros instead of allocation.





@bitmap: specify dirty bitmap to be merged to target bitamp(s)
@node: specify a node name, which allocation-map is to be merged to 
target bitmap(s)

{ 'alternate': 'BitmapPopulateSource',
   'data': { 'bitmap': 'BlockDirtyBitmap',
 'node': 'str' } }


This design is clever in that it lets us merge in both existing bitmaps 
and using a node-name for merging in an allocation map instead of a 
bitmap; but it limits us to only one pattern.  Better might be something 
where we supply a union (hmm, we've had proposals in the past for a 
default value to the discriminator to allow it to be optional, so I'll 
proceed as if we will finally implement that):


{ 'enum': 'BitmapPattern', 'data': [ 'bitmap', 'allocation-top' ] }
{ 'union': 'BitmapPopulateSource',
   'base': { '*pattern': 'BitmapPattern' },
   'discriminator': { 'name': 'pattern', 'default': 'bitmap' },
   'data': { 'bitmap': 'BitmapPopulateSource',
     'allocation-top': { 'node': 'str' } } }

so that you can then do:

{ "execute": "block-dirty-bitmap-populate",
   "arguments": { "targets": [ { "node": "base", "name": "b1" },
   { "node": "base", "name": "b2" } ],
     "sources": [ { "pattern": "allocation-top", "node": "top" } ]
   } }

to merge in the allocation information of top into multiple bitmaps of 
base at once,


Hmm, I left out the mandatory "job-id" parameter here; one of the key 
points of the new command is that some patterns (like allocation) 

Re: [PATCH 2/2] nbd: disable signals and forking on Windows builds

2020-08-24 Thread Eric Blake

On 8/24/20 12:02 PM, Daniel P. Berrangé wrote:

Disabling these parts are sufficient to get the qemu-nbd program
compiling in a Windows build.

Signed-off-by: Daniel P. Berrangé 
---
  meson.build |  7 ++-
  qemu-nbd.c  | 10 +-
  2 files changed, 11 insertions(+), 6 deletions(-)


Feels a bit hacky at what it supports, but certainly better than nothing ;)



diff --git a/meson.build b/meson.build
index df5bf728b5..1071871605 100644
--- a/meson.build
+++ b/meson.build
@@ -1074,12 +1074,9 @@ if have_tools
   dependencies: [authz, block, crypto, io, qom, qemuutil], 
install: true)
qemu_io = executable('qemu-io', files('qemu-io.c'),
   dependencies: [block, qemuutil], install: true)
-  qemu_block_tools += [qemu_img, qemu_io]
-  if targetos == 'linux' or targetos == 'sunos' or targetos.endswith('bsd')
-qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
+  qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'),
 dependencies: [block, qemuutil], install: true)


Conflicts with this patch:
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg05546.html

but this one gets rid of the need for that one.


-qemu_block_tools += [qemu_nbd]
-  endif
+  qemu_block_tools += [qemu_img, qemu_io, qemu_nbd]
  
subdir('storage-daemon')

subdir('contrib/rdmacm-mux')
diff --git a/qemu-nbd.c b/qemu-nbd.c
index b102874f0f..c6fd6524d3 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -155,12 +155,13 @@ QEMU_COPYRIGHT "\n"
  , name);
  }
  
+#ifndef WIN32

  static void termsig_handler(int signum)
  {
  atomic_cmpxchg(, RUNNING, TERMINATE);
  qemu_notify_event();
  }
-
+#endif


How does one terminate a long-running server on Windows if there is no 
SIGTERM handler?  I guess Ctrl-C does something, but without the state 
notification from a signal handler, you are getting less-clean 
shutdowns, which may explain the hangs you were seeing in testing?  But 
incremental progress is fine, and I see no reason to not take this patch 
as-is.


Reviewed-by: Eric Blake 

I'm happy to queue this series through my NBD tree.

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




Re: [PATCH 1/2] block: add missing socket_init() calls to tools

2020-08-24 Thread Eric Blake

On 8/24/20 12:02 PM, Daniel P. Berrangé wrote:

Any tool that uses sockets needs to call socket_init() in order to work
on the Windows platform.

Signed-off-by: Daniel P. Berrangé 
---
  qemu-img.c | 2 ++
  qemu-io.c  | 2 ++
  qemu-nbd.c | 1 +
  3 files changed, 5 insertions(+)



Reviewed-by: Eric Blake 

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




[PULL 09/14] qcow2_format.py: collect fields to dump in JSON format

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

As __dict__ is being extended with class members we do not want to
print, add the to_json() method to classes that returns a json-dumpable
object with desired fields and their values. Extend it in subclass when
necessary to print the final dictionary in the JSON output which
follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
Message-Id: 
<1596742557-320265-10-git-send-email-andrey.shinkev...@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/qcow2_format.py | 36 ++
 1 file changed, 36 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index de0adcbf9db0..5a298b2f1357 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -119,6 +119,9 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):

 print('{:<25} {}'.format(f[2], value_str))

+def to_json(self):
+return dict((f[2], self.__dict__[f[2]]) for f in self.fields)
+

 class Qcow2BitmapExt(Qcow2Struct):

@@ -151,6 +154,11 @@ class Qcow2BitmapExt(Qcow2Struct):
 print()
 entry.dump()

+def to_json(self):
+fields_dict = super().to_json()
+fields_dict['bitmap_directory'] = self.bitmap_directory
+return fields_dict
+

 class Qcow2BitmapDirEntry(Qcow2Struct):

@@ -189,6 +197,14 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 super(Qcow2BitmapDirEntry, self).dump()
 self.bitmap_table.dump()

+def to_json(self):
+# Put the name ahead of the dict
+return {
+'name': self.name,
+**super().to_json(),
+'bitmap_table': self.bitmap_table
+}
+

 class Qcow2BitmapTableEntry(Qcow2Struct):

@@ -214,6 +230,10 @@ class Qcow2BitmapTableEntry(Qcow2Struct):
 else:
 self.type = 'all-zeroes'

+def to_json(self):
+return {'type': self.type, 'offset': self.offset,
+'reserved': self.reserved}
+

 class Qcow2BitmapTable:

@@ -234,6 +254,9 @@ class Qcow2BitmapTable:
 size = 0
 print(f'{i:<14} {entry.type:<15} {size:<12} {entry.offset}')

+def to_json(self):
+return self.entries
+

 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875

@@ -249,6 +272,9 @@ class QcowHeaderExtension(Qcow2Struct):
 0x44415441: 'Data file'
 }

+def to_json(self):
+return self.mapping.get(self.value, "")
+
 fields = (
 ('u32', Magic, 'magic'),
 ('u32', '{}', 'length')
@@ -311,6 +337,16 @@ class QcowHeaderExtension(Qcow2Struct):
 else:
 self.obj.dump()

+def to_json(self):
+# Put the name ahead of the dict
+res = {'name': self.Magic(self.magic), **super().to_json()}
+if self.obj is not None:
+res['data'] = self.obj
+else:
+res['data_str'] = self.data_str
+
+return res
+
 @classmethod
 def create(cls, magic, data):
 return QcowHeaderExtension(magic, len(data), data)
-- 
2.28.0




[PULL 14/14] iotests: Test node/bitmap aliases during migration

2020-08-21 Thread Eric Blake
From: Max Reitz 

Signed-off-by: Max Reitz 
Message-Id: <20200820150725.68687-4-mre...@redhat.com>
Reviewed-by: Eric Blake 
Tested-by: Eric Blake 
[eblake: fold in python cleanups recommended by Vladimir]
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/300 | 593 +
 tests/qemu-iotests/300.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 599 insertions(+)
 create mode 100755 tests/qemu-iotests/300
 create mode 100644 tests/qemu-iotests/300.out

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
new file mode 100755
index ..5b75121b8496
--- /dev/null
+++ b/tests/qemu-iotests/300
@@ -0,0 +1,593 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2020 Red Hat, Inc.
+#
+# Tests for dirty bitmaps migration with node aliases
+#
+# 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 <http://www.gnu.org/licenses/>.
+#
+
+import os
+import random
+import re
+from typing import Dict, List, Optional, Union
+import iotests
+import qemu
+
+BlockBitmapMapping = List[Dict[str, Union[str, List[Dict[str, str]
+
+assert iotests.sock_dir is not None
+mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
+
+
+class TestDirtyBitmapMigration(iotests.QMPTestCase):
+src_node_name: str = ''
+dst_node_name: str = ''
+src_bmap_name: str = ''
+dst_bmap_name: str = ''
+
+def setUp(self) -> None:
+self.vm_a = iotests.VM(path_suffix='-a')
+self.vm_a.add_blockdev(f'node-name={self.src_node_name},'
+   'driver=null-co')
+self.vm_a.launch()
+
+self.vm_b = iotests.VM(path_suffix='-b')
+self.vm_b.add_blockdev(f'node-name={self.dst_node_name},'
+   'driver=null-co')
+self.vm_b.add_incoming(f'unix:{mig_sock}')
+self.vm_b.launch()
+
+result = self.vm_a.qmp('block-dirty-bitmap-add',
+   node=self.src_node_name,
+   name=self.src_bmap_name)
+self.assert_qmp(result, 'return', {})
+
+# Dirty some random megabytes
+for _ in range(9):
+mb_ofs = random.randrange(1024)
+self.vm_a.hmp_qemu_io(self.src_node_name, f'discard {mb_ofs}M 1M')
+
+result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',
+   node=self.src_node_name,
+   name=self.src_bmap_name)
+self.bitmap_hash_reference = result['return']['sha256']
+
+caps = [{'capability': name, 'state': True}
+for name in ('dirty-bitmaps', 'events')]
+
+for vm in (self.vm_a, self.vm_b):
+result = vm.qmp('migrate-set-capabilities', capabilities=caps)
+self.assert_qmp(result, 'return', {})
+
+def tearDown(self) -> None:
+self.vm_a.shutdown()
+self.vm_b.shutdown()
+try:
+os.remove(mig_sock)
+except OSError:
+pass
+
+def check_bitmap(self, bitmap_name_valid: bool) -> None:
+result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',
+   node=self.dst_node_name,
+   name=self.dst_bmap_name)
+if bitmap_name_valid:
+self.assert_qmp(result, 'return/sha256',
+self.bitmap_hash_reference)
+else:
+self.assert_qmp(result, 'error/desc',
+f"Dirty bitmap '{self.dst_bmap_name}' not found")
+
+def migrate(self, bitmap_name_valid: bool = True,
+migration_success: bool = True) -> None:
+result = self.vm_a.qmp('migrate', uri=f'unix:{mig_sock}')
+self.assert_qmp(result, 'return', {})
+
+with iotests.Timeout(5, 'Timeout waiting for migration to complete'):
+self.assertEqual(self.vm_a.wait_migration('postmigrate'),
+ migration_success)
+self.assertEqual(self.vm_b.wait_migration('running'),
+ migration_success)
+
+if migration_success:
+self.check_bitmap(bitmap_name_valid)
+
+def verify_dest_error(self, msg: Optional[str]) -> None:
+"""
+Check whether the given error message is present in vm_b's log.
+(vm_b is shut down to do so.)
+If @msg is None, check that there has not been

[PULL 13/14] iotests.py: Let wait_migration() return on failure

2020-08-21 Thread Eric Blake
From: Max Reitz 

Let wait_migration() return on failure (with the return value indicating
whether the migration was completed or has failed), so we can use it for
migrations that are expected to fail, too.

Signed-off-by: Max Reitz 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20200820150725.68687-3-mre...@redhat.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/iotests.py | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 717b5b652c45..e197c73ca501 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -729,16 +729,22 @@ class VM(qtest.QEMUQtestMachine):
 }
 ]))

-def wait_migration(self, expect_runstate):
+def wait_migration(self, expect_runstate: Optional[str]) -> bool:
 while True:
 event = self.event_wait('MIGRATION')
 log(event, filters=[filter_qmp_event])
-if event['data']['status'] == 'completed':
+if event['data']['status'] in ('completed', 'failed'):
 break
-# The event may occur in finish-migrate, so wait for the expected
-# post-migration runstate
-while self.qmp('query-status')['return']['status'] != expect_runstate:
-pass
+
+if event['data']['status'] == 'completed':
+# The event may occur in finish-migrate, so wait for the expected
+# post-migration runstate
+runstate = None
+while runstate != expect_runstate:
+runstate = self.qmp('query-status')['return']['status']
+return True
+else:
+return False

 def node_info(self, node_name):
 nodes = self.qmp('query-named-block-nodes')
-- 
2.28.0




[PULL 07/14] qcow2_format.py: Dump bitmap table serialized entries

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

Add bitmap table information to the QCOW2 metadata dump.

Bitmap name   bitmap-1
...
Bitmap table   typesize offset
0  serialized  6553610092544
1  all-zeroes  00
2  all-zeroes  00

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <1596742557-320265-8-git-send-email-andrey.shinkev...@virtuozzo.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/303.out |  4 +++
 tests/qemu-iotests/qcow2_format.py | 50 ++
 2 files changed, 54 insertions(+)

diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
index 038ba93a87d7..70828e05f11f 100644
--- a/tests/qemu-iotests/303.out
+++ b/tests/qemu-iotests/303.out
@@ -66,6 +66,8 @@ type  1
 granularity_bits  15
 name_size 8
 extra_data_size   0
+Bitmap table   typesize offset
+0  serialized  6553610092544

 Bitmap name   bitmap-2
 bitmap_table_offset   0x9c
@@ -75,4 +77,6 @@ type  1
 granularity_bits  16
 name_size 8
 extra_data_size   0
+Bitmap table   typesize offset
+0  all-zeroes  00

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index ca0d3501e0a2..574249bc463c 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -175,6 +175,10 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 entry_raw_size = self.bitmap_dir_entry_raw_size()
 padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
 fd.seek(padding, 1)
+self.bitmap_table = Qcow2BitmapTable(fd=fd,
+ offset=self.bitmap_table_offset,
+ nb_entries=self.bitmap_table_size,
+ cluster_size=self.cluster_size)

 def bitmap_dir_entry_raw_size(self):
 return struct.calcsize(self.fmt) + self.name_size + \
@@ -183,6 +187,52 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 def dump(self):
 print(f'{"Bitmap name":<25} {self.name}')
 super(Qcow2BitmapDirEntry, self).dump()
+self.bitmap_table.dump()
+
+
+class Qcow2BitmapTableEntry(Qcow2Struct):
+
+fields = (
+('u64',  '{}', 'entry'),
+)
+
+BME_TABLE_ENTRY_RESERVED_MASK = 0xff0001fe
+BME_TABLE_ENTRY_OFFSET_MASK = 0x00fffe00
+BME_TABLE_ENTRY_FLAG_ALL_ONES = 1
+
+def __init__(self, fd):
+super().__init__(fd=fd)
+self.reserved = self.entry & self.BME_TABLE_ENTRY_RESERVED_MASK
+self.offset = self.entry & self.BME_TABLE_ENTRY_OFFSET_MASK
+if self.offset:
+if self.entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
+self.type = 'invalid'
+else:
+self.type = 'serialized'
+elif self.entry & self.BME_TABLE_ENTRY_FLAG_ALL_ONES:
+self.type = 'all-ones'
+else:
+self.type = 'all-zeroes'
+
+
+class Qcow2BitmapTable:
+
+def __init__(self, fd, offset, nb_entries, cluster_size):
+self.cluster_size = cluster_size
+position = fd.tell()
+fd.seek(offset)
+self.entries = [Qcow2BitmapTableEntry(fd) for _ in range(nb_entries)]
+fd.seek(position)
+
+def dump(self):
+bitmap_table = enumerate(self.entries)
+print(f'{"Bitmap table":<14} {"type":<15} {"size":<12} {"offset"}')
+for i, entry in bitmap_table:
+if entry.type == 'serialized':
+size = self.cluster_size
+else:
+size = 0
+print(f'{i:<14} {entry.type:<15} {size:<12} {entry.offset}')


 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875
-- 
2.28.0




[PULL 11/14] iotests: dump QCOW2 header in JSON in #303

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

Extend the test case #303 by dumping QCOW2 image metadata in JSON
format.

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: 
<1596742557-320265-12-git-send-email-andrey.shinkev...@virtuozzo.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/303 |  3 ++
 tests/qemu-iotests/303.out | 76 ++
 2 files changed, 79 insertions(+)

diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
index e9accdc7bc92..6c2177448348 100755
--- a/tests/qemu-iotests/303
+++ b/tests/qemu-iotests/303
@@ -58,3 +58,6 @@ add_bitmap(1, 0, 6, False)
 add_bitmap(2, 6, 8, True)
 dump = ['qcow2.py', disk, 'dump-header']
 subprocess.run(dump)
+# Dump the metadata in JSON format
+dump.append('-j')
+subprocess.run(dump)
diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
index 70828e05f11f..7fa1edef0d89 100644
--- a/tests/qemu-iotests/303.out
+++ b/tests/qemu-iotests/303.out
@@ -80,3 +80,79 @@ extra_data_size   0
 Bitmap table   typesize offset
 0  all-zeroes  00

+{
+"magic": 1363560955,
+"version": 3,
+"backing_file_offset": 0,
+"backing_file_size": 0,
+"cluster_bits": 16,
+"size": 10485760,
+"crypt_method": 0,
+"l1_size": 1,
+"l1_table_offset": 196608,
+"refcount_table_offset": 65536,
+"refcount_table_clusters": 1,
+"nb_snapshots": 0,
+"snapshot_offset": 0,
+"incompatible_features": 0,
+"compatible_features": 0,
+"autoclear_features": 1,
+"refcount_order": 4,
+"header_length": 112
+}
+
+[
+{
+"name": "Feature table",
+"magic": 1745090647,
+"length": 336,
+"data_str": ""
+},
+{
+"name": "Bitmaps",
+"magic": 595929205,
+"length": 24,
+"data": {
+"nb_bitmaps": 2,
+"reserved32": 0,
+"bitmap_directory_size": 64,
+"bitmap_directory_offset": 10289152,
+"bitmap_directory": [
+{
+"name": "bitmap-1",
+"bitmap_table_offset": 10158080,
+"bitmap_table_size": 1,
+"flags": 2,
+"type": 1,
+"granularity_bits": 15,
+"name_size": 8,
+"extra_data_size": 0,
+"bitmap_table": [
+{
+"type": "serialized",
+"offset": 10092544,
+"reserved": 0
+}
+]
+},
+{
+"name": "bitmap-2",
+"bitmap_table_offset": 10223616,
+"bitmap_table_size": 1,
+"flags": 0,
+"type": 1,
+"granularity_bits": 16,
+"name_size": 8,
+"extra_data_size": 0,
+"bitmap_table": [
+{
+"type": "all-zeroes",
+"offset": 0,
+"reserved": 0
+}
+]
+}
+]
+}
+}
+]
-- 
2.28.0




[PULL 08/14] qcow2.py: Introduce '-j' key to dump in JSON format

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

Add the command key to the qcow2.py arguments list to dump QCOW2
metadata in JSON format. Here is the suggested way to do that. The
implementation of the dump in JSON format is in the patch that follows.

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <1596742557-320265-9-git-send-email-andrey.shinkev...@virtuozzo.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/qcow2.py| 18 ++
 tests/qemu-iotests/qcow2_format.py |  4 ++--
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index 0910e6ac0705..77ca59cc663d 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -26,16 +26,19 @@ from qcow2_format import (
 )


+is_json = False
+
+
 def cmd_dump_header(fd):
 h = QcowHeader(fd)
-h.dump()
+h.dump(is_json)
 print()
-h.dump_extensions()
+h.dump_extensions(is_json)


 def cmd_dump_header_exts(fd):
 h = QcowHeader(fd)
-h.dump_extensions()
+h.dump_extensions(is_json)


 def cmd_set_header(fd, name, value):
@@ -151,11 +154,14 @@ def main(filename, cmd, args):


 def usage():
-print("Usage: %s   [, ...]" % sys.argv[0])
+print("Usage: %s   [, ...] [, ...]" % sys.argv[0])
 print("")
 print("Supported commands:")
 for name, handler, num_args, desc in cmds:
 print("%-20s - %s" % (name, desc))
+print("")
+print("Supported keys:")
+print("%-20s - %s" % ('-j', 'Dump in JSON format'))


 if __name__ == '__main__':
@@ -163,4 +169,8 @@ if __name__ == '__main__':
 usage()
 sys.exit(1)

+is_json = '-j' in sys.argv
+if is_json:
+sys.argv.remove('-j')
+
 main(sys.argv[1], sys.argv[2], sys.argv[3:])
diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 574249bc463c..de0adcbf9db0 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -109,7 +109,7 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
 self.__dict__ = dict((field[2], values[i])
  for i, field in enumerate(self.fields))

-def dump(self):
+def dump(self, is_json=False):
 for f in self.fields:
 value = self.__dict__[f[2]]
 if isinstance(f[1], str):
@@ -408,7 +408,7 @@ class QcowHeader(Qcow2Struct):
 buf = buf[0:header_bytes-1]
 fd.write(buf)

-def dump_extensions(self):
+def dump_extensions(self, is_json=False):
 for ex in self.extensions:
 print('Header extension:')
 ex.dump()
-- 
2.28.0




[PULL 06/14] qcow2_format.py: pass cluster size to substructures

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

The cluster size of an image is the QcowHeader class member and may be
obtained by dependent extension structures such as Qcow2BitmapExt for
further bitmap table details print.

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <1596742557-320265-7-git-send-email-andrey.shinkev...@virtuozzo.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/qcow2_format.py | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 05a8aa98f72c..ca0d3501e0a2 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -129,19 +129,21 @@ class Qcow2BitmapExt(Qcow2Struct):
 ('u64', '{:#x}', 'bitmap_directory_offset')
 )

-def __init__(self, fd):
+def __init__(self, fd, cluster_size):
 super().__init__(fd=fd)
 tail = struct.calcsize(self.fmt) % 8
 if tail:
 fd.seek(8 - tail, 1)
 position = fd.tell()
+self.cluster_size = cluster_size
 self.read_bitmap_directory(fd)
 fd.seek(position)

 def read_bitmap_directory(self, fd):
 fd.seek(self.bitmap_directory_offset)
 self.bitmap_directory = \
-[Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]
+[Qcow2BitmapDirEntry(fd, cluster_size=self.cluster_size)
+ for _ in range(self.nb_bitmaps)]

 def dump(self):
 super().dump()
@@ -162,8 +164,9 @@ class Qcow2BitmapDirEntry(Qcow2Struct):
 ('u32', '{}', 'extra_data_size')
 )

-def __init__(self, fd):
+def __init__(self, fd, cluster_size):
 super().__init__(fd=fd)
+self.cluster_size = cluster_size
 # Seek relative to the current position in the file
 fd.seek(self.extra_data_size, 1)
 bitmap_name = fd.read(self.name_size)
@@ -203,11 +206,13 @@ class QcowHeaderExtension(Qcow2Struct):
 # then padding to next multiply of 8
 )

-def __init__(self, magic=None, length=None, data=None, fd=None):
+def __init__(self, magic=None, length=None, data=None, fd=None,
+ cluster_size=None):
 """
 Support both loading from fd and creation from user data.
 For fd-based creation current position in a file will be used to read
 the data.
+The cluster_size value may be obtained by dependent structures.

 This should be somehow refactored and functionality should be moved to
 superclass (to allow creation of any qcow2 struct), but then, fields
@@ -230,7 +235,7 @@ class QcowHeaderExtension(Qcow2Struct):
 assert all(v is None for v in (magic, length, data))
 super().__init__(fd=fd)
 if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
-self.obj = Qcow2BitmapExt(fd=fd)
+self.obj = Qcow2BitmapExt(fd=fd, cluster_size=cluster_size)
 self.data = None
 else:
 padded = (self.length + 7) & ~7
@@ -319,7 +324,7 @@ class QcowHeader(Qcow2Struct):
 end = self.cluster_size

 while fd.tell() < end:
-ext = QcowHeaderExtension(fd=fd)
+ext = QcowHeaderExtension(fd=fd, cluster_size=self.cluster_size)
 if ext.magic == 0:
 break
 else:
-- 
2.28.0




[PULL 01/14] iotests: add test for QCOW2 header dump

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

The simple script creates a QCOW2 image and fills it with some data.
Two bitmaps are created as well. Then the script reads the image header
with extensions from the disk by running the script qcow2.py and dumps
the information to the output. Other entities, such as snapshots, may
be added to the test later.

Suggested-by: Eric Blake 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <1596742557-320265-2-git-send-email-andrey.shinkev...@virtuozzo.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/303 | 60 ++
 tests/qemu-iotests/303.out | 60 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 121 insertions(+)
 create mode 100755 tests/qemu-iotests/303
 create mode 100644 tests/qemu-iotests/303.out

diff --git a/tests/qemu-iotests/303 b/tests/qemu-iotests/303
new file mode 100755
index ..e9accdc7bc92
--- /dev/null
+++ b/tests/qemu-iotests/303
@@ -0,0 +1,60 @@
+#!/usr/bin/env python3
+#
+# Test for dumping of qcow2 image metadata
+#
+# Copyright (c) 2020 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 <http://www.gnu.org/licenses/>.
+#
+
+import iotests
+import subprocess
+from iotests import qemu_img_create, qemu_io, file_path, log, filter_qemu_io
+
+iotests.script_initialize(supported_fmts=['qcow2'])
+
+disk = file_path('disk')
+chunk = 1024 * 1024
+
+
+def create_bitmap(bitmap_number, disabled):
+granularity = 1 << (14 + bitmap_number)
+bitmap_name = 'bitmap-' + str(bitmap_number)
+args = ['bitmap', '--add', '-g', f'{granularity}', '-f', iotests.imgfmt,
+disk, bitmap_name]
+if disabled:
+args.append('--disable')
+
+iotests.qemu_img_pipe(*args)
+
+
+def write_to_disk(offset, size):
+write = f'write {offset} {size}'
+log(qemu_io('-c', write, disk), filters=[filter_qemu_io])
+
+
+def add_bitmap(num, begin, end, disabled):
+log(f'Add bitmap {num}')
+create_bitmap(num, disabled)
+for i in range(begin, end):
+write_to_disk((i) * chunk, chunk)
+log('')
+
+
+qemu_img_create('-f', iotests.imgfmt, disk, '10M')
+
+add_bitmap(1, 0, 6, False)
+add_bitmap(2, 6, 8, True)
+dump = ['qcow2.py', disk, 'dump-header']
+subprocess.run(dump)
diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
new file mode 100644
index ..8d7973ccc201
--- /dev/null
+++ b/tests/qemu-iotests/303.out
@@ -0,0 +1,60 @@
+Add bitmap 1
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 1048576
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 2097152
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 3145728
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 4194304
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 5242880
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
+Add bitmap 2
+wrote 1048576/1048576 bytes at offset 6291456
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+wrote 1048576/1048576 bytes at offset 7340032
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+
+magic 0x514649fb
+version   3
+backing_file_offset   0x0
+backing_file_size 0x0
+cluster_bits  16
+size  10485760
+crypt_method  0
+l1_size   1
+l1_table_offset   0x3
+refcount_table_offset 0x1
+refcount_table_clusters   1
+nb_snapshots  0
+snapshot_offset   0x0
+incompatible_features []
+compatible_features   []
+autoclear_features[0]
+refcount_order4
+header_length 112
+
+Header extension:
+magic 0x6803f857 (Feature table)
+length336
+data  
+
+Header extension:
+magic 0x23852875 (Bitmaps)
+length24
+nb_bitmaps2
+reserved320
+bitmap_directory_size 0x40
+bitmap_directory_offset   0x9d
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 7f76066640a6..e

[PULL 04/14] qcow2_format.py: dump bitmap flags in human readable way.

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

Introduce the class BitmapFlags that parses a bitmap flags mask.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <1596742557-320265-5-git-send-email-andrey.shinkev...@virtuozzo.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/qcow2_format.py | 16 
 1 file changed, 16 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index d4a997453758..b4473442c9d4 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -40,6 +40,22 @@ class Flags64(Qcow2Field):
 return str(bits)


+class BitmapFlags(Qcow2Field):
+
+flags = {
+0x1: 'in-use',
+0x2: 'auto'
+}
+
+def __str__(self):
+bits = []
+for bit in range(64):
+flag = self.value & (1 << bit)
+if flag:
+bits.append(self.flags.get(flag, f'bit-{bit}'))
+return f'{self.value:#x} ({bits})'
+
+
 class Enum(Qcow2Field):

 def __str__(self):
-- 
2.28.0




[PULL 12/14] migration: Add block-bitmap-mapping parameter

2020-08-21 Thread Eric Blake
From: Max Reitz 

This migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.

This way, management tools can use different node and bitmap names on
the source and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).

While touching this code, fix a bug where bitmap names longer than 255
bytes would fail an assertion in qemu_put_counted_string().

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
Message-Id: <20200820150725.68687-2-mre...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Eric Blake 
---
 qapi/migration.json| 104 -
 migration/migration.h  |   3 +
 migration/block-dirty-bitmap.c | 410 -
 migration/migration.c  |  30 +++
 monitor/hmp-cmds.c |  30 +++
 5 files changed, 521 insertions(+), 56 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index ea53b23dca90..5f6b06172cab 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -508,6 +508,44 @@
   'data': [ 'none', 'zlib',
 { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }

+##
+# @BitmapMigrationBitmapAlias:
+#
+# @name: The name of the bitmap.
+#
+# @alias: An alias name for migration (for example the bitmap name on
+# the opposite site).
+#
+# Since: 5.2
+##
+{ 'struct': 'BitmapMigrationBitmapAlias',
+  'data': {
+  'name': 'str',
+  'alias': 'str'
+  } }
+
+##
+# @BitmapMigrationNodeAlias:
+#
+# Maps a block node name and the bitmaps it has to aliases for dirty
+# bitmap migration.
+#
+# @node-name: A block node name.
+#
+# @alias: An alias block node name for migration (for example the
+# node name on the opposite site).
+#
+# @bitmaps: Mappings for the bitmaps on this node.
+#
+# Since: 5.2
+##
+{ 'struct': 'BitmapMigrationNodeAlias',
+  'data': {
+  'node-name': 'str',
+  'alias': 'str',
+  'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
+  } }
+
 ##
 # @MigrationParameter:
 #
@@ -642,6 +680,25 @@
 #  will consume more CPU.
 #  Defaults to 1. (Since 5.0)
 #
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+#  aliases for the purpose of dirty bitmap migration.  Such
+#  aliases may for example be the corresponding names on the
+#  opposite site.
+#  The mapping must be one-to-one, but not necessarily
+#  complete: On the source, unmapped bitmaps and all bitmaps
+#  on unmapped nodes will be ignored.  On the destination,
+#  encountering an unmapped alias in the incoming migration
+#  stream will result in a report, and all further bitmap
+#  migration data will then be discarded.
+#  Note that the destination does not know about bitmaps it
+#  does not receive, so there is no limitation or requirement
+#  regarding the number of bitmaps received, or how they are
+#  named, or on which nodes they are placed.
+#  By default (when this parameter has never been set), bitmap
+#  names are mapped to themselves.  Nodes are mapped to their
+#  block device name if there is one, and to their node name
+#  otherwise. (Since 5.2)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
@@ -656,7 +713,8 @@
'multifd-channels',
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
-   'multifd-zlib-level' ,'multifd-zstd-level' ] }
+   'multifd-zlib-level' ,'multifd-zstd-level',
+   'block-bitmap-mapping' ] }

 ##
 # @MigrateSetParameters:
@@ -782,6 +840,25 @@
 #  will consume more CPU.
 #  Defaults to 1. (Since 5.0)
 #
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+#  aliases for the purpose of dirty bitmap migration.  Such
+#  aliases may for example be the corresponding names on the
+#  opposite site.
+#  The mapping must be one-to-one, but not necessarily
+#  complete: On the source, unmapped bitmaps and all bitmaps
+#  on unmapped nodes will be ignored.  On the destination,
+#  encountering an unmapped alias in the incoming migration
+#  stream will result in a report, and all further bitmap
+#  migration data will then be discarded.
+#  Note that the destination does not know about bitmaps it
+#  does not receive, so there is no limitation or requirement
+#  regarding the number of bitmaps received, or how they are
+#  named, or on which nodes they are placed.
+#  By default (when this parameter has never been set), bitmap
+#  names are mapped to themselves.  Nodes are mapped to their
+#  block device name if there is one, and to their nod

[PULL 03/14] qcow2_format.py: change Qcow2BitmapExt initialization method

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

There are two ways to initialize a class derived from Qcow2Struct:
1. Pass a block of binary data to the constructor.
2. Pass the file descriptor to allow reading the file from constructor.
Let's change the Qcow2BitmapExt initialization method from 1 to 2 to
support a scattered reading in the initialization chain.
The implementation comes with the patch that follows.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <1596742557-320265-4-git-send-email-andrey.shinkev...@virtuozzo.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/qcow2_format.py | 34 ++
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 2f3681bb5f7c..d4a997453758 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -113,6 +113,11 @@ class Qcow2BitmapExt(Qcow2Struct):
 ('u64', '{:#x}', 'bitmap_directory_offset')
 )

+def __init__(self, fd):
+super().__init__(fd=fd)
+tail = struct.calcsize(self.fmt) % 8
+if tail:
+fd.seek(8 - tail, 1)

 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875

@@ -161,21 +166,24 @@ class QcowHeaderExtension(Qcow2Struct):
 else:
 assert all(v is None for v in (magic, length, data))
 super().__init__(fd=fd)
-padded = (self.length + 7) & ~7
-self.data = fd.read(padded)
-assert self.data is not None
+if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
+self.obj = Qcow2BitmapExt(fd=fd)
+self.data = None
+else:
+padded = (self.length + 7) & ~7
+self.data = fd.read(padded)
+assert self.data is not None
+self.obj = None

-data_str = self.data[:self.length]
-if all(c in string.printable.encode('ascii') for c in data_str):
-data_str = f"'{ data_str.decode('ascii') }'"
-else:
-data_str = ''
-self.data_str = data_str
+if self.data is not None:
+data_str = self.data[:self.length]
+if all(c in string.printable.encode(
+'ascii') for c in data_str):
+data_str = f"'{ data_str.decode('ascii') }'"
+else:
+data_str = ''
+self.data_str = data_str

-if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
-self.obj = Qcow2BitmapExt(data=self.data)
-else:
-self.obj = None

 def dump(self):
 super().dump()
-- 
2.28.0




[PULL 10/14] qcow2_format.py: support dumping metadata in JSON format

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

Implementation of dumping QCOW2 image metadata.
The sample output:
{
"Header_extensions": [
{
"name": "Feature table",
"magic": 1745090647,
"length": 192,
"data_str": ""
},
{
"name": "Bitmaps",
"magic": 595929205,
"length": 24,
"data": {
"nb_bitmaps": 2,
"reserved32": 0,
"bitmap_directory_size": 64,
"bitmap_directory_offset": 1048576,
"bitmap_directory": [
{
"name": "bitmap-1",
"bitmap_table_offset": 589824,
"bitmap_table_size": 1,
"flags": 2,
"type": 1,
"granularity_bits": 15,
"name_size": 8,
"extra_data_size": 0,
"bitmap_table": [
{
"type": "serialized",
"offset": 655360
},
...

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: 
<1596742557-320265-11-git-send-email-andrey.shinkev...@virtuozzo.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/qcow2_format.py | 17 +
 1 file changed, 17 insertions(+)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index 5a298b2f1357..8adc9959e10b 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -19,6 +19,15 @@

 import struct
 import string
+import json
+
+
+class ComplexEncoder(json.JSONEncoder):
+def default(self, obj):
+if hasattr(obj, 'to_json'):
+return obj.to_json()
+else:
+return json.JSONEncoder.default(self, obj)


 class Qcow2Field:
@@ -110,6 +119,10 @@ class Qcow2Struct(metaclass=Qcow2StructMeta):
  for i, field in enumerate(self.fields))

 def dump(self, is_json=False):
+if is_json:
+print(json.dumps(self.to_json(), indent=4, cls=ComplexEncoder))
+return
+
 for f in self.fields:
 value = self.__dict__[f[2]]
 if isinstance(f[1], str):
@@ -445,6 +458,10 @@ class QcowHeader(Qcow2Struct):
 fd.write(buf)

 def dump_extensions(self, is_json=False):
+if is_json:
+print(json.dumps(self.extensions, indent=4, cls=ComplexEncoder))
+return
+
 for ex in self.extensions:
 print('Header extension:')
 ex.dump()
-- 
2.28.0




[PULL 05/14] qcow2_format.py: Dump bitmap directory information

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

Read and dump entries from the bitmap directory of QCOW2 image.

Header extension:
magic 0x23852875 (Bitmaps)
...
Bitmap name   bitmap-1
bitmap_table_offset   0xf
bitmap_table_size 1
flags 0x2 (['auto'])
type  1
granularity_bits  16
name_size 8
extra_data_size   0

Suggested-by: Kevin Wolf 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <1596742557-320265-6-git-send-email-andrey.shinkev...@virtuozzo.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/303.out | 18 
 tests/qemu-iotests/qcow2_format.py | 47 ++
 2 files changed, 65 insertions(+)

diff --git a/tests/qemu-iotests/303.out b/tests/qemu-iotests/303.out
index 8d7973ccc201..038ba93a87d7 100644
--- a/tests/qemu-iotests/303.out
+++ b/tests/qemu-iotests/303.out
@@ -58,3 +58,21 @@ reserved320
 bitmap_directory_size 0x40
 bitmap_directory_offset   0x9d

+Bitmap name   bitmap-1
+bitmap_table_offset   0x9b
+bitmap_table_size 1
+flags 0x2 (['auto'])
+type  1
+granularity_bits  15
+name_size 8
+extra_data_size   0
+
+Bitmap name   bitmap-2
+bitmap_table_offset   0x9c
+bitmap_table_size 1
+flags 0x0 ([])
+type  1
+granularity_bits  16
+name_size 8
+extra_data_size   0
+
diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index b4473442c9d4..05a8aa98f72c 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -134,6 +134,53 @@ class Qcow2BitmapExt(Qcow2Struct):
 tail = struct.calcsize(self.fmt) % 8
 if tail:
 fd.seek(8 - tail, 1)
+position = fd.tell()
+self.read_bitmap_directory(fd)
+fd.seek(position)
+
+def read_bitmap_directory(self, fd):
+fd.seek(self.bitmap_directory_offset)
+self.bitmap_directory = \
+[Qcow2BitmapDirEntry(fd) for _ in range(self.nb_bitmaps)]
+
+def dump(self):
+super().dump()
+for entry in self.bitmap_directory:
+print()
+entry.dump()
+
+
+class Qcow2BitmapDirEntry(Qcow2Struct):
+
+fields = (
+('u64', '{:#x}', 'bitmap_table_offset'),
+('u32', '{}', 'bitmap_table_size'),
+('u32', BitmapFlags, 'flags'),
+('u8',  '{}', 'type'),
+('u8',  '{}', 'granularity_bits'),
+('u16', '{}', 'name_size'),
+('u32', '{}', 'extra_data_size')
+)
+
+def __init__(self, fd):
+super().__init__(fd=fd)
+# Seek relative to the current position in the file
+fd.seek(self.extra_data_size, 1)
+bitmap_name = fd.read(self.name_size)
+self.name = bitmap_name.decode('ascii')
+# Move position to the end of the entry in the directory
+entry_raw_size = self.bitmap_dir_entry_raw_size()
+padding = ((entry_raw_size + 7) & ~7) - entry_raw_size
+fd.seek(padding, 1)
+
+def bitmap_dir_entry_raw_size(self):
+return struct.calcsize(self.fmt) + self.name_size + \
+self.extra_data_size
+
+def dump(self):
+print(f'{"Bitmap name":<25} {self.name}')
+super(Qcow2BitmapDirEntry, self).dump()
+

 QCOW2_EXT_MAGIC_BITMAPS = 0x23852875

-- 
2.28.0




[PULL 02/14] qcow2_format.py: make printable data an extension class member

2020-08-21 Thread Eric Blake
From: Andrey Shinkevich 

Let us differ binary data type from string one for the extension data
variable and keep the string as the QcowHeaderExtension class member.

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <1596742557-320265-3-git-send-email-andrey.shinkev...@virtuozzo.com>
Signed-off-by: Eric Blake 
---
 tests/qemu-iotests/qcow2_format.py | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tests/qemu-iotests/qcow2_format.py 
b/tests/qemu-iotests/qcow2_format.py
index cc432e7ae06c..2f3681bb5f7c 100644
--- a/tests/qemu-iotests/qcow2_format.py
+++ b/tests/qemu-iotests/qcow2_format.py
@@ -165,6 +165,13 @@ class QcowHeaderExtension(Qcow2Struct):
 self.data = fd.read(padded)
 assert self.data is not None

+data_str = self.data[:self.length]
+if all(c in string.printable.encode('ascii') for c in data_str):
+data_str = f"'{ data_str.decode('ascii') }'"
+else:
+data_str = ''
+self.data_str = data_str
+
 if self.magic == QCOW2_EXT_MAGIC_BITMAPS:
 self.obj = Qcow2BitmapExt(data=self.data)
 else:
@@ -174,12 +181,7 @@ class QcowHeaderExtension(Qcow2Struct):
 super().dump()

 if self.obj is None:
-data = self.data[:self.length]
-if all(c in string.printable.encode('ascii') for c in data):
-data = f"'{ data.decode('ascii') }'"
-else:
-data = ''
-print(f'{"data":<25} {data}')
+print(f'{"data":<25} {self.data_str}')
 else:
 self.obj.dump()

-- 
2.28.0




Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

2020-08-20 Thread Eric Blake

On 8/20/20 10:49 AM, Vladimir Sementsov-Ogievskiy wrote:

# MYPYPATH=../../python/ mypy 300
300:154: error: Item "None" of "Optional[Match[Any]]" has no attribute 
"group"

Found 1 error in 1 file (checked 1 source file)

- the only complain. Suggest a fix:

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index c6d86b1dbc..0241903743 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -148,11 +148,11 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
  result = vm.qmp('human-monitor-command',
  command_line='info migrate_parameters')

-    hmp_mapping = re.search(r'^block-bitmap-mapping:\r?(\n  
.*)*\n',

-    result['return'], flags=re.MULTILINE)
+    m = re.search(r'^block-bitmap-mapping:\r?(\n  .*)*\n',
+  result['return'], flags=re.MULTILINE)
+    hmp_mapping = m.group(0).replace('\r', '') if m else None

-    self.assertEqual(hmp_mapping.group(0).replace('\r', ''),
- self.to_hmp_mapping(mapping))
+    self.assertEqual(hmp_mapping, self.to_hmp_mapping(mapping))
  else:
  self.assert_qmp(result, 'error/desc', error)


Thanks; I'll fold that in to v5.



===

# flake8 300
300:24:1: F401 'time' imported but unused
300:34:1: E302 expected 2 blank lines, found 1
300:42:67: E502 the backslash is redundant between brackets
300:47:67: E502 the backslash is redundant between brackets
300:67:53: E502 the backslash is redundant between brackets
300:122:9: E125 continuation line with same indent as next logical line
300:134:9: E125 continuation line with same indent as next logical line
300:185:52: E502 the backslash is redundant between brackets
300:186:72: E502 the backslash is redundant between brackets
300:285:77: E502 the backslash is redundant between brackets
300:305:77: E502 the backslash is redundant between brackets
300:306:78: E502 the backslash is redundant between brackets
300:330:77: E502 the backslash is redundant between brackets
300:350:77: E502 the backslash is redundant between brackets
300:385:57: E502 the backslash is redundant between brackets
300:386:59: E502 the backslash is redundant between brackets
300:387:67: E502 the backslash is redundant between brackets
300:412:78: E502 the backslash is redundant between brackets
300:425:78: E502 the backslash is redundant between brackets
300:435:78: E502 the backslash is redundant between brackets
300:436:76: E502 the backslash is redundant between brackets
300:451:66: E502 the backslash is redundant between brackets
300:473:78: E502 the backslash is redundant between brackets
300:474:79: E502 the backslash is redundant between brackets
300:488:78: E502 the backslash is redundant between brackets
300:489:77: E502 the backslash is redundant between brackets

- I post it just because ALE plugin in vim highlights all these things 
for me. Up to you, I don't ask you to fix it.


Seems like an easy enough touchup to make.



+    def test_alias_on_both_migration(self) -> None:
+    src_map = self.mapping(self.src_node_name, 'node-alias',
+   self.src_bmap_name, 'bmap-alias')
+
+    dst_map = self.mapping(self.dst_node_name, 'node-alias',
+   self.dst_bmap_name, 'bmap-alias')
+
+    self.set_mapping(self.vm_a, src_map)
+    self.set_mapping(self.vm_b, dst_map)
+    self.migrate()
+    self.verify_dest_error(None)


Hmm, probably verify_dest_error() should be called from migrate(), as 
you call it (almost) always after migrate()


This one I did not touch; it can be a followup patch if desired.




+
+


[..]


+    def test_unused_mapping_on_dst(self) -> None:
+    # Let the source not send any bitmaps
+    self.set_mapping(self.vm_a, [])
+
+    # Establish some mapping on the destination
+    self.set_mapping(self.vm_b, [])


Seems, you wanted to specify non-empty mapping for vm_b, yes?
With any non-empty mapping here, just to better correspond to the comments:
Reviewed-by: Vladimir Sementsov-Ogievskiy 

(or keep this case with both mappings empty, and add one similar with 
empty mapping only on src)


Adding another case will now have to be a separate patch.  At any rate, 
I've taken the liberty of including your R-b in my staging tree for the 
pending pull request, let me know if I should drop it.


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




Re: [PATCH v5 3/3] iotests: Test node/bitmap aliases during migration

2020-08-20 Thread Eric Blake

On 8/20/20 10:07 AM, Max Reitz wrote:

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/300 | 595 +
  tests/qemu-iotests/300.out |   5 +
  tests/qemu-iotests/group   |   1 +
  3 files changed, 601 insertions(+)
  create mode 100755 tests/qemu-iotests/300
  create mode 100644 tests/qemu-iotests/300.out



The changes in patch 1 helped; the test completes for me in 11 s. 
Minimal differences to v4, and the switch to discard is nice.


Tested-by: Eric Blake 
Reviewed-by: Eric Blake 

I've gone ahead and queued this on my bitmaps tree with testing on top 
of Paolo's meson conversion; I'm prepared to send a pull request once 
that lands.


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




Re: [PATCH v5 1/3] migration: Add block-bitmap-mapping parameter

2020-08-20 Thread Eric Blake

On 8/20/20 10:07 AM, Max Reitz wrote:

This migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.

This way, management tools can use different node and bitmap names on
the source and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).

While touching this code, fix a bug where bitmap names longer than 255
bytes would fail an assertion in qemu_put_counted_string().

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
---


Changes from v4 look sane.

Reviwed-by: Eric Blake 

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




Re: [PATCH] qemu-iotests: Fix FilePaths cleanup

2020-08-20 Thread Eric Blake

On 8/20/20 4:29 PM, Eric Blake wrote:

On 8/20/20 4:19 PM, Nir Soffer wrote:

If os.remove() fails to remove one of the paths, for example if the file
was removed by the test, the cleanup loop would exit silently, without
removing the rest of the files.

Signed-off-by: Nir Soffer 
---
  dtc   | 2 +-
  tests/qemu-iotests/iotests.py | 8 
  2 files changed, 5 insertions(+), 5 deletions(-)




Reviewed-by: Eric Blake 


By the way, what test did you hit this on? If possible, I'd like to add 
a Fixes: tag pointing to a commit that includes the problem.


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




Re: [PATCH] qemu-iotests: Fix FilePaths cleanup

2020-08-20 Thread Eric Blake

On 8/20/20 4:19 PM, Nir Soffer wrote:

If os.remove() fails to remove one of the paths, for example if the file
was removed by the test, the cleanup loop would exit silently, without
removing the rest of the files.

Signed-off-by: Nir Soffer 
---
  dtc   | 2 +-
  tests/qemu-iotests/iotests.py | 8 
  2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/dtc b/dtc
index 85e5d83984..88f18909db 16
--- a/dtc
+++ b/dtc
@@ -1 +1 @@
-Subproject commit 85e5d839847af54efab170f2b1331b2a6421e647
+Subproject commit 88f18909db731a627456f26d779445f84e449536


Modulo the unintended submodule bump,


diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 717b5b652c..16a04df8a3 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -468,11 +468,11 @@ class FilePaths:
  return self.paths
  
  def __exit__(self, exc_type, exc_val, exc_tb):

-try:
-for path in self.paths:
+for path in self.paths:
+try:
  os.remove(path)
-except OSError:
-pass
+except OSError:
+pass
  return False


Reviewed-by: Eric Blake 

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




Re: [RFC PATCH 07/22] block/export: Remove magic from block-export-add

2020-08-20 Thread Eric Blake

On 8/20/20 6:05 AM, Kevin Wolf wrote:


As long as we can keep the compatibility code local to qmp_nbd_*(), I
don't think it's too bad. In particular because it's already written.

Instead of adjusting libvirt to changes in the nbd-* commands, I'd
rather have it change over to block-export-*. I would like to see the
nbd-server-add/remove commands deprecated soon after we have the
replacements.


Makes sense to me. So deprecate nbd-server-add in 5.2, and require 
block-export in 6.1.




+/*
+ * nbd-server-add doesn't complain when a read-only device should be
+ * exported as writable, but simply downgrades it. This is an error with
+ * block-export-add.


I'd be happy with either marking this deprecated now (and fixing it in two
releases), or declaring it a bug in nbd-server-add now (and fixing it
outright).


How about deprecating nbd-server-add completely?


Works for me. Keeping the warts backwards-compatible in nbd-server-add 
is more palatable if we know we are going to drop it wholesale down the 
road.



+/*
+ * nbd-server-add removes the export when the named BlockBackend used for
+ * @device goes away.
+ */
+on_eject_blk = blk_by_name(arg->device);
+if (on_eject_blk) {
+nbd_export_set_on_eject_blk(export, on_eject_blk);
+}


Wait - is the magic export removal tied only to exporting a drive name, and
not a node name?  So as long as libvirt is using only node names whwen
adding exports, a drive being unplugged won't interfere?


Yes, seems so. It's the existing behaviour, I'm only moving the code
around.


Overall, the change makes sense to me, although I'd love to see if we could
go further on the writable vs. read-only issue.


If nbd-server-add will be going away relatively soon, it's probably not
worth the trouble. But if you have reasons to keep it, maybe we should
consider it.


No, I'm fine with the idea of getting rid of nbd-server-add, at which 
point changing it before removal is pointless.



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




Re: [PATCH v4 4/4] iotests: Test node/bitmap aliases during migration

2020-08-19 Thread Eric Blake

On 8/18/20 8:32 AM, Max Reitz wrote:

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/300 | 595 +
  tests/qemu-iotests/300.out |   5 +


Rather sparse output (I hate debugging those sorts of outputs when the 
test is failing).



  tests/qemu-iotests/group   |   1 +
  3 files changed, 601 insertions(+)
  create mode 100755 tests/qemu-iotests/300
  create mode 100644 tests/qemu-iotests/300.out




+# Dirty some random megabytes
+for _ in range(9):
+mb_ofs = random.randrange(1024)
+self.vm_a.hmp_qemu_io(self.src_node_name, f'write {mb_ofs}M 1M')


It turns out that the discard operation likewise dirties the bitmap, but 
slightly faster (see edb90bbd).  We could optimize it on top, but I'm 
not going to require a micro-optimizing to get it in.  The test takes 
about 12 seconds to run for me, but you didn't mark it as such in 
'group', so that's good; but it turns up a problem:


300  fail   [20:55:54] [20:56:06]output 
mismatch (see 300.out.bad)
--- /home/eblake/qemu-tmp2/tests/qemu-iotests/300.out	2020-08-19 
20:53:11.087791988 -0500
+++ /home/eblake/qemu-tmp2/tests/qemu-iotests/300.out.bad	2020-08-19 
20:56:06.092428756 -0500

@@ -1,5 +1,41 @@
-.
+WARNING:qemu.machine:qemu received signal 11; command: 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64 
-display none -vga none -chardev 
socket,id=mon,path=/tmp/tmp.qT831UThme/qemu-b-798452-monitor.sock -mon 
chardev=mon,mode=control -qtest 
unix:path=/tmp/tmp.qT831UThme/qemu-b-798452-qtest.sock -accel qtest 
-nodefaults -display none -accel qtest -blockdev 
node-name=node0,driver=null-co -incoming unix:/tmp/tmp.qT831UThme/mig_sock"

+.FE...
+==
+ERROR: test_migratee_bitmap_is_not_mapped_on_dst 
(__main__.TestBlockBitmapMappingErrors)

+--
+Traceback (most recent call last):
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", 
line 435, in _do_shutdown

+self._soft_shutdown(timeout, has_quit)
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", 
line 415, in _soft_shutdown

+self._qmp.cmd('quit')
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/qmp.py", 
line 266, in cmd

+return self.cmd_obj(qmp_cmd)
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/qmp.py", 
line 246, in cmd_obj

+self.__sock.sendall(json.dumps(qmp_cmd).encode('utf-8'))
+BrokenPipeError: [Errno 32] Broken pipe
+
+The above exception was the direct cause of the following exception:
+
+Traceback (most recent call last):
+  File "300", line 76, in tearDown
+self.vm_b.shutdown()
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", 
line 465, in shutdown

+self._do_shutdown(timeout, has_quit)
+  File 
"/home/eblake/qemu-tmp2/tests/qemu-iotests/../../python/qemu/machine.py", 
line 438, in _do_shutdown

+raise AbnormalShutdown("Could not perform graceful shutdown") \
+qemu.machine.AbnormalShutdown: Could not perform graceful shutdown
+
+==
+FAIL: test_migratee_bitmap_is_not_mapped_on_dst 
(__main__.TestBlockBitmapMappingErrors)

+--
+Traceback (most recent call last):
+  File "300", line 384, in test_migratee_bitmap_is_not_mapped_on_dst
+self.migrate(False)
+  File "300", line 99, in migrate
+self.assertEqual(self.vm_a.wait_migration('postmigrate'),
+AssertionError: False != True
+
 --
 Ran 37 tests

-OK
+FAILED (failures=1, errors=1)

I'm not sure why I'm seeing that, but it looks like you've got a bad 
deref somewhere in the alias code.



+class TestLongBitmapNames(TestAliasMigration):
+# Giving long bitmap names is OK, as long as there is a short alias for
+# migration
+src_bmap_name = 'a' * 512
+dst_bmap_name = 'b' * 512


This part's new compared to v3 ;)  Looks like you've made several 
enhancements.


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




Re: [PATCH v7 0/7] coroutines: generate wrapper code

2020-08-19 Thread Eric Blake

On 7/27/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote:

27.07.2020 15:48, Stefan Hajnoczi wrote:
On Wed, Jun 10, 2020 at 01:03:29PM +0300, Vladimir Sementsov-Ogievskiy 
wrote:

Hi all!

The aim of the series is to reduce code-duplication and writing
parameters structure-packing by hand around coroutine function wrappers.

Benefits:
  - no code duplication
  - less indirection


Please add documentation so others know when and how to use this.

I suggest adding a docs/devel/coroutine-wrapper.rst document and adding
a code comment to #define generated_co_wrapper pointing to the
documentation.

Please rename coroutine-wrapper.py to block-coroutine-wrapper.py since
it is specific to the block layer.



OK, will do. Thanks for taking a look!


As this series touched Makefile to add a generated .c, you'll also need 
to rebase that part to apply on top of Paolo's meson conversion (cc'ing 
him if you need help figuring it out)


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




Re: [PATCH v4 3/4] iotests.py: Let wait_migration() return on failure

2020-08-19 Thread Eric Blake

On 8/18/20 8:32 AM, Max Reitz wrote:

Let wait_migration() return on failure (with the return value indicating
whether the migration was completed or has failed), so we can use it for
migrations that are expected to fail, too.

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/iotests.py | 18 --
  1 file changed, 12 insertions(+), 6 deletions(-)



Reviewed-by: Eric Blake 

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




Re: [PATCH v4 2/4] iotests.py: Add wait_for_runstate()

2020-08-19 Thread Eric Blake

On 8/18/20 8:32 AM, Max Reitz wrote:

Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/iotests.py | 4 
  1 file changed, 4 insertions(+)


Reviewed-by: Eric Blake 



diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 717b5b652c..ee93cf22db 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -833,6 +833,10 @@ class VM(qtest.QEMUQtestMachine):
 'Found node %s under %s (but expected %s)' % \
 (node['name'], path, expected_node)
  
+def wait_for_runstate(self, runstate: str) -> None:

+while self.qmp('query-status')['return']['status'] != runstate:
+time.sleep(0.2)


This looks like it could inf-loop if we have a bug where the status 
never changes as expected; but I guess CI bots have timeouts at higher 
layers that would detect if such a bug sneaks in.



+
  index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
  
  class QMPTestCase(unittest.TestCase):




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




Re: [PATCH v4 1/4] migration: Add block-bitmap-mapping parameter

2020-08-19 Thread Eric Blake

On 8/18/20 8:32 AM, Max Reitz wrote:

This migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.

This way, management tools can use different node and bitmap names on
the source and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).

While touching this code, fix a bug where bitmap names longer than 255
bytes would fail an assertion in qemu_put_counted_string().

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
---



+##
+# @BitmapMigrationNodeAlias:
+#
+# Maps a block node name and the bitmaps it has to aliases for dirty
+# bitmap migration.
+#
+# @node-name: A block node name.
+#
+# @alias: An alias block node name for migration (for example the
+# node name on the opposite site).
+#
+# @bitmaps: Mappings for the bitmaps on this node.
+#
+# Since: 5.2
+##
+{ 'struct': 'BitmapMigrationNodeAlias',
+  'data': {
+  'node-name': 'str',
+  'alias': 'str',
+  'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
+  } }


Possible change: should 'alias' be optional (if absent, it defaults to 
'node-name')?  But that can be done on top, if we like it.




+static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
+   bool name_to_alias,
+   Error **errp)
+{
+GHashTable *alias_map;
+size_t max_node_name_len =
+sizeof(((BlockDriverState *)NULL)->node_name) - 1;


Looks a bit nicer as = sizeof_field(BlockDriverState, node_name) - 1.


+
+alias_map = g_hash_table_new_full(g_str_hash, g_str_equal,
+  g_free, free_alias_map_inner_node);
+
+for (; bbm; bbm = bbm->next) {
+const BitmapMigrationNodeAlias *bmna = bbm->value;
+const BitmapMigrationBitmapAliasList *bmbal;
+AliasMapInnerNode *amin;
+GHashTable *bitmaps_map;
+const char *node_map_from, *node_map_to;
+
+if (!id_wellformed(bmna->alias)) {
+error_setg(errp, "The node alias '%s' is not well-formed",
+   bmna->alias);
+goto fail;
+}
+
+if (strlen(bmna->alias) > 255) {


Magic number.  UINT8_MAX seems better (since the limit really is due to 
our migration format limiting to one byte).


...

+g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
+
+for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
+const BitmapMigrationBitmapAlias *bmba = bmbal->value;
+const char *bmap_map_from, *bmap_map_to;
+
+if (strlen(bmba->alias) > 255) {


and again


+error_setg(errp,
+   "The bitmap alias '%s' is longer than 255 bytes",
+   bmba->alias);
+goto fail;
+}


Thanks for adding in the length checking since last revision!



@@ -326,12 +538,29 @@ static int add_bitmaps_to_list(DBMSaveState *s, 
BlockDriverState *bs,
  return -1;
  }
  
+if (bitmap_aliases) {

+bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
+if (!bitmap_alias) {
+/* Skip bitmaps with no alias */
+continue;
+}
+} else {
+if (strlen(bitmap_name) > 255) {
+error_report("Cannot migrate bitmap '%s' on node '%s': "
+ "Name is longer than 255 bytes",
+ bitmap_name, bs_name);
+return -1;


Another one.


Reviewed-by: Eric Blake 

I'm happy to make those touchups, and put this on my bitmaps queue for a 
pull request as soon as Paolo's meson stuff stabilizes.



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




Re: [PATCH v13 00/11] iotests: Dump QCOW2 dirty bitmaps metadata

2020-08-19 Thread Eric Blake

On 8/14/20 6:56 AM, Andrey Shinkevich wrote:

Dear Eric!

Vladimir has compeated reviewing this series. I have not received any 
other responses to it so far.


So, is it good for pull request now? Would you please consider taking 
this series as you did it with the Vladimir's related one?


I've spent some time playing with this; I have now queued it on my 
bitmaps tree, and will be posting a pull request as soon as Paolo's 
meson changes settle.  I also made the tweaks suggested on 9/11.


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




Re: [RFC PATCH 15/22] block/export: Move device to BlockExportOptions

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

Every block export needs a block node to export, so move the 'device'
option from BlockExportOptionsNbd to BlockExportOptions.

To maintain compatibility in nbd-server-add, BlockExportOptionsNbd needs
to be wrapped by a new type NbdServerAddOptions that adds 'device' back
because nbd-server-add doesn't use the BlockExportOptions base type.

Signed-off-by: Kevin Wolf 
---



+++ b/qapi/block-export.json
@@ -62,9 +62,8 @@
  ##
  # @BlockExportOptionsNbd:
  #
-# An NBD block export.
-#
-# @device: The device name or node name of the node to be exported
+# An NBD block export (options shared between nbd-server-add and the NBD branch
+# of block-export-add).
  #
  # @name: Export name. If unspecified, the @device parameter is used as the
  #export name. (Since 2.12)
@@ -82,8 +81,21 @@
  # Since: 5.0
  ##
  { 'struct': 'BlockExportOptionsNbd',
-  'data': {'device': 'str', '*name': 'str', '*description': 'str',
-   '*writable': 'bool', '*bitmap': 'str' } }
+  'data': { '*name': 'str', '*description': 'str',
+'*writable': 'bool', '*bitmap': 'str' } }
+
+##
+# @NbdServerAddOptions:
+#
+# An NBD block export.
+#
+# @device: The device name or node name of the node to be exported
+#
+# Since: 5.0


5.2, now?


+##
+{ 'struct': 'NbdServerAddOptions',
+  'base': 'BlockExportOptionsNbd',
+  'data': { 'device': 'str' } }


I understand why nbd sticks with device that can name device or node, but...

  
  ##

  # @nbd-server-add:
@@ -96,7 +108,7 @@
  # Since: 1.3.0
  ##
  { 'command': 'nbd-server-add',
-  'data': 'BlockExportOptionsNbd', 'boxed': true }
+  'data': 'NbdServerAddOptions', 'boxed': true }
  
  ##

  # @NbdServerRemoveMode:
@@ -167,6 +179,8 @@
  # Describes a block export, i.e. how single node should be exported on an
  # external interface.
  #
+# @device: The device name or node name of the node to be exported
+#
  # @writethrough: If true, caches are flushed after every write request to the
  #export before completion is signalled. (since: 5.2;
  #default: false)
@@ -175,6 +189,7 @@
  ##
  { 'union': 'BlockExportOptions',
'base': { 'type': 'BlockExportType',
+'device': 'str',


for block export, I'd prefer that we mandate node name only, and naming 
it @node-name (rather than @device) feels nicer, for something that only 
new code will be using (that is, we should be encouraging the use of 
node names, especially now that libvirt has finally made that leap).


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




Re: [RFC PATCH 13/22] block/export: Move refcount from NBDExport to BlockExport

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

Having a refcount makes sense for all types of block exports. It is also
a prerequisite for keeping a list of all exports at the BlockExport
level.

Signed-off-by: Kevin Wolf 
---



+++ b/include/block/export.h
@@ -21,14 +21,24 @@ typedef struct BlockExport BlockExport;
  typedef struct BlockExportDriver {
  BlockExportType type;
  BlockExport *(*create)(BlockExportOptions *, Error **);
+void (*delete)(BlockExport *);
  } BlockExportDriver;
  
  struct BlockExport {

  const BlockExportDriver *drv;
+
+/*
+ * Reference count for this block export. This includes strong references
+ * both from the owner (qemu-nbd or the monitor) and clients connected to
+ * the export.


I guess 'the monitor' includes qemu-storage-daemon.


+ */
+int refcount;
  };
  
  extern const BlockExportDriver blk_exp_nbd;
  
  BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp);

+void blk_exp_ref(BlockExport *exp);
+void blk_exp_unref(BlockExport *exp);


Yay, I think this naming is more consistent with the rest of qemu...

  
  #endif

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 23030db3f1..af8509ab70 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -336,8 +336,6 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
  void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk);
  void nbd_export_close(NBDExport *exp);
  void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error 
**errp);
-void nbd_export_get(NBDExport *exp);
-void nbd_export_put(NBDExport *exp);


...as opposed to this which is common in kernel but less so in this 
project.  No hard feelings from me :)



+++ b/blockdev-nbd.c
@@ -232,7 +232,7 @@ BlockExport *nbd_export_create(BlockExportOptions 
*exp_args, Error **errp)
  /* The list of named exports has a strong reference to this export now and
   * our only way of accessing it is through nbd_export_find(), so we can 
drop
   * the strong reference that is @exp. */
-nbd_export_put(exp);
+blk_exp_unref((BlockExport*) exp);


Even a helper function that converts NBDBlockExport* to BlockExport* 
rather than a cast might be nicer, but then again, I see from Max's 
review that this may be a temporary state of things.


(The QAPI contains such type-safe container casts, such as 
qapi_DriveBackup_base(), if that helps...)




@@ -1537,7 +1536,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
  
  exp = g_new0(NBDExport, 1);

  exp->common = (BlockExport) {
-.drv = _exp_nbd,
+.drv= _exp_nbd,
+.refcount   = 1,


I'm not sure whether trying to align the '=' is good, because the moment 
you add a longer field name, every other line has to be touched.  I'm 
fine with just one space on both side of =, even if it is more ragged to 
read.  But you're the author, so you get to pick.




@@ -1626,8 +1625,9 @@ NBDExport *nbd_export_new(BlockDriverState *bs,
  exp->ctx = ctx;
  blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);
  
+blk_exp_ref(>common);

  QTAILQ_INSERT_TAIL(, exp, next);
-nbd_export_get(exp);
+


Is there any consequence to this changed ordering in grabbing the 
reference vs. updating the list?



Overall looks nice.

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




Re: [RFC PATCH 12/22] nbd/server: Simplify export shutdown

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

Closing export is somewhat convoluted because nbd_export_close() and
nbd_export_put() call each other and the ways they actually end up being
nested is not necessarily obvious.


You are in a maze of twisty little passages, all alike.

Yes, I've always hated that part of the code; thanks for tackling a cleanup.



However, it is not really necessary to call nbd_export_close() from
nbd_export_put() when putting the last reference because it only does
three things:

1. Close all clients. We're going to refcount 0 and all clients hold a
reference, so we know there is no active client any more.

2. Close the user reference (represented by exp->name being non-NULL).
The same argument applies: If the export were still named, we would
still have a reference.

3. Freeing exp->description. This is really cleanup work to be done when
the export is finally freed. There is no reason to already clear it
while clients are still in the process of shutting down.

So after moving the cleanup of exp->description, the code can be
simplified so that only nbd_export_close() calls nbd_export_put(), but
never the other way around.

Signed-off-by: Kevin Wolf 
---
  nbd/server.c | 17 -
  1 file changed, 4 insertions(+), 13 deletions(-)



Reviewed-by: Eric Blake 

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




Re: [RFC PATCH 11/22] qemu-nbd: Use blk_exp_add() to create the export

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

With this change, NBD exports are only created through the BlockExport
interface any more. This allows us finally to move things from the NBD


s/are only/are now only/; s/any more //


layer to the BlockExport layer if they make sense for other export
types, too.

blk_exp_add() returns only a weak reference, so the explicit
nbd_export_put() goes away.


Feel free to rename get/put to ref/unref in your series if that makes 
life any easier.




Signed-off-by: Kevin Wolf 
---



@@ -1050,9 +1050,27 @@ int main(int argc, char **argv)
  
  bs->detect_zeroes = detect_zeroes;
  
-export = nbd_export_new(bs, export_name,

-export_description, bitmap, readonly, shared > 1,
-writethrough, _fatal);
+nbd_server_is_qemu_nbd(true);


Feels a bit like a backdoor hack, but gets the job done (and as you 
said, we had quite an IRC conversation about what it would take to get 
socket activation working, so leaving that in qemu-nbd for now is 
reasonable for the first patch series).



+
+export_opts = g_new(BlockExportOptions, 1);
+*export_opts = (BlockExportOptions) {
+.type   = BLOCK_EXPORT_TYPE_NBD,
+.has_writethrough   = true,
+.writethrough   = writethrough,
+.u.nbd = {
+.device = g_strdup(bdrv_get_node_name(bs)),
+.has_name   = true,
+.name   = g_strdup(export_name),
+.has_description= !!export_description,
+.description= g_strdup(export_description),
+.has_writable   = true,
+.writable   = !readonly,
+.has_bitmap = !!bitmap,
+.bitmap = g_strdup(bitmap),
+},
+};
+blk_exp_add(export_opts, _fatal);
+qapi_free_BlockExportOptions(export_opts);


Looks sane.

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




Re: [RFC PATCH 10/22] nbd: Remove NBDExport.close callback

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

The export close callback is unused by the built-in NBD server. qemu-nbd
uses it only during shutdown to wait for the unrefed export to actually
go away. It can just use nbd_export_close_all() instead and do without
the callback.

This removes the close callback from nbd_export_new() and makes both
callers of it more similar.

Signed-off-by: Kevin Wolf 
---


Reviewed-by: Eric Blake 

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




Re: [RFC PATCH 09/22] nbd: Add writethrough to block-export-add

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

qemu-nbd allows use of writethrough cache modes, which mean that write
requests made through NBD will cause a flush before they complete.
Expose the same functionality in block-export-add.

Signed-off-by: Kevin Wolf 
---
  qapi/block-export.json | 7 ++-
  blockdev-nbd.c | 2 +-
  2 files changed, 7 insertions(+), 2 deletions(-)




+++ b/blockdev-nbd.c
@@ -218,7 +218,7 @@ BlockExport *nbd_export_create(BlockExportOptions 
*exp_args, Error **errp)
  
  exp = nbd_export_new(bs, arg->name, arg->description, arg->bitmap,

   !arg->writable, !arg->writable,
- NULL, false, errp);
+ NULL, exp_args->writethrough, errp);


It would someday be nice to get rid of exp_args->has_writethrough in 
QAPI generated code, but that's independent of this series.  Meanwhile, 
you are safe in relying on writethrough being false when 
has_writethrough is false.


This change makes sense to me interface-wise.  QAPI-wise, should we try 
harder to make writethrough settable only for writable exports (and an 
error for read-only exports)?  I'm not sure what QAPI contortions would 
have to look like to make that enforced by the QMP parser; but it's also 
not the end of the world if we just always permit the optional field and 
then apply a post-parse semantic check.


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




Re: [RFC PATCH 09/22] nbd: Add writethrough to block-export-add

2020-08-19 Thread Eric Blake

On 8/17/20 7:56 AM, Max Reitz wrote:

On 13.08.20 18:29, Kevin Wolf wrote:

qemu-nbd allows use of writethrough cache modes, which mean that write
requests made through NBD will cause a flush before they complete.
Expose the same functionality in block-export-add.

Signed-off-by: Kevin Wolf 
---



+++ b/qapi/block-export.json
@@ -167,10 +167,15 @@
  # Describes a block export, i.e. how single node should be exported on an
  # external interface.
  #
+# @writethrough: If true, caches are flushed after every write request to the
+#export before completion is signalled. (since: 5.2;
+#default: false)
+#
  # Since: 4.2
  ##
  { 'union': 'BlockExportOptions',
-  'base': { 'type': 'BlockExportType' },
+  'base': { 'type': 'BlockExportType',
+'*writethrough': 'bool' },
'discriminator': 'type',
'data': {
'nbd': 'BlockExportOptionsNbd'


Hm.  I find it weird to have @writethrough in the base but @device in
the specialized class.

I think everything that will be common to all block exports should be in
the base, and that probably includes a node-name.  I’m aware that will
make things more tedious in the code, but perhaps it would be a nicer
interface in the end.  Or is the real problem that that would create
problems in the storage daemon’s command line interface, because then
the specialized (legacy) NBD interface would no longer be compatible
with the new generalized block export interface?

Anyway, @writable might be a similar story.  A @read-only may make sense
in general, I think.


And maybe even auto-read-only.  As for @writable vs. @read-only, that's 
a choice of spelling, but we don't want both; there's also the question 
of which default is saner (having to opt-in to allowing writes is more 
verbose than defaulting to allowing writes when possible, but arguably 
saner as it is less risk of unintended writes when you forgot to specify 
the option; @auto-read-only can help in choosing nicer defaults).




Basically, I think that the export code should be separate from the code
setting up the BlockBackend that should be exported, so all options
regarding that BB should be common; and those options are @node-name,
@writethrough, and @read-only.  (And perhaps other things like
@resizable, too, even though that isn’t something to consider for NBD.)


NBD isn't resizable yet, but extending the protocol to let it become so 
is on my TODO list.


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




Re: [RFC PATCH 08/22] nbd: Add max-connections to nbd-server-start

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

This is a QMP equivalent of qemu-nbd's --share option, limiting the
maximum number of clients that can attach at the same time.

Signed-off-by: Kevin Wolf 
---
  qapi/block-export.json | 10 --
  include/block/nbd.h|  3 ++-
  block/monitor/block-hmp-cmds.c |  2 +-
  blockdev-nbd.c | 33 ++---
  qemu-storage-daemon.c  |  4 ++--
  5 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index 40369814b4..1fdc55c53a 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -14,6 +14,8 @@
  # is only resolved at time of use, so can be deleted and
  # recreated on the fly while the NBD server is active.
  # If missing, it will default to denying access (since 4.0).
+# @max-connections: The maximum number of connections to allow at the same
+#   time, 0 for unlimited. (since 5.2; default: 0)


Nice way to add feature parity.

Limiting the number of connections (particularly for a writable client, 
where we cannot guarantee cache consistency between the connections), 
seems like a worthwhile feature to have; I've always found it odd that 
qemu-nbd and QMP nbd-server-add defaulted to different limits (1 vs. 
unlimited).  For reference, nbdkit defaults to unlimited, and I'm happy 
if qemu-storage-daemon does likewise; but changing qemu-nbd's default of 
1 would be backwards incompatible and may cause surprises (there's 
always 'qemu-nbd -e' when needed).  I also wonder if we should change 
'qemu-nbd -e 0' to mean unlimited rather than an error (right now, 
qemu-iotests/common.rc uses -e 42 for all nbd-based tests for a saner 
limit than just 1, but it smells of being arbitrary compared to unlimited).


Reviewed-by: Eric Blake 

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




Re: [RFC PATCH 07/22] block/export: Remove magic from block-export-add

2020-08-19 Thread Eric Blake

cc: Peter Krempa

On 8/13/20 11:29 AM, Kevin Wolf wrote:

nbd-server-add tries to be convenient and adds two questionable
features that we don't want to share in block-export-add, even for NBD
exports:

1. When requesting a writable export of a read-only device, the export
is silently downgraded to read-only. This should be an error in the
context of block-export-add.


I'd be happy for this to be an error even with nbd-export-add; I don't 
think it would harm any of libvirt's existing usage (either for storage 
migration, or for incremental backups).


Side note: In the past, I had a proposal to enhance the NBD Protocol to 
allow a client to advertise to the server its intent on being a 
read-only or read-write client.  Not relevant to this patch, but this 
part of the commit message reminds me that I should revisit that topic 
(Rich and I recently hit another case in nbdkit where such an extension 
would be nice, when it comes to using NBD's multi-conn for better 
performance on a read-only connection, but only if the server knows the 
client intends to be read-only)




2. When using a BlockBackend name, unplugging the device from the guest
will automatically stop the NBD server, too. This may sometimes be
what you want, but it could also be very surprising. Let's keep
things explicit with block-export-add. If the user wants to stop the
export, they should tell us so.


Here, keeping the nbd command different from the block-export command 
seems tolerable.  On the other hand, I wonder if Peter needs to change 
anything in libvirt's incremental backup code to handle this sudden 
disappearance of an NBD device during a disk hot-unplug (that is, either 
the presence of an ongoing pull-mode backup should block disk unplug, or 
libvirt needs a way to guarantee that an ongoing backup NBD device 
remains in spite of subsequent disk actions on the guest).  Depending on 
libvirt's needs, we may want to revisit the nbd command to have the same 
policy as block-export-add, plus an introspectible feature notation.




Move these things into the nbd-server-add QMP command handler so that
they apply only there.

Signed-off-by: Kevin Wolf 
---
  include/block/nbd.h   |  3 ++-



+void qmp_block_export_add(BlockExportOptions *export, Error **errp)
+{
+blk_exp_add(export, errp);
  }
  
  void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)

  {
-BlockExportOptions export = {
+BlockExport *export;
+BlockDriverState *bs;
+BlockBackend *on_eject_blk;
+
+BlockExportOptions export_opts = {
  .type = BLOCK_EXPORT_TYPE_NBD,
  .u.nbd = *arg,
  };
-qmp_block_export_add(, errp);
+
+/*
+ * nbd-server-add doesn't complain when a read-only device should be
+ * exported as writable, but simply downgrades it. This is an error with
+ * block-export-add.


I'd be happy with either marking this deprecated now (and fixing it in 
two releases), or declaring it a bug in nbd-server-add now (and fixing 
it outright).



+ */
+bs = bdrv_lookup_bs(arg->device, arg->device, NULL);
+if (bs && bdrv_is_read_only(bs)) {
+arg->writable = false;
+}
+
+export = blk_exp_add(_opts, errp);
+if (!export) {
+return;
+}
+
+/*
+ * nbd-server-add removes the export when the named BlockBackend used for
+ * @device goes away.
+ */
+on_eject_blk = blk_by_name(arg->device);
+if (on_eject_blk) {
+nbd_export_set_on_eject_blk(export, on_eject_blk);
+}


Wait - is the magic export removal tied only to exporting a drive name, 
and not a node name?  So as long as libvirt is using only node names 
whwen adding exports, a drive being unplugged won't interfere?


Overall, the change makes sense to me, although I'd love to see if we 
could go further on the writable vs. read-only issue.


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




Re: [RFC PATCH 06/22] qemu-nbd: Use raw block driver for --offset

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

Instead of implementing qemu-nbd --offset in the NBD code, just put a
raw block node with the requested offset on top of the user image and
rely on that doing the job.

This does not only simplify the nbd_export_new() interface and bring it
closer to the set of options that the nbd-server-add QMP command offers,
but in fact it also eliminates a potential source for bugs in the NBD
code which previously had to add the offset manually in all relevant
places.


Yay!  This patch alone is worth having, regardless of the fate of the 
rest of the series: no change in end-user functionality, but by making 
qemu-nbd turn it into proper syntactic sugar, we've reduced the 
maintenance burden of duplicated code.




Signed-off-by: Kevin Wolf 
---
  include/block/nbd.h |  4 ++--
  blockdev-nbd.c  |  9 +
  nbd/server.c| 34 +-
  qemu-nbd.c  | 27 ---
  4 files changed, 32 insertions(+), 42 deletions(-)




+++ b/nbd/server.c
@@ -89,7 +89,6 @@ struct NBDExport {
  BlockBackend *blk;
  char *name;
  char *description;
-uint64_t dev_offset;
  uint64_t size;


I'm trying to figure out if we can also drop 'size' here.  If we do, the 
consequence would be that an NBD client could ask for beyond-EOF I/O, 
and presumably the block layer would reject that gracefully (although 
not necessarily with the same errno as NBD currently reports).  I'm fine 
leaving it alone in this patch, though.



@@ -1569,7 +1574,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t 
dev_offset,
  exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |
NBD_FLAG_SEND_FAST_ZERO);
  }
-assert(size <= INT64_MAX - dev_offset);
+assert(size <= INT64_MAX);


As Max caught, this is now dead code.


@@ -2386,8 +2388,7 @@ static coroutine_fn int nbd_handle_request(NBDClient 
*client,
  if (request->flags & NBD_CMD_FLAG_FAST_ZERO) {
  flags |= BDRV_REQ_NO_FALLBACK;
  }
-ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
-request->len, flags);
+ret = blk_pwrite_zeroes(exp->blk, request->from, request->len, flags);
  return nbd_send_generic_reply(client, request->handle, ret,
"writing to file failed", errp);
  
@@ -2401,8 +2402,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,

"flush failed", errp);
  
  case NBD_CMD_TRIM:

-ret = blk_co_pdiscard(exp->blk, request->from + exp->dev_offset,
-  request->len);
+ret = blk_co_pdiscard(exp->blk, request->from, request->len);


Merge conflicts with 890cbccb08; should be obvious enough to resolve, 
though.



+++ b/qemu-nbd.c
@@ -523,7 +523,6 @@ int main(int argc, char **argv)
  const char *port = NULL;
  char *sockpath = NULL;
  char *device = NULL;
-int64_t fd_size;
  QemuOpts *sn_opts = NULL;
  const char *sn_id_or_name = NULL;
  const char *sopt = "hVb:o:p:rsnc:dvk:e:f:tl:x:T:D:B:L";
@@ -1028,6 +1027,17 @@ int main(int argc, char **argv)
  }
  bs = blk_bs(blk);
  
+if (dev_offset) {

+QDict *raw_opts = qdict_new();
+qdict_put_str(raw_opts, "driver", "raw");
+qdict_put_str(raw_opts, "file", bs->node_name);
+qdict_put_int(raw_opts, "offset", dev_offset);


Huh.  When 0bc16997f5 got rid of the --partition option, it also got rid 
of the only way that the NBD driver could clamp down requests to a range 
smaller than the end of the file.  Now that you are adding a raw driver 
in the mix, that ability to clamp the end of the range (aka a --size 
option, in addition to an --offset option) may be worth reinstating. 
But that can be done as a separate patch, if at all (and whether 
qemu-nbd should do it, or qemu-storage-daemon, or whether we just point 
people at 'nbdkit --filter=partition', is part of that discussion).  But 
for this patch, it looks like you are making a straight-across conversion.



+bs = bdrv_open(NULL, NULL, raw_opts, flags, _fatal);
+    blk_remove_bs(blk);
+blk_insert_bs(blk, bs, _fatal);
+bdrv_unref(bs);
+}
+


Slick.

Reviewed-by: Eric Blake 

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




Re: [RFC PATCH 05/22] qemu-storage-daemon: Use qmp_block_export_add()

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

No reason to duplicate the functionality locally, we can now just reuse
the QMP command block-export-add for --export.

Signed-off-by: Kevin Wolf 
---
  qemu-storage-daemon.c | 13 +
  1 file changed, 1 insertion(+), 12 deletions(-)



Reviewed-by: Eric Blake 

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




Re: [RFC PATCH 04/22] block/export: Add BlockExport infrastructure and block-export-add

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

We want to have a common set of commands for all types of block exports.
Currently, this is only NBD, but we're going to add more types.

This patch adds the basic BlockExport and BlockExportDriver structs and
a QMP command block-export-add that creates a new export based on the
given BlockExportOptions.

qmp_nbd_server_add() becomes a wrapper around qmp_block_export_add().

Signed-off-by: Kevin Wolf 
---


Seeing if I can spot anything beyond Max's fine points:


+++ b/qapi/block-export.json
@@ -170,3 +170,12 @@
'nbd': 'BlockExportOptionsNbd'
 } }
  
+##

+# @block-export-add:
+#
+# Creates a new block export.
+#
+# Since: 5.2
+##
+{ 'command': 'block-export-add',
+  'data': 'BlockExportOptions', 'boxed': true }


So if I read patch 3 correctly, the difference between nbd-server-add 
and block-export-add is that the latter includes a "type":"nbd" member. 
(If we ever play with allowing a default discriminator value in QAPI, we 
could declare the default for "type" to be NBD, and then the two would 
be identical - except that since you are adding a new command designed 
to extend to more than just nbd, I don't think keeping nbd as default 
makes sense)



+++ b/block/export/export.c



+void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
+{
+BlockExportOptions export = {
+.type = BLOCK_EXPORT_TYPE_NBD,
+.u.nbd = *arg,
+};
+qmp_block_export_add(, errp);
+}


And indeed, this matches that analysis.



@@ -217,6 +220,8 @@ void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error 
**errp)
  
   out:

  aio_context_release(aio_context);
+/* TODO Remove the cast: Move to server.c which can access fields of exp */
+return (BlockExport*) exp;


Should this use container_of()?  Ah, the TODO says you want to, but 
can't because exp is an opaque type in this file...



  }
  
  void qmp_nbd_server_remove(const char *name,

diff --git a/nbd/server.c b/nbd/server.c
index bee2ef8bd1..774325dbe5 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -18,6 +18,8 @@
   */
  
  #include "qemu/osdep.h"

+
+#include "block/export.h"
  #include "qapi/error.h"
  #include "qemu/queue.h"
  #include "trace.h"
@@ -80,6 +82,7 @@ struct NBDRequestData {
  };
  
  struct NBDExport {

+BlockExport common;


...but at least the cast is accurate.

Overall, the idea looks sane.  I'm happy if you want to move 
blockdev-nbd.c out of the top level.


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




Re: [RFC PATCH 03/22] qapi: Rename BlockExport to BlockExportOptions

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

The name BlockExport will be used for the struct containing the runtime
state of block exports, so change the name of export creation options.

Signed-off-by: Kevin Wolf 
---
  qapi/block-export.json | 12 ++--
  block/monitor/block-hmp-cmds.c |  6 +++---
  blockdev-nbd.c |  2 +-
  qemu-storage-daemon.c  |  8 
  4 files changed, 14 insertions(+), 14 deletions(-)


Mechanical, and doesn't impact introspection.

Reviewed-by: Eric Blake 

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




Re: [RFC PATCH 02/22] qapi: Create block-export module

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

Move all block export related types and commands from block-core to the
new QAPI module block-export.

Signed-off-by: Kevin Wolf 
---



+++ b/qapi/block-export.json
@@ -0,0 +1,172 @@
+##
+# == Block device exports
+##
+
+{ 'include': 'sockets.json' }
+



+++ b/qapi/Makefile.objs
@@ -5,7 +5,8 @@ util-obj-y += opts-visitor.o qapi-clone-visitor.o
  util-obj-y += qmp-event.o
  util-obj-y += qapi-util.o
  
-QAPI_COMMON_MODULES = audio authz block-core block char common control crypto

+QAPI_COMMON_MODULES = audio authz block-core block-export block char common
+QAPI_COMMON_MODULES += control crypto


I don't know what Paolo's Meson conversion will affect here, but even if 
it changes how this hunk looks, the intent is obvious.


Looks like a straightforward refactoring.  I agree with Max's comment on 
the missing modeline, but that doesn't stop me from:


Reviewed-by: Eric Blake 

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




Re: [RFC PATCH 01/22] nbd: Remove unused nbd_export_get_blockdev()

2020-08-19 Thread Eric Blake

On 8/13/20 11:29 AM, Kevin Wolf wrote:

Signed-off-by: Kevin Wolf 
---
  include/block/nbd.h | 2 --
  nbd/server.c| 5 -
  2 files changed, 7 deletions(-)


Reviewed-by: Eric Blake 

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




Re: [PATCH v3] block/nbd: use non-blocking connect: fix vm hang on connect()

2020-08-19 Thread Eric Blake

On 8/12/20 9:52 AM, Vladimir Sementsov-Ogievskiy wrote:

This make nbd connection_co to yield during reconnects, so that
reconnect doesn't hang up the main thread. This is very important in
case of unavailable nbd server host: connect() call may take a long
time, blocking the main thread (and due to reconnect, it will hang
again and again with small gaps of working time during pauses between
connection attempts).




How to reproduce the bug, fixed with this commit:

1. Create an image on node1:
qemu-img create -f qcow2 xx 100M

2. Start NBD server on node1:
qemu-nbd xx

3. Start vm with second nbd disk on node2, like this:

   ./x86_64-softmmu/qemu-system-x86_64 -nodefaults -drive \
 file=/work/images/cent7.qcow2 -drive file=nbd+tcp://192.168.100.2 \
 -vnc :0 -qmp stdio -m 2G -enable-kvm -vga std


Where is the configuration to set up retry on the nbd connection?  I 
wonder if you have a non-upstream patch that turns it on by default in 
your builds; for upstream, I would have expected something more along 
the lines of -blockdev 
driver=nbd,reconnect-delay=20,server.type=inet,server.data.hostname=192.168.100.2,server.data.port=10809 
(typing off the top of my head, rather than actually tested).




4. Access the vm through vnc (or some other way?), and check that NBD
drive works:

dd if=/dev/sdb of=/dev/null bs=1M count=10

- the command should succeed.

5. Now, let's trigger nbd-reconnect loop in Qemu process. For this:

5.1 Kill NBD server on node1

5.2 run "dd if=/dev/sdb of=/dev/null bs=1M count=10" in the guest
 again. The command should fail and a lot of error messages about
 failing disk may appear as well.


Why does the guest access fail when the server goes away?  Shouldn't the 
pending guest requests merely be queued for retry (where the guest has 
not seen a failure yet, but may do so if timeouts are reached), rather 
than being instant errors?




 Now NBD client driver in Qemu tries to reconnect.
 Still, VM works well.

6. Make node1 unavailable on NBD port, so connect() from node2 will
last for a long time:

On node1 (Note, that 10809 is just a default NBD port):

sudo iptables -A INPUT -p tcp --dport 10809 -j DROP

After some time the guest hangs, and you may check in gdb that Qemu
hangs in connect() call, issued from the main thread. This is the
BUG.

7. Don't forget to drop iptables rule from your node1:

sudo iptables -D INPUT -p tcp --dport 10809 -j DROP



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




Re: [PATCH v3] block/nbd: use non-blocking connect: fix vm hang on connect()

2020-08-19 Thread Eric Blake
+ * bs.
+ */
+s->wait_connect = true;
+qemu_coroutine_yield();
+
+qemu_mutex_lock(>mutex);
+
+switch (thr->state) {
+case CONNECT_THREAD_SUCCESS:
+case CONNECT_THREAD_FAIL:
+thr->state = CONNECT_THREAD_NONE;
+error_propagate(errp, thr->err);
+thr->err = NULL;
+res = thr->sioc;
+thr->sioc = NULL;
+break;
+case CONNECT_THREAD_RUNNING:
+case CONNECT_THREAD_RUNNING_DETACHED:
+/*
+ * Obviously, drained section wants to start. Report the attempt as
+ * failed. Still connect thread is executing in background, and its
+ * result may be used for next connection attempt.
+ */
+res = NULL;
+error_setg(errp, "Connection attempt cancelled by other operation");
+break;
+
+case CONNECT_THREAD_NONE:
+/*
+ * Impossible. We've seen this thread running. So it should be
+ * running or at least give some results.
+ */
+abort();
+
+default:
+abort();
+}
+
+qemu_mutex_unlock(>mutex);
+
+return res;
+}


Looks sensible.


+
+/*
+ * nbd_co_establish_connection_cancel
+ * Cancel nbd_co_establish_connection asynchronously: it will finish soon, to
+ * allow drained section to begin.
+ *
+ * If detach is true, also cleanup the state (or if thread is running, move it
+ * to CONNECT_THREAD_RUNNING_DETACHED state). s->connect_thread becomes NULL if
+ * detach is true.
+ */
+static void nbd_co_establish_connection_cancel(BlockDriverState *bs,
+   bool detach)
+{
+BDRVNBDState *s = (BDRVNBDState *)bs->opaque;


Is the cast necessary here?


+NBDConnectThread *thr = s->connect_thread;
+bool wake = false;
+bool do_free = false;
+
+qemu_mutex_lock(>mutex);
+
+if (thr->state == CONNECT_THREAD_RUNNING) {
+/* We can cancel only in running state, when bh is not yet scheduled */
+thr->bh_ctx = NULL;
+if (s->wait_connect) {
+s->wait_connect = false;
+wake = true;
+}
+if (detach) {
+thr->state = CONNECT_THREAD_RUNNING_DETACHED;
+s->connect_thread = NULL;
+}
+} else if (detach) {
+do_free = true;
+}
+
+qemu_mutex_unlock(>mutex);
+
+if (do_free) {
+nbd_free_connect_thread(thr);
+s->connect_thread = NULL;
+}
+
+if (wake) {
+aio_co_wake(s->connection_co);
+}
+}
+
  static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
  {
  int ret;
@@ -289,7 +551,7 @@ static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState 
*s)
  s->ioc = NULL;
  }
  
-sioc = nbd_establish_connection(s->saddr, _err);

+sioc = nbd_co_establish_connection(s->bs, _err);
  if (!sioc) {
  ret = -ECONNREFUSED;
  goto out;
@@ -1946,6 +2208,8 @@ static int nbd_open(BlockDriverState *bs, QDict *options, 
int flags,
  /* successfully connected */
  s->state = NBD_CLIENT_CONNECTED;
  
+nbd_init_connect_thread(s);

+
  s->connection_co = qemu_coroutine_create(nbd_connection_entry, s);
  bdrv_inc_in_flight(bs);
  aio_co_schedule(bdrv_get_aio_context(bs), s->connection_co);



Overall this looks good to me.  I still want to run some tests 
(including playing with your reproducer formula), but code-wise, I can 
offer:


Reviewed-by: Eric Blake 

I pointed out a number of grammar and format touchups; I don't mind 
applying those locally, if there is no other reason to send a v4.


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




Re: Choice of BDRV_REQUEST_MAX_SECTORS

2020-08-17 Thread Eric Blake

On 8/17/20 7:32 AM, Peter Lieven wrote:

Hi,


I am currently debugging a performance issue in qemu-img convert. I 
think I have found the cause and will send a patch later.


But is there any reason why BDRV_REQUEST_MAX_SECTORS is not at least 
aligned down to 8 (4k sectors)?


Any operation that is not able to determinate an optimal or maximum 
request size from a driver will likely use this value and


might generate unaligned requests (as qemu-img convert does) as soon as 
the first iteration of an operation is done.


Vladimir's work to make the block layer 64-bit clean should help resolve 
this, as we will be relying more heavily on values aligned to the 
driver's limits rather than arbitrary 32-bit cutoffs.


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




Re: [PATCH for-5.2 v3 1/3] migration: Add block-bitmap-mapping parameter

2020-08-12 Thread Eric Blake

On 7/22/20 3:05 AM, Max Reitz wrote:

This migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.

This way, management tools can use different node and bitmap names on
the source and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Max Reitz 
---
  qapi/migration.json| 104 -
  migration/migration.h  |   3 +
  migration/block-dirty-bitmap.c | 373 -
  migration/migration.c  |  30 +++
  monitor/hmp-cmds.c |  30 +++
  5 files changed, 485 insertions(+), 55 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index d5000558c6..d7e953cd73 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -507,6 +507,44 @@
'data': [ 'none', 'zlib',
  { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
  
+##

+# @BitmapMigrationBitmapAlias:
+#
+# @name: The name of the bitmap.
+#
+# @alias: An alias name for migration (for example the bitmap name on
+# the opposite site).
+#
+# Since: 5.1


5.2, now, but I can touch that up if it is the only problem raised.


+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+#  aliases for the purpose of dirty bitmap migration.  Such
+#  aliases may for example be the corresponding names on the
+#  opposite site.
+#  The mapping must be one-to-one, and on the destination also
+#  complete: On the source, bitmaps on nodes where either the
+#  bitmap or the node is not mapped will be ignored.  In
+#  contrast, on the destination, receiving a bitmap (by alias)
+#  from a node (by alias) when either alias is not mapped will
+#  result in an error.


Grammar is a bit odd and it feels repetitive.  How about:

The mapping must not provide more than one alias for a bitmap, nor reuse 
the same alias across multiple bitmaps in the same node. On the source, 
an omitted node or bitmap within a node will ignore those bitmaps. In 
contrast, on the destination, receiving a bitmap (by alias) from a node 
(by alias) when either alias is not mapped will result in an error.



+#  Note that the destination does not know about bitmaps it
+#  does not receive, so there is no limitation or requirement
+#  regarding the number of bitmaps received, or how they are
+#  named, or on which nodes they are placed.
+#  By default (when this parameter has never been set), bitmap
+#  names are mapped to themselves.  Nodes are mapped to their
+#  block device name if there is one, and to their node name
+#  otherwise. (Since 5.2)


Looks good.



@@ -781,6 +839,25 @@
  #  will consume more CPU.
  #  Defaults to 1. (Since 5.0)
  #
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+#  aliases for the purpose of dirty bitmap migration.  Such
+#  aliases may for example be the corresponding names on the
+#  opposite site.


Ah, the joys of duplicated text.


+++ b/migration/block-dirty-bitmap.c



@@ -128,7 +131,8 @@ typedef struct DirtyBitmapMigState {
  
  typedef struct DirtyBitmapLoadState {

  uint32_t flags;
-char node_name[256];
+char node_alias[256];
+char bitmap_alias[256];


Do we properly check that alias names will never overflow?


+static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
+   bool name_to_alias,
+   Error **errp)
+{
+GHashTable *alias_map;
+
+alias_map = g_hash_table_new_full(g_str_hash, g_str_equal,
+  g_free, free_alias_map_inner_node);
+
+for (; bbm; bbm = bbm->next) {
+const BitmapMigrationNodeAlias *bmna = bbm->value;
+const BitmapMigrationBitmapAliasList *bmbal;
+AliasMapInnerNode *amin;
+GHashTable *bitmaps_map;
+const char *node_map_from, *node_map_to;
+
+if (!id_wellformed(bmna->alias)) {
+error_setg(errp, "The node alias %s is not well-formed",
+   bmna->alias);
+goto fail;
+}


Maybe id_wellformed should enforce lengths?  Otherwise, I'm not seeing a 
length limit applied during the mapping process.


Modulo that, I think we're ready to go.

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




  1   2   3   4   5   6   7   8   9   10   >