Re: [PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Denis Plotnikov]

The 1st patch from the series seems to be useless. The patch extending 
queue length by adding machine type may break vm-s which use seabios 
with max queue size = 128.

Looks like only this patch doesn't break anything and helps to express 
queue size and seg max dependency (the specification constraint) 
explicitly. So, I would like to re-send this patch as a standalone one 
and send other patches including the test later, when we all agree on 
how exactly to deal with issues posted in the thread.

Any objections are welcome.

Denis

On 06.11.2019 14:54, Michael S. Tsirkin wrote:
> On Wed, Nov 06, 2019 at 10:07:02AM +, Denis Lunev wrote:
>> On 11/5/19 9:51 PM, Michael S. Tsirkin wrote:
>>> On Tue, Nov 05, 2019 at 07:11:03PM +0300, Denis Plotnikov wrote:
 seg_max has a restriction to be less or equal to virtqueue size
 according to Virtio 1.0 specification

 Although seg_max can't be set directly, it's worth to express this
 dependancy directly in the code for sanity purpose.

 Signed-off-by: Denis Plotnikov 
>>> This is guest visible so needs to be machine type dependent, right?
>> we have discussed this verbally with Stefan and think that
>> there is no need to add that to the machine type as:
>>
>> - original default was 126, which matches 128 as queue
>>    length in old machine types
>> - queue length > 128 is not observed in the field as
>>    SeaBios has quirk that asserts
> Well that's just the SeaBios virtio driver. Not everyone's using that to
> drive their devices.
>
>> - if queue length will be set to something < 128 - linux
>>    guest will crash
> Again that's just one guest driver. Not everyone is using that either.
>
>
>> If we really need to preserve original __buggy__ behavior -
>> we can add boolean property, pls let us know.
>>
>> Den
> Looks like some drivers are buggy but I'm not sure it's
> the same as saying the behavior is buggy.
> So yes, I'd say it's preferable to be compatible.
>
>
 ---
   hw/block/virtio-blk.c | 2 +-
   hw/scsi/virtio-scsi.c | 2 +-
   2 files changed, 2 insertions(+), 2 deletions(-)

 diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
 index 06e57a4d39..21530304cf 100644
 --- a/hw/block/virtio-blk.c
 +++ b/hw/block/virtio-blk.c
 @@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice 
 *vdev, uint8_t *config)
   blk_get_geometry(s->blk, &capacity);
   memset(&blkcfg, 0, sizeof(blkcfg));
   virtio_stq_p(vdev, &blkcfg.capacity, capacity);
 -virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
 +virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.queue_size - 2);
   virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
   virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
   virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / 
 blk_size);
 diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
 index 839f120256..f7e5533cd5 100644
 --- a/hw/scsi/virtio-scsi.c
 +++ b/hw/scsi/virtio-scsi.c
 @@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
   VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
   
   virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
 -virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
 +virtio_stl_p(vdev, &scsiconf->seg_max, s->conf.virtqueue_size - 2);
   virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
   virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
   virtio_stl_p(vdev, &scsiconf->event_info_size, 
 sizeof(VirtIOSCSIEvent));
 -- 
 2.17.0

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Denis Plotnikov]

On 07.11.2019 19:30, Cleber Rosa wrote:
> On Wed, Nov 06, 2019 at 04:26:41PM -0300, Eduardo Habkost wrote:
>> On Wed, Nov 06, 2019 at 11:04:16AM +0100, Max Reitz wrote:
>>> On 06.11.19 10:24, Stefan Hajnoczi wrote:
 On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
> It tests proper queue size settings for all available machine types.
>
> Signed-off-by: Denis Plotnikov 
> ---
>   tests/qemu-iotests/267 | 154 +
>   tests/qemu-iotests/267.out |   1 +
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 156 insertions(+)
>   create mode 100755 tests/qemu-iotests/267
>   create mode 100644 tests/qemu-iotests/267.out
 The qemu-iotests maintainers might prefer for this to be at the
 top-level in tests/ since it's not really an iotest, but the code itself
 looks fine to me:

 Reviewed-by: Stefan Hajnoczi 
>>> Good question.  I don’t really mind, but it would be weird if started
>>> adding all kinds of “external” qemu tests (i.e. that use QMP) in the
>>> iotests directory.
>>>
>>> What is the alternative?  Just putting it in a different directory
>>> doesn’t sound that appealing to me either, because it would still depend
>>> on the iotests infrastructure, right?  (i.e., iotests.py and check)
>> We do have tests/acceptance for simple test cases written in
>> Python.  What's the reason for this test case to depend on the
>> iotests infrastructure?
>>
>> -- 
>> Eduardo
> This test does look similar in spirit to "tests/acceptance/virtio_version.py".
>
> Denis,
>
> If you think this is more of a generic test than an IO test, and would
> rather want to have it a more agnostic location, I can provide you
> with tips (or a patch) to do so.

It would be great! Thanks!

Denis

>
> Cheers,
> - Cleber.
>

Deprecating stuff for 4.2 (was: [Qemu-devel] Exposing feature deprecation to machine clients)

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy  writes:

> 07.11.2019 21:52, Philippe Mathieu-Daudé wrote:
[...]
>> Pre-release period, time to deprecate some stuffs :)
>> 
>> How should we proceed? Do you have something in mind?
>> 
>> There are older threads about this. Should we start a new thread? Gather the 
>> different ideas on the Wiki?
>> 
>> (Obviously you are not the one responsible of this topic, you just happen to 
>> be the last one worried about it on the list).
>> 
>> Regards,
>> 
>> Phil.

4.2.0-rc0 has been tagged, i.e. we're in hard freeze already.  Only bug
fixes are accepted during hard freeze.  We've occasionally bent this
rule after -rc0 for borderline cases, e.g. to tweak a new external
interface before the release calcifies it.  Making a case for bending
the rules becomes harder with each -rc.

Ideally, we'd double-check new interfaces for gaffes before a release,
and whether old interfaces that have been replaced now should be
deprecated.  There's rarely time for that, and pretty much never for
releases right after KVM Forum.

So no, I don't have anything in mind for 4.2.

We intend to tag -rc1 next Tuesday.  To make that deadline, we'd need
patches, not just ideas.

> Hi!
>
> I wanted to resend, but faced some problems, and understand that I can't do 
> it in time before soft-freeze..
> But you say, that we can deprecate something even after hard-freeze?

See above.

> Ok, the problem that I faced, is that deprecation warnings breaks some 
> iotests.. What can we do?
>
> 1. Update iotests...
>1.1 Just update iotests outputs to show warnings. Then, in next release 
> cycle, update iotests, to not use deprecated things

Sounds workable to me, but I'm not the maintainer.

>or
>1.2 Update iotests to not use deprecated things.. Not appropriate for hard 
> freeze.

Unnecessarily risky compared to 1.1.

> or
> 2. Commit deprecations without warnings.. But how do people find out about 
> this?

Not nice.

We do it for QMP, but only because we still lack the means to warn
there.

> Next, what exactly to deprecate? As I understand, we can't deprecate 
> drive-mirror now?
> So I propose to:
>
> 1. deprecate drive-backup
> 2. add optional filter-node-name parameter to drive-mirror, to correspond to 
> commit and mirror
> 3. deprecate that filter-node-name is optional for commit and mirror.

To have a chance there, we need patches a.s.a.p.

Re: [PATCH v3 00/22] iotests: Allow ./check -o data_file

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191107163708.833192-1-mre...@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH v3 00/22] iotests: Allow ./check -o data_file
Type: series
Message-id: 20191107163708.833192-1-mre...@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
01a2839 iotests: Allow check -o data_file
31bc07b iotests: Disable data_file where it cannot be used
98a2575 iotests: Make 198 work with data_file
e8f406f iotests: Make 137 work with data_file
46cc09d iotests: Make 110 work with data_file
1f7b2e5 iotests: Make 091 work with data_file
401d3ef iotests: Avoid cp/mv of test images
a3746a2 iotests: Use _rm_test_img for deleting test images
37a01c8 iotests: Avoid qemu-img create
a05c5ec iotests: Drop IMGOPTS use in 267
44aac70 iotests: Replace IMGOPTS='' by --no-opts
cb9ee70 iotests: Replace IMGOPTS= by -o
3c2893f iotests: Inject space into -ocompat=0.10 in 051
8b5f9d4 iotests: Add -o and --no-opts to _make_test_img
239f933 iotests: Let _make_test_img parse its parameters
405ddde iotests: Drop compat=1.1 in 050
527ae22 iotests: Replace IMGOPTS by _unsupported_imgopts
77f688d iotests: Filter refcount_order in 036
3f29d5f iotests: Add _filter_json_filename
58975a8 iotests/qcow2.py: Split feature fields into bits
7ea641e iotests/qcow2.py: Add dump-header-exts
469af5e iotests: s/qocw2/qcow2/

=== OUTPUT BEGIN ===
1/22 Checking commit 469af5ede216 (iotests: s/qocw2/qcow2/)
2/22 Checking commit 7ea641ec6b0a (iotests/qcow2.py: Add dump-header-exts)
ERROR: line over 90 characters
#33: FILE: tests/qemu-iotests/qcow2.py:237:
+[ 'dump-header-exts', cmd_dump_header_exts, 0, 'Dump image header 
extensions' ],

total: 1 errors, 0 warnings, 17 lines checked

Patch 2/22 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/22 Checking commit 58975a850885 (iotests/qcow2.py: Split feature fields into 
bits)
4/22 Checking commit 3f29d5f2c82a (iotests: Add _filter_json_filename)
5/22 Checking commit 77f688d94ac8 (iotests: Filter refcount_order in 036)
6/22 Checking commit 527ae221d7bc (iotests: Replace IMGOPTS by 
_unsupported_imgopts)
7/22 Checking commit 405dddedf22d (iotests: Drop compat=1.1 in 050)
8/22 Checking commit 239f933e104c (iotests: Let _make_test_img parse its 
parameters)
9/22 Checking commit 8b5f9d4a9fff (iotests: Add -o and --no-opts to 
_make_test_img)
10/22 Checking commit 3c2893f30375 (iotests: Inject space into -ocompat=0.10 in 
051)
11/22 Checking commit cb9ee70ce491 (iotests: Replace IMGOPTS= by -o)
12/22 Checking commit 44aac701db74 (iotests: Replace IMGOPTS='' by --no-opts)
13/22 Checking commit a05c5ec14fb2 (iotests: Drop IMGOPTS use in 267)
14/22 Checking commit 37a01c83e4e6 (iotests: Avoid qemu-img create)
15/22 Checking commit a3746a2198bc (iotests: Use _rm_test_img for deleting test 
images)
16/22 Checking commit 401d3ef85556 (iotests: Avoid cp/mv of test images)
17/22 Checking commit 1f7b2e52555b (iotests: Make 091 work with data_file)
18/22 Checking commit 46cc09d0608f (iotests: Make 110 work with data_file)
19/22 Checking commit e8f406f2bda8 (iotests: Make 137 work with data_file)
20/22 Checking commit 98a25755fe17 (iotests: Make 198 work with data_file)
21/22 Checking commit 31bc07b55b8a (iotests: Disable data_file where it cannot 
be used)
22/22 Checking commit 01a283955091 (iotests: Allow check -o data_file)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191107163708.833192-1-mre...@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: [RFC PATCH 02/18] qemu-storage-daemon: Add --object option

[Markus Armbruster]

Kevin Wolf  writes:

> Add a command line option to create user-creatable QOM objects.
>
> Signed-off-by: Kevin Wolf 
> ---
>  qemu-storage-daemon.c | 35 +++
>  1 file changed, 35 insertions(+)
>
> diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> index a251dc255c..48d6af43a6 100644
> --- a/qemu-storage-daemon.c
> +++ b/qemu-storage-daemon.c
> @@ -35,6 +35,8 @@
>  #include "qemu/log.h"
>  #include "qemu/main-loop.h"
>  #include "qemu/module.h"
> +#include "qemu/option.h"
> +#include "qom/object_interfaces.h"
>  
>  #include "trace/control.h"
>  
> @@ -51,10 +53,26 @@ static void help(void)
>  " specify tracing options\n"
>  "  -V, --version  output version information and exit\n"
>  "\n"
> +"  --object   define a QOM object such as 'secret' for\n"
> +" passwords and/or encryption keys\n"

This is less helpful than qemu-system-FOO's help:

-object TYPENAME[,PROP1=VALUE1,...]
create a new object of type TYPENAME setting properties
in the order they are specified.  Note that the 'id'
property must be set.  These objects are placed in the
'/objects' path.

> +"\n"
>  QEMU_HELP_BOTTOM "\n",
>  error_get_progname());
>  }
>  
> +enum {
> +OPTION_OBJECT = 256,
> +};
> +
> +static QemuOptsList qemu_object_opts = {
> +.name = "object",
> +.implied_opt_name = "qom-type",
> +.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
> +.desc = {
> +{ }
> +},
> +};
> +

Note for later: copied from vl.c.

>  static int process_options(int argc, char *argv[], Error **errp)
>  {
>  int c;
> @@ -63,6 +81,7 @@ static int process_options(int argc, char *argv[], Error 
> **errp)
>  
>  static const struct option long_options[] = {
>  {"help", no_argument, 0, 'h'},
> +{"object", required_argument, 0, OPTION_OBJECT},
>  {"version", no_argument, 0, 'V'},
>  {"trace", required_argument, NULL, 'T'},
>  {0, 0, 0, 0}
> @@ -88,6 +107,22 @@ static int process_options(int argc, char *argv[], Error 
> **errp)
>  g_free(trace_file);
>  trace_file = trace_opt_parse(optarg);
>  break;
> +case OPTION_OBJECT:
> +{
> +QemuOpts *opts;
> +const char *type;
> +
> +opts = qemu_opts_parse(&qemu_object_opts,
> +   optarg, true, &error_fatal);
> +type = qemu_opt_get(opts, "qom-type");
> +
> +if (user_creatable_print_help(type, opts)) {
> +exit(EXIT_SUCCESS);
> +}
> +user_creatable_add_opts(opts, &error_fatal);
> +qemu_opts_del(opts);
> +break;
> +}
>  }
>  }
>  if (optind != argc) {

PATCH 01 duplicates case QEMU_OPTION_trace pretty much verbatim.  Makes
sense, as qemu-storage-daemon is basically qemu-system-FOO with "FOO"
and most "system" cut away.

This patch adds vl.c's case QEMU_OPTION_object in a much simpler form.
This is one of my least favourite options, and I'll tell you why below.
Let's compare the two versions.

vl.c:

case QEMU_OPTION_object:
opts = qemu_opts_parse_noisily(qemu_find_opts("object"),
   optarg, true);
if (!opts) {
exit(1);
}
break;

Further down:

qemu_opts_foreach(qemu_find_opts("object"),
  user_creatable_add_opts_foreach,
  object_create_initial, &error_fatal);

Still further down:

qemu_opts_foreach(qemu_find_opts("object"),
  user_creatable_add_opts_foreach,
  object_create_delayed, &error_fatal);

These are basically

for opts in qemu_object_opts {
type = qemu_opt_get(opts, "qom-type");
if (type) {
if (user_creatable_print_help(type, opts)) {
exit(0);
}
if (!predicate(type)) {
continue;
}
}
obj = user_creatable_add_opts(opts, &error_fatal);
object_unref(obj);
}

where predicate(type) is true in exactly one of the two places for each
QOM type.

The reason for these gymnastics is to create objects at the right time
during startup, except there is no right time, but two.

Differences:

* Options are processed left to right without gymnastics.  Getting their
  order right is the user's problem.  I consider this an improvement.

* You use &qemu_object_opts instead of qemu_find_opts("object").  Also
  an improvement.

* You use qemu_opts_parse() instead of qemu_opts_parse_noisily().
  The latter can print help.  I failed to find a case where we lose help
  compared to qemu-system-FOO.  I didn't try very hard.

* You neglect to guard user_creatable_print_

Re: [Qemu-devel] Exposing feature deprecation to machine clients

[Vladimir Sementsov-Ogievskiy]

07.11.2019 21:52, Philippe Mathieu-Daudé wrote:
> Hi Markus,
> 
> On 8/15/19 7:40 PM, John Snow wrote:
>> On 8/15/19 10:16 AM, Markus Armbruster wrote:
>>> John Snow  writes:
> [...]
 I asked Markus this not too long ago; do we want to amend the QAPI
 schema specification to allow commands to return with "Warning" strings,
 or "Deprecated" stings to allow in-band deprecation notices for cases
 like these?

 example:

 { "return": {},
    "deprecated": True,
    "warning": "Omitting filter-node-name parameter is deprecated, it will
 be required in the future"
 }

 There's no "error" key, so this should be recognized as success by
 compatible clients, but they'll definitely see the extra information.
>>>
>>> This is a compatible evolution of the QMP protocol.
>>>
 Part of my motivation is to facilitate a more aggressive deprecation of
 legacy features by ensuring that we are able to rigorously notify users
 through any means that they need to adjust their scripts.
>>>
>>> Yes, we should help libvirt etc. with detecting use of deprecated
>>> features.  We discussed this at the KVM Forum 2018 BoF on deprecating
>>> stuff.  Minutes:
>>>
>>>  Message-ID: <87mur0ls8o@dusky.pond.sub.org>
>>>  https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html
>>>
>>> Last item is relevant here.
>>>
>>> Adding deprecation information to QMP's success response belongs to "We
>>> can also pass the buck to the next layer up", next to "emit a QMP
>>> event".
>>>
>>> Let's compare the two, i.e. "deprecation info in success response"
>>> vs. "deprecation event".
>>>
>>> 1. Possible triggers
>>>
>>> Anything we put in the success response should only ever apply to the
>>> (successful) command.  So this one's limited to QMP commands.
>>>
>>> A QMP event is not limited to QMP commands.  For instance, it could be
>>> emitted for deprecated CLI features (long after the fact, in addition to
>>> human-readable warnings on stderr), or when we detect use of a
>>> deprecated feature only after we sent the success response, say in a
>>> job.  Neither use case is particularly convincing.  Reporting use of
>>> deprecated CLI in QMP feels like a work-around for the CLI's
>>> machine-unfriendliness.  Job-like commands should really check their
>>> arguments upfront.
>>>
>>> 2. Connection to trigger
>>>
>>> Connecting responses to commands is the QMP protocol's responsibility.
>>> Transmitting deprecation information in the response trivially ties it
>>> to the offending command.
>>>
>>> The QMP protocol doesn't tie events to anything.  Thus, a deprecation
>>> event needs an event-specific tie to its trigger.
>>>
>>> The obvious way to tie it to a command mirrors how the QMP protocol ties
>>> responses to commands: by command ID.  The event either has to be sent
>>> just to the offending monitor (currently, all events are broadcast to
>>> all monitors), or include a suitable monitor ID.
>>>
>>> For non-command triggers, we'd have to invent some other tie.
>>>
>>> 3. Interface complexity
>>>
>>> Tying the event to some arbitrary trigger adds complexity.
>>>
>>> Do we need non-command triggers, and badly enough to justify the
>>> additional complexity?
>>>
>>> 4. Implementation complexity
>>>
>>> Emitting an event could be as simple as
>>>
>>>  qapi_event_send_deprecated(qmp_command_id(),
>>>     "Omitting 'filter-node-name'");
>>>
>>> where qmp_command_id() returns the ID of the currently executing
>>> command.  Making qmp_command_id() work is up to the QMP core.  Simple
>>> enough as long as each QMP command runs to completion before its monitor
>>> starts the next one.
>>>
>>> The event is "fire and forget".  There is no warning object propagated
>>> up the call chain into the QMP core like errors objects are.
>>>
>>> "Fire and forget" is ideal for letting arbitrary code decide "this is
>>> deprecated".
>>>
>>> Note the QAPI schema remains untouched.
>>>
>>> Unlike an event, which can be emitted anywhere, the success response
>>> gets built in the QMP core.  To have the core add deprecation info to
>>> it, we need to get the info to the core.
>>>
>>> If deprecation info originates in command code, like errors do, we need
>>> to propagate it up the call chain into the QMP core like errors.
>>>
>>> Propagating errors is painful.  It has caused massive churn all over the
>>> place.
>>>
>>> I don't think we can hitch deprecation info to the existing error
>>> propagation, since we need to take the success path back to the QMP
>>> core, not an error path.
>>>
>>> Propagating a second object for warnings... thanks, but no thanks.
>>>
>>
>> Probably the best argument against it. Fire-and-forget avoids the
>> problem. Events might work just fine, but the "tie" bit seems like a yak
>> in need of a shave.
>>
>>> The QMP core could provide a function for recording deprecation info for
>>> the currently executing QMP command.  Thi

Re: [Qemu-devel] Exposing feature deprecation to machine clients

[Philippe Mathieu-Daudé]

Hi Markus,

On 8/15/19 7:40 PM, John Snow wrote:

On 8/15/19 10:16 AM, Markus Armbruster wrote:

John Snow  writes:

[...]

I asked Markus this not too long ago; do we want to amend the QAPI
schema specification to allow commands to return with "Warning" strings,
or "Deprecated" stings to allow in-band deprecation notices for cases
like these?

example:

{ "return": {},
   "deprecated": True,
   "warning": "Omitting filter-node-name parameter is deprecated, it will
be required in the future"
}

There's no "error" key, so this should be recognized as success by
compatible clients, but they'll definitely see the extra information.


This is a compatible evolution of the QMP protocol.


Part of my motivation is to facilitate a more aggressive deprecation of
legacy features by ensuring that we are able to rigorously notify users
through any means that they need to adjust their scripts.


Yes, we should help libvirt etc. with detecting use of deprecated
features.  We discussed this at the KVM Forum 2018 BoF on deprecating
stuff.  Minutes:

 Message-ID: <87mur0ls8o@dusky.pond.sub.org>
 https://lists.nongnu.org/archive/html/qemu-devel/2018-10/msg05828.html

Last item is relevant here.

Adding deprecation information to QMP's success response belongs to "We
can also pass the buck to the next layer up", next to "emit a QMP
event".

Let's compare the two, i.e. "deprecation info in success response"
vs. "deprecation event".

1. Possible triggers

Anything we put in the success response should only ever apply to the
(successful) command.  So this one's limited to QMP commands.

A QMP event is not limited to QMP commands.  For instance, it could be
emitted for deprecated CLI features (long after the fact, in addition to
human-readable warnings on stderr), or when we detect use of a
deprecated feature only after we sent the success response, say in a
job.  Neither use case is particularly convincing.  Reporting use of
deprecated CLI in QMP feels like a work-around for the CLI's
machine-unfriendliness.  Job-like commands should really check their
arguments upfront.

2. Connection to trigger

Connecting responses to commands is the QMP protocol's responsibility.
Transmitting deprecation information in the response trivially ties it
to the offending command.

The QMP protocol doesn't tie events to anything.  Thus, a deprecation
event needs an event-specific tie to its trigger.

The obvious way to tie it to a command mirrors how the QMP protocol ties
responses to commands: by command ID.  The event either has to be sent
just to the offending monitor (currently, all events are broadcast to
all monitors), or include a suitable monitor ID.

For non-command triggers, we'd have to invent some other tie.

3. Interface complexity

Tying the event to some arbitrary trigger adds complexity.

Do we need non-command triggers, and badly enough to justify the
additional complexity?

4. Implementation complexity

Emitting an event could be as simple as

 qapi_event_send_deprecated(qmp_command_id(),
"Omitting 'filter-node-name'");

where qmp_command_id() returns the ID of the currently executing
command.  Making qmp_command_id() work is up to the QMP core.  Simple
enough as long as each QMP command runs to completion before its monitor
starts the next one.

The event is "fire and forget".  There is no warning object propagated
up the call chain into the QMP core like errors objects are.

"Fire and forget" is ideal for letting arbitrary code decide "this is
deprecated".

Note the QAPI schema remains untouched.

Unlike an event, which can be emitted anywhere, the success response
gets built in the QMP core.  To have the core add deprecation info to
it, we need to get the info to the core.

If deprecation info originates in command code, like errors do, we need
to propagate it up the call chain into the QMP core like errors.

Propagating errors is painful.  It has caused massive churn all over the
place.

I don't think we can hitch deprecation info to the existing error
propagation, since we need to take the success path back to the QMP
core, not an error path.

Propagating a second object for warnings... thanks, but no thanks.



Probably the best argument against it. Fire-and-forget avoids the
problem. Events might work just fine, but the "tie" bit seems like a yak
in need of a shave.


The QMP core could provide a function for recording deprecation info for
the currently executing QMP command.  This is how we used to record
errors in QMP commands, until Anthony rammed through what we have now.
The commit messages (e.g. d5ec4f27c38) provide no justification.  I
dimly recall adamant (oral?) claims that recording errors in the Monitor
object cannot work for us.

I smell a swamp.

Can we avoid plumbing deprecation info from command code to QMP core?
Only if the QMP core itself can recognize deprecated interfaces.  I
believe it can for the cases we can expose in introspecion.  Let me
explain.

K

Re: [PULL 0/3] Block patches for 4.2.0-rc0/4.1.1

[Peter Maydell]

On Thu, 7 Nov 2019 at 14:34, Max Reitz  wrote:
>
> The following changes since commit d0f90e1423b4f412adc620eee93e8bfef8af4117:
>
>   Merge remote-tracking branch 
> 'remotes/kraxel/tags/audio-20191106-pull-request' into staging (2019-11-07 
> 09:21:52 +)
>
> are available in the Git repository at:
>
>   https://github.com/XanClic/qemu.git tags/pull-block-2019-11-07
>
> for you to fetch changes up to b7cd2c11f76d27930f53d3cf26d7b695c78d613b:
>
>   iotests: Add test for 4G+ compressed qcow2 write (2019-11-07 14:37:46 +0100)
>
> 
> Block patches for 4.2.0-rc0/4.1.1:
> - Fix writing to compressed qcow2 images > 4 GB
> - Fix size sanity check for qcow2 bitmaps
>


Applied, thanks.

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

-- PMM

Re: [PATCH v3 01/22] iotests: s/qocw2/qcow2/

[Eric Blake]

On 11/7/19 10:36 AM, Max Reitz wrote:

Probably due to blind copy-pasting, we have several instances of "qocw2"
in our iotests.  Fix them.

Reported-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
  tests/qemu-iotests/060 | 2 +-


Reviewed-by: Eric Blake 

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

[PATCH v3 21/22] iotests: Disable data_file where it cannot be used

[Max Reitz]

Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/007 | 5 +++--
 tests/qemu-iotests/014 | 2 ++
 tests/qemu-iotests/015 | 5 +++--
 tests/qemu-iotests/026 | 5 -
 tests/qemu-iotests/029 | 5 +++--
 tests/qemu-iotests/031 | 6 +++---
 tests/qemu-iotests/036 | 5 +++--
 tests/qemu-iotests/039 | 3 +++
 tests/qemu-iotests/046 | 2 ++
 tests/qemu-iotests/048 | 2 ++
 tests/qemu-iotests/051 | 5 +++--
 tests/qemu-iotests/058 | 5 +++--
 tests/qemu-iotests/060 | 6 --
 tests/qemu-iotests/061 | 6 --
 tests/qemu-iotests/062 | 2 +-
 tests/qemu-iotests/066 | 4 +++-
 tests/qemu-iotests/067 | 6 --
 tests/qemu-iotests/068 | 5 +++--
 tests/qemu-iotests/071 | 3 +++
 tests/qemu-iotests/073 | 4 
 tests/qemu-iotests/074 | 2 ++
 tests/qemu-iotests/080 | 5 +++--
 tests/qemu-iotests/090 | 2 ++
 tests/qemu-iotests/098 | 6 --
 tests/qemu-iotests/099 | 3 ++-
 tests/qemu-iotests/103 | 5 +++--
 tests/qemu-iotests/108 | 6 --
 tests/qemu-iotests/112 | 5 +++--
 tests/qemu-iotests/114 | 2 ++
 tests/qemu-iotests/121 | 3 +++
 tests/qemu-iotests/138 | 3 +++
 tests/qemu-iotests/156 | 2 ++
 tests/qemu-iotests/176 | 7 +--
 tests/qemu-iotests/191 | 2 ++
 tests/qemu-iotests/201 | 6 +++---
 tests/qemu-iotests/214 | 3 ++-
 tests/qemu-iotests/217 | 3 ++-
 tests/qemu-iotests/220 | 5 +++--
 tests/qemu-iotests/243 | 6 --
 tests/qemu-iotests/244 | 5 +++--
 tests/qemu-iotests/250 | 2 ++
 tests/qemu-iotests/261 | 3 ++-
 tests/qemu-iotests/267 | 5 +++--
 43 files changed, 124 insertions(+), 53 deletions(-)

diff --git a/tests/qemu-iotests/007 b/tests/qemu-iotests/007
index 7d3544b479..160683adf8 100755
--- a/tests/qemu-iotests/007
+++ b/tests/qemu-iotests/007
@@ -41,8 +41,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto generic
 # refcount_bits must be at least 4 so we can create ten internal snapshots
-# (1 bit supports none, 2 bits support two, 4 bits support 14)
-_unsupported_imgopts 'refcount_bits=\(1\|2\)[^0-9]'
+# (1 bit supports none, 2 bits support two, 4 bits support 14);
+# snapshot are generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=\(1\|2\)[^0-9]' data_file
 
 echo
 echo "creating image"
diff --git a/tests/qemu-iotests/014 b/tests/qemu-iotests/014
index 2f728a1956..e1221c0fff 100755
--- a/tests/qemu-iotests/014
+++ b/tests/qemu-iotests/014
@@ -43,6 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
+# Compression and snapshots do not work with external data files
+_unsupported_imgopts data_file
 
 TEST_OFFSETS="0 4294967296"
 TEST_OPS="writev read write readv"
diff --git a/tests/qemu-iotests/015 b/tests/qemu-iotests/015
index eec5387f3d..4d8effd0ae 100755
--- a/tests/qemu-iotests/015
+++ b/tests/qemu-iotests/015
@@ -40,8 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # actually any format that supports snapshots
 _supported_fmt qcow2
 _supported_proto generic
-# Internal snapshots are (currently) impossible with refcount_bits=1
-_unsupported_imgopts 'refcount_bits=1[^0-9]'
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
 echo
 echo "creating image"
diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index 3430029ed6..a4aa74764f 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -49,7 +49,10 @@ _supported_cache_modes writethrough none
 # 32 and 64 bits do not work either, however, due to different leaked cluster
 # count on error.
 # Thus, the only remaining option is refcount_bits=16.
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+#
+# As for data_file, none of the refcount tests can work for it.
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)' \
+data_file
 
 echo "Errors while writing 128 kB"
 echo
