Re: [Qemu-block] [Qemu-devel] [PATCH 1/5] option: Make option help nicer to read

[Marc-André Lureau]

Hi

On Mon, Oct 15, 2018 at 9:34 PM Max Reitz  wrote:
>
> This adds some whitespace into the option help (including indentation)
> and replaces '=' by ': ' (not least because '=' should be used for
> values, not types).

Without strong preference, I like the '=' better: it's the expected
syntax for the command line argument. (also, --help and man pages
describe options with "foo=type", not "foo: type")

> Furthermore, the list name is no longer printed as
> part of every line, but only once in advance, and only if the caller did
> not print a caption already.
>
> Signed-off-by: Max Reitz 

looks good to me otherwise

> ---
>  include/qemu/option.h  |   2 +-
>  qemu-img.c |   4 +-
>  util/qemu-option.c |  31 +-
>  tests/qemu-iotests/082.out | 956 ++---
>  4 files changed, 505 insertions(+), 488 deletions(-)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index 3dfb4493cc..844587cab3 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -132,7 +132,7 @@ typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts 
> *opts, Error **errp);
>  int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
>void *opaque, Error **errp);
>  void qemu_opts_print(QemuOpts *opts, const char *sep);
> -void qemu_opts_print_help(QemuOptsList *list);
> +void qemu_opts_print_help(QemuOptsList *list, bool print_caption);
>  void qemu_opts_free(QemuOptsList *list);
>  QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
>
> diff --git a/qemu-img.c b/qemu-img.c
> index b12f4cd19b..4c96db7ba4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -269,7 +269,7 @@ static int print_block_option_help(const char *filename, 
> const char *fmt)
>  }
>
>  printf("Supported options:\n");
> -qemu_opts_print_help(create_opts);
> +qemu_opts_print_help(create_opts, false);
>  qemu_opts_free(create_opts);
>  return 0;
>  }
> @@ -3773,7 +3773,7 @@ static int print_amend_option_help(const char *format)
>  assert(drv->create_opts);
>
>  printf("Creation options for '%s':\n", format);
> -qemu_opts_print_help(drv->create_opts);
> +qemu_opts_print_help(drv->create_opts, false);
>  printf("\nNote that not all of these options may be amendable.\n");
>  return 0;
>  }
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 9a5f263294..ce79332912 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -224,7 +224,14 @@ static const char *opt_type_to_string(enum QemuOptType 
> type)
>  g_assert_not_reached();
>  }
>
> -void qemu_opts_print_help(QemuOptsList *list)
> +/**
> + * Print the list of options available in the given list.  If
> + * @print_caption is true, a caption (including the list name, if it
> + * exists) is printed.  The options itself will be indented, so
> + * @print_caption should only be set to false if the caller prints its
> + * own custom caption (so that the indentation makes sense).
> + */
> +void qemu_opts_print_help(QemuOptsList *list, bool print_caption)
>  {
>  QemuOptDesc *desc;
>  int i;
> @@ -234,10 +241,7 @@ void qemu_opts_print_help(QemuOptsList *list)
>  desc = list->desc;
>  while (desc && desc->name) {
>  GString *str = g_string_new(NULL);
> -if (list->name) {
> -g_string_append_printf(str, "%s.", list->name);
> -}
> -g_string_append_printf(str, "%s=%s", desc->name,
> +g_string_append_printf(str, "%s: %s", desc->name,
> opt_type_to_string(desc->type));
>  if (desc->help) {
>  g_string_append_printf(str, " - %s", desc->help);
> @@ -247,8 +251,21 @@ void qemu_opts_print_help(QemuOptsList *list)
>  }
>
>  g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
> +if (print_caption && array->len > 0) {
> +if (list->name) {
> +printf("%s options:\n", list->name);
> +} else {
> +printf("Options:\n");
> +}
> +} else if (array->len == 0) {
> +if (list->name) {
> +printf("There are no options for %s.\n", list->name);
> +} else {
> +printf("No options available.\n");
> +}
> +}
>  for (i = 0; i < array->len; i++) {
> -printf("%s\n", (char *)array->pdata[i]);
> +printf("  %s\n", (char *)array->pdata[i]);
>  }
>  g_ptr_array_set_free_func(array, g_free);
>  g_ptr_array_free(array, true);
> @@ -930,7 +947,7 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, 
> const char *params,
>  opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
>  if (err) {
>  if (invalidp && has_help_option(params)) {
> -qemu_opts_print_help(list);
> +qemu_opts_print_help(list, true);
>  error_free(err);
>  } else {
>  error_report_err(err);
> diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iot

[Qemu-block] [RFC PATCH] block: Clean up bdrv_img_create()'s error reporting

[Markus Armbruster]

bdrv_img_create() takes an Error ** argument and used it in the
conventional way, except for one place: when qemu_opts_do_parse()
fails, it first reports its error to stderr or the HMP monitor with
error_report_err(), then error_setg()'s a generic error.  When the
caller reports that second error similarly, this produces two
consecutive error messages on stderr or the HMP monitor.  When the
caller does something else with it, such as send it via QMP, the first
error still goes to stderr or the HMP monitor.  Not good.

Turn the first message into a prefix for the second.

Signed-off-by: Markus Armbruster 
---

This is RFC because I didn't check whether "caller does something else
with it" can actually happen with current code, and I'm not sure the
prefix is wanted.  I hope we'll answer both questions during review.

 block.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 5d51419d21..08aafc20a2 100644
--- a/block.c
+++ b/block.c
@@ -4803,9 +4803,9 @@ void bdrv_img_create(const char *filename, const char 
*fmt,
 if (options) {
 qemu_opts_do_parse(opts, options, NULL, &local_err);
 if (local_err) {
-error_report_err(local_err);
-local_err = NULL;
-error_setg(errp, "Invalid options for file format '%s'", fmt);
+error_propagate_prepend(errp, local_err,
+"Invalid options for file format '%s'",
+fmt);
 goto out;
 }
 }
-- 
2.17.1

Re: [Qemu-block] [Qemu-devel] [RFC] Require Python 3 for building QEMU

[Markus Armbruster]

Eduardo Habkost  writes:

> On Mon, Oct 15, 2018 at 05:55:27PM +0100, Peter Maydell wrote:
>> On 15 October 2018 at 17:33, Markus Armbruster  wrote:
>> > Kevin Wolf  writes:
>> >
>> >> Am 15.10.2018 um 12:02 hat Peter Maydell geschrieben:
>> >> It's easier to port stuff to Python 3 though than making them work with
>> >> both. I think Eduardo's RFC is in part motivated by a patch from
>> >> Philippe that converted something in iotests to work with Python 3,
>> >> passed review and then turned out to break Python 2.
>> >
>> > Seconded.  This is not about the cost of maintaining existing
>> > compatibility gunk, it's about the extra effort to first get the
>> > remainder to work with 2 and 3, only to throw away 2 a few months later.
>> >
>> > I propose we permit ourselves to port stuff that isn't essential to
>> > building QEMU straight to 3 instead.  This includes iotests.
>> 
>> No particular objection, as long as nothing run via 'make'
>> or 'make check' needs Python 3.
>
> Sounds like a good plan for 3.1.
>
> But:
>
>> 
>> I also suspect "a few months" is an underestimate. My guess
>> would be we're going to want to keep Python 2 support for
>> at least the next year, maybe two.
>
> Python 2.7 will die in less than 15 months[1].  I really want us
> to stop reviewing and maintaining Python 2 code in QEMU in less
> than 1 year.  Preferably in less than 6 months.

Seconded.

> [1] https://pythonclock.org/

1 year, 2 months, 15 days, 17 hours, 1 minute, 36 seconds, and counting.

Re: [Qemu-block] [Qemu-devel] [PATCH 7/9] iotests: 'new' module replacement in 169

[Cleber Rosa]

On 10/15/18 7:57 PM, Eduardo Habkost wrote:
> On Mon, Oct 15, 2018 at 07:38:45PM -0400, Cleber Rosa wrote:
>>
>>
>> On 10/15/18 10:14 AM, Max Reitz wrote:
>>> iotest 169 uses the 'new' module to add methods to a class.  This module
>>> no longer exists in Python 3.  Instead, we can use a lambda.  Best of
>>> all, this works in 2.7 just as well.
>>>
>>> Signed-off-by: Max Reitz 
>>> ---
>>>  tests/qemu-iotests/169 | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
>>> index f243db9955..e5614b159d 100755
>>> --- a/tests/qemu-iotests/169
>>> +++ b/tests/qemu-iotests/169
>>> @@ -23,7 +23,6 @@ import iotests
>>>  import time
>>>  import itertools
>>>  import operator
>>> -import new
>>>  from iotests import qemu_img
>>>  
>>>  
>>> @@ -144,7 +143,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>>>  
>>>  def inject_test_case(klass, name, method, *args, **kwargs):
>>>  mc = operator.methodcaller(method, *args, **kwargs)
>>> -setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
>>> +setattr(klass, 'test_' + name, lambda self: mc(self))
>>>  
>>>  for cmb in list(itertools.product((True, False), repeat=4)):
>>>  name = ('_' if cmb[0] else '_not_') + 'persistent_'
>>>
>>
>> This can be simplified with:
>>
>> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
>> index e5614b159d..2199f14ae7 100755
>> --- a/tests/qemu-iotests/169
>> +++ b/tests/qemu-iotests/169
>> @@ -22,7 +22,6 @@ import os
>>  import iotests
>>  import time
>>  import itertools
>> -import operator
>>  from iotests import qemu_img
>>
>>
>> @@ -141,18 +140,15 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>>  self.check_bitmap(self.vm_b, sha256 if persistent else False)
>>
>>
>> -def inject_test_case(klass, name, method, *args, **kwargs):
>> -mc = operator.methodcaller(method, *args, **kwargs)
>> -setattr(klass, 'test_' + name, lambda self: mc(self))
>> -
>>  for cmb in list(itertools.product((True, False), repeat=4)):
>>  name = ('_' if cmb[0] else '_not_') + 'persistent_'
>>  name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
>>  name += '_online' if cmb[2] else '_offline'
>>  name += '_shared' if cmb[3] else '_nonshared'
>>
>> -inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
>> - *list(cmb))
>> +setattr(TestDirtyBitmapMigration, 'test_' + name,
>> +lambda self: TestDirtyBitmapMigration.do_test_migration(
>> +self, *list(cmb)))
> 
> I'm not fond of the long multi-line lambda expression, but I love
> that you got rid of operator.methodcaller().  :)
> 
> However, this doesn't seem to work: `*list(cmb)` will be
> evaluated only when the lambda is called, and `cmb` will be
> always `(False, False, False, False)`.
> 
> I was going to suggest defining a nested function inside
> inject_test_case() to replace operator.methodcaller(), but then I
> thought it was not worth it.
> 

You're right! I missed that.  Anyway, another possibility:

diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index e5614b159d..cd0d9c289c 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -22,7 +22,7 @@ import os
 import iotests
 import time
 import itertools
-import operator
+import functools
 from iotests import qemu_img


@@ -140,20 +140,15 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 self.vm_b.launch()
 self.check_bitmap(self.vm_b, sha256 if persistent else False)

-
-def inject_test_case(klass, name, method, *args, **kwargs):
-mc = operator.methodcaller(method, *args, **kwargs)
-setattr(klass, 'test_' + name, lambda self: mc(self))
-
-for cmb in list(itertools.product((True, False), repeat=4)):
+for cmb in itertools.product((True, False), repeat=4):
 name = ('_' if cmb[0] else '_not_') + 'persistent_'
 name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
 name += '_online' if cmb[2] else '_offline'
 name += '_shared' if cmb[3] else '_nonshared'

-inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
- *list(cmb))
-
+test =
functools.partialmethod(TestDirtyBitmapMigration.do_test_migration,
+   *cmb)
+setattr(TestDirtyBitmapMigration, 'test_' + name, test)

 if __name__ == '__main__':
 iotests.main(supported_fmts=['qcow2'])

Re: [Qemu-block] [PATCH 8/9] iotests: Modify imports for Python 3

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 08:05:02PM -0400, Cleber Rosa wrote:
> 
> 
> On 10/15/18 5:17 PM, Eduardo Habkost wrote:
> > On Mon, Oct 15, 2018 at 04:14:52PM +0200, Max Reitz wrote:
> >> There are two imports that need to be modified when running the iotests
> >> under Python 3: One is StringIO, which no longer exists; instead, the
> >> StringIO class comes from the io module, so import it from there.  The
> >> other is the ConfigParser, which has just been renamed to configparser.
> >>
> >> Signed-off-by: Max Reitz 
> >> ---
> >>  tests/qemu-iotests/iotests.py| 8 ++--
> >>  tests/qemu-iotests/nbd-fault-injector.py | 7 +--
> >>  2 files changed, 11 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >> index 7ca94e9278..a64ea90fb4 100644
> >> --- a/tests/qemu-iotests/iotests.py
> >> +++ b/tests/qemu-iotests/iotests.py
> >> @@ -683,13 +683,17 @@ def main(supported_fmts=[], 
> >> supported_oses=['linux'], supported_cache_modes=[],
> >>  
> >>  # We need to filter out the time taken from the output so that 
> >> qemu-iotest
> >>  # can reliably diff the results against master output.
> >> -import StringIO
> >> +if sys.version_info.major >= 3:
> >> +from io import StringIO
> >> +else:
> >> +from StringIO import StringIO
> > 
> > Considering that io.StringIO exists on Python 2.7, a comment
> > explaining why exactly it doesn't work would be nice.
> > 
> 
> Another possibility, that I find self explanatory:
> 
> import io
> 
> if sys.version_info.major >= 3:
>output = io.StringIO()
> else:
>output = io.BytesIO()

Looks nice and clean.

But I'm not sure all output sent to `output` when running with
Python 2 will be byte strings.  What if `unittest.TextTestRunner`
tries to write unicode strings to `output`?

-- 
Eduardo

Re: [Qemu-block] [PATCH 8/9] iotests: Modify imports for Python 3

[Cleber Rosa]

On 10/15/18 5:17 PM, Eduardo Habkost wrote:
> On Mon, Oct 15, 2018 at 04:14:52PM +0200, Max Reitz wrote:
>> There are two imports that need to be modified when running the iotests
>> under Python 3: One is StringIO, which no longer exists; instead, the
>> StringIO class comes from the io module, so import it from there.  The
>> other is the ConfigParser, which has just been renamed to configparser.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/iotests.py| 8 ++--
>>  tests/qemu-iotests/nbd-fault-injector.py | 7 +--
>>  2 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 7ca94e9278..a64ea90fb4 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], 
>> supported_cache_modes=[],
>>  
>>  # We need to filter out the time taken from the output so that 
>> qemu-iotest
>>  # can reliably diff the results against master output.
>> -import StringIO
>> +if sys.version_info.major >= 3:
>> +from io import StringIO
>> +else:
>> +from StringIO import StringIO
> 
> Considering that io.StringIO exists on Python 2.7, a comment
> explaining why exactly it doesn't work would be nice.
> 

Another possibility, that I find self explanatory:

import io

if sys.version_info.major >= 3:
   output = io.StringIO()
else:
   output = io.BytesIO()

- Cleber.

> But this shouldn't block this workaround, so:
> 
> Reviewed-by: Eduardo Habkost 
> 
> 
>> +
>>  if debug:
>>  output = sys.stdout
>>  verbosity = 2
>>  sys.argv.remove('-d')
>>  else:
>> -output = StringIO.StringIO()
>> +output = StringIO()
>>  
>>  logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
>>  
>> diff --git a/tests/qemu-iotests/nbd-fault-injector.py 
>> b/tests/qemu-iotests/nbd-fault-injector.py
>> index d45e2e0a6a..6b2d659dee 100755
>> --- a/tests/qemu-iotests/nbd-fault-injector.py
>> +++ b/tests/qemu-iotests/nbd-fault-injector.py
>> @@ -48,7 +48,10 @@ import sys
>>  import socket
>>  import struct
>>  import collections
>> -import ConfigParser
>> +if sys.version_info.major >= 3:
>> +import configparser
>> +else:
>> +import ConfigParser as configparser
>>  
>>  FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB
>>  
>> @@ -225,7 +228,7 @@ def parse_config(config):
>>  return rules
>>  
>>  def load_rules(filename):
>> -config = ConfigParser.RawConfigParser()
>> +config = configparser.RawConfigParser()
>>  with open(filename, 'rt') as f:
>>  config.readfp(f, filename)
>>  return parse_config(config)
>> -- 
>> 2.17.1
>>
> 

Re: [Qemu-block] [PATCH 7/9] iotests: 'new' module replacement in 169

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 07:38:45PM -0400, Cleber Rosa wrote:
> 
> 
> On 10/15/18 10:14 AM, Max Reitz wrote:
> > iotest 169 uses the 'new' module to add methods to a class.  This module
> > no longer exists in Python 3.  Instead, we can use a lambda.  Best of
> > all, this works in 2.7 just as well.
> > 
> > Signed-off-by: Max Reitz 
> > ---
> >  tests/qemu-iotests/169 | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
> > index f243db9955..e5614b159d 100755
> > --- a/tests/qemu-iotests/169
> > +++ b/tests/qemu-iotests/169
> > @@ -23,7 +23,6 @@ import iotests
> >  import time
> >  import itertools
> >  import operator
> > -import new
> >  from iotests import qemu_img
> >  
> >  
> > @@ -144,7 +143,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
> >  
> >  def inject_test_case(klass, name, method, *args, **kwargs):
> >  mc = operator.methodcaller(method, *args, **kwargs)
> > -setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
> > +setattr(klass, 'test_' + name, lambda self: mc(self))
> >  
> >  for cmb in list(itertools.product((True, False), repeat=4)):
> >  name = ('_' if cmb[0] else '_not_') + 'persistent_'
> > 
> 
> This can be simplified with:
> 
> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
> index e5614b159d..2199f14ae7 100755
> --- a/tests/qemu-iotests/169
> +++ b/tests/qemu-iotests/169
> @@ -22,7 +22,6 @@ import os
>  import iotests
>  import time
>  import itertools
> -import operator
>  from iotests import qemu_img
> 
> 
> @@ -141,18 +140,15 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>  self.check_bitmap(self.vm_b, sha256 if persistent else False)
> 
> 
> -def inject_test_case(klass, name, method, *args, **kwargs):
> -mc = operator.methodcaller(method, *args, **kwargs)
> -setattr(klass, 'test_' + name, lambda self: mc(self))
> -
>  for cmb in list(itertools.product((True, False), repeat=4)):
>  name = ('_' if cmb[0] else '_not_') + 'persistent_'
>  name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
>  name += '_online' if cmb[2] else '_offline'
>  name += '_shared' if cmb[3] else '_nonshared'
> 
> -inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
> - *list(cmb))
> +setattr(TestDirtyBitmapMigration, 'test_' + name,
> +lambda self: TestDirtyBitmapMigration.do_test_migration(
> +self, *list(cmb)))

I'm not fond of the long multi-line lambda expression, but I love
that you got rid of operator.methodcaller().  :)

However, this doesn't seem to work: `*list(cmb)` will be
evaluated only when the lambda is called, and `cmb` will be
always `(False, False, False, False)`.

I was going to suggest defining a nested function inside
inject_test_case() to replace operator.methodcaller(), but then I
thought it was not worth it.

-- 
Eduardo

Re: [Qemu-block] [PATCH 7/9] iotests: 'new' module replacement in 169

[Cleber Rosa]

On 10/15/18 10:14 AM, Max Reitz wrote:
> iotest 169 uses the 'new' module to add methods to a class.  This module
> no longer exists in Python 3.  Instead, we can use a lambda.  Best of
> all, this works in 2.7 just as well.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/169 | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
> index f243db9955..e5614b159d 100755
> --- a/tests/qemu-iotests/169
> +++ b/tests/qemu-iotests/169
> @@ -23,7 +23,6 @@ import iotests
>  import time
>  import itertools
>  import operator
> -import new
>  from iotests import qemu_img
>  
>  
> @@ -144,7 +143,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>  
>  def inject_test_case(klass, name, method, *args, **kwargs):
>  mc = operator.methodcaller(method, *args, **kwargs)
> -setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
> +setattr(klass, 'test_' + name, lambda self: mc(self))
>  
>  for cmb in list(itertools.product((True, False), repeat=4)):
>  name = ('_' if cmb[0] else '_not_') + 'persistent_'
> 

This can be simplified with:

diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index e5614b159d..2199f14ae7 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -22,7 +22,6 @@ import os
 import iotests
 import time
 import itertools
-import operator
 from iotests import qemu_img


@@ -141,18 +140,15 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 self.check_bitmap(self.vm_b, sha256 if persistent else False)


-def inject_test_case(klass, name, method, *args, **kwargs):
-mc = operator.methodcaller(method, *args, **kwargs)
-setattr(klass, 'test_' + name, lambda self: mc(self))
-
 for cmb in list(itertools.product((True, False), repeat=4)):
 name = ('_' if cmb[0] else '_not_') + 'persistent_'
 name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
 name += '_online' if cmb[2] else '_offline'
 name += '_shared' if cmb[3] else '_nonshared'

-inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
- *list(cmb))
+setattr(TestDirtyBitmapMigration, 'test_' + name,
+lambda self: TestDirtyBitmapMigration.do_test_migration(
+self, *list(cmb)))


 if __name__ == '__main__':

Re: [Qemu-block] [Qemu-devel] [PATCH 6/9] iotests: Explicitly inherit FDs in Python

[Cleber Rosa]

On 10/15/18 10:14 AM, Max Reitz wrote:
> Python 3.2 introduced the inheritable attribute for FDs.  At the same
> time, it changed the default so that all FDs are not inheritable by
> default, that only inheritable FDs are inherited to subprocesses, and
> only if close_fds is explicitly set to False.
> 

It's actually a change that was introduced in 3.4:

https://docs.python.org/3/library/os.html#inheritance-of-file-descriptors

> Adhere to this by setting close_fds to False when working with
> subprocesses that may want to inherit FDs, and by trying to
> set_inheritable() on FDs that we do want to bequeath to them.
> 
> Signed-off-by: Max Reitz 
> ---
>  scripts/qemu.py| 13 +++--
>  scripts/qmp/qmp.py |  7 +++
>  tests/qemu-iotests/147 |  7 +++
>  3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index f099ce7278..28366c4a67 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -142,10 +142,18 @@ class QEMUMachine(object):
>  if opts:
>  options.append(opts)
>  
> +# This did not exist before 3.2, but since then it is
> +# mandatory for our purpose
> +try:

Version should be 3.4 here as well.

> +os.set_inheritable(fd, True)
> +except AttributeError:
> +pass
> +

Doing hasattr(os, "set_inheritable") is cheaper.

- Cleber.

