Re: [Qemu-block] [PATCH] scsi: Address spurious clang warning

[Peter Maydell]

On Tue, 27 Nov 2018 at 18:49, John Snow  wrote:
>
> Some versions of Clang prior to 6.0 (and some builds of clang after,
> such as 6.0.1-2.fc28) fail to recognize { 0 } as a valid initializer
> for a struct with subobjects when -Wmissing-braces is enabled.
>
> https://bugs.llvm.org/show_bug.cgi?id=21689 and
> https://reviews.llvm.org/rL314499 suggests this should be fixed in 6.0,
> but it might not be the case for older versions or downstream versions.
>
> For now, follow the precedent of ebf2a499 and replace the standard { 0 }
> with the accepted { } to silence this warning and allow the build to
> work under clang 6.0.1-2.fc28, and builds prior to 6.0.
>
> Signed-off-by: John Snow 

Applied to master as a build fix for rc3, thanks.

-- PMM

Re: [Qemu-block] [PATCH] qemu-img info lists bitmap directory entries

[Eric Blake]

On 11/27/18 6:41 AM, Andrey Shinkevich wrote:

The 'Format specific information' of qemu-img info command will show
the name, flags and granularity for every QCOW2 bitmap.

Signed-off-by: Andrey Shinkevich 
---
Dear colleagues,

With this patch, qemu-img info will display a name, flags and granularity
information for every bitmap in the directory section of a QCOW2 image.
That information appears in the 'Format specific information' section as
it's shown in the following example:

image: /vz/vmprivate/VM1/harddisk.hdd
file format: qcow2
virtual size: 64G (68719476736 bytes)
disk size: 3.0M
cluster_size: 1048576
Format specific information:
 compat: 1.1
 lazy refcounts: true
 refcount bits: 16
 dirty bitmaps:
 [0]:
 flags:
 [0]: in-use
 [1]: auto
 name: {257b5ea5-38c5-410e-a457-71c76f763c60}
 granularity: 65536
 [1]:
 flags:
 [0]: in-use
 [1]: auto
 name: back-up
 granularity: 65536
 corrupt: false


This example is worth including in the commit message proper (by putting 
it above the Signed-off-by and --- lines).


In addition to Vladimir's review (he's correct about using 4.0),


+++ b/block/qcow2-bitmap.c
@@ -1008,6 +1008,56 @@ fail:
  return false;
  }
  
+static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)

+{
+Qcow2BitmapInfoFlagsList *list = NULL;
+Qcow2BitmapInfoFlagsList **plist = 
+
+if (flags & BME_FLAG_IN_USE) {
+Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
+entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE;
+*plist = entry;
+plist = >next;
+}
+if (flags & BME_FLAG_AUTO) {
+Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
+entry->value = QCOW2_BITMAP_INFO_FLAGS_AUTO;
+*plist = entry;
+}
+return list;
+}


This implementation means that qemu won't report bits that are set but 
which qemu doesn't recognize.  Is that a problem?  I wouldn't 
over-engineer it unless we have a use case, but maybe one way would be 
to have an optional integer field alongside the array of known flags, 
where the integer is present only if there are unrecognized flags 
present.  But by the time we encounter an image with more than the two 
flags we currently recognize, we may have also patched this code to 
recognize a new flag.



@@ -4279,6 +4287,8 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
QCOW2_INCOMPAT_CORRUPT,
  .has_corrupt= true,
  .refcount_bits  = s->refcount_bits,
+.has_bitmaps= bitmaps ? true : false,


This looks awkward; I'd write it ' = !!bitmaps'.



+++ b/qapi/block-core.json
@@ -77,7 +77,8 @@
'*lazy-refcounts': 'bool',
'*corrupt': 'bool',
'refcount-bits': 'int',
-  '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+  '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+  '*bitmaps': ['Qcow2BitmapInfo']


Missing documentation on the new member, including a '(since 4.0)' tag.

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

Re: [Qemu-block] [Qemu-devel] [PATCH] scsi: Address spurious clang warning

[John Snow]

On 11/27/18 2:02 PM, Eric Blake wrote:
> On 11/27/18 12:49 PM, John Snow wrote:
>> Some versions of Clang prior to 6.0 (and some builds of clang after,
>> such as 6.0.1-2.fc28) fail to recognize { 0 } as a valid initializer
>> for a struct with subobjects when -Wmissing-braces is enabled.
>>
>> https://bugs.llvm.org/show_bug.cgi?id=21689 and
>> https://reviews.llvm.org/rL314499 suggests this should be fixed in 6.0,
>> but it might not be the case for older versions or downstream versions.
>>
>> For now, follow the precedent of ebf2a499 and replace the standard { 0 }
>> with the accepted { } to silence this warning and allow the build to
>> work under clang 6.0.1-2.fc28, and builds prior to 6.0.
>>
>> Signed-off-by: John Snow 
>>
> 
> Reviewed-by: Eric Blake 
> 
> I'm okay if this goes into -rc3 as a build-fix; I'm also okay if it
> slips to 4.0.
> 
>> ---
>>
>> What I am actually less clear on is why this appears to be a problem
>> only now; since the introduction of { 0 } was in 2.11. It might be
>> a regression only in the fedora distribution of Clang 6.0.
> 
> Or even a redefinition of struct dm_ioctl in some header where you are
> just now picking up a new struct layout that tickles the Clang issue in
> relation to the previous layout (since it is possible to have two
> structs that are ABI-compatible but where only one of the two has a
> nested substruct).
> 
>> +++ b/scsi/qemu-pr-helper.c
>> @@ -236,7 +236,7 @@ static void dm_init(void)
>>   perror("Cannot open " CONTROL_PATH);
>>   exit(1);
>>   }
>> -    struct dm_ioctl dm = { 0 };
>> +    struct dm_ioctl dm = { };
> 
> Random thought: would it be worth having "qemu/compiler.h" define a macro:
> 
> #if ...broken clang
> #define ZERO_INIT {}
> #else
> #define ZERO_INIT {0}
> #endif
> 
> and then rewrite all our '= { 0? }' initializers into '= ZERO_INIT'?  Or
> is that aesthetically too ugly?
> 

Obscures perfectly legitimate C code without solving anything, IMO. As
much code as can reflect "naked" C89/C99/GNU99, the better.

--js

Re: [Qemu-block] [Qemu-devel] [PATCH] scsi: Address spurious clang warning

[Peter Maydell]

On Tue, 27 Nov 2018 at 19:02, Eric Blake  wrote:
>
> On 11/27/18 12:49 PM, John Snow wrote:
> > Some versions of Clang prior to 6.0 (and some builds of clang after,
> > such as 6.0.1-2.fc28) fail to recognize { 0 } as a valid initializer
> > for a struct with subobjects when -Wmissing-braces is enabled.
> >
> > https://bugs.llvm.org/show_bug.cgi?id=21689 and
> > https://reviews.llvm.org/rL314499 suggests this should be fixed in 6.0,
> > but it might not be the case for older versions or downstream versions.
> >
> > For now, follow the precedent of ebf2a499 and replace the standard { 0 }
> > with the accepted { } to silence this warning and allow the build to
> > work under clang 6.0.1-2.fc28, and builds prior to 6.0.
> >
> > Signed-off-by: John Snow 
> >
>
> Reviewed-by: Eric Blake 
>
> I'm okay if this goes into -rc3 as a build-fix; I'm also okay if it
> slips to 4.0.
>
> > ---
> >
> > What I am actually less clear on is why this appears to be a problem
> > only now; since the introduction of { 0 } was in 2.11. It might be
> > a regression only in the fedora distribution of Clang 6.0.

Upstream clang 6.0 should have this bug fixed (ie it does
not warn about this construct), so it is odd that Fedora's does not.

> Or even a redefinition of struct dm_ioctl in some header where you are
> just now picking up a new struct layout that tickles the Clang issue in
> relation to the previous layout (since it is possible to have two
> structs that are ABI-compatible but where only one of the two has a
> nested substruct).
>
> > +++ b/scsi/qemu-pr-helper.c
> > @@ -236,7 +236,7 @@ static void dm_init(void)
> >   perror("Cannot open " CONTROL_PATH);
> >   exit(1);
> >   }
> > -struct dm_ioctl dm = { 0 };
> > +struct dm_ioctl dm = { };
>
> Random thought: would it be worth having "qemu/compiler.h" define a macro:
>
> #if ...broken clang
> #define ZERO_INIT {}
> #else
> #define ZERO_INIT {0}
> #endif
>
> and then rewrite all our '= { 0? }' initializers into '= ZERO_INIT'?  Or
> is that aesthetically too ugly?

I think that's worse than just writing {} everywhere (which
does work on every compiler we need to support, including
older gcc which also used to warn here).

thanks
-- PMM

Re: [Qemu-block] [RFC PATCH] Introduce Python module structure

[Caio Carrara]

On Tue, Nov 27, 2018 at 03:02:03PM -0500, Cleber Rosa wrote:
> 
> 
> On 11/27/18 2:00 PM, Caio Carrara wrote:
> > Hi, Cleber.
> > 
> > On Tue, Nov 27, 2018 at 05:50:34AM -0500, Cleber Rosa wrote:
> >> This is a simple move of Python code that wraps common QEMU
> >> functionality, and are used by a number of different tests
> >> and scripts.
> >>
> >> By treating that code as a real Python module, we can more easily,
> >> among other things:
> >>  * reuse more code
> >>  * apply a more consistent style
> >>  * add tests to that code
> >>  * generate documentation
> >>
> >> Signed-off-by: Cleber Rosa 
> >> ---
> >>  configure  |  1 +
> >>  scripts/qemu.py => python/qemu/__init__.py | 11 ++-
> > 
> > Well, we all know how difficult is to pick up names, but I would avoid
> > use `python` here. This is the name of python bin itself. Is there any
> > chance of a clash? I do not have a specific case right now, I'm just
> > wondering if it can happen we should avoid.
> > 
> 
> The point of the "python" directory, is that it's not supposed to be the
> *module* name, but a holder for the Python module, which is "qemu".
> Given that "python is not a module", has no __init__.py, I don't think
> it'll ever clash.
> 
> The reason for picking the name is that it makes it obvious, within the
> QEMU tree, that this is Python stuff.  I already have some tests, to
> test the "Python stuff", to put it under "python", probably in
> "python/tests".