diff --git a/tests/qemu-iotests/029 b/tests/qemu-iotests/029
index 9254ede5e5..2161a4b87a 100755
--- a/tests/qemu-iotests/029
+++ b/tests/qemu-iotests/029
@@ -42,8 +42,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto generic
 _unsupported_proto vxhs
-# Internal snapshots are (currently) impossible with refcount_bits=1
-_unsupported_imgopts 'refcount_bits=1[^0-9]'
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
 
 offset_size=24
 offset_l1_size=36
diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
index c44fcf91bb..646ecd593f 100755
--- a/tests/qemu-iotests/031
+++ b/tests/qemu-iotests/031
@@ -40,9 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto file
-# We want to test compat=0.10, which does not support refcount widths
-# other than 16
-_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
+# We want to test compat=0

[PATCH v3 20/22] iotests: Make 198 work with data_file

[Max Reitz]

We do not care about the json:{} filenames here, so we can just filter
them out and thus make the test work both with and without external data
files.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/198 | 6 --
 tests/qemu-iotests/198.out | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/198 b/tests/qemu-iotests/198
index c8f824cfae..fb0d5a29d3 100755
--- a/tests/qemu-iotests/198
+++ b/tests/qemu-iotests/198
@@ -92,13 +92,15 @@ echo
 echo "== checking image base =="
 $QEMU_IMG info --image-opts $IMGSPECBASE | _filter_img_info --format-specific \
 | sed -e "/^disk size:/ D" -e '/refcount bits:/ D' -e '/compat:/ D' \
-  -e '/lazy refcounts:/ D' -e '/corrupt:/ D'
+  -e '/lazy refcounts:/ D' -e '/corrupt:/ D' -e '/^\s*data file/ D' \
+| _filter_json_filename
 
 echo
 echo "== checking image layer =="
 $QEMU_IMG info --image-opts $IMGSPECLAYER | _filter_img_info --format-specific 
\
 | sed -e "/^disk size:/ D" -e '/refcount bits:/ D' -e '/compat:/ D' \
-  -e '/lazy refcounts:/ D' -e '/corrupt:/ D'
+  -e '/lazy refcounts:/ D' -e '/corrupt:/ D' -e '/^\s*data file/ D' \
+| _filter_json_filename
 
 
 # success, all done
diff --git a/tests/qemu-iotests/198.out b/tests/qemu-iotests/198.out
index e86b175e39..831ce3a289 100644
--- a/tests/qemu-iotests/198.out
+++ b/tests/qemu-iotests/198.out
@@ -32,7 +32,7 @@ read 16777216/16777216 bytes at offset 0
 16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 
 == checking image base ==
-image: json:{"encrypt.key-secret": "sec0", "driver": "IMGFMT", "file": 
{"driver": "file", "filename": "TEST_DIR/t.IMGFMT.base"}}
+image: json:{ /* filtered */ }
 file format: IMGFMT
 virtual size: 16 MiB (16777216 bytes)
 Format specific information:
@@ -74,7 +74,7 @@ Format specific information:
 master key iters: 1024
 
 == checking image layer ==
-image: json:{"encrypt.key-secret": "sec1", "driver": "IMGFMT", "file": 
{"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}}
+image: json:{ /* filtered */ }
 file format: IMGFMT
 virtual size: 16 MiB (16777216 bytes)
 backing file: TEST_DIR/t.IMGFMT.base
-- 
2.23.0

Re: [PATCH v2 20/21] iotests: Disable data_file where it cannot be used

[Max Reitz]

On 07.11.19 16:19, Maxim Levitsky wrote:
> On Thu, 2019-11-07 at 12:36 +0100, Max Reitz wrote:
>> On 06.11.19 16:52, Maxim Levitsky wrote:
>>> On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
 Signed-off-by: Max Reitz 
 ---
  tests/qemu-iotests/007 | 5 +++--
  tests/qemu-iotests/014 | 2 ++
  tests/qemu-iotests/015 | 5 +++--
  tests/qemu-iotests/026 | 5 -
  tests/qemu-iotests/029 | 5 +++--
  tests/qemu-iotests/031 | 6 +++---
  tests/qemu-iotests/036 | 5 +++--
  tests/qemu-iotests/039 | 3 +++
  tests/qemu-iotests/046 | 2 ++
  tests/qemu-iotests/048 | 2 ++
  tests/qemu-iotests/051 | 5 +++--
  tests/qemu-iotests/058 | 5 +++--
  tests/qemu-iotests/060 | 6 --
  tests/qemu-iotests/061 | 6 --
  tests/qemu-iotests/062 | 2 +-
  tests/qemu-iotests/066 | 2 +-
  tests/qemu-iotests/067 | 6 --
  tests/qemu-iotests/068 | 5 +++--
  tests/qemu-iotests/071 | 3 +++
  tests/qemu-iotests/073 | 2 ++
  tests/qemu-iotests/074 | 2 ++
  tests/qemu-iotests/080 | 5 +++--
  tests/qemu-iotests/090 | 2 ++
  tests/qemu-iotests/098 | 6 --
  tests/qemu-iotests/099 | 3 ++-
  tests/qemu-iotests/103 | 5 +++--
  tests/qemu-iotests/108 | 6 --
  tests/qemu-iotests/112 | 5 +++--
  tests/qemu-iotests/114 | 2 ++
  tests/qemu-iotests/121 | 3 +++
  tests/qemu-iotests/138 | 2 ++
  tests/qemu-iotests/156 | 2 ++
  tests/qemu-iotests/176 | 7 +--
  tests/qemu-iotests/191 | 2 ++
  tests/qemu-iotests/201 | 6 +++---
  tests/qemu-iotests/214 | 3 ++-
  tests/qemu-iotests/217 | 3 ++-
  tests/qemu-iotests/220 | 5 +++--
  tests/qemu-iotests/243 | 6 --
  tests/qemu-iotests/244 | 5 +++--
  tests/qemu-iotests/250 | 2 ++
  tests/qemu-iotests/267 | 5 +++--
  42 files changed, 117 insertions(+), 52 deletions(-)
>>
>> [...]
>>
 diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
 index c44fcf91bb..646ecd593f 100755
 --- a/tests/qemu-iotests/031
 +++ b/tests/qemu-iotests/031
 @@ -40,9 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  # This tests qcow2-specific low-level functionality
  _supported_fmt qcow2
  _supported_proto file
 -# We want to test compat=0.10, which does not support refcount widths
 -# other than 16
 -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
 +# We want to test compat=0.10, which does not support external data
 +# files or refcount widths other than 16
 +_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>>>
>>> This is maybe another reason to split this test for compat=0.10 and for 
>>> compat=1.1
>>> But still can be done later of course.
>>
>> Hm, but I don’t really think this test is important for external data
>> files.  There is no I/O.
> I guess both yes and no, the external data file is a header extension as well.

Yes, but the test already involves a header extension.

 diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
 index bbaf0ef45b..512598421c 100755
 --- a/tests/qemu-iotests/036
 +++ b/tests/qemu-iotests/036
 @@ -43,8 +43,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
  # This tests qcow2-specific low-level functionality
  _supported_fmt qcow2
  _supported_proto file
 -# Only qcow2v3 and later supports feature bits
 -_unsupported_imgopts 'compat=0.10'
 +# Only qcow2v3 and later supports feature bits;
 +# qcow2.py does not support external data files
>>>
>>> Minor nitpick, maybe tag this with TODO or so. No need to do now.
>>
>> Hm, well, and the same applies here.  (Just not a very important test.)
> Same here, in theory external data file is a feature, and it could
> 'interact' with other features, but most likely you are right here as well.

Well, but the test currently doesn’t involve any known feature bits.
It’s mostly about checking what our qcow2 driver does with unknown
feature bits.

(If it wanted to involve known feature bits, it could have easily used
e.g. the dirty feature.)

 diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
 index a8feb76184..2af6b74b41 100755
 --- a/tests/qemu-iotests/048
 +++ b/tests/qemu-iotests/048
 @@ -49,6 +49,8 @@ _compare()
  _supported_fmt raw qcow2 qed luks
  _supported_proto file
  _supported_os Linux
 +# Using 'cp' is incompatible with external data files
 +_unsupported_imgopts data_file
>>>
>>> You could compare the external files instead in theory *I think*.
>>> Another item on some TODO list I guess.
>>
>> This is a test of qemu-img compare, not of the image format.  So it
>> doesn’t make much sense to me to compare the external files, and also it
>> should be completely sufficient to run this test only without external
>> data files.
> Yes but on the other hand, its is kind of nice to test that it can compare 
> correctly
> two qcow2 files which have external data files

Re: [PATCH v3 21/22] iotests: Disable data_file where it cannot be used

[Max Reitz]

On 07.11.19 17:37, Max Reitz wrote:
> Signed-off-by: Max Reitz 
> ---
>  tests/qemu-iotests/007 | 5 +++--
>  tests/qemu-iotests/014 | 2 ++
>  tests/qemu-iotests/015 | 5 +++--
>  tests/qemu-iotests/026 | 5 -
>  tests/qemu-iotests/029 | 5 +++--
>  tests/qemu-iotests/031 | 6 +++---
>  tests/qemu-iotests/036 | 5 +++--
>  tests/qemu-iotests/039 | 3 +++
>  tests/qemu-iotests/046 | 2 ++
>  tests/qemu-iotests/048 | 2 ++
>  tests/qemu-iotests/051 | 5 +++--
>  tests/qemu-iotests/058 | 5 +++--
>  tests/qemu-iotests/060 | 6 --
>  tests/qemu-iotests/061 | 6 --
>  tests/qemu-iotests/062 | 2 +-
>  tests/qemu-iotests/066 | 4 +++-
>  tests/qemu-iotests/067 | 6 --
>  tests/qemu-iotests/068 | 5 +++--
>  tests/qemu-iotests/071 | 3 +++
>  tests/qemu-iotests/073 | 4 
>  tests/qemu-iotests/074 | 2 ++
>  tests/qemu-iotests/080 | 5 +++--
>  tests/qemu-iotests/090 | 2 ++
>  tests/qemu-iotests/098 | 6 --
>  tests/qemu-iotests/099 | 3 ++-
>  tests/qemu-iotests/103 | 5 +++--
>  tests/qemu-iotests/108 | 6 --
>  tests/qemu-iotests/112 | 5 +++--
>  tests/qemu-iotests/114 | 2 ++
>  tests/qemu-iotests/121 | 3 +++
>  tests/qemu-iotests/138 | 3 +++
>  tests/qemu-iotests/156 | 2 ++
>  tests/qemu-iotests/176 | 7 +--
>  tests/qemu-iotests/191 | 2 ++
>  tests/qemu-iotests/201 | 6 +++---
>  tests/qemu-iotests/214 | 3 ++-
>  tests/qemu-iotests/217 | 3 ++-
>  tests/qemu-iotests/220 | 5 +++--
>  tests/qemu-iotests/243 | 6 --
>  tests/qemu-iotests/244 | 5 +++--
>  tests/qemu-iotests/250 | 2 ++
>  tests/qemu-iotests/261 | 3 ++-
>  tests/qemu-iotests/267 | 5 +++--
>  43 files changed, 124 insertions(+), 53 deletions(-)

[...]

> diff --git a/tests/qemu-iotests/067 b/tests/qemu-iotests/067
> index 926c79b37c..3bc6e719eb 100755
> --- a/tests/qemu-iotests/067
> +++ b/tests/qemu-iotests/067
> @@ -32,8 +32,10 @@ status=1   # failure is the default!
>  
>  _supported_fmt qcow2
>  _supported_proto file
> -# Because anything other than 16 would change the output of query-block
> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> +# Because anything other than 16 would change the output of query-block,
> +# and external data files would change the output of
> +# query-named-block-ndoes

s/ndoes/nodes/...



signature.asc
Description: OpenPGP digital signature

[PATCH v3 19/22] iotests: Make 137 work with data_file

[Max Reitz]

When using an external data file, there are no refcounts for data
clusters.  We thus have to adjust the corruption test in this patch to
not be based around a data cluster allocation, but the L2 table
allocation (L2 tables are still refcounted with external data files).

Furthermore, we should not print qcow2.py's list of incompatible
features because it differs depending on whether there is an external
data file or not.

With those two changes, the test will work both with and without
external data files (once that options works with the iotests at all).

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/137 | 15 +++
 tests/qemu-iotests/137.out |  6 ++
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
index 6cf2997577..7ae86892f7 100755
--- a/tests/qemu-iotests/137
+++ b/tests/qemu-iotests/137
@@ -138,14 +138,21 @@ $QEMU_IO \
 "$TEST_IMG" 2>&1 | _filter_qemu_io
 
 # The dirty bit must not be set
-$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+# (Filter the external data file bit)
+if $PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features \
+| grep -q '\<0\>'
+then
+echo 'ERROR: Dirty bit set'
+else
+echo 'OK: Dirty bit not set'
+fi
 
 # Similarly we can test whether corruption detection has been enabled:
-# Create L1/L2, overwrite first entry in refcount block, allocate something.
+# Create L1, overwrite refcounts, force allocation of L2 by writing
+# data.
 # Disabling the checks should fail, so the corruption must be detected.
 _make_test_img 64M
-$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
-poke_file "$TEST_IMG" "$((0x2))" "\x00\x00"
+poke_file "$TEST_IMG" "$((0x2))" "\x00\x00\x00\x00\x00\x00\x00\x00"
 $QEMU_IO \
 -c "reopen -o overlap-check=none,lazy-refcounts=42" \
 -c "write 64k 64k" \
diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
index bd4523a853..86377c80cd 100644
--- a/tests/qemu-iotests/137.out
+++ b/tests/qemu-iotests/137.out
@@ -36,11 +36,9 @@ qemu-io: Unsupported value 'blubb' for qcow2 option 
'overlap-check'. Allowed are
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 ./common.rc: Killed  ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" 
_qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
-incompatible_features []
+OK: Dirty bit not set
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-wrote 65536/65536 bytes at offset 0
-64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qemu-io: Parameter 'lazy-refcounts' expects 'on' or 'off'
-qcow2: Marking image as corrupt: Preventing invalid write on metadata 
(overlaps with qcow2_header); further corruption events will be suppressed
+qcow2: Marking image as corrupt: Preventing invalid allocation of L2 table at 
offset 0; further corruption events will be suppressed
 write failed: Input/output error
 *** done
-- 
2.23.0

[PATCH v3 16/22] iotests: Avoid cp/mv of test images

[Max Reitz]

This will not work with external data files, so try to get tests working
without it as far as possible.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/063 | 12 
 tests/qemu-iotests/063.out |  3 ++-
 tests/qemu-iotests/085 |  9 +++--
 tests/qemu-iotests/085.out |  8 
 4 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/tests/qemu-iotests/063 b/tests/qemu-iotests/063
index eef2b8a534..c750b3806e 100755
--- a/tests/qemu-iotests/063
+++ b/tests/qemu-iotests/063
@@ -51,15 +51,13 @@ _unsupported_imgopts "subformat=monolithicFlat" \
 _make_test_img 4M
 
 echo "== Testing conversion with -n fails with no target file =="
-# check .orig file does not exist
-rm -f "$TEST_IMG.orig"
 if $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG.orig" 
>/dev/null 2>&1; then
 exit 1
 fi
 
 echo "== Testing conversion with -n succeeds with a target file =="
-rm -f "$TEST_IMG.orig"
-cp "$TEST_IMG" "$TEST_IMG.orig"
+_rm_test_img "$TEST_IMG.orig"
+TEST_IMG="$TEST_IMG.orig" _make_test_img 4M
 if ! $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG.orig" ; 
then
 exit 1
 fi
@@ -85,10 +83,8 @@ fi
 _check_test_img
 
 echo "== Testing conversion to a smaller file fails =="
-rm -f "$TEST_IMG.orig"
-mv "$TEST_IMG" "$TEST_IMG.orig"
-_make_test_img 2M
-if $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n "$TEST_IMG.orig" "$TEST_IMG" 
>/dev/null 2>&1; then
+TEST_IMG="$TEST_IMG.target" _make_test_img 2M
+if $QEMU_IMG convert -f $IMGFMT -O $IMGFMT -n "$TEST_IMG" "$TEST_IMG.target" 
>/dev/null 2>&1; then
 exit 1
 fi
 
diff --git a/tests/qemu-iotests/063.out b/tests/qemu-iotests/063.out
index 7b691b2c9e..890b719bf0 100644
--- a/tests/qemu-iotests/063.out
+++ b/tests/qemu-iotests/063.out
@@ -2,11 +2,12 @@ QA output created by 063
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4194304
 == Testing conversion with -n fails with no target file ==
 == Testing conversion with -n succeeds with a target file ==
+Formatting 'TEST_DIR/t.IMGFMT.orig', fmt=IMGFMT size=4194304
 == Testing conversion to raw is the same after conversion with -n ==
 == Testing conversion back to original format ==
 No errors were found on the image.
 == Testing conversion to a smaller file fails ==
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
+Formatting 'TEST_DIR/t.IMGFMT.target', fmt=IMGFMT size=2097152
 == Regression testing for copy offloading bug ==
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
 Formatting 'TEST_DIR/t.IMGFMT.target', fmt=IMGFMT size=1048576
diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index bbea1252d2..46981dbb64 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -105,8 +105,7 @@ add_snapshot_image()
 {
 base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
 snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
-_make_test_img -u -b "${base_image}" "$size"
-mv "${TEST_IMG}" "${snapshot_file}"
+TEST_IMG=$snapshot_file _make_test_img -u -b "${base_image}" "$size"
 do_blockdev_add "$1" "'backing': null, " "${snapshot_file}"
 }
 
@@ -122,10 +121,8 @@ blockdev_snapshot()
 
 size=128M
 
-_make_test_img $size
-mv "${TEST_IMG}" "${TEST_IMG}.1"
-_make_test_img $size
-mv "${TEST_IMG}" "${TEST_IMG}.2"
+TEST_IMG="$TEST_IMG.1" _make_test_img $size
+TEST_IMG="$TEST_IMG.2" _make_test_img $size
 
 echo
 echo === Running QEMU ===
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 2a5f256cd3..313198f182 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -1,6 +1,6 @@
 QA output created by 085
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT.1', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT.2', fmt=IMGFMT size=134217728
 
 === Running QEMU ===
 
@@ -55,10 +55,10 @@ Formatting 'TEST_DIR/10-snapshot-v1.qcow2', fmt=qcow2 
size=134217728 backing_fil
 
 === Create a couple of snapshots using blockdev-snapshot ===
 
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/10-snapshot-v0.IMGFMT
+Formatting 'TEST_DIR/11-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/10-snapshot-v0.IMGFMT
 {"return": {}}
 {"return": {}}
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/11-snapshot-v0.IMGFMT
+Formatting 'TEST_DIR/12-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 
backing_file=TEST_DIR/11-snapshot-v0.IMGFMT
 {"return": {}}
 {"return": {}}
 
-- 
2.23.0

[PATCH v3 18/22] iotests: Make 110 work with data_file

[Max Reitz]

The only difference is that the json:{} filename of the image looks
different.  We actually do not care about that filename in this test, we
are only interested in (1) that there is a json:{} filename, and (2)
whether the backing filename can be constructed.

So just filter out the json:{} data, thus making this test pass both
with and without data_file.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/110 | 7 +--
 tests/qemu-iotests/110.out | 4 ++--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/110 b/tests/qemu-iotests/110
index f78df0e6e1..139c02c2cf 100755
--- a/tests/qemu-iotests/110
+++ b/tests/qemu-iotests/110
@@ -67,6 +67,7 @@ echo
 # Across blkdebug without a config file, you cannot reconstruct filenames, so
 # qemu is incapable of knowing the directory of the top image from the filename
 # alone. However, using bdrv_dirname(), it should still work.
+# (Filter out the json:{} filename so this test works with external data files)
 TEST_IMG="json:{
 'driver': '$IMGFMT',
 'file': {
@@ -82,7 +83,8 @@ TEST_IMG="json:{
 }
 ]
 }