>  self._args.append('-add-fd')
>  self._args.append(','.join(options))
>  return self
>  
> +# The caller needs to make sure the FD is inheritable
>  def send_fd_scm(self, fd_file_path):
>  # In iotest.py, the qmp should always use unix socket.
>  assert self._qmp.is_scm_available()
> @@ -159,7 +167,7 @@ class QEMUMachine(object):
>  "%s" % fd_file_path]
>  devnull = open(os.path.devnull, 'rb')
>  proc = subprocess.Popen(fd_param, stdin=devnull, 
> stdout=subprocess.PIPE,
> -stderr=subprocess.STDOUT)
> +stderr=subprocess.STDOUT, close_fds=False)
>  output = proc.communicate()[0]
>  if output:
>  LOG.debug(output)
> @@ -280,7 +288,8 @@ class QEMUMachine(object):
> stdin=devnull,
> stdout=self._qemu_log_file,
> stderr=subprocess.STDOUT,
> -   shell=False)
> +   shell=False,
> +   close_fds=False)
>  self._post_launch()
>  
>  def wait(self):
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> index 5c8cf6a056..009be8345b 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -10,6 +10,7 @@
>  
>  import json
>  import errno
> +import os
>  import socket
>  import logging
>  
> @@ -253,4 +254,10 @@ class QEMUMonitorProtocol(object):
>  return self.__sock.fileno()
>  
>  def is_scm_available(self):
> +# This did not exist before 3.2, but since then it is
> +# mandatory for our purpose
> +try:
> +os.set_inheritable(self.get_sock_fd(), True)
> +except AttributeError:
> +pass
>  return self.__sock.family == socket.AF_UNIX
> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
> index d2081df84b..b58455645b 100755
> --- a/tests/qemu-iotests/147
> +++ b/tests/qemu-iotests/147
> @@ -229,6 +229,13 @@ class BuiltinNBD(NBDBlockdevAddBase):
>  sockfd = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
>  sockfd.connect(unix_socket)
>  
> +# This did not exist before 3.2, but since then it is
> +# mandatory for our purpose
> +try:
> +os.set_inheritable(sockfd.fileno(), True)
> +except AttributeError:
> +pass
> +
>  result = self.vm.send_fd_scm(str(sockfd.fileno()))
>  self.assertEqual(result, 0, 'Failed to send socket FD')
>  
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 5/9] iotests: Different iterator behavior in Python 3

[Cleber Rosa]

On 10/15/18 10:14 AM, Max Reitz wrote:
> In Python 3, several functions now return iterators instead of lists.
> This includes range(), items(), map(), and filter().  This means that if
> we really want a list, we have to wrap those instances with list().  On
> the other hand, sometimes we do just want an iterator, in which case we
> have sometimes used xrange() and iteritems() which no longer exist in
> Python 3.  Just change these calls to be range() and items(), which
> costs a bit of performance in Python 2, but will do the right thing in
> Python 3 (which is what is important).
> 
> In one instance, we only wanted the first instance of the result of a
> filter() call.  Instead of using next(filter()) which would work only in
> Python 3, or list(filter())[0] which would work everywhere but is a bit
> weird, this instance is changed to a single-line for with next() wrapped
> around, which works both in 2.7 and 3.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/044 | 12 ++--
>  tests/qemu-iotests/056 |  2 +-
>  tests/qemu-iotests/065 |  4 ++--
>  tests/qemu-iotests/124 |  4 ++--
>  tests/qemu-iotests/139 |  2 +-
>  tests/qemu-iotests/163 |  6 +++---
>  6 files changed, 15 insertions(+), 15 deletions(-)
> 

You have 2 files here which use xrange (which is a manageable size, and
whose occurrences involve a moderate size of items) to also consider:

if sys.version_info.major == 2:
   range = xrange

Defaulting to the Python 3 names, but behaving the same across Python 2
and 3.

To do the same for dict.iteritems() => dict.items() requires a lot more
code, so I'd stay away from it for now.  Also, it looks like most of
those dicts are small in size (**kwargs and the like).

Other than that suggestion,

Reviewed-by: Cleber Rosa 

Re: [Qemu-block] [Qemu-devel] [PATCH 5/5] fw_cfg: Drop newline in @file description

[Philippe Mathieu-Daudé]

On 15/10/2018 19:28, Max Reitz wrote:
> There is no good reason why there should be a newline in this
> description, so remove it.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index f9ed053eab..19f8cc67ff 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -529,7 +529,7 @@ static QemuOptsList qemu_fw_cfg_opts = {
>  }, {
>  .name = "file",
>  .type = QEMU_OPT_STRING,
> -.help = "Sets the name of the file from which\n"
> +.help = "Sets the name of the file from which "
>  "the fw_cfg blob will be loaded",
>  }, {
>  .name = "string",
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 9/9] iotests: Unify log outputs between Python 2 and 3

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 04:14:53PM +0200, Max Reitz wrote:
> When dumping an object into the log, there are differences between
> Python 2 and 3.  First, unicode strings are prefixed by 'u' in Python 2
> (they are no longer in 3, because unicode strings are the default
> there).  [...]

This could be addressed using JSON.  See below[1].

> [...] Second, the order of keys in dicts may differ.  [...]

Can be addressed using json.dumps(..., sort_keys=True).

> [...] Third,
> especially long numbers are longs in Python 2 and thus get an 'L'
> suffix, which does not happen in Python 3.

print() doesn't add a L suffix on Python 2.7 on my system:

Python 2.7.15 (default, May 15 2018, 15:37:31)
[GCC 7.3.1 20180303 (Red Hat 7.3.1-5)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> print(999L)
999

So I assume this happens only for QMP commands.  It would be
addressed if using JSON, too.


> 
> To get around these differences, this patch introduces functions to
> convert an object to a string that looks the same regardless of the
> Python version: In Python 2, they decode unicode strings to byte strings
> (normal str); in Python 3, they encode byte strings to unicode strings
> (normal str).  They also manually convert lists and dicts to strings,
> which allows sorting the dicts by key, so we are no longer at the mercy
> of the internal implementation when it comes to how the keys appear in
> the output.
> 
> This changes the output of all tests that use these logging functions.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/194.out|  22 +-
>  tests/qemu-iotests/202.out|  12 +-
>  tests/qemu-iotests/203.out|  14 +-
>  tests/qemu-iotests/206.out| 144 +-
>  tests/qemu-iotests/207.out|  52 ++--
>  tests/qemu-iotests/208.out|   8 +-
>  tests/qemu-iotests/210.out|  72 ++---
>  tests/qemu-iotests/211.out|  66 ++---
>  tests/qemu-iotests/212.out| 102 +++
>  tests/qemu-iotests/213.out| 124 
>  tests/qemu-iotests/216.out|   4 +-
>  tests/qemu-iotests/218.out|  20 +-
>  tests/qemu-iotests/219.out| 526 +-
>  tests/qemu-iotests/222.out|  24 +-
>  tests/qemu-iotests/iotests.py |  42 ++-
>  15 files changed, 634 insertions(+), 598 deletions(-)
[...]
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index a64ea90fb4..f8dbc8cc71 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -250,10 +250,45 @@ def filter_img_info(output, filename):
>  lines.append(line)
>  return '\n'.join(lines)
>  
> +def log_to_string_repr(obj):
> +# Normal Python 3 strings are the unicode strings from Python 2;
> +# and normal Python 2 strings are byte strings in Python 3.  Thus,
> +# we convert bytes -> str in py3 and unicode -> str in py2.
> +if sys.version_info.major >= 3:
> +if type(obj) is bytes:
> +return repr(obj.decode('utf-8'))
> +else:
> +if type(obj) is unicode:
> +return repr(obj.encode('utf-8'))
> +elif type(obj) is long:
> +return str(obj) # repr() would include an 'L' suffix
> +
> +if type(obj) is list:
> +return '[' + ', '.join(map(log_to_string_repr, obj)) + ']'
> +elif type(obj) is dict:
> +return '{' + ', '.join(map(lambda k: log_to_string_repr(k) + ': ' +
> + log_to_string_repr(obj[k]),
> +   sorted(obj.keys( + '}'
> +else:
> +return repr(obj)

I assume this function exists only because of QMP logging,
correct?

I would just use json.dumps() at qmp_log(), see below[1].


> +
> +def log_to_string(obj):
> +if type(obj) is str:
> +return obj
> +
> +if sys.version_info.major >= 3:
> +if type(obj) is bytes:
> +return obj.decode('utf-8')

Do you know how many of existing log() calls actually pass a byte
string as argument?

> +else:
> +if type(obj) is unicode:
> +return obj.encode('utf-8')

Is this really necessary?  The existing code just calls
print(msg) and it works, doesn't it?



> +
> +return log_to_string_repr(obj)
> +
>  def log(msg, filters=[]):
>  for flt in filters:
>  msg = flt(msg)
> -print(msg)
> +print(log_to_string(msg))
>  
>  class Timeout:
>  def __init__(self, seconds, errmsg = "Timeout"):
> @@ -441,10 +476,11 @@ class VM(qtest.QEMUQtestMachine):
>  return result
>  
>  def qmp_log(self, cmd, filters=[filter_testfiles], **kwargs):
> -logmsg = "{'execute': '%s', 'arguments': %s}" % (cmd, kwargs)
> +logmsg = "{'execute': '%s', 'arguments': %s}" % \
> +(cmd, log_to_string(kwargs))
>  log(logmsg, filters)
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 0/9] iotests: Make them work for both Python 2 and 3

[Philippe Mathieu-Daudé]

Hi Max,

On 15/10/2018 16:14, Max Reitz wrote:
> This series prepares the iotests to work with both Python 2 and 3.  In
> some places, it adds version-specific code and decides what to do based
> on the version (for instance, whether to import the StringIO class from
> the 'io' or the 'StringIO' module), but most of the time, it just makes
> code work for both versions in general.
> 
> And when we make the switch to make Python 3 mandatory, we can simply
> drop the Python 2 branches.
> 
> 
> Max Reitz (9):
>   iotests: Make nbd-fault-injector flush
>   iotests: Flush in iotests.py's QemuIoInteractive
>   iotests: Use Python byte strings where appropriate
>   iotests: Use // for Python integer division
>   iotests: Different iterator behavior in Python 3
>   iotests: Explicitly inherit FDs in Python
>   iotests: 'new' module replacement in 169
>   iotests: Modify imports for Python 3
>   iotests: Unify log outputs between Python 2 and 3

You forgot:

MAINTAINERS: Add myself in the Python scripts section

;)

> 
>  scripts/qemu.py  |  13 +-
>  scripts/qmp/qmp.py   |   7 +
>  scripts/qtest.py |   2 +-
>  tests/qemu-iotests/040   |   4 +-
>  tests/qemu-iotests/044   |  20 +-
>  tests/qemu-iotests/056   |   2 +-
>  tests/qemu-iotests/065   |   4 +-
>  tests/qemu-iotests/083.out   |   9 +
>  tests/qemu-iotests/093   |  18 +-
>  tests/qemu-iotests/124   |   4 +-
>  tests/qemu-iotests/139   |   2 +-
>  tests/qemu-iotests/147   |   7 +
>  tests/qemu-iotests/149   |  14 +-
>  tests/qemu-iotests/151   |  12 +-
>  tests/qemu-iotests/163   |   8 +-
>  tests/qemu-iotests/169   |   3 +-
>  tests/qemu-iotests/194.out   |  22 +-
>  tests/qemu-iotests/202.out   |  12 +-
>  tests/qemu-iotests/203.out   |  14 +-
>  tests/qemu-iotests/206.out   | 144 +++
>  tests/qemu-iotests/207   |   4 +-
>  tests/qemu-iotests/207.out   |  52 +--
>  tests/qemu-iotests/208.out   |   8 +-
>  tests/qemu-iotests/210.out   |  72 ++--
>  tests/qemu-iotests/211.out   |  66 +--
>  tests/qemu-iotests/212.out   | 102 ++---
>  tests/qemu-iotests/213.out   | 124 +++---
>  tests/qemu-iotests/216.out   |   4 +-
>  tests/qemu-iotests/218.out   |  20 +-
>  tests/qemu-iotests/219.out   | 526 +++
>  tests/qemu-iotests/222.out   |  24 +-
>  tests/qemu-iotests/iotests.py|  64 ++-
>  tests/qemu-iotests/nbd-fault-injector.py |  12 +-
>  tests/qemu-iotests/qcow2.py  |  10 +-
>  34 files changed, 745 insertions(+), 664 deletions(-)
> 

Re: [Qemu-block] [Qemu-devel] [PATCH 3/9] iotests: Use Python byte strings where appropriate

[Philippe Mathieu-Daudé]

On 15/10/2018 16:14, Max Reitz wrote:
> Since byte strings are no longer the default in Python 3, we have to
> explicitly use them where we need to, which is mostly when working with
> structures.  It also means that we need to open a file in binary mode
> when we want to use structures.
> 
> On the other hand, we have to accomodate for the fact that some
> functions (still) work with byte strings but we want to use unicode
> strings (in Python 3 at least, and it does not matter in Python 2).
> This includes base64 encoding, but it is most notable when working with
> the subprocess module: Either we set univeral_newlines to True so that

'universal_newlines'

> the default streams are opened in text mode (hence this parameter is
> aliased as "text" as of 3.7), or, if that is not possible, we have to
> decode the output to a normal string.
> 
> Signed-off-by: Max Reitz 
> ---
>  scripts/qtest.py |  2 +-
>  tests/qemu-iotests/044   |  8 
>  tests/qemu-iotests/149   |  8 +---
>  tests/qemu-iotests/207   |  4 ++--
>  tests/qemu-iotests/iotests.py| 11 +++
>  tests/qemu-iotests/nbd-fault-injector.py |  4 ++--
>  tests/qemu-iotests/qcow2.py  | 10 +-
>  7 files changed, 26 insertions(+), 21 deletions(-)
> 
> diff --git a/scripts/qtest.py b/scripts/qtest.py
> index df0daf26ca..adf1fe3f26 100644
> --- a/scripts/qtest.py
> +++ b/scripts/qtest.py
> @@ -64,7 +64,7 @@ class QEMUQtestProtocol(object):
>  
>  @param qtest_cmd: qtest command text to be sent
>  """
> -self._sock.sendall(qtest_cmd + "\n")
> +self._sock.sendall((qtest_cmd + "\n").encode('utf-8'))
>  
>  def close(self):
>  self._sock.close()
> diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
> index 11ea0f4d35..69e736f687 100755
> --- a/tests/qemu-iotests/044
> +++ b/tests/qemu-iotests/044
> @@ -53,21 +53,21 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
>  fd.seek(off_reftable)
>  
>  for i in xrange(0, h.refcount_table_clusters):
> -sector = ''.join(struct.pack('>Q',
> +sector = b''.join(struct.pack('>Q',
>  off_refblock + i * 64 * 512 + j * 512)
>  for j in xrange(0, 64))
>  fd.write(sector)
>  
>  # Write the refcount blocks
>  assert(fd.tell() == off_refblock)
> -sector = ''.join(struct.pack('>H', 1) for j in xrange(0, 64 * 
> 256))
> +sector = b''.join(struct.pack('>H', 1) for j in range(0, 64 * 
> 256))
>  for block in xrange(0, h.refcount_table_clusters):
>  fd.write(sector)
>  
>  # Write the L1 table
>  assert(fd.tell() == off_l1)
>  assert(off_l2 + 512 * h.l1_size == off_data)
> -table = ''.join(struct.pack('>Q', (1 << 63) | off_l2 + 512 * j)
> +table = b''.join(struct.pack('>Q', (1 << 63) | off_l2 + 512 * j)
>  for j in xrange(0, h.l1_size))
>  fd.write(table)
>  
> @@ -85,7 +85,7 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
>  remaining = remaining - 1024 * 512
>  off = off + 1024 * 512
>  
> -table = ''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
> +table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
>  for j in xrange(0, remaining / 512))
>  fd.write(table)
>  
> diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
> index 9e0cad76f9..1225334cb8 100755
> --- a/tests/qemu-iotests/149
> +++ b/tests/qemu-iotests/149
> @@ -79,7 +79,7 @@ class LUKSConfig(object):
>  
>  def first_password_base64(self):
>  (pw, slot) = self.first_password()
> -return base64.b64encode(pw)
> +return base64.b64encode(pw.encode('ascii')).decode('ascii')
>  
>  def active_slots(self):
>  slots = []
> @@ -98,7 +98,8 @@ def verify_passwordless_sudo():
>  proc = subprocess.Popen(args,
>  stdin=subprocess.PIPE,
>  stdout=subprocess.PIPE,
> -stderr=subprocess.STDOUT)
> +stderr=subprocess.STDOUT,
> +universal_newlines=True)
>  
>  msg = proc.communicate()[0]
>  
> @@ -116,7 +117,8 @@ def cryptsetup(args, password=None):
>  proc = subprocess.Popen(fullargs,
>  stdin=subprocess.PIPE,
>  stdout=subprocess.PIPE,
> -stderr=subprocess.STDOUT)
> +stderr=subprocess.STDOUT,
> +universal_newlines=True)
>  
>  msg = proc.communicate(password)[0]
>  
> diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
> index 444ae233ae..2d86a3da37 100755
> --- a/tests/qemu-iotests/207
> +++ b/t

Re: [Qemu-block] [PATCH 8/9] iotests: Modify imports for Python 3

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 04:14:52PM +0200, Max Reitz wrote:
> There are two imports that need to be modified when running the iotests
> under Python 3: One is StringIO, which no longer exists; instead, the
> StringIO class comes from the io module, so import it from there.  The
> other is the ConfigParser, which has just been renamed to configparser.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/iotests.py| 8 ++--
>  tests/qemu-iotests/nbd-fault-injector.py | 7 +--
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7ca94e9278..a64ea90fb4 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], 
> supported_cache_modes=[],
>  
>  # We need to filter out the time taken from the output so that 
> qemu-iotest
>  # can reliably diff the results against master output.
> -import StringIO
> +if sys.version_info.major >= 3:
> +from io import StringIO
> +else:
> +from StringIO import StringIO

Considering that io.StringIO exists on Python 2.7, a comment
explaining why exactly it doesn't work would be nice.

But this shouldn't block this workaround, so:

Reviewed-by: Eduardo Habkost 


> +
>  if debug:
>  output = sys.stdout
>  verbosity = 2
>  sys.argv.remove('-d')
>  else:
> -output = StringIO.StringIO()
> +output = StringIO()
>  
>  logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
>  
> diff --git a/tests/qemu-iotests/nbd-fault-injector.py 
> b/tests/qemu-iotests/nbd-fault-injector.py
> index d45e2e0a6a..6b2d659dee 100755
> --- a/tests/qemu-iotests/nbd-fault-injector.py
> +++ b/tests/qemu-iotests/nbd-fault-injector.py
> @@ -48,7 +48,10 @@ import sys
>  import socket
>  import struct
>  import collections
> -import ConfigParser
> +if sys.version_info.major >= 3:
> +import configparser
> +else:
> +import ConfigParser as configparser
>  
>  FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB
>  
> @@ -225,7 +228,7 @@ def parse_config(config):
>  return rules
>  
>  def load_rules(filename):
> -config = ConfigParser.RawConfigParser()
> +config = configparser.RawConfigParser()
>  with open(filename, 'rt') as f:
>  config.readfp(f, filename)
>  return parse_config(config)
> -- 
> 2.17.1
> 

-- 
Eduardo

Re: [Qemu-block] [PATCH 7/9] iotests: 'new' module replacement in 169

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 04:14:51PM +0200, Max Reitz wrote:
> iotest 169 uses the 'new' module to add methods to a class.  This module
> no longer exists in Python 3.  Instead, we can use a lambda.  Best of
> all, this works in 2.7 just as well.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/169 | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
> index f243db9955..e5614b159d 100755
> --- a/tests/qemu-iotests/169
> +++ b/tests/qemu-iotests/169
> @@ -23,7 +23,6 @@ import iotests
>  import time
>  import itertools
>  import operator
> -import new
>  from iotests import qemu_img
>  
>  
> @@ -144,7 +143,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
>  
>  def inject_test_case(klass, name, method, *args, **kwargs):
>  mc = operator.methodcaller(method, *args, **kwargs)
> -setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
> +setattr(klass, 'test_' + name, lambda self: mc(self))

The "lambda self: mc(self)" expression looks weird and
unnecessary at first look, but I have just confirmed that this
doesn't work:

setattr(klass, 'test_' + name, mc)

Probably because the methodcaller object won't automatically
become a instance method after the TestCase class is
instantiated.

Reviewed-by: Eduardo Habkost 

>  
>  for cmb in list(itertools.product((True, False), repeat=4)):
>  name = ('_' if cmb[0] else '_not_') + 'persistent_'
> -- 
> 2.17.1
> 

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH 4/9] iotests: Use // for Python integer division

[Cleber Rosa]

On 10/15/18 10:14 AM, Max Reitz wrote:
> In Python 3, / is always a floating-point division.  We usually do not
> want this, and as Python 2.7 understands // as well, change all integer
> divisions to use that.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/040|  4 ++--
>  tests/qemu-iotests/044|  2 +-
>  tests/qemu-iotests/093| 18 +-
>  tests/qemu-iotests/149|  6 +++---
>  tests/qemu-iotests/151| 12 ++--
>  tests/qemu-iotests/163|  2 +-
>  tests/qemu-iotests/iotests.py |  2 +-
>  7 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
> index 1cb1ceeb33..b81133a474 100755
> --- a/tests/qemu-iotests/040
> +++ b/tests/qemu-iotests/040
> @@ -195,7 +195,7 @@ class TestSingleDrive(ImageCommitTestCase):
>  
>  self.assert_no_active_block_jobs()
>  result = self.vm.qmp('block-commit', device='drive0', top=mid_img,
> - base=backing_img, speed=(self.image_len / 4))
> + base=backing_img, speed=(self.image_len // 4))
>  self.assert_qmp(result, 'return', {})
>  result = self.vm.qmp('device_del', id='scsi0')
>  self.assert_qmp(result, 'return', {})
> @@ -225,7 +225,7 @@ class TestSingleDrive(ImageCommitTestCase):
>  
>  self.assert_no_active_block_jobs()
>  result = self.vm.qmp('block-commit', device='drive0', top=mid_img,
> - base=backing_img, speed=(self.image_len / 4))
> + base=backing_img, speed=(self.image_len // 4))
>  self.assert_qmp(result, 'return', {})
>  
>  result = self.vm.qmp('query-block')
> diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
> index 69e736f687..7ef5e46fe9 100755
> --- a/tests/qemu-iotests/044
> +++ b/tests/qemu-iotests/044
> @@ -86,7 +86,7 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
>  off = off + 1024 * 512
>  
>  table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
> -for j in xrange(0, remaining / 512))
> +for j in xrange(0, remaining // 512))
>  fd.write(table)
>  
>  
> diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
> index 9d1971a56c..d88fbc182e 100755
> --- a/tests/qemu-iotests/093
> +++ b/tests/qemu-iotests/093
> @@ -69,18 +69,18 @@ class ThrottleTestCase(iotests.QMPTestCase):
>  # in. The throttled requests won't be executed until we
>  # advance the virtual clock.
>  rq_size = 512
> -rd_nr = max(params['bps'] / rq_size / 2,
> -params['bps_rd'] / rq_size,
> -params['iops'] / 2,
> +rd_nr = max(params['bps'] // rq_size // 2,
> +params['bps_rd'] // rq_size,
> +params['iops'] // 2,
>  params['iops_rd'])
>  rd_nr *= seconds * 2
> -rd_nr /= ndrives
> -wr_nr = max(params['bps'] / rq_size / 2,
> -params['bps_wr'] / rq_size,
> -params['iops'] / 2,
> +rd_nr //= ndrives
> +wr_nr = max(params['bps'] // rq_size // 2,
> +params['bps_wr'] // rq_size,
> +params['iops'] // 2,
>  params['iops_wr'])
>  wr_nr *= seconds * 2
> -wr_nr /= ndrives
> +wr_nr //= ndrives
>  
>  # Send I/O requests to all drives
>  for i in range(rd_nr):
> @@ -196,7 +196,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
>  self.configure_throttle(ndrives, settings)
>  
>  # Wait for the bucket to empty so we can do bursts
> -wait_ns = nsec_per_sec * burst_length * burst_rate / rate
> +wait_ns = nsec_per_sec * burst_length * burst_rate // rate
>  self.vm.qtest("clock_step %d" % wait_ns)
>  
>  # Test I/O at the max burst rate
> diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
> index 1225334cb8..4f363f295f 100755
> --- a/tests/qemu-iotests/149
> +++ b/tests/qemu-iotests/149
> @@ -314,13 +314,13 @@ def test_once(config, qemu_img=False):
>  image_size = 4 * oneTB
>  if qemu_img:
>  iotests.log("# Create image")
> -qemu_img_create(config, image_size / oneMB)
> +qemu_img_create(config, image_size // oneMB)
>  else:
>  iotests.log("# Create image")
> -create_image(config, image_size / oneMB)
> +create_image(config, image_size // oneMB)
>  
>  lowOffsetMB = 100
> -highOffsetMB = 3 * oneTB / oneMB
> +highOffsetMB = 3 * oneTB // oneMB
>  
>  try:
>  if not qemu_img:
> diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
> index fe53b9f446..1bb74d67c4 100755
> --- a/tests/qemu-iotests/151
> +++ b/tests/qemu-iotests/151
> @@ -67,9 +67,9 @@ class TestActiveMirror(iotests.QMPTestCase):
>  

Re: [Qemu-block] [Qemu-devel] [PATCH 2/9] iotests: Flush in iotests.py's QemuIoInteractive

[Cleber Rosa]

On 10/15/18 10:14 AM, Max Reitz wrote:
> After issuing a command, flush the pipe.  This does not change anything
> in Python 2, but it makes a difference in Python 3.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Cleber Rosa 

Re: [Qemu-block] [PATCH 6/9] iotests: Explicitly inherit FDs in Python

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 04:14:50PM +0200, Max Reitz wrote:
> Python 3.2 introduced the inheritable attribute for FDs.  At the same
> time, it changed the default so that all FDs are not inheritable by
> default, that only inheritable FDs are inherited to subprocesses, and
> only if close_fds is explicitly set to False.
> 
> Adhere to this by setting close_fds to False when working with
> subprocesses that may want to inherit FDs, and by trying to
> set_inheritable() on FDs that we do want to bequeath to them.
> 
> Signed-off-by: Max Reitz 
> ---
>  scripts/qemu.py| 13 +++--
>  scripts/qmp/qmp.py |  7 +++
>  tests/qemu-iotests/147 |  7 +++
>  3 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index f099ce7278..28366c4a67 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -142,10 +142,18 @@ class QEMUMachine(object):
>  if opts:
>  options.append(opts)
>  
> +# This did not exist before 3.2, but since then it is
> +# mandatory for our purpose
> +try:
> +os.set_inheritable(fd, True)
> +except AttributeError:
> +pass
> +

This is add_fd(), so calling set_inheritable() automatically here
makes sense.

>  self._args.append('-add-fd')
>  self._args.append(','.join(options))
>  return self
>  
> +# The caller needs to make sure the FD is inheritable
>  def send_fd_scm(self, fd_file_path):
>  # In iotest.py, the qmp should always use unix socket.
>  assert self._qmp.is_scm_available()
> @@ -159,7 +167,7 @@ class QEMUMachine(object):
>  "%s" % fd_file_path]
>  devnull = open(os.path.devnull, 'rb')
>  proc = subprocess.Popen(fd_param, stdin=devnull, 
> stdout=subprocess.PIPE,
> -stderr=subprocess.STDOUT)
> +stderr=subprocess.STDOUT, close_fds=False)
>  output = proc.communicate()[0]
>  if output:
>  LOG.debug(output)
> @@ -280,7 +288,8 @@ class QEMUMachine(object):
> stdin=devnull,
> stdout=self._qemu_log_file,
> stderr=subprocess.STDOUT,
> -   shell=False)
> +   shell=False,
> +   close_fds=False)
>  self._post_launch()
>  
>  def wait(self):
> diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
> index 5c8cf6a056..009be8345b 100644
> --- a/scripts/qmp/qmp.py
> +++ b/scripts/qmp/qmp.py
> @@ -10,6 +10,7 @@
>  
>  import json
>  import errno
> +import os
>  import socket
>  import logging
>  
> @@ -253,4 +254,10 @@ class QEMUMonitorProtocol(object):
>  return self.__sock.fileno()
>  
>  def is_scm_available(self):
> +# This did not exist before 3.2, but since then it is
> +# mandatory for our purpose
> +try:
> +os.set_inheritable(self.get_sock_fd(), True)
> +except AttributeError:
> +pass

Why did you decide to place this code inside is_scm_available()?

For reference, this is the only caller of is_scm_available():

def send_fd_scm(self, fd_file_path):
# In iotest.py, the qmp should always use unix socket.
assert self._qmp.is_scm_available()
...

In addition to making a method called is_*() have an unexpected
side-effect, the method won't be called at all if running with
debugging disabled.

I suggest simply placing the os.set_inheritable() call inside
send_fd_scm(), as close as possible to the subprocess.Popen()
call.


>  return self.__sock.family == socket.AF_UNIX
> diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
> index d2081df84b..b58455645b 100755
> --- a/tests/qemu-iotests/147
> +++ b/tests/qemu-iotests/147
> @@ -229,6 +229,13 @@ class BuiltinNBD(NBDBlockdevAddBase):
>  sockfd = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
>  sockfd.connect(unix_socket)
>  
> +# This did not exist before 3.2, but since then it is
> +# mandatory for our purpose
> +try:
> +os.set_inheritable(sockfd.fileno(), True)
> +except AttributeError:
> +pass
> +

Why not make send_fd_scm() responsible for calling
os.set_inheritable(), making this hunk unnecessary?


>  result = self.vm.send_fd_scm(str(sockfd.fileno()))
>  self.assertEqual(result, 0, 'Failed to send socket FD')
>  
> -- 
> 2.17.1
> 

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [PATCH 1/9] iotests: Make nbd-fault-injector flush

[Cleber Rosa]

On 10/15/18 10:14 AM, Max Reitz wrote:
> When closing a connection, make the nbd-fault-injector flush the socket.
> Without this, the output is a bit unreliable with Python 3.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Cleber Rosa 

Re: [Qemu-block] [PATCH 8/9] iotests: Modify imports for Python 3

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 02:59:28PM -0400, Cleber Rosa wrote:
> 
> 
> On 10/15/18 10:14 AM, Max Reitz wrote:
> > There are two imports that need to be modified when running the iotests
> > under Python 3: One is StringIO, which no longer exists; instead, the
> > StringIO class comes from the io module, so import it from there.  The
> > other is the ConfigParser, which has just been renamed to configparser.
> > 
> > Signed-off-by: Max Reitz 
> > ---
> >  tests/qemu-iotests/iotests.py| 8 ++--
> >  tests/qemu-iotests/nbd-fault-injector.py | 7 +--
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > index 7ca94e9278..a64ea90fb4 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], 
> > supported_cache_modes=[],
> >  
> >  # We need to filter out the time taken from the output so that 
> > qemu-iotest
> >  # can reliably diff the results against master output.
> > -import StringIO
> > +if sys.version_info.major >= 3:
> > +from io import StringIO
> > +else:
> > +from StringIO import StringIO
> > +
> >  if debug:
> >  output = sys.stdout
> >  verbosity = 2
> >  sys.argv.remove('-d')
> >  else:
> > -output = StringIO.StringIO()
> > +output = StringIO()
> >  
> >  logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
> >  
> > diff --git a/tests/qemu-iotests/nbd-fault-injector.py 
> > b/tests/qemu-iotests/nbd-fault-injector.py
> > index d45e2e0a6a..6b2d659dee 100755
> > --- a/tests/qemu-iotests/nbd-fault-injector.py
> > +++ b/tests/qemu-iotests/nbd-fault-injector.py
> > @@ -48,7 +48,10 @@ import sys
> >  import socket
> >  import struct
> >  import collections
> > -import ConfigParser
> > +if sys.version_info.major >= 3:
> > +import configparser
> > +else:
> > +import ConfigParser as configparser
> >  
> >  FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB
> >  
> > @@ -225,7 +228,7 @@ def parse_config(config):
> >  return rules
> >  
> >  def load_rules(filename):
> > -config = ConfigParser.RawConfigParser()
> > +config = configparser.RawConfigParser()
> >  with open(filename, 'rt') as f:
> >  config.readfp(f, filename)
> >  return parse_config(config)
> > 
> 
> This may be a type of culture clash (on my side, due to not enough QEMU
> culture), but shouldn't this be applied before anything else on this series?
> 
> I mean, PATCH 1/9 is supposed to fix the reliability aspects of
> nbd-fault-injector under Python 3, but without this patch, it won't
> actually run on Python 3.

Both patches are required to make the code run on Python 3, and
they don't depend on each other.  So I think the order doesn't
matter.

But I think the order chosen by Max is slightly more intuitive:
having explicit mentions of Python 3 in the code would be
confusing if we didn't fix the compatibility issues on patches
1-7 first.

-- 
Eduardo

Re: [Qemu-block] [PATCH 5/9] iotests: Different iterator behavior in Python 3

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 04:14:49PM +0200, Max Reitz wrote:
> In Python 3, several functions now return iterators instead of lists.
> This includes range(), items(), map(), and filter().  This means that if
> we really want a list, we have to wrap those instances with list().  On
> the other hand, sometimes we do just want an iterator, in which case we
> have sometimes used xrange() and iteritems() which no longer exist in
> Python 3.  Just change these calls to be range() and items(), which
> costs a bit of performance in Python 2, but will do the right thing in
> Python 3 (which is what is important).
> 
> In one instance, we only wanted the first instance of the result of a
> filter() call.  Instead of using next(filter()) which would work only in
> Python 3, or list(filter())[0] which would work everywhere but is a bit
> weird, this instance is changed to a single-line for with next() wrapped
> around, which works both in 2.7 and 3.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Eduardo Habkost 

Minor comments below:

> ---
>  tests/qemu-iotests/044 | 12 ++--
>  tests/qemu-iotests/056 |  2 +-
>  tests/qemu-iotests/065 |  4 ++--
>  tests/qemu-iotests/124 |  4 ++--
>  tests/qemu-iotests/139 |  2 +-
>  tests/qemu-iotests/163 |  6 +++---
>  6 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
> index 7ef5e46fe9..e2d6c9b189 100755
> --- a/tests/qemu-iotests/044
> +++ b/tests/qemu-iotests/044
> @@ -52,23 +52,23 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
>  # Write a refcount table
>  fd.seek(off_reftable)
>  
> -for i in xrange(0, h.refcount_table_clusters):
> +for i in range(0, h.refcount_table_clusters):
>  sector = b''.join(struct.pack('>Q',
>  off_refblock + i * 64 * 512 + j * 512)
> -for j in xrange(0, 64))
> +for j in range(0, 64))
>  fd.write(sector)
>  
>  # Write the refcount blocks
>  assert(fd.tell() == off_refblock)
>  sector = b''.join(struct.pack('>H', 1) for j in range(0, 64 * 
> 256))
> -for block in xrange(0, h.refcount_table_clusters):
> +for block in range(0, h.refcount_table_clusters):
>  fd.write(sector)
>  
>  # Write the L1 table
>  assert(fd.tell() == off_l1)
>  assert(off_l2 + 512 * h.l1_size == off_data)
>  table = b''.join(struct.pack('>Q', (1 << 63) | off_l2 + 512 * j)
> -for j in xrange(0, h.l1_size))
> +for j in range(0, h.l1_size))
>  fd.write(table)
>  
>  # Write the L2 tables
> @@ -79,14 +79,14 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
>  off = off_data
>  while remaining > 1024 * 512:
>  pytable = list((1 << 63) | off + 512 * j
> -for j in xrange(0, 1024))
> +for j in range(0, 1024))
>  table = struct.pack('>1024Q', *pytable)
>  fd.write(table)
>  remaining = remaining - 1024 * 512
>  off = off + 1024 * 512
>  
>  table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
> -for j in xrange(0, remaining // 512))
> +for j in range(0, remaining // 512))
>  fd.write(table)
>  
>  
> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
> index 223292175a..3df323984d 100755
> --- a/tests/qemu-iotests/056
> +++ b/tests/qemu-iotests/056
> @@ -32,7 +32,7 @@ target_img = os.path.join(iotests.test_dir, 'target.img')
>  def img_create(img, fmt=iotests.imgfmt, size='64M', **kwargs):
>  fullname = os.path.join(iotests.test_dir, '%s.%s' % (img, fmt))
>  optargs = []
> -for k,v in kwargs.iteritems():
> +for k,v in kwargs.items():
>  optargs = optargs + ['-o', '%s=%s' % (k,v)]
>  args = ['create', '-f', fmt] + optargs + [fullname, size]
>  iotests.qemu_img(*args)
> diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
> index 72aa9707c7..a339bf6069 100755
> --- a/tests/qemu-iotests/065
> +++ b/tests/qemu-iotests/065
> @@ -59,7 +59,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
>  :data.index('')]
>  for field in data:
>  self.assertTrue(re.match('^ {4}[^ ]', field) is not None)
> -data = map(lambda line: line.strip(), data)
> +data = list(map(lambda line: line.strip(), data))

I would find this more readable:

data = [line.strip() for line in data]

Not a blocker, though.

>  self.assertEqual(data, self.human_compare)
>  
>  class TestQMP(TestImageInfoSpecific):
> @@ -80,7 +80,7 @@ class TestQMP(TestImageInfoSpecific):
>  
>  def test_qmp(self):
>  result = self.vm.qmp('query-block')['return']
> -drive = filter(lambda drive: drive['device'] ==

[Qemu-block] [RFC PATCH] iotests: make 083 specific to raw

[Cleber Rosa]

While testing the Python 3 changes which touch the 083 test, I noticed
that it would fail with qcow2.  Expanding the testing, I noticed it
had nothing to do with the Python 3 changes, and in fact, it would not
pass on anything but raw:

 raw: pass
 bochs: not generic
 cloop: not generic
 parallels: fail
 qcow: fail
 qcow2: fail
 qed: fail
 vdi: fail
 vhdx: fail
 vmdk: fail
 vpc: fail
 luks: fail

The errors are a mixture I/O and "image not in xxx format", such as:

  === Check disconnect before data ===

  Unexpected end-of-file before all bytes were read
 -read failed: Input/output error
 +can't open device nbd+tcp://127.0.0.1:PORT/foo: Could not open 
'nbd://127.0.0.1:PORT/foo': Input/output error

  === Check disconnect after data ===

 -read 512/512 bytes at offset 0
 -512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 +can't open device nbd+tcp://127.0.0.1:PORT/foo: Image not in qcow format

I'm not aware if there's a quick fix, so, for the time being, it looks
like the honest approach is to make the test known to work on raw
only.

Signed-off-by: Cleber Rosa 
---
 tests/qemu-iotests/083 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/083 b/tests/qemu-iotests/083
index 3c1adbf0fb..9f92317b0a 100755
--- a/tests/qemu-iotests/083
+++ b/tests/qemu-iotests/083
@@ -39,7 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-_supported_fmt generic
+_supported_fmt raw
 _supported_proto nbd
 _supported_os Linux
 
-- 
2.17.1

Re: [Qemu-block] [PATCH 4/9] iotests: Use // for Python integer division

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 04:14:48PM +0200, Max Reitz wrote:
> In Python 3, / is always a floating-point division.  We usually do not
> want this, and as Python 2.7 understands // as well, change all integer
> divisions to use that.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-block] [PATCH 3/9] iotests: Use Python byte strings where appropriate

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 04:14:47PM +0200, Max Reitz wrote:
> Since byte strings are no longer the default in Python 3, we have to
> explicitly use them where we need to, which is mostly when working with
> structures.  It also means that we need to open a file in binary mode
> when we want to use structures.
> 
> On the other hand, we have to accomodate for the fact that some
> functions (still) work with byte strings but we want to use unicode
> strings (in Python 3 at least, and it does not matter in Python 2).
> This includes base64 encoding, but it is most notable when working with
> the subprocess module: Either we set univeral_newlines to True so that
> the default streams are opened in text mode (hence this parameter is
> aliased as "text" as of 3.7), or, if that is not possible, we have to
> decode the output to a normal string.
> 
> Signed-off-by: Max Reitz 
[...]
> diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
> index 9e0cad76f9..1225334cb8 100755
> --- a/tests/qemu-iotests/149
> +++ b/tests/qemu-iotests/149
> @@ -79,7 +79,7 @@ class LUKSConfig(object):
>  
>  def first_password_base64(self):
>  (pw, slot) = self.first_password()
> -return base64.b64encode(pw)
> +return base64.b64encode(pw.encode('ascii')).decode('ascii')

Would we want to have a test case for non-ascii passwords in the
future?  In that case you would probably need to make
self.passwords[] contain byte strings instead of text.

In either case, using 'ascii' as the encoding everywhere will
ensure the code will not try to be too smart about string
encodings if that happens.  I like that.

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-block] [PATCH 2/9] iotests: Flush in iotests.py's QemuIoInteractive

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 04:14:46PM +0200, Max Reitz wrote:
> After issuing a command, flush the pipe.  This does not change anything
> in Python 2, but it makes a difference in Python 3.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-block] [PATCH 1/9] iotests: Make nbd-fault-injector flush

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 04:14:45PM +0200, Max Reitz wrote:
> When closing a connection, make the nbd-fault-injector flush the socket.
> Without this, the output is a bit unreliable with Python 3.
> 
> Signed-off-by: Max Reitz 

Reviewed-by: Eduardo Habkost 

-- 
Eduardo

Re: [Qemu-block] [PATCH 8/9] iotests: Modify imports for Python 3

[Cleber Rosa]

On 10/15/18 10:14 AM, Max Reitz wrote:
> There are two imports that need to be modified when running the iotests
> under Python 3: One is StringIO, which no longer exists; instead, the
> StringIO class comes from the io module, so import it from there.  The
> other is the ConfigParser, which has just been renamed to configparser.
> 
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/iotests.py| 8 ++--
>  tests/qemu-iotests/nbd-fault-injector.py | 7 +--
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 7ca94e9278..a64ea90fb4 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], 
> supported_cache_modes=[],
>  
>  # We need to filter out the time taken from the output so that 
> qemu-iotest
>  # can reliably diff the results against master output.
> -import StringIO
> +if sys.version_info.major >= 3:
> +from io import StringIO
> +else:
> +from StringIO import StringIO
> +
>  if debug:
>  output = sys.stdout
>  verbosity = 2
>  sys.argv.remove('-d')
>  else:
> -output = StringIO.StringIO()
> +output = StringIO()
>  
>  logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
>  
> diff --git a/tests/qemu-iotests/nbd-fault-injector.py 
> b/tests/qemu-iotests/nbd-fault-injector.py
> index d45e2e0a6a..6b2d659dee 100755
> --- a/tests/qemu-iotests/nbd-fault-injector.py
> +++ b/tests/qemu-iotests/nbd-fault-injector.py
> @@ -48,7 +48,10 @@ import sys
>  import socket
>  import struct
>  import collections
> -import ConfigParser
> +if sys.version_info.major >= 3:
> +import configparser
> +else:
> +import ConfigParser as configparser
>  
>  FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB
>  
> @@ -225,7 +228,7 @@ def parse_config(config):
>  return rules
>  
>  def load_rules(filename):
> -config = ConfigParser.RawConfigParser()
> +config = configparser.RawConfigParser()
>  with open(filename, 'rt') as f:
>  config.readfp(f, filename)
>  return parse_config(config)
> 

This may be a type of culture clash (on my side, due to not enough QEMU
culture), but shouldn't this be applied before anything else on this series?

I mean, PATCH 1/9 is supposed to fix the reliability aspects of
nbd-fault-injector under Python 3, but without this patch, it won't
actually run on Python 3.

Regards,
- Cleber.

Re: [Qemu-block] [Qemu-devel] [RFC] Require Python 3 for building QEMU

[Christian Borntraeger]

On 10/15/2018 08:33 PM, Eduardo Habkost wrote:
> On Mon, Oct 15, 2018 at 08:19:18PM +0200, Christian Borntraeger wrote:
[...]


 It's easier to port stuff to Python 3 though than making them work with
 both. I think Eduardo's RFC is in part motivated by a patch from
 Philippe that converted something in iotests to work with Python 3,
 passed review and then turned out to break Python 2.
>>>
>>> Seconded.  This is not about the cost of maintaining existing
>>> compatibility gunk, it's about the extra effort to first get the
>>> remainder to work with 2 and 3, only to throw away 2 a few months later.
>>>
>>> I propose we permit ourselves to port stuff that isn't essential to
>>> building QEMU straight to 3 instead.  This includes iotests.
>>>
 Having to test every iotests patch twice with different Python versions
 isn't something I would like to do for extended periods of time.
>>>
>>> It's worth doing only if the benefits of doing it outweigh the costs.  I
>>> don't think they do.
>>
>> FWIW, I do not care about python 2 vs 3. I just want to emphasize that I 
>> consider the qemu iotest a very valuable part of the qemu test suite as it
>> has detected a lot of regressions over the past years. So as long as we keep
>> that running I am fine.
> 
> It depends where exactly one needs to keep them running.  Does
> anybody need to run iotests for QEMU 3.0+ builds in systems where
> it's impossible to install Python 3?

I run that as a regression package. Most distros seems to be fine,
with RHEL7 being the problematic case. But for upstream testing on a redhat
like distro fedora is good enough. 
> 
> If somebody really needs that, then I'd kindly ask them to
> volunteer to do the extra work to make iotests compatible with
> both Python 2 and 3.

As I said. As long as things continue to work on a resonably new distro
things are ok.

Re: [Qemu-block] [Qemu-devel] [RFC] Require Python 3 for building QEMU

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 08:19:18PM +0200, Christian Borntraeger wrote:
> 
> 
> On 10/15/2018 06:33 PM, Markus Armbruster wrote:
> > Kevin Wolf  writes:
> > 
> >> Am 15.10.2018 um 12:02 hat Peter Maydell geschrieben:
> >>> On 15 October 2018 at 10:32, Daniel P. Berrangé  
> >>> wrote:
>  On Sat, Oct 13, 2018 at 02:02:27AM -0300, Eduardo Habkost wrote:
> > Signed-off-by: Eduardo Habkost 
> > ---
> > I'd like to do this in QEMU 3.1.  I think it's time to drop
> > support for old systems that have only Python 2.
> >
> > We still have a few scripts that are not required for building
> > QEMU that still work only with Python 2 (iotests being the most
> > relevant set).  Requiring Python 3 for building QEMU won't
> > prevent people from using those scripts with Python 2 until they
> > are finally ported.
> 
>  I think it is premature & unecessary to do this. We just got QEMU 
>  building
>  with dual Python2/3 in 3.0 to give people leeway in the migration path to
>  a fully v3 future. The code to support building 2/3 in parallel is not
>  imposing a unreasonable maint burden. Dropping py2 suport would have
>  negligible impact on the code, as there's no v3-only features we have
>  used. IOW, I don't think there's a compelling reason to rush into forcing
>  users onto v3.
> 
>  If we want to drop py2, we should give people a warning of such a planned
>  change, especially since some of our targetted host OS[1] don't even
>  include a py3 as standard without acquiring extra add-on repos. Devs in
>  a typical corporate env will not have the freedom to install such extra
>  repos on their machines.
> >>>
> >>> I agree. I also think that dropping python 2 support before we've
> >>> even converted all our python scripts to handle python 3 is the
> >>> wrong order to do things. People interested in moving forward with
> >>> the transition to python-3-only should start by making sure everything
> >>> we have works with python 3...
> >>
> >> It's easier to port stuff to Python 3 though than making them work with
> >> both. I think Eduardo's RFC is in part motivated by a patch from
> >> Philippe that converted something in iotests to work with Python 3,
> >> passed review and then turned out to break Python 2.
> > 
> > Seconded.  This is not about the cost of maintaining existing
> > compatibility gunk, it's about the extra effort to first get the
> > remainder to work with 2 and 3, only to throw away 2 a few months later.
> > 
> > I propose we permit ourselves to port stuff that isn't essential to
> > building QEMU straight to 3 instead.  This includes iotests.
> > 
> >> Having to test every iotests patch twice with different Python versions
> >> isn't something I would like to do for extended periods of time.
> > 
> > It's worth doing only if the benefits of doing it outweigh the costs.  I
> > don't think they do.
> 
> FWIW, I do not care about python 2 vs 3. I just want to emphasize that I 
> consider the qemu iotest a very valuable part of the qemu test suite as it
> has detected a lot of regressions over the past years. So as long as we keep
> that running I am fine.

It depends where exactly one needs to keep them running.  Does
anybody need to run iotests for QEMU 3.0+ builds in systems where
it's impossible to install Python 3?

If somebody really needs that, then I'd kindly ask them to
volunteer to do the extra work to make iotests compatible with
both Python 2 and 3.

-- 
Eduardo

Re: [Qemu-block] [Qemu-devel] [RFC] Require Python 3 for building QEMU

[Christian Borntraeger]

On 10/15/2018 06:33 PM, Markus Armbruster wrote:
> Kevin Wolf  writes:
> 
>> Am 15.10.2018 um 12:02 hat Peter Maydell geschrieben:
>>> On 15 October 2018 at 10:32, Daniel P. Berrangé  wrote:
 On Sat, Oct 13, 2018 at 02:02:27AM -0300, Eduardo Habkost wrote:
> Signed-off-by: Eduardo Habkost 
> ---
> I'd like to do this in QEMU 3.1.  I think it's time to drop
> support for old systems that have only Python 2.
>
> We still have a few scripts that are not required for building
> QEMU that still work only with Python 2 (iotests being the most
> relevant set).  Requiring Python 3 for building QEMU won't
> prevent people from using those scripts with Python 2 until they
> are finally ported.

 I think it is premature & unecessary to do this. We just got QEMU building
 with dual Python2/3 in 3.0 to give people leeway in the migration path to
 a fully v3 future. The code to support building 2/3 in parallel is not
 imposing a unreasonable maint burden. Dropping py2 suport would have
 negligible impact on the code, as there's no v3-only features we have
 used. IOW, I don't think there's a compelling reason to rush into forcing
 users onto v3.

 If we want to drop py2, we should give people a warning of such a planned
 change, especially since some of our targetted host OS[1] don't even
 include a py3 as standard without acquiring extra add-on repos. Devs in
 a typical corporate env will not have the freedom to install such extra
 repos on their machines.
>>>
>>> I agree. I also think that dropping python 2 support before we've
>>> even converted all our python scripts to handle python 3 is the
>>> wrong order to do things. People interested in moving forward with
>>> the transition to python-3-only should start by making sure everything
>>> we have works with python 3...
>>
>> It's easier to port stuff to Python 3 though than making them work with
>> both. I think Eduardo's RFC is in part motivated by a patch from
>> Philippe that converted something in iotests to work with Python 3,
>> passed review and then turned out to break Python 2.
> 
> Seconded.  This is not about the cost of maintaining existing
> compatibility gunk, it's about the extra effort to first get the
> remainder to work with 2 and 3, only to throw away 2 a few months later.
> 
> I propose we permit ourselves to port stuff that isn't essential to
> building QEMU straight to 3 instead.  This includes iotests.
> 
>> Having to test every iotests patch twice with different Python versions
>> isn't something I would like to do for extended periods of time.
> 
> It's worth doing only if the benefits of doing it outweigh the costs.  I
> don't think they do.

FWIW, I do not care about python 2 vs 3. I just want to emphasize that I 
consider the qemu iotest a very valuable part of the qemu test suite as it
has detected a lot of regressions over the past years. So as long as we keep
that running I am fine.
> 

Re: [Qemu-block] [Qemu-devel] [RFC] Require Python 3 for building QEMU

[Eduardo Habkost]

On Mon, Oct 15, 2018 at 05:55:27PM +0100, Peter Maydell wrote:
> On 15 October 2018 at 17:33, Markus Armbruster  wrote:
> > Kevin Wolf  writes:
> >
> >> Am 15.10.2018 um 12:02 hat Peter Maydell geschrieben:
> >> It's easier to port stuff to Python 3 though than making them work with
> >> both. I think Eduardo's RFC is in part motivated by a patch from
> >> Philippe that converted something in iotests to work with Python 3,
> >> passed review and then turned out to break Python 2.
> >
> > Seconded.  This is not about the cost of maintaining existing
> > compatibility gunk, it's about the extra effort to first get the
> > remainder to work with 2 and 3, only to throw away 2 a few months later.
> >
> > I propose we permit ourselves to port stuff that isn't essential to
> > building QEMU straight to 3 instead.  This includes iotests.
> 
> No particular objection, as long as nothing run via 'make'
> or 'make check' needs Python 3.

Sounds like a good plan for 3.1.

But:

> 
> I also suspect "a few months" is an underestimate. My guess
> would be we're going to want to keep Python 2 support for
> at least the next year, maybe two.

Python 2.7 will die in less than 15 months[1].  I really want us
to stop reviewing and maintaining Python 2 code in QEMU in less
than 1 year.  Preferably in less than 6 months.

[1] https://pythonclock.org/

-- 
Eduardo

[Qemu-block] [PATCH 5/5] fw_cfg: Drop newline in @file description

[Max Reitz]

There is no good reason why there should be a newline in this
description, so remove it.

Signed-off-by: Max Reitz 
---
 vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index f9ed053eab..19f8cc67ff 100644
--- a/vl.c
+++ b/vl.c
@@ -529,7 +529,7 @@ static QemuOptsList qemu_fw_cfg_opts = {
 }, {
 .name = "file",
 .type = QEMU_OPT_STRING,
-.help = "Sets the name of the file from which\n"
+.help = "Sets the name of the file from which "
 "the fw_cfg blob will be loaded",
 }, {
 .name = "string",
-- 
2.17.1

[Qemu-block] [PATCH 3/5] qdev-monitor: Make device options help nicer

[Max Reitz]

Just like in qemu_opts_print_help(), print the device name as a caption
instead of on every single line, indent all options, and replace the '='
by ': '.

Signed-off-by: Max Reitz 
---
 qdev-monitor.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 802c18a74e..481bb4bbf9 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -285,8 +285,13 @@ int qdev_device_help(QemuOpts *opts)
 goto error;
 }
 
+if (prop_list) {
+out_printf("%s options:\n", driver);
+} else {
+out_printf("There are no options for %s.\n", driver);
+}
 for (prop = prop_list; prop; prop = prop->next) {
-out_printf("%s.%s=%s", driver, prop->value->name, prop->value->type);
+out_printf("  %s: %s", prop->value->name, prop->value->type);
 if (prop->value->has_description) {
 out_printf(" (%s)\n", prop->value->description);
 } else {
-- 
2.17.1

[Qemu-block] [PATCH 4/5] object: Make option help nicer to read

[Max Reitz]

Just like in qemu_opts_print_help(), print the object name as a caption
instead of on every single line, indent all options, and replace the '='
by ': '.

Also, indent every object name in the list of available objects.

Signed-off-by: Max Reitz 
---
 vl.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/vl.c b/vl.c
index 4e25c78bff..f9ed053eab 100644
--- a/vl.c
+++ b/vl.c
@@ -2707,7 +2707,7 @@ static bool object_create_initial(const char *type, 
QemuOpts *opts)
 list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
 for (l = list; l != NULL; l = l->next) {
 ObjectClass *oc = OBJECT_CLASS(l->data);
-printf("%s\n", object_class_get_name(oc));
+printf("  %s\n", object_class_get_name(oc));
 }
 g_slist_free(list);
 exit(0);
@@ -2729,16 +2729,20 @@ static bool object_create_initial(const char *type, 
QemuOpts *opts)
 }
 
 str = g_string_new(NULL);
-g_string_append_printf(str, "%s.%s=%s", type,
-   prop->name, prop->type);
+g_string_append_printf(str, "%s: %s", prop->name, prop->type);
 if (prop->description) {
 g_string_append_printf(str, " - %s", prop->description);
 }
 g_ptr_array_add(array, g_string_free(str, false));
 }
 g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
+if (array->len > 0) {
+printf("%s options:\n", type);
+} else {
+printf("There are no options for %s.\n", type);
+}
 for (i = 0; i < array->len; i++) {
-printf("%s\n", (char *)array->pdata[i]);
+printf("  %s\n", (char *)array->pdata[i]);
 }
 g_ptr_array_set_free_func(array, g_free);
 g_ptr_array_free(array, true);
-- 
2.17.1

[Qemu-block] [PATCH 0/5] Various option help readability improvement suggestions

[Max Reitz]

I noticed that with the (more or less) recent series from Marc-André the
output of qemu-img amend -f qcow2 -o help changed to this:

$ ./qemu-img amend -f qcow2 -o help
Creation options for 'qcow2':
qcow2-create-opts.backing_file=str - File name of a base image
qcow2-create-opts.backing_fmt=str - Image format of the base image
qcow2-create-opts.cluster_size=size - qcow2 cluster size
qcow2-create-opts.compat=str - Compatibility level (0.10 or 1.1)
[...]

The types are a nice addition, but I didn't like having the list name
printed in every single line (in fact, the list name does not make any
sense here at all, because there already is a caption which reads
"Creation options for 'qcow2'"), and I did not like the use of '=' for
types.

In general, I don't like the robot-y appearance, which is even worse in
things like -device virtio-blk,help, which gives you this (among
other lines):

> virtio-blk-pci.iothread=link

Sadly, there isn't much we can do about the "link", so this
series doesn't improve on that point.

What this series does do, however, is it changes these lists not to
print the list name on every single line, but only as a caption (and for
option lists, this caption is option, because the caller may want to
print a custom caption that is more expressive -- as is the case for
qemu-img amend -o help).

Consequentially, all list items are indented by two spaces to make clear
they belong to the caption.  I can already see that some people might
disagree on having this indentation, but I like it, so I have it in this
series.

Furthermore, '=' for types is changed to ': '.


Thus, after this series, the amend output looks like this:

$ ./qemu-img amend -f qcow2 -o help
Creation options for 'qcow2':
  backing_file: str - File name of a base image
  backing_fmt: str - Image format of the base image
  cluster_size: size - qcow2 cluster size
  compat: str - Compatibility level (0.10 or 1.1)
[...]


virtio-blk's list presents itself like so:

$ x86_64-softmmu/qemu-system-x86_64 -device virtio-blk,help
virtio-blk-pci options:
  iothread: link
  request-merging: bool (on/off)
  secs: uint32
[...]

(which is still not perfect, but at least it no longer looks like it's
been generated for consumption by libvirt)


And now we even print something when there are no options:

$ x86_64-softmmu/qemu-system-x86_64 -object can-bus,help
There are no options for can-bus.

(Before this series, there just is no output.)


As a side effect, patch 1 fixes iotest 082.


Max Reitz (5):
  option: Make option help nicer to read
  chardev: Indent list of chardevs
  qdev-monitor: Make device options help nicer
  object: Make option help nicer to read
  fw_cfg: Drop newline in @file description

 include/qemu/option.h  |   2 +-
 chardev/char.c |   2 +-
 qdev-monitor.c |   7 +-
 qemu-img.c |   4 +-
 util/qemu-option.c |  31 +-
 vl.c   |  14 +-
 tests/qemu-iotests/082.out | 956 ++---
 7 files changed, 521 insertions(+), 495 deletions(-)

-- 
2.17.1

[Qemu-block] [PATCH 1/5] option: Make option help nicer to read

[Max Reitz]

This adds some whitespace into the option help (including indentation)
and replaces '=' by ': ' (not least because '=' should be used for
values, not types).  Furthermore, the list name is no longer printed as
part of every line, but only once in advance, and only if the caller did
not print a caption already.

Signed-off-by: Max Reitz 
---
 include/qemu/option.h  |   2 +-
 qemu-img.c |   4 +-
 util/qemu-option.c |  31 +-
 tests/qemu-iotests/082.out | 956 ++---
 4 files changed, 505 insertions(+), 488 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 3dfb4493cc..844587cab3 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -132,7 +132,7 @@ typedef int (*qemu_opts_loopfunc)(void *opaque, QemuOpts 
*opts, Error **errp);
 int qemu_opts_foreach(QemuOptsList *list, qemu_opts_loopfunc func,
   void *opaque, Error **errp);
 void qemu_opts_print(QemuOpts *opts, const char *sep);
-void qemu_opts_print_help(QemuOptsList *list);
+void qemu_opts_print_help(QemuOptsList *list, bool print_caption);
 void qemu_opts_free(QemuOptsList *list);
 QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
 
diff --git a/qemu-img.c b/qemu-img.c
index b12f4cd19b..4c96db7ba4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -269,7 +269,7 @@ static int print_block_option_help(const char *filename, 
const char *fmt)
 }
 
 printf("Supported options:\n");
-qemu_opts_print_help(create_opts);
+qemu_opts_print_help(create_opts, false);
 qemu_opts_free(create_opts);
 return 0;
 }
@@ -3773,7 +3773,7 @@ static int print_amend_option_help(const char *format)
 assert(drv->create_opts);
 
 printf("Creation options for '%s':\n", format);
-qemu_opts_print_help(drv->create_opts);
+qemu_opts_print_help(drv->create_opts, false);
 printf("\nNote that not all of these options may be amendable.\n");
 return 0;
 }
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 9a5f263294..ce79332912 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -224,7 +224,14 @@ static const char *opt_type_to_string(enum QemuOptType 
type)
 g_assert_not_reached();
 }
 
-void qemu_opts_print_help(QemuOptsList *list)
+/**
+ * Print the list of options available in the given list.  If
+ * @print_caption is true, a caption (including the list name, if it
+ * exists) is printed.  The options itself will be indented, so
+ * @print_caption should only be set to false if the caller prints its
+ * own custom caption (so that the indentation makes sense).
+ */
+void qemu_opts_print_help(QemuOptsList *list, bool print_caption)
 {
 QemuOptDesc *desc;
 int i;
@@ -234,10 +241,7 @@ void qemu_opts_print_help(QemuOptsList *list)
 desc = list->desc;
 while (desc && desc->name) {
 GString *str = g_string_new(NULL);
-if (list->name) {
-g_string_append_printf(str, "%s.", list->name);
-}
-g_string_append_printf(str, "%s=%s", desc->name,
+g_string_append_printf(str, "%s: %s", desc->name,
opt_type_to_string(desc->type));
 if (desc->help) {
 g_string_append_printf(str, " - %s", desc->help);
@@ -247,8 +251,21 @@ void qemu_opts_print_help(QemuOptsList *list)
 }
 
 g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
+if (print_caption && array->len > 0) {
+if (list->name) {
+printf("%s options:\n", list->name);
+} else {
+printf("Options:\n");
+}
+} else if (array->len == 0) {
+if (list->name) {
+printf("There are no options for %s.\n", list->name);
+} else {
+printf("No options available.\n");
+}
+}
 for (i = 0; i < array->len; i++) {
-printf("%s\n", (char *)array->pdata[i]);
+printf("  %s\n", (char *)array->pdata[i]);
 }
 g_ptr_array_set_free_func(array, g_free);
 g_ptr_array_free(array, true);
@@ -930,7 +947,7 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const 
char *params,
 opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
 if (err) {
 if (invalidp && has_help_option(params)) {
-qemu_opts_print_help(list);
+qemu_opts_print_help(list, true);
 error_free(err);
 } else {
 error_report_err(err);
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 19e9fb13ff..115745f474 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -44,171 +44,171 @@ cluster_size: 8192
 
 Testing: create -f qcow2 -o help TEST_DIR/t.qcow2 128M
 Supported options:
-size Virtual disk size
-compat   Compatibility level (0.10 or 1.1)
-backing_file File name of a base image
-backing_fmt  Image format of the base image
-encryption   Encrypt the image with format 'aes'. (Deprecated in favor of 
en

[Qemu-block] [PATCH 2/5] chardev: Indent list of chardevs

[Max Reitz]

Following the example of qemu_opts_print_help(), indent all entries in
the list of character devices.

Signed-off-by: Max Reitz 
---
 chardev/char.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/char.c b/chardev/char.c
index e115166995..10d44aaefc 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -569,7 +569,7 @@ help_string_append(const char *name, void *opaque)
 {
 GString *str = opaque;
 
-g_string_append_printf(str, "\n%s", name);
+g_string_append_printf(str, "\n  %s", name);
 }
 
 static const char *chardev_alias_translate(const char *name)
-- 
2.17.1

Re: [Qemu-block] [Qemu-devel] [RFC] Require Python 3 for building QEMU

[Peter Maydell]

On 15 October 2018 at 17:33, Markus Armbruster  wrote:
> Kevin Wolf  writes:
>
>> Am 15.10.2018 um 12:02 hat Peter Maydell geschrieben:
>> It's easier to port stuff to Python 3 though than making them work with
>> both. I think Eduardo's RFC is in part motivated by a patch from
>> Philippe that converted something in iotests to work with Python 3,
>> passed review and then turned out to break Python 2.
>
> Seconded.  This is not about the cost of maintaining existing
> compatibility gunk, it's about the extra effort to first get the
> remainder to work with 2 and 3, only to throw away 2 a few months later.
>
> I propose we permit ourselves to port stuff that isn't essential to
> building QEMU straight to 3 instead.  This includes iotests.

No particular objection, as long as nothing run via 'make'
or 'make check' needs Python 3.

I also suspect "a few months" is an underestimate. My guess
would be we're going to want to keep Python 2 support for
at least the next year, maybe two.

thanks
-- PMM

[Qemu-block] [PATCH v5 1/3] qapi: add x-debug-query-block-graph

[Vladimir Sementsov-Ogievskiy]

Add a new command, returning block nodes (and their users) graph.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 qapi/block-core.json   | 108 
 include/block/block.h  |   1 +
 include/sysemu/block-backend.h |   2 +
 block.c| 147 +
 block/block-backend.c  |   5 ++
 blockdev.c |   5 ++
 6 files changed, 268 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index cfb37f8c1d..2acc8b5ad2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1657,6 +1657,114 @@
 ##
 { 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] }
 
+##
+# @XDbgBlockGraphNodeType:
+#
+# @block-backend: corresponds to BlockBackend
+#
+# @block-job: corresonds to BlockJob
+#
+# @block-driver: corresponds to BlockDriverState
+#
+# Since: 3.1
+##
+{ 'enum': 'XDbgBlockGraphNodeType',
+  'data': [ 'block-backend', 'block-job', 'block-driver' ] }
+
+##
+# @XDbgBlockGraphNode:
+#
+# @id: Block graph node identifier. This @id is generated only for
+#  x-debug-query-block-graph and does not relate to any other identifiers 
in
+#  Qemu.
+#
+# @type: Type of graph node. Can be one of block-backend, block-job or
+#block-driver-state.
+#
+# @name: Human readable name of the node. Corresponds to node-name for
+#block-driver-state nodes; is not guaranteed to be unique in the whole
+#graph (with block-jobs and block-backends).
+#
+# Since: 3.1
+##
+{ 'struct': 'XDbgBlockGraphNode',
+  'data': { 'id': 'uint64', 'type': 'XDbgBlockGraphNodeType', 'name': 'str' } }
+
+##
+# @BlockPermission:
+#
+# Enum of base block permissions.
+#
+# @consistent-read: A user that has the "permission" of consistent reads is
+#   guaranteed that their view of the contents of the block
+#   device is complete and self-consistent, representing the
+#   contents of a disk at a specific point.
+#   For most block devices (including their backing files) this
+#   is true, but the property cannot be maintained in a few
+#   situations like for intermediate nodes of a commit block
+#   job.
+#
+# @write: This permission is required to change the visible disk contents.
+#
+# @write-unchanged: This permission (which is weaker than BLK_PERM_WRITE) is
+#   both enough and required for writes to the block node when
+#   the caller promises that the visible disk content doesn't
+#   change.
+#   As the BLK_PERM_WRITE permission is strictly stronger,
+#   either is sufficient to perform an unchanging write.
+#
+# @resize: This permission is required to change the size of a block node.
+#
+# @graph-mod: This permission is required to change the node that this
+# BdrvChild points to.
+#
+# Since: 3.1
+##
+  { 'enum': 'BlockPermission',
+'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize',
+  'graph-mod' ] }
+##
+# @XDbgBlockGraphEdge:
+#
+# Block Graph edge description for x-debug-query-block-graph.
+#
+# @parent: parent id
+#
+# @child: child id
+#
+# @name: name of the relation (examples are 'file' and 'backing')
+#
+# @perm: granted permissions for the parent operating on the child
+#
+# @shared-perm: permissions that can still be granted to other users of the
+#   child while it is still attached to this parent
+#
+# Since: 3.1
+##
+{ 'struct': 'XDbgBlockGraphEdge',
+  'data': { 'parent': 'uint64', 'child': 'uint64',
+'name': 'str', 'perm': [ 'BlockPermission' ],
+'shared-perm': [ 'BlockPermission' ] } }
+
+##
+# @XDbgBlockGraph:
+#
+# Block Graph - list of nodes and list of edges.
+#
+# Since: 3.1
+##
+{ 'struct': 'XDbgBlockGraph',
+  'data': { 'nodes': ['XDbgBlockGraphNode'], 'edges': ['XDbgBlockGraphEdge'] } 
}
+
+##
+# @x-debug-query-block-graph:
+#
+# Get the block graph.
+#
+# Since: 3.1
+##
+{ 'command': 'x-debug-query-block-graph', 'returns': 'XDbgBlockGraph' }
+
 ##
 # @drive-mirror:
 #
diff --git a/include/block/block.h b/include/block/block.h
index b189cf422e..1d734b8dfd 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -445,6 +445,7 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag);
 const char *bdrv_get_format_name(BlockDriverState *bs);
 BlockDriverState *bdrv_find_node(const char *node_name);
 BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp);
+XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp);
 BlockDriverState *bdrv_lookup_bs(const char *device,
  const char *node_name,
  Error **errp);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 830d873f24..32ca5c1b12 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -237,4 +237,6 @@ int corout

[Qemu-block] [PATCH v5 2/3] scripts: add render_block_graph function for QEMUMachine

[Vladimir Sementsov-Ogievskiy]

Render block nodes graph with help of graphviz. This new function is
for debugging, so there is no sense to put it into qemu.py as a method
of QEMUMachine. Let's instead put it separately.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Acked-by: Eduardo Habkost 
Reviewed-by: Max Reitz 
---
 scripts/render_block_graph.py | 120 ++
 1 file changed, 120 insertions(+)
 create mode 100755 scripts/render_block_graph.py

diff --git a/scripts/render_block_graph.py b/scripts/render_block_graph.py
new file mode 100755
index 00..ed7e581b4f
--- /dev/null
+++ b/scripts/render_block_graph.py
@@ -0,0 +1,120 @@
+#!/usr/bin/env python
+#
+# Render Qemu Block Graph
+#
+# Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import sys
+import subprocess
+import json
+from graphviz import Digraph
+from qemu import MonitorResponseError
+
+
+def perm(arr):
+s = 'w' if 'write' in arr else '_'
+s += 'r' if 'consistent-read' in arr else '_'
+s += 'u' if 'write-unchanged' in arr else '_'
+s += 'g' if 'graph-mod' in arr else '_'
+s += 's' if 'resize' in arr else '_'
+return s
+
+
+def render_block_graph(qmp, filename, format='png'):
+'''
+Render graph in text (dot) representation into "@filename" and
+representation in @format into "@filename.@format"
+'''
+
+bds_nodes = qmp.command('query-named-block-nodes')
+bds_nodes = {n['node-name']: n for n in bds_nodes}
+
+job_nodes = qmp.command('query-block-jobs')
+job_nodes = {n['device']: n for n in job_nodes}
+
+block_graph = qmp.command('x-debug-query-block-graph')
+
+graph = Digraph(comment='Block Nodes Graph')
+graph.format = format
+graph.node('permission symbols:\l'
+   '  w - Write\l'
+   '  r - consistent-Read\l'
+   '  u - write - Unchanged\l'
+   '  g - Graph-mod\l'
+   '  s - reSize\l'
+   'edge label scheme:\l'
+   '  \l'
+   '  \l'
+   '  \l', shape='none')
+
+for n in block_graph['nodes']:
+if n['type'] == 'block-driver':
+info = bds_nodes[n['name']]
+label = n['name'] + ' [' + info['drv'] + ']'
+if info['drv'] == 'file':
+label += '\n' + os.path.basename(info['file'])
+shape = 'ellipse'
+elif n['type'] == 'block-job':
+info = job_nodes[n['name']]
+label = info['type'] + ' job (' + n['name'] + ')'
+shape = 'box'
+else:
+assert n['type'] == 'block-backend'
+label = n['name'] if n['name'] else 'unnamed blk'
+shape = 'box'
+
+graph.node(str(n['id']), label, shape=shape)
+
+for e in block_graph['edges']:
+label = '%s\l%s\l%s\l' % (e['name'], perm(e['perm']),
+  perm(e['shared-perm']))
+graph.edge(str(e['parent']), str(e['child']), label=label)
+
+graph.render(filename)
+
+
+class LibvirtGuest():
+def __init__(self, name):
+self.name = name
+
+def command(self, cmd):
+# only supports qmp commands without parameters
+m = {'execute': cmd}
+ar = ['virsh', 'qemu-monitor-command', self.name, json.dumps(m)]
+
+reply = json.loads(subprocess.check_output(ar))
+
+if 'error' in reply:
+raise MonitorResponseError(reply)
+
+return reply['return']
+
+
+if __name__ == '__main__':
+obj = sys.argv[1]
+out = sys.argv[2]
+
+if os.path.exists(obj):
+# assume unix socket
+qmp = QEMUMonitorProtocol(obj)
+qmp.connect()
+else:
+# assume libvirt guest name
+qmp = LibvirtGuest(obj)
+
+render_block_graph(qmp, out)
-- 
2.18.0

[Qemu-block] [PATCH v5 3/3] not-for-commit: example of new command usage for debugging

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/222 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
index 0ead56d574..91d88aa5c0 100644
--- a/tests/qemu-iotests/222
+++ b/tests/qemu-iotests/222
@@ -137,6 +137,8 @@ with iotests.FilePath('base.img') as base_img_path, \
 log('--- Cleanup ---')
 log('')
 
+from render_block_graph import render_block_graph
+render_block_graph(vm, '/tmp/out.dot')
 log(vm.qmp('block-job-cancel', device=src_node))
 log(vm.event_wait('BLOCK_JOB_CANCELLED'),
 filters=[iotests.filter_qmp_event])
-- 
2.18.0

[Qemu-block] [PATCH v5 0/3] block nodes graph visualization

[Vladimir Sementsov-Ogievskiy]

Hi all!

On the way of backup schemes development (and in general any complicated
developments in Qemu block layer) it would be good to have an ability to print
out graph of block nodes with their permissions.

v4:
01: fix !name -> !*name for checking the result of blk_name
rename BlockGraph -> XDbgBlockGraph, to not occupy such a
generic name for x-debug command, correspondingly switch
dbg_ prefix for functions to xdbg_, add xdbg_ into
bdrv_get_{xdbg_}block_graph too.

v4:
01: - improve documentation and graph node type names
- parentheses in macro
- s/hash/graph_nodes
- add dbg_ prefix to graph functions
- free leaked gr in dbg_graph_finalize()
- use uintptr_t and "!= 0" for hash keys, add comment
  about reserved 0 key
- add const specifier for permissions array and iterator,
  add static on permissions array size
- fall back to blk_get_attached_dev_id() for block-backend
  nodes if blk_name() returns NULL 
02: - add a-b by Eduardo and r-b by Max
- change graph node type names (rebase on 01 change)

v3: again, major rework, after long discussion with Max:
   - start creating graph looping through blk's and block jobs, don't use opaque
   - don't export pointers, generate ids instead
   (graphical representation didn't significantly changed, you can look at the
picture, attached to v2 cover-letter)

v2: major rework: Identifying non-bds nodes by their description was a bad idea,
descriptions are not guaranteed to be different for different nodes. So, the 
only
way is use pointer to identify them. And to be unique, let's use pointers to
identify all the nodes in the graph. As additional benefit, we have pointers for
each node, which is good for debugging (imagine a gdb session).

Vladimir Sementsov-Ogievskiy (3):
  qapi: add x-debug-query-block-graph
  scripts: add render_block_graph function for QEMUMachine
  not-for-commit: example of new command usage for debugging

 qapi/block-core.json   | 108 
 include/block/block.h  |   1 +
 include/sysemu/block-backend.h |   2 +
 block.c| 147 +
 block/block-backend.c  |   5 ++
 blockdev.c |   5 ++
 scripts/render_block_graph.py  | 120 +++
 tests/qemu-iotests/222 |   2 +
 8 files changed, 390 insertions(+)
 create mode 100755 scripts/render_block_graph.py

-- 
2.18.0

Re: [Qemu-block] [PATCH] qcow2: Get the request alignment for encrypted images from QCryptoBlock

[Max Reitz]

On 11.10.18 12:58, Alberto Garcia wrote:
> This doesn't have any practical effect at the moment because the
> values of BDRV_SECTOR_SIZE, QCRYPTO_BLOCK_LUKS_SECTOR_SIZE and
> QCRYPTO_BLOCK_QCOW_SECTOR_SIZE are all the same (512 bytes), but
> future encryption methods could have different requirements.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block/qcow2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This breaks non-LUKS encryption:

$ ./qemu-img create -f qcow2 -o encryption=on,encrypt.key-secret=secret
--object secret,id=secret,data=foo foo.qcow2 64M
Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encryption=on
encrypt.key-secret=secret cluster_size=65536 lazy_refcounts=off
refcount_bits=16
qemu-img: block.c:1248: bdrv_open_driver: Assertion
`is_power_of_2(bs->bl.request_alignment)' failed.
[1]13589 abort (core dumped)  ./qemu-img create -f qcow2 -o
encryption=on,encrypt.key-secret=secret --objec

(As seen in iotests 049, 087, 134, and 158.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [RFC] Require Python 3 for building QEMU

[Markus Armbruster]

Kevin Wolf  writes:

> Am 15.10.2018 um 12:02 hat Peter Maydell geschrieben:
>> On 15 October 2018 at 10:32, Daniel P. Berrangé  wrote:
>> > On Sat, Oct 13, 2018 at 02:02:27AM -0300, Eduardo Habkost wrote:
>> >> Signed-off-by: Eduardo Habkost 
>> >> ---
>> >> I'd like to do this in QEMU 3.1.  I think it's time to drop
>> >> support for old systems that have only Python 2.
>> >>
>> >> We still have a few scripts that are not required for building
>> >> QEMU that still work only with Python 2 (iotests being the most
>> >> relevant set).  Requiring Python 3 for building QEMU won't
>> >> prevent people from using those scripts with Python 2 until they
>> >> are finally ported.
>> >
>> > I think it is premature & unecessary to do this. We just got QEMU building
>> > with dual Python2/3 in 3.0 to give people leeway in the migration path to
>> > a fully v3 future. The code to support building 2/3 in parallel is not
>> > imposing a unreasonable maint burden. Dropping py2 suport would have
>> > negligible impact on the code, as there's no v3-only features we have
>> > used. IOW, I don't think there's a compelling reason to rush into forcing
>> > users onto v3.
>> >
>> > If we want to drop py2, we should give people a warning of such a planned
>> > change, especially since some of our targetted host OS[1] don't even
>> > include a py3 as standard without acquiring extra add-on repos. Devs in
>> > a typical corporate env will not have the freedom to install such extra
>> > repos on their machines.
>> 
>> I agree. I also think that dropping python 2 support before we've
>> even converted all our python scripts to handle python 3 is the
>> wrong order to do things. People interested in moving forward with
>> the transition to python-3-only should start by making sure everything
>> we have works with python 3...
>
> It's easier to port stuff to Python 3 though than making them work with
> both. I think Eduardo's RFC is in part motivated by a patch from
> Philippe that converted something in iotests to work with Python 3,
> passed review and then turned out to break Python 2.

Seconded.  This is not about the cost of maintaining existing
compatibility gunk, it's about the extra effort to first get the
remainder to work with 2 and 3, only to throw away 2 a few months later.

I propose we permit ourselves to port stuff that isn't essential to
building QEMU straight to 3 instead.  This includes iotests.

> Having to test every iotests patch twice with different Python versions
> isn't something I would like to do for extended periods of time.

It's worth doing only if the benefits of doing it outweigh the costs.  I
don't think they do.

[Qemu-block] [PATCH v4 07/11] iotests: prepare 055 to graph changes during backup job

[Vladimir Sementsov-Ogievskiy]

Backup will append fleecing-hook node above source node, so, we can't
resume by device name (because resume don't search recursively through
backing chain).

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/055 | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 3437c11507..be5451e1c5 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -48,7 +48,8 @@ class TestSingleDrive(iotests.QMPTestCase):
 def setUp(self):
 qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, 
str(image_len))
 
-self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img,
+ 'node-name=source')
 self.vm.add_drive(blockdev_target_img, interface="none")
 if iotests.qemu_default_machine == 'pc':
 self.vm.add_drive(None, 'media=cdrom', 'ide')
@@ -69,7 +70,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 result = self.vm.qmp(cmd, device='drive0', target=target, sync='full')
 self.assert_qmp(result, 'return', {})
 
-event = self.cancel_and_wait(resume=True)
+event = self.cancel_and_wait(resume=True, resume_node='source')
 self.assert_qmp(event, 'data/type', 'backup')
 
 def test_cancel_drive_backup(self):
@@ -87,7 +88,7 @@ class TestSingleDrive(iotests.QMPTestCase):
 self.assert_qmp(result, 'return', {})
 
 self.pause_job('drive0', wait=False)
-self.vm.resume_drive('drive0')
+self.vm.resume_drive('source')
 self.pause_wait('drive0')
 
 result = self.vm.qmp('query-block-jobs')
@@ -165,7 +166,8 @@ class TestSetSpeed(iotests.QMPTestCase):
 def setUp(self):
 qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, 
str(image_len))
 
-self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img,
+ 'node-name=source')
 self.vm.add_drive(blockdev_target_img, interface="none")
 self.vm.launch()
 
@@ -197,7 +199,7 @@ class TestSetSpeed(iotests.QMPTestCase):
 self.assert_qmp(result, 'return[0]/device', 'drive0')
 self.assert_qmp(result, 'return[0]/speed', 8 * 1024 * 1024)
 
-event = self.cancel_and_wait(resume=True)
+event = self.cancel_and_wait(resume=True, resume_node='source')
 self.assert_qmp(event, 'data/type', 'backup')
 
 # Check setting speed option works
@@ -210,7 +212,7 @@ class TestSetSpeed(iotests.QMPTestCase):
 self.assert_qmp(result, 'return[0]/device', 'drive0')
 self.assert_qmp(result, 'return[0]/speed', 4 * 1024 * 1024)
 
-event = self.cancel_and_wait(resume=True)
+event = self.cancel_and_wait(resume=True, resume_node='source')
 self.assert_qmp(event, 'data/type', 'backup')
 
 def test_set_speed_drive_backup(self):
@@ -236,7 +238,7 @@ class TestSetSpeed(iotests.QMPTestCase):
 result = self.vm.qmp('block-job-set-speed', device='drive0', speed=-1)
 self.assert_qmp(result, 'error/class', 'GenericError')
 
-event = self.cancel_and_wait(resume=True)
+event = self.cancel_and_wait(resume=True, resume_node='source')
 self.assert_qmp(event, 'data/type', 'backup')
 
 def test_set_speed_invalid_drive_backup(self):
@@ -464,7 +466,8 @@ class TestDriveCompression(iotests.QMPTestCase):
 pass
 
 def do_prepare_drives(self, fmt, args, attach_target):
-self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
+self.vm = iotests.VM().add_drive('blkdebug::' + test_img,
+ 'node-name=source')
 
 qemu_img('create', '-f', fmt, blockdev_target_img,
  str(TestDriveCompression.image_len), *args)
@@ -507,7 +510,7 @@ class TestDriveCompression(iotests.QMPTestCase):
 result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, 
**args)
 self.assert_qmp(result, 'return', {})
 
-event = self.cancel_and_wait(resume=True)
+event = self.cancel_and_wait(resume=True, resume_node='source')
 self.assert_qmp(event, 'data/type', 'backup')
 
 self.vm.shutdown()
@@ -532,7 +535,7 @@ class TestDriveCompression(iotests.QMPTestCase):
 self.assert_qmp(result, 'return', {})
 
 self.pause_job('drive0', wait=False)
-self.vm.resume_drive('drive0')
+self.vm.resume_drive('source')
 self.pause_wait('drive0')
 
 result = self.vm.qmp('query-block-jobs')
-- 
2.18.0

[Qemu-block] [PATCH v4 08/11] block: introduce backup-top filter driver

[Vladimir Sementsov-Ogievskiy]

Backup-top filter does copy-before-write operation. It should be
inserted above active disk and has a target node for CBW, like the
following:

+---+
| Guest |
+---+---+
|r,w
v
+---+---+  target   +---+
| backup_top|-->| target(qcow2) |
+---+---+   CBW +---+---+
|
backing |r,w
v
+---+-+
| Active disk |
+-+

The driver will be used in backup instead of write-notifiers.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/backup-top.h  |  44 +++
 block/backup-top.c  | 298 
 block/Makefile.objs |   2 +
 3 files changed, 344 insertions(+)
 create mode 100644 block/backup-top.h
 create mode 100644 block/backup-top.c

diff --git a/block/backup-top.h b/block/backup-top.h
new file mode 100644
index 00..c26af9fb78
--- /dev/null
+++ b/block/backup-top.h
@@ -0,0 +1,44 @@
+/*
+ * backup-top filter driver
+ *
+ * The driver performs Copy-Before-Write (CBW) operation: it is injected above
+ * some node, and before each write it copies _old_ data to the target node.
+ *
+ * Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
+ *
+ * Author:
+ *  Sementsov-Ogievskiy Vladimir 
+ *
+ * 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 .
+ */
+
+#include "qemu/osdep.h"
+
+#include "block/block_int.h"
+
+typedef struct BDRVBackupTopState {
+HBitmap *copy_bitmap; /* what should be copied to @target
+ on guest write. */
+BdrvChild *target;
+
+uint64_t bytes_copied;
+} BDRVBackupTopState;
+
+void bdrv_backup_top_drop(BlockDriverState *bs);
+uint64_t bdrv_backup_top_progress(BlockDriverState *bs);
+
+BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
+ BlockDriverState *target,
+ HBitmap *copy_bitmap,
+ Error **errp);
diff --git a/block/backup-top.c b/block/backup-top.c
new file mode 100644
index 00..8cb081f6f3
--- /dev/null
+++ b/block/backup-top.c
@@ -0,0 +1,298 @@
+/*
+ * backup-top filter driver
+ *
+ * The driver performs Copy-Before-Write (CBW) operation: it is injected above
+ * some node, and before each write it copies _old_ data to the target node.
+ *
+ * Copyright (c) 2018 Virtuozzo International GmbH. All rights reserved.
+ *
+ * Author:
+ *  Sementsov-Ogievskiy Vladimir 
+ *
+ * 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 .
+ */
+
+#include "qemu/osdep.h"
+
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+#include "block/block_int.h"
+#include "block/qdict.h"
+
+#include "block/backup-top.h"
+
+static coroutine_fn int backup_top_co_preadv(
+BlockDriverState *bs, uint64_t offset, uint64_t bytes,
+QEMUIOVector *qiov, int flags)
+{
+/* Features to be implemented:
+ * F1. COR. save read data to fleecing target for fast access
+ * (to reduce reads). This possibly may be done with use of 
copy-on-read
+ * filter, but we need an ability to make COR requests optional: for
+ * example, if target is a ram-cache, and if it is full now, we should
+ * skip doing COR request, as it is actually not necessary.
+ *
+ * F2. Feature for guest: read from fleecing target if data is in ram-cache
+ * and is unchanged
+ */
+
+return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
+}
+
+static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset,
+  uint64_t bytes)
+{
+int ret = 0;
+BDRVBackupTopState *s = bs->opaque;
+uint64_t gran = 1UL << hbitmap_granularity(s->copy_bitmap);
+uint64_t end = QE

[Qemu-block] [PATCH v4 06/11] iotests: allow resume_drive by node name

[Vladimir Sementsov-Ogievskiy]

After node graph changes, we may not be able to resume_drive by device
name (backing files are not recursively searched). So, lets allow to
resume by node-name. Set constant name for breakpoints, to avoid
introducing extra parameters.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index bc0b8851bd..5cc47c85de 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -404,11 +404,11 @@ class VM(qtest.QEMUQtestMachine):
 self.pause_drive(drive, "write_aio")
 return
 self.qmp('human-monitor-command',
-command_line='qemu-io %s "break %s bp_%s"' % (drive, 
event, drive))
+command_line='qemu-io %s "break %s bp_0"' % (drive, event))
 
 def resume_drive(self, drive):
 self.qmp('human-monitor-command',
-command_line='qemu-io %s "remove_break bp_%s"' % (drive, 
drive))
+command_line='qemu-io %s "remove_break bp_0"' % (drive))
 
 def hmp_qemu_io(self, drive, cmd):
 '''Write to a given drive using an HMP command'''
@@ -531,13 +531,14 @@ class QMPTestCase(unittest.TestCase):
 
self.assertEqual(self.vm.flatten_qmp_object(json.loads(json_filename[5:])),
  self.vm.flatten_qmp_object(reference))
 
-def cancel_and_wait(self, drive='drive0', force=False, resume=False):
+def cancel_and_wait(self, drive='drive0', force=False, resume=False,
+resume_node=None):
 '''Cancel a block job and wait for it to finish, returning the event'''
 result = self.vm.qmp('block-job-cancel', device=drive, force=force)
 self.assert_qmp(result, 'return', {})
 
 if resume:
-self.vm.resume_drive(drive)
+self.vm.resume_drive(resume_node or drive)
 
 cancelled = False
 result = None
-- 
2.18.0

[Qemu-block] [PATCH v4 11/11] block/backup: use backup-top instead of write notifiers

[Vladimir Sementsov-Ogievskiy]

Drop write notifiers and use filter node instead. Changes:

1. copy-before-writes now handled by filter node, so, drop all
   is_write_notifier arguments.

2. we don't have intersecting requests, so their handling is dropped.
Instead, synchronization works as follows:
when backup or backup-top starts copying of some area it firstly
clears copy-bitmap bits, and nobody touches areas, not marked with
dirty bits in copy-bitmap, so there is no intersection. Also, backup
job copy operations are surrounded by bdrv region lock, which is
actually serializing request, to not interfer with guest writes and
not read changed data from source (before reading we clear
corresponding bit in copy-bitmap, so, this area is not more handled by
backup-top).

3. To sync with in-flight requests we now just drain hook node, we
don't need rw-lock.

4. After the whole backup loop (top, full, incremental modes), we need
to check for not copied clusters, which were under backup-top operation
and we skipped them, but backup-top operation failed, error returned to
the guest and dirty bits set back.

5. Don't create additional blk, use backup-top children for copy
operations.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/backup.c | 278 +
 1 file changed, 142 insertions(+), 136 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index eda6f22318..6a2f520b28 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -26,105 +26,61 @@
 #include "qemu/bitmap.h"
 #include "qemu/error-report.h"
 
-#define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
+#include "block/backup-top.h"
 
-typedef struct CowRequest {
-int64_t start_byte;
-int64_t end_byte;
-QLIST_ENTRY(CowRequest) list;
-CoQueue wait_queue; /* coroutines blocked on this request */
-} CowRequest;
+#define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
 
 typedef struct BackupBlockJob {
 BlockJob common;
-BlockBackend *target;
+BdrvChild *source;
+BdrvChild *target;
 /* bitmap for sync=incremental */
 BdrvDirtyBitmap *sync_bitmap;
 MirrorSyncMode sync_mode;
 BlockdevOnError on_source_error;
 BlockdevOnError on_target_error;
-CoRwlock flush_rwlock;
 uint64_t len;
 uint64_t bytes_read;
 int64_t cluster_size;
 bool compress;
-NotifierWithReturn before_write;
-QLIST_HEAD(, CowRequest) inflight_reqs;
 
 HBitmap *copy_bitmap;
 bool use_copy_range;
 int64_t copy_range_size;
 
 bool serialize_target_writes;
+
+BlockDriverState *backup_top;
+uint64_t backup_top_progress;
 } BackupBlockJob;
 
 static const BlockJobDriver backup_job_driver;
 
-/* See if in-flight requests overlap and wait for them to complete */
-static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
-   int64_t start,
-   int64_t end)
-{
-CowRequest *req;
-bool retry;
-
-do {
-retry = false;
-QLIST_FOREACH(req, &job->inflight_reqs, list) {
-if (end > req->start_byte && start < req->end_byte) {
-qemu_co_queue_wait(&req->wait_queue, NULL);
-retry = true;
-break;
-}
-}
-} while (retry);
-}
-
-/* Keep track of an in-flight request */
-static void cow_request_begin(CowRequest *req, BackupBlockJob *job,
-  int64_t start, int64_t end)
-{
-req->start_byte = start;
-req->end_byte = end;
-qemu_co_queue_init(&req->wait_queue);
-QLIST_INSERT_HEAD(&job->inflight_reqs, req, list);
-}
-
-/* Forget about a completed request */
-static void cow_request_end(CowRequest *req)
-{
-QLIST_REMOVE(req, list);
-qemu_co_queue_restart_all(&req->wait_queue);
-}
-
 /* Copy range to target with a bounce buffer and return the bytes copied. If
  * error occurred, return a negative error number */
 static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
   int64_t start,
   int64_t end,
-  bool is_write_notifier,
   bool *error_is_read,
   void **bounce_buffer)
 {
 int ret;
 struct iovec iov;
 QEMUIOVector qiov;
-BlockBackend *blk = job->common.blk;
 int nbytes;
-int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
 int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
 assert(QEMU_IS_ALIGNED(start, job->cluster_size));
 hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
 nbytes = MIN(job->cluster_size, job->len - start);
 if (!*bounce_buffer) {
-*bounce_buffer = blk_blockalign(blk, job->cluster_size);
+*bounce_buffer = qemu_blockalign(job->source->bs, job->c

[Qemu-block] [PATCH v4 02/11] block/backup: move to copy_bitmap with granularity

[Vladimir Sementsov-Ogievskiy]

We are going to share this bitmap between backup and backup-top filter
driver, so let's share something more meaningful. It also simplifies
some calculations.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/backup.c | 48 +++-
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index fbe7ce19e1..0b3fddeb6c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -114,7 +114,8 @@ static int coroutine_fn 
backup_cow_with_bounce_buffer(BackupBlockJob *job,
 int read_flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
 int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
-hbitmap_reset(job->copy_bitmap, start / job->cluster_size, 1);
+assert(QEMU_IS_ALIGNED(start, job->cluster_size));
+hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
 nbytes = MIN(job->cluster_size, job->len - start);
 if (!*bounce_buffer) {
 *bounce_buffer = blk_blockalign(blk, job->cluster_size);
@@ -150,7 +151,7 @@ static int coroutine_fn 
backup_cow_with_bounce_buffer(BackupBlockJob *job,
 
 return nbytes;
 fail:
-hbitmap_set(job->copy_bitmap, start / job->cluster_size, 1);
+hbitmap_set(job->copy_bitmap, start, job->cluster_size);
 return ret;
 
 }
@@ -170,16 +171,15 @@ static int coroutine_fn 
backup_cow_with_offload(BackupBlockJob *job,
 int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
 assert(QEMU_IS_ALIGNED(job->copy_range_size, job->cluster_size));
+assert(QEMU_IS_ALIGNED(start, job->cluster_size));
 nbytes = MIN(job->copy_range_size, end - start);
 nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
-hbitmap_reset(job->copy_bitmap, start / job->cluster_size,
-  nr_clusters);
+hbitmap_reset(job->copy_bitmap, start, job->cluster_size * nr_clusters);
 ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
 read_flags, write_flags);
 if (ret < 0) {
 trace_backup_do_cow_copy_range_fail(job, start, ret);
-hbitmap_set(job->copy_bitmap, start / job->cluster_size,
-nr_clusters);
+hbitmap_set(job->copy_bitmap, start, job->cluster_size * nr_clusters);
 return ret;
 }
 
@@ -207,7 +207,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
 cow_request_begin(&cow_request, job, start, end);
 
 while (start < end) {
-if (!hbitmap_get(job->copy_bitmap, start / job->cluster_size)) {
+if (!hbitmap_get(job->copy_bitmap, start)) {
 trace_backup_do_cow_skip(job, start);
 start += job->cluster_size;
 continue; /* already copied */
@@ -303,6 +303,11 @@ static void backup_clean(Job *job)
 assert(s->target);
 blk_unref(s->target);
 s->target = NULL;
+
+if (s->copy_bitmap) {
+hbitmap_free(s->copy_bitmap);
+s->copy_bitmap = NULL;
+}
 }
 
 static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
@@ -315,7 +320,6 @@ static void backup_attached_aio_context(BlockJob *job, 
AioContext *aio_context)
 void backup_do_checkpoint(BlockJob *job, Error **errp)
 {
 BackupBlockJob *backup_job = container_of(job, BackupBlockJob, common);
-int64_t len;
 
 assert(block_job_driver(job) == &backup_job_driver);
 
@@ -325,8 +329,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
 return;
 }
 
-len = DIV_ROUND_UP(backup_job->len, backup_job->cluster_size);
-hbitmap_set(backup_job->copy_bitmap, 0, len);
+hbitmap_set(backup_job->copy_bitmap, 0, backup_job->len);
 }
 
 static void backup_drain(BlockJob *job)
@@ -381,16 +384,16 @@ static int coroutine_fn 
backup_run_incremental(BackupBlockJob *job)
 {
 int ret;
 bool error_is_read;
-int64_t cluster;
+int64_t offset;
 HBitmapIter hbi;
 
 hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
-while ((cluster = hbitmap_iter_next(&hbi)) != -1) {
+while ((offset = hbitmap_iter_next(&hbi)) != -1) {
 do {
 if (yield_and_check(job)) {
 return 0;
 }
-ret = backup_do_cow(job, cluster * job->cluster_size,
+ret = backup_do_cow(job, offset,
 job->cluster_size, &error_is_read, false);
 if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
BLOCK_ERROR_ACTION_REPORT)
@@ -412,12 +415,9 @@ static void 
backup_incremental_init_copy_bitmap(BackupBlockJob *job)
 while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
  &offset, &bytes))
 {
-uint64_t cluster = offset / job->cluster_size;
-uint64_t last_cluster = (offset + bytes) / job->cluster_size;
+hbitmap_set(job->copy_bitmap, offset, bytes);
 
-hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
-
-   

[Qemu-block] [PATCH v4 09/11] block: add lock/unlock range functions

[Vladimir Sementsov-Ogievskiy]

From: Vladimir Sementsov-Ogievskiy 

Introduce lock/unlock range functionality, based on serialized
requests. This is needed to refactor backup, dropping local
tracked-request-like synchronization.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 include/block/block_int.h |  3 +++
 block/io.c| 35 +++
 2 files changed, 38 insertions(+)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 92ecbd866e..20617e5853 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -856,6 +856,9 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
 int coroutine_fn bdrv_co_pwritev(BdrvChild *child,
 int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
 BdrvRequestFlags flags);
+void *coroutine_fn bdrv_co_try_lock(BdrvChild *child,
+int64_t offset, unsigned int bytes);
+void coroutine_fn bdrv_co_unlock(void *opaque);
 
 extern unsigned int bdrv_drain_all_count;
 void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent);
diff --git a/block/io.c b/block/io.c
index d4e46cb3dc..fd4ec53167 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3243,3 +3243,38 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, 
PreallocMode prealloc,
 
 return tco.ret;
 }
+
+void *coroutine_fn bdrv_co_try_lock(BdrvChild *child,
+int64_t offset, unsigned int bytes)
+{
+BlockDriverState *bs = child->bs;
+BdrvTrackedRequest *req;
+
+qemu_co_mutex_lock(&bs->reqs_lock);
+
+QLIST_FOREACH(req, &bs->tracked_requests, list) {
+if (req->type == BDRV_TRACKED_READ) {
+continue;
+}
+if (tracked_request_overlaps(req, offset, bytes)) {
+qemu_co_mutex_unlock(&bs->reqs_lock);
+return NULL;
+}
+}
+
+qemu_co_mutex_unlock(&bs->reqs_lock);
+
+req = g_new(BdrvTrackedRequest, 1);
+tracked_request_begin(req, bs, offset, bytes, BDRV_TRACKED_READ);
+mark_request_serialising(req, bdrv_get_cluster_size(bs));
+
+return req;
+}
+
+void coroutine_fn bdrv_co_unlock(void *opaque)
+{
+BdrvTrackedRequest *req = opaque;
+
+tracked_request_end(req);
+g_free(req);
+}
-- 
2.18.0

[Qemu-block] [PATCH v4 04/11] block: improve should_update_child

[Vladimir Sementsov-Ogievskiy]

As it already said in the comment, we don't want to create loops in
parent->child relations. So, when we try to append @to to @c, we should
check that @c is not in @to children subtree, and we should check it
recursively, not only the first level. The patch provides BFS-based
search, to check the relations.

This is needed for further fleecing-hook filter usage: we need to
append it to source, when the hook is already a parent of target, and
source may be in a backing chain of target (fleecing-scheme). So, on
appending, the hook should not became a child (direct or through
children subtree) of the target.

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

diff --git a/block.c b/block.c
index c298ca6a19..7f605b0bf0 100644
--- a/block.c
+++ b/block.c
@@ -3432,7 +3432,7 @@ void bdrv_close_all(void)
 
 static bool should_update_child(BdrvChild *c, BlockDriverState *to)
 {
-BdrvChild *to_c;
+GList *queue = NULL, *pos;
 
 if (c->role->stay_at_node) {
 return false;
@@ -3468,13 +3468,35 @@ static bool should_update_child(BdrvChild *c, 
BlockDriverState *to)
  * if A is a child of B, that means we cannot replace A by B there
  * because that would create a loop.  Silently detaching A from B
  * is also not really an option.  So overall just leaving A in
- * place there is the most sensible choice. */
-QLIST_FOREACH(to_c, &to->children, next) {
-if (to_c == c) {
-return false;
+ * place there is the most sensible choice.
+ *
+ * upd: If the child @c belongs to the @to's children, or children of it's
+ * children and so on - this would create a loop to. To prevent it let's do
+ * a BFS search on @to children subtree.
+ */
+
+pos = queue = g_list_append(queue, to);
+while (pos) {
+BlockDriverState *v = pos->data;
+BdrvChild *c2;
+
+QLIST_FOREACH(c2, &v->children, next) {
+if (c2 == c) {
+g_list_free(queue);
+return false;
+}
+
+if (g_list_find(queue, c2->bs)) {
+continue;
+}
+
+queue = g_list_append(queue, c2->bs);
 }
+
+pos = pos->next;
 }
 
+g_list_free(queue);
 return true;
 }
 
-- 
2.18.0

[Qemu-block] [PATCH v4 10/11] block/backup: tiny refactor backup_job_create

[Vladimir Sementsov-Ogievskiy]

Move copy-bitmap find/create code. It's needed for the following
commit, as we'll need copy_bitmap before actual block job creation. Do
it in a separate commit to simplify review.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/backup.c | 67 --
 1 file changed, 38 insertions(+), 29 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 0b3fddeb6c..eda6f22318 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -561,6 +561,8 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 BlockDriverInfo bdi;
 BackupBlockJob *job = NULL;
 int ret;
+int64_t cluster_size;
+HBitmap *copy_bitmap = NULL;
 
 assert(bs);
 assert(target);
@@ -615,6 +617,33 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 return NULL;
 }
 
+/* If there is no backing file on the target, we cannot rely on COW if our
+ * backup cluster size is smaller than the target cluster size. Even for
+ * targets with a backing file, try to avoid COW if possible. */
+ret = bdrv_get_info(target, &bdi);
+if (ret == -ENOTSUP && !target->backing) {
+/* Cluster size is not defined */
+warn_report("The target block device doesn't provide "
+"information about the block size and it doesn't have a "
+"backing file. The default block size of %u bytes is "
+"used. If the actual block size of the target exceeds "
+"this default, the backup may be unusable",
+BACKUP_CLUSTER_SIZE_DEFAULT);
+cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
+} else if (ret < 0 && !target->backing) {
+error_setg_errno(errp, -ret,
+"Couldn't determine the cluster size of the target image, "
+"which has no backing file");
+error_append_hint(errp,
+"Aborting, since this may create an unusable destination image\n");
+return NULL;
+} else if (ret < 0 && target->backing) {
+/* Not fatal; just trudge on ahead. */
+cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
+} else {
+cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
+}
+
 len = bdrv_getlength(bs);
 if (len < 0) {
 error_setg_errno(errp, -len, "unable to get length for '%s'",
@@ -622,6 +651,8 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 goto error;
 }
 
+copy_bitmap = hbitmap_alloc(len, ctz32(cluster_size));
+
 /* job->len is fixed, so we can't allow resize */
 job = block_job_create(job_id, &backup_job_driver, txn, bs,
BLK_PERM_CONSISTENT_READ,
@@ -650,35 +681,9 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 
 /* Detect image-fleecing (and similar) schemes */
 job->serialize_target_writes = bdrv_chain_contains(target, bs);
-
-/* If there is no backing file on the target, we cannot rely on COW if our
- * backup cluster size is smaller than the target cluster size. Even for
- * targets with a backing file, try to avoid COW if possible. */
-ret = bdrv_get_info(target, &bdi);
-if (ret == -ENOTSUP && !target->backing) {
-/* Cluster size is not defined */
-warn_report("The target block device doesn't provide "
-"information about the block size and it doesn't have a "
-"backing file. The default block size of %u bytes is "
-"used. If the actual block size of the target exceeds "
-"this default, the backup may be unusable",
-BACKUP_CLUSTER_SIZE_DEFAULT);
-job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
-} else if (ret < 0 && !target->backing) {
-error_setg_errno(errp, -ret,
-"Couldn't determine the cluster size of the target image, "
-"which has no backing file");
-error_append_hint(errp,
-"Aborting, since this may create an unusable destination image\n");
-goto error;
-} else if (ret < 0 && target->backing) {
-/* Not fatal; just trudge on ahead. */
-job->cluster_size = BACKUP_CLUSTER_SIZE_DEFAULT;
-} else {
-job->cluster_size = MAX(BACKUP_CLUSTER_SIZE_DEFAULT, bdi.cluster_size);
-}
-
-job->copy_bitmap = hbitmap_alloc(len, ctz32(job->cluster_size));
+job->cluster_size = cluster_size;
+job->copy_bitmap = copy_bitmap;
+copy_bitmap = NULL;
 job->use_copy_range = true;
 job->copy_range_size = MIN_NON_ZERO(blk_get_max_transfer(job->common.blk),
 blk_get_max_transfer(job->target));
@@ -694,6 +699,10 @@ BlockJob *backup_job_create(const char *job_id, 
BlockDriverState *bs,
 return &job->common;
 
  error:
+if (copy_bitmap) {
+assert(!job || !job->copy_bitmap);
+hbitmap_free(copy_bitmap);
+}
 i

[Qemu-block] [PATCH v4 03/11] block: allow serialized reads to intersect

[Vladimir Sementsov-Ogievskiy]

Otherwise, if we have serialized read-part in copy_range from backing
file to its parent if CoW take place, this CoW's sub-reads will
intersect with firstly created serialized read request.

Anyway, reads should not influence on disk view, let's allow them to
intersect.

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

diff --git a/block/io.c b/block/io.c
index bd9d688f8b..d4e46cb3dc 100644
--- a/block/io.c
+++ b/block/io.c
@@ -735,7 +735,8 @@ static bool coroutine_fn 
wait_serialising_requests(BdrvTrackedRequest *self)
 retry = false;
 qemu_co_mutex_lock(&bs->reqs_lock);
 QLIST_FOREACH(req, &bs->tracked_requests, list) {
-if (req == self || (!req->serialising && !self->serialising)) {
+if (req == self || (!req->serialising && !self->serialising) ||
+(self->type == BDRV_TRACKED_READ && req->type == self->type)) {
 continue;
 }
 if (tracked_request_overlaps(req, self->overlap_offset,
-- 
2.18.0

[Qemu-block] [PATCH v4 01/11] block/backup: simplify backup_incremental_init_copy_bitmap

[Vladimir Sementsov-Ogievskiy]

Simplify backup_incremental_init_copy_bitmap using the function
bdrv_dirty_bitmap_next_dirty_area.

Note: move to job->len instead of bitmap size: it should not matter but
less code.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/backup.c | 40 
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 435414e964..fbe7ce19e1 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -406,43 +406,27 @@ static int coroutine_fn 
backup_run_incremental(BackupBlockJob *job)
 /* init copy_bitmap from sync_bitmap */
 static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
 {
-BdrvDirtyBitmapIter *dbi;
-int64_t offset;
-int64_t end = DIV_ROUND_UP(bdrv_dirty_bitmap_size(job->sync_bitmap),
-   job->cluster_size);
-
-dbi = bdrv_dirty_iter_new(job->sync_bitmap);
-while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
-int64_t cluster = offset / job->cluster_size;
-int64_t next_cluster;
-
-offset += bdrv_dirty_bitmap_granularity(job->sync_bitmap);
-if (offset >= bdrv_dirty_bitmap_size(job->sync_bitmap)) {
-hbitmap_set(job->copy_bitmap, cluster, end - cluster);
-break;
-}
+uint64_t offset = 0;
+uint64_t bytes = job->len;
 
-offset = bdrv_dirty_bitmap_next_zero(job->sync_bitmap, offset,
- UINT64_MAX);
-if (offset == -1) {
-hbitmap_set(job->copy_bitmap, cluster, end - cluster);
-break;
-}
+while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
+ &offset, &bytes))
+{
+uint64_t cluster = offset / job->cluster_size;
+uint64_t last_cluster = (offset + bytes) / job->cluster_size;
 
-next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
-hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
-if (next_cluster >= end) {
+hbitmap_set(job->copy_bitmap, cluster, last_cluster - cluster + 1);
+
+offset = (last_cluster + 1) * job->cluster_size;
+if (offset >= job->len) {
 break;
 }
-
-bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
+bytes = job->len - offset;
 }
 
 /* TODO job_progress_set_remaining() would make more sense */
 job_progress_update(&job->common.job,
 job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
-
-bdrv_dirty_iter_free(dbi);
 }
 
 static int coroutine_fn backup_run(Job *job, Error **errp)
-- 
2.18.0

[Qemu-block] [PATCH v4 05/11] iotests: handle -f argument correctly for qemu_io_silent

[Vladimir Sementsov-Ogievskiy]

Correctly rewrite default argument. After the patch, the function can
be used for other (not only default test-chosen) image format.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/iotests.py | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4e67fbbe96..bc0b8851bd 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -136,7 +136,12 @@ def qemu_io(*args):
 
 def qemu_io_silent(*args):
 '''Run qemu-io and return the exit code, suppressing stdout'''
-args = qemu_io_args + list(args)
+if '-f' in qemu_io_args and '-f' in args:
+ind = qemu_io_args.index('-f')
+args = qemu_io_args[:ind] + qemu_io_args[ind+2:] + list(args)
+else:
+args = qemu_io_args + list(args)
+
 exitcode = subprocess.call(args, stdout=open('/dev/null', 'w'))
 if exitcode < 0:
 sys.stderr.write('qemu-io received signal %i: %s\n' %
-- 
2.18.0

[Qemu-block] [PATCH v4 00/11] backup-top filter driver for backup

[Vladimir Sementsov-Ogievskiy]

Hi all!

These series introduce backup-top driver. It's a filter-node, which
do copy-before-write operation. Mirror uses filter-node for handling
guest writes, let's move to filter-node (from write-notifiers) for
backup too (patch 16)

v4:
fixes, rewrite driver to be implicit, drop new interfaces and
don't move to BdrvDirtyBitmap for now, as it's not obvious will
it be really needed and don't relate to these series more.

v3 was "[PATCH v3 00/18] fleecing-hook driver for backup"

v2 was "[RFC v2] new, node-graph-based fleecing and backup"

These series are based on
 [PATCH v4 0/8] dirty-bitmap: rewrite bdrv_dirty_iter_next_area
and
 [PATCH 0/2] replication: drop extra sync

Based-on: <20180919124343.28206-1-vsement...@virtuozzo.com>
Based-on: <20180917145732.48590-1-vsement...@virtuozzo.com>

Vladimir Sementsov-Ogievskiy (11):
  block/backup: simplify backup_incremental_init_copy_bitmap
  block/backup: move to copy_bitmap with granularity
  block: allow serialized reads to intersect
  block: improve should_update_child
  iotests: handle -f argument correctly for qemu_io_silent
  iotests: allow resume_drive by node name
  iotests: prepare 055 to graph changes during backup job
  block: introduce backup-top filter driver
  block: add lock/unlock range functions
  block/backup: tiny refactor backup_job_create
  block/backup: use backup-top instead of write notifiers

 block/backup-top.h|  44 
 include/block/block_int.h |   3 +
 block.c   |  32 ++-
 block/backup-top.c| 298 
 block/backup.c| 415 +-
 block/io.c|  38 +++-
 block/Makefile.objs   |   2 +
 tests/qemu-iotests/055|  23 +-
 tests/qemu-iotests/iotests.py |  16 +-
 9 files changed, 641 insertions(+), 230 deletions(-)
 create mode 100644 block/backup-top.h
 create mode 100644 block/backup-top.c

-- 
2.18.0

[Qemu-block] [PATCH 9/9] iotests: Unify log outputs between Python 2 and 3

[Max Reitz]

When dumping an object into the log, there are differences between
Python 2 and 3.  First, unicode strings are prefixed by 'u' in Python 2
(they are no longer in 3, because unicode strings are the default
there).  Second, the order of keys in dicts may differ.  Third,
especially long numbers are longs in Python 2 and thus get an 'L'
suffix, which does not happen in Python 3.

To get around these differences, this patch introduces functions to
convert an object to a string that looks the same regardless of the
Python version: In Python 2, they decode unicode strings to byte strings
(normal str); in Python 3, they encode byte strings to unicode strings
(normal str).  They also manually convert lists and dicts to strings,
which allows sorting the dicts by key, so we are no longer at the mercy
of the internal implementation when it comes to how the keys appear in
the output.

This changes the output of all tests that use these logging functions.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/194.out|  22 +-
 tests/qemu-iotests/202.out|  12 +-
 tests/qemu-iotests/203.out|  14 +-
 tests/qemu-iotests/206.out| 144 +-
 tests/qemu-iotests/207.out|  52 ++--
 tests/qemu-iotests/208.out|   8 +-
 tests/qemu-iotests/210.out|  72 ++---
 tests/qemu-iotests/211.out|  66 ++---
 tests/qemu-iotests/212.out| 102 +++
 tests/qemu-iotests/213.out| 124 
 tests/qemu-iotests/216.out|   4 +-
 tests/qemu-iotests/218.out|  20 +-
 tests/qemu-iotests/219.out| 526 +-
 tests/qemu-iotests/222.out|  24 +-
 tests/qemu-iotests/iotests.py |  42 ++-
 15 files changed, 634 insertions(+), 598 deletions(-)

diff --git a/tests/qemu-iotests/194.out b/tests/qemu-iotests/194.out
index 50ac50da5e..722af888f6 100644
--- a/tests/qemu-iotests/194.out
+++ b/tests/qemu-iotests/194.out
@@ -1,18 +1,18 @@
 Launching VMs...
 Launching NBD server on destination...
-{u'return': {}}
-{u'return': {}}
+{'return': {}}
+{'return': {}}
 Starting `drive-mirror` on source...
-{u'return': {}}
+{'return': {}}
 Waiting for `drive-mirror` to complete...
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': 
{u'device': u'mirror-job0', u'type': u'mirror', u'speed': 0, u'len': 
1073741824, u'offset': 1073741824}, u'event': u'BLOCK_JOB_READY'}
+{'data': {'device': 'mirror-job0', 'len': 1073741824, 'offset': 1073741824, 
'speed': 0, 'type': 'mirror'}, 'event': 'BLOCK_JOB_READY', 'timestamp': 
{'microseconds': 'USECS', 'seconds': 'SECS'}}
 Starting migration...
-{u'return': {}}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': 
{u'status': u'setup'}, u'event': u'MIGRATION'}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': 
{u'status': u'active'}, u'event': u'MIGRATION'}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': 
{u'status': u'completed'}, u'event': u'MIGRATION'}
+{'return': {}}
+{'data': {'status': 'setup'}, 'event': 'MIGRATION', 'timestamp': 
{'microseconds': 'USECS', 'seconds': 'SECS'}}
+{'data': {'status': 'active'}, 'event': 'MIGRATION', 'timestamp': 
{'microseconds': 'USECS', 'seconds': 'SECS'}}
+{'data': {'status': 'completed'}, 'event': 'MIGRATION', 'timestamp': 
{'microseconds': 'USECS', 'seconds': 'SECS'}}
 Gracefully ending the `drive-mirror` job on source...
-{u'return': {}}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': 
{u'device': u'mirror-job0', u'type': u'mirror', u'speed': 0, u'len': 
1073741824, u'offset': 1073741824}, u'event': u'BLOCK_JOB_COMPLETED'}
+{'return': {}}
+{'data': {'device': 'mirror-job0', 'len': 1073741824, 'offset': 1073741824, 
'speed': 0, 'type': 'mirror'}, 'event': 'BLOCK_JOB_COMPLETED', 'timestamp': 
{'microseconds': 'USECS', 'seconds': 'SECS'}}
 Stopping the NBD server on destination...
-{u'return': {}}
+{'return': {}}
diff --git a/tests/qemu-iotests/202.out b/tests/qemu-iotests/202.out
index d5ea374e17..d133f09fe2 100644
--- a/tests/qemu-iotests/202.out
+++ b/tests/qemu-iotests/202.out
@@ -1,11 +1,11 @@
 Launching VM...
 Adding IOThread...
-{u'return': {}}
+{'return': {}}
 Adding blockdevs...
-{u'return': {}}
-{u'return': {}}
+{'return': {}}
+{'return': {}}
 Setting iothread...
-{u'return': {}}
-{u'return': {}}
+{'return': {}}
+{'return': {}}
 Creating external snapshots...
-{u'return': {}}
+{'return': {}}
diff --git a/tests/qemu-iotests/203.out b/tests/qemu-iotests/203.out
index 1a11f0975c..b156388a0b 100644
--- a/tests/qemu-iotests/203.out
+++ b/tests/qemu-iotests/203.out
@@ -1,11 +1,11 @@
 Launching VM...
 Setting IOThreads...
-{u'return': {}}
-{u'return': {}}
+{'return': {}}
+{'return': {}}
 Enabling migration QMP events...
-{u'return': {}}
+{'return': {}}
 Starting migration...
-{u'return': {}}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': 
{u'status': u'setup'}, u'event': u'MIGRATION'}
-{u'timestamp': {u'seconds': 'SECS', u'microseconds': 'USECS'}, u'data': 
{u'status': u'active'

[Qemu-block] [PATCH 8/9] iotests: Modify imports for Python 3

[Max Reitz]

There are two imports that need to be modified when running the iotests
under Python 3: One is StringIO, which no longer exists; instead, the
StringIO class comes from the io module, so import it from there.  The
other is the ConfigParser, which has just been renamed to configparser.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py| 8 ++--
 tests/qemu-iotests/nbd-fault-injector.py | 7 +--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 7ca94e9278..a64ea90fb4 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -683,13 +683,17 @@ def main(supported_fmts=[], supported_oses=['linux'], 
supported_cache_modes=[],
 
 # We need to filter out the time taken from the output so that qemu-iotest
 # can reliably diff the results against master output.
-import StringIO
+if sys.version_info.major >= 3:
+from io import StringIO
+else:
+from StringIO import StringIO
+
 if debug:
 output = sys.stdout
 verbosity = 2
 sys.argv.remove('-d')
 else:
-output = StringIO.StringIO()
+output = StringIO()
 
 logging.basicConfig(level=(logging.DEBUG if debug else logging.WARN))
 
diff --git a/tests/qemu-iotests/nbd-fault-injector.py 
b/tests/qemu-iotests/nbd-fault-injector.py
index d45e2e0a6a..6b2d659dee 100755
--- a/tests/qemu-iotests/nbd-fault-injector.py
+++ b/tests/qemu-iotests/nbd-fault-injector.py
@@ -48,7 +48,10 @@ import sys
 import socket
 import struct
 import collections
-import ConfigParser
+if sys.version_info.major >= 3:
+import configparser
+else:
+import ConfigParser as configparser
 
 FAKE_DISK_SIZE = 8 * 1024 * 1024 * 1024 # 8 GB
 
@@ -225,7 +228,7 @@ def parse_config(config):
 return rules
 
 def load_rules(filename):
-config = ConfigParser.RawConfigParser()
+config = configparser.RawConfigParser()
 with open(filename, 'rt') as f:
 config.readfp(f, filename)
 return parse_config(config)
-- 
2.17.1

[Qemu-block] [PATCH 6/9] iotests: Explicitly inherit FDs in Python

[Max Reitz]

Python 3.2 introduced the inheritable attribute for FDs.  At the same
time, it changed the default so that all FDs are not inheritable by
default, that only inheritable FDs are inherited to subprocesses, and
only if close_fds is explicitly set to False.

Adhere to this by setting close_fds to False when working with
subprocesses that may want to inherit FDs, and by trying to
set_inheritable() on FDs that we do want to bequeath to them.

Signed-off-by: Max Reitz 
---
 scripts/qemu.py| 13 +++--
 scripts/qmp/qmp.py |  7 +++
 tests/qemu-iotests/147 |  7 +++
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index f099ce7278..28366c4a67 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -142,10 +142,18 @@ class QEMUMachine(object):
 if opts:
 options.append(opts)
 
+# This did not exist before 3.2, but since then it is
+# mandatory for our purpose
+try:
+os.set_inheritable(fd, True)
+except AttributeError:
+pass
+
 self._args.append('-add-fd')
 self._args.append(','.join(options))
 return self
 
+# The caller needs to make sure the FD is inheritable
 def send_fd_scm(self, fd_file_path):
 # In iotest.py, the qmp should always use unix socket.
 assert self._qmp.is_scm_available()
@@ -159,7 +167,7 @@ class QEMUMachine(object):
 "%s" % fd_file_path]
 devnull = open(os.path.devnull, 'rb')
 proc = subprocess.Popen(fd_param, stdin=devnull, 
stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT)
+stderr=subprocess.STDOUT, close_fds=False)
 output = proc.communicate()[0]
 if output:
 LOG.debug(output)
@@ -280,7 +288,8 @@ class QEMUMachine(object):
stdin=devnull,
stdout=self._qemu_log_file,
stderr=subprocess.STDOUT,
-   shell=False)
+   shell=False,
+   close_fds=False)
 self._post_launch()
 
 def wait(self):
diff --git a/scripts/qmp/qmp.py b/scripts/qmp/qmp.py
index 5c8cf6a056..009be8345b 100644
--- a/scripts/qmp/qmp.py
+++ b/scripts/qmp/qmp.py
@@ -10,6 +10,7 @@
 
 import json
 import errno
+import os
 import socket
 import logging
 
@@ -253,4 +254,10 @@ class QEMUMonitorProtocol(object):
 return self.__sock.fileno()
 
 def is_scm_available(self):
+# This did not exist before 3.2, but since then it is
+# mandatory for our purpose
+try:
+os.set_inheritable(self.get_sock_fd(), True)
+except AttributeError:
+pass
 return self.__sock.family == socket.AF_UNIX
diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index d2081df84b..b58455645b 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -229,6 +229,13 @@ class BuiltinNBD(NBDBlockdevAddBase):
 sockfd = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
 sockfd.connect(unix_socket)
 
+# This did not exist before 3.2, but since then it is
+# mandatory for our purpose
+try:
+os.set_inheritable(sockfd.fileno(), True)
+except AttributeError:
+pass
+
 result = self.vm.send_fd_scm(str(sockfd.fileno()))
 self.assertEqual(result, 0, 'Failed to send socket FD')
 
-- 
2.17.1

[Qemu-block] [PATCH 7/9] iotests: 'new' module replacement in 169

[Max Reitz]

iotest 169 uses the 'new' module to add methods to a class.  This module
no longer exists in Python 3.  Instead, we can use a lambda.  Best of
all, this works in 2.7 just as well.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/169 | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index f243db9955..e5614b159d 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -23,7 +23,6 @@ import iotests
 import time
 import itertools
 import operator
-import new
 from iotests import qemu_img
 
 
@@ -144,7 +143,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
 
 def inject_test_case(klass, name, method, *args, **kwargs):
 mc = operator.methodcaller(method, *args, **kwargs)
-setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
+setattr(klass, 'test_' + name, lambda self: mc(self))
 
 for cmb in list(itertools.product((True, False), repeat=4)):
 name = ('_' if cmb[0] else '_not_') + 'persistent_'
-- 
2.17.1

[Qemu-block] [PATCH 3/9] iotests: Use Python byte strings where appropriate

[Max Reitz]

Since byte strings are no longer the default in Python 3, we have to
explicitly use them where we need to, which is mostly when working with
structures.  It also means that we need to open a file in binary mode
when we want to use structures.

On the other hand, we have to accomodate for the fact that some
functions (still) work with byte strings but we want to use unicode
strings (in Python 3 at least, and it does not matter in Python 2).
This includes base64 encoding, but it is most notable when working with
the subprocess module: Either we set univeral_newlines to True so that
the default streams are opened in text mode (hence this parameter is
aliased as "text" as of 3.7), or, if that is not possible, we have to
decode the output to a normal string.

Signed-off-by: Max Reitz 
---
 scripts/qtest.py |  2 +-
 tests/qemu-iotests/044   |  8 
 tests/qemu-iotests/149   |  8 +---
 tests/qemu-iotests/207   |  4 ++--
 tests/qemu-iotests/iotests.py| 11 +++
 tests/qemu-iotests/nbd-fault-injector.py |  4 ++--
 tests/qemu-iotests/qcow2.py  | 10 +-
 7 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/scripts/qtest.py b/scripts/qtest.py
index df0daf26ca..adf1fe3f26 100644
--- a/scripts/qtest.py
+++ b/scripts/qtest.py
@@ -64,7 +64,7 @@ class QEMUQtestProtocol(object):
 
 @param qtest_cmd: qtest command text to be sent
 """
-self._sock.sendall(qtest_cmd + "\n")
+self._sock.sendall((qtest_cmd + "\n").encode('utf-8'))
 
 def close(self):
 self._sock.close()
diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
index 11ea0f4d35..69e736f687 100755
--- a/tests/qemu-iotests/044
+++ b/tests/qemu-iotests/044
@@ -53,21 +53,21 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
 fd.seek(off_reftable)
 
 for i in xrange(0, h.refcount_table_clusters):
-sector = ''.join(struct.pack('>Q',
+sector = b''.join(struct.pack('>Q',
 off_refblock + i * 64 * 512 + j * 512)
 for j in xrange(0, 64))
 fd.write(sector)
 
 # Write the refcount blocks
 assert(fd.tell() == off_refblock)
-sector = ''.join(struct.pack('>H', 1) for j in xrange(0, 64 * 256))
+sector = b''.join(struct.pack('>H', 1) for j in range(0, 64 * 256))
 for block in xrange(0, h.refcount_table_clusters):
 fd.write(sector)
 
 # Write the L1 table
 assert(fd.tell() == off_l1)
 assert(off_l2 + 512 * h.l1_size == off_data)
-table = ''.join(struct.pack('>Q', (1 << 63) | off_l2 + 512 * j)
+table = b''.join(struct.pack('>Q', (1 << 63) | off_l2 + 512 * j)
 for j in xrange(0, h.l1_size))
 fd.write(table)
 
@@ -85,7 +85,7 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
 remaining = remaining - 1024 * 512
 off = off + 1024 * 512
 
-table = ''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
+table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
 for j in xrange(0, remaining / 512))
 fd.write(table)
 
diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 9e0cad76f9..1225334cb8 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -79,7 +79,7 @@ class LUKSConfig(object):
 
 def first_password_base64(self):
 (pw, slot) = self.first_password()
-return base64.b64encode(pw)
+return base64.b64encode(pw.encode('ascii')).decode('ascii')
 
 def active_slots(self):
 slots = []
@@ -98,7 +98,8 @@ def verify_passwordless_sudo():
 proc = subprocess.Popen(args,
 stdin=subprocess.PIPE,
 stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT)
+stderr=subprocess.STDOUT,
+universal_newlines=True)
 
 msg = proc.communicate()[0]
 
@@ -116,7 +117,8 @@ def cryptsetup(args, password=None):
 proc = subprocess.Popen(fullargs,
 stdin=subprocess.PIPE,
 stdout=subprocess.PIPE,
-stderr=subprocess.STDOUT)
+stderr=subprocess.STDOUT,
+universal_newlines=True)
 
 msg = proc.communicate(password)[0]
 
diff --git a/tests/qemu-iotests/207 b/tests/qemu-iotests/207
index 444ae233ae..2d86a3da37 100755
--- a/tests/qemu-iotests/207
+++ b/tests/qemu-iotests/207
@@ -109,7 +109,7 @@ with iotests.FilePath('t.img') as disk_path, \
 md5_key = subprocess.check_output(
 'ssh-keyscan -t rsa 127.0.0.1 2>/dev/null | grep -v "\\^#" | ' +
 'cut -d" " -f3 | base64 -d | md5sum -b | cut -d" " -f1',
-shell=True).rst

[Qemu-block] [PATCH 5/9] iotests: Different iterator behavior in Python 3

[Max Reitz]

In Python 3, several functions now return iterators instead of lists.
This includes range(), items(), map(), and filter().  This means that if
we really want a list, we have to wrap those instances with list().  On
the other hand, sometimes we do just want an iterator, in which case we
have sometimes used xrange() and iteritems() which no longer exist in
Python 3.  Just change these calls to be range() and items(), which
costs a bit of performance in Python 2, but will do the right thing in
Python 3 (which is what is important).

In one instance, we only wanted the first instance of the result of a
filter() call.  Instead of using next(filter()) which would work only in
Python 3, or list(filter())[0] which would work everywhere but is a bit
weird, this instance is changed to a single-line for with next() wrapped
around, which works both in 2.7 and 3.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/044 | 12 ++--
 tests/qemu-iotests/056 |  2 +-
 tests/qemu-iotests/065 |  4 ++--
 tests/qemu-iotests/124 |  4 ++--
 tests/qemu-iotests/139 |  2 +-
 tests/qemu-iotests/163 |  6 +++---
 6 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
index 7ef5e46fe9..e2d6c9b189 100755
--- a/tests/qemu-iotests/044
+++ b/tests/qemu-iotests/044
@@ -52,23 +52,23 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
 # Write a refcount table
 fd.seek(off_reftable)
 
-for i in xrange(0, h.refcount_table_clusters):
+for i in range(0, h.refcount_table_clusters):
 sector = b''.join(struct.pack('>Q',
 off_refblock + i * 64 * 512 + j * 512)
-for j in xrange(0, 64))
+for j in range(0, 64))
 fd.write(sector)
 
 # Write the refcount blocks
 assert(fd.tell() == off_refblock)
 sector = b''.join(struct.pack('>H', 1) for j in range(0, 64 * 256))
-for block in xrange(0, h.refcount_table_clusters):
+for block in range(0, h.refcount_table_clusters):
 fd.write(sector)
 
 # Write the L1 table
 assert(fd.tell() == off_l1)
 assert(off_l2 + 512 * h.l1_size == off_data)
 table = b''.join(struct.pack('>Q', (1 << 63) | off_l2 + 512 * j)
-for j in xrange(0, h.l1_size))
+for j in range(0, h.l1_size))
 fd.write(table)
 
 # Write the L2 tables
@@ -79,14 +79,14 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
 off = off_data
 while remaining > 1024 * 512:
 pytable = list((1 << 63) | off + 512 * j
-for j in xrange(0, 1024))
+for j in range(0, 1024))
 table = struct.pack('>1024Q', *pytable)
 fd.write(table)
 remaining = remaining - 1024 * 512
 off = off + 1024 * 512
 
 table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