Ok, now I got your point. No problem.

> 
> Anyway, it'd be nice to think of alternatives here.
> 
> >>  {scripts/qmp => python/qemu}/qmp.py|  0
> >>  {scripts => python/qemu}/qtest.py  |  5 +++--
> >>  scripts/device-crash-test  |  5 +
> >>  scripts/qmp/__init__.py|  0
> >>  tests/acceptance/avocado_qemu/__init__.py  |  9 +
> >>  tests/migration/guestperf/engine.py| 10 +++---
> >>  tests/qemu-iotests/iotests.py  |  8 ++--
> >>  tests/vm/basevm.py |  9 +++--
> >>  10 files changed, 40 insertions(+), 18 deletions(-)
> >>  rename scripts/qemu.py => python/qemu/__init__.py (98%)
> >>  rename {scripts/qmp => python/qemu}/qmp.py (100%)
> >>  rename {scripts => python/qemu}/qtest.py (97%)
> > 
> > What if we keep `qmp.py` and `qtest.py` directly under `/python`
> > directory?  It seems it can be more semantic regarding the subject of
> > each module. I'm not completely sure about `qmp.py`, but definetly I
> > think qtest should be under python directly.
> > 
> 
> I still think having a uniform starting point for the module, such as
> "qemu" is a good practice for all QEMU Python code.  Having independent
> first level modules will make it harder to understand where the code is
> coming from.  For instance, we have the option of coming up with a
> structure that would allow:
> 
> 
>  import os
>  import struct
>  import qmp
>  import urllib
> 
> 
> And the origin of "qmp" would go unnoticed.  Alternatively:
> 
> 
>  import os
>  import struct
>  import urllib
>  from qemu import qmp
> 
> 
> Is much clearer.  Now, with regards to "qmp", it's pretty much
> standalone, uses only Python libraries, while "qemu" consumes "qmp"
> functionality.  I believe having "qemu.qmp" is appropriate here.
> 
> Now, for "qtest", you have a point, it uses QEMUMachine, and its content
> is very similar to what's available under "qemu" itself (that is,
> QEMUMachine itself).  IMO, it would make sense to just have the
> QEMUQtestProtocol and QEMUQtestMachine at the "qemu" level at this
> point, that is, fold their content into "qemu/__init__.py".
> 
> The API usage would then become:
> 
> 
>  from qemu import QEMUQTestMachine
> 
> 
> Similar to:
> 
> 
>  from qemu import QEMUMachine
> 
> 
> I refrained from changing too much at the initial RFC time, though,
> exactly to make less biased discussions.

Well, I agree with you here. For a first step we shouldn't be changing too
much the structure. I'm ok moving forward this way for now.

> 
> 
> >>  delete mode 100644 scripts/qmp/__init__.py
> >>
> >> diff --git a/configure b/configure
> >> index 0a3c6a72c3..2b64c51009 100755
> > [...]
> >> rename from scripts/qemu.py
> >> rename to python/qemu/__init__.py
> > [...]
> >> diff --git a/scripts/qmp/qmp.py b/python/qemu/qmp.py
> >> similarity index 100%
> >> rename from scripts/qmp/qmp.py
> >> rename to python/qemu/qmp.py
> >> diff --git a/scripts/qtest.py b/python/qemu/qtest.py
> >> similarity index 97%
> >> rename from scripts/qtest.py
> >> rename to python/qemu/qtest.py
> >> index adf1fe3f26..bff79cdd13 100644
> >> --- a/scripts/qtest.py
> >> +++ b/python/qemu/qtest.py
> > [...]
> >> diff --git a/scripts/device-crash-test b/scripts/device-crash-test
> >> index e93a7c0c84..c75ae0ecbc 100755
> >> --- a/scripts/device-crash-test
> >> +++ b/scripts/device-crash-test
> >> @@ -35,6 +35,11 @@ import random
> >>  import argparse
> >>  from itertools import chain
> >>  

Re: [Qemu-block] [RFC PATCH] Introduce Python module structure

[Cleber Rosa]

On 11/27/18 2:49 PM, Eduardo Habkost wrote:
> On Tue, Nov 27, 2018 at 05:00:07PM -0200, Caio Carrara wrote:
>> Hi, Cleber.
>>
>> On Tue, Nov 27, 2018 at 05:50:34AM -0500, Cleber Rosa wrote:
>>> This is a simple move of Python code that wraps common QEMU
>>> functionality, and are used by a number of different tests
>>> and scripts.
>>>
>>> By treating that code as a real Python module, we can more easily,
>>> among other things:
>>>  * reuse more code
>>>  * apply a more consistent style
>>>  * add tests to that code
>>>  * generate documentation
>>>
>>> Signed-off-by: Cleber Rosa 
>>> ---
>>>  configure  |  1 +
>>>  scripts/qemu.py => python/qemu/__init__.py | 11 ++-
>>
>> Well, we all know how difficult is to pick up names, but I would avoid
>> use `python` here. This is the name of python bin itself. Is there any
>> chance of a clash? I do not have a specific case right now, I'm just
>> wondering if it can happen we should avoid.
> 
> I'm don't see how it would be a problem: this won't be the name
> of a Python module, but just a directory to appear on sys.path.
> 
> 

Right, that's my understanding.

>>
>>>  {scripts/qmp => python/qemu}/qmp.py|  0
>>>  {scripts => python/qemu}/qtest.py  |  5 +++--
>>>  scripts/device-crash-test  |  5 +
>>>  scripts/qmp/__init__.py|  0
>>>  tests/acceptance/avocado_qemu/__init__.py  |  9 +
>>>  tests/migration/guestperf/engine.py| 10 +++---
>>>  tests/qemu-iotests/iotests.py  |  8 ++--
>>>  tests/vm/basevm.py |  9 +++--
>>>  10 files changed, 40 insertions(+), 18 deletions(-)
>>>  rename scripts/qemu.py => python/qemu/__init__.py (98%)
>>>  rename {scripts/qmp => python/qemu}/qmp.py (100%)
>>>  rename {scripts => python/qemu}/qtest.py (97%)
>>
>> What if we keep `qmp.py` and `qtest.py` directly under `/python`
>> directory?  It seems it can be more semantic regarding the subject of
>> each module. I'm not completely sure about `qmp.py`, but definetly I
>> think qtest should be under python directly.
> 
> I'd prefer to have everything inside a "qemu" top-level package
> to avoid module namespace conflicts with other software.
> 
> 

Agreed.  See my other reply on this thread.

>>
>>>  delete mode 100644 scripts/qmp/__init__.py
>>>
>>> diff --git a/configure b/configure
>>> index 0a3c6a72c3..2b64c51009 100755
>> [...]
>>> rename from scripts/qemu.py
>>> rename to python/qemu/__init__.py
>> [...]
>>> diff --git a/scripts/qmp/qmp.py b/python/qemu/qmp.py
>>> similarity index 100%
>>> rename from scripts/qmp/qmp.py
>>> rename to python/qemu/qmp.py
>>> diff --git a/scripts/qtest.py b/python/qemu/qtest.py
>>> similarity index 97%
>>> rename from scripts/qtest.py
>>> rename to python/qemu/qtest.py
>>> index adf1fe3f26..bff79cdd13 100644
>>> --- a/scripts/qtest.py
>>> +++ b/python/qemu/qtest.py
>> [...]
>>> diff --git a/scripts/device-crash-test b/scripts/device-crash-test
>>> index e93a7c0c84..c75ae0ecbc 100755
>>> --- a/scripts/device-crash-test
>>> +++ b/scripts/device-crash-test
>>> @@ -35,6 +35,11 @@ import random
>>>  import argparse
>>>  from itertools import chain
>>>  
>>> +THIS_DIR = os.path.dirname(os.path.abspath(__file__))
>>> +TOP_DIR = os.path.dirname(THIS_DIR)
>>> +PYTHON_MODULE_PATH = os.path.join(TOP_DIR, 'python')
>>> +sys.path.append(PYTHON_MODULE_PATH)
> 
> I would prefer to use pathlib:
> 
>   from pathlib import Path
>   sys.path.append(str(Path(__file__).parent / '..' / 'python'))
> 
> but pathlib is not available on Python 2, so we could at least
> avoid using dirname(dirname(abspath(...))) and use '..' instead:

Right, I mostly have to live with Python compatible code, so pathlib is
something that is constantly under my radar.

> 
>   MY_DIR = os.path.dirname(__file__)
>   sys.path.append(os.path.join(MY_DIR, '..', 'python'))
> 
> 

Yep, I have no problem with alternative ways of getting to the Python
module directory.

What I've found though on many situations, though, is that having easy
access to the top most directory is helpful in a lot of Python modules.
 For instance, both "tests/acceptance/avocado_qemu/__init__.py" and
"tests/vm/basevm.py" are relevant examples.  Another experience I have
that confirms that is the Avocado selfests:

 $ cd avocado/selftests
 $ git grep BASEDIR | wc -l
 96

But this is a really minor thing at this point.