-}" _img_info | _filter_img_info | grep -v 'backing file format'
+}" _img_info | _filter_img_info | grep -v 'backing file format' \
+| _filter_json_filename
 
 echo
 echo '=== Backing name is always relative to the backed image ==='
@@ -114,7 +116,8 @@ TEST_IMG="json:{
 }
 ]
 }
-}" _img_info | _filter_img_info | grep -v 'backing file format'
+}" _img_info | _filter_img_info | grep -v 'backing file format' \
+| _filter_json_filename
 
 
 # success, all done
diff --git a/tests/qemu-iotests/110.out b/tests/qemu-iotests/110.out
index f60b26390e..f835553a99 100644
--- a/tests/qemu-iotests/110.out
+++ b/tests/qemu-iotests/110.out
@@ -11,7 +11,7 @@ backing file: t.IMGFMT.base (actual path: 
TEST_DIR/t.IMGFMT.base)
 
 === Non-reconstructable filename ===
 
-image: json:{"driver": "IMGFMT", "file": {"set-state.0.event": "read_aio", 
"image": {"driver": "file", "filename": "TEST_DIR/t.IMGFMT"}, "driver": 
"blkdebug", "set-state.0.new_state": 42}}
+image: json:{ /* filtered */ }
 file format: IMGFMT
 virtual size: 64 MiB (67108864 bytes)
 backing file: t.IMGFMT.base (actual path: TEST_DIR/t.IMGFMT.base)
@@ -22,7 +22,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
backing_file=t.IMGFMT.b
 
 === Nodes without a common directory ===
 
-image: json:{"driver": "IMGFMT", "file": {"children": [{"driver": "file", 
"filename": "TEST_DIR/t.IMGFMT"}, {"driver": "file", "filename": 
"TEST_DIR/t.IMGFMT.copy"}], "driver": "quorum", "vote-threshold": 1}}
+image: json:{ /* filtered */ }
 file format: IMGFMT
 virtual size: 64 MiB (67108864 bytes)
 backing file: t.IMGFMT.base (cannot determine actual path)
-- 
2.23.0

[PATCH v3 13/22] iotests: Drop IMGOPTS use in 267

[Max Reitz]

Overwriting IMGOPTS means ignoring all user-supplied options, which is
not what we want.  Replace the current IMGOPTS use by a new BACKING_FILE
variable.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/267 | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/267 b/tests/qemu-iotests/267
index 170e173c0a..f54262a4ad 100755
--- a/tests/qemu-iotests/267
+++ b/tests/qemu-iotests/267
@@ -68,7 +68,11 @@ size=128M
 
 run_test()
 {
-_make_test_img $size
+if [ -n "$BACKING_FILE" ]; then
+_make_test_img -b "$BACKING_FILE" $size
+else
+_make_test_img $size
+fi
 printf "savevm snap0\ninfo snapshots\nloadvm snap0\n" | run_qemu "$@" | 
_filter_date
 }
 
@@ -119,12 +123,12 @@ echo
 
 TEST_IMG="$TEST_IMG.base" _make_test_img $size
 
-IMGOPTS="backing_file=$TEST_IMG.base" \
+BACKING_FILE="$TEST_IMG.base" \
 run_test -blockdev 
driver=file,filename="$TEST_IMG.base",node-name=backing-file \
  -blockdev driver=file,filename="$TEST_IMG",node-name=file \
  -blockdev driver=$IMGFMT,file=file,backing=backing-file,node-name=fmt
 
-IMGOPTS="backing_file=$TEST_IMG.base" \
+BACKING_FILE="$TEST_IMG.base" \
 run_test -blockdev 
driver=file,filename="$TEST_IMG.base",node-name=backing-file \
  -blockdev driver=$IMGFMT,file=backing-file,node-name=backing-fmt \
  -blockdev driver=file,filename="$TEST_IMG",node-name=file \
@@ -141,7 +145,7 @@ echo
 echo "=== -blockdev with NBD server on the backing file ==="
 echo
 
-IMGOPTS="backing_file=$TEST_IMG.base" _make_test_img $size
+_make_test_img -b "$TEST_IMG.base" $size
 cat <

[PATCH v3 12/22] iotests: Replace IMGOPTS='' by --no-opts

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/071 | 4 ++--
 tests/qemu-iotests/174 | 2 +-
 tests/qemu-iotests/178 | 4 ++--
 tests/qemu-iotests/197 | 4 ++--
 tests/qemu-iotests/215 | 4 ++--
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
index fab52b..4e31943244 100755
--- a/tests/qemu-iotests/071
+++ b/tests/qemu-iotests/071
@@ -58,7 +58,7 @@ echo
 echo "=== Testing blkverify through filename ==="
 echo
 
-TEST_IMG="$TEST_IMG.base" IMGOPTS="" IMGFMT="raw" _make_test_img $IMG_SIZE |\
+TEST_IMG="$TEST_IMG.base" IMGFMT="raw" _make_test_img --no-opts $IMG_SIZE |\
 _filter_imgfmt
 _make_test_img $IMG_SIZE
 $QEMU_IO -c "open -o 
driver=raw,file.driver=blkverify,file.raw.filename=$TEST_IMG.base $TEST_IMG" \
@@ -73,7 +73,7 @@ echo
 echo "=== Testing blkverify through file blockref ==="
 echo
 
-TEST_IMG="$TEST_IMG.base" IMGOPTS="" IMGFMT="raw" _make_test_img $IMG_SIZE |\
+TEST_IMG="$TEST_IMG.base" IMGFMT="raw" _make_test_img --no-opts $IMG_SIZE |\
 _filter_imgfmt
 _make_test_img $IMG_SIZE
 $QEMU_IO -c "open -o 
driver=raw,file.driver=blkverify,file.raw.filename=$TEST_IMG.base,file.test.driver=$IMGFMT,file.test.file.filename=$TEST_IMG"
 \
diff --git a/tests/qemu-iotests/174 b/tests/qemu-iotests/174
index 0a952a73fd..e2f14a38c6 100755
--- a/tests/qemu-iotests/174
+++ b/tests/qemu-iotests/174
@@ -40,7 +40,7 @@ _unsupported_fmt raw
 
 
 size=256K
-IMGFMT=raw IMGKEYSECRET= IMGOPTS= _make_test_img $size | _filter_imgfmt
+IMGFMT=raw IMGKEYSECRET= _make_test_img --no-opts $size | _filter_imgfmt
 
 echo
 echo "== reading wrong format should fail =="