-for j in xrange(0, remaining // 512))
+for j in range(0, remaining // 512))
 fd.write(table)
 
 
diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
index 223292175a..3df323984d 100755
--- a/tests/qemu-iotests/056
+++ b/tests/qemu-iotests/056
@@ -32,7 +32,7 @@ target_img = os.path.join(iotests.test_dir, 'target.img')
 def img_create(img, fmt=iotests.imgfmt, size='64M', **kwargs):
 fullname = os.path.join(iotests.test_dir, '%s.%s' % (img, fmt))
 optargs = []
-for k,v in kwargs.iteritems():
+for k,v in kwargs.items():
 optargs = optargs + ['-o', '%s=%s' % (k,v)]
 args = ['create', '-f', fmt] + optargs + [fullname, size]
 iotests.qemu_img(*args)
diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
index 72aa9707c7..a339bf6069 100755
--- a/tests/qemu-iotests/065
+++ b/tests/qemu-iotests/065
@@ -59,7 +59,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
 :data.index('')]
 for field in data:
 self.assertTrue(re.match('^ {4}[^ ]', field) is not None)
-data = map(lambda line: line.strip(), data)
+data = list(map(lambda line: line.strip(), data))
 self.assertEqual(data, self.human_compare)
 
 class TestQMP(TestImageInfoSpecific):
@@ -80,7 +80,7 @@ class TestQMP(TestImageInfoSpecific):
 
 def test_qmp(self):
 result = self.vm.qmp('query-block')['return']
-drive = filter(lambda drive: drive['device'] == 'drive0', result)[0]
+drive = next(drive for drive in result if drive['device'] == 'drive0')
 data = drive['inserted']['image']['format-specific']
 self.assertEqual(data['type'], iotests.imgfmt)
 self.assertEqual(data['data'], self.compare)
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 3ea4ac53f5..9f189e3b54 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -3

[Qemu-block] [PATCH 2/9] iotests: Flush in iotests.py's QemuIoInteractive

[Max Reitz]

After issuing a command, flush the pipe.  This does not change anything
in Python 2, but it makes a difference in Python 3.

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

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 4e67fbbe96..10f2d17419 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -178,6 +178,7 @@ class QemuIoInteractive:
 cmd = cmd.strip()
 assert cmd != 'q' and cmd != 'quit'
 self._p.stdin.write(cmd + '\n')
+self._p.stdin.flush()
 return self._read_output()
 
 
-- 
2.17.1

[Qemu-block] [PATCH 4/9] iotests: Use // for Python integer division

[Max Reitz]

In Python 3, / is always a floating-point division.  We usually do not
want this, and as Python 2.7 understands // as well, change all integer
divisions to use that.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/040|  4 ++--
 tests/qemu-iotests/044|  2 +-
 tests/qemu-iotests/093| 18 +-
 tests/qemu-iotests/149|  6 +++---
 tests/qemu-iotests/151| 12 ++--
 tests/qemu-iotests/163|  2 +-
 tests/qemu-iotests/iotests.py |  2 +-
 7 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 1cb1ceeb33..b81133a474 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -195,7 +195,7 @@ class TestSingleDrive(ImageCommitTestCase):
 
 self.assert_no_active_block_jobs()
 result = self.vm.qmp('block-commit', device='drive0', top=mid_img,
- base=backing_img, speed=(self.image_len / 4))
+ base=backing_img, speed=(self.image_len // 4))
 self.assert_qmp(result, 'return', {})
 result = self.vm.qmp('device_del', id='scsi0')
 self.assert_qmp(result, 'return', {})
@@ -225,7 +225,7 @@ class TestSingleDrive(ImageCommitTestCase):
 
 self.assert_no_active_block_jobs()
 result = self.vm.qmp('block-commit', device='drive0', top=mid_img,
- base=backing_img, speed=(self.image_len / 4))
+ base=backing_img, speed=(self.image_len // 4))
 self.assert_qmp(result, 'return', {})
 
 result = self.vm.qmp('query-block')
diff --git a/tests/qemu-iotests/044 b/tests/qemu-iotests/044
index 69e736f687..7ef5e46fe9 100755
--- a/tests/qemu-iotests/044
+++ b/tests/qemu-iotests/044
@@ -86,7 +86,7 @@ class TestRefcountTableGrowth(iotests.QMPTestCase):
 off = off + 1024 * 512
 
 table = b''.join(struct.pack('>Q', (1 << 63) | off + 512 * j)
-for j in xrange(0, remaining / 512))
+for j in xrange(0, remaining // 512))
 fd.write(table)
 
 
diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index 9d1971a56c..d88fbc182e 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -69,18 +69,18 @@ class ThrottleTestCase(iotests.QMPTestCase):
 # in. The throttled requests won't be executed until we
 # advance the virtual clock.
 rq_size = 512
-rd_nr = max(params['bps'] / rq_size / 2,
-params['bps_rd'] / rq_size,
-params['iops'] / 2,
+rd_nr = max(params['bps'] // rq_size // 2,
+params['bps_rd'] // rq_size,
+params['iops'] // 2,
 params['iops_rd'])
 rd_nr *= seconds * 2
-rd_nr /= ndrives
-wr_nr = max(params['bps'] / rq_size / 2,
-params['bps_wr'] / rq_size,
-params['iops'] / 2,
+rd_nr //= ndrives
+wr_nr = max(params['bps'] // rq_size // 2,
+params['bps_wr'] // rq_size,
+params['iops'] // 2,
 params['iops_wr'])
 wr_nr *= seconds * 2
-wr_nr /= ndrives
+wr_nr //= ndrives
 
 # Send I/O requests to all drives
 for i in range(rd_nr):
@@ -196,7 +196,7 @@ class ThrottleTestCase(iotests.QMPTestCase):
 self.configure_throttle(ndrives, settings)
 
 # Wait for the bucket to empty so we can do bursts
-wait_ns = nsec_per_sec * burst_length * burst_rate / rate
+wait_ns = nsec_per_sec * burst_length * burst_rate // rate
 self.vm.qtest("clock_step %d" % wait_ns)
 
 # Test I/O at the max burst rate
diff --git a/tests/qemu-iotests/149 b/tests/qemu-iotests/149
index 1225334cb8..4f363f295f 100755
--- a/tests/qemu-iotests/149
+++ b/tests/qemu-iotests/149
@@ -314,13 +314,13 @@ def test_once(config, qemu_img=False):
 image_size = 4 * oneTB
 if qemu_img:
 iotests.log("# Create image")
-qemu_img_create(config, image_size / oneMB)
+qemu_img_create(config, image_size // oneMB)
 else:
 iotests.log("# Create image")
-create_image(config, image_size / oneMB)
+create_image(config, image_size // oneMB)
 
 lowOffsetMB = 100
-highOffsetMB = 3 * oneTB / oneMB
+highOffsetMB = 3 * oneTB // oneMB
 
 try:
 if not qemu_img:
diff --git a/tests/qemu-iotests/151 b/tests/qemu-iotests/151
index fe53b9f446..1bb74d67c4 100755
--- a/tests/qemu-iotests/151
+++ b/tests/qemu-iotests/151
@@ -67,9 +67,9 @@ class TestActiveMirror(iotests.QMPTestCase):
 'write -P 1 0 %i' % self.image_len);
 
 # Start some background requests
-for offset in range(1 * self.image_len / 8, 3 * self.image_len / 8, 
1024 * 1024):
+for offset in range(1 * self.image_len // 8, 3 * self.image_len // 8, 
1024 * 1024):
 

[Qemu-block] [PATCH 0/9] iotests: Make them work for both Python 2 and 3

[Max Reitz]

This series prepares the iotests to work with both Python 2 and 3.  In
some places, it adds version-specific code and decides what to do based
on the version (for instance, whether to import the StringIO class from
the 'io' or the 'StringIO' module), but most of the time, it just makes
code work for both versions in general.

And when we make the switch to make Python 3 mandatory, we can simply
drop the Python 2 branches.


Max Reitz (9):
  iotests: Make nbd-fault-injector flush
  iotests: Flush in iotests.py's QemuIoInteractive
  iotests: Use Python byte strings where appropriate
  iotests: Use // for Python integer division
  iotests: Different iterator behavior in Python 3
  iotests: Explicitly inherit FDs in Python
  iotests: 'new' module replacement in 169
  iotests: Modify imports for Python 3
  iotests: Unify log outputs between Python 2 and 3

 scripts/qemu.py  |  13 +-
 scripts/qmp/qmp.py   |   7 +
 scripts/qtest.py |   2 +-
 tests/qemu-iotests/040   |   4 +-
 tests/qemu-iotests/044   |  20 +-
 tests/qemu-iotests/056   |   2 +-
 tests/qemu-iotests/065   |   4 +-
 tests/qemu-iotests/083.out   |   9 +
 tests/qemu-iotests/093   |  18 +-
 tests/qemu-iotests/124   |   4 +-
 tests/qemu-iotests/139   |   2 +-
 tests/qemu-iotests/147   |   7 +
 tests/qemu-iotests/149   |  14 +-
 tests/qemu-iotests/151   |  12 +-
 tests/qemu-iotests/163   |   8 +-
 tests/qemu-iotests/169   |   3 +-
 tests/qemu-iotests/194.out   |  22 +-
 tests/qemu-iotests/202.out   |  12 +-
 tests/qemu-iotests/203.out   |  14 +-
 tests/qemu-iotests/206.out   | 144 +++
 tests/qemu-iotests/207   |   4 +-
 tests/qemu-iotests/207.out   |  52 +--
 tests/qemu-iotests/208.out   |   8 +-
 tests/qemu-iotests/210.out   |  72 ++--
 tests/qemu-iotests/211.out   |  66 +--
 tests/qemu-iotests/212.out   | 102 ++---
 tests/qemu-iotests/213.out   | 124 +++---
 tests/qemu-iotests/216.out   |   4 +-
 tests/qemu-iotests/218.out   |  20 +-
 tests/qemu-iotests/219.out   | 526 +++
 tests/qemu-iotests/222.out   |  24 +-
 tests/qemu-iotests/iotests.py|  64 ++-
 tests/qemu-iotests/nbd-fault-injector.py |  12 +-
 tests/qemu-iotests/qcow2.py  |  10 +-
 34 files changed, 745 insertions(+), 664 deletions(-)

-- 
2.17.1

[Qemu-block] [PATCH 1/9] iotests: Make nbd-fault-injector flush

[Max Reitz]

When closing a connection, make the nbd-fault-injector flush the socket.
Without this, the output is a bit unreliable with Python 3.

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/083.out   | 9 +
 tests/qemu-iotests/nbd-fault-injector.py | 1 +
 2 files changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/083.out b/tests/qemu-iotests/083.out
index be6079d27e..f9af8bb691 100644
--- a/tests/qemu-iotests/083.out
+++ b/tests/qemu-iotests/083.out
@@ -41,6 +41,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/foo
 
 === Check disconnect after neg2 ===
 
+Unable to read from socket: Connection reset by peer
 Connection closed
 read failed: Input/output error
 
@@ -54,6 +55,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/foo
 
 === Check disconnect before request ===
 
+Unable to read from socket: Connection reset by peer
 Connection closed
 read failed: Input/output error
 
@@ -116,6 +118,7 @@ can't open device nbd+tcp://127.0.0.1:PORT/
 
 === Check disconnect after neg-classic ===
 
+Unable to read from socket: Connection reset by peer
 Connection closed
 read failed: Input/output error
 
@@ -161,6 +164,8 @@ can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
 
 === Check disconnect after neg2 ===
 
+Unable to read from socket: Connection reset by peer
+Connection closed
 read failed: Input/output error
 
 === Check disconnect 8 neg2 ===
@@ -173,6 +178,8 @@ can't open device nbd+unix:///foo?socket=TEST_DIR/nbd.sock
 
 === Check disconnect before request ===
 
+Unable to read from socket: Connection reset by peer
+Connection closed
 read failed: Input/output error
 
 === Check disconnect after request ===
@@ -234,6 +241,8 @@ can't open device nbd+unix:///?socket=TEST_DIR/nbd.sock
 
 === Check disconnect after neg-classic ===
 
+Unable to read from socket: Connection reset by peer
+Connection closed
 read failed: Input/output error
 
 *** done
diff --git a/tests/qemu-iotests/nbd-fault-injector.py 
b/tests/qemu-iotests/nbd-fault-injector.py
index f9193c0fae..439a090eb6 100755
--- a/tests/qemu-iotests/nbd-fault-injector.py
+++ b/tests/qemu-iotests/nbd-fault-injector.py
@@ -112,6 +112,7 @@ class FaultInjectionSocket(object):
 if rule.match(event, io):
 if rule.when == 0 or bufsize is None:
 print('Closing connection on rule match %s' % rule.name)
+self.sock.flush()
 sys.exit(0)
 if rule.when != -1:
 return rule.when
-- 
2.17.1

Re: [Qemu-block] [Qemu-devel] [RFC] Require Python 3 for building QEMU

[Max Reitz]

On 15.10.18 13:01, Max Reitz wrote:
> On 15.10.18 07:55, Markus Armbruster wrote:
>> Max Reitz  writes:
>>
>>> On 13.10.18 22:36, Eduardo Habkost wrote:
 On Sat, Oct 13, 2018 at 08:20:25PM +0200, Max Reitz wrote:
> On 13.10.18 07:02, Eduardo Habkost wrote:
>> Signed-off-by: Eduardo Habkost 
>> ---
>> I'd like to do this in QEMU 3.1.  I think it's time to drop
>> support for old systems that have only Python 2.
>>
>> We still have a few scripts that are not required for building
>> QEMU that still work only with Python 2 (iotests being the most
>> relevant set).  Requiring Python 3 for building QEMU won't
>> prevent people from using those scripts with Python 2 until they
>> are finally ported.
>
> It very much does because the iotests specifically use the python path
> qemu was configured with.
>
> To fix this, configure would need to write something else for into
> tests/qemu-iotests/common.env for $PYTHON.  But what?  I don't really
> want to introduce a new configure option for this.

 What's wrong with '/usr/bin/env python2' and just using the
 python2 binary from $PATH?  Why do we need to make the Python
 interpreter path for iotests configurable?
>>>
>>> Nothing, sounds good.  No idea why I discarded that idea.
>>>
> So the real fix is indeed to make the iotests work with Python 3, and I
> think that needs to be done before we can require Python 3.  Maybe it
> even needs to be done at the same time.

 I agree that this would be even better.  I just don't think the
 pending iotest porting should force all the rest of the build
 scripts to be compatible with Python 2.
>>>
>>> True.  It just means that we have to do something about the iotests
>>> before this patch can be merged.
>>
>> I keep hearing about that "we" guy, and all the stuff he has to do, but
>> I've never seen him deliver anything.
> 
> I've actually been working on it since yesterday.

Also, nobody has to do anything.  It's just that before this very patch
proposed here can go in, someone needs to do something about the
iotests.  If nobody does anything, this patch can't go in as-is.  Simple
as that.

Max

Re: [Qemu-block] [Qemu-devel] [RFC] Require Python 3 for building QEMU

[Max Reitz]

On 15.10.18 07:55, Markus Armbruster wrote:
> Max Reitz  writes:
> 
>> On 13.10.18 22:36, Eduardo Habkost wrote:
>>> On Sat, Oct 13, 2018 at 08:20:25PM +0200, Max Reitz wrote:
 On 13.10.18 07:02, Eduardo Habkost wrote:
> Signed-off-by: Eduardo Habkost 
> ---
> I'd like to do this in QEMU 3.1.  I think it's time to drop
> support for old systems that have only Python 2.
>
> We still have a few scripts that are not required for building
> QEMU that still work only with Python 2 (iotests being the most
> relevant set).  Requiring Python 3 for building QEMU won't
> prevent people from using those scripts with Python 2 until they
> are finally ported.

 It very much does because the iotests specifically use the python path
 qemu was configured with.

 To fix this, configure would need to write something else for into
 tests/qemu-iotests/common.env for $PYTHON.  But what?  I don't really
 want to introduce a new configure option for this.
>>>
>>> What's wrong with '/usr/bin/env python2' and just using the
>>> python2 binary from $PATH?  Why do we need to make the Python
>>> interpreter path for iotests configurable?
>>
>> Nothing, sounds good.  No idea why I discarded that idea.
>>
 So the real fix is indeed to make the iotests work with Python 3, and I
 think that needs to be done before we can require Python 3.  Maybe it
 even needs to be done at the same time.
>>>
>>> I agree that this would be even better.  I just don't think the
>>> pending iotest porting should force all the rest of the build
>>> scripts to be compatible with Python 2.
>>
>> True.  It just means that we have to do something about the iotests
>> before this patch can be merged.
> 
> I keep hearing about that "we" guy, and all the stuff he has to do, but
> I've never seen him deliver anything.

I've actually been working on it since yesterday.

Max

Re: [Qemu-block] [RFC] Require Python 3 for building QEMU

[Kevin Wolf]

Am 15.10.2018 um 12:02 hat Peter Maydell geschrieben:
> On 15 October 2018 at 10:32, Daniel P. Berrangé  wrote:
> > On Sat, Oct 13, 2018 at 02:02:27AM -0300, Eduardo Habkost wrote:
> >> Signed-off-by: Eduardo Habkost 
> >> ---
> >> I'd like to do this in QEMU 3.1.  I think it's time to drop
> >> support for old systems that have only Python 2.
> >>
> >> We still have a few scripts that are not required for building
> >> QEMU that still work only with Python 2 (iotests being the most
> >> relevant set).  Requiring Python 3 for building QEMU won't
> >> prevent people from using those scripts with Python 2 until they
> >> are finally ported.
> >
> > I think it is premature & unecessary to do this. We just got QEMU building
> > with dual Python2/3 in 3.0 to give people leeway in the migration path to
> > a fully v3 future. The code to support building 2/3 in parallel is not
> > imposing a unreasonable maint burden. Dropping py2 suport would have
> > negligible impact on the code, as there's no v3-only features we have
> > used. IOW, I don't think there's a compelling reason to rush into forcing
> > users onto v3.
> >
> > If we want to drop py2, we should give people a warning of such a planned
> > change, especially since some of our targetted host OS[1] don't even
> > include a py3 as standard without acquiring extra add-on repos. Devs in
> > a typical corporate env will not have the freedom to install such extra
> > repos on their machines.
> 
> I agree. I also think that dropping python 2 support before we've
> even converted all our python scripts to handle python 3 is the
> wrong order to do things. People interested in moving forward with
> the transition to python-3-only should start by making sure everything
> we have works with python 3...

It's easier to port stuff to Python 3 though than making them work with
both. I think Eduardo's RFC is in part motivated by a patch from
Philippe that converted something in iotests to work with Python 3,
passed review and then turned out to break Python 2.

Having to test every iotests patch twice with different Python versions
isn't something I would like to do for extended periods of time.

Kevin

Re: [Qemu-block] [RFC] Require Python 3 for building QEMU

[Daniel P . Berrangé]

On Mon, Oct 15, 2018 at 11:02:03AM +0100, Peter Maydell wrote:
> On 15 October 2018 at 10:32, Daniel P. Berrangé  wrote:
> > On Sat, Oct 13, 2018 at 02:02:27AM -0300, Eduardo Habkost wrote:
> >> Signed-off-by: Eduardo Habkost 
> >> ---
> >> I'd like to do this in QEMU 3.1.  I think it's time to drop
> >> support for old systems that have only Python 2.
> >>
> >> We still have a few scripts that are not required for building
> >> QEMU that still work only with Python 2 (iotests being the most
> >> relevant set).  Requiring Python 3 for building QEMU won't
> >> prevent people from using those scripts with Python 2 until they
> >> are finally ported.
> >
> > I think it is premature & unecessary to do this. We just got QEMU building
> > with dual Python2/3 in 3.0 to give people leeway in the migration path to
> > a fully v3 future. The code to support building 2/3 in parallel is not
> > imposing a unreasonable maint burden. Dropping py2 suport would have
> > negligible impact on the code, as there's no v3-only features we have
> > used. IOW, I don't think there's a compelling reason to rush into forcing
> > users onto v3.
> >
> > If we want to drop py2, we should give people a warning of such a planned
> > change, especially since some of our targetted host OS[1] don't even
> > include a py3 as standard without acquiring extra add-on repos. Devs in
> > a typical corporate env will not have the freedom to install such extra
> > repos on their machines.
> 
> I agree. I also think that dropping python 2 support before we've
> even converted all our python scripts to handle python 3 is the
> wrong order to do things. People interested in moving forward with
> the transition to python-3-only should start by making sure everything
> we have works with python 3...

FWIW,  I would /not/ object to the iotests directory being converted to
pure py3 only, skipping 2/3 compat, if that made it easier, since it is
not a commonly used thing by most people building QEMU.

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] block: change some function return type to bool

[Kevin Wolf]

Am 13.10.2018 um 10:52 hat Li Qiang geschrieben:
> Signed-off-by: Li Qiang 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [RFC] Require Python 3 for building QEMU

[Peter Maydell]

On 15 October 2018 at 10:32, Daniel P. Berrangé  wrote:
> On Sat, Oct 13, 2018 at 02:02:27AM -0300, Eduardo Habkost wrote:
>> Signed-off-by: Eduardo Habkost 
>> ---
>> I'd like to do this in QEMU 3.1.  I think it's time to drop
>> support for old systems that have only Python 2.
>>
>> We still have a few scripts that are not required for building
>> QEMU that still work only with Python 2 (iotests being the most
>> relevant set).  Requiring Python 3 for building QEMU won't
>> prevent people from using those scripts with Python 2 until they
>> are finally ported.
>
> I think it is premature & unecessary to do this. We just got QEMU building
> with dual Python2/3 in 3.0 to give people leeway in the migration path to
> a fully v3 future. The code to support building 2/3 in parallel is not
> imposing a unreasonable maint burden. Dropping py2 suport would have
> negligible impact on the code, as there's no v3-only features we have
> used. IOW, I don't think there's a compelling reason to rush into forcing
> users onto v3.
>
> If we want to drop py2, we should give people a warning of such a planned
> change, especially since some of our targetted host OS[1] don't even
> include a py3 as standard without acquiring extra add-on repos. Devs in
> a typical corporate env will not have the freedom to install such extra
> repos on their machines.

I agree. I also think that dropping python 2 support before we've
even converted all our python scripts to handle python 3 is the
wrong order to do things. People interested in moving forward with
the transition to python-3-only should start by making sure everything
we have works with python 3...

thanks
-- PMM

[Qemu-block] ping Re: [Qemu-devel] [PATCH 4/6] dirty-bitmaps: clean-up bitmaps loading and migration logic

[Vladimir Sementsov-Ogievskiy]

ping

18.09.2018 13:37, Vladimir Sementsov-Ogievskiy wrote:
> 17.09.2018 21:33, John Snow wrote:
>>
>> On 09/17/2018 11:51 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Hmm, ping, anybody here ?)
>>>
>> Was preparing to stage on Friday, working on it now.
>>
>> I never understood why you forbid the transfer of read only bitmaps
>> though, can you point that out for me?
>>
>> --js
>
> readonly bitmap - means it is not marked in_use in qcow2 file.
>
> Hm, I don't remember exact reason, but at least for shared migration 
> it is unsafe to migrate readonly bitmap without special handling: we 
> migrate it, it becomes not-readonly, it changes in RAM, then (assume) 
> storing failed and we have old version ofthe bitmap in qcow2, which is 
> not marked in_use.. So it should be somehow carefully handled, the 
> simplest option is just to forbid it.
>


-- 
Best regards,
Vladimir

Re: [Qemu-block] [PATCH v2 2/8] block: Add auto-read-only option

[Kevin Wolf]

Am 12.10.2018 um 18:47 hat Eric Blake geschrieben:
> On 10/12/18 6:55 AM, Kevin Wolf wrote:
> > If a management application builds the block graph node by node, the
> > protocol layer doesn't inherit its read-only option from the format
> > layer any more, so it must be set explicitly.
> > 
> 
> > The documentation for this option is consciously phrased in a way that
> > allows QEMU to switch to a better model eventually: Instead of trying
> > when the image is first opened, making the read-only flag dynamic and
> > changing it automatically whenever the first BLK_PERM_WRITE user is
> > attached or the last one is detached would be much more useful
> > behaviour.
> > 
> > Unfortunately, this more useful behaviour is also a lot harder to
> > implement, and libvirt needs a solution now before it can switch to
> > -blockdev, so let's start with this easier approach for now.
> 
> I agree both with the approach of getting the simpler implementation in now
> (always writable, even when we don't need to write) as well as wording the
> documentation to permit a future stricter approach (only writable at the
> points where we need to write).
> 
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >   qapi/block-core.json  |  6 ++
> >   include/block/block.h |  2 ++
> >   block.c   | 21 -
> >   block/vvfat.c |  1 +
> >   4 files changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index cfb37f8c1d..3a899298de 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -3651,6 +3651,11 @@
> >   # either generally or in certain configurations. In this 
> > case,
> >   # the default value does not work and the option must be
> >   # specified explicitly.
> > +# @auto-read-only: if true, QEMU may ignore the @read-only option and
> > +#  automatically decide whether to open the image 
> > read-only or
> > +#  read-write (and switch between the modes later), e.g.
> > +#  depending on whether the image file is writable or 
> > whether a
> > +#  writing user is attached to the node (default: false).
> 
> Bike-shedding: Do we really want to ignore @read-only? Here's the table of 9
> combinations ('t'rue, 'f'alse, 'o'mitted), with '*' on the rows that must be
> preserved for back-compat:
> 
> RO   Auto   effect
> oo  *open for write, fail if not possible
> fo  *open for write, fail if not possible
> to  *open for read, no conversion to write
> of  open for write, fail if not possible
> ff  open for write, fail if not possible
> tf  open for read, no conversion to write
> ot  attempt write but graceful fall back to read
> ft  attempt write but graceful fall back to read
> tt  ignore RO flag, attempt write anyway
> 
> That last row is weird, why not make it an explicit error instead of
> ignoring the implied difference in semantics between the two?

You're right that the description allows this. In practice,
auto-read-only can only make a node go from rw to ro, not the other way
round.

So our options are to document the current behaviour (auto-read-only has
no effect when the image is already read-only) or to make it an error.

One thought I had is that for convenience options like -hda (or in fact
-drive), auto-read-only=on could be the default, and only -blockdev and
blockdev-add would disable it by default. That would suggest that we
don't want to make it an error.

> Or, another idea: is it worth trying to support a single tri-state member
> (via an alternative between bool and enum, since the existing code uses a
> JSON bool):
> 
> "read-only": false (open for write, fail if not possible)
> "read-only": true (open read-only, no later switching)
> "read-only": "auto" (switch as needed; or for initial implementation attempt
> for write with graceful fallback to read)
> omitting read-only: same as "read-only":false for back-compat

If read-only were new, I would probably make it an enum, but adding it
now isn't very practical. I did actually start with an alternate and it
just wasn't very nice. One thing I remember is places that directly
accessed the options QDict, for which you could now have either a bool, a
string, an int or not present. It becomes a bit too much.

As read-only is optional, we could make it true/false/absent without
introducing an alternate and the additional int/string options, but I
don't like that very much either.


While we're talking about the schema, another thing I considered was
making auto-read-only an option only for the specific drivers that
support it so introspection could tell the management tool whether the
functionality is available. However, if we do this, we can't parse it in
block.c code and use a flag any more, but need to parse it in each
driver individually. Maybe it would be a better design 

Re: [Qemu-block] [RFC] Require Python 3 for building QEMU

[Daniel P . Berrangé]

On Sat, Oct 13, 2018 at 02:02:27AM -0300, Eduardo Habkost wrote:
> Signed-off-by: Eduardo Habkost 
> ---
> I'd like to do this in QEMU 3.1.  I think it's time to drop
> support for old systems that have only Python 2.
>
> We still have a few scripts that are not required for building
> QEMU that still work only with Python 2 (iotests being the most
> relevant set).  Requiring Python 3 for building QEMU won't
> prevent people from using those scripts with Python 2 until they
> are finally ported.

I think it is premature & unecessary to do this. We just got QEMU building
with dual Python2/3 in 3.0 to give people leeway in the migration path to
a fully v3 future. The code to support building 2/3 in parallel is not
imposing a unreasonable maint burden. Dropping py2 suport would have
negligible impact on the code, as there's no v3-only features we have
used. IOW, I don't think there's a compelling reason to rush into forcing
users onto v3.

If we want to drop py2, we should give people a warning of such a planned
change, especially since some of our targetted host OS[1] don't even
include a py3 as standard without acquiring extra add-on repos. Devs in
a typical corporate env will not have the freedom to install such extra
repos on their machines.

Regards,
Daniel

[1] https://qemu.weilnetz.de/doc/qemu-doc.html#Supported-build-platforms
-- 
|: 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] block: change some function return type to bool

[Alberto Garcia]

On Sat 13 Oct 2018 10:52:31 AM CEST, Li Qiang wrote:
> Signed-off-by: Li Qiang 

Reviewed-by: Alberto Garcia 

Berto

Re: [Qemu-block] [PATCH 2/3] Introduce attributes to qemu timer subsystem

[Paolo Bonzini]

On 14/10/2018 16:55, Artem Pisarenko wrote:
> Attributes are simple flags, associated with individual timers for their 
> whole lifetime.
> They intended to be used to mark individual timers for special handling by 
> various qemu features operating at qemu core level.
> Existing timer, aio and coroutine interface extended with attribute-enabled 
> variants of functions, which create/initialize timers.
> 
> Signed-off-by: Artem Pisarenko 
> ---
> 
> Notes:
> Conversion and association between QEMUTimerAttrBit and accessor macro 
> are dumb.
> Maybe better alternatives exist (like QFlags in Qt framework) or existing 
> qemu code may be reused, if any.
> Attributes also may be better named as flags, but they looks like 
> something volatile, whereas 'attribute' expresses constant nature better.
> 
>  include/block/aio.h |  50 +++-
>  include/qemu/coroutine.h|   5 +-
>  include/qemu/timer.h| 110 
> ++--
>  tests/ptimer-test-stubs.c   |   7 +--
>  util/qemu-coroutine-sleep.c |   6 ++-
>  util/qemu-timer.c   |  12 +++--
>  6 files changed, 165 insertions(+), 25 deletions(-)
> 
> diff --git a/include/block/aio.h b/include/block/aio.h
> index f08630c..a6be3fb 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -407,10 +407,35 @@ static inline QEMUTimer *aio_timer_new(AioContext *ctx, 
> QEMUClockType type,
> int scale,
> QEMUTimerCB *cb, void *opaque)
>  {
> -return timer_new_tl(ctx->tlg.tl[type], scale, cb, opaque);
> +return timer_new_a_tl(ctx->tlg.tl[type], scale, 0, cb, opaque);
>  }

The new function _a_tl is a bit ugly.

I would prefer to:

- only have a new timer_new_full that takes all of scale, attribute and
timerlist

- accept a NULL timerlist and default to main_loop_tlg.tl[type]

- add aio_timer_{new,init}_with_attrs

> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 9801e7f..cffc2b2 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -276,7 +276,10 @@ void qemu_co_rwlock_unlock(CoRwlock *lock);
>  /**
>   * Yield the coroutine for a given duration
>   */
> -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
> +#define qemu_co_sleep_ns(type, ns) \
> +qemu_co_sleep_a_ns(type, 0, ns)
> +void coroutine_fn qemu_co_sleep_a_ns(QEMUClockType type, int attributes,
> + int64_t ns);

I wouldn't bother adding co_sleep_a_ns unless it's needed.

>  /**
>   * Yield until a file descriptor becomes readable
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 39ea907..031e3a1 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -52,6 +52,28 @@ typedef enum {
>  QEMU_CLOCK_MAX
>  } QEMUClockType;
>  
> +/**
> + * QEMU Timer attributes:
> + *
> + * An individual timer may be assigned with one or multiple attributes when
> + * initialized.
> + * Attribute is a static flag, meaning that timer has corresponding property.
> + * Attributes are defined in QEMUTimerAttrBit enum and encoded to bit set,
> + * which used to initialize timer, stored to 'attributes' member and can be
> + * retrieved externally with timer_get_attributes() call.
> + * Values of QEMUTimerAttrBit aren't used directly,
> + * instead each attribute in bit set accessed with QEMU_TIMER_ATTR(id) macro,
> + * where 'id' is a unique part of attribute identifier.

I think QEMU_TIMER_ATTR is an unnecessary complication.  I would just
add a TIMER_ATTR_EXTERNAL constant.

Paolo