>>
>> This sys.path handling to use the new QEMU Python module is not good. I
>> understand it can be a first step, but to expect everyone knows/do it to
>> use the module is a bad assumption because it's not intuitive and can
>> cause some confusion.
>>
>> If we need something available from a Python script/module that is not
>> directly acessible from PYTHONPATH we should install it so Python can
>> turn it available. So, probably we need to think make `python/qemu` a
>> proper installable module.
> 
> Avoiding the sys.path trick would be nice, but 

Re: [Qemu-block] [RFC PATCH] Introduce Python module structure

[Cleber Rosa]

On 11/27/18 2:00 PM, Caio Carrara wrote:
> Hi, Cleber.
> 
> On Tue, Nov 27, 2018 at 05:50:34AM -0500, Cleber Rosa wrote:
>> This is a simple move of Python code that wraps common QEMU
>> functionality, and are used by a number of different tests
>> and scripts.
>>
>> By treating that code as a real Python module, we can more easily,
>> among other things:
>>  * reuse more code
>>  * apply a more consistent style
>>  * add tests to that code
>>  * generate documentation
>>
>> Signed-off-by: Cleber Rosa 
>> ---
>>  configure  |  1 +
>>  scripts/qemu.py => python/qemu/__init__.py | 11 ++-
> 
> Well, we all know how difficult is to pick up names, but I would avoid
> use `python` here. This is the name of python bin itself. Is there any
> chance of a clash? I do not have a specific case right now, I'm just
> wondering if it can happen we should avoid.
> 

The point of the "python" directory, is that it's not supposed to be the
*module* name, but a holder for the Python module, which is "qemu".
Given that "python is not a module", has no __init__.py, I don't think
it'll ever clash.

The reason for picking the name is that it makes it obvious, within the
QEMU tree, that this is Python stuff.  I already have some tests, to
test the "Python stuff", to put it under "python", probably in
"python/tests".

Anyway, it'd be nice to think of alternatives here.

>>  {scripts/qmp => python/qemu}/qmp.py|  0
>>  {scripts => python/qemu}/qtest.py  |  5 +++--
>>  scripts/device-crash-test  |  5 +
>>  scripts/qmp/__init__.py|  0
>>  tests/acceptance/avocado_qemu/__init__.py  |  9 +
>>  tests/migration/guestperf/engine.py| 10 +++---
>>  tests/qemu-iotests/iotests.py  |  8 ++--
>>  tests/vm/basevm.py |  9 +++--
>>  10 files changed, 40 insertions(+), 18 deletions(-)
>>  rename scripts/qemu.py => python/qemu/__init__.py (98%)
>>  rename {scripts/qmp => python/qemu}/qmp.py (100%)
>>  rename {scripts => python/qemu}/qtest.py (97%)
> 
> What if we keep `qmp.py` and `qtest.py` directly under `/python`
> directory?  It seems it can be more semantic regarding the subject of
> each module. I'm not completely sure about `qmp.py`, but definetly I
> think qtest should be under python directly.
> 

I still think having a uniform starting point for the module, such as
"qemu" is a good practice for all QEMU Python code.  Having independent
first level modules will make it harder to understand where the code is
coming from.  For instance, we have the option of coming up with a
structure that would allow:


 import os
 import struct
 import qmp
 import urllib


And the origin of "qmp" would go unnoticed.  Alternatively:


 import os
 import struct
 import urllib
 from qemu import qmp


Is much clearer.  Now, with regards to "qmp", it's pretty much
standalone, uses only Python libraries, while "qemu" consumes "qmp"
functionality.  I believe having "qemu.qmp" is appropriate here.

Now, for "qtest", you have a point, it uses QEMUMachine, and its content
is very similar to what's available under "qemu" itself (that is,
QEMUMachine itself).  IMO, it would make sense to just have the
QEMUQtestProtocol and QEMUQtestMachine at the "qemu" level at this
point, that is, fold their content into "qemu/__init__.py".

The API usage would then become:


 from qemu import QEMUQTestMachine


Similar to:


 from qemu import QEMUMachine


I refrained from changing too much at the initial RFC time, though,
exactly to make less biased discussions.


>>  delete mode 100644 scripts/qmp/__init__.py
>>
>> diff --git a/configure b/configure
>> index 0a3c6a72c3..2b64c51009 100755
> [...]
>> rename from scripts/qemu.py
>> rename to python/qemu/__init__.py
> [...]
>> diff --git a/scripts/qmp/qmp.py b/python/qemu/qmp.py
>> similarity index 100%
>> rename from scripts/qmp/qmp.py
>> rename to python/qemu/qmp.py
>> diff --git a/scripts/qtest.py b/python/qemu/qtest.py
>> similarity index 97%
>> rename from scripts/qtest.py
>> rename to python/qemu/qtest.py
>> index adf1fe3f26..bff79cdd13 100644
>> --- a/scripts/qtest.py
>> +++ b/python/qemu/qtest.py
> [...]
>> diff --git a/scripts/device-crash-test b/scripts/device-crash-test
>> index e93a7c0c84..c75ae0ecbc 100755
>> --- a/scripts/device-crash-test
>> +++ b/scripts/device-crash-test
>> @@ -35,6 +35,11 @@ import random
>>  import argparse
>>  from itertools import chain
>>  
>> +THIS_DIR = os.path.dirname(os.path.abspath(__file__))
>> +TOP_DIR = os.path.dirname(THIS_DIR)
>> +PYTHON_MODULE_PATH = os.path.join(TOP_DIR, 'python')
>> +sys.path.append(PYTHON_MODULE_PATH)
> 
> This sys.path handling to use the new QEMU Python module is not good. I
> understand it can be a first step, but to expect everyone knows/do it to
> use the module is a bad assumption because it's not intuitive and can
> cause some confusion.
> 
> If we need something available from a Python 

Re: [Qemu-block] [RFC PATCH] Introduce Python module structure

[Eduardo Habkost]

On Tue, Nov 27, 2018 at 05:00:07PM -0200, Caio Carrara wrote:
> Hi, Cleber.
> 
> On Tue, Nov 27, 2018 at 05:50:34AM -0500, Cleber Rosa wrote:
> > This is a simple move of Python code that wraps common QEMU
> > functionality, and are used by a number of different tests
> > and scripts.
> > 
> > By treating that code as a real Python module, we can more easily,
> > among other things:
> >  * reuse more code
> >  * apply a more consistent style
> >  * add tests to that code
> >  * generate documentation
> > 
> > Signed-off-by: Cleber Rosa 
> > ---
> >  configure  |  1 +
> >  scripts/qemu.py => python/qemu/__init__.py | 11 ++-
> 
> Well, we all know how difficult is to pick up names, but I would avoid
> use `python` here. This is the name of python bin itself. Is there any
> chance of a clash? I do not have a specific case right now, I'm just
> wondering if it can happen we should avoid.

I'm don't see how it would be a problem: this won't be the name
of a Python module, but just a directory to appear on sys.path.


> 
> >  {scripts/qmp => python/qemu}/qmp.py|  0
> >  {scripts => python/qemu}/qtest.py  |  5 +++--
> >  scripts/device-crash-test  |  5 +
> >  scripts/qmp/__init__.py|  0
> >  tests/acceptance/avocado_qemu/__init__.py  |  9 +
> >  tests/migration/guestperf/engine.py| 10 +++---
> >  tests/qemu-iotests/iotests.py  |  8 ++--
> >  tests/vm/basevm.py |  9 +++--
> >  10 files changed, 40 insertions(+), 18 deletions(-)
> >  rename scripts/qemu.py => python/qemu/__init__.py (98%)
> >  rename {scripts/qmp => python/qemu}/qmp.py (100%)
> >  rename {scripts => python/qemu}/qtest.py (97%)
> 
> What if we keep `qmp.py` and `qtest.py` directly under `/python`
> directory?  It seems it can be more semantic regarding the subject of
> each module. I'm not completely sure about `qmp.py`, but definetly I
> think qtest should be under python directly.

I'd prefer to have everything inside a "qemu" top-level package
to avoid module namespace conflicts with other software.


> 
> >  delete mode 100644 scripts/qmp/__init__.py
> > 
> > diff --git a/configure b/configure
> > index 0a3c6a72c3..2b64c51009 100755
> [...]
> > rename from scripts/qemu.py
> > rename to python/qemu/__init__.py
> [...]
> > diff --git a/scripts/qmp/qmp.py b/python/qemu/qmp.py
> > similarity index 100%
> > rename from scripts/qmp/qmp.py
> > rename to python/qemu/qmp.py
> > diff --git a/scripts/qtest.py b/python/qemu/qtest.py
> > similarity index 97%
> > rename from scripts/qtest.py
> > rename to python/qemu/qtest.py
> > index adf1fe3f26..bff79cdd13 100644
> > --- a/scripts/qtest.py
> > +++ b/python/qemu/qtest.py
> [...]
> > diff --git a/scripts/device-crash-test b/scripts/device-crash-test
> > index e93a7c0c84..c75ae0ecbc 100755
> > --- a/scripts/device-crash-test
> > +++ b/scripts/device-crash-test
> > @@ -35,6 +35,11 @@ import random
> >  import argparse
> >  from itertools import chain
> >  
> > +THIS_DIR = os.path.dirname(os.path.abspath(__file__))
> > +TOP_DIR = os.path.dirname(THIS_DIR)
> > +PYTHON_MODULE_PATH = os.path.join(TOP_DIR, 'python')
> > +sys.path.append(PYTHON_MODULE_PATH)

I would prefer to use pathlib:

  from pathlib import Path
  sys.path.append(str(Path(__file__).parent / '..' / 'python'))