diff --git a/tests/qemu-iotests/178 b/tests/qemu-iotests/178
index 21231cadd3..75b5e8f314 100755
--- a/tests/qemu-iotests/178
+++ b/tests/qemu-iotests/178
@@ -62,8 +62,8 @@ $QEMU_IMG measure -O foo "$TEST_IMG" # unknown image file 
format
 
 make_test_img_with_fmt() {
 # Shadow global variables within this function
-local IMGFMT="$1" IMGOPTS=""
-_make_test_img "$2"
+local IMGFMT="$1"
+_make_test_img --no-opts "$2"
 }
 
 qemu_io_with_fmt() {
diff --git a/tests/qemu-iotests/197 b/tests/qemu-iotests/197
index 1d4f6786db..4d3d08ad6f 100755
--- a/tests/qemu-iotests/197
+++ b/tests/qemu-iotests/197
@@ -66,8 +66,8 @@ if [ "$IMGFMT" = "vpc" ]; then
 fi
 _make_test_img 4G
 $QEMU_IO -c "write -P 55 3G 1k" "$TEST_IMG" | _filter_qemu_io
-IMGPROTO=file IMGFMT=qcow2 IMGOPTS= TEST_IMG_FILE="$TEST_WRAP" \
-_make_test_img -F "$IMGFMT" -b "$TEST_IMG" | _filter_img_create
+IMGPROTO=file IMGFMT=qcow2 TEST_IMG_FILE="$TEST_WRAP" \
+_make_test_img --no-opts -F "$IMGFMT" -b "$TEST_IMG" | _filter_img_create
 $QEMU_IO -f qcow2 -c "write -z -u 1M 64k" "$TEST_WRAP" | _filter_qemu_io
 
 # Ensure that a read of two clusters, but where one is already allocated,
diff --git a/tests/qemu-iotests/215 b/tests/qemu-iotests/215
index 2eb377d682..55a1874dcd 100755
--- a/tests/qemu-iotests/215
+++ b/tests/qemu-iotests/215
@@ -63,8 +63,8 @@ if [ "$IMGFMT" = "vpc" ]; then
 fi
 _make_test_img 4G
 $QEMU_IO -c "write -P 55 3G 1k" "$TEST_IMG" | _filter_qemu_io
-IMGPROTO=file IMGFMT=qcow2 IMGOPTS= TEST_IMG_FILE="$TEST_WRAP" \
-_make_test_img -F "$IMGFMT" -b "$TEST_IMG" | _filter_img_create
+IMGPROTO=file IMGFMT=qcow2 TEST_IMG_FILE="$TEST_WRAP" \
+_make_test_img --no-opts -F "$IMGFMT" -b "$TEST_IMG" | _filter_img_create
 $QEMU_IO -f qcow2 -c "write -z -u 1M 64k" "$TEST_WRAP" | _filter_qemu_io
 
 # Ensure that a read of two clusters, but where one is already allocated,
-- 
2.23.0

[PATCH v3 22/22] iotests: Allow check -o data_file

[Max Reitz]

The problem with allowing the data_file option is that you want to use a
different data file per image used in the test.  Therefore, we need to
allow patterns like -o data_file='$TEST_IMG.data_file'.

Then, we need to filter it out from qemu-img map, qemu-img create, and
remove the data file in _rm_test_img.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/common.filter | 23 +--
 tests/qemu-iotests/common.rc | 22 +-
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 8a0169f19a..6902f9d94b 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -122,7 +122,13 @@ _filter_actual_image_size()
 # replace driver-specific options in the "Formatting..." line
 _filter_img_create()
 {
-$SED -e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
+data_file_filter=()
+if data_file=$(_get_data_file "$TEST_IMG"); then
+data_file_filter=(-e "s# data_file=$data_file##")
+fi
+
+$SED "${data_file_filter[@]}" \
+-e "s#$REMOTE_TEST_DIR#TEST_DIR#g" \
 -e "s#$IMGPROTO:$TEST_DIR#TEST_DIR#g" \
 -e "s#$TEST_DIR#TEST_DIR#g" \
 -e "s#$SOCK_DIR#SOCK_DIR#g" \
@@ -207,9 +213,22 @@ _filter_img_info()
 # human and json output
 _filter_qemu_img_map()
 {
+# Assuming the data_file value in $IMGOPTS contains a '$TEST_IMG',
+# create a filter that replaces the data file name by $TEST_IMG.
+# Example:
+#   In $IMGOPTS: 'data_file=$TEST_IMG.data_file'
+#   Then data_file_pattern == '\(.*\).data_file'
+#   And  data_file_filter  == -e 's#\(.*\).data_file#\1#
+data_file_filter=()
+if data_file_pattern=$(_get_data_file '\\(.*\\)'); then
+data_file_filter=(-e "s#$data_file_pattern#\\1#")
+fi
+
 $SED -e 's/\([0-9a-fx]* *[0-9a-fx]* *\)[0-9a-fx]* */\1/g' \
 -e 's/"offset": [0-9]\+/"offset": OFFSET/g' \
--e 's/Mapped to *//' | _filter_testdir | _filter_imgfmt
+-e 's/Mapped to *//' \
+"${data_file_filter[@]}" \
+| _filter_testdir | _filter_imgfmt
 }
 
 _filter_nbd()
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index a623485f96..a07a10a305 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -297,6 +297,20 @@ _stop_nbd_server()
 fi
 }
 
+# Gets the data_file value from IMGOPTS and replaces the '$TEST_IMG'
+# pattern by '$1'
+# Caution: The replacement is done with sed, so $1 must be escaped
+#  properly.  (The delimiter is '#'.)
+_get_data_file()
+{
+if ! echo "$IMGOPTS" | grep -q 'data_file='; then
+return 1
+fi
+
+echo "$IMGOPTS" | sed -e 's/.*data_file=\([^,]*\).*/\1/' \
+| sed -e "s#\\\$TEST_IMG#$1#"
+}
+
 _make_test_img()
 {
 # extra qemu-img options can be added by tests
@@ -317,7 +331,8 @@ _make_test_img()
 fi
 
 if [ -n "$IMGOPTS" ]; then
-optstr=$(_optstr_add "$optstr" "$IMGOPTS")
+imgopts_expanded=$(echo "$IMGOPTS" | sed -e 
"s#\\\$TEST_IMG#$img_name#")
+optstr=$(_optstr_add "$optstr" "$imgopts_expanded")
 fi
 if [ -n "$IMGKEYSECRET" ]; then
 object_options="--object secret,id=keysec0,data=$IMGKEYSECRET"
@@ -396,6 +411,11 @@ _rm_test_img()
 # Remove all the extents for vmdk
 "$QEMU_IMG" info "$img" 2>/dev/null | grep 'filename:' | cut -f 2 -d: \
 | xargs -I {} rm -f "{}"
+elif [ "$IMGFMT" = "qcow2" ]; then
+# Remove external data file
+if data_file=$(_get_data_file "$img"); then
+rm -f "$data_file"
+fi
 fi
 rm -f "$img"
 }
-- 
2.23.0

[PATCH v3 11/22] iotests: Replace IMGOPTS= by -o

[Max Reitz]

Tests should not overwrite all user-supplied image options, but only add
to it (which will effectively overwrite conflicting values).  Accomplish
this by passing options to _make_test_img via -o instead of $IMGOPTS.

For some tests, there is no functional change because they already only
appended options to IMGOPTS.  For these, this patch is just a
simplification.

For others, this is a change, so they now heed user-specified $IMGOPTS.
Some of those tests do not work with all image options, though, so we
need to disable them accordingly.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/031 |  9 ---
 tests/qemu-iotests/039 | 24 ++
 tests/qemu-iotests/059 | 18 ++---
 tests/qemu-iotests/060 |  6 ++---
 tests/qemu-iotests/061 | 57 ++
 tests/qemu-iotests/079 |  3 +--
 tests/qemu-iotests/106 |  2 +-
 tests/qemu-iotests/108 |  2 +-
 tests/qemu-iotests/112 | 32 
 tests/qemu-iotests/115 |  3 +--
 tests/qemu-iotests/121 |  6 ++---
 tests/qemu-iotests/125 |  2 +-
 tests/qemu-iotests/137 |  2 +-
 tests/qemu-iotests/138 |  3 +--
 tests/qemu-iotests/175 |  2 +-
 tests/qemu-iotests/190 |  2 +-
 tests/qemu-iotests/191 |  3 +--
 tests/qemu-iotests/220 |  4 ++-
 tests/qemu-iotests/243 |  6 +++--
 tests/qemu-iotests/244 | 10 +---
 tests/qemu-iotests/250 |  3 +--
 tests/qemu-iotests/265 |  2 +-
 22 files changed, 100 insertions(+), 101 deletions(-)

diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
index a3c25ec237..c44fcf91bb 100755
--- a/tests/qemu-iotests/031
+++ b/tests/qemu-iotests/031
@@ -40,19 +40,22 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto file
+# We want to test compat=0.10, which does not support refcount widths
+# other than 16
+_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
 
 CLUSTER_SIZE=65536
 
 # qcow2.py output depends on the exact options used, so override the command
 # line here as an exception
-for IMGOPTS in "compat=0.10" "compat=1.1"; do
+for compat in "compat=0.10" "compat=1.1"; do
 
 echo
-echo = Testing with -o $IMGOPTS =
+echo = Testing with -o $compat =
 echo
 echo === Create image with unknown header extension ===
 echo
-_make_test_img 64M
+_make_test_img -o $compat 64M
 $PYTHON qcow2.py "$TEST_IMG" add-header-ext 0x12345678 "This is a test 
header extension"
 $PYTHON qcow2.py "$TEST_IMG" dump-header
 _check_test_img
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 325da63a4c..99563bf126 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -50,8 +50,7 @@ size=128M
 echo
 echo "== Checking that image is clean on shutdown =="
 
-IMGOPTS="compat=1.1,lazy_refcounts=on"
-_make_test_img $size
+_make_test_img -o "compat=1.1,lazy_refcounts=on" $size
 
 $QEMU_IO -c "write -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
 
@@ -62,8 +61,7 @@ _check_test_img
 echo
 echo "== Creating a dirty image file =="
 
-IMGOPTS="compat=1.1,lazy_refcounts=on"
-_make_test_img $size
+_make_test_img -o "compat=1.1,lazy_refcounts=on" $size
 
 _NO_VALGRIND \
 $QEMU_IO -c "write -P 0x5a 0 512" \
@@ -98,8 +96,7 @@ $QEMU_IO -c "read -P 0x5a 0 512" "$TEST_IMG" | _filter_qemu_io
 echo
 echo "== Opening a dirty image read/write should repair it =="
 
-IMGOPTS="compat=1.1,lazy_refcounts=on"
-_make_test_img $size
+_make_test_img -o "compat=1.1,lazy_refcounts=on" $size
 
 _NO_VALGRIND \
 $QEMU_IO -c "write -P 0x5a 0 512" \
@@ -117,8 +114,7 @@ $PYTHON qcow2.py "$TEST_IMG" dump-header | grep 
incompatible_features
 echo
 echo "== Creating an image file with lazy_refcounts=off =="
 
-IMGOPTS="compat=1.1,lazy_refcounts=off"
-_make_test_img $size
+_make_test_img -o "compat=1.1,lazy_refcounts=off" $size
 
 _NO_VALGRIND \
 $QEMU_IO -c "write -P 0x5a 0 512" \
@@ -132,11 +128,9 @@ _check_test_img
 echo
 echo "== Committing to a backing file with lazy_refcounts=on =="
 
-IMGOPTS="compat=1.1,lazy_refcounts=on"
-TEST_IMG="$TEST_IMG".base _make_test_img $size
+TEST_IMG="$TEST_IMG".base _make_test_img -o "compat=1.1,lazy_refcounts=on" 
$size
 
-IMGOPTS="compat=1.1,lazy_refcounts=on,backing_file=$TEST_IMG.base"
-_make_test_img $size
+_make_test_img -o "compat=1.1,lazy_refcounts=on,backing_file=$TEST_IMG.base" 
$size
 
 $QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io
 $QEMU_IMG commit "$TEST_IMG"
@@ -151,8 +145,7 @@ TEST_IMG="$TEST_IMG".base _check_test_img
 echo
 echo "== Changing lazy_refcounts setting at runtime =="
 
-IMGOPTS="compat=1.1,lazy_refcounts=off"
-_make_test_img $size
+_make_test_img -o "compat=1.1,lazy_refcounts=off" $size
 
 _NO_VALGRIND \
 $QEMU_IO -c "reopen -o lazy-refcounts=on" \
@@ -164,8 +157,7 @@ $QEMU_IO -c "reopen -o lazy-refcounts=on" \
 $PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
 _check_test_img
 
-IMGOPTS="compat=1.1,lazy_refcounts=on"
-_make_test_img $size

[PATCH v3 14/22] iotests: Avoid qemu-img create

[Max Reitz]

Use _make_test_img whenever possible.  This way, we will not ignore
user-specified image options.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/094 | 2 +-
 tests/qemu-iotests/111 | 3 +--
 tests/qemu-iotests/123 | 2 +-
 tests/qemu-iotests/153 | 2 +-
 tests/qemu-iotests/200 | 4 ++--
 5 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/094 b/tests/qemu-iotests/094
index 9343e09492..d645952d54 100755
--- a/tests/qemu-iotests/094
+++ b/tests/qemu-iotests/094
@@ -45,7 +45,7 @@ _supported_proto nbd
 _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
 
 _make_test_img 64M
-$QEMU_IMG create -f $IMGFMT "$TEST_DIR/source.$IMGFMT" 64M | _filter_img_create
+TEST_IMG_FILE="$TEST_DIR/source.$IMGFMT" IMGPROTO=file _make_test_img 64M
 
 _launch_qemu -drive if=none,id=src,file="$TEST_DIR/source.$IMGFMT",format=raw \
  -nodefaults
diff --git a/tests/qemu-iotests/111 b/tests/qemu-iotests/111
index 490a5bbcb5..3b43d1bd83 100755
--- a/tests/qemu-iotests/111
+++ b/tests/qemu-iotests/111
@@ -41,8 +41,7 @@ _supported_fmt qed qcow qcow2 vmdk
 _supported_proto file
 _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
 
-$QEMU_IMG create -f $IMGFMT -b "$TEST_IMG.inexistent" "$TEST_IMG" 2>&1 \
-| _filter_testdir | _filter_imgfmt
+_make_test_img -b "$TEST_IMG.inexistent"
 
 # success, all done
 echo '*** done'
diff --git a/tests/qemu-iotests/123 b/tests/qemu-iotests/123
index d33950eb54..74d40d0478 100755
--- a/tests/qemu-iotests/123
+++ b/tests/qemu-iotests/123
@@ -44,7 +44,7 @@ _supported_os Linux
 SRC_IMG="$TEST_DIR/source.$IMGFMT"
 
 _make_test_img 1M
-$QEMU_IMG create -f $IMGFMT "$SRC_IMG" 1M | _filter_img_create
+TEST_IMG_FILE=$SRC_IMG IMGPROTO=file _make_test_img 1M
 
 $QEMU_IO -c 'write -P 42 0 1M' "$SRC_IMG" | _filter_qemu_io
 
diff --git a/tests/qemu-iotests/153 b/tests/qemu-iotests/153
index c969a1a16f..e59090259c 100755
--- a/tests/qemu-iotests/153
+++ b/tests/qemu-iotests/153
@@ -98,7 +98,7 @@ for opts1 in "" "read-only=on" "read-only=on,force-share=on"; 
do
 
 echo
 echo "== Creating test image =="
-$QEMU_IMG create -f $IMGFMT "${TEST_IMG}" -b ${TEST_IMG}.base | 
_filter_img_create
+_make_test_img -b "${TEST_IMG}.base"
 
 echo
 echo "== Launching QEMU, opts: '$opts1' =="
diff --git a/tests/qemu-iotests/200 b/tests/qemu-iotests/200
index 72d431f251..d904885136 100755
--- a/tests/qemu-iotests/200
+++ b/tests/qemu-iotests/200
@@ -46,8 +46,8 @@ _supported_proto file
 BACKING_IMG="${TEST_DIR}/backing.img"
 TEST_IMG="${TEST_DIR}/test.img"
 
-${QEMU_IMG} create -f $IMGFMT "${BACKING_IMG}" 512M | _filter_img_create
-${QEMU_IMG} create -f $IMGFMT -F $IMGFMT "${TEST_IMG}" -b "${BACKING_IMG}" 
512M | _filter_img_create
+TEST_IMG="$BACKING_IMG" _make_test_img 512M
+_make_test_img -F $IMGFMT -b "$BACKING_IMG" 512M
 
 ${QEMU_IO} -c "write -P 0xa5 512 300M" "${BACKING_IMG}" | _filter_qemu_io
 
-- 
2.23.0

[PATCH v3 06/22] iotests: Replace IMGOPTS by _unsupported_imgopts

[Max Reitz]

Some tests require compat=1.1 and thus set IMGOPTS='compat=1.1'
globally.  That is not how it should be done; instead, they should
simply set _unsupported_imgopts to compat=0.10 (compat=1.1 is the
default anyway).

This makes the tests heed user-specified $IMGOPTS.  Some do not work
with all image options, though, so we need to disable them accordingly.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/036 | 3 +--
 tests/qemu-iotests/060 | 4 ++--
 tests/qemu-iotests/062 | 3 ++-
 tests/qemu-iotests/066 | 3 ++-
 tests/qemu-iotests/068 | 3 ++-
 tests/qemu-iotests/098 | 4 ++--
 6 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
index 5f929ad3be..bbaf0ef45b 100755
--- a/tests/qemu-iotests/036
+++ b/tests/qemu-iotests/036
@@ -43,9 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto file
-
 # Only qcow2v3 and later supports feature bits
-IMGOPTS="compat=1.1"
+_unsupported_imgopts 'compat=0.10'
 
 echo
 echo === Image with unknown incompatible feature bit ===
diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index 725e58a5a5..e0117aa4ae 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -48,6 +48,8 @@ _filter_io_error()
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
+# These tests only work for compat=1.1 images with refcount_bits=16
+_unsupported_imgopts 'compat=0.10' 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
 
 rt_offset=65536  # 0x1 (XXX: just an assumption)
 rb_offset=131072 # 0x2 (XXX: just an assumption)
@@ -55,8 +57,6 @@ l1_offset=196608 # 0x3 (XXX: just an assumption)
 l2_offset=262144 # 0x4 (XXX: just an assumption)
 l2_offset_after_snapshot=524288 # 0x8 (XXX: just an assumption)
 
-IMGOPTS="compat=1.1"
-
 OPEN_RW="open -o overlap-check=all $TEST_IMG"
 # Overlap checks are done before write operations only, therefore opening an
 # image read-only makes the overlap-check option irrelevant
diff --git a/tests/qemu-iotests/062 b/tests/qemu-iotests/062
index 79738b1c26..0df8667e5a 100755
--- a/tests/qemu-iotests/062
+++ b/tests/qemu-iotests/062
@@ -40,8 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto generic
+# We need zero clusters and snapshots
+_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
 
-IMGOPTS="compat=1.1"
 IMG_SIZE=64M
 
 echo
diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
index cacbdb6ae0..71e8df598a 100755
--- a/tests/qemu-iotests/066
+++ b/tests/qemu-iotests/066
@@ -39,9 +39,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto generic
+# We need zero clusters and snapshots
+_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
 
 # Intentionally create an unaligned image
-IMGOPTS="compat=1.1"
 IMG_SIZE=$((64 * 1024 * 1024 + 512))
 
 echo
diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index c164ccc64a..fe9d7ae1be 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -39,8 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto generic
+# Internal snapshots are (currently) impossible with refcount_bits=1
+_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]'
 
-IMGOPTS="compat=1.1"
 IMG_SIZE=128K
 
 case "$QEMU_DEFAULT_MACHINE" in
diff --git a/tests/qemu-iotests/098 b/tests/qemu-iotests/098
index 1c1d1c468f..700068b328 100755
--- a/tests/qemu-iotests/098
+++ b/tests/qemu-iotests/098
@@ -40,8 +40,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto file
-
-IMGOPTS="compat=1.1"
+# The code path we want to test here only works for compat=1.1 images
+_unsupported_imgopts 'compat=0.10'
 
 for event in l1_update empty_image_prepare reftable_update refblock_alloc; do
 
-- 
2.23.0

[PATCH v3 09/22] iotests: Add -o and --no-opts to _make_test_img

[Max Reitz]

Blindly overriding IMGOPTS is suboptimal as this discards user-specified
options.  Whatever options the test needs should simply be appended.

Some tests do this (with IMGOPTS=$(_optstr_add "$IMGOPTS" "...")), but
that is cumbersome.  It’s simpler to just give _make_test_img an -o
parameter with which tests can add options.

Some tests actually must override the user-specified options, though,
for example when creating an image in a different format than the test
$IMGFMT.  For such cases, --no-opts allows clearing the current option
list.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/common.rc | 13 +
 1 file changed, 13 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index e12216b5f2..a623485f96 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -307,6 +307,7 @@ _make_test_img()
 local use_backing=0
 local backing_file=""
 local object_options=""
+local opts_param=false
 local misc_params=()
 
 if [ -n "$TEST_IMG_FILE" ]; then
@@ -327,6 +328,10 @@ _make_test_img()
 if [ "$use_backing" = "1" -a -z "$backing_file" ]; then
 backing_file=$param
 continue
+elif $opts_param; then
+optstr=$(_optstr_add "$optstr" "$param")
+opts_param=false
+continue
 fi
 
 case "$param" in
@@ -334,6 +339,14 @@ _make_test_img()
 use_backing=1
 ;;
 
+-o)
+opts_param=true
+;;
+
+--no-opts)
+optstr=""
+;;
+
 *)
 misc_params=("${misc_params[@]}" "$param")
 ;;
-- 
2.23.0

[PATCH v3 10/22] iotests: Inject space into -ocompat=0.10 in 051

[Max Reitz]

It did not matter before, but now that _make_test_img understands -o, we
should use it properly here.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/051 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 53bcdbc911..9cd1d60d45 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -157,7 +157,7 @@ echo
 echo === With version 2 images enabling lazy refcounts must fail ===
 echo
 
-_make_test_img -ocompat=0.10 $size
+_make_test_img -o compat=0.10 $size
 
 run_qemu -drive file="$TEST_IMG",format=qcow2,lazy-refcounts=on
 run_qemu -drive file="$TEST_IMG",format=qcow2,lazy-refcounts=off
-- 
2.23.0

[PATCH v3 17/22] iotests: Make 091 work with data_file

[Max Reitz]

The image end offset as reported by qemu-img check is different when
using an external data file; we do not care about its value here, so we
can just filter it.  Incidentally, common.rc already has _check_test_img
for us which does exactly that.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/091 | 2 +-
 tests/qemu-iotests/091.out | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/091 b/tests/qemu-iotests/091
index f4b44659ae..0874fa84c8 100755
--- a/tests/qemu-iotests/091
+++ b/tests/qemu-iotests/091
@@ -101,7 +101,7 @@ echo "Check image pattern"
 ${QEMU_IO} -c "read -P 0x22 0 4M" "${TEST_IMG}" | _filter_testdir | 
_filter_qemu_io
 
 echo "Running 'qemu-img check -r all \$TEST_IMG'"
-"${QEMU_IMG}" check -r all "${TEST_IMG}" 2>&1 | _filter_testdir | _filter_qemu
+_check_test_img -r all
 
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/091.out b/tests/qemu-iotests/091.out
index 5017f8c2d9..5ec7b00f13 100644
--- a/tests/qemu-iotests/091.out
+++ b/tests/qemu-iotests/091.out
@@ -23,6 +23,4 @@ read 4194304/4194304 bytes at offset 0
 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 Running 'qemu-img check -r all $TEST_IMG'
 No errors were found on the image.
-80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
-Image end offset: 5570560
 *** done
-- 
2.23.0

[PATCH v3 15/22] iotests: Use _rm_test_img for deleting test images

[Max Reitz]

Just rm will not delete external data files.  Use _rm_test_img every
time we delete a test image.

(In the process, clean up the indentation of every _cleanup() this patch
touches.)

((Also, use quotes consistently.  I am happy to see unquoted instances
like "rm -rf $TEST_DIR/..." go.))

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/005 |  2 +-
 tests/qemu-iotests/019 |  6 +++---
 tests/qemu-iotests/020 |  6 +++---
 tests/qemu-iotests/024 | 10 +-
 tests/qemu-iotests/028 |  2 +-
 tests/qemu-iotests/029 |  2 +-
 tests/qemu-iotests/043 |  4 +++-
 tests/qemu-iotests/048 |  2 +-
 tests/qemu-iotests/050 |  4 ++--
 tests/qemu-iotests/053 |  4 ++--
 tests/qemu-iotests/058 |  2 +-
 tests/qemu-iotests/059 |  2 +-
 tests/qemu-iotests/061 |  2 +-
 tests/qemu-iotests/063 |  6 --
 tests/qemu-iotests/069 |  2 +-
 tests/qemu-iotests/074 |  2 +-
 tests/qemu-iotests/080 |  2 +-
 tests/qemu-iotests/081 |  6 +++---
 tests/qemu-iotests/085 |  9 ++---
 tests/qemu-iotests/088 |  2 +-
 tests/qemu-iotests/092 |  2 +-
 tests/qemu-iotests/094 |  2 +-
 tests/qemu-iotests/095 |  5 +++--
 tests/qemu-iotests/099 |  7 ---
 tests/qemu-iotests/109 |  4 ++--
 tests/qemu-iotests/110 |  4 ++--
 tests/qemu-iotests/122 |  6 --
 tests/qemu-iotests/123 |  2 +-
 tests/qemu-iotests/141 |  4 +++-
 tests/qemu-iotests/142 |  2 +-
 tests/qemu-iotests/144 |  4 +++-
 tests/qemu-iotests/153 | 10 +++---
 tests/qemu-iotests/156 |  8 ++--
 tests/qemu-iotests/159 |  2 +-
 tests/qemu-iotests/160 |  3 ++-
 tests/qemu-iotests/161 |  4 ++--
 tests/qemu-iotests/170 |  2 +-
 tests/qemu-iotests/172 |  6 +++---
 tests/qemu-iotests/173 |  3 ++-
 tests/qemu-iotests/178 |  2 +-
 tests/qemu-iotests/182 |  2 +-
 tests/qemu-iotests/183 |  2 +-
 tests/qemu-iotests/185 |  4 ++--
 tests/qemu-iotests/187 |  6 +++---
 tests/qemu-iotests/190 |  2 +-
 tests/qemu-iotests/191 |  6 +++---
 tests/qemu-iotests/195 |  2 +-
 tests/qemu-iotests/197 |  2 +-
 tests/qemu-iotests/200 |  3 ++-
 tests/qemu-iotests/215 |  2 +-
 tests/qemu-iotests/225 |  2 +-
 tests/qemu-iotests/229 |  3 ++-
 tests/qemu-iotests/232 |  4 +++-
 tests/qemu-iotests/243 |  2 +-
 tests/qemu-iotests/244 |  4 ++--
 tests/qemu-iotests/247 |  4 +++-
 tests/qemu-iotests/249 |  4 ++--
 tests/qemu-iotests/252 |  2 +-
 58 files changed, 119 insertions(+), 96 deletions(-)

diff --git a/tests/qemu-iotests/005 b/tests/qemu-iotests/005
index 58442762fe..2b651f2c37 100755
--- a/tests/qemu-iotests/005
+++ b/tests/qemu-iotests/005
@@ -62,7 +62,7 @@ if [ "$IMGFMT" = "raw" ]; then
 if ! truncate --size=5T "$TEST_IMG"; then
 _notrun "file system on $TEST_DIR does not support large enough files"
 fi
-rm "$TEST_IMG"
+_rm_test_img "$TEST_IMG"
 fi
 
 echo