but pathlib is not available on Python 2, so we could at least
avoid using dirname(dirname(abspath(...))) and use '..' instead:

  MY_DIR = os.path.dirname(__file__)
  sys.path.append(os.path.join(MY_DIR, '..', 'python'))


> 
> This sys.path handling to use the new QEMU Python module is not good. I
> understand it can be a first step, but to expect everyone knows/do it to
> use the module is a bad assumption because it's not intuitive and can
> cause some confusion.
> 
> If we need something available from a Python script/module that is not
> directly acessible from PYTHONPATH we should install it so Python can
> turn it available. So, probably we need to think make `python/qemu` a
> proper installable module.

Avoiding the sys.path trick would be nice, but how can we do that
while keeping scripts still working out of the box?  In other
words, how can we keep this working:

  $ git clone https://.../qemu.git
  $ cd qemu
  $ ./scripts/qmp-shell /path/to/qmp-socket


> 
> > +
> >  from qemu import QEMUMachine
> >  
> >  logger = logging.getLogger('device-crash-test')
> > diff --git a/scripts/qmp/__init__.py b/scripts/qmp/__init__.py
> > deleted file mode 100644
> [...]
> > diff --git a/tests/migration/guestperf/engine.py 
> > b/tests/migration/guestperf/engine.py
> > index 398e3f2706..73c9b66821 100644
> > --- a/tests/migration/guestperf/engine.py
> > +++ b/tests/migration/guestperf/engine.py
> > @@ -24,13 +24,17 @@ import re
> >  import sys
> >  import time
> >  
> > -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', '..', 
> > 'scripts'))
> > -import qemu
> > 

Re: [Qemu-block] [RFC PATCH] Introduce Python module structure

[Caio Carrara]

Hi, Cleber.

On Tue, Nov 27, 2018 at 05:50:34AM -0500, Cleber Rosa wrote:
> This is a simple move of Python code that wraps common QEMU
> functionality, and are used by a number of different tests
> and scripts.
> 
> By treating that code as a real Python module, we can more easily,
> among other things:
>  * reuse more code
>  * apply a more consistent style
>  * add tests to that code
>  * generate documentation
> 
> Signed-off-by: Cleber Rosa 
> ---
>  configure  |  1 +
>  scripts/qemu.py => python/qemu/__init__.py | 11 ++-

Well, we all know how difficult is to pick up names, but I would avoid
use `python` here. This is the name of python bin itself. Is there any
chance of a clash? I do not have a specific case right now, I'm just
wondering if it can happen we should avoid.

>  {scripts/qmp => python/qemu}/qmp.py|  0
>  {scripts => python/qemu}/qtest.py  |  5 +++--
>  scripts/device-crash-test  |  5 +
>  scripts/qmp/__init__.py|  0
>  tests/acceptance/avocado_qemu/__init__.py  |  9 +
>  tests/migration/guestperf/engine.py| 10 +++---
>  tests/qemu-iotests/iotests.py  |  8 ++--
>  tests/vm/basevm.py |  9 +++--
>  10 files changed, 40 insertions(+), 18 deletions(-)
>  rename scripts/qemu.py => python/qemu/__init__.py (98%)
>  rename {scripts/qmp => python/qemu}/qmp.py (100%)
>  rename {scripts => python/qemu}/qtest.py (97%)

What if we keep `qmp.py` and `qtest.py` directly under `/python`
directory?  It seems it can be more semantic regarding the subject of
each module. I'm not completely sure about `qmp.py`, but definetly I
think qtest should be under python directly.

>  delete mode 100644 scripts/qmp/__init__.py
> 
> diff --git a/configure b/configure
> index 0a3c6a72c3..2b64c51009 100755
[...]
> rename from scripts/qemu.py
> rename to python/qemu/__init__.py
[...]
> diff --git a/scripts/qmp/qmp.py b/python/qemu/qmp.py
> similarity index 100%
> rename from scripts/qmp/qmp.py
> rename to python/qemu/qmp.py
> diff --git a/scripts/qtest.py b/python/qemu/qtest.py
> similarity index 97%
> rename from scripts/qtest.py
> rename to python/qemu/qtest.py
> index adf1fe3f26..bff79cdd13 100644
> --- a/scripts/qtest.py
> +++ b/python/qemu/qtest.py
[...]
> diff --git a/scripts/device-crash-test b/scripts/device-crash-test
> index e93a7c0c84..c75ae0ecbc 100755
> --- a/scripts/device-crash-test
> +++ b/scripts/device-crash-test
> @@ -35,6 +35,11 @@ import random
>  import argparse
>  from itertools import chain
>  
> +THIS_DIR = os.path.dirname(os.path.abspath(__file__))
> +TOP_DIR = os.path.dirname(THIS_DIR)
> +PYTHON_MODULE_PATH = os.path.join(TOP_DIR, 'python')
> +sys.path.append(PYTHON_MODULE_PATH)

This sys.path handling to use the new QEMU Python module is not good. I
understand it can be a first step, but to expect everyone knows/do it to
use the module is a bad assumption because it's not intuitive and can
cause some confusion.

If we need something available from a Python script/module that is not
directly acessible from PYTHONPATH we should install it so Python can
turn it available. So, probably we need to think make `python/qemu` a
proper installable module.

> +
>  from qemu import QEMUMachine
>  
>  logger = logging.getLogger('device-crash-test')
> diff --git a/scripts/qmp/__init__.py b/scripts/qmp/__init__.py
> deleted file mode 100644
[...]
> diff --git a/tests/migration/guestperf/engine.py 
> b/tests/migration/guestperf/engine.py
> index 398e3f2706..73c9b66821 100644
> --- a/tests/migration/guestperf/engine.py
> +++ b/tests/migration/guestperf/engine.py
> @@ -24,13 +24,17 @@ import re
>  import sys
>  import time
>  
> -sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', '..', 
> 'scripts'))
> -import qemu
> -import qmp.qmp
>  from guestperf.progress import Progress, ProgressStats
>  from guestperf.report import Report
>  from guestperf.timings import TimingRecord, Timings
>  
> +THIS_DIR = os.path.dirname(os.path.abspath(__file__))
> +TOP_DIR = os.path.dirname(os.path.dirname(os.path.dirname(THIS_DIR)))
> +PYTHON_MODULE_PATH = os.path.join(TOP_DIR, 'python')
> +sys.path.append(PYTHON_MODULE_PATH)
> +
> +import qemu

Since `qemu` is a common word here, I would rather import the members
directly than only the module. Just like you did in
`/tests/vm/basevm,py`

> +
>  
>  class Engine(object):
>  
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index d537538ba0..92fddd2a58 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -31,8 +31,12 @@ import logging
[...]
> --- a/tests/vm/basevm.py
> +++ b/tests/vm/basevm.py
> @@ -17,8 +17,6 @@ import sys
>  import logging
>  import time
>  import datetime
> -sys.path.append(os.path.join(os.path.dirname(__file__), "..", "..", 
> "scripts"))
> -from qemu import QEMUMachine, kvm_available
>  import subprocess
>  import hashlib

Re: [Qemu-block] [Qemu-devel] [PATCH] scsi: Address spurious clang warning

[Eric Blake]

On 11/27/18 12:49 PM, John Snow wrote:

Some versions of Clang prior to 6.0 (and some builds of clang after,
such as 6.0.1-2.fc28) fail to recognize { 0 } as a valid initializer
for a struct with subobjects when -Wmissing-braces is enabled.

https://bugs.llvm.org/show_bug.cgi?id=21689 and
https://reviews.llvm.org/rL314499 suggests this should be fixed in 6.0,
but it might not be the case for older versions or downstream versions.

For now, follow the precedent of ebf2a499 and replace the standard { 0 }
with the accepted { } to silence this warning and allow the build to
work under clang 6.0.1-2.fc28, and builds prior to 6.0.

Signed-off-by: John Snow 



Reviewed-by: Eric Blake 

I'm okay if this goes into -rc3 as a build-fix; I'm also okay if it 
slips to 4.0.



---

What I am actually less clear on is why this appears to be a problem
only now; since the introduction of { 0 } was in 2.11. It might be
a regression only in the fedora distribution of Clang 6.0.


Or even a redefinition of struct dm_ioctl in some header where you are 
just now picking up a new struct layout that tickles the Clang issue in 
relation to the previous layout (since it is possible to have two 
structs that are ABI-compatible but where only one of the two has a 
nested substruct).



+++ b/scsi/qemu-pr-helper.c
@@ -236,7 +236,7 @@ static void dm_init(void)
  perror("Cannot open " CONTROL_PATH);
  exit(1);
  }
-struct dm_ioctl dm = { 0 };
+struct dm_ioctl dm = { };


Random thought: would it be worth having "qemu/compiler.h" define a macro:

#if ...broken clang
#define ZERO_INIT {}
#else
#define ZERO_INIT {0}
#endif

and then rewrite all our '= { 0? }' initializers into '= ZERO_INIT'?  Or 
is that aesthetically too ugly?


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

Re: [Qemu-block] [PATCH 17/18] MAINTAINERS: add myself as a Xen maintainer

[Stefano Stabellini]

On Wed, 21 Nov 2018, Paul Durrant wrote:
> I have made many significant contributions to the Xen code in QEMU,
> particularly the recent patches introducing a new PV device framework.
> I intend to make further significant contributions, porting other PV back-
> ends to the new framework with the intent of eventually removing the
> legacy code. It therefore seems reasonable that I become a maintiner of
> the Xen code.
> 
> Signed-off-by: Paul Durrant 

Acked-by: Stefano Stabellini 

> ---
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paolo Bonzini 
> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5871f035c3..0b668dd205 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -382,6 +382,7 @@ Guest CPU Cores (Xen):
>  X86
>  M: Stefano Stabellini 
>  M: Anthony Perard 
> +M: Paul Durrant 
>  L: xen-de...@lists.xenproject.org
>  S: Supported
>  F: */xen*
> -- 
> 2.11.0
> 

[Qemu-block] [PATCH] scsi: Address spurious clang warning

[John Snow]

Some versions of Clang prior to 6.0 (and some builds of clang after,
such as 6.0.1-2.fc28) fail to recognize { 0 } as a valid initializer
for a struct with subobjects when -Wmissing-braces is enabled.

https://bugs.llvm.org/show_bug.cgi?id=21689 and
https://reviews.llvm.org/rL314499 suggests this should be fixed in 6.0,
but it might not be the case for older versions or downstream versions.

For now, follow the precedent of ebf2a499 and replace the standard { 0 }
with the accepted { } to silence this warning and allow the build to
work under clang 6.0.1-2.fc28, and builds prior to 6.0.

Signed-off-by: John Snow 

---

What I am actually less clear on is why this appears to be a problem
only now; since the introduction of { 0 } was in 2.11. It might be
a regression only in the fedora distribution of Clang 6.0.

With apologies to Paolo, who hates these patches.
---
 scsi/qemu-pr-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index ce40008bfc..e7af637232 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -236,7 +236,7 @@ static void dm_init(void)
 perror("Cannot open " CONTROL_PATH);
 exit(1);
 }
-struct dm_ioctl dm = { 0 };
+struct dm_ioctl dm = { };
 if (!dm_ioctl(DM_VERSION, )) {
 perror("ioctl");
 exit(1);
-- 
2.17.2

Re: [Qemu-block] [PATCH 00/11] qcow2: encryption threads

[Daniel P . Berrangé]

On Fri, Nov 23, 2018 at 07:55:00PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> The series brings threads to qcow2 encryption/decryption path,
> like it is already done for compression.
> 
> Based-on: Kevin's block-next branch [d3db1496c5]
> 
> Performance gain is illustrated by the following test:
> 
> ]# cat test.sh 
> #!/bin/bash
> 
> size=1G
> src=/ssd/src.raw
> dst=/ssd/dst.enc.qcow2
> 
> echo create source for tests
> ./qemu-img create -f raw "$src" $size
> ./qemu-io -f raw -c "write -P 0xa 0 $size" "$src"
> 
> for w in "" "-W"; do
> echo -e "\n\nTest with additional paramter for qemu-img: '$w'\n"
> 
> echo create target...
> ./qemu-img create -f qcow2 --object secret,id=sec0,data=test -o 
> encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10 "$dst" $size
> echo
> 
> echo test...
> time ./qemu-img convert $w -f raw --object secret,id=sec0,data=test 
> --target-image-opts -n "$src" 
> "driver=qcow2,file.filename=$dst,encrypt.key-secret=sec0"
> done

Note that using  iter-time=10 is removing the time penalty for opening
the luks device. This is why you didn't see the significant negative
performance impact of creating many  QCryptoBlock instances, one for
each thread.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [PATCH 07/11] qcow2-threads: add encryption

[Daniel P . Berrangé]

On Fri, Nov 23, 2018 at 07:55:07PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Add thread-based encrypt/decrypt. QCrypto don't support parallel
> operations with one block, so we need QCryptoBlock for each thread.



> +static int qcow2_crypto_blocks_open(BDRVQcow2State *s,
> +const char *optprefix,
> +QCryptoBlockReadFunc readfunc,
> +void *opaque,
> +unsigned int flags,
> +Error **errp)
> +{
> +int i;
> +
> +s->crypto = qcrypto_block_open(s->crypto_opts, optprefix,
> +   readfunc, opaque, flags, errp);
> +if (!s->crypto) {
> +qcrypto_block_free(s->crypto);
> +return -EINVAL;
> +}
> +
> +for (i = 0; i < QCOW2_MAX_THREADS; i++) {
> +s->threads.per_thread[i].crypto =
> +qcrypto_block_open(s->crypto_opts, optprefix,
> +   readfunc, opaque, flags, errp);

We really don't want to be doing this.  LUKS has an intentional time
penalty for opening devices. Each time you open a disk, we expect to
burn 1-2 seconds in CPU time. So this is multiplying that burn by
QCOW2_MAX_THREADS.

What we need todo is modify QCryptoBlock so that it can (optionally)
create many QCryptoCipher instances, allowing each thread to have its
own instance. We'll also need locking around the iv generator calls.

> +if (!s->threads.per_thread[i].crypto) {
> +qcow2_crypto_blocks_free(s);
> +return -EINVAL;
> +}
> +}
> +
> +return 0;
> +}

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] encrypt in threads

[Daniel P . Berrangé]

On Thu, Nov 22, 2018 at 01:01:20PM +, Vladimir Sementsov-Ogievskiy wrote:
> 21.11.2018 20:30, Vladimir Sementsov-Ogievskiy wrote:
> > Hi Daniel!
> > 
> > After moving compression to threads in Qcow2 it's an obvious next step to
> > "threadyfy" encryption in Qcow2 too.
> > 
> > But it turned out to be not as simple as I assumed. If I call 
> > qcrypto_block_encrypt
> > in parallel threads with the same first argument (block), it just produce 
> > wrong
> > things (pattern verification fails in iotests)..
> > 
> > So, can you advise the way to parallelize encryption/decryption?
> > 
> 
> Hmm, just creating QCryptoBlock per each thread helped. Is it correct thing 
> to do?

That's rather a heavy weight approach and would cause pain when we want
to support future options such as keyslot updates, and in the future,
LUKSv2 with master key changes.

Probably what's needed is change to QCryptoBlock struct so that there
can be multiple QCryptoCipher instances allocated - one per thread.

We might also need to introduce some locking around usage of the
QCryptoIVGen object, since that has a QCryptoCipher handle too.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] encrypt in threads

[Daniel P . Berrangé]

On Wed, Nov 21, 2018 at 05:30:53PM +, Vladimir Sementsov-Ogievskiy wrote:
> Hi Daniel!
> 
> After moving compression to threads in Qcow2 it's an obvious next step to
> "threadyfy" encryption in Qcow2 too.
> 
> But it turned out to be not as simple as I assumed. If I call 
> qcrypto_block_encrypt
> in parallel threads with the same first argument (block), it just produce 
> wrong
> things (pattern verification fails in iotests)..

Yes, this makes sense, because the underlying crypto impls all require
that their state is only used from a single thread at any time. What's
likely happening is that the IV is being scrambled so we're encrypting
with the wrong tweak.

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [Qemu-block] [PATCH 0/6] Misc fixes to NBD

[Eric Blake]

On 11/16/18 9:53 AM, Daniel P. Berrangé wrote:

This does two minor fixes to the NBD code and adds significant coverage
of the NBD TLS support to detect future problems.

The first two patches should be for 3.1.

The tests can wait till 4.0 if desired.


Although this series is now in 3.1, I can think of further enhancements 
we should add for 4.0 (summarizing an IRC conversation with Dan). 
Capturing it here to remember things...


- we need iotests coverage of Pre-Shared Keys (PSK) as an alternative to 
certificates (either add on to 233, or a new test)
- add an optional QMP parameter for specifying the hostname to validate 
a certificate against when using a Unix socket with TLS (compare 
tls-hostname added to 'migrate'), rather than the current restriction 
that using TLS with an NBD client requires TCP


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

Re: [Qemu-block] [PULL 0/3] Block layer patches

[Peter Maydell]

On Tue, 27 Nov 2018 at 14:07, Kevin Wolf  wrote:
>
> The following changes since commit d5d31c9a8ab5e87db4230602a6fd5da8eb13135c:
>
>   Merge remote-tracking branch 
> 'remotes/ehabkost/tags/x86-for-3.1-pull-request' into staging (2018-11-27 
> 09:55:05 +)
>
> are available in the Git repository at:
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 6da021815e752b3ca3a547eed53f3e92a8a35452:
>
>   nvme: Fix spurious interrupts (2018-11-27 12:59:00 +0100)
>
> 
> Block layer patches:
>
> - block: Fix crash on migration with explicit child nodes
> - nvme: Fix spurious interrupts
>
> 

Applied, thanks.

-- PMM

[Qemu-block] [PULL 0/3] Block layer patches

[Kevin Wolf]

The following changes since commit d5d31c9a8ab5e87db4230602a6fd5da8eb13135c:

  Merge remote-tracking branch 'remotes/ehabkost/tags/x86-for-3.1-pull-request' 
into staging (2018-11-27 09:55:05 +)

are available in the Git repository at:

  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 6da021815e752b3ca3a547eed53f3e92a8a35452:

  nvme: Fix spurious interrupts (2018-11-27 12:59:00 +0100)


Block layer patches:

- block: Fix crash on migration with explicit child nodes
- nvme: Fix spurious interrupts


Keith Busch (1):
  nvme: Fix spurious interrupts

Kevin Wolf (2):
  block: Don't inactivate children before parents
  iotests: Test migration with -blockdev

 block.c|  84 +++
 hw/block/nvme.c|   4 +-
 tests/qemu-iotests/234 | 121 +
 tests/qemu-iotests/234.out |  30 +++
 tests/qemu-iotests/group   |   1 +
 5 files changed, 208 insertions(+), 32 deletions(-)
 create mode 100755 tests/qemu-iotests/234
 create mode 100644 tests/qemu-iotests/234.out

[Qemu-block] [PULL 2/3] iotests: Test migration with -blockdev

[Kevin Wolf]