diff --git a/tests/qemu-iotests/019 b/tests/qemu-iotests/019
index b4f5234609..813a84acac 100755
--- a/tests/qemu-iotests/019
+++ b/tests/qemu-iotests/019
@@ -30,9 +30,9 @@ status=1  # failure is the default!
 
 _cleanup()
 {
-   _cleanup_test_img
-rm -f "$TEST_IMG.base"
-rm -f "$TEST_IMG.orig"
+_cleanup_test_img
+_rm_test_img "$TEST_IMG.base"
+_rm_test_img "$TEST_IMG.orig"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
diff --git a/tests/qemu-iotests/020 b/tests/qemu-iotests/020
index f41b92f35f..20f8f185d0 100755
--- a/tests/qemu-iotests/020
+++ b/tests/qemu-iotests/020
@@ -28,9 +28,9 @@ status=1  # failure is the default!
 
 _cleanup()
 {
-   _cleanup_test_img
-rm -f "$TEST_IMG.base"
-rm -f "$TEST_IMG.orig"
+_cleanup_test_img
+_rm_test_img "$TEST_IMG.base"
+_rm_test_img "$TEST_IMG.orig"
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
 
diff --git a/tests/qemu-iotests/024 b/tests/qemu-iotests/024
index 23298c6f59..e2e766241e 100755
--- a/tests/qemu-iotests/024
+++ b/tests/qemu-iotests/024
@@ -29,12 +29,12 @@ status=1# failure is the default!
 _cleanup()
 {
 _cleanup_test_img
-rm -f "$TEST_DIR/t.$IMGFMT.base_old"
-rm -f "$TEST_DIR/t.$IMGFMT.base_new"
+_rm_test_img "$TEST_DIR/t.$IMGFMT.base_old"
+_rm_test_img "$TEST_DIR/t.$IMGFMT.base_new"
 
-rm -f "$TEST_DIR/subdir/t.$IMGFMT"
-rm -f "$TEST_DIR/subdir/t.$IMGFMT.base_old"
-rm -f "$TEST_DIR/subdir/t.$IMGFMT.base_new"
+_rm_test_img "$TEST_DIR/subdir/t.$IMGFMT"
+_rm_test_img "$TEST_DIR/subdir/t.$IMGFMT.base_old"
+_rm_test_img "$TEST_DIR/subdir/t.$IMGFMT.base_new"
 rmdir "$TEST_DIR/subdir" 2> /dev/null
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
index bba1ee59ae..e2556d8e57 100755
--- a/tests/qemu-iotests/028
+++ b/tests/qemu-iotests/028
@@ -32,7 +32,7 @@ status=1  # failure is the default!
 _cleanup()
 {
 _cleanup_qemu
-rm -f "${TEST_IMG}.copy"
+_rm_test_img "${TEST_IMG}.copy"
 _cleanup_test_img
 }
 trap "_cleanup; exit \$status" 0 1 2 3 15
diff --git a/tests/qemu-iotests/029 b/tests/qemu-iotests/029
index 94c2713132..9254ede5e5 100755
--- a/tests/qemu-iotests/029
+++ 

[PATCH v3 08/22] iotests: Let _make_test_img parse its parameters

[Max Reitz]

This will allow us to add more options than just -b.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/common.rc | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index fa7bae2422..e12216b5f2 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -302,12 +302,12 @@ _make_test_img()
 # extra qemu-img options can be added by tests
 # at least one argument (the image size) needs to be added
 local extra_img_options=""
-local image_size=$*
 local optstr=""
 local img_name=""
 local use_backing=0
 local backing_file=""
 local object_options=""
+local misc_params=()
 
 if [ -n "$TEST_IMG_FILE" ]; then
 img_name=$TEST_IMG_FILE
@@ -323,11 +323,23 @@ _make_test_img()
 optstr=$(_optstr_add "$optstr" "key-secret=keysec0")
 fi
 
-if [ "$1" = "-b" ]; then
-use_backing=1
-backing_file=$2
-image_size=$3
-fi
+for param; do
+if [ "$use_backing" = "1" -a -z "$backing_file" ]; then
+backing_file=$param
+continue
+fi
+
+case "$param" in
+-b)
+use_backing=1
+;;
+
+*)
+misc_params=("${misc_params[@]}" "$param")
+;;
+esac
+done
+
 if [ \( "$IMGFMT" = "qcow2" -o "$IMGFMT" = "qed" \) -a -n "$CLUSTER_SIZE" 
]; then
 optstr=$(_optstr_add "$optstr" "cluster_size=$CLUSTER_SIZE")
 fi
@@ -343,9 +355,9 @@ _make_test_img()
 # XXX(hch): have global image options?
 (
  if [ $use_backing = 1 ]; then
-$QEMU_IMG create $object_options -f $IMGFMT $extra_img_options -b 
"$backing_file" "$img_name" $image_size 2>&1
+$QEMU_IMG create $object_options -f $IMGFMT $extra_img_options -b 
"$backing_file" "$img_name" "${misc_params[@]}" 2>&1
  else
-$QEMU_IMG create $object_options -f $IMGFMT $extra_img_options 
"$img_name" $image_size 2>&1
+$QEMU_IMG create $object_options -f $IMGFMT $extra_img_options 
"$img_name" "${misc_params[@]}" 2>&1
  fi
 ) | _filter_img_create
 
-- 
2.23.0

[PATCH v3 00/22] iotests: Allow ./check -o data_file

[Max Reitz]

Hi,

The cover letter from v1 (explaining the motivation behind this series
and the general structure) is here:

https://lists.nongnu.org/archive/html/qemu-block/2019-09/msg01323.html


For v2, I’ve addressed more of Maxim’s comments:
- Patch 1: Added; Maxim noted this problem on patch 5, but that patch
   doesn’t touch all files that have this mistake, so I decided
   to make it an extra patch

- Patch 20 (now patch 21):
  - Added TODO comments where it would make sense to at some point split
off some cases into an own test file (so they can run with an
external data file, where the whole test now has to be skipped)
  - Fixed the reason why we have to skip 138 with external data files
  - Disable 261, too (which was added in the meantime)

- Some contextual differences in some patches due to the $SOCK_DIR
  series

git-backport-diff against v2:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/22:[down] 'iotests: s/qocw2/qcow2/'
002/22:[] [--] 'iotests/qcow2.py: Add dump-header-exts'
003/22:[] [--] 'iotests/qcow2.py: Split feature fields into bits'
004/22:[] [--] 'iotests: Add _filter_json_filename'
005/22:[] [--] 'iotests: Filter refcount_order in 036'
006/22:[] [-C] 'iotests: Replace IMGOPTS by _unsupported_imgopts'
007/22:[] [--] 'iotests: Drop compat=1.1 in 050'
008/22:[] [--] 'iotests: Let _make_test_img parse its parameters'
009/22:[] [--] 'iotests: Add -o and --no-opts to _make_test_img'
010/22:[] [--] 'iotests: Inject space into -ocompat=0.10 in 051'
011/22:[] [--] 'iotests: Replace IMGOPTS= by -o'
012/22:[] [--] 'iotests: Replace IMGOPTS='' by --no-opts'
013/22:[] [-C] 'iotests: Drop IMGOPTS use in 267'
014/22:[] [--] 'iotests: Avoid qemu-img create'
015/22:[] [-C] 'iotests: Use _rm_test_img for deleting test images'
016/22:[] [--] 'iotests: Avoid cp/mv of test images'
017/22:[] [--] 'iotests: Make 091 work with data_file'
018/22:[] [--] 'iotests: Make 110 work with data_file'
019/22:[] [--] 'iotests: Make 137 work with data_file'
020/22:[] [--] 'iotests: Make 198 work with data_file'
021/22:[0010] [FC] 'iotests: Disable data_file where it cannot be used'
022/22:[] [-C] 'iotests: Allow check -o data_file'


Max Reitz (22):
  iotests: s/qocw2/qcow2/
  iotests/qcow2.py: Add dump-header-exts
  iotests/qcow2.py: Split feature fields into bits
  iotests: Add _filter_json_filename
  iotests: Filter refcount_order in 036
  iotests: Replace IMGOPTS by _unsupported_imgopts
  iotests: Drop compat=1.1 in 050
  iotests: Let _make_test_img parse its parameters
  iotests: Add -o and --no-opts to _make_test_img
  iotests: Inject space into -ocompat=0.10 in 051
  iotests: Replace IMGOPTS= by -o
  iotests: Replace IMGOPTS='' by --no-opts
  iotests: Drop IMGOPTS use in 267
  iotests: Avoid qemu-img create
  iotests: Use _rm_test_img for deleting test images
  iotests: Avoid cp/mv of test images
  iotests: Make 091 work with data_file
  iotests: Make 110 work with data_file
  iotests: Make 137 work with data_file
  iotests: Make 198 work with data_file
  iotests: Disable data_file where it cannot be used
  iotests: Allow check -o data_file

 tests/qemu-iotests/005   |  2 +-
 tests/qemu-iotests/007   |  5 ++-
 tests/qemu-iotests/014   |  2 +
 tests/qemu-iotests/015   |  5 ++-
 tests/qemu-iotests/019   |  6 +--
 tests/qemu-iotests/020   |  6 +--
 tests/qemu-iotests/024   | 10 ++---
 tests/qemu-iotests/026   |  5 ++-
 tests/qemu-iotests/028   |  2 +-
 tests/qemu-iotests/029   |  7 ++--
 tests/qemu-iotests/031   |  9 ++--
 tests/qemu-iotests/031.out   | 36 
 tests/qemu-iotests/036   | 15 ---
 tests/qemu-iotests/036.out   | 66 -
 tests/qemu-iotests/039   | 27 +---
 tests/qemu-iotests/039.out   | 22 +-
 tests/qemu-iotests/043   |  4 +-
 tests/qemu-iotests/046   |  2 +
 tests/qemu-iotests/048   |  4 +-
 tests/qemu-iotests/050   |  8 +---
 tests/qemu-iotests/051   |  7 ++--
 tests/qemu-iotests/053   |  4 +-
 tests/qemu-iotests/058   |  7 ++--
 tests/qemu-iotests/059   | 20 -
 tests/qemu-iotests/060   | 14 ---
 tests/qemu-iotests/060.out   | 20 -
 tests/qemu-iotests/061   | 63 +++-
 tests/qemu-iotests/061.out   | 72 
 tests/qemu-iotests/062   |  5 ++-
 tests/qemu-iotests/063   | 18 
 tests/qemu-iotests/063.out   |  3 +-
 tests/qemu-iotests/066   |  7 +++-
 tests/qemu-iotests/067   |  6 ++-
 tests/qemu-iotests/068   |  6 ++-
 tests/qemu-iotests/069

[PATCH v3 02/22] iotests/qcow2.py: Add dump-header-exts

[Max Reitz]

This is useful for tests that want to whitelist fields from dump-header
(with grep) but still print all header extensions.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/qcow2.py | 5 +
 1 file changed, 5 insertions(+)

diff --git a/tests/qemu-iotests/qcow2.py b/tests/qemu-iotests/qcow2.py
index b392972d1b..d813b4fc81 100755
--- a/tests/qemu-iotests/qcow2.py
+++ b/tests/qemu-iotests/qcow2.py
@@ -154,6 +154,10 @@ def cmd_dump_header(fd):
 h.dump()
 h.dump_extensions()
 
+def cmd_dump_header_exts(fd):
+h = QcowHeader(fd)
+h.dump_extensions()
+
 def cmd_set_header(fd, name, value):
 try:
 value = int(value, 0)
@@ -230,6 +234,7 @@ def cmd_set_feature_bit(fd, group, bit):
 
 cmds = [
 [ 'dump-header',  cmd_dump_header,  0, 'Dump image header 
and header extensions' ],
+[ 'dump-header-exts', cmd_dump_header_exts, 0, 'Dump image header 
extensions' ],
 [ 'set-header',   cmd_set_header,   2, 'Set a field in the 
header'],
 [ 'add-header-ext',   cmd_add_header_ext,   2, 'Add a header 
extension' ],
 [ 'add-header-ext-stdio', cmd_add_header_ext_stdio, 1, 'Add a header 
extension, data from stdin' ],
-- 
2.23.0

[PATCH v3 05/22] iotests: Filter refcount_order in 036

[Max Reitz]

This test can run just fine with other values for refcount_bits, so we
should filter the value from qcow2.py's dump-header.  In fact, we can
filter everything but the feature bits and header extensions, because
that is what the test is about.

(036 currently ignores user-specified image options, but that will be
fixed in the next patch.)

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/036 |  9 ---
 tests/qemu-iotests/036.out | 48 --
 2 files changed, 6 insertions(+), 51 deletions(-)

diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
index f06ff67408..5f929ad3be 100755
--- a/tests/qemu-iotests/036
+++ b/tests/qemu-iotests/036
@@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit incompatible 63
 
 # Without feature table
 $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857
-$PYTHON qcow2.py "$TEST_IMG" dump-header
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep features
+$PYTHON qcow2.py "$TEST_IMG" dump-header-exts
 _img_info
 
 # With feature table containing bit 63
@@ -103,14 +104,16 @@ echo === Create image with unknown autoclear feature bit 
===
 echo
 _make_test_img 64M
 $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63
-$PYTHON qcow2.py "$TEST_IMG" dump-header
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep features
+$PYTHON qcow2.py "$TEST_IMG" dump-header-exts
 
 echo
 echo === Repair image ===
 echo
 _check_test_img -r all
 
-$PYTHON qcow2.py "$TEST_IMG" dump-header
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep features
+$PYTHON qcow2.py "$TEST_IMG" dump-header-exts
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
index 15229a9604..0b52b934e1 100644
--- a/tests/qemu-iotests/036.out
+++ b/tests/qemu-iotests/036.out
@@ -3,25 +3,9 @@ QA output created by 036
 === Image with unknown incompatible feature bit ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-magic 0x514649fb
-version   3
-backing_file_offset   0x0
-backing_file_size 0x0
-cluster_bits  16
-size  67108864
-crypt_method  0
-l1_size   1
-l1_table_offset   0x3
-refcount_table_offset 0x1
-refcount_table_clusters   1
-nb_snapshots  0
-snapshot_offset   0x0
 incompatible_features [63]
 compatible_features   []
 autoclear_features[]
-refcount_order4
-header_length 104
-
 qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): 
Unknown incompatible feature: 8000
 qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): 
Test feature
 
@@ -37,25 +21,9 @@ qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported 
IMGFMT feature(s): tes
 === Create image with unknown autoclear feature bit ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-magic 0x514649fb
-version   3
-backing_file_offset   0x0
-backing_file_size 0x0
-cluster_bits  16
-size  67108864
-crypt_method  0
-l1_size   1
-l1_table_offset   0x3
-refcount_table_offset 0x1
-refcount_table_clusters   1
-nb_snapshots  0
-snapshot_offset   0x0
 incompatible_features []
 compatible_features   []
 autoclear_features[63]
-refcount_order4
-header_length 104
-
 Header extension:
 magic 0x6803f857
 length192
@@ -65,25 +33,9 @@ data  
 === Repair image ===
 
 No errors were found on the image.
-magic 0x514649fb
-version   3
-backing_file_offset   0x0
-backing_file_size 0x0
-cluster_bits  16
-size  67108864
-crypt_method  0
-l1_size   1
-l1_table_offset   0x3
-refcount_table_offset 0x1
-refcount_table_clusters   1
-nb_snapshots  0
-snapshot_offset   0x0
 incompatible_features []
 compatible_features   []
 autoclear_features[]
-refcount_order4
-header_length 104
-
 Header extension:
 magic 0x6803f857
 length192
-- 
2.23.0

[PATCH v3 04/22] iotests: Add _filter_json_filename

[Max Reitz]

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/common.filter | 24 
 1 file changed, 24 insertions(+)

diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index f870e00e44..8a0169f19a 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -230,5 +230,29 @@ _filter_qmp_empty_return()
 grep -v '{"return": {}}'
 }
 
+_filter_json_filename()
+{
+$PYTHON -c 'import sys
+result, *fnames = sys.stdin.read().split("json:{")
+depth = 0
+for fname in fnames:
+depth += 1 # For the opening brace in the split separator
+for chr_i, chr in enumerate(fname):
+if chr == "{":
+depth += 1
+elif chr == "}":
+depth -= 1
+if depth == 0:
+break
+
+# json:{} filenames may be nested; filter out everything from
+# inside the outermost one
+if depth == 0:
+chr_i += 1 # First character past the filename
+result += "json:{ /* filtered */ }" + fname[chr_i:]
+
+sys.stdout.write(result)'
+}
+
 # make sure this script returns success
 true
-- 
2.23.0

[PATCH v3 07/22] iotests: Drop compat=1.1 in 050

[Max Reitz]

IMGOPTS can never be empty for qcow2, because the check scripts adds
compat=1.1 unless the user specified any compat option themselves.
Thus, this block does not do anything and can be dropped.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/050 | 4 
 1 file changed, 4 deletions(-)

diff --git a/tests/qemu-iotests/050 b/tests/qemu-iotests/050
index 211fc00797..272ecab195 100755
--- a/tests/qemu-iotests/050
+++ b/tests/qemu-iotests/050
@@ -41,10 +41,6 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2 qed
 _supported_proto file
 
-if test "$IMGFMT" = qcow2 && test $IMGOPTS = ""; then
-  IMGOPTS=compat=1.1
-fi
-
 echo
 echo "== Creating images =="
 
-- 
2.23.0

[PATCH v3 03/22] iotests/qcow2.py: Split feature fields into bits

[Max Reitz]

Print the feature fields as a set of bits so that filtering is easier.

Signed-off-by: Max Reitz 
Reviewed-by: Maxim Levitsky 
---
 tests/qemu-iotests/031.out  | 36 +--
 tests/qemu-iotests/036.out  | 18 +-
 tests/qemu-iotests/039.out  | 22 ++--
 tests/qemu-iotests/060.out  | 20 +--
 tests/qemu-iotests/061.out  | 72 ++---
 tests/qemu-iotests/137.out  |  2 +-
 tests/qemu-iotests/qcow2.py | 18 +++---
 7 files changed, 99 insertions(+), 89 deletions(-)

diff --git a/tests/qemu-iotests/031.out b/tests/qemu-iotests/031.out
index 68a74d03b9..d535e407bc 100644
--- a/tests/qemu-iotests/031.out
+++ b/tests/qemu-iotests/031.out
@@ -18,9 +18,9 @@ refcount_table_offset 0x1
 refcount_table_clusters   1
 nb_snapshots  0
 snapshot_offset   0x0
-incompatible_features 0x0
-compatible_features   0x0
-autoclear_features0x0
+incompatible_features []
+compatible_features   []
+autoclear_features[]
 refcount_order4
 header_length 72
 
@@ -46,9 +46,9 @@ refcount_table_offset 0x1
 refcount_table_clusters   1
 nb_snapshots  0
 snapshot_offset   0x0
-incompatible_features 0x0
-compatible_features   0x0
-autoclear_features0x0
+incompatible_features []
+compatible_features   []
+autoclear_features[]
 refcount_order4
 header_length 72
 
@@ -74,9 +74,9 @@ refcount_table_offset 0x1
 refcount_table_clusters   1
 nb_snapshots  0
 snapshot_offset   0x0
-incompatible_features 0x0
-compatible_features   0x0
-autoclear_features0x0
+incompatible_features []
+compatible_features   []
+autoclear_features[]
 refcount_order4
 header_length 72
 
@@ -109,9 +109,9 @@ refcount_table_offset 0x1
 refcount_table_clusters   1
 nb_snapshots  0
 snapshot_offset   0x0
-incompatible_features 0x0
-compatible_features   0x0
-autoclear_features0x0
+incompatible_features []
+compatible_features   []
+autoclear_features[]
 refcount_order4
 header_length 104
 
@@ -142,9 +142,9 @@ refcount_table_offset 0x1
 refcount_table_clusters   1
 nb_snapshots  0
 snapshot_offset   0x0
-incompatible_features 0x0
-compatible_features   0x0
-autoclear_features0x0
+incompatible_features []
+compatible_features   []
+autoclear_features[]
 refcount_order4
 header_length 104
 
@@ -175,9 +175,9 @@ refcount_table_offset 0x1
 refcount_table_clusters   1
 nb_snapshots  0
 snapshot_offset   0x0
-incompatible_features 0x0
-compatible_features   0x0
-autoclear_features0x0
+incompatible_features []
+compatible_features   []
+autoclear_features[]
 refcount_order4
 header_length 104
 
diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
index e489b44386..15229a9604 100644
--- a/tests/qemu-iotests/036.out
+++ b/tests/qemu-iotests/036.out
@@ -16,9 +16,9 @@ refcount_table_offset 0x1
 refcount_table_clusters   1
 nb_snapshots  0
 snapshot_offset   0x0
-incompatible_features 0x8000
-compatible_features   0x0
-autoclear_features0x0
+incompatible_features [63]
+compatible_features   []
+autoclear_features[]
 refcount_order4
 header_length 104
 
@@ -50,9 +50,9 @@ refcount_table_offset 0x1
 refcount_table_clusters   1
 nb_snapshots  0
 snapshot_offset   0x0
-incompatible_features 0x0
-compatible_features   0x0
-autoclear_features0x8000
+incompatible_features []
+compatible_features   []
+autoclear_features[63]
 refcount_order4
 header_length 104
 
@@ -78,9 +78,9 @@ refcount_table_offset 0x1
 refcount_table_clusters   1
 nb_snapshots  0
 snapshot_offset   0x0
-incompatible_features 0x0
-compatible_features   0x0
-autoclear_features0x0
+incompatible_features []
+compatible_features   []
+autoclear_features[]
 refcount_order4
 header_length 104
 
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index 2e356d51b6..bdafa3ace3 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -4,7 +4,7 @@ QA output created by 039
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-incompatible_features 0x0
+incompatible_features []
 No errors were found on the image.
 
 == Creating a dirty image file ==
@@ -12,7 +12,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
 wrote 512/512 bytes at offset 0
 512 byte

[PATCH v3 01/22] iotests: s/qocw2/qcow2/

[Max Reitz]

Probably due to blind copy-pasting, we have several instances of "qocw2"
in our iotests.  Fix them.

Reported-by: Maxim Levitsky 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/060 | 2 +-
 tests/qemu-iotests/061 | 2 +-
 tests/qemu-iotests/062 | 2 +-
 tests/qemu-iotests/066 | 2 +-
 tests/qemu-iotests/068 | 2 +-
 tests/qemu-iotests/108 | 2 +-
 tests/qemu-iotests/138 | 2 +-
 tests/qemu-iotests/261 | 2 +-
 8 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
index b91d8321bb..725e58a5a5 100755
--- a/tests/qemu-iotests/060
+++ b/tests/qemu-iotests/060
@@ -44,7 +44,7 @@ _filter_io_error()
 . ./common.rc
 . ./common.filter
 
-# This tests qocw2-specific low-level functionality
+# This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index 4eac5b83bd..e1b8044630 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-# This tests qocw2-specific low-level functionality
+# This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
diff --git a/tests/qemu-iotests/062 b/tests/qemu-iotests/062
index d5f818fcce..79738b1c26 100755
--- a/tests/qemu-iotests/062
+++ b/tests/qemu-iotests/062
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-# This tests qocw2-specific low-level functionality
+# This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto generic
 
diff --git a/tests/qemu-iotests/066 b/tests/qemu-iotests/066
index 28f8c98412..cacbdb6ae0 100755
--- a/tests/qemu-iotests/066
+++ b/tests/qemu-iotests/066
@@ -36,7 +36,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-# This tests qocw2-specific low-level functionality
+# This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto generic
 
diff --git a/tests/qemu-iotests/068 b/tests/qemu-iotests/068
index 22f5ca3ba6..c164ccc64a 100755
--- a/tests/qemu-iotests/068
+++ b/tests/qemu-iotests/068
@@ -36,7 +36,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-# This tests qocw2-specific low-level functionality
+# This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto generic
 
diff --git a/tests/qemu-iotests/108 b/tests/qemu-iotests/108
index 9c08172237..872a9afec9 100755
--- a/tests/qemu-iotests/108
+++ b/tests/qemu-iotests/108
@@ -37,7 +37,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-# This tests qocw2-specific low-level functionality
+# This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
index 6a731370db..8b2f587af0 100755
--- a/tests/qemu-iotests/138
+++ b/tests/qemu-iotests/138
@@ -36,7 +36,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-# This tests qocw2-specific low-level functionality
+# This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
diff --git a/tests/qemu-iotests/261 b/tests/qemu-iotests/261
index fb96bcfbe2..9f2817251f 100755
--- a/tests/qemu-iotests/261
+++ b/tests/qemu-iotests/261
@@ -40,7 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-# This tests qocw2-specific low-level functionality
+# This tests qcow2-specific low-level functionality
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
-- 
2.23.0

Re: [PATCH v1 4/4] iotests: add test for virtio-scsi and virtio-blk machine type settings

[Cleber Rosa]

On Wed, Nov 06, 2019 at 04:26:41PM -0300, Eduardo Habkost wrote:
> On Wed, Nov 06, 2019 at 11:04:16AM +0100, Max Reitz wrote:
> > On 06.11.19 10:24, Stefan Hajnoczi wrote:
> > > On Tue, Nov 05, 2019 at 07:11:05PM +0300, Denis Plotnikov wrote:
> > >> It tests proper queue size settings for all available machine types.
> > >>
> > >> Signed-off-by: Denis Plotnikov 
> > >> ---
> > >>  tests/qemu-iotests/267 | 154 +
> > >>  tests/qemu-iotests/267.out |   1 +
> > >>  tests/qemu-iotests/group   |   1 +
> > >>  3 files changed, 156 insertions(+)
> > >>  create mode 100755 tests/qemu-iotests/267
> > >>  create mode 100644 tests/qemu-iotests/267.out
> > > 
> > > The qemu-iotests maintainers might prefer for this to be at the
> > > top-level in tests/ since it's not really an iotest, but the code itself
> > > looks fine to me:
> > > 
> > > Reviewed-by: Stefan Hajnoczi 
> > 
> > Good question.  I don’t really mind, but it would be weird if started
> > adding all kinds of “external” qemu tests (i.e. that use QMP) in the
> > iotests directory.
> > 
> > What is the alternative?  Just putting it in a different directory
> > doesn’t sound that appealing to me either, because it would still depend
> > on the iotests infrastructure, right?  (i.e., iotests.py and check)
> 
> We do have tests/acceptance for simple test cases written in
> Python.  What's the reason for this test case to depend on the
> iotests infrastructure?
> 
> -- 
> Eduardo

This test does look similar in spirit to "tests/acceptance/virtio_version.py".

Denis,

If you think this is more of a generic test than an IO test, and would
rather want to have it a more agnostic location, I can provide you
with tips (or a patch) to do so.

Cheers,
- Cleber.

Re: [RFC PATCH 01/18] qemu-storage-daemon: Add barebone tool

[Markus Armbruster]

In addition to Eric's review:

Kevin Wolf  writes:

> This adds a new binary qemu-storage-daemon that doesn't yet do more than
> some typical initialisation for tools and parsing the basic command
> options --version, --help and --trace.
>
> Signed-off-by: Kevin Wolf 
> ---
>  configure |   2 +-
>  qemu-storage-daemon.c | 141 ++
>  Makefile  |   1 +
>  3 files changed, 143 insertions(+), 1 deletion(-)
>  create mode 100644 qemu-storage-daemon.c
>
> diff --git a/configure b/configure
> index 08ca4bcb46..bb3d55fb25 100755
> --- a/configure
> +++ b/configure
> @@ -6034,7 +6034,7 @@ tools=""
>  if test "$want_tools" = "yes" ; then
>tools="qemu-img\$(EXESUF) qemu-io\$(EXESUF) qemu-edid\$(EXESUF) $tools"
>if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
> -tools="qemu-nbd\$(EXESUF) $tools"
> +tools="qemu-nbd\$(EXESUF) qemu-storage-daemon\$(EXESUF) $tools"
>fi
>if [ "$ivshmem" = "yes" ]; then
>  tools="ivshmem-client\$(EXESUF) ivshmem-server\$(EXESUF) $tools"
> diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c
> new file mode 100644
> index 00..a251dc255c
> --- /dev/null
> +++ b/qemu-storage-daemon.c
> @@ -0,0 +1,141 @@
> +/*
> + * QEMU storage daemon
> + *
> + * Copyright (c) 2019 Kevin Wolf 
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */

Standard request for new code: please make this GPLv2+, or tell us why
it has to be something else.

> +
> +#include "qemu/osdep.h"
> +
> +#include "block/block.h"
> +#include "crypto/init.h"
> +
> +#include "qapi/error.h"
> +#include "qemu-common.h"
> +#include "qemu-version.h"
> +#include "qemu/config-file.h"
> +#include "qemu/error-report.h"
> +#include "qemu/log.h"
> +#include "qemu/main-loop.h"
> +#include "qemu/module.h"
> +
> +#include "trace/control.h"
> +
> +#include 
> +
> +static void help(void)
> +{
> +printf(
> +"Usage: %s [options]\n"
> +"QEMU storage daemon\n"
> +"\n"
> +"  -h, --help display this help and exit\n"
> +"  -T, --trace [[enable=]][,events=][,file=]\n"
> +" specify tracing options\n"
> +"  -V, --version  output version information and exit\n"
> +"\n"
> +QEMU_HELP_BOTTOM "\n",
> +error_get_progname());
> +}
> +
> +static int process_options(int argc, char *argv[], Error **errp)
> +{
> +int c;
> +char *trace_file = NULL;
> +int ret = -EINVAL;
> +
> +static const struct option long_options[] = {
> +{"help", no_argument, 0, 'h'},

You initialize member int *flag with 0 here, 

> +{"version", no_argument, 0, 'V'},
> +{"trace", required_argument, NULL, 'T'},

... and with NULL here.  Recommend to pick one and stick to it.

> +{0, 0, 0, 0}

{0} or {} would do.

> +};
> +
> +while ((c = getopt_long(argc, argv, ":hT:V", long_options, NULL)) != -1) 
> {
> +switch (c) {
> +case '?':
> +error_setg(errp, "Unknown option '%s'", argv[optind - 1]);
> +goto out;
> +case ':':
> +error_setg(errp, "Missing option argument for '%s'",
> +   argv[optind - 1]);
> +goto out;
> +case 'h':
> +help();
> +exit(EXIT_SUCCESS);
> +case 'V':
> +printf("qemu-storage-daemon version "
> +   QEMU_FULL_VERSION "\n" QEMU_COPYRIGHT "\n");
> +exit(EXIT_SUCCESS);
> +case 'T':
> +g_free(trace_file);
> +trace_file = trace_opt_parse(optarg);

This is QemuOpts below the hood.  Fact, not criticism :)

> +break;
> +}

Suggest (your preferred variation of) default: assert(0) to catch
omissions.

> +}
> +if (optind != argc) {
> +error_setg(errp, "Unexpected argument: %s", argv[optind]);
> +goto out;
> +}
> +
> +if (!trace_init_bac

Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option

[Eric Blake]

On 11/7/19 9:27 AM, Kevin Wolf wrote:

Am 07.11.2019 um 14:45 hat Eric Blake geschrieben:

On 11/7/19 2:33 AM, Kevin Wolf wrote:

As a replacement nbd-server-add, I envisioned adding something like a
block-export-add, which would work the way that --export already does.
It would also come with query-block-exports and block-export-del, and it
wouldn't contain only NBD devices, but also vhost-user, FUSE, etc.
exports.

Now I'm wondering if the same would make sense for nbd-server-start.
Maybe an API change would even allow us to start multiple NBD servers
(e.g. listening on different IP addresses or using different tls-creds).


We want that (the ability to run multiple parallel NBD servers) anyway, to
allow parallel incremental backups from different points in time to
different clients.


Can't you already have multiple exports on a single NBD server for
multiple clients today? Or do you need a different server configuration
for each client?


With our current code base, you can only run a single NBD server, with 
multiple exports, but the TLS creds are shared among all exports.  It is 
indeed technically possible to tweak things where the single server 
changes _which_ exports are exposed based on _which_ creds were used by 
the client (but only when TLS is used, and note that qemu-nbd currently 
refuses to mix TLS and Unix sockets, although I need to post a v2 of a 
patch I proposed a while back to improve that).  But it is easier still 
to run two separate servers on different ports with two different creds, 
and where there is no magic on which exports to show merely based on 
which creds were presented (and this includes a plaintext connection 
over Unix).  Either way, it requires code changes, and most likely for 5.0.


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

Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option

[Kevin Wolf]

Am 07.11.2019 um 14:45 hat Eric Blake geschrieben:
> On 11/7/19 2:33 AM, Kevin Wolf wrote:
> > As a replacement nbd-server-add, I envisioned adding something like a
> > block-export-add, which would work the way that --export already does.
> > It would also come with query-block-exports and block-export-del, and it
> > wouldn't contain only NBD devices, but also vhost-user, FUSE, etc.
> > exports.
> > 
> > Now I'm wondering if the same would make sense for nbd-server-start.
> > Maybe an API change would even allow us to start multiple NBD servers
> > (e.g. listening on different IP addresses or using different tls-creds).
> 
> We want that (the ability to run multiple parallel NBD servers) anyway, to
> allow parallel incremental backups from different points in time to
> different clients.

Can't you already have multiple exports on a single NBD server for
multiple clients today? Or do you need a different server configuration
for each client?

Kevin

Re: [PATCH v2 20/21] iotests: Disable data_file where it cannot be used

[Maxim Levitsky]

On Thu, 2019-11-07 at 12:36 +0100, Max Reitz wrote:
> On 06.11.19 16:52, Maxim Levitsky wrote:
> > On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
> > > Signed-off-by: Max Reitz 
> > > ---
> > >  tests/qemu-iotests/007 | 5 +++--
> > >  tests/qemu-iotests/014 | 2 ++
> > >  tests/qemu-iotests/015 | 5 +++--
> > >  tests/qemu-iotests/026 | 5 -
> > >  tests/qemu-iotests/029 | 5 +++--
> > >  tests/qemu-iotests/031 | 6 +++---
> > >  tests/qemu-iotests/036 | 5 +++--
> > >  tests/qemu-iotests/039 | 3 +++
> > >  tests/qemu-iotests/046 | 2 ++
> > >  tests/qemu-iotests/048 | 2 ++
> > >  tests/qemu-iotests/051 | 5 +++--
> > >  tests/qemu-iotests/058 | 5 +++--
> > >  tests/qemu-iotests/060 | 6 --
> > >  tests/qemu-iotests/061 | 6 --
> > >  tests/qemu-iotests/062 | 2 +-
> > >  tests/qemu-iotests/066 | 2 +-
> > >  tests/qemu-iotests/067 | 6 --
> > >  tests/qemu-iotests/068 | 5 +++--
> > >  tests/qemu-iotests/071 | 3 +++
> > >  tests/qemu-iotests/073 | 2 ++
> > >  tests/qemu-iotests/074 | 2 ++
> > >  tests/qemu-iotests/080 | 5 +++--
> > >  tests/qemu-iotests/090 | 2 ++
> > >  tests/qemu-iotests/098 | 6 --
> > >  tests/qemu-iotests/099 | 3 ++-
> > >  tests/qemu-iotests/103 | 5 +++--
> > >  tests/qemu-iotests/108 | 6 --
> > >  tests/qemu-iotests/112 | 5 +++--
> > >  tests/qemu-iotests/114 | 2 ++
> > >  tests/qemu-iotests/121 | 3 +++
> > >  tests/qemu-iotests/138 | 2 ++
> > >  tests/qemu-iotests/156 | 2 ++
> > >  tests/qemu-iotests/176 | 7 +--
> > >  tests/qemu-iotests/191 | 2 ++
> > >  tests/qemu-iotests/201 | 6 +++---
> > >  tests/qemu-iotests/214 | 3 ++-
> > >  tests/qemu-iotests/217 | 3 ++-
> > >  tests/qemu-iotests/220 | 5 +++--
> > >  tests/qemu-iotests/243 | 6 --
> > >  tests/qemu-iotests/244 | 5 +++--
> > >  tests/qemu-iotests/250 | 2 ++
> > >  tests/qemu-iotests/267 | 5 +++--
> > >  42 files changed, 117 insertions(+), 52 deletions(-)
> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
> > > index c44fcf91bb..646ecd593f 100755
> > > --- a/tests/qemu-iotests/031
> > > +++ b/tests/qemu-iotests/031
> > > @@ -40,9 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  # This tests qcow2-specific low-level functionality
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > > -# We want to test compat=0.10, which does not support refcount widths
> > > -# other than 16
> > > -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> > > +# We want to test compat=0.10, which does not support external data
> > > +# files or refcount widths other than 16
> > > +_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> > 
> > This is maybe another reason to split this test for compat=0.10 and for 
> > compat=1.1
> > But still can be done later of course.
> 
> Hm, but I don’t really think this test is important for external data
> files.  There is no I/O.
I guess both yes and no, the external data file is a header extension as well.
I am looking at the tests from the point of view of someone that
doesn't know the qcow2 internally well yet, so I noted all the tests
that looked like they can still catch something.


> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
> > > index bbaf0ef45b..512598421c 100755
> > > --- a/tests/qemu-iotests/036
> > > +++ b/tests/qemu-iotests/036
> > > @@ -43,8 +43,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  # This tests qcow2-specific low-level functionality
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > > -# Only qcow2v3 and later supports feature bits
> > > -_unsupported_imgopts 'compat=0.10'
> > > +# Only qcow2v3 and later supports feature bits;
> > > +# qcow2.py does not support external data files
> > 
> > Minor nitpick, maybe tag this with TODO or so. No need to do now.
> 
> Hm, well, and the same applies here.  (Just not a very important test.)
Same here, in theory external data file is a feature, and it could
'interact' with other features, but most likely you are right here as well.

> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
> > > index 4e03ead7b1..a066eec605 100755
> > > --- a/tests/qemu-iotests/046
> > > +++ b/tests/qemu-iotests/046
> > > @@ -38,6 +38,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > > +# data_file does not support compressed clusters
> > > +_unsupported_imgopts data_file
> > 
> > This is a very nice test, which doesn't seem to  use compressed
> > clusters that much. I think it should be split as well.
> > No need to do this now of course, but maybe mark with TODO to 
> > avoid loosing that info.
> 
> The other problem is that blkdebug doesn’t work so well with external
> data files, so basically this whole test doesn’t work.
Yes, I see now that the test uses the blkdebug.

> 
> [...]
> 
> > > diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
> > > index a8feb76184..2af6b74b41 100755
> > > --- a/tests/qemu-iotests/

Re: [RFC PATCH v2 12/26] qcow2: Handle QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER

[Alberto Garcia]

On Mon 04 Nov 2019 02:10:37 PM CET, Max Reitz wrote:

   [QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER]
> I still don’t know what you’re doing in the later patches, but to me
> it looks a bit like you don’t dare breaking up the existing structure
> that just deals with clusters.

Yeah, I decided to extend the existing type to make the changes less
invasive, but I'm also leaning towards creating a different type
now. I'll give it a try.

Berto

[PULL 2/3] qcow2: Fix QCOW2_COMPRESSED_SECTOR_MASK

[Max Reitz]

Masks for L2 table entries should have 64 bit.

Fixes: b6c246942b14d3e0dec46a6c5868ed84e7dbea19
Buglink: https://bugs.launchpad.net/qemu/+bug/185
Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 
Message-id: 20191028161841.1198-2-mre...@redhat.com
Reviewed-by: Alberto Garcia 
Signed-off-by: Max Reitz 
---
 block/qcow2.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 601c2e4c82..0942126232 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -83,7 +83,7 @@
 
 /* Defined in the qcow2 spec (compressed cluster descriptor) */
 #define QCOW2_COMPRESSED_SECTOR_SIZE 512U
-#define QCOW2_COMPRESSED_SECTOR_MASK (~(QCOW2_COMPRESSED_SECTOR_SIZE - 1))
+#define QCOW2_COMPRESSED_SECTOR_MASK (~(QCOW2_COMPRESSED_SECTOR_SIZE - 1ULL))
 
 /* Must be at least 2 to cover COW */
 #define MIN_L2_CACHE_SIZE 2 /* cache entries */
-- 
2.23.0

[PULL 3/3] iotests: Add test for 4G+ compressed qcow2 write

[Max Reitz]

Test what qemu-img check says about an image after one has written
compressed data to an offset above 4 GB.

Signed-off-by: Max Reitz 
Message-id: 20191028161841.1198-3-mre...@redhat.com
Reviewed-by: Alberto Garcia 
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/272 | 79 ++
 tests/qemu-iotests/272.out | 10 +
 tests/qemu-iotests/group   |  1 +
 3 files changed, 90 insertions(+)
 create mode 100755 tests/qemu-iotests/272
 create mode 100644 tests/qemu-iotests/272.out

diff --git a/tests/qemu-iotests/272 b/tests/qemu-iotests/272
new file mode 100755
index 00..c2f782d47b
--- /dev/null
+++ b/tests/qemu-iotests/272
@@ -0,0 +1,79 @@
+#!/usr/bin/env bash
+#
+# Test compressed write to a qcow2 image at an offset above 4 GB
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+seq=$(basename "$0")
+echo "QA output created by $seq"
+
+status=1   # failure is the default!
+
+_cleanup()
+{
+_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+# This is a qcow2 regression test
+_supported_fmt qcow2
+_supported_proto file
+
+# External data files do not support compression;
+# We need an exact cluster size (2M) and refcount width (2) so we can
+# get this test quickly over with; and this in turn require
+# compat=1.1
+_unsupported_imgopts data_file cluster_size refcount_bits 'compat=0.10'
+
+# The idea is: Create an empty file, mark the first 4 GB as used, then
+# do a compressed write that thus must be put beyond 4 GB.
+# (This used to fail because the compressed sector mask was just a
+# 32 bit mask, so qemu-img check will count a cluster before 4 GB as
+# referenced twice.)
+
+# We would like to use refcount_bits=1 here, but then qemu-img check
+# will throw an error when trying to count a cluster as referenced
+# twice.
+_make_test_img -o cluster_size=2M,refcount_bits=2 64M
+
+reft_offs=$(peek_file_be "$TEST_IMG" 48 8)
+refb_offs=$(peek_file_be "$TEST_IMG" $reft_offs 8)
+
+# We want to cover 4 GB, those are 2048 clusters, equivalent to
+# 4096 bit = 512 B.
+truncate -s 4G "$TEST_IMG"
+for ((in_refb_offs = 0; in_refb_offs < 512; in_refb_offs += 8)); do
+poke_file "$TEST_IMG" $((refb_offs + in_refb_offs)) \
+'\x55\x55\x55\x55\x55\x55\x55\x55'
+done
+
+$QEMU_IO -c 'write -c -P 42 0 2M' "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo '--- Check ---'
+
+# This should only print the leaked clusters in the first 4 GB
+_check_test_img | grep -v '^Leaked cluster '
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/272.out b/tests/qemu-iotests/272.out
new file mode 100644
index 00..35698b0e73
--- /dev/null
+++ b/tests/qemu-iotests/272.out
@@ -0,0 +1,10 @@
+QA output created by 272
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 2097152/2097152 bytes at offset 0
+2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+--- Check ---
+
+2044 leaked clusters were found on the image.
+This means waste of disk space, but no harm to data.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 095ed1b880..065040398d 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -283,3 +283,4 @@
 267 rw auto quick snapshot
 268 rw auto quick
 270 rw backing quick
+272 rw
-- 
2.23.0

[PULL 0/3] Block patches for 4.2.0-rc0/4.1.1

[Max Reitz]

The following changes since commit d0f90e1423b4f412adc620eee93e8bfef8af4117:

  Merge remote-tracking branch 
'remotes/kraxel/tags/audio-20191106-pull-request' into staging (2019-11-07 
09:21:52 +)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2019-11-07

for you to fetch changes up to b7cd2c11f76d27930f53d3cf26d7b695c78d613b:

  iotests: Add test for 4G+ compressed qcow2 write (2019-11-07 14:37:46 +0100)


Block patches for 4.2.0-rc0/4.1.1:
- Fix writing to compressed qcow2 images > 4 GB
- Fix size sanity check for qcow2 bitmaps


Max Reitz (2):
  qcow2: Fix QCOW2_COMPRESSED_SECTOR_MASK
  iotests: Add test for 4G+ compressed qcow2 write

Tuguoyi (1):
  qcow2-bitmap: Fix uint64_t left-shift overflow

 block/qcow2-bitmap.c   | 14 +--
 block/qcow2.h  |  2 +-
 tests/qemu-iotests/272 | 79 ++
 tests/qemu-iotests/272.out | 10 +
 tests/qemu-iotests/group   |  1 +
 5 files changed, 102 insertions(+), 4 deletions(-)
 create mode 100755 tests/qemu-iotests/272
 create mode 100644 tests/qemu-iotests/272.out

-- 
2.23.0

[PULL 1/3] qcow2-bitmap: Fix uint64_t left-shift overflow

[Max Reitz]

From: Tuguoyi 

There are two issues in In check_constraints_on_bitmap(),
1) The sanity check on the granularity will cause uint64_t
integer left-shift overflow when cluster_size is 2M and the
granularity is BIGGER than 32K.
2) The way to calculate image size that the maximum bitmap
supported can map to is a bit incorrect.
This patch fix it by add a helper function to calculate the
number of bytes needed by a normal bitmap in image and compare
it to the maximum bitmap bytes supported by qemu.

Fixes: 5f72826e7fc62167cf3a
Signed-off-by: Guoyi Tu 
Message-id: 4ba40cd1e7ee4a708b40899952e49...@h3c.com
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Reitz 
---
 block/qcow2-bitmap.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 98294a7696..ef9ef628a0 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -142,6 +142,13 @@ static int check_table_entry(uint64_t entry, int 
cluster_size)
 return 0;
 }
 
+static int64_t get_bitmap_bytes_needed(int64_t len, uint32_t granularity)
+{
+int64_t num_bits = DIV_ROUND_UP(len, granularity);
+
+return DIV_ROUND_UP(num_bits, 8);
+}
+
 static int check_constraints_on_bitmap(BlockDriverState *bs,
const char *name,
uint32_t granularity,
@@ -150,6 +157,7 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
 BDRVQcow2State *s = bs->opaque;
 int granularity_bits = ctz32(granularity);
 int64_t len = bdrv_getlength(bs);
+int64_t bitmap_bytes;
 
 assert(granularity > 0);
 assert((granularity & (granularity - 1)) == 0);
@@ -171,9 +179,9 @@ static int check_constraints_on_bitmap(BlockDriverState *bs,
 return -EINVAL;
 }
 
-if ((len > (uint64_t)BME_MAX_PHYS_SIZE << granularity_bits) ||
-(len > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size <<
-   granularity_bits))
+bitmap_bytes = get_bitmap_bytes_needed(len, granularity);
+if ((bitmap_bytes > (uint64_t)BME_MAX_PHYS_SIZE) ||
+(bitmap_bytes > (uint64_t)BME_MAX_TABLE_SIZE * s->cluster_size))
 {
 error_setg(errp, "Too much space will be occupied by the bitmap. "
"Use larger granularity");
-- 
2.23.0

Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option

[Eric Blake]

On 11/7/19 2:33 AM, Kevin Wolf wrote:



As a replacement nbd-server-add, I envisioned adding something like a
block-export-add, which would work the way that --export already does.
It would also come with query-block-exports and block-export-del, and it
wouldn't contain only NBD devices, but also vhost-user, FUSE, etc.
exports.

Now I'm wondering if the same would make sense for nbd-server-start.
Maybe an API change would even allow us to start multiple NBD servers
(e.g. listening on different IP addresses or using different tls-creds).


We want that (the ability to run multiple parallel NBD servers) anyway, 
to allow parallel incremental backups from different points in time to 
different clients.


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

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

[Vladimir Sementsov-Ogievskiy]

06.11.2019 22:19, Eric Blake wrote:
> On 10/18/19 9:36 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
>>> Maybe:
>>>
>>> if software doesn't know how to interpret the field, it may be safely 
>>> ignored unless a corresponding incompatible feature flag bit is set; 
>>> however, the field should be preserved unchanged when rewriting the image 
>>> header.
>>>
 +
 +For all additional fields zero value equals to absence of field (absence 
 is
 +when field.offset + field.size > @header_length). This implies
 +that if software want's to set fields up to some field not aligned to 
 multiply
 +of 8 it must align header up by zeroes. And on the other hand, if software
 +need some optional field which is absent it should assume that it's value 
 is
 +zero.
>>>
>>> Maybe:
>>>
>>> Each optional field that does not have a corresponding incompatible feature 
>>> bit must support the value 0 that gives the same default behavior as when 
>>> the optional field is omitted.
>>
>> Hmmm. That doesn't work, as "corresponding" is something not actually 
>> defined. Consider our zstd extension.
>>
>> It has corresponding incompatible bit, therefore, this sentence doesn't 
>> apply to it. But still, if incompatible bit is unset we can have this field. 
>> And it's zero value must correspond
>> to the absence of the field.
>>
>> So, additional field may use incomaptible bit only for subset of its values.
>>
>> But, I see, that you want to allow 0 value to not match field-absence if 
>> incompatible bit is set?
> 
> Not necessarily.  Rather, if the value of an unknown field can be safely 
> ignored, then it should default to 0.  If it cannot be safely ignored, then 
> that field will not be set to a non-zero value without also setting an 
> incompatible feature flag, so that software that does not know how to 
> interpret the field will fail to load the image because it also fails to 
> recognize the associated new incompatible feature bit.
> 
> But I'd really like Kevin's opinion on how much wording is worth adding.
> 
>>
>> So, may be
>>
>> Additional fields has the following compatible behavior by default:
> 
> s/has/have/
> 
>>
>> 1. If software doesn't know how to interpret the field, it may be safely 
>> ignored, other than preserving the field unchanged when rewriting the image 
>> header.
>> 2. Zeroed additional field gives the same behavior as when this field is 
>> omitted.
> 
> Both good.
> 
>>
>> This default behavior may be altered with help of incompatible feature bits. 
>> So, if, for example, additional field has corresponding incompatible feature 
>> bit, and it is set, we are sure that software which opens the image knows 
>> how to interpret the field, so,
>> 1. The field definitely will not be ignored when corresponding incompatible 
>> bit is set.
>> 2. The field may define any meaning it wants for zero value for the case 
>> when corresponding incompatible bit is set.
> 
> Rather wordy but seems accurate.  Perhaps compress it to:
> 
> 3. Any additional field whose value must not be ignored for correct handling 
> of the file will be accompanied by a corresponding incompatible feature bit.
> 
> and maybe even reorder it to list the points as:
> 
> Additional fields have the following compatibility rules:
> 1. Any additional field whose value must not be ignored for correct handling 
> of the file will be accompanied by a corresponding incompatible feature bit.

I'd like to stress, that incompatible bit is needed for incompatible value, not 
for the field itself. (So field may be accompanied by incompatible bit for some
it's values and for others - not), So, what about

1. If the value of the additional field must not be ignored for correct 
handling of the file, it will be accompanied by a corresponding incompatible 
feature bit.

> 2. If there are no unrecognized incompatible feature bits set, an additional 
> field may be safely ignored other than preserving its value when rewriting 
> the image header.

Sounds like we can ignore the field if we know its meaning and know its 
incompatible bit..

2. If there are no unrecognized incompatible feature bits set, an unknown 
additional field may be safely ignored other than preserving its value when 
rewriting the image header.

> 3. An explicit value of 0 will have the same behavior as when the field is 
> not present.

But it may be changed by incompatible bit..

3. An explicit value of 0 will have the same behavior as when the field is not 
present, if not altered by specific incompatible bit.

> 
> 
 +It's allowed for the header end to cut some field in the middle (in this 
 case
 +the field is considered as absent), but in this case the part of the field
 +which is covered by @header_length must be zeroed.
 +
 +    < ... No additional fields in the header currently ... >
>>>
>>> Do we even still need this if we require 8-byte alignment?  We'd never be 
>>> able to cut a single field in the middle

Re: [RFC PATCH 00/18] Add qemu-storage-daemon

[Kevin Wolf]

Am 07.11.2019 um 11:33 hat Daniel P. Berrangé geschrieben:
> On Thu, Oct 17, 2019 at 03:01:46PM +0200, Kevin Wolf wrote:
> > 2. I'm not completely sure if the command line syntax is the final
> >version that we want to support long-term. Many options directly use
> >QAPI visitors (--blockdev, --export, --nbd-server) and should be
> >fine. However, others use QemuOpts (--chardev, --monitor, --object).
> > 
> >This is the same as in the system emulator, so we wouldn't be adding
> >a new problem, but as there was talk about QAPIfying the command
> >line, and I wouldn't want later syntax changes or adding lots of
> >compatibility code to a new tool, I thought we should probably
> >discuss whether QAPIfying from the start would be an option.
> 
> I think that following what the QEMU emulators currently do for
> CLI args should be an explicit anti-goal, because we know that it is
> a long standing source of pain.  Fixing it in the emulator binaries
> is hard due to backward compatibility, but for this new binary we
> have a clean slate.
> 
> This feels like a good opportunity to implement & demonstrate what
> we think QEMU configuration ought to look like. Work done for this
> in the qemu-storage-daemon may well help us understand how we'll
> be able to move the QEMU emulators into a new scheme later.

It might be, which is why I'm asking. Now that the storage daemon has
missed 4.2, we have a little more time to decide what the command line
should look like in detail.

However, I don't think this is something that should delay the storage
daemon until after 5.0.

> My personal wish would be to have no use of QemuOpts at all.
> 
> Use GOptionContext *only* for parsing command line arguments
> related to execution of the daemon - ie things like --help,
> --version, --daemon, --pid-file.

I really don't believe that the solution for having too much variety in
option parsing is adding in yet another type. GOptionContext is not
something I'm considering at the moment.

But it's a getopt() replacement, not something that could actually parse
the more complex options, so it's a separate question anyway. If we ever
want to use it, we can replace getopt() in all binaries at once.

> The use a "--config /path/to/json/file" arg to point to the config
> file for everything else using QAPI schema to describe it fully.

If this implies that the storage daemon can only do useful things if you
specify a config file, I disagree.

I agree that it's not really nice if you can't use a config file to
specify a lengthy configuration and that supporting one would be good.

But it is at least equally unfriendly to require a config file for
simple configurations where using command line arguments is easily
possible.

> When loading the config file, things should be created in order
> in which they are specified. ie don't try and group things,
> otherwise we end up back with the horrific hacks for objects
> where some are created early & some late.

Yes. This is how the storage daemon command line works, too.

I think Markus already had some patches for command line QAPIfication
that were incomplete at least for the system emulator. It might be
easier to make it feature complete for the storage daemon because it
supports much less problematic options. Maybe he can post a useful
subset (if it's too much work to clean up the full thing right now) and
we can work from there.

The one that I expect to be a bit tricky to be QAPIfied is --object.

> For an ambitious stretch goal, I think we should seriously consider
> whether our use of chardevs is appropriate in all cases that exist,
> and whether we can avoid the trap of over-using chardev in the new
> storage daemon since it is a clean slate in terms of user facing
> CLI config.
> 
> chardevs are designed for & reasonably well suited to attaching to
> devices like serial ports, parallel ports, etc. You have a 1:1
> remote:local peer relationship. The transport is a dumb byte
> stream, nothing special needed on top & the user can cope with
> any type of chardev.
> 
> Many cases where we've used chardevs as a backend in QEMU are a
> poor fit. We just used chardevs as an "easy" way to configure a
> UNIX or TCP socket from the CLI, and don't care about, nor work
> with, any othuer chardev backends. As a result of this misuse
> we've had to put in an increasing number of hacks in the chardev
> code to deal with fact that callers want to know about  & use
> socket semantics. eg FD passing, the chardev reconnection polling
> code.
> 
> The monitor is a prime example of a bad fit - it would be better
> suited by simply referencing a SocketAddress QAPI type, instead
> of having the chardev indirection. It would then directly use
> the QIOChannelSocket APIs and avoid the inconvenient chardev
> abstractions which are a source of complexity & instability for
> no net gain.  vhostuser is another prime example, responsible
> for much of the complexity & bugs recently added

Re: [PATCH v2 20/21] iotests: Disable data_file where it cannot be used

[Max Reitz]

On 06.11.19 16:52, Maxim Levitsky wrote:
> On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/007 | 5 +++--
>>  tests/qemu-iotests/014 | 2 ++
>>  tests/qemu-iotests/015 | 5 +++--
>>  tests/qemu-iotests/026 | 5 -
>>  tests/qemu-iotests/029 | 5 +++--
>>  tests/qemu-iotests/031 | 6 +++---
>>  tests/qemu-iotests/036 | 5 +++--
>>  tests/qemu-iotests/039 | 3 +++
>>  tests/qemu-iotests/046 | 2 ++
>>  tests/qemu-iotests/048 | 2 ++
>>  tests/qemu-iotests/051 | 5 +++--
>>  tests/qemu-iotests/058 | 5 +++--
>>  tests/qemu-iotests/060 | 6 --
>>  tests/qemu-iotests/061 | 6 --
>>  tests/qemu-iotests/062 | 2 +-
>>  tests/qemu-iotests/066 | 2 +-
>>  tests/qemu-iotests/067 | 6 --
>>  tests/qemu-iotests/068 | 5 +++--
>>  tests/qemu-iotests/071 | 3 +++
>>  tests/qemu-iotests/073 | 2 ++
>>  tests/qemu-iotests/074 | 2 ++
>>  tests/qemu-iotests/080 | 5 +++--
>>  tests/qemu-iotests/090 | 2 ++
>>  tests/qemu-iotests/098 | 6 --
>>  tests/qemu-iotests/099 | 3 ++-
>>  tests/qemu-iotests/103 | 5 +++--
>>  tests/qemu-iotests/108 | 6 --
>>  tests/qemu-iotests/112 | 5 +++--
>>  tests/qemu-iotests/114 | 2 ++
>>  tests/qemu-iotests/121 | 3 +++
>>  tests/qemu-iotests/138 | 2 ++
>>  tests/qemu-iotests/156 | 2 ++
>>  tests/qemu-iotests/176 | 7 +--
>>  tests/qemu-iotests/191 | 2 ++
>>  tests/qemu-iotests/201 | 6 +++---
>>  tests/qemu-iotests/214 | 3 ++-
>>  tests/qemu-iotests/217 | 3 ++-
>>  tests/qemu-iotests/220 | 5 +++--
>>  tests/qemu-iotests/243 | 6 --
>>  tests/qemu-iotests/244 | 5 +++--
>>  tests/qemu-iotests/250 | 2 ++
>>  tests/qemu-iotests/267 | 5 +++--
>>  42 files changed, 117 insertions(+), 52 deletions(-)

[...]

>> diff --git a/tests/qemu-iotests/031 b/tests/qemu-iotests/031
>> index c44fcf91bb..646ecd593f 100755
>> --- a/tests/qemu-iotests/031
>> +++ b/tests/qemu-iotests/031
>> @@ -40,9 +40,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  # This tests qcow2-specific low-level functionality
>>  _supported_fmt qcow2
>>  _supported_proto file
>> -# We want to test compat=0.10, which does not support refcount widths
>> -# other than 16
>> -_unsupported_imgopts 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
>> +# We want to test compat=0.10, which does not support external data
>> +# files or refcount widths other than 16
>> +_unsupported_imgopts data_file 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> 
> This is maybe another reason to split this test for compat=0.10 and for 
> compat=1.1
> But still can be done later of course.

Hm, but I don’t really think this test is important for external data
files.  There is no I/O.

[...]

>> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
>> index bbaf0ef45b..512598421c 100755
>> --- a/tests/qemu-iotests/036
>> +++ b/tests/qemu-iotests/036
>> @@ -43,8 +43,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  # This tests qcow2-specific low-level functionality
>>  _supported_fmt qcow2
>>  _supported_proto file
>> -# Only qcow2v3 and later supports feature bits
>> -_unsupported_imgopts 'compat=0.10'
>> +# Only qcow2v3 and later supports feature bits;
>> +# qcow2.py does not support external data files
> 
> Minor nitpick, maybe tag this with TODO or so. No need to do now.

Hm, well, and the same applies here.  (Just not a very important test.)

[...]

>> diff --git a/tests/qemu-iotests/046 b/tests/qemu-iotests/046
>> index 4e03ead7b1..a066eec605 100755
>> --- a/tests/qemu-iotests/046
>> +++ b/tests/qemu-iotests/046
>> @@ -38,6 +38,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  
>>  _supported_fmt qcow2
>>  _supported_proto file
>> +# data_file does not support compressed clusters
>> +_unsupported_imgopts data_file
> This is a very nice test, which doesn't seem to  use compressed
> clusters that much. I think it should be split as well.
> No need to do this now of course, but maybe mark with TODO to 
> avoid loosing that info.

The other problem is that blkdebug doesn’t work so well with external
data files, so basically this whole test doesn’t work.

[...]

>> diff --git a/tests/qemu-iotests/048 b/tests/qemu-iotests/048
>> index a8feb76184..2af6b74b41 100755
>> --- a/tests/qemu-iotests/048
>> +++ b/tests/qemu-iotests/048
>> @@ -49,6 +49,8 @@ _compare()
>>  _supported_fmt raw qcow2 qed luks
>>  _supported_proto file
>>  _supported_os Linux
>> +# Using 'cp' is incompatible with external data files
>> +_unsupported_imgopts data_file
> You could compare the external files instead in theory *I think*.
> Another item on some TODO list I guess.

This is a test of qemu-img compare, not of the image format.  So it
doesn’t make much sense to me to compare the external files, and also it
should be completely sufficient to run this test only without external
data files.

[...]

>> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
>> index 9cd1d60d45..0053bad46a 100755
>> --- a/tests/qemu-iotests/051
>> +++ b/tests/qemu-iotests/051
>> @@ -39,8 +39,9 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  _sup

Re: [RFC PATCH 18/18] qemu-storage-daemon: Add --monitor option

[Max Reitz]

On 07.11.19 11:12, Kevin Wolf wrote:
> Am 06.11.2019 um 15:32 hat Max Reitz geschrieben:
>> On 17.10.19 15:02, Kevin Wolf wrote:
>>> This adds and parses the --monitor option, so that a QMP monitor can be
>>> used in the storage daemon. The monitor offers commands defined in the
>>> QAPI schema at storage-daemon/qapi/qapi-schema.json.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> ---
>>>  storage-daemon/qapi/qapi-schema.json | 15 
>>>  qemu-storage-daemon.c| 34 
>>>  Makefile | 30 
>>>  Makefile.objs|  4 ++--
>>>  monitor/Makefile.objs|  2 ++
>>>  qapi/Makefile.objs   |  5 
>>>  qom/Makefile.objs|  1 +
>>>  scripts/qapi/gen.py  |  5 
>>>  storage-daemon/Makefile.objs |  1 +
>>>  storage-daemon/qapi/Makefile.objs|  1 +
>>>  10 files changed, 96 insertions(+), 2 deletions(-)
>>>  create mode 100644 storage-daemon/qapi/qapi-schema.json
>>>  create mode 100644 storage-daemon/Makefile.objs
>>>  create mode 100644 storage-daemon/qapi/Makefile.objs
>>
>> [...]
>>
>>> diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
>>> index 3e04e299ed..03d256f0a4 100644
>>> --- a/qapi/Makefile.objs
>>> +++ b/qapi/Makefile.objs
>>> @@ -30,3 +30,8 @@ obj-y += $(QAPI_TARGET_MODULES:%=qapi-events-%.o)
>>>  obj-y += qapi-events.o
>>>  obj-y += $(QAPI_TARGET_MODULES:%=qapi-commands-%.o)
>>>  obj-y += qapi-commands.o
>>> +
>>> +QAPI_MODULES_STORAGE_DAEMON = block block-core char common crypto 
>>> introspect
>>> +QAPI_MODULES_STORAGE_DAEMON += job monitor qom sockets pragma transaction
>>
>> I took a look into the rest, and I wonder whether query-iothreads from
>> misc.json would be useful, too.
> 
> Possibly. It would be a separate patch, but I can add it.
> 
> The question is just where to move query-iothreads. Do we have a good
> place, or do I need to separate misc.json and a new misc-sysemu.json?

I’d just put it in block.json because of the “io”... O:-)

>>> diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
>>> index 796c17c38a..c25634f673 100644
>>> --- a/scripts/qapi/gen.py
>>> +++ b/scripts/qapi/gen.py
>>> @@ -44,6 +44,11 @@ class QAPIGen(object):
>>>  return ''
>>>  
>>>  def write(self, output_dir):
>>> +# Include paths starting with ../ are used to reuse modules of the 
>>> main
>>> +# schema in specialised schemas. Don't overwrite the files that are
>>> +# already generated for the main schema.
>>> +if self.fname.startswith('../'):
>>> +return
>>
>> Sorry, but I’m about to ask a clueless question: How do we ensure that
>> the main schema is generated before something else could make sure of
>> the specialized schemas?
> 
> "Make sure"?

Oops, s/ sure/ use/.

> I think the order of the generation doesn't matter because generating
> the storage daemon files doesn't actually access the main ones.
> Generated C files shouldn't be a problem either because if we link an
> object file into a binary, we have a make dependency for it.

I was mostly wondering about the fact that make mustn’t try to compile
the “generated files” (which aren’t really generated here) before they
are actually generated when the main schema is processed.

Max

> Maybe the only a bit trickier question is whether we have the
> dependencies right so that qemu-storage-daemon.c is only built after the
> header files of both the main schema and the specific one have been
> generated. If I understand the Makefile correctly, generated-files-y
> takes care of this, and this patch adds all new header files to it if I
> didn't miss any.
> 
> Kevin
> 




signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCH 00/18] Add qemu-storage-daemon

[Daniel P . Berrangé]

On Thu, Oct 17, 2019 at 03:01:46PM +0200, Kevin Wolf wrote:
> This series adds a new tool 'qemu-storage-daemon', which can be used to
> export and perform operations on block devices. There is some overlap
> between qemu-img/qemu-nbd and the new qemu-storage-daemon, but there are
> a few important differences:
> 
> * The qemu-storage-daemon has QMP support. The command set is obviously
>   restricted compared to the system emulator because there is no guest,
>   but all of the block operations are present.
> 
>   This means that it can access advanced options or operations that the
>   qemu-img command line doesn't expose. For example, blockdev-create is
>   a lot more powerful than 'qemu-img create', and qemu-storage-daemon
>   allows to execute it without starting a guest.
> 
>   Compared to qemu-nbd it means that, for example, block jobs can now be
>   executed on the server side, and backing chains shared by multiple VMs
>   can be modified this way.
> 
> * The existing tools all have a separately invented one-off syntax for
>   the job at hand, which usually comes with restrictions compared to the
>   system emulator. qemu-storage-daemon shares the same syntax with the
>   system emulator for most options and prefers QAPI based interfaces
>   where possible (such as --blockdev), so it should be easy to make use
>   of in libvirt.
> 
> * While this series implements only NBD exports, the storage daemon is
>   intended to serve multiple protocols and its syntax reflects this. In
>   the past, we had proposals to add new one-off tools for exporting over
>   new protocols like FUSE or TCMU.
> 
>   With a generic storage daemon, additional export methods have a home
>   without adding a new tool for each of them.
> 
> I'm posting this as an RFC mainly for two reasons:
> 
> 1. The monitor integration, which could be argued to be a little hackish
>(some generated QAPI source files are built from a separate QAPI
>schema, but the per-module ones are taken from the system emulator)
>and Markus will want to have a closer look there. But from the IRC
>discussions we had, we seem to agree on the general approach here.
> 
> 2. I'm not completely sure if the command line syntax is the final
>version that we want to support long-term. Many options directly use
>QAPI visitors (--blockdev, --export, --nbd-server) and should be
>fine. However, others use QemuOpts (--chardev, --monitor, --object).
> 
>This is the same as in the system emulator, so we wouldn't be adding
>a new problem, but as there was talk about QAPIfying the command
>line, and I wouldn't want later syntax changes or adding lots of
>compatibility code to a new tool, I thought we should probably
>discuss whether QAPIfying from the start would be an option.

I think that following what the QEMU emulators currently do for
CLI args should be an explicit anti-goal, because we know that it is
a long standing source of pain.  Fixing it in the emulator binaries
is hard due to backward compatibility, but for this new binary we
have a clean slate.

This feels like a good opportunity to implement & demonstrate what
we think QEMU configuration ought to look like. Work done for this
in the qemu-storage-daemon may well help us understand how we'll
be able to move the QEMU emulators into a new scheme later.

My personal wish would be to have no use of QemuOpts at all.

Use GOptionContext *only* for parsing command line arguments
related to execution of the daemon - ie things like --help,
--version, --daemon, --pid-file.

The use a "--config /path/to/json/file" arg to point to the config
file for everything else using QAPI schema to describe it fully.

When loading the config file, things should be created in order
in which they are specified. ie don't try and group things,
otherwise we end up back with the horrific hacks for objects
where some are created early & some late.



For an ambitious stretch goal, I think we should seriously consider
whether our use of chardevs is appropriate in all cases that exist,
and whether we can avoid the trap of over-using chardev in the new
storage daemon since it is a clean slate in terms of user facing
CLI config.

chardevs are designed for & reasonably well suited to attaching to
devices like serial ports, parallel ports, etc. You have a 1:1
remote:local peer relationship. The transport is a dumb byte
stream, nothing special needed on top & the user can cope with
any type of chardev.

Many cases where we've used chardevs as a backend in QEMU are a
poor fit. We just used chardevs as an "easy" way to configure a
UNIX or TCP socket from the CLI, and don't care about, nor work
with, any othuer chardev backends. As a result of this misuse
we've had to put in an increasing number of hacks in the chardev
code to deal with fact that callers want to know about  & use
socket semantics. eg FD passing, the chardev reconnection polling
code.

The monitor is a prime example of a bad fit - it would

Re: [RFC PATCH 18/18] qemu-storage-daemon: Add --monitor option

[Kevin Wolf]

Am 06.11.2019 um 15:32 hat Max Reitz geschrieben:
> On 17.10.19 15:02, Kevin Wolf wrote:
> > This adds and parses the --monitor option, so that a QMP monitor can be
> > used in the storage daemon. The monitor offers commands defined in the
> > QAPI schema at storage-daemon/qapi/qapi-schema.json.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  storage-daemon/qapi/qapi-schema.json | 15 
> >  qemu-storage-daemon.c| 34 
> >  Makefile | 30 
> >  Makefile.objs|  4 ++--
> >  monitor/Makefile.objs|  2 ++
> >  qapi/Makefile.objs   |  5 
> >  qom/Makefile.objs|  1 +
> >  scripts/qapi/gen.py  |  5 
> >  storage-daemon/Makefile.objs |  1 +
> >  storage-daemon/qapi/Makefile.objs|  1 +
> >  10 files changed, 96 insertions(+), 2 deletions(-)
> >  create mode 100644 storage-daemon/qapi/qapi-schema.json
> >  create mode 100644 storage-daemon/Makefile.objs
> >  create mode 100644 storage-daemon/qapi/Makefile.objs
> 
> [...]
> 
> > diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
> > index 3e04e299ed..03d256f0a4 100644
> > --- a/qapi/Makefile.objs
> > +++ b/qapi/Makefile.objs
> > @@ -30,3 +30,8 @@ obj-y += $(QAPI_TARGET_MODULES:%=qapi-events-%.o)
> >  obj-y += qapi-events.o
> >  obj-y += $(QAPI_TARGET_MODULES:%=qapi-commands-%.o)
> >  obj-y += qapi-commands.o
> > +
> > +QAPI_MODULES_STORAGE_DAEMON = block block-core char common crypto 
> > introspect
> > +QAPI_MODULES_STORAGE_DAEMON += job monitor qom sockets pragma transaction
> 
> I took a look into the rest, and I wonder whether query-iothreads from
> misc.json would be useful, too.

Possibly. It would be a separate patch, but I can add it.

The question is just where to move query-iothreads. Do we have a good
place, or do I need to separate misc.json and a new misc-sysemu.json?

> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > index 796c17c38a..c25634f673 100644
> > --- a/scripts/qapi/gen.py
> > +++ b/scripts/qapi/gen.py
> > @@ -44,6 +44,11 @@ class QAPIGen(object):
> >  return ''
> >  
> >  def write(self, output_dir):
> > +# Include paths starting with ../ are used to reuse modules of the 
> > main
> > +# schema in specialised schemas. Don't overwrite the files that are
> > +# already generated for the main schema.
> > +if self.fname.startswith('../'):
> > +return
> 
> Sorry, but I’m about to ask a clueless question: How do we ensure that
> the main schema is generated before something else could make sure of
> the specialized schemas?

"Make sure"?

I think the order of the generation doesn't matter because generating
the storage daemon files doesn't actually access the main ones.
Generated C files shouldn't be a problem either because if we link an
object file into a binary, we have a make dependency for it.

Maybe the only a bit trickier question is whether we have the
dependencies right so that qemu-storage-daemon.c is only built after the
header files of both the main schema and the specific one have been
generated. If I understand the Makefile correctly, generated-files-y
takes care of this, and this patch adds all new header files to it if I
didn't miss any.

Kevin


signature.asc
Description: PGP signature

Re: [PATCH v2 03/21] iotests: Add _filter_json_filename

[Maxim Levitsky]

On Thu, 2019-11-07 at 09:59 +0100, Max Reitz wrote:
> On 06.11.19 16:44, Maxim Levitsky wrote:
> > On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
> > > Signed-off-by: Max Reitz 
> > > ---
> > >  tests/qemu-iotests/common.filter | 24 
> > >  1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/tests/qemu-iotests/common.filter 
> > > b/tests/qemu-iotests/common.filter
> > > index 9f418b4881..63bc6f6f26 100644
> > > --- a/tests/qemu-iotests/common.filter
> > > +++ b/tests/qemu-iotests/common.filter
> > > @@ -227,5 +227,29 @@ _filter_qmp_empty_return()
> > >  grep -v '{"return": {}}'
> > >  }
> > >  
> > > +_filter_json_filename()
> > > +{
> > > +$PYTHON -c 'import sys
> > > +result, *fnames = sys.stdin.read().split("json:{")
> > 
> > Very minor nitpick, maybe I would give 'fnames' a more generic name,
> > since its is just result of a split, thus not really a list of filenames.
> > Feel free to ignore that though.
> 
> Hm...  It is a list of filenames, namely of all nested json:{}
> filenames.  I could call it fname_split, but I actually think fnames is
> not too wrong.

Makes sense, I guess leave it as is.

> 
> In any case, thanks for reviewing again!

No problem! Thanks to you too for making these tests more generic,
this is IMHO very very good thing, especially with all the qcow2
corruptions we see recently.


Best regards,
Maxim Levitsky


> 
> Max
> 

Re: [PATCH v2 05/21] iotests: Replace IMGOPTS by _unsupported_imgopts

[Maxim Levitsky]

On Thu, 2019-11-07 at 10:08 +0100, Max Reitz wrote:
> On 06.11.19 16:45, Maxim Levitsky wrote:
> > On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
> > > Some tests require compat=1.1 and thus set IMGOPTS='compat=1.1'
> > > globally.  That is not how it should be done; instead, they should
> > > simply set _unsupported_imgopts to compat=0.10 (compat=1.1 is the
> > > default anyway).
> > > 
> > > This makes the tests heed user-specified $IMGOPTS.  Some do not work
> > > with all image options, though, so we need to disable them accordingly.
> > > 
> > > Signed-off-by: Max Reitz 
> > > ---
> > >  tests/qemu-iotests/036 | 3 +--
> > >  tests/qemu-iotests/060 | 4 ++--
> > >  tests/qemu-iotests/062 | 3 ++-
> > >  tests/qemu-iotests/066 | 3 ++-
> > >  tests/qemu-iotests/068 | 3 ++-
> > >  tests/qemu-iotests/098 | 4 ++--
> > >  6 files changed, 11 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
> > > index 5f929ad3be..bbaf0ef45b 100755
> > > --- a/tests/qemu-iotests/036
> > > +++ b/tests/qemu-iotests/036
> > > @@ -43,9 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> > >  # This tests qcow2-specific low-level functionality
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > > -
> > >  # Only qcow2v3 and later supports feature bits
> > > -IMGOPTS="compat=1.1"
> > > +_unsupported_imgopts 'compat=0.10'
> > >  
> > >  echo
> > >  echo === Image with unknown incompatible feature bit ===
> > > diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
> > > index b91d8321bb..9c2ef42522 100755
> > > --- a/tests/qemu-iotests/060
> > > +++ b/tests/qemu-iotests/060
> > > @@ -48,6 +48,8 @@ _filter_io_error()
> > >  _supported_fmt qcow2
> > >  _supported_proto file
> > >  _supported_os Linux
> > > +# These tests only work for compat=1.1 images with refcount_bits=16
> > > +_unsupported_imgopts 'compat=0.10' 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> > 
> > Looks like the reason for that is that the test hardcodes (or guesses that 
> > is) various qcow2 structures
> > thing I have seen few times already in the iotests.
> > Not now but sometime in the future it would be nice to extend qcow2.py (or 
> > something
> > like that) to dump location of all qcow2 structures so that the guesswork 
> > could be eliminated.
> 
> With the peek_file* functions we have now it’s actually simple to dump
> that location ($(peek_file_be "$TEST_IMG" 48 8) for the refcount table
> offset, for example).
> 
> But it wouldn’t help, because compat=0.10 or refcount_bits != 16 won’t
> change those locations.  So the locations aren’t the reason why we need
> to forbid those options here.
> 
> The reason we need refcount_bits=16 is that we’re going to directly
> manipulate a refcount block.  To do so, we need to know the refcount
> width, and I don’t think it’s worth trying to implement something generic.
> 
> We need compat=1.1 because compat=0.10 doesn’t have feature bits, so
> there’s no “corrupt” bit there.
> 
> Max
> 
This makes sense! Sorry for the noise!

Best regards,
Maxim Levitsky

Re: [PATCH v2 10/21] iotests: Replace IMGOPTS= by -o

[Maxim Levitsky]

On Thu, 2019-11-07 at 10:20 +0100, Max Reitz wrote:
> On 06.11.19 16:47, Maxim Levitsky wrote:
> > On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
> > > Tests should not overwrite all user-supplied image options, but only add
> > > to it (which will effectively overwrite conflicting values).  Accomplish
> > > this by passing options to _make_test_img via -o instead of $IMGOPTS.
> > > 
> > > For some tests, there is no functional change because they already only
> > > appended options to IMGOPTS.  For these, this patch is just a
> > > simplification.
> > > 
> > > For others, this is a change, so they now heed user-specified $IMGOPTS.
> > > Some of those tests do not work with all image options, though, so we
> > > need to disable them accordingly.
> > > 
> > > Signed-off-by: Max Reitz 
> > > ---
> > >  tests/qemu-iotests/031 |  9 ---
> > >  tests/qemu-iotests/039 | 24 ++
> > >  tests/qemu-iotests/059 | 18 ++---
> > >  tests/qemu-iotests/060 |  6 ++---
> > >  tests/qemu-iotests/061 | 57 ++
> > >  tests/qemu-iotests/079 |  3 +--
> > >  tests/qemu-iotests/106 |  2 +-
> > >  tests/qemu-iotests/108 |  2 +-
> > >  tests/qemu-iotests/112 | 32 
> > >  tests/qemu-iotests/115 |  3 +--
> > >  tests/qemu-iotests/121 |  6 ++---
> > >  tests/qemu-iotests/125 |  2 +-
> > >  tests/qemu-iotests/137 |  2 +-
> > >  tests/qemu-iotests/138 |  3 +--
> > >  tests/qemu-iotests/175 |  2 +-
> > >  tests/qemu-iotests/190 |  2 +-
> > >  tests/qemu-iotests/191 |  3 +--
> > >  tests/qemu-iotests/220 |  4 ++-
> > >  tests/qemu-iotests/243 |  6 +++--
> > >  tests/qemu-iotests/244 | 10 +---
> > >  tests/qemu-iotests/250 |  3 +--
> > >  tests/qemu-iotests/265 |  2 +-
> > >  22 files changed, 100 insertions(+), 101 deletions(-)
> 
> [...]
> 
> > > @@ -161,7 +161,7 @@ _cleanup_test_img
> > >  
> > >  echo
> > >  echo "=== Testing 4TB monolithicFlat creation and IO ==="
> > > -IMGOPTS="subformat=monolithicFlat" _make_test_img 4T
> > > +_make_test_img -o "subformat=monolithicFlat" 4T
> > >  _img_info
> > >  $QEMU_IO -c "write -P 0xa 900G 512" "$TEST_IMG" | _filter_qemu_io
> > >  $QEMU_IO -c "read -v 900G 1024" "$TEST_IMG" | _filter_qemu_io
> > > @@ -170,7 +170,7 @@ _cleanup_test_img
> > >  echo
> > >  echo "=== Testing qemu-img map on extents ==="
> > >  for fmt in monolithicSparse twoGbMaxExtentSparse; do
> > > -IMGOPTS="subformat=$fmt" _make_test_img 31G
> > > +_make_test_img -o "subformat=$fmt" 31G
> > >  $QEMU_IO -c "write 65024 1k" "$TEST_IMG" | _filter_qemu_io
> > >  $QEMU_IO -c "write 2147483136 1k" "$TEST_IMG" | _filter_qemu_io
> > >  $QEMU_IO -c "write 5G 1k" "$TEST_IMG" | _filter_qemu_io
> > 
> > Looks good. Another test that pokes at guessed locations... :-)
> 
> Actually, no.  These are writes on the format, not the file itself.  The
> monolithicSparse subformat will store everything in a single file,
> whereas twoGbMaxExtentSparse will create one file per 2 GB of guest
> disk.  So the locations are chosen accordingly to that 2 GB limit ((1)
> something somewhere in the first extent, (2) something that wraps around
> the first 2 GB limit, so hits extents #0 and #1, and (3) something in
> the middle of extent #2.)
> 
> (The following qemu-img map call then verifies that it lands in the
> different files for twoGbMaxExtentSparse, and that monolithicSparse is
> at least indeed sparsely allocated.)
> 
> Max
Good to know, I missed this one.


Best regards,
Maxim Levitsky

Re: [PATCH v2 10/21] iotests: Replace IMGOPTS= by -o

[Max Reitz]

On 06.11.19 16:47, Maxim Levitsky wrote:
> On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
>> Tests should not overwrite all user-supplied image options, but only add
>> to it (which will effectively overwrite conflicting values).  Accomplish
>> this by passing options to _make_test_img via -o instead of $IMGOPTS.
>>
>> For some tests, there is no functional change because they already only
>> appended options to IMGOPTS.  For these, this patch is just a
>> simplification.
>>
>> For others, this is a change, so they now heed user-specified $IMGOPTS.
>> Some of those tests do not work with all image options, though, so we
>> need to disable them accordingly.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/031 |  9 ---
>>  tests/qemu-iotests/039 | 24 ++
>>  tests/qemu-iotests/059 | 18 ++---
>>  tests/qemu-iotests/060 |  6 ++---
>>  tests/qemu-iotests/061 | 57 ++
>>  tests/qemu-iotests/079 |  3 +--
>>  tests/qemu-iotests/106 |  2 +-
>>  tests/qemu-iotests/108 |  2 +-
>>  tests/qemu-iotests/112 | 32 
>>  tests/qemu-iotests/115 |  3 +--
>>  tests/qemu-iotests/121 |  6 ++---
>>  tests/qemu-iotests/125 |  2 +-
>>  tests/qemu-iotests/137 |  2 +-
>>  tests/qemu-iotests/138 |  3 +--
>>  tests/qemu-iotests/175 |  2 +-
>>  tests/qemu-iotests/190 |  2 +-
>>  tests/qemu-iotests/191 |  3 +--
>>  tests/qemu-iotests/220 |  4 ++-
>>  tests/qemu-iotests/243 |  6 +++--
>>  tests/qemu-iotests/244 | 10 +---
>>  tests/qemu-iotests/250 |  3 +--
>>  tests/qemu-iotests/265 |  2 +-
>>  22 files changed, 100 insertions(+), 101 deletions(-)

[...]

>> @@ -161,7 +161,7 @@ _cleanup_test_img
>>  
>>  echo
>>  echo "=== Testing 4TB monolithicFlat creation and IO ==="
>> -IMGOPTS="subformat=monolithicFlat" _make_test_img 4T
>> +_make_test_img -o "subformat=monolithicFlat" 4T
>>  _img_info
>>  $QEMU_IO -c "write -P 0xa 900G 512" "$TEST_IMG" | _filter_qemu_io
>>  $QEMU_IO -c "read -v 900G 1024" "$TEST_IMG" | _filter_qemu_io
>> @@ -170,7 +170,7 @@ _cleanup_test_img
>>  echo
>>  echo "=== Testing qemu-img map on extents ==="
>>  for fmt in monolithicSparse twoGbMaxExtentSparse; do
>> -IMGOPTS="subformat=$fmt" _make_test_img 31G
>> +_make_test_img -o "subformat=$fmt" 31G
>>  $QEMU_IO -c "write 65024 1k" "$TEST_IMG" | _filter_qemu_io
>>  $QEMU_IO -c "write 2147483136 1k" "$TEST_IMG" | _filter_qemu_io
>>  $QEMU_IO -c "write 5G 1k" "$TEST_IMG" | _filter_qemu_io
> 
> Looks good. Another test that pokes at guessed locations... :-)

Actually, no.  These are writes on the format, not the file itself.  The
monolithicSparse subformat will store everything in a single file,
whereas twoGbMaxExtentSparse will create one file per 2 GB of guest
disk.  So the locations are chosen accordingly to that 2 GB limit ((1)
something somewhere in the first extent, (2) something that wraps around
the first 2 GB limit, so hits extents #0 and #1, and (3) something in
the middle of extent #2.)

(The following qemu-img map call then verifies that it lands in the
different files for twoGbMaxExtentSparse, and that monolithicSparse is
at least indeed sparsely allocated.)

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 05/21] iotests: Replace IMGOPTS by _unsupported_imgopts

[Max Reitz]

On 06.11.19 16:45, Maxim Levitsky wrote:
> On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
>> Some tests require compat=1.1 and thus set IMGOPTS='compat=1.1'
>> globally.  That is not how it should be done; instead, they should
>> simply set _unsupported_imgopts to compat=0.10 (compat=1.1 is the
>> default anyway).
>>
>> This makes the tests heed user-specified $IMGOPTS.  Some do not work
>> with all image options, though, so we need to disable them accordingly.
>>
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/036 | 3 +--
>>  tests/qemu-iotests/060 | 4 ++--
>>  tests/qemu-iotests/062 | 3 ++-
>>  tests/qemu-iotests/066 | 3 ++-
>>  tests/qemu-iotests/068 | 3 ++-
>>  tests/qemu-iotests/098 | 4 ++--
>>  6 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
>> index 5f929ad3be..bbaf0ef45b 100755
>> --- a/tests/qemu-iotests/036
>> +++ b/tests/qemu-iotests/036
>> @@ -43,9 +43,8 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>  # This tests qcow2-specific low-level functionality
>>  _supported_fmt qcow2
>>  _supported_proto file
>> -
>>  # Only qcow2v3 and later supports feature bits
>> -IMGOPTS="compat=1.1"
>> +_unsupported_imgopts 'compat=0.10'
>>  
>>  echo
>>  echo === Image with unknown incompatible feature bit ===
>> diff --git a/tests/qemu-iotests/060 b/tests/qemu-iotests/060
>> index b91d8321bb..9c2ef42522 100755
>> --- a/tests/qemu-iotests/060
>> +++ b/tests/qemu-iotests/060
>> @@ -48,6 +48,8 @@ _filter_io_error()
>>  _supported_fmt qcow2
>>  _supported_proto file
>>  _supported_os Linux
>> +# These tests only work for compat=1.1 images with refcount_bits=16
>> +_unsupported_imgopts 'compat=0.10' 'refcount_bits=\([^1]\|.\([^6]\|$\)\)'
> Looks like the reason for that is that the test hardcodes (or guesses that 
> is) various qcow2 structures
> thing I have seen few times already in the iotests.
> Not now but sometime in the future it would be nice to extend qcow2.py (or 
> something
> like that) to dump location of all qcow2 structures so that the guesswork 
> could be eliminated.

With the peek_file* functions we have now it’s actually simple to dump
that location ($(peek_file_be "$TEST_IMG" 48 8) for the refcount table
offset, for example).

But it wouldn’t help, because compat=0.10 or refcount_bits != 16 won’t
change those locations.  So the locations aren’t the reason why we need
to forbid those options here.

The reason we need refcount_bits=16 is that we’re going to directly
manipulate a refcount block.  To do so, we need to know the refcount
width, and I don’t think it’s worth trying to implement something generic.

We need compat=1.1 because compat=0.10 doesn’t have feature bits, so
there’s no “corrupt” bit there.

Max



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 03/21] iotests: Add _filter_json_filename

[Max Reitz]

On 06.11.19 16:44, Maxim Levitsky wrote:
> On Tue, 2019-10-15 at 16:27 +0200, Max Reitz wrote:
>> Signed-off-by: Max Reitz 
>> ---
>>  tests/qemu-iotests/common.filter | 24 
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/common.filter 
>> b/tests/qemu-iotests/common.filter
>> index 9f418b4881..63bc6f6f26 100644
>> --- a/tests/qemu-iotests/common.filter
>> +++ b/tests/qemu-iotests/common.filter
>> @@ -227,5 +227,29 @@ _filter_qmp_empty_return()
>>  grep -v '{"return": {}}'
>>  }
>>  
>> +_filter_json_filename()
>> +{
>> +$PYTHON -c 'import sys
>> +result, *fnames = sys.stdin.read().split("json:{")
> 
> Very minor nitpick, maybe I would give 'fnames' a more generic name,
> since its is just result of a split, thus not really a list of filenames.
> Feel free to ignore that though.

Hm...  It is a list of filenames, namely of all nested json:{}
filenames.  I could call it fname_split, but I actually think fnames is
not too wrong.

In any case, thanks for reviewing again!

Max



signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCH 06/18] qemu-storage-daemon: Add --nbd-server option

[Kevin Wolf]

Am 06.11.2019 um 20:25 hat Eric Blake geschrieben:
> On 11/6/19 6:51 AM, Max Reitz wrote:
> > On 17.10.19 15:01, Kevin Wolf wrote:
> > > Add a --nbd-server option to qemu-storage-daemon to start the built-in
> > > NBD server right away. It maps the arguments for nbd-server-start to the
> > > command line.
> > 
> > Well, it doesn’t quite, because nbd-server-start takes a
> > SocketAddressLegacy, and this takes a SocketAddress.
> > 
> > On one hand I can understand why you would do it differently (especially
> > for command-line options), but on the other I find it a bit problematic
> > to have --nbd-server be slightly different from nbd-server-start when
> > both are intended to be the same.
> > 
> > My biggest problem though lies in the duplication in the QAPI schema.
> > If NbdServerOptions.addr were a SocketAddressLegacy, we could let
> > nbd-server-start’s options just be of type NbdServerOptions and thus get
> > rid of the duplication.
> 
> I would love to somehow deprecate the use of SocketAddressLegacy and get QMP
> nbd-server-start to accept SocketAddress instead.  Maybe it could be done by
> adding a new nbd-server-begin command in 5.0 with a saner wire layout, and
> deprecating nbd-server-start at that time; by the 5.2 release, we could then
> drop nbd-server-start.  But we're too late for 4.2.

As a replacement nbd-server-add, I envisioned adding something like a
block-export-add, which would work the way that --export already does.
It would also come with query-block-exports and block-export-del, and it
wouldn't contain only NBD devices, but also vhost-user, FUSE, etc.
exports.

Now I'm wondering if the same would make sense for nbd-server-start.
Maybe an API change would even allow us to start multiple NBD servers
(e.g. listening on different IP addresses or using different tls-creds).

Kevin