Check that block node activation and inactivation works with a block
graph that is built with individually created nodes.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/234 | 121 +
 tests/qemu-iotests/234.out |  30 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 152 insertions(+)
 create mode 100755 tests/qemu-iotests/234
 create mode 100644 tests/qemu-iotests/234.out

diff --git a/tests/qemu-iotests/234 b/tests/qemu-iotests/234
new file mode 100755
index 00..a8185b4360
--- /dev/null
+++ b/tests/qemu-iotests/234
@@ -0,0 +1,121 @@
+#!/usr/bin/env python
+#
+# Copyright (C) 2018 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 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 .
+#
+# Creator/Owner: Kevin Wolf 
+#
+# Check that block node activation and inactivation works with a block graph
+# that is built with individually created nodes
+
+import iotests
+import os
+
+iotests.verify_image_format(supported_fmts=['qcow2'])
+iotests.verify_platform(['linux'])
+
+with iotests.FilePath('img') as img_path, \
+ iotests.FilePath('backing') as backing_path, \
+ iotests.FilePath('mig_fifo_a') as fifo_a, \
+ iotests.FilePath('mig_fifo_b') as fifo_b, \
+ iotests.VM(path_suffix='a') as vm_a, \
+ iotests.VM(path_suffix='b') as vm_b:
+
+iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, backing_path, '64M')
+iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, img_path, '64M')
+
+os.mkfifo(fifo_a)
+os.mkfifo(fifo_b)
+
+iotests.log('Launching source VM...')
+(vm_a.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path))
+ .add_blockdev('%s,file=drive0-file,node-name=drive0' % 
(iotests.imgfmt))
+ .add_blockdev('file,filename=%s,node-name=drive0-backing-file' % 
(backing_path))
+ .add_blockdev('%s,file=drive0-backing-file,node-name=drive0-backing' 
% (iotests.imgfmt))
+ .launch())
+
+iotests.log('Launching destination VM...')
+(vm_b.add_blockdev('file,filename=%s,node-name=drive0-file' % (img_path))
+ .add_blockdev('%s,file=drive0-file,node-name=drive0' % 
(iotests.imgfmt))
+ .add_blockdev('file,filename=%s,node-name=drive0-backing-file' % 
(backing_path))
+ .add_blockdev('%s,file=drive0-backing-file,node-name=drive0-backing' 
% (iotests.imgfmt))
+ .add_incoming("exec: cat '%s'" % (fifo_a))
+ .launch())
+
+# Add a child node that was created after the parent node. The reverse case
+# is covered by the -blockdev options above.
+iotests.log(vm_a.qmp('blockdev-snapshot', node='drive0-backing',
+ overlay='drive0'))
+iotests.log(vm_b.qmp('blockdev-snapshot', node='drive0-backing',
+ overlay='drive0'))
+
+iotests.log('Enabling migration QMP events on A...')
+iotests.log(vm_a.qmp('migrate-set-capabilities', capabilities=[
+{
+'capability': 'events',
+'state': True
+}
+]))
+
+iotests.log('Starting migration to B...')
+iotests.log(vm_a.qmp('migrate', uri='exec:cat >%s' % (fifo_a)))
+with iotests.Timeout(3, 'Migration does not complete'):
+while True:
+event = vm_a.event_wait('MIGRATION')
+iotests.log(event, filters=[iotests.filter_qmp_event])
+if event['data']['status'] == 'completed':
+break
+
+iotests.log(vm_a.qmp('query-migrate')['return']['status'])
+iotests.log(vm_b.qmp('query-migrate')['return']['status'])
+
+iotests.log(vm_a.qmp('query-status'))
+iotests.log(vm_b.qmp('query-status'))
+
+iotests.log('Add a second parent to drive0-file...')
+iotests.log(vm_b.qmp('blockdev-add', driver='raw', file='drive0-file',
+ node_name='drive0-raw'))
+
+iotests.log('Restart A with -incoming and second parent...')
+vm_a.shutdown()
+(vm_a.add_blockdev('raw,file=drive0-file,node-name=drive0-raw')
+ .add_incoming("exec: cat '%s'" % (fifo_b))
+ .launch())
+
+iotests.log(vm_a.qmp('blockdev-snapshot', node='drive0-backing',
+ overlay='drive0'))
+
+iotests.log('Enabling migration QMP events on B...')
+iotests.log(vm_b.qmp('migrate-set-capabilities', capabilities=[
+{
+'capability': 'events',
+'state': True
+}
+]))
+
+

[Qemu-block] [PULL 1/3] block: Don't inactivate children before parents

[Kevin Wolf]

bdrv_child_cb_inactivate() asserts that parents are already inactive
when children get inactivated. This precondition is necessary because
parents could still issue requests in their inactivation code.

When block nodes are created individually with -blockdev, all of them
are monitor owned and will be returned by bdrv_next() in an undefined
order (in practice, in the order of their creation, which is usually
children before parents), which obviously fails the assertion:

qemu: block.c:899: bdrv_child_cb_inactivate: Assertion `bs->open_flags & 
BDRV_O_INACTIVE' failed.

This patch fixes the ordering by skipping nodes with still active
parents in bdrv_inactivate_recurse() because we know that they will be
covered by recursion when the last active parent becomes inactive.

With the correct parents-before-children ordering, we also got rid of
the reason why commit aad0b7a0bfb introduced two passes, so we can go
back to a single-pass recursion. This is necessary so we can rely on the
BDRV_O_INACTIVE flag to skip nodes with active parents (the flag used
to be set only in pass 2, so we would always skip non-root nodes in
pass 1 because all parents would still be considered active; setting the
flag in pass 1 would mean, that we never skip anything in pass 2 because
all parents are already considered inactive).

Because of the change to single pass, this patch is best reviewed with
whitespace changes ignored.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c | 84 -
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/block.c b/block.c
index 5ba3435f8f..811239ca23 100644
--- a/block.c
+++ b/block.c
@@ -4612,45 +4612,68 @@ void bdrv_invalidate_cache_all(Error **errp)
 }
 }
 
-static int bdrv_inactivate_recurse(BlockDriverState *bs,
-   bool setting_flag)
+static bool bdrv_has_bds_parent(BlockDriverState *bs, bool only_active)
+{
+BdrvChild *parent;
+
+QLIST_FOREACH(parent, >parents, next_parent) {
+if (parent->role->parent_is_bds) {
+BlockDriverState *parent_bs = parent->opaque;
+if (!only_active || !(parent_bs->open_flags & BDRV_O_INACTIVE)) {
+return true;
+}
+}
+}
+
+return false;
+}
+
+static int bdrv_inactivate_recurse(BlockDriverState *bs)
 {
 BdrvChild *child, *parent;
+uint64_t perm, shared_perm;
 int ret;
 
 if (!bs->drv) {
 return -ENOMEDIUM;
 }
 
-if (!setting_flag && bs->drv->bdrv_inactivate) {
+/* Make sure that we don't inactivate a child before its parent.
+ * It will be covered by recursion from the yet active parent. */
+if (bdrv_has_bds_parent(bs, true)) {
+return 0;
+}
+
+assert(!(bs->open_flags & BDRV_O_INACTIVE));
+
+/* Inactivate this node */
+if (bs->drv->bdrv_inactivate) {
 ret = bs->drv->bdrv_inactivate(bs);
 if (ret < 0) {
 return ret;
 }
 }
 
-if (setting_flag && !(bs->open_flags & BDRV_O_INACTIVE)) {
-uint64_t perm, shared_perm;
-
-QLIST_FOREACH(parent, >parents, next_parent) {
-if (parent->role->inactivate) {
-ret = parent->role->inactivate(parent);
-if (ret < 0) {
-return ret;
-}
+QLIST_FOREACH(parent, >parents, next_parent) {
+if (parent->role->inactivate) {
+ret = parent->role->inactivate(parent);
+if (ret < 0) {
+return ret;
 }
 }
+}
 
-bs->open_flags |= BDRV_O_INACTIVE;
+bs->open_flags |= BDRV_O_INACTIVE;
+
+/* Update permissions, they may differ for inactive nodes */
+bdrv_get_cumulative_perm(bs, , _perm);
+bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, _abort);
+bdrv_set_perm(bs, perm, shared_perm);
 
-/* Update permissions, they may differ for inactive nodes */
-bdrv_get_cumulative_perm(bs, , _perm);
-bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, _abort);
-bdrv_set_perm(bs, perm, shared_perm);
-}
 
+/* Recursively inactivate children */
 QLIST_FOREACH(child, >children, next) {
-ret = bdrv_inactivate_recurse(child->bs, setting_flag);
+ret = bdrv_inactivate_recurse(child->bs);
 if (ret < 0) {
 return ret;
 }
@@ -4664,7 +4687,6 @@ int bdrv_inactivate_all(void)
 BlockDriverState *bs = NULL;
 BdrvNextIterator it;
 int ret = 0;
-int pass;
 GSList *aio_ctxs = NULL, *ctx;
 
 for (bs = bdrv_first(); bs; bs = bdrv_next()) {
@@ -4676,17 +4698,17 @@ int bdrv_inactivate_all(void)
 }
 }
 
-/* We do two passes of inactivation. The first pass calls to drivers'
- * .bdrv_inactivate callbacks recursively so all cache is flushed to disk;
- * the second pass sets the BDRV_O_INACTIVE flag so that no further write
- * is allowed. */
-for (pass = 

[Qemu-block] [PULL 3/3] nvme: Fix spurious interrupts

[Kevin Wolf]

From: Keith Busch 

The code had asserted an interrupt every time it was requested to check
for new completion queue entries.This can result in spurious interrupts
seen by the guest OS.

Fix this by asserting an interrupt only if there are un-acknowledged
completion queue entries available.

Reported-by: Guenter Roeck 
Signed-off-by: Keith Busch 
Tested-by: Guenter Roeck 
Signed-off-by: Kevin Wolf 
---
 hw/block/nvme.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9fbe5673cb..7c8c63e8f5 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -272,7 +272,9 @@ static void nvme_post_cqes(void *opaque)
 sizeof(req->cqe));
 QTAILQ_INSERT_TAIL(>req_list, req, entry);
 }
-nvme_irq_assert(n, cq);
+if (cq->tail != cq->head) {
+nvme_irq_assert(n, cq);
+}
 }
 
 static void nvme_enqueue_req_completion(NvmeCQueue *cq, NvmeRequest *req)
-- 
2.19.1

Re: [Qemu-block] [PATCH] qemu-img info lists bitmap directory entries

[Andrey Shinkevich]

Patch amendments below:

1. A description of the 'bitmaps' new member of the structure 
'ImageInfoSpecificQCow2' has been added to the file qapi/block-core.json.

2. Per Eric Blake's note, the version number at the 'Since:' field of 
comments was changed to '4.0'.

Kindly,

Andrey Shinkevich


On 27.11.2018 15:41, Andrey Shinkevich wrote:
> The 'Format specific information' of qemu-img info command will show
> the name, flags and granularity for every QCOW2 bitmap.
>
> Signed-off-by: Andrey Shinkevich 
> ---
> Dear colleagues,
>
> With this patch, qemu-img info will display a name, flags and granularity
> information for every bitmap in the directory section of a QCOW2 image.
> That information appears in the 'Format specific information' section as
> it's shown in the following example:
>
> image: /vz/vmprivate/VM1/harddisk.hdd
> file format: qcow2
> virtual size: 64G (68719476736 bytes)
> disk size: 3.0M
> cluster_size: 1048576
> Format specific information:
>  compat: 1.1
>  lazy refcounts: true
>  refcount bits: 16
>  dirty bitmaps:
>  [0]:
>  flags:
>  [0]: in-use
>  [1]: auto
>  name: {257b5ea5-38c5-410e-a457-71c76f763c60}
>  granularity: 65536
>  [1]:
>  flags:
>  [0]: in-use
>  [1]: auto
>  name: back-up
>  granularity: 65536
>  corrupt: false
>
>   block/qcow2-bitmap.c | 50 ++
>   block/qcow2.c| 10 ++
>   block/qcow2.h|  2 ++
>   qapi/block-core.json | 35 ++-
>   4 files changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index accebef..dcc53db 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1008,6 +1008,56 @@ fail:
>   return false;
>   }
>   
> +static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
> +{
> +Qcow2BitmapInfoFlagsList *list = NULL;
> +Qcow2BitmapInfoFlagsList **plist = 
> +
> +if (flags & BME_FLAG_IN_USE) {
> +Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 
> 1);
> +entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE;
> +*plist = entry;
> +plist = >next;
> +}
> +if (flags & BME_FLAG_AUTO) {
> +Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 
> 1);
> +entry->value = QCOW2_BITMAP_INFO_FLAGS_AUTO;
> +*plist = entry;
> +}
> +return list;
> +}
> +
> +Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
> +Error **errp)
> +{
> +BDRVQcow2State *s = bs->opaque;
> +Qcow2BitmapList *bm_list;
> +Qcow2Bitmap *bm;
> +Qcow2BitmapInfoList *list = NULL;
> +Qcow2BitmapInfoList **plist = 
> +
> +bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
> +   s->bitmap_directory_size, errp);
> +if (bm_list == NULL) {
> +return NULL;
> +}
> +
> +QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> +Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
> +Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
> +info->granularity = 1U << bm->granularity_bits;
> +info->name = g_strdup(bm->name);
> +info->flags = get_bitmap_info_flags(bm->flags);
> +obj->value = info;
> +*plist = obj;
> +plist = >next;
> +}
> +
> +bitmap_list_free(bm_list);
> +
> +return list;
> +}
> +
>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>Error **errp)
>   {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 991d6ac..6485f79 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4254,6 +4254,13 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
>   BDRVQcow2State *s = bs->opaque;
>   ImageInfoSpecific *spec_info;
>   QCryptoBlockInfo *encrypt_info = NULL;
> +Error *local_err = NULL;
> +Qcow2BitmapInfoList *bitmaps;
> +
> +bitmaps = qcow2_get_bitmap_info_list(bs, _err);
> +if (local_err != NULL) {
> +error_report_err(local_err);
> +}
>   
>   if (s->crypto != NULL) {
>   encrypt_info = qcrypto_block_get_info(s->crypto, _abort);
> @@ -4268,6 +4275,7 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
>   *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>   .compat = g_strdup("0.10"),
>   .refcount_bits  = s->refcount_bits,
> +.bitmaps= bitmaps,
>   };
>   } else if (s->qcow_version == 3) {
>   *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> @@ -4279,6 +4287,8 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
> 

Re: [Qemu-block] [PATCH] qemu-img info lists bitmap directory entries

[Vladimir Sementsov-Ogievskiy]

27.11.2018 15:41, Andrey Shinkevich wrote:
> The 'Format specific information' of qemu-img info command will show
> the name, flags and granularity for every QCOW2 bitmap.
> 
> Signed-off-by: Andrey Shinkevich 
> ---
> Dear colleagues,
> 
> With this patch, qemu-img info will display a name, flags and granularity
> information for every bitmap in the directory section of a QCOW2 image.
> That information appears in the 'Format specific information' section as
> it's shown in the following example:
> 

[...]

> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4254,6 +4254,13 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
>   BDRVQcow2State *s = bs->opaque;
>   ImageInfoSpecific *spec_info;
>   QCryptoBlockInfo *encrypt_info = NULL;
> +Error *local_err = NULL;
> +Qcow2BitmapInfoList *bitmaps;
> +
> +bitmaps = qcow2_get_bitmap_info_list(bs, _err);
> +if (local_err != NULL) {
> +error_report_err(local_err);
> +}
>   
>   if (s->crypto != NULL) {
>   encrypt_info = qcrypto_block_get_info(s->crypto, _abort);
> @@ -4268,6 +4275,7 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
>   *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
>   .compat = g_strdup("0.10"),
>   .refcount_bits  = s->refcount_bits,
> +.bitmaps= bitmaps,

Bitmaps are possible only in version >=3, so this line should be dropped



>   };
>   } else if (s->qcow_version == 3) {
>   *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
> @@ -4279,6 +4287,8 @@ static ImageInfoSpecific 
> *qcow2_get_specific_info(BlockDriverState *bs)
> QCOW2_INCOMPAT_CORRUPT,
>   .has_corrupt= true,
>   .refcount_bits  = s->refcount_bits,
> +.has_bitmaps= bitmaps ? true : false,
> +.bitmaps= bitmaps,
>   };
>   } else {
>   /* if this assertion fails, this probably means a new version was
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 8662b68..0ec2b3d 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -685,6 +685,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
> BdrvCheckResult *res,
> void **refcount_table,
> int64_t *refcount_table_size);
>   bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
> +Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
> +Error **errp);
>   int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
>Error **errp);
>   int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f4538fa..e021ead 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -77,7 +77,8 @@
> '*lazy-refcounts': 'bool',
> '*corrupt': 'bool',
> 'refcount-bits': 'int',
> -  '*encrypt': 'ImageInfoSpecificQCow2Encryption'
> +  '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> +  '*bitmaps': ['Qcow2BitmapInfo']
> } }
>   
>   ##
> @@ -454,6 +455,38 @@
>  'status': 'DirtyBitmapStatus'} }
>   
>   ##
> +# @Qcow2BitmapInfoFlags:
> +#
> +# An enumeration of states that a bitmap can report to the user.
> +#
> +# @in-use: The bitmap was not saved correctly and may be inconsistent.
> +#
> +# @auto: The bitmap must reflect all changes of the virtual disk by any
> +#application that would write to this qcow2 file.
> +#
> +# Since: 3.2

Hm, I heard, the next is 4.0?

> +##
> +{ 'enum': 'Qcow2BitmapInfoFlags',
> +  'data': ['in-use', 'auto'] }
> +
> +##
> +# @Qcow2BitmapInfo:
> +#
> +# Image bitmap information.
> +#
> +# @name: the name of the dirty bitmap
> +#
> +# @granularity: granularity of the dirty bitmap in bytes
> +#
> +# @flags: flags of the dirty bitmap

drop "dirty" word, they are just bitmaps by spec.

> +#
> +# Since: 3.2

and here, 4.0?

> +##
> +{ 'struct': 'Qcow2BitmapInfo',
> +  'data': {'name': 'str', 'granularity': 'uint32',
> +   'flags': ['Qcow2BitmapInfoFlags']} }
> +
> +##
>   # @BlockLatencyHistogramInfo:
>   #
>   # Block latency histogram.
> 


with these small fixes:

Reviewed-by: Vladimir Sementsov-Ogievskiy 


-- 
Best regards,
Vladimir

[Qemu-block] [PATCH] qemu-img info lists bitmap directory entries

[Andrey Shinkevich]

The 'Format specific information' of qemu-img info command will show
the name, flags and granularity for every QCOW2 bitmap.

Signed-off-by: Andrey Shinkevich 
---
Dear colleagues,

With this patch, qemu-img info will display a name, flags and granularity
information for every bitmap in the directory section of a QCOW2 image.
That information appears in the 'Format specific information' section as
it's shown in the following example:

image: /vz/vmprivate/VM1/harddisk.hdd
file format: qcow2
virtual size: 64G (68719476736 bytes)
disk size: 3.0M
cluster_size: 1048576
Format specific information:
compat: 1.1
lazy refcounts: true
refcount bits: 16
dirty bitmaps:
[0]:
flags:
[0]: in-use
[1]: auto
name: {257b5ea5-38c5-410e-a457-71c76f763c60}
granularity: 65536
[1]:
flags:
[0]: in-use
[1]: auto
name: back-up
granularity: 65536
corrupt: false

 block/qcow2-bitmap.c | 50 ++
 block/qcow2.c| 10 ++
 block/qcow2.h|  2 ++
 qapi/block-core.json | 35 ++-
 4 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index accebef..dcc53db 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1008,6 +1008,56 @@ fail:
 return false;
 }
 
+static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
+{
+Qcow2BitmapInfoFlagsList *list = NULL;
+Qcow2BitmapInfoFlagsList **plist = 
+
+if (flags & BME_FLAG_IN_USE) {
+Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
+entry->value = QCOW2_BITMAP_INFO_FLAGS_IN_USE;
+*plist = entry;
+plist = >next;
+}
+if (flags & BME_FLAG_AUTO) {
+Qcow2BitmapInfoFlagsList *entry = g_new0(Qcow2BitmapInfoFlagsList, 1);
+entry->value = QCOW2_BITMAP_INFO_FLAGS_AUTO;
+*plist = entry;
+}
+return list;
+}
+
+Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
+Error **errp)
+{
+BDRVQcow2State *s = bs->opaque;
+Qcow2BitmapList *bm_list;
+Qcow2Bitmap *bm;
+Qcow2BitmapInfoList *list = NULL;
+Qcow2BitmapInfoList **plist = 
+
+bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
+   s->bitmap_directory_size, errp);
+if (bm_list == NULL) {
+return NULL;
+}
+
+QSIMPLEQ_FOREACH(bm, bm_list, entry) {
+Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
+Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
+info->granularity = 1U << bm->granularity_bits;
+info->name = g_strdup(bm->name);
+info->flags = get_bitmap_info_flags(bm->flags);
+obj->value = info;
+*plist = obj;
+plist = >next;
+}
+
+bitmap_list_free(bm_list);
+
+return list;
+}
+
 int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
  Error **errp)
 {
diff --git a/block/qcow2.c b/block/qcow2.c
index 991d6ac..6485f79 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4254,6 +4254,13 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
 BDRVQcow2State *s = bs->opaque;
 ImageInfoSpecific *spec_info;
 QCryptoBlockInfo *encrypt_info = NULL;
+Error *local_err = NULL;
+Qcow2BitmapInfoList *bitmaps;
+
+bitmaps = qcow2_get_bitmap_info_list(bs, _err);
+if (local_err != NULL) {
+error_report_err(local_err);
+}
 
 if (s->crypto != NULL) {
 encrypt_info = qcrypto_block_get_info(s->crypto, _abort);
@@ -4268,6 +4275,7 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
 *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
 .compat = g_strdup("0.10"),
 .refcount_bits  = s->refcount_bits,
+.bitmaps= bitmaps,
 };
 } else if (s->qcow_version == 3) {
 *spec_info->u.qcow2.data = (ImageInfoSpecificQCow2){
@@ -4279,6 +4287,8 @@ static ImageInfoSpecific 
*qcow2_get_specific_info(BlockDriverState *bs)
   QCOW2_INCOMPAT_CORRUPT,
 .has_corrupt= true,
 .refcount_bits  = s->refcount_bits,
+.has_bitmaps= bitmaps ? true : false,
+.bitmaps= bitmaps,
 };
 } else {
 /* if this assertion fails, this probably means a new version was
diff --git a/block/qcow2.h b/block/qcow2.h
index 8662b68..0ec2b3d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -685,6 +685,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, 
BdrvCheckResult *res,
   void **refcount_table,
   int64_t 

[Qemu-block] [RFC PATCH] Introduce Python module structure

[Cleber Rosa]

This is a simple move of Python code that wraps common QEMU
functionality, and are used by a number of different tests
and scripts.

By treating that code as a real Python module, we can more easily,
among other things:
 * reuse more code
 * apply a more consistent style
 * add tests to that code
 * generate documentation

Signed-off-by: Cleber Rosa 
---
 configure  |  1 +
 scripts/qemu.py => python/qemu/__init__.py | 11 ++-
 {scripts/qmp => python/qemu}/qmp.py|  0
 {scripts => python/qemu}/qtest.py  |  5 +++--
 scripts/device-crash-test  |  5 +
 scripts/qmp/__init__.py|  0
 tests/acceptance/avocado_qemu/__init__.py  |  9 +
 tests/migration/guestperf/engine.py| 10 +++---
 tests/qemu-iotests/iotests.py  |  8 ++--
 tests/vm/basevm.py |  9 +++--
 10 files changed, 40 insertions(+), 18 deletions(-)
 rename scripts/qemu.py => python/qemu/__init__.py (98%)
 rename {scripts/qmp => python/qemu}/qmp.py (100%)
 rename {scripts => python/qemu}/qtest.py (97%)
 delete mode 100644 scripts/qmp/__init__.py

diff --git a/configure b/configure
index 0a3c6a72c3..2b64c51009 100755
--- a/configure
+++ b/configure
@@ -7510,6 +7510,7 @@ LINKS="$LINKS pc-bios/qemu-icon.bmp"
 LINKS="$LINKS .gdbinit scripts" # scripts needed by relative path in .gdbinit
 LINKS="$LINKS tests/acceptance tests/data"
 LINKS="$LINKS tests/qemu-iotests/check"
+LINKS="$LINKS python"
 for bios_file in \
 $source_path/pc-bios/*.bin \
 $source_path/pc-bios/*.lid \
diff --git a/scripts/qemu.py b/python/qemu/__init__.py
similarity index 98%
rename from scripts/qemu.py
rename to python/qemu/__init__.py
index 6e3b0e6771..2ae5e1f632 100644
--- a/scripts/qemu.py
+++ b/python/qemu/__init__.py
@@ -16,12 +16,13 @@ import errno
 import logging
 import os
 import subprocess
-import qmp.qmp
 import re
 import shutil
 import socket
 import tempfile
 
+from . import qmp
+
 
 LOG = logging.getLogger(__name__)
 
@@ -58,7 +59,7 @@ class QEMUMachineAddDeviceError(QEMUMachineError):
 failures reported by the QEMU binary itself.
 """
 
-class MonitorResponseError(qmp.qmp.QMPError):
+class MonitorResponseError(qmp.QMPError):
 """
 Represents erroneous QMP monitor reply
 """
@@ -259,8 +260,8 @@ class QEMUMachine(object):
 self._qemu_log_path = os.path.join(self._temp_dir, self._name + ".log")
 self._qemu_log_file = open(self._qemu_log_path, 'wb')
 
-self._qmp = qmp.qmp.QEMUMonitorProtocol(self._vm_monitor,
-server=True)
+self._qmp = qmp.QEMUMonitorProtocol(self._vm_monitor,
+server=True)
 
 def _post_launch(self):
 self._qmp.accept()
@@ -376,7 +377,7 @@ class QEMUMachine(object):
 """
 reply = self.qmp(cmd, conv_keys, **args)
 if reply is None:
-raise qmp.qmp.QMPError("Monitor is closed")
+raise qmp.QMPError("Monitor is closed")
 if "error" in reply:
 raise MonitorResponseError(reply)
 return reply["return"]
diff --git a/scripts/qmp/qmp.py b/python/qemu/qmp.py
similarity index 100%
rename from scripts/qmp/qmp.py
rename to python/qemu/qmp.py
diff --git a/scripts/qtest.py b/python/qemu/qtest.py
similarity index 97%
rename from scripts/qtest.py
rename to python/qemu/qtest.py
index adf1fe3f26..bff79cdd13 100644
--- a/scripts/qtest.py
+++ b/python/qemu/qtest.py
@@ -13,7 +13,8 @@
 
 import socket
 import os
-import qemu
+
+from . import QEMUMachine
 
 
 class QEMUQtestProtocol(object):
@@ -73,7 +74,7 @@ class QEMUQtestProtocol(object):
 self._sock.settimeout(timeout)
 
 
-class QEMUQtestMachine(qemu.QEMUMachine):
+class QEMUQtestMachine(QEMUMachine):
 '''A QEMU VM'''
 
 def __init__(self, binary, args=None, name=None, test_dir="/var/tmp",
diff --git a/scripts/device-crash-test b/scripts/device-crash-test
index e93a7c0c84..c75ae0ecbc 100755
--- a/scripts/device-crash-test
+++ b/scripts/device-crash-test
@@ -35,6 +35,11 @@ import random
 import argparse
 from itertools import chain
 
+THIS_DIR = os.path.dirname(os.path.abspath(__file__))
+TOP_DIR = os.path.dirname(THIS_DIR)
+PYTHON_MODULE_PATH = os.path.join(TOP_DIR, 'python')
+sys.path.append(PYTHON_MODULE_PATH)
+
 from qemu import QEMUMachine
 
 logger = logging.getLogger('device-crash-test')
diff --git a/scripts/qmp/__init__.py b/scripts/qmp/__init__.py
deleted file mode 100644
index e69de29bb2..00
diff --git a/tests/acceptance/avocado_qemu/__init__.py 
b/tests/acceptance/avocado_qemu/__init__.py
index 1e54fd5932..4f3e426ebd 100644
--- a/tests/acceptance/avocado_qemu/__init__.py
+++ b/tests/acceptance/avocado_qemu/__init__.py
@@ -13,9 +13,10 @@ import sys
 
 import avocado
 
-SRC_ROOT_DIR = os.path.dirname(os.path.dirname(os.path.dirname(__file__)))
-SRC_ROOT_DIR = os.path.abspath(os.path.dirname(SRC_ROOT_DIR))