Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Gerd Hoffmann]

  Hi,

> It would be much nicer to do the wrapper the other way round, i.e.
> setting properties before the device is realized would update a
> configuration struct and realize would then call .create() with that
> struct. To me, this sounds much harder, though also a more useful state.

Well, in some places we already have separate config structs.  We have
NICConf for example, which is typically used like this:

struct USBNetState {
   USBDevice dev;
   [ ... ]
   NICConf conf;
   [ ... ]
};

and

static Property net_properties[] = {
DEFINE_NIC_PROPERTIES(USBNetState, conf),
DEFINE_PROP_END_OF_LIST(),
};

So I think we could:

  (1) move *all* properties into structs.
  (2) generate those structs from qapi schemas.
  (3) generate Property lists (or functions with
  object_class_property_add_*() calls) from qapi
  schema.

We could then convert devices one-by-one without breaking anything
or needing two code paths essentially doing the same thing in two
different ways.

take care,
  Gerd

Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes

[Alistair Francis]

On Wed, Dec 2, 2020 at 3:09 PM Bin Meng  wrote:
>
> Hi Alistair,
>
> On Thu, Dec 3, 2020 at 3:52 AM Alistair Francis  wrote:
> >
> > On Sun, Nov 29, 2020 at 6:55 PM Bin Meng  wrote:
> > >
> > > From: Bin Meng 
> > >
> > > SST flashes require a dummy byte after the address bits.
> > >
> > > Signed-off-by: Bin Meng 
> >
> > I couldn't find a datasheet that says this... But the actual code
> > change looks fine, so:
> >
>
> Please find the SST25VF016B datasheet at
> http://ww1.microchip.com/downloads/en/devicedoc/s71271_04.pdf. The
> fast read sequence is on page 11.

Ah cool. I thought it would be somewhere, I just couldn't find it.

Alistair

>
> > Acked-by: Alistair Francis 
> >
>
> Thanks!
>
> Regards,
> Bin

Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes

[Bin Meng]

Hi Alistair,

On Thu, Dec 3, 2020 at 3:52 AM Alistair Francis  wrote:
>
> On Sun, Nov 29, 2020 at 6:55 PM Bin Meng  wrote:
> >
> > From: Bin Meng 
> >
> > SST flashes require a dummy byte after the address bits.
> >
> > Signed-off-by: Bin Meng 
>
> I couldn't find a datasheet that says this... But the actual code
> change looks fine, so:
>

Please find the SST25VF016B datasheet at
http://ww1.microchip.com/downloads/en/devicedoc/s71271_04.pdf. The
fast read sequence is on page 11.

> Acked-by: Alistair Francis 
>

Thanks!

Regards,
Bin

Re: [PATCH] hw/block: m25p80: Fix fast read for SST flashes

[Alistair Francis]

On Sun, Nov 29, 2020 at 6:55 PM Bin Meng  wrote:
>
> From: Bin Meng 
>
> SST flashes require a dummy byte after the address bits.
>
> Signed-off-by: Bin Meng 

I couldn't find a datasheet that says this... But the actual code
change looks fine, so:

Acked-by: Alistair Francis 

Alistair

> ---
>
>  hw/block/m25p80.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 483925f..9b36762 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -825,6 +825,9 @@ static void decode_fast_read_cmd(Flash *s)
>  s->needed_bytes = get_addr_length(s);
>  switch (get_man(s)) {
>  /* Dummy cycles - modeled with bytes writes instead of bits */
> +case MAN_SST:
> +s->needed_bytes += 1;
> +break;
>  case MAN_WINBOND:
>  s->needed_bytes += 8;
>  break;
> --
> 2.7.4
>
>

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Wed, Dec 02, 2020 at 06:35:06PM +0100, Kevin Wolf wrote:
> Am 02.12.2020 um 17:05 hat Eduardo Habkost geschrieben:
> > > > Looks nice as end goal.  Then, these are a few questions I would
> > > > have about the transition plan:
> > > > 
> > > > Would it require changing both device implementation and device
> > > > users in lockstep?  Should we have a compatibility layer to allow
> > > > existing qdev_new()+qdev_prop_set_*() code to keep working after
> > > > the devices are converted to the new system?  If not, why not?
> > > 
> > > Technically, it doesn't strictly require a lockstep update. You can have
> > > two code paths leading to a fully initialised device, one being the
> > > traditional way of setting properties and finally calling dc->realize,
> > > the other being a new method that takes the configuration in its
> > > parameters and also sets dev->realized = true at the end.
> > > 
> > > If at all possible, I would however prefer a lockstep update because
> > > having two paths is a weird intermediate state and the code paths could
> > > easily diverge. Keeping the old way around for a device also means that
> > > property setters are still doing two different jobs (initial
> > > configuration and updates at runtime).
> > 
> > I'd like to understand better how that intermediate state would
> > look like and why there's risk of separate code paths diverging.
> >
> > Could we have an intermediate state that doesn't require any
> > duplication and thus have no separate code paths that could
> > diverge?
> 
> The one requirement we have for an intermediate state is that it
> supports both interfaces: The well-know create/set properties/realize
> dance, and a new DeviceClass method, say .create(), that takes the
> configuration in parameters instead of relying on previously set
> properties.

I agree completely.

> 
> I assumed two separate implementations of transferring the configuration
> into the internal state. On second thought, this assumption is maybe
> wrong.
> 
> You can implement the new method as wrapper around the old way: It could
> just set all the properties and call realize. Of course, you don't win
> much in terms of improving the class implementation this way, but just
> support the new interface, but I guess it can be a reasonable
> intermediate step to resolve complicated dependencies etc.
> 
> It would be much nicer to do the wrapper the other way round, i.e.
> setting properties before the device is realized would update a
> configuration struct and realize would then call .create() with that
> struct. To me, this sounds much harder, though also a more useful state.

Comment about this below (look for [1]).

> 
> As you have worked a lot with properties recently, maybe you have a good
> idea how we could get an intermediate state closer to this?

I'd have to re-read this whole thread and think about it.

> 
> > > > If we add a compatibility layer, is the end goal to convert all
> > > > existing qdev_new() users to the new system?  If yes, why?  If
> > > > not, why not?
> > > 
> > > My personal goal is covering -object and -device, i.e. the external
> > > interfaces. Converting purely internally created devices is not as
> > > interesting (especially as long as we focus only on object creation),
> > > but might be desirable for consistency.
> > 
> > I wonder how much consistency we will lose and how much confusion
> > we'll cause if we end up with two completely separate methods for
> > creating devices.
> 
> I do think we should follow through and convert everything. It's just
> not my main motivation, and if the people who work more with qdev think
> it's better to leave that part unchanged (or that it won't make much of
> a difference), I won't insist.

This worries me.  Converting thousands of lines of code that
don't involve user-visible interfaces seems complicated and maybe
pointless.  On the other hand, having two separate APIs for
creating objects internally would cause confusion.

Maybe we should accept the fact that the 2 APIs will exist, and
address the confusion part: we should guarantee the two APIs to
be 100% equivalent, except for the fact that the newer one gives
us type safety in the C code.

I'd like to avoid a case like qdev vs QOM APIs, where they have
similar but slightly different features, and nobody knows which
one to use.

> 
> > > > What about subclasses?  Would base classes need to be converted
> > > > in lockstep with all subclasses?  How would the transition
> > > > process of (e.g.) PCI devices look like?
> > > 
> > > I don't think so.
> > > 
> > > If you want to convert base classes first, you may need to take the
> > > path shown above where both initialisation paths coexist while the
> > > children are converted because instantiation of a child class involves
> > > setting properties of the base class. So you can only restrict these
> > > properties to runtime-only after all children have been converted.
> > > 
> > > The other way around migh

[PATCH v13 10/10] block: apply COR-filter to block-stream jobs

[Andrey Shinkevich via]

This patch completes the series with the COR-filter applied to
block-stream operations. Adding the filter makes it possible for copied
regions to be discarded in backing files during the block-stream job,
what will reduce the disk overuse.
The COR-filter insertion incurs changes in the test case
245:test_block_stream_4 that reopens the backing chain during a
block-stream job. There are changes in the test #030 as well.
The test case 030:test_stream_parallel was deleted due to multiple
conflicts between the concurrent job operations over the same backing
chain. All the nodes involved into one job are being frozen, including
the filter node. Operations over the mentioned nodes, including the
filter one, are being blocked for other jobs. So, the filter node gets
involved into two concurrent jobs with the adjacent data node. That is
not allowed. It is what the test cases with overlapping jobs are about.
The concept of the parallel jobs with common nodes is considered vital
no more.

Signed-off-by: Andrey Shinkevich 
---
 block/stream.c | 97 ++
 tests/qemu-iotests/030 | 51 +++-
 tests/qemu-iotests/030.out |  4 +-
 tests/qemu-iotests/141.out |  2 +-
 tests/qemu-iotests/245 | 22 +++
 5 files changed, 86 insertions(+), 90 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 061268b..2f80fae 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -18,8 +18,10 @@
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
 #include "qemu/error-report.h"
+#include "qapi/qmp/qdict.h"
 #include "qemu/ratelimit.h"
 #include "sysemu/block-backend.h"
+#include "block/copy-on-read.h"
 
 enum {
 /*
@@ -34,6 +36,8 @@ typedef struct StreamBlockJob {
 BlockJob common;
 BlockDriverState *base_overlay; /* COW overlay (stream from this) */
 BlockDriverState *above_base;   /* Node directly above the base */
+BlockDriverState *cor_filter_bs;
+BlockDriverState *target_bs;
 BlockdevOnError on_error;
 char *backing_file_str;
 bool bs_read_only;
@@ -45,8 +49,7 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
 {
 assert(bytes < SIZE_MAX);
 
-return blk_co_preadv(blk, offset, bytes, NULL,
- BDRV_REQ_COPY_ON_READ | BDRV_REQ_PREFETCH);
+return blk_co_preadv(blk, offset, bytes, NULL, BDRV_REQ_PREFETCH);
 }
 
 static void stream_abort(Job *job)
@@ -54,24 +57,21 @@ static void stream_abort(Job *job)
 StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
 
 if (s->chain_frozen) {
-BlockJob *bjob = &s->common;
-bdrv_unfreeze_backing_chain(blk_bs(bjob->blk), s->above_base);
+bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
 }
 }
 
 static int stream_prepare(Job *job)
 {
 StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
-BlockJob *bjob = &s->common;
-BlockDriverState *bs = blk_bs(bjob->blk);
-BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
+BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
 BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
 BlockDriverState *base_unfiltered;
 BlockDriverState *backing_bs;
 Error *local_err = NULL;
 int ret = 0;
 
-bdrv_unfreeze_backing_chain(bs, s->above_base);
+bdrv_unfreeze_backing_chain(s->cor_filter_bs, s->above_base);
 s->chain_frozen = false;
 
 if (bdrv_cow_child(unfiltered_bs)) {
@@ -79,7 +79,7 @@ static int stream_prepare(Job *job)
 if (base) {
 base_id = s->backing_file_str;
 if (base_id) {
-backing_bs = bdrv_find_backing_image(bs, base_id);
+backing_bs = bdrv_find_backing_image(unfiltered_bs, base_id);
 if (backing_bs && backing_bs->drv) {
 base_fmt = backing_bs->drv->format_name;
 } else {
@@ -111,15 +111,16 @@ static void stream_clean(Job *job)
 {
 StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
 BlockJob *bjob = &s->common;
-BlockDriverState *bs = blk_bs(bjob->blk);
 
 /* Reopen the image back in read-only mode if necessary */
 if (s->bs_read_only) {
 /* Give up write permissions before making it read-only */
 blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
-bdrv_reopen_set_read_only(bs, true, NULL);
+bdrv_reopen_set_read_only(s->target_bs, true, NULL);
 }
 
+bdrv_cor_filter_drop(s->cor_filter_bs);
+
 g_free(s->backing_file_str);
 }
 
@@ -127,9 +128,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
 {
 StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
 BlockBackend *blk = s->common.blk;
-BlockDriverState *bs = blk_bs(blk);
-BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
-bool enable_cor = !bdrv_cow_child(s->base_overlay);
+BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_

[PATCH v13 03/10] copy-on-read: add filter drop function

[Andrey Shinkevich via]

Provide API for the COR-filter removal. Also, drop the filter child
permissions for an inactive state when the filter node is being
removed.
To insert the filter, the block generic layer function
bdrv_insert_node() can be used.
The new function bdrv_cor_filter_drop() may be considered as an
intermediate solution before the QEMU permission update system has
overhauled. Then we are able to implement the API function
bdrv_remove_node() on the block generic layer.

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/copy-on-read.c | 56 
 block/copy-on-read.h | 32 ++
 2 files changed, 88 insertions(+)
 create mode 100644 block/copy-on-read.h

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index cb03e0f..618c4c4 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -23,11 +23,20 @@
 #include "qemu/osdep.h"
 #include "block/block_int.h"
 #include "qemu/module.h"
+#include "qapi/error.h"
+#include "block/copy-on-read.h"
+
+
+typedef struct BDRVStateCOR {
+bool active;
+} BDRVStateCOR;
 
 
 static int cor_open(BlockDriverState *bs, QDict *options, int flags,
 Error **errp)
 {
+BDRVStateCOR *state = bs->opaque;
+
 bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
false, errp);
@@ -42,6 +51,13 @@ static int cor_open(BlockDriverState *bs, QDict *options, 
int flags,
 ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
 bs->file->bs->supported_zero_flags);
 
+state->active = true;
+
+/*
+ * We don't need to call bdrv_child_refresh_perms() now as the permissions
+ * will be updated later when the filter node gets its parent.
+ */
+
 return 0;
 }
 
@@ -57,6 +73,17 @@ static void cor_child_perm(BlockDriverState *bs, BdrvChild 
*c,
uint64_t perm, uint64_t shared,
uint64_t *nperm, uint64_t *nshared)
 {
+BDRVStateCOR *s = bs->opaque;
+
+if (!s->active) {
+/*
+ * While the filter is being removed
+ */
+*nperm = 0;
+*nshared = BLK_PERM_ALL;
+return;
+}
+
 *nperm = perm & PERM_PASSTHROUGH;
 *nshared = (shared & PERM_PASSTHROUGH) | PERM_UNCHANGED;
 
@@ -135,6 +162,7 @@ static void cor_lock_medium(BlockDriverState *bs, bool 
locked)
 
 static BlockDriver bdrv_copy_on_read = {
 .format_name= "copy-on-read",
+.instance_size  = sizeof(BDRVStateCOR),
 
 .bdrv_open  = cor_open,
 .bdrv_child_perm= cor_child_perm,
@@ -154,6 +182,34 @@ static BlockDriver bdrv_copy_on_read = {
 .is_filter  = true,
 };
 
+
+void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs)
+{
+BdrvChild *child;
+BlockDriverState *bs;
+BDRVStateCOR *s = cor_filter_bs->opaque;
+
+child = bdrv_filter_child(cor_filter_bs);
+if (!child) {
+return;
+}
+bs = child->bs;
+
+/* Retain the BDS until we complete the graph change. */
+bdrv_ref(bs);
+/* Hold a guest back from writing while permissions are being reset. */
+bdrv_drained_begin(bs);
+/* Drop permissions before the graph change. */
+s->active = false;
+bdrv_child_refresh_perms(cor_filter_bs, child, &error_abort);
+bdrv_replace_node(cor_filter_bs, bs, &error_abort);
+
+bdrv_drained_end(bs);
+bdrv_unref(bs);
+bdrv_unref(cor_filter_bs);
+}
+
+
 static void bdrv_copy_on_read_init(void)
 {
 bdrv_register(&bdrv_copy_on_read);
diff --git a/block/copy-on-read.h b/block/copy-on-read.h
new file mode 100644
index 000..7bf405d
--- /dev/null
+++ b/block/copy-on-read.h
@@ -0,0 +1,32 @@
+/*
+ * Copy-on-read filter block driver
+ *
+ * The filter driver performs Copy-On-Read (COR) operations
+ *
+ * Copyright (c) 2018-2020 Virtuozzo International GmbH.
+ *
+ * Author:
+ *   Andrey Shinkevich 
+ *
+ * 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 .
+ */
+
+#ifndef BLOCK_COPY_ON_READ
+#define BLOCK_COPY_ON_READ
+
+#include "block/block_int.h"
+
+void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
+
+#endif /* BLOCK_COPY_ON_READ */
-- 
1.8.3.1

[PATCH v13 01/10] copy-on-read: support preadv/pwritev_part functions

[Andrey Shinkevich via]

Add support for the recently introduced functions
bdrv_co_preadv_part()
and
bdrv_co_pwritev_part()
to the COR-filter driver.

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/copy-on-read.c | 28 
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 2816e61..cb03e0f 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -74,21 +74,25 @@ static int64_t cor_getlength(BlockDriverState *bs)
 }
 
 
-static int coroutine_fn cor_co_preadv(BlockDriverState *bs,
-  uint64_t offset, uint64_t bytes,
-  QEMUIOVector *qiov, int flags)
+static int coroutine_fn cor_co_preadv_part(BlockDriverState *bs,
+   uint64_t offset, uint64_t bytes,
+   QEMUIOVector *qiov,
+   size_t qiov_offset,
+   int flags)
 {
-return bdrv_co_preadv(bs->file, offset, bytes, qiov,
-  flags | BDRV_REQ_COPY_ON_READ);
+return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+   flags | BDRV_REQ_COPY_ON_READ);
 }
 
 
-static int coroutine_fn cor_co_pwritev(BlockDriverState *bs,
-   uint64_t offset, uint64_t bytes,
-   QEMUIOVector *qiov, int flags)
+static int coroutine_fn cor_co_pwritev_part(BlockDriverState *bs,
+uint64_t offset,
+uint64_t bytes,
+QEMUIOVector *qiov,
+size_t qiov_offset, int flags)
 {
-
-return bdrv_co_pwritev(bs->file, offset, bytes, qiov, flags);
+return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
+flags);
 }
 
 
@@ -137,8 +141,8 @@ static BlockDriver bdrv_copy_on_read = {
 
 .bdrv_getlength = cor_getlength,
 
-.bdrv_co_preadv = cor_co_preadv,
-.bdrv_co_pwritev= cor_co_pwritev,
+.bdrv_co_preadv_part= cor_co_preadv_part,
+.bdrv_co_pwritev_part   = cor_co_pwritev_part,
 .bdrv_co_pwrite_zeroes  = cor_co_pwrite_zeroes,
 .bdrv_co_pdiscard   = cor_co_pdiscard,
 .bdrv_co_pwritev_compressed = cor_co_pwritev_compressed,
-- 
1.8.3.1

[PATCH v13 02/10] block: add API function to insert a node

[Andrey Shinkevich via]

Provide API for insertion a node to backing chain.

Suggested-by: Max Reitz 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block.c   | 25 +
 include/block/block.h |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/block.c b/block.c
index f1cedac..b71c39f 100644
--- a/block.c
+++ b/block.c
@@ -4698,6 +4698,31 @@ static void bdrv_delete(BlockDriverState *bs)
 g_free(bs);
 }
 
+BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
+   int flags, Error **errp)
+{
+BlockDriverState *new_node_bs;
+Error *local_err = NULL;
+
+new_node_bs =  bdrv_open(NULL, NULL, node_options, flags, errp);
+if (new_node_bs == NULL) {
+error_prepend(errp, "Could not create node: ");
+return NULL;
+}
+
+bdrv_drained_begin(bs);
+bdrv_replace_node(bs, new_node_bs, &local_err);
+bdrv_drained_end(bs);
+
+if (local_err) {
+bdrv_unref(new_node_bs);
+error_propagate(errp, local_err);
+return NULL;
+}
+
+return new_node_bs;
+}
+
 /*
  * Run consistency checks on an image
  *
diff --git a/include/block/block.h b/include/block/block.h
index c9d7c58..81a3894 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -350,6 +350,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState 
*bs_top,
  Error **errp);
 void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
Error **errp);
+BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
+   int flags, Error **errp);
 
 int bdrv_parse_aio(const char *mode, int *flags);
 int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
-- 
1.8.3.1

[PATCH v13 06/10] iotests: add #310 to test bottom node in COR driver

[Andrey Shinkevich via]

The test case #310 is similar to #216 by Max Reitz. The difference is
that the test #310 involves a bottom node to the COR filter driver.

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 tests/qemu-iotests/310 | 114 +
 tests/qemu-iotests/310.out |  15 ++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 130 insertions(+)
 create mode 100755 tests/qemu-iotests/310
 create mode 100644 tests/qemu-iotests/310.out

diff --git a/tests/qemu-iotests/310 b/tests/qemu-iotests/310
new file mode 100755
index 000..c8b34cd
--- /dev/null
+++ b/tests/qemu-iotests/310
@@ -0,0 +1,114 @@
+#!/usr/bin/env python3
+#
+# Copy-on-read tests using a COR filter with a bottom node
+#
+# Copyright (C) 2018 Red Hat, Inc.
+# Copyright (c) 2020 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import iotests
+from iotests import log, qemu_img, qemu_io_silent
+
+# Need backing file support
+iotests.script_initialize(supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk'],
+  supported_platforms=['linux'])
+
+log('')
+log('=== Copy-on-read across nodes ===')
+log('')
+
+# This test is similar to the 216 one by Max Reitz 
+# The difference is that this test case involves a bottom node to the
+# COR filter driver.
+
+with iotests.FilePath('base.img') as base_img_path, \
+ iotests.FilePath('mid.img') as mid_img_path, \
+ iotests.FilePath('top.img') as top_img_path, \
+ iotests.VM() as vm:
+
+log('--- Setting up images ---')
+log('')
+
+assert qemu_img('create', '-f', iotests.imgfmt, base_img_path, '64M') == 0
+assert qemu_io_silent(base_img_path, '-c', 'write -P 1 0M 1M') == 0
+assert qemu_io_silent(base_img_path, '-c', 'write -P 1 3M 1M') == 0
+assert qemu_img('create', '-f', iotests.imgfmt, '-b', base_img_path,
+'-F', iotests.imgfmt, mid_img_path) == 0
+assert qemu_io_silent(mid_img_path,  '-c', 'write -P 3 2M 1M') == 0
+assert qemu_io_silent(mid_img_path,  '-c', 'write -P 3 4M 1M') == 0
+assert qemu_img('create', '-f', iotests.imgfmt, '-b', mid_img_path,
+'-F', iotests.imgfmt, top_img_path) == 0
+assert qemu_io_silent(top_img_path,  '-c', 'write -P 2 1M 1M') == 0
+
+#  0 1 2 3 4
+# top2
+# mid  3   3
+# base 1 1
+
+log('Done')
+
+log('')
+log('--- Doing COR ---')
+log('')
+
+vm.launch()
+
+log(vm.qmp('blockdev-add',
+   node_name='node0',
+   driver='copy-on-read',
+   bottom='node2',
+   file={
+   'driver': iotests.imgfmt,
+   'file': {
+   'driver': 'file',
+   'filename': top_img_path
+   },
+   'backing': {
+   'node-name': 'node2',
+   'driver': iotests.imgfmt,
+   'file': {
+   'driver': 'file',
+   'filename': mid_img_path
+   },
+   'backing': {
+   'driver': iotests.imgfmt,
+   'file': {
+   'driver': 'file',
+   'filename': base_img_path
+   }
+   },
+   }
+   }))
+
+# Trigger COR
+log(vm.qmp('human-monitor-command',
+   command_line='qemu-io node0 "read 0 5M"'))
+
+vm.shutdown()
+
+log('')
+log('--- Checking COR result ---')
+log('')
+
+assert qemu_io_silent(base_img_path, '-c', 'discard 0 4M') == 0
+assert qemu_io_silent(mid_img_path, '-c', 'discard 0M 5M') == 0
+assert qemu_io_silent(top_img_path,  '-c', 'read -P 0 0 1M') == 0
+assert qemu_io_silent(top_img_path,  '-c', 'read -P 2 1M 1M') == 0
+assert qemu_io_silent(top_img_path,  '-c', 'read -P 3 2M 1M') == 0
+assert qemu_io_silent(top_img_path,  '-c', 'read -P 0 3M 1M') == 0
+assert qemu_io_silent(top_img_path,  '-c', 'read -P 3 4M 1M') == 0
+
+log('Done')
diff --git a/tests/qemu-iotests/310.out b/tests/qemu-iotests/310.out
new file mode 100644
index 000..a70aa5c
--- /dev/null
+++ b/tests/qemu-iotests/310.out
@@ -0,0 +1,15 @@
+
+=== Copy-on-read across nodes ===
+
+--- Setting

[PATCH v13 09/10] stream: skip filters when writing backing file name to QCOW2 header

[Andrey Shinkevich via]

Avoid writing a filter JSON file name and a filter format name to QCOW2
image when the backing file is being changed after the block stream
job. It can occur due to a concurrent commit job on the same backing
chain.
A user is still able to assign the 'backing-file' parameter for a
block-stream job keeping in mind the possible issue mentioned above.
If the user does not specify the 'backing-file' parameter, QEMU will
assign it automatically.

Signed-off-by: Andrey Shinkevich 
---
 block/stream.c | 21 +++--
 blockdev.c |  8 +---
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 6e281c7..061268b 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -17,6 +17,7 @@
 #include "block/blockjob_int.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qerror.h"
+#include "qemu/error-report.h"
 #include "qemu/ratelimit.h"
 #include "sysemu/block-backend.h"
 
@@ -65,6 +66,8 @@ static int stream_prepare(Job *job)
 BlockDriverState *bs = blk_bs(bjob->blk);
 BlockDriverState *unfiltered_bs = bdrv_skip_filters(bs);
 BlockDriverState *base = bdrv_filter_or_cow_bs(s->above_base);
+BlockDriverState *base_unfiltered;
+BlockDriverState *backing_bs;
 Error *local_err = NULL;
 int ret = 0;
 
@@ -75,8 +78,22 @@ static int stream_prepare(Job *job)
 const char *base_id = NULL, *base_fmt = NULL;
 if (base) {
 base_id = s->backing_file_str;
-if (base->drv) {
-base_fmt = base->drv->format_name;
+if (base_id) {
+backing_bs = bdrv_find_backing_image(bs, base_id);
+if (backing_bs && backing_bs->drv) {
+base_fmt = backing_bs->drv->format_name;
+} else {
+error_report("Format not found for backing file %s",
+ s->backing_file_str);
+}
+} else {
+base_unfiltered = bdrv_skip_filters(base);
+if (base_unfiltered) {
+base_id = base_unfiltered->filename;
+if (base_unfiltered->drv) {
+base_fmt = base_unfiltered->drv->format_name;
+}
+}
 }
 }
 bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
diff --git a/blockdev.c b/blockdev.c
index c917625..70900f4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2508,7 +2508,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 BlockDriverState *base_bs = NULL;
 AioContext *aio_context;
 Error *local_err = NULL;
-const char *base_name = NULL;
 int job_flags = JOB_DEFAULT;
 
 if (!has_on_error) {
@@ -2536,7 +2535,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 goto out;
 }
 assert(bdrv_get_aio_context(base_bs) == aio_context);
-base_name = base;
 }
 
 if (has_base_node) {
@@ -2551,7 +2549,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 }
 assert(bdrv_get_aio_context(base_bs) == aio_context);
 bdrv_refresh_filename(base_bs);
-base_name = base_bs->filename;
 }
 
 /* Check for op blockers in the whole chain between bs and base */
@@ -2571,9 +2568,6 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 goto out;
 }
 
-/* backing_file string overrides base bs filename */
-base_name = has_backing_file ? backing_file : base_name;
-
 if (has_auto_finalize && !auto_finalize) {
 job_flags |= JOB_MANUAL_FINALIZE;
 }
@@ -2581,7 +2575,7 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 job_flags |= JOB_MANUAL_DISMISS;
 }
 
-stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+stream_start(has_job_id ? job_id : NULL, bs, base_bs, backing_file,
  job_flags, has_speed ? speed : 0, on_error,
  filter_node_name, &local_err);
 if (local_err) {
-- 
1.8.3.1

[PATCH v13 07/10] block: include supported_read_flags into BDS structure

[Andrey Shinkevich via]

Add the new member supported_read_flags to the BlockDriverState
structure. It will control the flags set for copy-on-read operations.
Make the block generic layer evaluate supported read flags before they
go to a block driver.

Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/io.c| 12 ++--
 include/block/block_int.h |  4 
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index ec5e152..e28b11c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1405,6 +1405,9 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild 
*child,
 if (flags & BDRV_REQ_COPY_ON_READ) {
 int64_t pnum;
 
+/* The flag BDRV_REQ_COPY_ON_READ has reached its addressee */
+flags &= ~BDRV_REQ_COPY_ON_READ;
+
 ret = bdrv_is_allocated(bs, offset, bytes, &pnum);
 if (ret < 0) {
 goto out;
@@ -1426,9 +1429,13 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild 
*child,
 goto out;
 }
 
+if (flags & ~bs->supported_read_flags) {
+abort();
+}
+
 max_bytes = ROUND_UP(MAX(0, total_bytes - offset), align);
 if (bytes <= max_bytes && bytes <= max_transfer) {
-ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset, 0);
+ret = bdrv_driver_preadv(bs, offset, bytes, qiov, qiov_offset, flags);
 goto out;
 }
 
@@ -1441,7 +1448,8 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild 
*child,
 
 ret = bdrv_driver_preadv(bs, offset + bytes - bytes_remaining,
  num, qiov,
- qiov_offset + bytes - bytes_remaining, 0);
+ qiov_offset + bytes - bytes_remaining,
+ flags);
 max_bytes -= num;
 } else {
 num = bytes_remaining;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c05fa1e..247e166 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -873,6 +873,10 @@ struct BlockDriverState {
 /* I/O Limits */
 BlockLimits bl;
 
+/*
+ * Flags honored during pread
+ */
+unsigned int supported_read_flags;
 /* Flags honored during pwrite (so far: BDRV_REQ_FUA,
  * BDRV_REQ_WRITE_UNCHANGED).
  * If a driver does not support BDRV_REQ_WRITE_UNCHANGED, those
-- 
1.8.3.1

[PATCH v13 00/10] Apply COR-filter to the block-stream permanently

[Andrey Shinkevich via]

The previous version 12 was discussed in the email thread:
Message-Id: <1603390423-980205-1-git-send-email-andrey.shinkev...@virtuozzo.com>

v13:
  02: The bdrv_remove_node() was dropped.
  05: Three patches with fixes were merged into one.
  06: Minor changes based on Vladimir's suggestions.
  08: Three patches with fixes were merged into one.
  09: The search for format_name of backing file was added.
  10: The flag BLK_PERM_GRAPH_MOD was removed.

Andrey Shinkevich (10):
  copy-on-read: support preadv/pwritev_part functions
  block: add API function to insert a node
  copy-on-read: add filter drop function
  qapi: add filter-node-name to block-stream
  qapi: create BlockdevOptionsCor structure for COR driver
  iotests: add #310 to test bottom node in COR driver
  block: include supported_read_flags into BDS structure
  copy-on-read: skip non-guest reads if no copy needed
  stream: skip filters when writing backing file name to QCOW2 header
  block: apply COR-filter to block-stream jobs

 block.c|  25 +++
 block/copy-on-read.c   | 143 +
 block/copy-on-read.h   |  32 +
 block/io.c |  12 +++-
 block/monitor/block-hmp-cmds.c |   4 +-
 block/stream.c | 120 +++---
 blockdev.c |  12 ++--
 include/block/block.h  |  10 ++-
 include/block/block_int.h  |  11 +++-
 qapi/block-core.json   |  27 +++-
 tests/qemu-iotests/030 |  51 ++-
 tests/qemu-iotests/030.out |   4 +-
 tests/qemu-iotests/141.out |   2 +-
 tests/qemu-iotests/245 |  22 +--
 tests/qemu-iotests/310 | 114 
 tests/qemu-iotests/310.out |  15 +
 tests/qemu-iotests/group   |   1 +
 17 files changed, 484 insertions(+), 121 deletions(-)
 create mode 100644 block/copy-on-read.h
 create mode 100755 tests/qemu-iotests/310
 create mode 100644 tests/qemu-iotests/310.out

-- 
1.8.3.1

[PATCH v13 05/10] qapi: create BlockdevOptionsCor structure for COR driver

[Andrey Shinkevich via]

Create the BlockdevOptionsCor structure for COR driver specific options
splitting it off form the BlockdevOptionsGenericFormat. The only option
'bottom' node in the structure denotes an image file that limits the
COR operations in the backing chain.
We are going to use the COR-filter for a block-stream job and will pass
a bottom node name to the COR driver. The bottom node is the first
non-filter overlay of the base. It was introduced because the base node
itself may change due to possible concurrent jobs.

Suggested-by: Max Reitz 
Suggested-by: Vladimir Sementsov-Ogievskiy 
Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/copy-on-read.c | 57 ++--
 qapi/block-core.json | 21 ++-
 2 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 618c4c4..2cddc96 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -24,18 +24,23 @@
 #include "block/block_int.h"
 #include "qemu/module.h"
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
 #include "block/copy-on-read.h"
 
 
 typedef struct BDRVStateCOR {
 bool active;
+BlockDriverState *bottom_bs;
 } BDRVStateCOR;
 
 
 static int cor_open(BlockDriverState *bs, QDict *options, int flags,
 Error **errp)
 {
+BlockDriverState *bottom_bs = NULL;
 BDRVStateCOR *state = bs->opaque;
+/* Find a bottom node name, if any */
+const char *bottom_node = qdict_get_try_str(options, "bottom");
 
 bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -51,7 +56,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, 
int flags,
 ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
 bs->file->bs->supported_zero_flags);
 
+if (bottom_node) {
+bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
+if (!bottom_bs) {
+error_setg(errp, "Bottom node '%s' not found", bottom_node);
+qdict_del(options, "bottom");
+return -EINVAL;
+}
+qdict_del(options, "bottom");
+}
 state->active = true;
+state->bottom_bs = bottom_bs;
 
 /*
  * We don't need to call bdrv_child_refresh_perms() now as the permissions
@@ -107,8 +122,46 @@ static int coroutine_fn 
cor_co_preadv_part(BlockDriverState *bs,
size_t qiov_offset,
int flags)
 {
-return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
-   flags | BDRV_REQ_COPY_ON_READ);
+int64_t n;
+int local_flags;
+int ret;
+BDRVStateCOR *state = bs->opaque;
+
+if (!state->bottom_bs) {
+return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+   flags | BDRV_REQ_COPY_ON_READ);
+}
+
+while (bytes) {
+local_flags = flags;
+
+/* In case of failure, try to copy-on-read anyway */
+ret = bdrv_is_allocated(bs->file->bs, offset, bytes, &n);
+if (ret <= 0) {
+ret = 
bdrv_is_allocated_above(bdrv_backing_chain_next(bs->file->bs),
+  state->bottom_bs, true, offset,
+  n, &n);
+if (ret == 1 || ret < 0) {
+local_flags |= BDRV_REQ_COPY_ON_READ;
+}
+/* Finish earlier if the end of a backing file has been reached */
+if (n == 0) {
+break;
+}
+}
+
+ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
+  local_flags);
+if (ret < 0) {
+return ret;
+}
+
+offset += n;
+qiov_offset += n;
+bytes -= n;
+}
+
+return 0;
 }
 
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8ef3df6..04055ef 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3942,6 +3942,25 @@
   'data': { 'throttle-group': 'str',
 'file' : 'BlockdevRef'
  } }
+
+##
+# @BlockdevOptionsCor:
+#
+# Driver specific block device options for the copy-on-read driver.
+#
+# @bottom: the name of a non-filter node (allocation-bearing layer) that limits
+#  the COR operations in the backing chain (inclusive).
+#  For the block-stream job, it will be the first non-filter overlay of
+#  the base node. We do not involve the base node into the COR
+#  operations because the base may change due to a concurrent
+#  block-commit job on the same backing chain.
+#
+# Since: 5.2
+##
+{ 'struct': 'BlockdevOptionsCor',
+  'base': 'BlockdevOptionsGenericFormat',
+  'data': { '*bottom': 'str' } }
+
 ##
 # @BlockdevOptions:
 #
@@ -3994,7 +4013,7 @@
   'bochs':  'BlockdevOptionsGener

[PATCH v13 08/10] copy-on-read: skip non-guest reads if no copy needed

[Andrey Shinkevich via]

If the flag BDRV_REQ_PREFETCH was set, skip idling read/write
operations in COR-driver. It can be taken into account for the
COR-algorithms optimization. That check is being made during the
block stream job by the moment.

Add the BDRV_REQ_PREFETCH flag to the supported_read_flags of the
COR-filter.

block: Modify the comment for the flag BDRV_REQ_PREFETCH as we are
going to use it alone and pass it to the COR-filter driver for further
processing.

Signed-off-by: Andrey Shinkevich 
---
 block/copy-on-read.c  | 14 ++
 include/block/block.h |  8 +---
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 2cddc96..123d197 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -49,6 +49,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int 
flags,
 return -EINVAL;
 }
 
+bs->supported_read_flags = BDRV_REQ_PREFETCH;
+
 bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
 (BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
 
@@ -150,10 +152,14 @@ static int coroutine_fn 
cor_co_preadv_part(BlockDriverState *bs,
 }
 }
 
-ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
-  local_flags);
-if (ret < 0) {
-return ret;
+/* Skip if neither read nor write are needed */
+if ((local_flags & (BDRV_REQ_PREFETCH | BDRV_REQ_COPY_ON_READ)) !=
+BDRV_REQ_PREFETCH) {
+ret = bdrv_co_preadv_part(bs->file, offset, n, qiov, qiov_offset,
+  local_flags);
+if (ret < 0) {
+return ret;
+}
 }
 
 offset += n;
diff --git a/include/block/block.h b/include/block/block.h
index 81a3894..3499554 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -81,9 +81,11 @@ typedef enum {
 BDRV_REQ_NO_FALLBACK= 0x100,
 
 /*
- * BDRV_REQ_PREFETCH may be used only together with BDRV_REQ_COPY_ON_READ
- * on read request and means that caller doesn't really need data to be
- * written to qiov parameter which may be NULL.
+ * BDRV_REQ_PREFETCH makes sense only in the context of copy-on-read
+ * (i.e., together with the BDRV_REQ_COPY_ON_READ flag or when a COR
+ * filter is involved), in which case it signals that the COR operation
+ * need not read the data into memory (qiov) but only ensure they are
+ * copied to the top layer (i.e., that COR operation is done).
  */
 BDRV_REQ_PREFETCH  = 0x200,
 /* Mask of valid flags */
-- 
1.8.3.1

[PATCH v13 04/10] qapi: add filter-node-name to block-stream

[Andrey Shinkevich via]

Provide the possibility to pass the 'filter-node-name' parameter to the
block-stream job as it is done for the commit block job.

Signed-off-by: Andrey Shinkevich 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
---
 block/monitor/block-hmp-cmds.c | 4 ++--
 block/stream.c | 4 +++-
 blockdev.c | 4 +++-
 include/block/block_int.h  | 7 ++-
 qapi/block-core.json   | 6 ++
 5 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index d15a2be..e8a58f3 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -508,8 +508,8 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
 
 qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
  false, NULL, qdict_haskey(qdict, "speed"), speed, true,
- BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
- &error);
+ BLOCKDEV_ON_ERROR_REPORT, false, NULL, false, false, 
false,
+ false, &error);
 
 hmp_handle_error(mon, error);
 }
diff --git a/block/stream.c b/block/stream.c
index 236384f..6e281c7 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -221,7 +221,9 @@ static const BlockJobDriver stream_job_driver = {
 void stream_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *base, const char *backing_file_str,
   int creation_flags, int64_t speed,
-  BlockdevOnError on_error, Error **errp)
+  BlockdevOnError on_error,
+  const char *filter_node_name,
+  Error **errp)
 {
 StreamBlockJob *s;
 BlockDriverState *iter;
diff --git a/blockdev.c b/blockdev.c
index fe6fb5d..c917625 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2499,6 +2499,7 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
   bool has_backing_file, const char *backing_file,
   bool has_speed, int64_t speed,
   bool has_on_error, BlockdevOnError on_error,
+  bool has_filter_node_name, const char *filter_node_name,
   bool has_auto_finalize, bool auto_finalize,
   bool has_auto_dismiss, bool auto_dismiss,
   Error **errp)
@@ -2581,7 +2582,8 @@ void qmp_block_stream(bool has_job_id, const char 
*job_id, const char *device,
 }
 
 stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
- job_flags, has_speed ? speed : 0, on_error, &local_err);
+ job_flags, has_speed ? speed : 0, on_error,
+ filter_node_name, &local_err);
 if (local_err) {
 error_propagate(errp, local_err);
 goto out;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 95d9333..c05fa1e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1134,6 +1134,9 @@ int is_windows_drive(const char *filename);
  *  See @BlockJobCreateFlags
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @on_error: The action to take upon error.
+ * @filter_node_name: The node name that should be assigned to the filter
+ * driver that the commit job inserts into the graph above @bs. NULL means
+ * that a node name should be autogenerated.
  * @errp: Error object.
  *
  * Start a streaming operation on @bs.  Clusters that are unallocated
@@ -1146,7 +1149,9 @@ int is_windows_drive(const char *filename);
 void stream_start(const char *job_id, BlockDriverState *bs,
   BlockDriverState *base, const char *backing_file_str,
   int creation_flags, int64_t speed,
-  BlockdevOnError on_error, Error **errp);
+  BlockdevOnError on_error,
+  const char *filter_node_name,
+  Error **errp);
 
 /**
  * commit_start:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 04ad80b..8ef3df6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2543,6 +2543,11 @@
 #'stop' and 'enospc' can only be used if the block device
 #supports io-status (see BlockInfo).  Since 1.3.
 #
+# @filter-node-name: the node name that should be assigned to the
+#filter driver that the stream job inserts into the graph
+#above @device. If this option is not given, a node name is
+#autogenerated. (Since: 5.2)
+#
 # @auto-finalize: When false, this job will wait in a PENDING state after it 
has
 # finished its work, waiting for @block-job-finalize before
 # making any block graph changes.
@@ -2573,6 +2578,7 @@
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
 '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
 '*on-error

Re: [PATCH v12 14/14] block: apply COR-filter to block-stream jobs

[Andrey Shinkevich]

On 27.10.2020 21:24, Andrey Shinkevich wrote:


On 27.10.2020 20:57, Vladimir Sementsov-Ogievskiy wrote:

27.10.2020 20:48, Andrey Shinkevich wrote:


On 27.10.2020 19:13, Vladimir Sementsov-Ogievskiy wrote:

22.10.2020 21:13, Andrey Shinkevich wrote:

This patch completes the series with the COR-filter insertion for
block-stream operations. Adding the filter makes it possible for 
copied

regions to be discarded in backing files during the block-stream job,
what will reduce the disk overuse.
The COR-filter insertion incurs changes in the iotests case
245:test_block_stream_4 that reopens the backing chain during a
block-stream job. There are changes in the iotests #030 as well.
The iotests case 030:test_stream_parallel was deleted due to multiple
conflicts between the concurrent job operations over the same backing
chain. The base backing node for one job is the top node for another
job. It may change due to the filter node inserted into the backing
chain while both jobs are running. Another issue is that the parts of
the backing chain are being frozen by the running job and may not be
changed by the concurrent job when needed. The concept of the parallel
jobs with common nodes is considered vital no more.

Signed-off-by: Andrey Shinkevich 
---
  block/stream.c | 98 
++

  tests/qemu-iotests/030 | 51 +++-
  tests/qemu-iotests/030.out |  4 +-
  tests/qemu-iotests/141.out |  2 +-
  tests/qemu-iotests/245 | 22 +++
  5 files changed, 87 insertions(+), 90 deletions(-)

diff --git a/block/stream.c b/block/stream.c



[...]

+    s = block_job_create(job_id, &stream_job_driver, NULL, 
cor_filter_bs,

+ BLK_PERM_CONSISTENT_READ,
+ basic_flags | BLK_PERM_WRITE | 
BLK_PERM_GRAPH_MOD,


I think that BLK_PERM_GRAPH_MOD is something outdated. We have 
chain-feeze, what BLK_PERM_GRAPH_MOD adds to it? I don't know, and 
doubt that somebody knows.




That is true for the commit/mirror jobs also. If we agree to remove 
the flag BLK_PERM_GRAPH_MOD from all these jobs, it will be made in a 
separate series, won't it?


Hmm. At least, let's not implement new logic based on 
BLK_PERM_GRAPH_MOD. In original code it's only block_job_create's 
perm, not in shared_perm, not somewhere else.. So, if we keep it, 
let's keep it as is: only in perm in block_job_create, not 
implementing additional perm/shared_perm logic.




With @perm=0 in the block_job_add_bdrv(&s->common, "active node"...), it 
won't.





   speed, creation_flags, NULL, NULL, errp);
  if (!s) {
  goto fail;
  }
+    /*
+ * Prevent concurrent jobs trying to modify the graph 
structure here, we
+ * already have our own plans. Also don't allow resize as the 
image size is

+ * queried only at the job start and then cached.
+ */
+    if (block_job_add_bdrv(&s->common, "active node", bs,
+   basic_flags | BLK_PERM_GRAPH_MOD,


why not 0, like for other nodes? We don't use this BdrvChild at all, 
why to requre permissions?




Yes, '0' s right.

+   basic_flags | BLK_PERM_WRITE, 
&error_abort)) {

+    goto fail;
+    }
+
  /* Block all intermediate nodes between bs and base, because 



[...]


diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index dcb4b5d..0064590 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -227,61 +227,20 @@ class TestParallelOps(iotests.QMPTestCase):
  for img in self.imgs:
  os.remove(img)
-    # Test that it's possible to run several block-stream operations
-    # in parallel in the same snapshot chain
-    @unittest.skipIf(os.environ.get('QEMU_CHECK_BLOCK_AUTO'), 
'disabled in CI')

-    def test_stream_parallel(self):


Didn't we agree to add "bottom" paramter to qmp? Than this test-case 
can be rewritten using

node-names and new "bottom" stream argument.



The QMP new "bottom" option is passed to the COR-driver. It is done 
withing the stream-job code. So, it works.




I guess it will not help for the whole test. Particularly, there is 
an issue with freezing the child link to COR-filter of the cuncurrent 
job, then it fails to finish first.


We should not have such frozen link, as our bottom node should be 
above COR-filter of concurrent job.





The bdrv_freeze_backing_chain(bs, above_base, errp) does that job. Max 
insisted on keeping it.


Andrey


I have kept the test_stream_parallel() deleted in the coming v13 because 
it was agreed to make the above_base node frozen. With this, the test 
case can not pass. It is also true because the operations over the 
COR-filter node are blocked for the parallel jobs.


Andrey

Re: Plans to bring QMP 'x-blockdev-reopen' out of experimental?

[Kevin Wolf]

Am 02.12.2020 um 17:40 hat Alberto Garcia geschrieben:
> On Wed 02 Dec 2020 05:28:08 PM CET, Kevin Wolf wrote:
> 
> >> So x-blockdev-reopen sees that we want to replace the current
> >> bs->file ("hd0-file") with a new one ("throttle0"). The problem here
> >> is that throttle0 has hd0-file as its child, so when we check the
> >> permissions on throttle0 (and its children) we get that hd0-file
> >> refuses because it's already being used (although in in the process
> >> of being replaced) by hd0:
> >> 
> >> "Conflicts with use by hd0 as 'file', which does not allow 'write, resize' 
> >> on hd0-file"
> >> 
> > This kind of situation isn't new, I believe some of the existing graph
> > changes (iirc in the context of block jobs) can cause the same problem.
> >
> > This is essentially why some functions in the permission system take a
> > GSList *ignore_children. So I believe the right thing to do here is
> > telling the permission system that it needs to check the situation
> > without the BdrvChild that links hd0 with hd0-file.
> 
> I had tried this already and it does work when inserting the filter (we
> know that 'hd0-file' is about to be detached from the parent so we can
> put it in the list) but I don't think it's so easy if we want to remove
> the filter, i.e.
> 
>hd0 -> throttle -> hd0-file ==> hd0 -> hd0-file
> 
> In this case we get a similar error, we want to make hd0-file a child of
> hd0 but it is being used by the throttle filter.
> 
> Telling bdrv_check_update_perm() to ignore hd0's current child
> (throttle) won't solve the problem.

Isn't this the very same case as removing e.g. a mirror filter from the
chain? I'm sure we have already solved this somewhere.

Hm, no, it might actually be different in that the throttle node
survives this, so we do have to check that the resulting graph is
valid. Do we need a combined operation to remove the throttle node from
the graph and immediately delete it?

> > I don't know the exact stack trace of your failure, so maybe this
> > parameter isn't available yet in the place where you need it, but in
> > the core functions it exists.
> 
> This is in bdrv_reopen_multiple(), in the same place where we are
> currently checking the permissions of the new backing file.

Oh, it's not happening while actually changing the links, but the check
before trying? I guess both would fail in this case anyway, but good to
know.

Kevin

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 02.12.2020 um 17:05 hat Eduardo Habkost geschrieben:
> > > Looks nice as end goal.  Then, these are a few questions I would
> > > have about the transition plan:
> > > 
> > > Would it require changing both device implementation and device
> > > users in lockstep?  Should we have a compatibility layer to allow
> > > existing qdev_new()+qdev_prop_set_*() code to keep working after
> > > the devices are converted to the new system?  If not, why not?
> > 
> > Technically, it doesn't strictly require a lockstep update. You can have
> > two code paths leading to a fully initialised device, one being the
> > traditional way of setting properties and finally calling dc->realize,
> > the other being a new method that takes the configuration in its
> > parameters and also sets dev->realized = true at the end.
> > 
> > If at all possible, I would however prefer a lockstep update because
> > having two paths is a weird intermediate state and the code paths could
> > easily diverge. Keeping the old way around for a device also means that
> > property setters are still doing two different jobs (initial
> > configuration and updates at runtime).
> 
> I'd like to understand better how that intermediate state would
> look like and why there's risk of separate code paths diverging.
>
> Could we have an intermediate state that doesn't require any
> duplication and thus have no separate code paths that could
> diverge?

The one requirement we have for an intermediate state is that it
supports both interfaces: The well-know create/set properties/realize
dance, and a new DeviceClass method, say .create(), that takes the
configuration in parameters instead of relying on previously set
properties.

I assumed two separate implementations of transferring the configuration
into the internal state. On second thought, this assumption is maybe
wrong.

You can implement the new method as wrapper around the old way: It could
just set all the properties and call realize. Of course, you don't win
much in terms of improving the class implementation this way, but just
support the new interface, but I guess it can be a reasonable
intermediate step to resolve complicated dependencies etc.

It would be much nicer to do the wrapper the other way round, i.e.
setting properties before the device is realized would update a
configuration struct and realize would then call .create() with that
struct. To me, this sounds much harder, though also a more useful state.

As you have worked a lot with properties recently, maybe you have a good
idea how we could get an intermediate state closer to this?

> > > If we add a compatibility layer, is the end goal to convert all
> > > existing qdev_new() users to the new system?  If yes, why?  If
> > > not, why not?
> > 
> > My personal goal is covering -object and -device, i.e. the external
> > interfaces. Converting purely internally created devices is not as
> > interesting (especially as long as we focus only on object creation),
> > but might be desirable for consistency.
> 
> I wonder how much consistency we will lose and how much confusion
> we'll cause if we end up with two completely separate methods for
> creating devices.

I do think we should follow through and convert everything. It's just
not my main motivation, and if the people who work more with qdev think
it's better to leave that part unchanged (or that it won't make much of
a difference), I won't insist.

> > > What about subclasses?  Would base classes need to be converted
> > > in lockstep with all subclasses?  How would the transition
> > > process of (e.g.) PCI devices look like?
> > 
> > I don't think so.
> > 
> > If you want to convert base classes first, you may need to take the
> > path shown above where both initialisation paths coexist while the
> > children are converted because instantiation of a child class involves
> > setting properties of the base class. So you can only restrict these
> > properties to runtime-only after all children have been converted.
> > 
> > The other way around might be easier: You will need to describe the
> > properties of base classes in the QAPI schema from the beginning, but
> > that doesn't mean that their initialisation code has to change just yet.
> > The child classes will need to forward the part of their configuration
> > that belongs to the base class. The downside is that this code will have
> > to be changed again when the base class is finally converted.
> > 
> > So we have options there, and we can decide case by case which one is
> > most appropriate for the specific class to be converted (depending on
> > how many child classes exist, how many properties are inherited, etc.)
> 
> Right now it's hard for me to understand what those intermediate
> states would look like.  It sounds like it requires too many
> complicated manual changes to be done by humans, and lots of room
> for mistakes when maintaining two parallel code paths.  I'd
> prefer to delegate the translation job to a machine

Re: Plans to bring QMP 'x-blockdev-reopen' out of experimental?

[Alberto Garcia]

On Wed 02 Dec 2020 05:28:08 PM CET, Kevin Wolf wrote:

>> So x-blockdev-reopen sees that we want to replace the current
>> bs->file ("hd0-file") with a new one ("throttle0"). The problem here
>> is that throttle0 has hd0-file as its child, so when we check the
>> permissions on throttle0 (and its children) we get that hd0-file
>> refuses because it's already being used (although in in the process
>> of being replaced) by hd0:
>> 
>> "Conflicts with use by hd0 as 'file', which does not allow 'write, resize' 
>> on hd0-file"
>> 
> This kind of situation isn't new, I believe some of the existing graph
> changes (iirc in the context of block jobs) can cause the same problem.
>
> This is essentially why some functions in the permission system take a
> GSList *ignore_children. So I believe the right thing to do here is
> telling the permission system that it needs to check the situation
> without the BdrvChild that links hd0 with hd0-file.

I had tried this already and it does work when inserting the filter (we
know that 'hd0-file' is about to be detached from the parent so we can
put it in the list) but I don't think it's so easy if we want to remove
the filter, i.e.

   hd0 -> throttle -> hd0-file ==> hd0 -> hd0-file

In this case we get a similar error, we want to make hd0-file a child of
hd0 but it is being used by the throttle filter.

Telling bdrv_check_update_perm() to ignore hd0's current child
(throttle) won't solve the problem.

> I don't know the exact stack trace of your failure, so maybe this
> parameter isn't available yet in the place where you need it, but in
> the core functions it exists.

This is in bdrv_reopen_multiple(), in the same place where we are
currently checking the permissions of the new backing file.

Berto

Re: Plans to bring QMP 'x-blockdev-reopen' out of experimental?

[Kevin Wolf]

Am 02.12.2020 um 17:12 hat Alberto Garcia geschrieben:
> On Tue 20 Oct 2020 10:23:33 AM CEST, Kevin Wolf wrote:
> >> I forgot to add, we still don't support changing bs->file with this
> >> command, so I guess that would be one blocker?
> >> 
> >> There's no other way of inserting filter nodes, or is there?
> >
> > Not that I'm aware of.
> >
> > So yes, changing bs->file is the one thing I had in mind for
> > implementing before we mark it stable.
> >
> > I'm not entirely sure if we should make some restrictions or allow
> > arbitrary changes. If it's only about filters, we could check that the
> > node returned by bdrv_skip_filters() stays the same. This would be
> > almost certainly safe (if the chain is not frozen, of course).
> >
> > If people want to dynamically insert non-filters (maybe quorum?), it
> > might be more restrictive than necessary, though.
> >
> > Other cases like inserting a qcow2 file in the chain where the old
> > child becomes the backing file of the newly inserted node should in
> > theory already be covered by blockdev-snapshot.
> 
> Hi,
> 
> I have been working a bit on this

Oh, nice! And you might have mentioned this just in time to stop me from
duplicating your work. There is a strong desire from libvirt to have a
stable blockdev-reopen in QEMU 6.0.

> and I have doubts about the following situation: let's say we have a
> normal qcow2 image with two BDS for format (node-name "hd0") and
> protocol ("hd0-file"):
> 
>hd0 -> hd0-file
> 
> { "execute": "blockdev-add", "arguments":
>{'driver': 'file', 'node-name': 'hd0-file', 'filename':  'hd0.qcow2 }}
> { "execute": "blockdev-add", "arguments":
>{'driver': 'qcow2', 'node-name': 'hd0', 'file': 'hd0-file'}}
> 
> Now we want to use x-blockdev-reopen to insert a throttle filter
> between them, so the result would be:
> 
>hd0 -> throttle -> hd0-file
> 
> First we add the filter:
> 
> { "execute": "object-add", "arguments":
>{ 'qom-type': 'throttle-group', 'id': 'group0',
>  'props': { 'limits': { 'iops-total': 1000 } } } }
> { "execute": "blockdev-add", "arguments":
>{ 'driver': 'throttle', 'node-name': 'throttle0',
>  'throttle-group': 'group0', 'file': "hd0-file" } }
> 
> And then we insert it:
> 
> { "execute": "x-blockdev-reopen", "arguments":
>{'driver': 'qcow2', 'node-name': 'hd0', 'file': 'throttle0'}}
> 
> So x-blockdev-reopen sees that we want to replace the current bs->file
> ("hd0-file") with a new one ("throttle0"). The problem here is that
> throttle0 has hd0-file as its child, so when we check the permissions on
> throttle0 (and its children) we get that hd0-file refuses because it's
> already being used (although in in the process of being replaced) by
> hd0:
> 
> "Conflicts with use by hd0 as 'file', which does not allow 'write, resize' on 
> hd0-file"
> 
> And we would get a similar problem with the reverse operation (removing
> the filter).

This kind of situation isn't new, I believe some of the existing graph
changes (iirc in the context of block jobs) can cause the same problem.

This is essentially why some functions in the permission system take a
GSList *ignore_children. So I believe the right thing to do here is
telling the permission system that it needs to check the situation
without the BdrvChild that links hd0 with hd0-file.

I don't know the exact stack trace of your failure, so maybe this
parameter isn't available yet in the place where you need it, but in the
core functions it exists.

Does this help or am I missing some details?

Kevin

Re: Plans to bring QMP 'x-blockdev-reopen' out of experimental?

[Alberto Garcia]

On Tue 20 Oct 2020 10:23:33 AM CEST, Kevin Wolf wrote:
>> I forgot to add, we still don't support changing bs->file with this
>> command, so I guess that would be one blocker?
>> 
>> There's no other way of inserting filter nodes, or is there?
>
> Not that I'm aware of.
>
> So yes, changing bs->file is the one thing I had in mind for
> implementing before we mark it stable.
>
> I'm not entirely sure if we should make some restrictions or allow
> arbitrary changes. If it's only about filters, we could check that the
> node returned by bdrv_skip_filters() stays the same. This would be
> almost certainly safe (if the chain is not frozen, of course).
>
> If people want to dynamically insert non-filters (maybe quorum?), it
> might be more restrictive than necessary, though.
>
> Other cases like inserting a qcow2 file in the chain where the old
> child becomes the backing file of the newly inserted node should in
> theory already be covered by blockdev-snapshot.

Hi,

I have been working a bit on this and I have doubts about the
following situation: let's say we have a normal qcow2 image with two
BDS for format (node-name "hd0") and protocol ("hd0-file"):

   hd0 -> hd0-file

{ "execute": "blockdev-add", "arguments":
   {'driver': 'file', 'node-name': 'hd0-file', 'filename':  'hd0.qcow2 }}
{ "execute": "blockdev-add", "arguments":
   {'driver': 'qcow2', 'node-name': 'hd0', 'file': 'hd0-file'}}

Now we want to use x-blockdev-reopen to insert a throttle filter
between them, so the result would be:

   hd0 -> throttle -> hd0-file

First we add the filter:

{ "execute": "object-add", "arguments":
   { 'qom-type': 'throttle-group', 'id': 'group0',
 'props': { 'limits': { 'iops-total': 1000 } } } }
{ "execute": "blockdev-add", "arguments":
   { 'driver': 'throttle', 'node-name': 'throttle0',
 'throttle-group': 'group0', 'file': "hd0-file" } }

And then we insert it:

{ "execute": "x-blockdev-reopen", "arguments":
   {'driver': 'qcow2', 'node-name': 'hd0', 'file': 'throttle0'}}

So x-blockdev-reopen sees that we want to replace the current bs->file
("hd0-file") with a new one ("throttle0"). The problem here is that
throttle0 has hd0-file as its child, so when we check the permissions on
throttle0 (and its children) we get that hd0-file refuses because it's
already being used (although in in the process of being replaced) by
hd0:

"Conflicts with use by hd0 as 'file', which does not allow 'write, resize' on 
hd0-file"

And we would get a similar problem with the reverse operation (removing
the filter).

Berto

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Wed, Dec 02, 2020 at 04:17:13PM +0100, Kevin Wolf wrote:
> Am 02.12.2020 um 14:54 hat Eduardo Habkost geschrieben:
> > On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:
> > > On 02/12/20 13:51, Eduardo Habkost wrote:
> > > > > > I'm liking the direction this is taking.  However, I would still
> > > > > > like to have a clearer and feasible plan that would work for
> > > > > > -device, -machine, and -cpu.
> > > > > 
> > > > > -cpu is not a problem since it's generally created with a static
> > > > > configuration (now done with global properties, in the future it 
> > > > > could be a
> > > > > struct).
> > > > 
> > > > It is a problem if it requires manually converting existing code
> > > > defining CPU properties and we don't have a transition plan.
> > > 
> > > We do not have to convert everything _if_ for some objects there are good
> > > reasons to do programmatically-generated properties.  CPUs might be one of
> > > those cases (or if we decide to convert them, they might endure some more
> > > code duplication than other devices because they have so many properties).
> > 
> > OK, we just need to agree on what the transition will look like
> > when we do it.  I think we should put as much care into
> > transition/glue infrastructure as we put into the new
> > infrastructure.
> > 
> > 
> > > 
> > > > Would a -device conversion also involve non-user-creatable
> > > > devices, or would we keep existing internal usage of QOM
> > > > properties?
> > > > 
> > > > Even if it's just for user-creatable devices, getting rid of QOM
> > > > property usage in devices sounds like a very ambitious goal.  I'd
> > > > like us to have a good transition plan, in addition to declaring
> > > > what's our ideal end goal.
> > > 
> > > For user-creatable objects Kevin is doing work in lockstep on all classes;
> > > but once we have the infrastructure for QAPI object configuration schemas 
> > > we
> > > can proceed in the other direction and operate on one device at a time.
> > > 
> > > With some handwaving, something like (see create_unimplemented_device)
> > > 
> > > DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> > > 
> > > qdev_prop_set_string(dev, "name", name);
> > > qdev_prop_set_uint64(dev, "size", size);
> > > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > > 
> > > might become something like
> > > 
> > > { 'object': 'unimplemented-device',
> > >   'config': {
> > >  'name': 'str',
> > >  'size': 'size'
> > >   },
> > > }
> > > 
> > > DeviceState *dev = qapi_Unimplemented_new(&(
> > >  (UnimplementedDeviceOptions) {
> > >  .name = name,
> > >  .size = size
> > >  }, &error_fatal);
> > > object_unref(dev);
> > > 
> > > (i.e. all typesafe!) and the underlying code would be something like
> > [...]
> > > 
> > 
> > Looks nice as end goal.  Then, these are a few questions I would
> > have about the transition plan:
> > 
> > Would it require changing both device implementation and device
> > users in lockstep?  Should we have a compatibility layer to allow
> > existing qdev_new()+qdev_prop_set_*() code to keep working after
> > the devices are converted to the new system?  If not, why not?
> 
> Technically, it doesn't strictly require a lockstep update. You can have
> two code paths leading to a fully initialised device, one being the
> traditional way of setting properties and finally calling dc->realize,
> the other being a new method that takes the configuration in its
> parameters and also sets dev->realized = true at the end.
> 
> If at all possible, I would however prefer a lockstep update because
> having two paths is a weird intermediate state and the code paths could
> easily diverge. Keeping the old way around for a device also means that
> property setters are still doing two different jobs (initial
> configuration and updates at runtime).

I'd like to understand better how that intermediate state would
look like and why there's risk of separate code paths diverging.

Could we have an intermediate state that doesn't require any
duplication and thus have no separate code paths that could
diverge?


> 
> > If we add a compatibility layer, is the end goal to convert all
> > existing qdev_new() users to the new system?  If yes, why?  If
> > not, why not?
> 
> My personal goal is covering -object and -device, i.e. the external
> interfaces. Converting purely internally created devices is not as
> interesting (especially as long as we focus only on object creation),
> but might be desirable for consistency.

I wonder how much consistency we will lose and how much confusion
we'll cause if we end up with two completely separate methods for
creating devices.

> 
> > What about subclasses?  Would base classes need to be converted
> > in lockstep with all subclasses?  How would the transition
> > process of (e.g.) PCI devices look like?
> 
> I don't think so.
> 
> If you want to conve

Re: [PATCH v2 4/4] block/export: avoid g_return_val_if() input validation

[Philippe Mathieu-Daudé]

On 12/2/20 4:26 PM, Stefan Hajnoczi wrote:
> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
> 
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
> 
> Suggested-by: Markus Armbruster 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  block/export/vhost-user-blk-server.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 1/4] contrib/vhost-user-blk: avoid g_return_val_if() input validation

[Philippe Mathieu-Daudé]

On 12/2/20 4:26 PM, Stefan Hajnoczi wrote:
> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
> 
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
> 
> Suggested-by: Markus Armbruster 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  contrib/vhost-user-blk/vhost-user-blk.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé 

Re: [PATCH v2 1/4] contrib/vhost-user-blk: avoid g_return_val_if() input validation

[Raphael Norwitz]

On Wed, Dec 2, 2020 at 10:27 AM Stefan Hajnoczi  wrote:
>
> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster 
> Signed-off-by: Stefan Hajnoczi 

Reviewed-by: Raphael Norwitz 

> ---
>  contrib/vhost-user-blk/vhost-user-blk.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c 
> b/contrib/vhost-user-blk/vhost-user-blk.c
> index dc981bf945..60e3c9ed37 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -404,7 +404,11 @@ vub_get_config(VuDev *vu_dev, uint8_t *config, uint32_t 
> len)
>  VugDev *gdev;
>  VubDev *vdev_blk;
>
> -g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
> +if (len > sizeof(struct virtio_blk_config)) {
> +fprintf(stderr, "Invalid get_config len %u, expected <= %zu\n",
> +len, sizeof(struct virtio_blk_config));
> +return -1;
> +}
>
>  gdev = container_of(vu_dev, VugDev, parent);
>  vdev_blk = container_of(gdev, VubDev, parent);
> --
> 2.28.0
>

Re: [PATCH v2 4/4] block/export: avoid g_return_val_if() input validation

[Marc-André Lureau]

On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi  wrote:

> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster 
> Signed-off-by: Stefan Hajnoczi 
>

Reviewed-by: Marc-André Lureau 

---
>  block/export/vhost-user-blk-server.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/block/export/vhost-user-blk-server.c
> b/block/export/vhost-user-blk-server.c
> index 62672d1cb9..bccbc98d57 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
> @@ -267,7 +267,11 @@ vu_blk_get_config(VuDev *vu_dev, uint8_t *config,
> uint32_t len)
>  VuServer *server = container_of(vu_dev, VuServer, vu_dev);
>  VuBlkExport *vexp = container_of(server, VuBlkExport, vu_server);
>
> -g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
> +if (len > sizeof(struct virtio_blk_config)) {
> +error_report("Invalid get_config len %u, expected <= %zu",
> + len, sizeof(struct virtio_blk_config));
> +return -1;
> +}
>
>  memcpy(config, &vexp->blkcfg, len);
>  return 0;
> --
> 2.28.0
>
>

-- 
Marc-André Lureau

Re: [PATCH v2 3/4] contrib/vhost-user-input: avoid g_return_val_if() input validation

[Marc-André Lureau]

On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi  wrote:

> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  contrib/vhost-user-input/main.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-input/main.c
> b/contrib/vhost-user-input/main.c
> index 6020c6f33a..1d79c61200 100644
> --- a/contrib/vhost-user-input/main.c
> +++ b/contrib/vhost-user-input/main.c
> @@ -212,7 +212,11 @@ static int vi_get_config(VuDev *dev, uint8_t *config,
> uint32_t len)
>  {
>  VuInput *vi = container_of(dev, VuInput, dev.parent);
>
> -g_return_val_if_fail(len <= sizeof(*vi->sel_config), -1);
> +if (len > sizeof(*vi->sel_config)) {
> +g_critical("%s: len %u is larger than %zu",
> +   __func__, len, sizeof(*vi->sel_config));
> +return -1;
>

g_critical() already has __FILE__ __LINE__ and G_STRFUNC.

otherwise looks good:
Reviewed-by: Marc-André Lureau 



> +}
>
>  if (vi->sel_config) {
>  memcpy(config, vi->sel_config, len);
> --
> 2.28.0
>
>

-- 
Marc-André Lureau

Re: [PATCH v2 2/4] contrib/vhost-user-gpu: avoid g_return_val_if() input validation

[Marc-André Lureau]

On Wed, Dec 2, 2020 at 7:27 PM Stefan Hajnoczi  wrote:

> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c
> b/contrib/vhost-user-gpu/vhost-user-gpu.c
> index a019d0a9ac..534bad24d1 100644
> --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> @@ -1044,7 +1044,11 @@ vg_get_config(VuDev *dev, uint8_t *config, uint32_t
> len)
>  {
>  VuGpu *g = container_of(dev, VuGpu, dev.parent);
>
> -g_return_val_if_fail(len <= sizeof(struct virtio_gpu_config), -1);
> +if (len > sizeof(struct virtio_gpu_config)) {
> +g_critical("%s: len %u is larger than %zu",
> +   __func__, len, sizeof(struct virtio_gpu_config));
>

g_critical() already has __FILE__ __LINE__ and G_STRFUNC.

otherwise looks good:
Reviewed-by: Marc-André Lureau 

+return -1;
> +}
>
>  if (opt_virgl) {
>  g->virtio_config.num_capsets = vg_virgl_get_num_capsets();
> --
> 2.28.0
>
>

-- 
Marc-André Lureau

Re: [PATCH v2 1/4] contrib/vhost-user-blk: avoid g_return_val_if() input validation

[Marc-André Lureau]

On Wed, Dec 2, 2020 at 7:26 PM Stefan Hajnoczi  wrote:

> Do not validate input with g_return_val_if(). This API is intended for
> checking programming errors and is compiled out with -DG_DISABLE_CHECKS.
>
> Use an explicit if statement for input validation so it cannot
> accidentally be compiled out.
>
> Suggested-by: Markus Armbruster 
> Signed-off-by: Stefan Hajnoczi 
>

Reviewed-by: Marc-André Lureau 

---
>  contrib/vhost-user-blk/vhost-user-blk.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/vhost-user-blk/vhost-user-blk.c
> b/contrib/vhost-user-blk/vhost-user-blk.c
> index dc981bf945..60e3c9ed37 100644
> --- a/contrib/vhost-user-blk/vhost-user-blk.c
> +++ b/contrib/vhost-user-blk/vhost-user-blk.c
> @@ -404,7 +404,11 @@ vub_get_config(VuDev *vu_dev, uint8_t *config,
> uint32_t len)
>  VugDev *gdev;
>  VubDev *vdev_blk;
>
> -g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
> +if (len > sizeof(struct virtio_blk_config)) {
> +fprintf(stderr, "Invalid get_config len %u, expected <= %zu\n",
> +len, sizeof(struct virtio_blk_config));
> +return -1;
> +}
>
>  gdev = container_of(vu_dev, VugDev, parent);
>  vdev_blk = container_of(gdev, VubDev, parent);
> --
> 2.28.0
>
>

-- 
Marc-André Lureau

Re: [raw] Guest stuck during live live-migration

[Kevin Wolf]

Am 02.12.2020 um 16:09 hat Quentin Grolleau geschrieben:
> Do you think that, applying this patch ( replacing by "#if 0" there :
> https://github.com/qemu/qemu/blob/master/block/file-posix.c#L2601 ),
> could affect for any reason the customer data ?
> 
> As we are on full NVME and 10G networks it should fix our vm which
> completely freeze.

It's not a real fix for the general case, of course, but yes, you can do
that safely. It means that QEMU will assume that all of your raw images
are fully allocated.

Kevin

> De : Qemu-devel  
> de la part de Quentin Grolleau 
> Envoyé : mardi 24 novembre 2020 13:58:53
> À : Kevin Wolf
> Cc : qemu-de...@nongnu.org; qemu-block@nongnu.org
> Objet : RE: [raw] Guest stuck during live live-migration
> 
> Thanks Kevin,
> 
> > > Hello,
> > >
> > > In our company, we are hosting a large number of Vm, hosted behind 
> > > Openstack (so libvirt/qemu).
> > > A large majority of our Vms are runnign with local data only, stored on 
> > > NVME, and most of them are RAW disks.
> > >
> > > With Qemu 4.0 (can be even with older version) we see strange  
> > > live-migration comportement:
> 
> > First of all, 4.0 is relatively old. Generally it is worth retrying with
> > the most recent code (git master or 5.2.0-rc2) before having a closer
> > look at problems, because it is frustrating to spend considerable time
> > debugging an issue and then find out it has already been fixed a year
> > ago.
> 
> > I will try to build it the most recent code
> 
> 
> I will try to build with the most recent code, but it will take me some time 
> to do it
> 
> 
> > > - some Vms live migrate at very high speed without issue (> 6 Gbps)
> > > - some Vms are running correctly, but migrating at a strange low 
> > > speed (3Gbps)
> > > - some Vms are migrating at a very low speed (1Gbps, sometime less) 
> > > and during the migration the guest is completely I/O stuck
> > >
> > > When this issue happen the VM is completly block, iostat in the Vm show 
> > > us a latency of 30 secs
> 
> > Can you get the stack backtraces of all QEMU threads while the VM is
> > blocked (e.g. with gdb or pstack)?
> 
> (gdb) thread apply all bt
> 
> Thread 20 (Thread 0x7f8a0effd700 (LWP 201248)):
> #0  pthread_cond_wait@@GLIBC_2.3.2 () at 
> ../sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185
> #1  0x56520139878b in qemu_cond_wait_impl (cond=0x5652020f27b0, 
> mutex=0x5652020f27e8, file=0x5652014e4178 
> "/root/qemu_debug_LSEEK/qemu_debug/qemu/ui/vnc-jobs.c", line=214) at 
> /root/qemu_debug_LSEEK/qemu_debug/qemu/util/qemu-thread-posix.c:161
> #2  0x5652012a264d in vnc_worker_thread_loop 
> (queue=queue@entry=0x5652020f27b0) at 
> /root/qemu_debug_LSEEK/qemu_debug/qemu/ui/vnc-jobs.c:214
> #3  0x5652012a2c18 in vnc_worker_thread (arg=arg@entry=0x5652020f27b0) at 
> /root/qemu_debug_LSEEK/qemu_debug/qemu/ui/vnc-jobs.c:324
> #4  0x565201398116 in qemu_thread_start (args=) at 
> /root/qemu_debug_LSEEK/qemu_debug/qemu/util/qemu-thread-posix.c:502
> #5  0x7f8a5e24a6ba in start_thread (arg=0x7f8a0effd700) at 
> pthread_create.c:333
> #6  0x7f8a5df8041d in clone () at 
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
> 
> Thread 19 (Thread 0x7f8a0700 (LWP 201222)):
> #0  __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> #1  0x7f8a5e24cdbd in __GI___pthread_mutex_lock 
> (mutex=mutex@entry=0x565201adb680 ) at 
> ../nptl/pthread_mutex_lock.c:80
> #2  0x565201398263 in qemu_mutex_lock_impl (mutex=0x565201adb680 
> , file=0x5652013d7c68 
> "/root/qemu_debug_LSEEK/qemu_debug/qemu/accel/kvm/kvm-all.c", line=2089) at 
> /root/qemu_debug_LSEEK/qemu_debug/qemu/util/qemu-thread-posix.c:66
> #3  0x565200f7d00e in qemu_mutex_lock_iothread_impl 
> (file=file@entry=0x5652013d7c68 
> "/root/qemu_debug_LSEEK/qemu_debug/qemu/accel/kvm/kvm-all.c", 
> line=line@entry=2089) at /root/qemu_debug_LSEEK/qemu_debug/qemu/cpus.c:1850
> #4  0x565200fa7ca8 in kvm_cpu_exec (cpu=cpu@entry=0x565202354480) at 
> /root/qemu_debug_LSEEK/qemu_debug/qemu/accel/kvm/kvm-all.c:2089
> #5  0x565200f7d1ce in qemu_kvm_cpu_thread_fn 
> (arg=arg@entry=0x565202354480) at 
> /root/qemu_debug_LSEEK/qemu_debug/qemu/cpus.c:1281
> #6  0x565201398116 in qemu_thread_start (args=) at 
> /root/qemu_debug_LSEEK/qemu_debug/qemu/util/qemu-thread-posix.c:502
> #7  0x7f8a5e24a6ba in start_thread (arg=0x7f8a0700) at 
> pthread_create.c:333
> #8  0x7f8a5df8041d in clone () at 
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
> 
> Thread 18 (Thread 0x7f8a2cff9700 (LWP 201221)):
> #0  __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
> #1  0x7f8a5e24cdbd in __GI___pthread_mutex_lock 
> (mutex=mutex@entry=0x565201adb680 ) at 
> ../nptl/pthread_mutex_lock.c:80
> #2  0x565201398263 in qemu_mutex_lock_impl (mutex=0x565201adb680 
> , file=0x5652013d7c68 
> "/root/qemu_debug_LSEEK/qemu_debug/qemu/accel/kvm/kvm-all.c", line=2089) at 
> /root/qemu_debug_LSEEK/qemu_debu

[PATCH v2 3/4] contrib/vhost-user-input: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

Do not validate input with g_return_val_if(). This API is intended for
checking programming errors and is compiled out with -DG_DISABLE_CHECKS.

Use an explicit if statement for input validation so it cannot
accidentally be compiled out.

Suggested-by: Markus Armbruster 
Signed-off-by: Stefan Hajnoczi 
---
 contrib/vhost-user-input/main.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-input/main.c b/contrib/vhost-user-input/main.c
index 6020c6f33a..1d79c61200 100644
--- a/contrib/vhost-user-input/main.c
+++ b/contrib/vhost-user-input/main.c
@@ -212,7 +212,11 @@ static int vi_get_config(VuDev *dev, uint8_t *config, 
uint32_t len)
 {
 VuInput *vi = container_of(dev, VuInput, dev.parent);
 
-g_return_val_if_fail(len <= sizeof(*vi->sel_config), -1);
+if (len > sizeof(*vi->sel_config)) {
+g_critical("%s: len %u is larger than %zu",
+   __func__, len, sizeof(*vi->sel_config));
+return -1;
+}
 
 if (vi->sel_config) {
 memcpy(config, vi->sel_config, len);
-- 
2.28.0

[PATCH v2 2/4] contrib/vhost-user-gpu: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

Do not validate input with g_return_val_if(). This API is intended for
checking programming errors and is compiled out with -DG_DISABLE_CHECKS.

Use an explicit if statement for input validation so it cannot
accidentally be compiled out.

Suggested-by: Markus Armbruster 
Signed-off-by: Stefan Hajnoczi 
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c 
b/contrib/vhost-user-gpu/vhost-user-gpu.c
index a019d0a9ac..534bad24d1 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -1044,7 +1044,11 @@ vg_get_config(VuDev *dev, uint8_t *config, uint32_t len)
 {
 VuGpu *g = container_of(dev, VuGpu, dev.parent);
 
-g_return_val_if_fail(len <= sizeof(struct virtio_gpu_config), -1);
+if (len > sizeof(struct virtio_gpu_config)) {
+g_critical("%s: len %u is larger than %zu",
+   __func__, len, sizeof(struct virtio_gpu_config));
+return -1;
+}
 
 if (opt_virgl) {
 g->virtio_config.num_capsets = vg_virgl_get_num_capsets();
-- 
2.28.0

[PATCH v2 4/4] block/export: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

Do not validate input with g_return_val_if(). This API is intended for
checking programming errors and is compiled out with -DG_DISABLE_CHECKS.

Use an explicit if statement for input validation so it cannot
accidentally be compiled out.

Suggested-by: Markus Armbruster 
Signed-off-by: Stefan Hajnoczi 
---
 block/export/vhost-user-blk-server.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/block/export/vhost-user-blk-server.c 
b/block/export/vhost-user-blk-server.c
index 62672d1cb9..bccbc98d57 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -267,7 +267,11 @@ vu_blk_get_config(VuDev *vu_dev, uint8_t *config, uint32_t 
len)
 VuServer *server = container_of(vu_dev, VuServer, vu_dev);
 VuBlkExport *vexp = container_of(server, VuBlkExport, vu_server);
 
-g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
+if (len > sizeof(struct virtio_blk_config)) {
+error_report("Invalid get_config len %u, expected <= %zu",
+ len, sizeof(struct virtio_blk_config));
+return -1;
+}
 
 memcpy(config, &vexp->blkcfg, len);
 return 0;
-- 
2.28.0

[PATCH v2 1/4] contrib/vhost-user-blk: avoid g_return_val_if() input validation

[Stefan Hajnoczi]

Do not validate input with g_return_val_if(). This API is intended for
checking programming errors and is compiled out with -DG_DISABLE_CHECKS.

Use an explicit if statement for input validation so it cannot
accidentally be compiled out.

Suggested-by: Markus Armbruster 
Signed-off-by: Stefan Hajnoczi 
---
 contrib/vhost-user-blk/vhost-user-blk.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contrib/vhost-user-blk/vhost-user-blk.c 
b/contrib/vhost-user-blk/vhost-user-blk.c
index dc981bf945..60e3c9ed37 100644
--- a/contrib/vhost-user-blk/vhost-user-blk.c
+++ b/contrib/vhost-user-blk/vhost-user-blk.c
@@ -404,7 +404,11 @@ vub_get_config(VuDev *vu_dev, uint8_t *config, uint32_t 
len)
 VugDev *gdev;
 VubDev *vdev_blk;
 
-g_return_val_if_fail(len <= sizeof(struct virtio_blk_config), -1);
+if (len > sizeof(struct virtio_blk_config)) {
+fprintf(stderr, "Invalid get_config len %u, expected <= %zu\n",
+len, sizeof(struct virtio_blk_config));
+return -1;
+}
 
 gdev = container_of(vu_dev, VugDev, parent);
 vdev_blk = container_of(gdev, VubDev, parent);
-- 
2.28.0

[PATCH v2 0/4] vhost-user: avoid g_return_val_if() in get/set_config()

[Stefan Hajnoczi]

v2:
 * Print errors [Marc-André]

Markus Armbruster pointed out that g_return_val_if() is meant for programming
errors. It must not be used for input validation since it can be compiled out.
Use explicit if statements instead.

This patch series converts vhost-user device backends that use
g_return_val_if() in get/set_config().

Stefan Hajnoczi (4):
  contrib/vhost-user-blk: avoid g_return_val_if() input validation
  contrib/vhost-user-gpu: avoid g_return_val_if() input validation
  contrib/vhost-user-input: avoid g_return_val_if() input validation
  block/export: avoid g_return_val_if() input validation

 block/export/vhost-user-blk-server.c| 6 +-
 contrib/vhost-user-blk/vhost-user-blk.c | 6 +-
 contrib/vhost-user-gpu/vhost-user-gpu.c | 6 +-
 contrib/vhost-user-input/main.c | 6 +-
 4 files changed, 20 insertions(+), 4 deletions(-)

-- 
2.28.0

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 02.12.2020 um 14:54 hat Eduardo Habkost geschrieben:
> On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:
> > On 02/12/20 13:51, Eduardo Habkost wrote:
> > > > > I'm liking the direction this is taking.  However, I would still
> > > > > like to have a clearer and feasible plan that would work for
> > > > > -device, -machine, and -cpu.
> > > > 
> > > > -cpu is not a problem since it's generally created with a static
> > > > configuration (now done with global properties, in the future it could 
> > > > be a
> > > > struct).
> > > 
> > > It is a problem if it requires manually converting existing code
> > > defining CPU properties and we don't have a transition plan.
> > 
> > We do not have to convert everything _if_ for some objects there are good
> > reasons to do programmatically-generated properties.  CPUs might be one of
> > those cases (or if we decide to convert them, they might endure some more
> > code duplication than other devices because they have so many properties).
> 
> OK, we just need to agree on what the transition will look like
> when we do it.  I think we should put as much care into
> transition/glue infrastructure as we put into the new
> infrastructure.
> 
> 
> > 
> > > Would a -device conversion also involve non-user-creatable
> > > devices, or would we keep existing internal usage of QOM
> > > properties?
> > > 
> > > Even if it's just for user-creatable devices, getting rid of QOM
> > > property usage in devices sounds like a very ambitious goal.  I'd
> > > like us to have a good transition plan, in addition to declaring
> > > what's our ideal end goal.
> > 
> > For user-creatable objects Kevin is doing work in lockstep on all classes;
> > but once we have the infrastructure for QAPI object configuration schemas we
> > can proceed in the other direction and operate on one device at a time.
> > 
> > With some handwaving, something like (see create_unimplemented_device)
> > 
> > DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> > 
> > qdev_prop_set_string(dev, "name", name);
> > qdev_prop_set_uint64(dev, "size", size);
> > sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > 
> > might become something like
> > 
> > { 'object': 'unimplemented-device',
> >   'config': {
> >  'name': 'str',
> >  'size': 'size'
> >   },
> > }
> > 
> > DeviceState *dev = qapi_Unimplemented_new(&(
> >  (UnimplementedDeviceOptions) {
> >  .name = name,
> >  .size = size
> >  }, &error_fatal);
> > object_unref(dev);
> > 
> > (i.e. all typesafe!) and the underlying code would be something like
> [...]
> > 
> 
> Looks nice as end goal.  Then, these are a few questions I would
> have about the transition plan:
> 
> Would it require changing both device implementation and device
> users in lockstep?  Should we have a compatibility layer to allow
> existing qdev_new()+qdev_prop_set_*() code to keep working after
> the devices are converted to the new system?  If not, why not?

Technically, it doesn't strictly require a lockstep update. You can have
two code paths leading to a fully initialised device, one being the
traditional way of setting properties and finally calling dc->realize,
the other being a new method that takes the configuration in its
parameters and also sets dev->realized = true at the end.

If at all possible, I would however prefer a lockstep update because
having two paths is a weird intermediate state and the code paths could
easily diverge. Keeping the old way around for a device also means that
property setters are still doing two different jobs (initial
configuration and updates at runtime).

> If we add a compatibility layer, is the end goal to convert all
> existing qdev_new() users to the new system?  If yes, why?  If
> not, why not?

My personal goal is covering -object and -device, i.e. the external
interfaces. Converting purely internally created devices is not as
interesting (especially as long as we focus only on object creation),
but might be desirable for consistency.

> What about subclasses?  Would base classes need to be converted
> in lockstep with all subclasses?  How would the transition
> process of (e.g.) PCI devices look like?

I don't think so.

If you want to convert base classes first, you may need to take the
path shown above where both initialisation paths coexist while the
children are converted because instantiation of a child class involves
setting properties of the base class. So you can only restrict these
properties to runtime-only after all children have been converted.

The other way around might be easier: You will need to describe the
properties of base classes in the QAPI schema from the beginning, but
that doesn't mean that their initialisation code has to change just yet.
The child classes will need to forward the part of their configuration
that belongs to the base class. The downside is that this code will have
to be change

RE: [raw] Guest stuck during live live-migration

[Quentin Grolleau]

Do you think that, applying this patch ( replacing by "#if 0" there : 
https://github.com/qemu/qemu/blob/master/block/file-posix.c#L2601 ), could 
affect for any reason the customer data ?

As we are on full NVME and 10G networks it should fix our vm which completely 
freeze.

Quentin



De : Qemu-devel  
de la part de Quentin Grolleau 
Envoyé : mardi 24 novembre 2020 13:58:53
À : Kevin Wolf
Cc : qemu-de...@nongnu.org; qemu-block@nongnu.org
Objet : RE: [raw] Guest stuck during live live-migration

Thanks Kevin,

> > Hello,
> >
> > In our company, we are hosting a large number of Vm, hosted behind 
> > Openstack (so libvirt/qemu).
> > A large majority of our Vms are runnign with local data only, stored on 
> > NVME, and most of them are RAW disks.
> >
> > With Qemu 4.0 (can be even with older version) we see strange  
> > live-migration comportement:

> First of all, 4.0 is relatively old. Generally it is worth retrying with
> the most recent code (git master or 5.2.0-rc2) before having a closer
> look at problems, because it is frustrating to spend considerable time
> debugging an issue and then find out it has already been fixed a year
> ago.

> I will try to build it the most recent code


I will try to build with the most recent code, but it will take me some time to 
do it


> > - some Vms live migrate at very high speed without issue (> 6 Gbps)
> > - some Vms are running correctly, but migrating at a strange low speed 
> > (3Gbps)
> > - some Vms are migrating at a very low speed (1Gbps, sometime less) and 
> > during the migration the guest is completely I/O stuck
> >
> > When this issue happen the VM is completly block, iostat in the Vm show us 
> > a latency of 30 secs

> Can you get the stack backtraces of all QEMU threads while the VM is
> blocked (e.g. with gdb or pstack)?

(gdb) thread apply all bt

Thread 20 (Thread 0x7f8a0effd700 (LWP 201248)):
#0  pthread_cond_wait@@GLIBC_2.3.2 () at 
../sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:185
#1  0x56520139878b in qemu_cond_wait_impl (cond=0x5652020f27b0, 
mutex=0x5652020f27e8, file=0x5652014e4178 
"/root/qemu_debug_LSEEK/qemu_debug/qemu/ui/vnc-jobs.c", line=214) at 
/root/qemu_debug_LSEEK/qemu_debug/qemu/util/qemu-thread-posix.c:161
#2  0x5652012a264d in vnc_worker_thread_loop 
(queue=queue@entry=0x5652020f27b0) at 
/root/qemu_debug_LSEEK/qemu_debug/qemu/ui/vnc-jobs.c:214
#3  0x5652012a2c18 in vnc_worker_thread (arg=arg@entry=0x5652020f27b0) at 
/root/qemu_debug_LSEEK/qemu_debug/qemu/ui/vnc-jobs.c:324
#4  0x565201398116 in qemu_thread_start (args=) at 
/root/qemu_debug_LSEEK/qemu_debug/qemu/util/qemu-thread-posix.c:502
#5  0x7f8a5e24a6ba in start_thread (arg=0x7f8a0effd700) at 
pthread_create.c:333
#6  0x7f8a5df8041d in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Thread 19 (Thread 0x7f8a0700 (LWP 201222)):
#0  __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x7f8a5e24cdbd in __GI___pthread_mutex_lock 
(mutex=mutex@entry=0x565201adb680 ) at 
../nptl/pthread_mutex_lock.c:80
#2  0x565201398263 in qemu_mutex_lock_impl (mutex=0x565201adb680 
, file=0x5652013d7c68 
"/root/qemu_debug_LSEEK/qemu_debug/qemu/accel/kvm/kvm-all.c", line=2089) at 
/root/qemu_debug_LSEEK/qemu_debug/qemu/util/qemu-thread-posix.c:66
#3  0x565200f7d00e in qemu_mutex_lock_iothread_impl 
(file=file@entry=0x5652013d7c68 
"/root/qemu_debug_LSEEK/qemu_debug/qemu/accel/kvm/kvm-all.c", 
line=line@entry=2089) at /root/qemu_debug_LSEEK/qemu_debug/qemu/cpus.c:1850
#4  0x565200fa7ca8 in kvm_cpu_exec (cpu=cpu@entry=0x565202354480) at 
/root/qemu_debug_LSEEK/qemu_debug/qemu/accel/kvm/kvm-all.c:2089
#5  0x565200f7d1ce in qemu_kvm_cpu_thread_fn (arg=arg@entry=0x565202354480) 
at /root/qemu_debug_LSEEK/qemu_debug/qemu/cpus.c:1281
#6  0x565201398116 in qemu_thread_start (args=) at 
/root/qemu_debug_LSEEK/qemu_debug/qemu/util/qemu-thread-posix.c:502
#7  0x7f8a5e24a6ba in start_thread (arg=0x7f8a0700) at 
pthread_create.c:333
#8  0x7f8a5df8041d in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Thread 18 (Thread 0x7f8a2cff9700 (LWP 201221)):
#0  __lll_lock_wait () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1  0x7f8a5e24cdbd in __GI___pthread_mutex_lock 
(mutex=mutex@entry=0x565201adb680 ) at 
../nptl/pthread_mutex_lock.c:80
#2  0x565201398263 in qemu_mutex_lock_impl (mutex=0x565201adb680 
, file=0x5652013d7c68 
"/root/qemu_debug_LSEEK/qemu_debug/qemu/accel/kvm/kvm-all.c", line=2089) at 
/root/qemu_debug_LSEEK/qemu_debug/qemu/util/qemu-thread-posix.c:66
#3  0x565200f7d00e in qemu_mutex_lock_iothread_impl 
(file=file@entry=0x5652013d7c68 
"/root/qemu_debug_LSEEK/qemu_debug/qemu/accel/kvm/kvm-all.c", 
line=line@entry=2089) at /root/qemu_debug_LSEEK/qemu_debug/qemu/cpus.c:1850
#4  0x565200fa7ca8 in kvm_cpu_exec (cpu=cpu@entry=0x565202331320) at 
/root/qemu_debug_LSEEK/qemu_debug/qemu/accel/kvm/kvm-all.c:2

Re: [PATCH 0/4] vhost-user: avoid g_return_val_if() in get/set_config()

[Stefan Hajnoczi]

On Wed, Nov 18, 2020 at 07:21:15PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Nov 18, 2020 at 1:17 PM Stefan Hajnoczi  wrote:
> 
> > Markus Armbruster pointed out that g_return_val_if() is meant for
> > programming
> > errors. It must not be used for input validation since it can be compiled
> > out.
> > Use explicit if statements instead.
> >
> > This patch series converts vhost-user device backends that use
> > g_return_val_if() in get/set_config().
> >
> > Stefan Hajnoczi (4):
> >   contrib/vhost-user-blk: avoid g_return_val_if() input validation
> >   contrib/vhost-user-gpu: avoid g_return_val_if() input validation
> >   contrib/vhost-user-input: avoid g_return_val_if() input validation
> >   block/export: avoid g_return_val_if() input validation
> >
> >
> The condition is the same for all the patches, checking the message config
> payload is large enough. Afaict, the value is set by the client, so it
> could be a runtime error, and thus explicit checking is required.
> 
> Nevertheless, one nice thing about g_return* macros, is that it provides an
> error message when the condition fails, which helps. Could you replace it?
> 
> (fwiw, I think g_return* macros are so convenient, I would simply make
> G_DISABLE_CHECKS forbidden and call it a day)

I'll add an error message in v2.

Stefan


signature.asc
Description: PGP signature

[PATCH] hw/block: m25p80: Implement AAI-WP command support for SST flashes

[Bin Meng]

From: Xuzhou Cheng 

Auto Address Increment (AAI) Word-Program is a special command of
SST flashes. AAI-WP allows multiple bytes of data to be programmed
without re-issuing the next sequential address location.

Signed-off-by: Xuzhou Cheng 
Signed-off-by: Bin Meng 
---

 hw/block/m25p80.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 9b36762df9..f225d9c96d 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -359,6 +359,7 @@ typedef enum {
 QPP_4 = 0x34,
 RDID_90 = 0x90,
 RDID_AB = 0xab,
+AAI_WP = 0xad,
 
 ERASE_4K = 0x20,
 ERASE4_4K = 0x21,
@@ -449,6 +450,7 @@ struct Flash {
 bool four_bytes_address_mode;
 bool reset_enable;
 bool quad_enable;
+bool aai_enable;
 uint8_t ear;
 
 int64_t dirty_page;
@@ -661,6 +663,7 @@ static void complete_collecting_data(Flash *s)
 case PP:
 case PP4:
 case PP4_4:
+case AAI_WP:
 s->state = STATE_PAGE_PROGRAM;
 break;
 case READ:
@@ -1010,6 +1013,9 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 
 case WRDI:
 s->write_enable = false;
+if (get_man(s) == MAN_SST) {
+s->aai_enable = false;
+}
 break;
 case WREN:
 s->write_enable = true;
@@ -1162,6 +1168,17 @@ static void decode_new_cmd(Flash *s, uint32_t value)
 case RSTQIO:
 s->quad_enable = false;
 break;
+case AAI_WP:
+if (get_man(s) == MAN_SST && s->write_enable) {
+if (s->aai_enable) {
+s->state = STATE_PAGE_PROGRAM;
+} else {
+s->aai_enable = true;
+s->needed_bytes = get_addr_length(s);
+s->state = STATE_COLLECTING_DATA;
+}
+}
+break;
 default:
 s->pos = 0;
 s->len = 1;
-- 
2.25.1

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Wed, Dec 02, 2020 at 02:26:44PM +0100, Paolo Bonzini wrote:
> On 02/12/20 13:51, Eduardo Habkost wrote:
> > > > I'm liking the direction this is taking.  However, I would still
> > > > like to have a clearer and feasible plan that would work for
> > > > -device, -machine, and -cpu.
> > > 
> > > -cpu is not a problem since it's generally created with a static
> > > configuration (now done with global properties, in the future it could be 
> > > a
> > > struct).
> > 
> > It is a problem if it requires manually converting existing code
> > defining CPU properties and we don't have a transition plan.
> 
> We do not have to convert everything _if_ for some objects there are good
> reasons to do programmatically-generated properties.  CPUs might be one of
> those cases (or if we decide to convert them, they might endure some more
> code duplication than other devices because they have so many properties).

OK, we just need to agree on what the transition will look like
when we do it.  I think we should put as much care into
transition/glue infrastructure as we put into the new
infrastructure.


> 
> > Would a -device conversion also involve non-user-creatable
> > devices, or would we keep existing internal usage of QOM
> > properties?
> > 
> > Even if it's just for user-creatable devices, getting rid of QOM
> > property usage in devices sounds like a very ambitious goal.  I'd
> > like us to have a good transition plan, in addition to declaring
> > what's our ideal end goal.
> 
> For user-creatable objects Kevin is doing work in lockstep on all classes;
> but once we have the infrastructure for QAPI object configuration schemas we
> can proceed in the other direction and operate on one device at a time.
> 
> With some handwaving, something like (see create_unimplemented_device)
> 
> DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);
> 
> qdev_prop_set_string(dev, "name", name);
> qdev_prop_set_uint64(dev, "size", size);
> sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> 
> might become something like
> 
> { 'object': 'unimplemented-device',
>   'config': {
>  'name': 'str',
>  'size': 'size'
>   },
> }
> 
> DeviceState *dev = qapi_Unimplemented_new(&(
>  (UnimplementedDeviceOptions) {
>  .name = name,
>  .size = size
>  }, &error_fatal);
> object_unref(dev);
> 
> (i.e. all typesafe!) and the underlying code would be something like
[...]
> 

Looks nice as end goal.  Then, these are a few questions I would
have about the transition plan:

Would it require changing both device implementation and device
users in lockstep?  Should we have a compatibility layer to allow
existing qdev_new()+qdev_prop_set_*() code to keep working after
the devices are converted to the new system?  If not, why not?

If we add a compatibility layer, is the end goal to convert all
existing qdev_new() users to the new system?  If yes, why?  If
not, why not?

What about subclasses?  Would base classes need to be converted
in lockstep with all subclasses?  How would the transition
process of (e.g.) PCI devices look like?

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 02/12/20 13:51, Eduardo Habkost wrote:

I'm liking the direction this is taking.  However, I would still
like to have a clearer and feasible plan that would work for
-device, -machine, and -cpu.


-cpu is not a problem since it's generally created with a static
configuration (now done with global properties, in the future it could be a
struct).


It is a problem if it requires manually converting existing code
defining CPU properties and we don't have a transition plan.


We do not have to convert everything _if_ for some objects there are 
good reasons to do programmatically-generated properties.  CPUs might be 
one of those cases (or if we decide to convert them, they might endure 
some more code duplication than other devices because they have so many 
properties).



Would a -device conversion also involve non-user-creatable
devices, or would we keep existing internal usage of QOM
properties?

Even if it's just for user-creatable devices, getting rid of QOM
property usage in devices sounds like a very ambitious goal.  I'd
like us to have a good transition plan, in addition to declaring
what's our ideal end goal.


For user-creatable objects Kevin is doing work in lockstep on all 
classes; but once we have the infrastructure for QAPI object 
configuration schemas we can proceed in the other direction and operate 
on one device at a time.


With some handwaving, something like (see create_unimplemented_device)

DeviceState *dev = qdev_new(TYPE_UNIMPLEMENTED_DEVICE);

qdev_prop_set_string(dev, "name", name);
qdev_prop_set_uint64(dev, "size", size);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);

might become something like

{ 'object': 'unimplemented-device',
  'config': {
 'name': 'str',
 'size': 'size'
  },
}

DeviceState *dev = qapi_Unimplemented_new(&(
 (UnimplementedDeviceOptions) {
 .name = name,
 .size = size
 }, &error_fatal);
object_unref(dev);

(i.e. all typesafe!) and the underlying code would be something like

void (*init_properties)(Object *obj, Visitor *v, Error **errp);
...

// QAPI generated constructor
UnimplementedDevice *qapi_Unimplemented_new(
UnimplementedDeviceOptions *opt, Error **errp)
{
Object *obj = object_new(TYPE_UNIMPLEMENTED_DEVICE);
if (!qapi_Unimplemented_init_properties(obj, opt, errp)) {
object_unref(obj);
return NULL;
}
return obj;
}

// QAPI generated Visitor->struct adapter
bool qapi_Unimplemented_init_properties(
Object *obj, Visitor *v, Error **errp)
{
UnimplementedDeviceOptions opt;
Error *local_err = NULL;
visit_type_UnimplementedDeviceOptions(v, NULL,
  &opt, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return false;
}
unimplemented_init_properties(UNIMPLEMENTED_DEVICE(obj),
  &opt, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return false;
}
return true;
}

// Hand-written (?) initializer:
void unimplemented_init_properties(Object *obj,
 UnimplementedDeviceOptions *opt, Error **errp)
{
 obj->name = name;
 obj->size = size;
 device_realize(obj, errp);
}

// class_init code:
oc->init_properties = qapi_Unimplemented_init_properties;

and all read-only-after-realize QOM properties could be made *really* 
read-only.


Paolo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Eduardo Habkost]

On Wed, Dec 02, 2020 at 10:30:11AM +0100, Paolo Bonzini wrote:
> On 01/12/20 23:08, Eduardo Habkost wrote:
> > > Properties are only a useful concept if they have a use.  If
> > > -object/object_add/object-add can do the same job without properties,
> > > properties are not needed anymore.
> > 
> > Do you mean "not needed for -object anymore"?  Properties are
> > still used by internal C code (esp. board code),
> > -device/device_add, -machine, -cpu, and debugging commands (like
> > "info qtree" and qom-list/qom-get/qom-set).
> 
> Yes.
> 
> > > Right now QOM is all about exposing properties, and having multiple
> > > interfaces to set them (by picking a different visitor).  But in practice
> > > most QOM objects have a lifetime that consists of 1) set properties 2) 
> > > flip
> > > a switch (realized/complete/open) 3) let the object live on its own.  1+2
> > > are a single monitor command or CLI option; during 3 you access the object
> > > through monitor commands, not properties.
> > 
> > I agree with this, except for the word "all" in "QOM is all
> > about".  QOM is also an extensively used internal QEMU API,
> > including internal usage of the QOM property system.
> 
> Yeah, "all about exposing properties" includes internal usage.  And you're
> right that some "phase 3" monitor commands do work at the property level
> (mostly "info qtree", but also "qom-get" because there are some cases of
> public run-time properties).

I still disagree on the "all about" part even for internal usage.
But this shouldn't really matter for this discussion, I guess.

> 
> > I'm liking the direction this is taking.  However, I would still
> > like to have a clearer and feasible plan that would work for
> > -device, -machine, and -cpu.
> 
> -cpu is not a problem since it's generally created with a static
> configuration (now done with global properties, in the future it could be a
> struct).

It is a problem if it requires manually converting existing code
defining CPU properties and we don't have a transition plan.

> 
> -machine and -device in principle could be done the same way as -object,
> just through a different registry (_not_ a huge struct; that's an acceptable
> stopgap for -object but that's it).  The static aka field properties would
> remain as read-only, with defaults moved to instance_init or realize.  But
> there would be again "triplication" with a trivial conversion:

> 
> 1) in the QAPI schema, e.g. 'num_queues': 'int16'
> 
> 2) in the struct, "int16_t num_queues;"
> 
> 3) in the realize function,
> 
> s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8;
> 
> So having a mechanism for defaults in the QAPI schema would be good. Maybe
> 'num_queues': { 'type': 'int16', 'default': '8' }?
> 

Would a -device conversion also involve non-user-creatable
devices, or would we keep existing internal usage of QOM
properties?

Even if it's just for user-creatable devices, getting rid of QOM
property usage in devices sounds like a very ambitious goal.  I'd
like us to have a good transition plan, in addition to declaring
what's our ideal end goal.


> I also need to review more the part of this code with respect to the
> application of global properties.  I wonder if there are visitor tricks that
> we can do, so that global properties keep working but correspond to QAPI
> fields instead of QOM properties.
> 
> Paolo
> 

-- 
Eduardo

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 02/12/20 11:27, Kevin Wolf wrote:

Declaring read-only QOM properties is trivial.


Trivial sounds like it's something the computer should be doing.


Possibly, but not necessarily.  There's always a cost to automatic code 
generation.  If things are _too_ trivial and easy to get right, the cost 
of automatic code generation exceeds the cost of doing things by hand.


You pretty much proved that ucc->complete is hard enough to get right, 
that it benefits from code generation.  But I am a bit more conservative 
about extending that to the rest of QOM.



I'm just worried about having yet another incomplete transition.


Would you really feel at home in a QEMU without incomplete transitions?


One can always dream. :)


But since you had already posted RFC patches a while ago to
address this, I considered it solved.


Yup, I posted the non-RFC today.


1. This series (mostly for documentation and introspection)

2. -object and HMP object_add (so that we get QAPI's validation, and to
make sure that forgetting to update the schema means that the new
options just doesn't work)

3. Create a separate 'object' entity in QAPI, generate ucc->complete
implementations that call a create function in the C source code with
type-specific parameters (like QMP command handlers)


If we agree on all of these that's already a pretty good place to be in. 
The decreasing length of the replies to the thread suggests that we are.


I wouldn't mind an example of (3) that is hand-coded and may only work 
for one object type (even a simple one such as iothread), to show what 
the generated QAPI code would look like.  It's not a blocker as far as I 
am concerned, but it would be nice.


More important, Markus and John should agree with the plan before (1) is 
committed.  That may also require the aforementioned example, but it's 
up to them.



What's still open: Should QAPI cover run-time properties, too? Should
run-time properties even exist at all in the final state? What is the
exact QAPI representation of everything? (The last one includes that I
need to have a closer look at how QAPI can best be taught about
inheritance.)


Run-time properties will exist but they will probably be cut down 
substantially, and most of them will be read-only (they already are as 
far as external code is concerned, i.e. they have a setter but qom-set 
always fails because it's called too late).


Paolo

Re: [PATCH v11 2/7] block/nbd.c: Add yank feature

[Lukas Straub]

On Wed, 2 Dec 2020 15:18:48 +0300
Vladimir Sementsov-Ogievskiy  wrote:

> 15.11.2020 14:36, Lukas Straub wrote:
> > Register a yank function which shuts down the socket and sets
> > s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an
> > error occured.
> > 
> > Signed-off-by: Lukas Straub
> > Acked-by: Stefan Hajnoczi  
> 
> Hi! Could I ask, what's the reason for qatomic_load_acquire access to 
> s->state? Is there same bug fixed? Or is it related somehow to new feature?
> 

Hi,
This is for the new feature, as the yank function runs in a separate thread.

Regards,
Lukas Straub

-- 



pgpx90ndOfANX.pgp
Description: OpenPGP digital signature

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 02/12/20 11:38, Kevin Wolf wrote:

Am 02.12.2020 um 10:30 hat Paolo Bonzini geschrieben:

On 01/12/20 23:08, Eduardo Habkost wrote:

Properties are only a useful concept if they have a use.  If
-object/object_add/object-add can do the same job without properties,
properties are not needed anymore.


Do you mean "not needed for -object anymore"?  Properties are
still used by internal C code (esp. board code),
-device/device_add, -machine, -cpu, and debugging commands (like
"info qtree" and qom-list/qom-get/qom-set).


Yes.


Are internal uses mostly just right after object creation, or do we make
a lot of use of them during runtime?


Not "a lot" but enough to be nasty.  There are a couple uses of 
"well-known" QOM properties (e.g. to access the RTC time or the balloon 
stats), plus "info qtree".


Paolo

Re: [PATCH v11 2/7] block/nbd.c: Add yank feature

[Vladimir Sementsov-Ogievskiy]

15.11.2020 14:36, Lukas Straub wrote:

Register a yank function which shuts down the socket and sets
s->state = NBD_CLIENT_QUIT. This is the same behaviour as if an
error occured.

Signed-off-by: Lukas Straub
Acked-by: Stefan Hajnoczi


Hi! Could I ask, what's the reason for qatomic_load_acquire access to s->state? 
Is there same bug fixed? Or is it related somehow to new feature?

--
Best regards,
Vladimir

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 02.12.2020 um 10:30 hat Paolo Bonzini geschrieben:
> On 01/12/20 23:08, Eduardo Habkost wrote:
> > > Properties are only a useful concept if they have a use.  If
> > > -object/object_add/object-add can do the same job without properties,
> > > properties are not needed anymore.
> > 
> > Do you mean "not needed for -object anymore"?  Properties are
> > still used by internal C code (esp. board code),
> > -device/device_add, -machine, -cpu, and debugging commands (like
> > "info qtree" and qom-list/qom-get/qom-set).
> 
> Yes.

Are internal uses mostly just right after object creation, or do we make
a lot of use of them during runtime?

> > > Right now QOM is all about exposing properties, and having multiple
> > > interfaces to set them (by picking a different visitor).  But in practice
> > > most QOM objects have a lifetime that consists of 1) set properties 2) 
> > > flip
> > > a switch (realized/complete/open) 3) let the object live on its own.  1+2
> > > are a single monitor command or CLI option; during 3 you access the object
> > > through monitor commands, not properties.
> > 
> > I agree with this, except for the word "all" in "QOM is all
> > about".  QOM is also an extensively used internal QEMU API,
> > including internal usage of the QOM property system.
> 
> Yeah, "all about exposing properties" includes internal usage.  And you're
> right that some "phase 3" monitor commands do work at the property level
> (mostly "info qtree", but also "qom-get" because there are some cases of
> public run-time properties).
> 
> > I'm liking the direction this is taking.  However, I would still
> > like to have a clearer and feasible plan that would work for
> > -device, -machine, and -cpu.
> 
> -cpu is not a problem since it's generally created with a static
> configuration (now done with global properties, in the future it could be a
> struct).
> 
> -machine and -device in principle could be done the same way as -object,
> just through a different registry (_not_ a huge struct; that's an acceptable
> stopgap for -object but that's it).  The static aka field properties would
> remain as read-only, with defaults moved to instance_init or realize.  But
> there would be again "triplication" with a trivial conversion:
> 
> 1) in the QAPI schema, e.g. 'num_queues': 'int16'
> 
> 2) in the struct, "int16_t num_queues;"

This one is optional, you can use the QAPI type even in the run-time
state. I guess this goes back to how much separation you want between
the configuration and the internal state.

> 3) in the realize function,
> 
> s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8;
> 
> So having a mechanism for defaults in the QAPI schema would be good. Maybe
> 'num_queues': { 'type': 'int16', 'default': '8' }?

Defaults have been on the QAPI wishlist for a long time, and everyone
agrees that it would be nice to have them. Maybe it's time to finally
implement them.

> I also need to review more the part of this code with respect to the
> application of global properties.  I wonder if there are visitor tricks that
> we can do, so that global properties keep working but correspond to QAPI
> fields instead of QOM properties.

Kevin

[PATCH 20/28] qemu-nbd: use keyval for -object parsing

[Paolo Bonzini]

Enable creation of object with non-scalar properties.

Signed-off-by: Paolo Bonzini 
---
 qemu-nbd.c | 42 +-
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/qemu-nbd.c b/qemu-nbd.c
index a7075c5419..b4bd21a21e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -407,23 +407,6 @@ static QemuOptsList file_opts = {
 },
 };
 
-static QemuOptsList qemu_object_opts = {
-.name = "object",
-.implied_opt_name = "qom-type",
-.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-.desc = {
-{ }
-},
-};
-
-static bool qemu_nbd_object_print_help(const char *type, QemuOpts *opts)
-{
-if (user_creatable_print_help(type, opts)) {
-exit(0);
-}
-return true;
-}
-
 
 static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, bool list,
   Error **errp)
@@ -599,7 +582,6 @@ int main(int argc, char **argv)
 qcrypto_init(&error_fatal);
 
 module_call_init(MODULE_INIT_QOM);
-qemu_add_opts(&qemu_object_opts);
 qemu_add_opts(&qemu_trace_opts);
 qemu_init_exec_dir(argv[0]);
 
@@ -752,14 +734,20 @@ int main(int argc, char **argv)
 case '?':
 error_report("Try `%s --help' for more information.", argv[0]);
 exit(EXIT_FAILURE);
-case QEMU_NBD_OPT_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(&qemu_object_opts,
-   optarg, true);
-if (!opts) {
-exit(EXIT_FAILURE);
+case QEMU_NBD_OPT_OBJECT:
+{
+QDict *args;
+bool help;
+
+args = keyval_parse(optarg, "qom-type", &help, &error_fatal);
+if (help) {
+user_creatable_print_help_from_qdict(args);
+exit(EXIT_SUCCESS);
+}
+user_creatable_add_dict(args, true, &error_fatal);
+qobject_unref(args);
+break;
 }
-}   break;
 case QEMU_NBD_OPT_TLSCREDS:
 tlscredsid = optarg;
 break;
@@ -807,10 +795,6 @@ int main(int argc, char **argv)
 export_name = "";
 }
 
-qemu_opts_foreach(&qemu_object_opts,
-  user_creatable_add_opts_foreach,
-  qemu_nbd_object_print_help, &error_fatal);
-
 if (!trace_init_backends()) {
 exit(1);
 }
-- 
2.26.2

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Kevin Wolf]

Am 01.12.2020 um 22:23 hat Paolo Bonzini geschrieben:
> On 01/12/20 20:35, Kevin Wolf wrote:
> > Am 01.12.2020 um 18:16 hat Paolo Bonzini geschrieben:
> > I don't think this is actually a new things. We already have types and
> > commands declared with things like 'if': 'defined(TARGET_S390X)'.
> > As far as I understand, QAPI generated files are already built per
> > target, so not compiling things into all binaries should be entirely
> > possible.
> 
> There is some complication due to having different discriminators per
> target.  So yes it should be possible.

We have conditional union variants in existing code, too.

> But probably best left after objects because it's so much bigger a
> task and because objects have a bit more freedom for experimentation
> (less ties to other qdev-specific concepts, e.g.  the magic "bus"
> property).

Yes, I would like to get user creatable objects to a state that we
like and only then start converting other object types. I feel user
creatable objects are a nice set of types that aren't so trivial that
major problems could go unnoticed, but still a managable enough number
that it doesn't matter much if we take one or two extra steps until we
find our preferred shape.

> > So maybe only the abstract base class that actually defines the machine
> > properties (like generic-pc-machine) should be described in QAPI, and
> > then the concrete machine types would inherit from it without being
> > described in QAPI themselves?
> 
> Yes, maybe.
> 
> > > 1) whether to generate _all_ boilerplate or only properties
> > 
> > I would like to generate as much boilerplate as possible. That is, I
> > don't want to constrain us to only properties, but at the same time, I'm
> > not sure if it's possible to get rid of all boilerplate.
> > 
> > Basically, the vision I have in mind is that QAPI would generate code
> > that is the same for most instances, and then provide an option that
> > prevents code generation for a specific part for more complicated cases,
> > so that the respective function can (and must) be provided in the C
> > source.
> 
> Ok, so that's a bit different from what I am thinking of.  I don't
> care very much about the internal boilerplate, only the external
> interface for configuration.  So I don't care about type registration,
> dynamic cast macros etc., only essentially the part that leads to
> ucc->complete.

Once QAPI knows about the class hierarchy you get the internal
boilerplate generation almost for free, so I'm not sure why we would
pass on it. But I agree it's not critical to have it.

> > > 2) whether we want to introduce a separation between configuration
> > > schema and run-time state
> > 
> > You mean the internal run-time state? How is this separation not already
> > present with getter/setter functions for each property? In many cases
> > they just directly access the run-time state, but there are other cases
> > where they actually do things.
> 
> I mean moving more towards the blockdev/chardev way of doing things,
> increasing the separation very much by having separate configuration
> structs that have (potentially) no link to the run-time state struct.

I'm not sure, I'm seeing pros and contras. Also, to be honest, I'm
quite certain that the blockdev/chardev way is accidental rather than
the result of a careful design process.

Having to copy every option from the separate configuration into the
run-time state is somewhat inconvenient. On the other hand, it ensures
that every option is touched in the initialisation function, which
probably increases chances that it's checked for validity.

The separation might have made the kind of bugs more obvious where
property setters just change the configuration without actually applying
the updated value.

I guess we might end up with a mixed model where configuration is
separated into a different struct (the ObjectOptions branches), but
still kept around for the lifetime of the object so that qom-get can
keep working.

> > > 3) in the latter case, whether properties will survive at all---iothread 
> > > and
> > > throttle-groups don't really need them even if they're writable after
> > > creation.
> > 
> > How do you define properties, i.e. at which point would they stop
> > existing and what would be a non-property alternative?
> 
> Properties are only a useful concept if they have a use.  If
> -object/object_add/object-add can do the same job without properties,
> properties are not needed anymore.

Yes, I think object creation doesn't need properties. But no, that
doesn't automatically make them useless because of property updates
later on. If you want to get fully rid of them, you need a replacement
for the latter.

But you're right that I would like to make property setters only about
runtime changes (i.e. changes after creation) rather than part of the
creation itself. When they have only one job to do, it's more likely
that they actually implement something working for it.

I guess fully getting r

[PATCH 23/28] storage-daemon: do not register the "object" group with QemuOpts

[Paolo Bonzini]

Since qsd uses keyval instead of -object, do not bother
registering qemu_object_opts.  It used to be necessary
for object-del; that requirement is not there anymore
since emulators have switched to keyval.

Signed-off-by: Paolo Bonzini 
---
 storage-daemon/qemu-storage-daemon.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/storage-daemon/qemu-storage-daemon.c 
b/storage-daemon/qemu-storage-daemon.c
index 7c914b0dc1..f42f883299 100644
--- a/storage-daemon/qemu-storage-daemon.c
+++ b/storage-daemon/qemu-storage-daemon.c
@@ -130,15 +130,6 @@ enum {
 
 extern QemuOptsList qemu_chardev_opts;
 
-static QemuOptsList qemu_object_opts = {
-.name = "object",
-.implied_opt_name = "qom-type",
-.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-.desc = {
-{ }
-},
-};
-
 static void init_qmp_commands(void)
 {
 qmp_init_marshal(&qmp_commands);
@@ -295,7 +286,6 @@ int main(int argc, char *argv[])
 
 module_call_init(MODULE_INIT_QOM);
 module_call_init(MODULE_INIT_TRACE);
-qemu_add_opts(&qemu_object_opts);
 qemu_add_opts(&qemu_trace_opts);
 qcrypto_init(&error_fatal);
 bdrv_init();
-- 
2.26.2

[PATCH 28/28] vl: switch -accel parsing to keyval

[Paolo Bonzini]

Switch from QemuOpts to keyval.  This enables compound options
for accelerators.

Signed-off-by: Paolo Bonzini 
---
 accel/accel.c  |   6 ++
 include/sysemu/accel.h |   1 +
 softmmu/vl.c   | 134 ++---
 3 files changed, 67 insertions(+), 74 deletions(-)

diff --git a/accel/accel.c b/accel/accel.c
index cb555e3b06..f7fdc2f5a8 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -46,6 +46,12 @@ AccelClass *accel_find(const char *opt_name)
 return ac;
 }
 
+bool accel_print_class_properties(const char *opt_name)
+{
+g_autofree char *class_name = g_strdup_printf(ACCEL_CLASS_NAME("%s"), 
opt_name);
+return type_print_class_properties(class_name);
+}
+
 int accel_init_machine(AccelState *accel, MachineState *ms)
 {
 AccelClass *acc = ACCEL_GET_CLASS(accel);
diff --git a/include/sysemu/accel.h b/include/sysemu/accel.h
index e08b8ab8fa..737db49d21 100644
--- a/include/sysemu/accel.h
+++ b/include/sysemu/accel.h
@@ -67,6 +67,7 @@ typedef struct AccelClass {
 OBJECT_GET_CLASS(AccelClass, (obj), TYPE_ACCEL)
 
 AccelClass *accel_find(const char *opt_name);
+bool accel_print_class_properties(const char *opt_name);
 int accel_init_machine(AccelState *accel, MachineState *ms);
 
 /* Called just before os_setup_post (ie just before drop OS privs) */
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 5ade1cf6c5..88738a9f5a 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -137,6 +137,7 @@ static const char *loadvm;
 static const char *accelerators;
 static QDict *machine_opts_dict;
 static GSList *object_opts_list = NULL;
+static GSList *accel_opts_list = NULL;
 static ram_addr_t maxram_size;
 static uint64_t ram_slots;
 static int display_remote;
@@ -227,20 +228,6 @@ static QemuOptsList qemu_option_rom_opts = {
 },
 };
 
-static QemuOptsList qemu_accel_opts = {
-.name = "accel",
-.implied_opt_name = "accel",
-.head = QTAILQ_HEAD_INITIALIZER(qemu_accel_opts.head),
-.desc = {
-/*
- * no elements => accept any
- * sanity checking will happen later
- * when setting accelerator properties
- */
-{ }
-},
-};
-
 static QemuOptsList qemu_boot_opts = {
 .name = "boot-opts",
 .implied_opt_name = "order",
@@ -1555,21 +1542,6 @@ static MachineClass *select_machine(QDict *qdict, Error 
**errp)
 return machine_class;
 }
 
-static int object_parse_property_opt(Object *obj,
- const char *name, const char *value,
- const char *skip, Error **errp)
-{
-if (g_str_equal(name, skip)) {
-return 0;
-}
-
-if (!object_property_parse(obj, name, value, errp)) {
-return -1;
-}
-
-return 0;
-}
-
 /* *Non*recursively replace underscores with dashes in QDict keys.  */
 static void keyval_dashify(QDict *qdict, Error **errp)
 {
@@ -2025,7 +1997,8 @@ static int global_init_func(void *opaque, QemuOpts *opts, 
Error **errp)
 static bool is_qemuopts_group(const char *group)
 {
 if (g_str_equal(group, "object") ||
-g_str_equal(group, "machine")) {
+g_str_equal(group, "machine") ||
+g_str_equal(group, "accel")) {
 return false;
 }
 return true;
@@ -2039,6 +2012,8 @@ static GSList **qemu_config_list(const char *group)
 {
 if (g_str_equal(group, "object")) {
 return &object_opts_list;
+} else if (g_str_equal(group, "accel")) {
+return &accel_opts_list;
 }
 return NULL;
 }
@@ -2177,22 +2152,20 @@ static int do_configure_icount(void *opaque, QemuOpts 
*opts, Error **errp)
 return 0;
 }
 
-static int accelerator_set_property(void *opaque,
-const char *name, const char *value,
-Error **errp)
-{
-return object_parse_property_opt(opaque, name, value, "accel", errp);
-}
-
-static int do_configure_accelerator(void *opaque, QemuOpts *opts, Error **errp)
+static void do_configure_accelerator(void *data, void *opaque)
 {
 bool *p_init_failed = opaque;
-const char *acc = qemu_opt_get(opts, "accel");
+QDict *qdict = data;
+const char *acc = qdict_get_try_str(qdict, "accel");
 AccelClass *ac = accel_find(acc);
 AccelState *accel;
 int ret;
 bool qtest_with_kvm;
 
+if (current_accel()) {
+return;
+}
+
 qtest_with_kvm = g_str_equal(acc, "kvm") && qtest_chrdev != NULL;
 
 if (!ac) {
@@ -2200,24 +2173,20 @@ static int do_configure_accelerator(void *opaque, 
QemuOpts *opts, Error **errp)
 if (!qtest_with_kvm) {
 error_report("invalid accelerator %s", acc);
 }
-return 0;
+return;
 }
 accel = ACCEL(object_new_with_class(OBJECT_CLASS(ac)));
 object_apply_compat_props(OBJECT(accel));
-qemu_opt_foreach(opts, accelerator_set_property,
- accel,
- &error_fatal);
+qdict_del(qdict, "accel");
+object_set_properties_from_keyval(OBJECT(acce

[PATCH 26/28] vl: switch -M parsing to keyval

[Paolo Bonzini]

Switch from QemuOpts to keyval.  This enables non-scalar
machine properties.

Signed-off-by: Paolo Bonzini 
---
 softmmu/vl.c | 296 +++
 1 file changed, 132 insertions(+), 164 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index a942ce2004..5ade1cf6c5 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -134,6 +134,8 @@ static const char *cpu_option;
 static const char *mem_path;
 static const char *incoming;
 static const char *loadvm;
+static const char *accelerators;
+static QDict *machine_opts_dict;
 static GSList *object_opts_list = NULL;
 static ram_addr_t maxram_size;
 static uint64_t ram_slots;
@@ -225,21 +227,6 @@ static QemuOptsList qemu_option_rom_opts = {
 },
 };
 
-static QemuOptsList qemu_machine_opts = {
-.name = "machine",
-.implied_opt_name = "type",
-.merge_lists = true,
-.head = QTAILQ_HEAD_INITIALIZER(qemu_machine_opts.head),
-.desc = {
-/*
- * no elements => accept any
- * sanity checking will happen later
- * when setting machine properties
- */
-{ }
-},
-};
-
 static QemuOptsList qemu_accel_opts = {
 .name = "accel",
 .implied_opt_name = "accel",
@@ -469,16 +456,6 @@ static QemuOptsList qemu_fw_cfg_opts = {
 },
 };
 
-/**
- * Get machine options
- *
- * Returns: machine options (never null).
- */
-static QemuOpts *qemu_get_machine_opts(void)
-{
-return qemu_find_opts_singleton("machine");
-}
-
 const char *qemu_get_vm_name(void)
 {
 return qemu_name;
@@ -786,33 +763,6 @@ static MachineClass *find_default_machine(GSList *machines)
 return default_machineclass;
 }
 
-static int machine_help_func(QemuOpts *opts, MachineState *machine)
-{
-ObjectProperty *prop;
-ObjectPropertyIterator iter;
-
-if (!qemu_opt_has_help_opt(opts)) {
-return 0;
-}
-
-object_property_iter_init(&iter, OBJECT(machine));
-while ((prop = object_property_iter_next(&iter))) {
-if (!prop->set) {
-continue;
-}
-
-printf("%s.%s=%s", MACHINE_GET_CLASS(machine)->name,
-   prop->name, prop->type);
-if (prop->description) {
-printf(" (%s)\n", prop->description);
-} else {
-printf("\n");
-}
-}
-
-return 1;
-}
-
 static void version(void)
 {
 printf("QEMU emulator version " QEMU_FULL_VERSION "\n"
@@ -1503,33 +1453,28 @@ static gint machine_class_cmp(gconstpointer a, 
gconstpointer b)
   object_class_get_name(OBJECT_CLASS(mc1)));
 }
 
-static MachineClass *machine_parse(const char *name, GSList *machines)
+static void machine_help_func(const QDict *qdict)
 {
-MachineClass *mc;
-GSList *el;
+GSList *machines, *el;
+const char *type = qdict_get_try_str(qdict, "type");
 
-if (is_help_option(name)) {
-printf("Supported machines are:\n");
-machines = g_slist_sort(machines, machine_class_cmp);
-for (el = machines; el; el = el->next) {
-MachineClass *mc = el->data;
-if (mc->alias) {
-printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc, 
mc->name);
-}
-printf("%-20s %s%s%s\n", mc->name, mc->desc,
-   mc->is_default ? " (default)" : "",
-   mc->deprecation_reason ? " (deprecated)" : "");
-}
-exit(0);
+if (type) {
+type_print_class_properties(type);
+return;
 }
 
-mc = find_machine(name, machines);
-if (!mc) {
-error_report("unsupported machine type");
-error_printf("Use -machine help to list supported machines\n");
-exit(1);
+printf("Supported machines are:\n");
+machines = object_class_get_list(TYPE_MACHINE, false);
+machines = g_slist_sort(machines, machine_class_cmp);
+for (el = machines; el; el = el->next) {
+MachineClass *mc = el->data;
+if (mc->alias) {
+printf("%-20s %s (alias of %s)\n", mc->alias, mc->desc, mc->name);
+}
+printf("%-20s %s%s%s\n", mc->name, mc->desc,
+   mc->is_default ? " (default)" : "",
+   mc->deprecation_reason ? " (deprecated)" : "");
 }
-return mc;
 }
 
 static const char *pid_file;
@@ -1582,32 +1527,31 @@ static const QEMUOption *lookup_opt(int argc, char 
**argv,
 return popt;
 }
 
-static MachineClass *select_machine(void)
+static MachineClass *select_machine(QDict *qdict, Error **errp)
 {
+const char *optarg = qdict_get_try_str(qdict, "type");
 GSList *machines = object_class_get_list(TYPE_MACHINE, false);
-MachineClass *machine_class = find_default_machine(machines);
-const char *optarg;
-QemuOpts *opts;
-Location loc;
-
-loc_push_none(&loc);
-
-opts = qemu_get_machine_opts();
-qemu_opts_loc_restore(opts);
+MachineClass *machine_class;
+Error *local_err = NULL;
 
-optarg = qemu_opt_get(opts, "type");
 if (optarg) {
-machine_class = ma

Re: [PATCH 00/18] qapi/qom: QAPIfy object-add

[Paolo Bonzini]

On 01/12/20 23:08, Eduardo Habkost wrote:

Properties are only a useful concept if they have a use.  If
-object/object_add/object-add can do the same job without properties,
properties are not needed anymore.


Do you mean "not needed for -object anymore"?  Properties are
still used by internal C code (esp. board code),
-device/device_add, -machine, -cpu, and debugging commands (like
"info qtree" and qom-list/qom-get/qom-set).


Yes.


Right now QOM is all about exposing properties, and having multiple
interfaces to set them (by picking a different visitor).  But in practice
most QOM objects have a lifetime that consists of 1) set properties 2) flip
a switch (realized/complete/open) 3) let the object live on its own.  1+2
are a single monitor command or CLI option; during 3 you access the object
through monitor commands, not properties.


I agree with this, except for the word "all" in "QOM is all
about".  QOM is also an extensively used internal QEMU API,
including internal usage of the QOM property system.


Yeah, "all about exposing properties" includes internal usage.  And 
you're right that some "phase 3" monitor commands do work at the 
property level (mostly "info qtree", but also "qom-get" because there 
are some cases of public run-time properties).



I'm liking the direction this is taking.  However, I would still
like to have a clearer and feasible plan that would work for
-device, -machine, and -cpu.


-cpu is not a problem since it's generally created with a static 
configuration (now done with global properties, in the future it could 
be a struct).


-machine and -device in principle could be done the same way as -object, 
just through a different registry (_not_ a huge struct; that's an 
acceptable stopgap for -object but that's it).  The static aka field 
properties would remain as read-only, with defaults moved to 
instance_init or realize.  But there would be again "triplication" with 
a trivial conversion:


1) in the QAPI schema, e.g. 'num_queues': 'int16'

2) in the struct, "int16_t num_queues;"

3) in the realize function,

s->num_queues = cfg->has_num_queues ? cfg->num_queues : 8;

So having a mechanism for defaults in the QAPI schema would be good. 
Maybe 'num_queues': { 'type': 'int16', 'default': '8' }?


I also need to review more the part of this code with respect to the 
application of global properties.  I wonder if there are visitor tricks 
that we can do, so that global properties keep working but correspond to 
QAPI fields instead of QOM properties.


Paolo

[PATCH 27/28] qemu-option: remove now-dead code

[Paolo Bonzini]

-M was the sole user of qemu_opts_set and qemu_opts_set_defaults,
remove them and the arguments that they used.

Signed-off-by: Paolo Bonzini 
---
 include/qemu/option.h  |  3 ---
 tests/test-qemu-opts.c | 35 -
 util/qemu-option.c | 51 +-
 3 files changed, 10 insertions(+), 79 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index fffb03d848..306bf07575 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -119,7 +119,6 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char 
*id,
int fail_if_exists, Error **errp);
 void qemu_opts_reset(QemuOptsList *list);
 void qemu_opts_loc_restore(QemuOpts *opts);
-bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, 
Error **errp);
 const char *qemu_opts_id(QemuOpts *opts);
 void qemu_opts_set_id(QemuOpts *opts, char *id);
 void qemu_opts_del(QemuOpts *opts);
@@ -130,8 +129,6 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const 
char *params,
   bool permit_abbrev);
 QemuOpts *qemu_opts_parse(QemuOptsList *list, const char *params,
   bool permit_abbrev, Error **errp);
-void qemu_opts_set_defaults(QemuOptsList *list, const char *params,
-int permit_abbrev);
 QemuOpts *qemu_opts_from_qdict(QemuOptsList *list, const QDict *qdict,
Error **errp);
 QDict *qemu_opts_to_qdict_filtered(QemuOpts *opts, QDict *qdict,
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 8bbb17b1c7..0239876e36 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -410,40 +410,6 @@ static void test_qemu_opts_reset(void)
 g_assert(opts == NULL);
 }
 
-static void test_qemu_opts_set(void)
-{
-QemuOptsList *list;
-QemuOpts *opts;
-const char *opt;
-
-list = qemu_find_opts("opts_list_04");
-g_assert(list != NULL);
-g_assert(QTAILQ_EMPTY(&list->head));
-g_assert_cmpstr(list->name, ==, "opts_list_04");
-
-/* should not find anything at this point */
-opts = qemu_opts_find(list, NULL);
-g_assert(opts == NULL);
-
-/* implicitly create opts and set str3 value */
-qemu_opts_set(list, "str3", "value", &error_abort);
-g_assert(!QTAILQ_EMPTY(&list->head));
-
-/* get the just created opts */
-opts = qemu_opts_find(list, NULL);
-g_assert(opts != NULL);
-
-/* check the str3 value */
-opt = qemu_opt_get(opts, "str3");
-g_assert_cmpstr(opt, ==, "value");
-
-qemu_opts_del(opts);
-
-/* should not find anything at this point */
-opts = qemu_opts_find(list, NULL);
-g_assert(opts == NULL);
-}
-
 static int opts_count_iter(void *opaque, const char *name, const char *value,
Error **errp)
 {
@@ -1030,7 +996,6 @@ int main(int argc, char *argv[])
 g_test_add_func("/qemu-opts/opt_get_size", test_qemu_opt_get_size);
 g_test_add_func("/qemu-opts/opt_unset", test_qemu_opt_unset);
 g_test_add_func("/qemu-opts/opts_reset", test_qemu_opts_reset);
-g_test_add_func("/qemu-opts/opts_set", test_qemu_opts_set);
 g_test_add_func("/qemu-opts/opts_parse/general", test_opts_parse);
 g_test_add_func("/qemu-opts/opts_parse/bool", test_opts_parse_bool);
 g_test_add_func("/qemu-opts/opts_parse/number", test_opts_parse_number);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index afba08d92e..94a6bea767 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -479,19 +479,14 @@ int qemu_opt_unset(QemuOpts *opts, const char *name)
 }
 }
 
-static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value,
-   bool prepend)
+static QemuOpt *opt_create(QemuOpts *opts, const char *name, char *value)
 {
 QemuOpt *opt = g_malloc0(sizeof(*opt));
 
 opt->name = g_strdup(name);
 opt->str = value;
 opt->opts = opts;
-if (prepend) {
-QTAILQ_INSERT_HEAD(&opts->head, opt, next);
-} else {
-QTAILQ_INSERT_TAIL(&opts->head, opt, next);
-}
+QTAILQ_INSERT_TAIL(&opts->head, opt, next);
 
 return opt;
 }
@@ -518,7 +513,7 @@ static bool opt_validate(QemuOpt *opt, Error **errp)
 bool qemu_opt_set(QemuOpts *opts, const char *name, const char *value,
   Error **errp)
 {
-QemuOpt *opt = opt_create(opts, name, g_strdup(value), false);
+QemuOpt *opt = opt_create(opts, name, g_strdup(value));
 
 if (!opt_validate(opt, errp)) {
 qemu_opt_del(opt);
@@ -662,15 +657,6 @@ void qemu_opts_loc_restore(QemuOpts *opts)
 loc_restore(&opts->loc);
 }
 
-bool qemu_opts_set(QemuOptsList *list, const char *name, const char *value, 
Error **errp)
-{
-QemuOpts *opts;
-
-assert(list->merge_lists);
-opts = qemu_opts_create(list, NULL, 0, &error_abort);
-return qemu_opt_set(opts, name, value, errp);
-}
-
 const char *qemu_opts_id(QemuOpts *opts)
 {
 return opts->id;
@@ -807,7 +793

[PATCH 14/28] qemu-config: add error propagation to qemu_config_parse

[Paolo Bonzini]

This enables some simplification of vl.c via error_fatal.

Signed-off-by: Paolo Bonzini 
---
 block/blkdebug.c   |  3 +--
 include/qemu/config-file.h |  4 ++--
 softmmu/vl.c   | 30 --
 util/qemu-config.c | 20 ++--
 4 files changed, 25 insertions(+), 32 deletions(-)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5fe6172da9..7eaa8a28bf 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -279,9 +279,8 @@ static int read_config(BDRVBlkdebugState *s, const char 
*filename,
 return -errno;
 }
 
-ret = qemu_config_parse(f, config_groups, filename);
+ret = qemu_config_parse(f, config_groups, filename, errp);
 if (ret < 0) {
-error_setg(errp, "Could not parse blkdebug config file");
 goto fail;
 }
 }
diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index 7d26fe3816..da6f4690b7 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -10,9 +10,9 @@ void qemu_add_opts(QemuOptsList *list);
 void qemu_add_drive_opts(QemuOptsList *list);
 int qemu_global_option(const char *str);
 
-int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
+int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error 
**errp);
 
-int qemu_read_config_file(const char *filename);
+int qemu_read_config_file(const char *filename, Error **errp);
 
 /* Parse QDict options as a replacement for a config file (allowing multiple
enumerated (0..(n-1)) configuration "sections") */
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 4039bf3a39..16a5cd1046 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2045,17 +2045,20 @@ static int global_init_func(void *opaque, QemuOpts 
*opts, Error **errp)
 return 0;
 }
 
-static int qemu_read_default_config_file(void)
+static void qemu_read_default_config_file(Error **errp)
 {
 int ret;
+Error *local_err = NULL;
 g_autofree char *file = get_relocated_path(CONFIG_QEMU_CONFDIR 
"/qemu.conf");
 
-ret = qemu_read_config_file(file);
-if (ret < 0 && ret != -ENOENT) {
-return ret;
+ret = qemu_read_config_file(file, &local_err);
+if (ret < 0) {
+if (ret == -ENOENT) {
+error_free(local_err);
+} else {
+error_propagate(errp, local_err);
+}
 }
-
-return 0;
 }
 
 static int qemu_set_option(const char *str)
@@ -2586,9 +2589,7 @@ void qemu_init(int argc, char **argv, char **envp)
 }
 
 if (userconfig) {
-if (qemu_read_default_config_file() < 0) {
-exit(1);
-}
+qemu_read_default_config_file(&error_fatal);
 }
 
 /* second pass of option parsing */
@@ -3284,15 +3285,8 @@ void qemu_init(int argc, char **argv, char **envp)
 qemu_plugin_opt_parse(optarg, &plugin_list);
 break;
 case QEMU_OPTION_readconfig:
-{
-int ret = qemu_read_config_file(optarg);
-if (ret < 0) {
-error_report("read config %s: %s", optarg,
- strerror(-ret));
-exit(1);
-}
-break;
-}
+qemu_read_config_file(optarg, &error_fatal);
+break;
 case QEMU_OPTION_spice:
 olist = qemu_find_opts_err("spice", NULL);
 if (!olist) {
diff --git a/util/qemu-config.c b/util/qemu-config.c
index cc5be3c779..85f20079de 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -314,7 +314,7 @@ void qemu_add_opts(QemuOptsList *list)
 }
 
 /* Returns number of config groups on success, -errno on error */
-int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
+int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error 
**errp)
 {
 char line[1024], group[64], id[64], arg[64], value[1024];
 Location loc;
@@ -339,7 +339,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const 
char *fname)
 /* group with id */
 list = find_list(lists, group, &local_err);
 if (local_err) {
-error_report_err(local_err);
+error_propagate(errp, local_err);
 goto out;
 }
 opts = qemu_opts_create(list, id, 1, NULL);
@@ -350,7 +350,7 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, const 
char *fname)
 /* group without id */
 list = find_list(lists, group, &local_err);
 if (local_err) {
-error_report_err(local_err);
+error_propagate(errp, local_err);
 goto out;
 }
 opts = qemu_opts_create(list, NULL, 0, &error_abort);
@@ -362,20 +362,19 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, 
const char *fname)
 sscanf(line, " %63s = \"\

[PATCH 24/28] qom: export more functions for use with non-UserCreatable objects

[Paolo Bonzini]

Machines and accelerators are not user-creatable but they share
similar parsing machinery.  Export functions that will be used
with -machine and -accel in softmmu/vl.c.

Signed-off-by: Paolo Bonzini 
---
 include/qom/object.h| 21 +
 qom/object_interfaces.c | 51 +
 2 files changed, 57 insertions(+), 15 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index d378f13a11..cd27facd8a 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -860,6 +860,27 @@ static void do_qemu_init_ ## type_array(void)  
 \
 }   \
 type_init(do_qemu_init_ ## type_array)
 
+/**
+ * type_print_class_properties:
+ * @type: a QOM class name
+ *
+ * Print the object's class properties to stdout or the monitor.
+ * Return whether an object was found.
+ */
+bool type_print_class_properties(const char *type);
+
+/**
+ * object_set_properties_from_keyval:
+ * @obj: a QOM object
+ * @qdict: a dictionary whose leaf values are strings
+ * @errp: pointer to error object
+ *
+ * For each key in the dictionary, parse the value string and set the
+ * corresponding property in @obj.
+ */
+void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
+  Error **errp);
+
 /**
  * object_class_dynamic_cast_assert:
  * @klass: The #ObjectClass to attempt to cast.
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 107c378c27..7c6d591731 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -39,13 +39,45 @@ bool user_creatable_can_be_deleted(UserCreatable *uc)
 }
 }
 
+static void object_set_properties_from_qdict(Object *obj, const QDict *qdict,
+ Visitor *v, Error **errp)
+{
+const QDictEntry *e;
+Error *local_err = NULL;
+
+if (!visit_start_struct(v, NULL, NULL, 0, &local_err)) {
+goto out;
+}
+for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
+if (!object_property_set(obj, e->key, v, &local_err)) {
+break;
+}
+}
+if (!local_err) {
+visit_check_struct(v, &local_err);
+}
+visit_end_struct(v, NULL);
+
+out:
+if (local_err) {
+error_propagate(errp, local_err);
+}
+}
+
+void object_set_properties_from_keyval(Object *obj, const QDict *qdict,
+   Error **errp)
+{
+Visitor *v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+object_set_properties_from_qdict(obj, qdict, v, errp);
+visit_free(v);
+}
+
 Object *user_creatable_add_type(const char *type, const char *id,
 const QDict *qdict,
 Visitor *v, Error **errp)
 {
 Object *obj;
 ObjectClass *klass;
-const QDictEntry *e;
 Error *local_err = NULL;
 
 klass = object_class_by_name(type);
@@ -67,18 +99,7 @@ Object *user_creatable_add_type(const char *type, const char 
*id,
 
 assert(qdict);
 obj = object_new(type);
-if (!visit_start_struct(v, NULL, NULL, 0, &local_err)) {
-goto out;
-}
-for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-if (!object_property_set(obj, e->key, v, &local_err)) {
-break;
-}
-}
-if (!local_err) {
-visit_check_struct(v, &local_err);
-}
-visit_end_struct(v, NULL);
+object_set_properties_from_qdict(obj, qdict, v, &local_err);
 if (local_err) {
 goto out;
 }
@@ -177,7 +198,7 @@ void user_creatable_print_types(void)
 g_slist_free(list);
 }
 
-static bool user_creatable_print_type_properites(const char *type)
+bool type_print_class_properties(const char *type)
 {
 ObjectClass *klass;
 ObjectPropertyIterator iter;
@@ -219,7 +240,7 @@ void user_creatable_print_help_from_qdict(const QDict *args)
 {
 const char *type = qdict_get_try_str(args, "qom-type");
 
-if (!type || !user_creatable_print_type_properites(type)) {
+if (!type || !type_print_class_properties(type)) {
 user_creatable_print_types();
 }
 }
-- 
2.26.2

[PATCH 22/28] qemu: use keyval for -object parsing

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 include/qom/object_interfaces.h | 65 +---
 qom/object_interfaces.c | 75 -
 softmmu/vl.c| 75 +
 3 files changed, 48 insertions(+), 167 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index ed0d7d663b..1caa4fca57 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -104,51 +104,6 @@ Object *user_creatable_add_type(const char *type, const 
char *id,
  */
 bool user_creatable_add_dict(const QDict *qdict, bool keyval, Error **errp);
 
-/**
- * user_creatable_add_opts:
- * @opts: the object definition
- * @errp: if an error occurs, a pointer to an area to store the error
- *
- * Create an instance of the user creatable object whose type
- * is defined in @opts by the 'qom-type' option, placing it
- * in the object composition tree with name provided by the
- * 'id' field. The remaining options in @opts are used to
- * initialize the object properties.
- *
- * Returns: the newly created object or NULL on error
- */
-Object *user_creatable_add_opts(QemuOpts *opts, Error **errp);
-
-
-/**
- * user_creatable_add_opts_predicate:
- * @type: the QOM type to be added
- *
- * A callback function to determine whether an object
- * of type @type should be created. Instances of this
- * callback should be passed to user_creatable_add_opts_foreach
- */
-typedef bool (*user_creatable_add_opts_predicate)(const char *type);
-
-/**
- * user_creatable_add_opts_foreach:
- * @opaque: a user_creatable_add_opts_predicate callback or NULL
- * @opts: options to create
- * @errp: unused
- *
- * An iterator callback to be used in conjunction with
- * the qemu_opts_foreach() method for creating a list of
- * objects from a set of QemuOpts
- *
- * The @opaque parameter can be passed a user_creatable_add_opts_predicate
- * callback to filter which types of object are created during iteration.
- * When it fails, report the error.
- *
- * Returns: 0 on success, -1 when an error was reported.
- */
-int user_creatable_add_opts_foreach(void *opaque,
-QemuOpts *opts, Error **errp);
-
 /**
  * user_creatable_print_types:
  *
@@ -156,30 +111,12 @@ int user_creatable_add_opts_foreach(void *opaque,
  */
 void user_creatable_print_types(void);
 
-/**
- * user_creatable_print_help:
- * @type: the QOM type to be added
- * @opts: options to create
- *
- * Prints help if requested in @type or @opts. Note that if @type is neither
- * "help"/"?" nor a valid user creatable type, no help will be printed
- * regardless of @opts.
- *
- * Returns: true if a help option was found and help was printed, false
- * otherwise.
- */
-bool user_creatable_print_help(const char *type, QemuOpts *opts);
-
 /**
  * user_creatable_print_help_from_qdict:
  * @args: options to create
  *
  * Prints help considering the other options given in @args (if "qom-type" is
- * given and valid, print properties for the type, otherwise print valid types)
- *
- * In contrast to user_creatable_print_help(), this function can't return that
- * no help was requested. It should only be called if we know that help is
- * requested and it will always print some help.
+ * given and valid, print properties for the type, otherwise print valid 
types).
  */
 void user_creatable_print_help_from_qdict(const QDict *args);
 
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 80814ae7b5..107c378c27 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -141,60 +141,6 @@ out:
 return !!obj;
 }
 
-Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
-{
-Visitor *v;
-QDict *pdict;
-Object *obj;
-const char *id = qemu_opts_id(opts);
-char *type = qemu_opt_get_del(opts, "qom-type");
-
-if (!type) {
-error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
-return NULL;
-}
-if (!id) {
-error_setg(errp, QERR_MISSING_PARAMETER, "id");
-qemu_opt_set(opts, "qom-type", type, &error_abort);
-g_free(type);
-return NULL;
-}
-
-qemu_opts_set_id(opts, NULL);
-pdict = qemu_opts_to_qdict(opts, NULL);
-
-v = opts_visitor_new(opts);
-obj = user_creatable_add_type(type, id, pdict, v, errp);
-visit_free(v);
-
-qemu_opts_set_id(opts, (char *) id);
-qemu_opt_set(opts, "qom-type", type, &error_abort);
-g_free(type);
-qobject_unref(pdict);
-return obj;
-}
-
-
-int user_creatable_add_opts_foreach(void *opaque, QemuOpts *opts, Error **errp)
-{
-bool (*type_opt_predicate)(const char *, QemuOpts *) = opaque;
-Object *obj = NULL;
-const char *type;
-
-type = qemu_opt_get(opts, "qom-type");
-if (type && type_opt_predicate &&
-!type_opt_predicate(type, opts)) {
-return 0;
-}
-
-obj = user_creatable_add_opts(opts, errp);
-if (!obj) {
-return -1;
-}
-

[PATCH 11/28] qom: use qemu_printf to print help for user-creatable objects

[Paolo Bonzini]

This is needed when we add help support for object_add.

Signed-off-by: Paolo Bonzini 
---
 qom/object_interfaces.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index ed896fe764..34edc3d1d8 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -13,6 +13,7 @@
 #include "qemu/option.h"
 #include "qapi/opts-visitor.h"
 #include "qemu/config-file.h"
+#include "qemu/qemu-print.h"
 
 bool user_creatable_complete(UserCreatable *uc, Error **errp)
 {
@@ -214,15 +215,15 @@ char *object_property_help(const char *name, const char 
*type,
 return g_string_free(str, false);
 }
 
-static void user_creatable_print_types(void)
+void user_creatable_print_types(void)
 {
 GSList *l, *list;
 
-printf("List of user creatable objects:\n");
+qemu_printf("List of user creatable objects:\n");
 list = object_class_get_list_sorted(TYPE_USER_CREATABLE, false);
 for (l = list; l != NULL; l = l->next) {
 ObjectClass *oc = OBJECT_CLASS(l->data);
-printf("  %s\n", object_class_get_name(oc));
+qemu_printf("  %s\n", object_class_get_name(oc));
 }
 g_slist_free(list);
 }
@@ -253,12 +254,12 @@ static bool user_creatable_print_type_properites(const 
char *type)
 }
 g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
 if (array->len > 0) {
-printf("%s options:\n", type);
+qemu_printf("%s options:\n", type);
 } else {
-printf("There are no options for %s.\n", type);
+qemu_printf("There are no options for %s.\n", type);
 }
 for (i = 0; i < array->len; i++) {
-printf("%s\n", (char *)array->pdata[i]);
+qemu_printf("%s\n", (char *)array->pdata[i]);
 }
 g_ptr_array_set_free_func(array, g_free);
 g_ptr_array_free(array, true);
-- 
2.26.2

[PATCH 04/28] qemu-option: move help handling to get_opt_name_value

[Paolo Bonzini]

Right now, help options are parsed normally and then checked
specially in opt_validate, but only if coming from
qemu_opts_parse_noisily.  has_help_option does the check on its own.

opt_validate() has two callers: qemu_opt_set(), which passes null and is
therefore unaffected, and opts_do_parse(), which is affected.

opts_do_parse() is called by qemu_opts_do_parse(), which passes null and
is therefore unaffected, and opts_parse().

opts_parse() is called by qemu_opts_parse() and qemu_opts_set_defaults(),
which pass null and are therefore unaffected, and
qemu_opts_parse_noisily().

Move the check from opt_validate to the parsing workhorse of QemuOpts,
get_opt_name_value.  This will come in handy in the next patch, which
will raise a warning for "-object memory-backend-ram,share" ("flag" option
with no =on/=off part) but not for "-object memory-backend-ram,help".

As a result:

- opts_parse and opts_do_parse do not return an error anymore
  when help is requested; qemu_opts_parse_noisily does not have
  to work around that anymore.

- various crazy ways to request help are not recognized anymore:
  - "help=..."
  - "nohelp" (sugar for "help=off")
  - "?=..."
  - "no?" (sugar for "?=off")

Signed-off-by: Paolo Bonzini 
---
 util/qemu-option.c | 38 +++---
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 91f4120ce1..5f27d4369d 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -496,8 +496,7 @@ static QemuOpt *opt_create(QemuOpts *opts, const char 
*name, char *value,
 return opt;
 }
 
-static bool opt_validate(QemuOpt *opt, bool *help_wanted,
- Error **errp)
+static bool opt_validate(QemuOpt *opt, Error **errp)
 {
 const QemuOptDesc *desc;
 const QemuOptsList *list = opt->opts->list;
@@ -505,9 +504,6 @@ static bool opt_validate(QemuOpt *opt, bool *help_wanted,
 desc = find_desc_by_name(list->desc, opt->name);
 if (!desc && !opts_accepts_any(list)) {
 error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
-if (help_wanted && is_help_option(opt->name)) {
-*help_wanted = true;
-}
 return false;
 }
 
@@ -524,7 +520,7 @@ bool qemu_opt_set(QemuOpts *opts, const char *name, const 
char *value,
 {
 QemuOpt *opt = opt_create(opts, name, g_strdup(value), false);
 
-if (!opt_validate(opt, NULL, errp)) {
+if (!opt_validate(opt, errp)) {
 qemu_opt_del(opt);
 return false;
 }
@@ -760,10 +756,12 @@ void qemu_opts_print(QemuOpts *opts, const char 
*separator)
 
 static const char *get_opt_name_value(const char *params,
   const char *firstname,
+  bool *help_wanted,
   char **name, char **value)
 {
 const char *p;
 size_t len;
+bool is_help = false;
 
 len = strcspn(params, "=,");
 if (params[len] != '=') {
@@ -780,6 +778,7 @@ static const char *get_opt_name_value(const char *params,
 *value = g_strdup("off");
 } else {
 *value = g_strdup("on");
+is_help = is_help_option(*name);
 }
 }
 } else {
@@ -791,6 +790,9 @@ static const char *get_opt_name_value(const char *params,
 }
 
 assert(!*p || *p == ',');
+if (help_wanted && is_help) {
+*help_wanted = true;
+}
 if (*p == ',') {
 p++;
 }
@@ -806,7 +808,12 @@ static bool opts_do_parse(QemuOpts *opts, const char 
*params,
 QemuOpt *opt;
 
 for (p = params; *p;) {
-p = get_opt_name_value(p, firstname, &option, &value);
+p = get_opt_name_value(p, firstname, help_wanted, &option, &value);
+if (help_wanted && *help_wanted) {
+g_free(option);
+g_free(value);
+return false;
+}
 firstname = NULL;
 
 if (!strcmp(option, "id")) {
@@ -817,7 +824,7 @@ static bool opts_do_parse(QemuOpts *opts, const char 
*params,
 
 opt = opt_create(opts, option, value, prepend);
 g_free(option);
-if (!opt_validate(opt, help_wanted, errp)) {
+if (!opt_validate(opt, errp)) {
 qemu_opt_del(opt);
 return false;
 }
@@ -832,7 +839,7 @@ static char *opts_parse_id(const char *params)
 char *name, *value;
 
 for (p = params; *p;) {
-p = get_opt_name_value(p, NULL, &name, &value);
+p = get_opt_name_value(p, NULL, NULL, &name, &value);
 if (!strcmp(name, "id")) {
 g_free(name);
 return value;
@@ -848,11 +855,10 @@ bool has_help_option(const char *params)
 {
 const char *p;
 char *name, *value;
-bool ret;
+bool ret = false;
 
 for (p = params; *p;) {
-p = get_opt_name_value(p, NULL, &name, &value);
-ret = is_help_option(name);
+p = get_opt_name_value(p, NULL, &ret, &name, &value);
 g_free(name);

[PATCH 06/28] keyval: accept escaped commas in implied option

[Paolo Bonzini]

This is used with the weirdly-named device "SUNFD,fdtwo":

  $ qemu-system-sparc -device SUNW,,fdtwo,help
  SUNW,fdtwo options:
drive=- Node name or ID of a block device to use as a 
backend
fallback= - FDC drive type, 144/288/120/none/auto (default: 
"144")
...

Therefore, accepting it is a preparatory step towards keyval-ifying
-device and the device_add monitor command.  In general, however, this
unexpected wart of the keyval syntax leads to suboptimal errors compared
to QemuOpts:

  $ ./qemu-system-x86_64 -object foo,,bar,id=obj
  qemu-system-x86_64: -object foo,,bar,id=obj: invalid object type: foo,bar
  $ storage-daemon/qemu-storage-daemon --object foo,,bar,id=obj
  qemu-storage-daemon: Invalid parameter ''

To implement this, the flow of the parser is changed to first unescape
everything up to the next comma or equal sign.  This is done in a
new function keyval_fetch_string for both the key and value part.
Keys therefore are now parsed in unescaped form, but this makes no
difference in practice because a comma is an invalid character for a
QAPI name.  Thus keys with a comma in them are rejected anyway, as
demonstrated by the new testcase.

As a side effect of the new code, parse errors are slightly improved as
well: "Invalid parameter ''" becomes "Expected parameter before '='"
when keyval is fed a string starting with an equal sign.

The slightly baroque interface of keyval_fetch_string lets me keep the
key parsing loop mostly untouched.  It is simplified in the next patch,
however.

Signed-off-by: Paolo Bonzini 
---
 include/qemu/help_option.h |  11 ---
 tests/test-keyval.c|  21 +++---
 util/keyval.c  | 142 -
 3 files changed, 91 insertions(+), 83 deletions(-)

diff --git a/include/qemu/help_option.h b/include/qemu/help_option.h
index ca6389a154..328d2a89fd 100644
--- a/include/qemu/help_option.h
+++ b/include/qemu/help_option.h
@@ -19,15 +19,4 @@ static inline bool is_help_option(const char *s)
 return !strcmp(s, "?") || !strcmp(s, "help");
 }
 
-static inline int starts_with_help_option(const char *s)
-{
-if (*s == '?') {
-return 1;
-}
-if (g_str_has_prefix(s, "help")) {
-return 4;
-}
-return 0;
-}
-
 #endif
diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index ee927fe4e4..19f664f535 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -89,6 +89,11 @@ static void test_keyval_parse(void)
 error_free_or_abort(&err);
 g_assert(!qdict);
 
+/* Keys must be QAPI identifiers */
+qdict = keyval_parse("weird,,=key", NULL, NULL, &err);
+error_free_or_abort(&err);
+g_assert(!qdict);
+
 /* Multiple keys, last one wins */
 qdict = keyval_parse("a=1,b=2,,x,a=3", NULL, NULL, &error_abort);
 g_assert_cmpuint(qdict_size(qdict), ==, 2);
@@ -178,15 +183,15 @@ static void test_keyval_parse(void)
 error_free_or_abort(&err);
 g_assert(!qdict);
 
-/* Likewise (qemu_opts_parse(): implied key with comma value) */
-qdict = keyval_parse(",,,a=1", "implied", NULL, &err);
-error_free_or_abort(&err);
-g_assert(!qdict);
+/* Implied key's value can have a comma */
+qdict = keyval_parse(",,,a=1", "implied", NULL, &error_abort);
+g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, ",");
+g_assert_cmpstr(qdict_get_try_str(qdict, "a"), ==, "1");
+qobject_unref(qdict);
 
-/* Implied key's value can't have comma (qemu_opts_parse(): it can) */
-qdict = keyval_parse("val,,ue", "implied", NULL, &err);
-error_free_or_abort(&err);
-g_assert(!qdict);
+qdict = keyval_parse("val,,ue", "implied", NULL, &error_abort);
+g_assert_cmpstr(qdict_get_try_str(qdict, "implied"), ==, "val,ue");
+qobject_unref(qdict);
 
 /* Empty key is not an implied key */
 qdict = keyval_parse("=val", "implied", NULL, &err);
diff --git a/util/keyval.c b/util/keyval.c
index 7f625ad33c..eb9b9c55ec 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -16,8 +16,8 @@
  *   key-vals = [ key-val { ',' key-val } [ ',' ] ]
  *   key-val  = key '=' val | help
  *   key  = key-fragment { '.' key-fragment }
- *   key-fragment = / [^=,.]+ /
- *   val  = { / [^,]+ / | ',,' }
+ *   key-fragment = { / [^=,.] / | ',,' }
+ *   val  = { / [^,] / | ',,' }
  *   help = 'help' | '?'
  *
  * Semantics defined by reduction to JSON:
@@ -78,13 +78,13 @@
  * Alternative syntax for use with an implied key:
  *
  *   key-vals = [ key-val-1st { ',' key-val } [ ',' ] ]
- *   key-val-1st  = val-no-key | key-val
- *   val-no-key   = / [^=,]+ / - help
+ *   key-val-1st  = (val-no-key - help) | key-val
+ *   val-no-key   = { / [^=,] / | ',,' }
  *
  * where val-no-key is syntactic sugar for implied-key=val-no-key.
  *
- * Note that you can't use the sugared form when the value contains
- * '=' or ','.
+ * Note that you can't use the sugared form when the value is empty
+ * or contains '='.
  */
 
 #include "

[PATCH 02/28] qemu-option: pass QemuOptsList to opts_accepts_any

[Paolo Bonzini]

A QemuOptsList can be of one of two kinds: either it is pre-validated, or
it accepts any key and validation happens somewhere else (typically in
a Visitor or against a list of QOM properties).  opts_accepts_any
returns true if a QemuOpts instance was created from a QemuOptsList of
the latter kind, but there is no function to do the check on a QemuOptsList.

We will need it in the next patch; since almost all callers of
opts_accepts_any use opts->list anyway, simply repurpose it instead
of adding a new function.

Signed-off-by: Paolo Bonzini 
---
 util/qemu-option.c | 23 +--
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 6bd654a473..c88e159f18 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -460,16 +460,16 @@ static bool qemu_opt_parse(QemuOpt *opt, Error **errp)
 }
 }
 
-static bool opts_accepts_any(const QemuOpts *opts)
+static bool opts_accepts_any(const QemuOptsList *list)
 {
-return opts->list->desc[0].name == NULL;
+return list->desc[0].name == NULL;
 }
 
 int qemu_opt_unset(QemuOpts *opts, const char *name)
 {
 QemuOpt *opt = qemu_opt_find(opts, name);
 
-assert(opts_accepts_any(opts));
+assert(opts_accepts_any(opts->list));
 
 if (opt == NULL) {
 return -1;
@@ -500,9 +500,10 @@ static bool opt_validate(QemuOpt *opt, bool *help_wanted,
  Error **errp)
 {
 const QemuOptDesc *desc;
+const QemuOptsList *list = opt->opts->list;
 
-desc = find_desc_by_name(opt->opts->list->desc, opt->name);
-if (!desc && !opts_accepts_any(opt->opts)) {
+desc = find_desc_by_name(list->desc, opt->name);
+if (!desc && !opts_accepts_any(list)) {
 error_setg(errp, QERR_INVALID_PARAMETER, opt->name);
 if (help_wanted && is_help_option(opt->name)) {
 *help_wanted = true;
@@ -535,9 +536,10 @@ bool qemu_opt_set_bool(QemuOpts *opts, const char *name, 
bool val,
 {
 QemuOpt *opt;
 const QemuOptDesc *desc;
+const QemuOptsList *list = opts->list;
 
-desc = find_desc_by_name(opts->list->desc, name);
-if (!desc && !opts_accepts_any(opts)) {
+desc = find_desc_by_name(list->desc, name);
+if (!desc && !opts_accepts_any(list)) {
 error_setg(errp, QERR_INVALID_PARAMETER, name);
 return false;
 }
@@ -557,9 +559,10 @@ bool qemu_opt_set_number(QemuOpts *opts, const char *name, 
int64_t val,
 {
 QemuOpt *opt;
 const QemuOptDesc *desc;
+const QemuOptsList *list = opts->list;
 
-desc = find_desc_by_name(opts->list->desc, name);
-if (!desc && !opts_accepts_any(opts)) {
+desc = find_desc_by_name(list->desc, name);
+if (!desc && !opts_accepts_any(list)) {
 error_setg(errp, QERR_INVALID_PARAMETER, name);
 return false;
 }
@@ -1107,7 +1110,7 @@ bool qemu_opts_validate(QemuOpts *opts, const QemuOptDesc 
*desc, Error **errp)
 {
 QemuOpt *opt;
 
-assert(opts_accepts_any(opts));
+assert(opts_accepts_any(opts->list));
 
 QTAILQ_FOREACH(opt, &opts->head, next) {
 opt->desc = find_desc_by_name(desc, opt->name);
-- 
2.26.2

[PATCH 15/28] qemu-option: support accept-any QemuOptsList in qemu_opts_absorb_qdict

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini 
---
 util/qemu-option.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 40564a12eb..afba08d92e 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -1052,7 +1052,8 @@ bool qemu_opts_absorb_qdict(QemuOpts *opts, QDict *qdict, 
Error **errp)
 while (entry != NULL) {
 next = qdict_next(qdict, entry);
 
-if (find_desc_by_name(opts->list->desc, entry->key)) {
+if (opts_accepts_any(opts->list) ||
+find_desc_by_name(opts->list->desc, entry->key)) {
 if (!qemu_opts_from_qdict_entry(opts, entry, errp)) {
 return false;
 }
-- 
2.26.2

[PATCH 12/28] hmp: special case help options for object_add

[Paolo Bonzini]

Fix "object_add help" and "object_add TYPE,help".

Signed-off-by: Paolo Bonzini 
---
 include/qom/object_interfaces.h |  9 -
 monitor/hmp-cmds.c  | 22 --
 qom/object_interfaces.c |  2 +-
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 07d5cc8832..abb23eaea3 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -149,6 +149,13 @@ typedef bool (*user_creatable_add_opts_predicate)(const 
char *type);
 int user_creatable_add_opts_foreach(void *opaque,
 QemuOpts *opts, Error **errp);
 
+/**
+ * user_creatable_print_types:
+ *
+ * Prints a list of user-creatable objects to stdout or the monitor.
+ */
+void user_creatable_print_types(void);
+
 /**
  * user_creatable_print_help:
  * @type: the QOM type to be added
@@ -174,7 +181,7 @@ bool user_creatable_print_help(const char *type, QemuOpts 
*opts);
  * no help was requested. It should only be called if we know that help is
  * requested and it will always print some help.
  */
-void user_creatable_print_help_from_qdict(QDict *args);
+void user_creatable_print_help_from_qdict(const QDict *args);
 
 /**
  * user_creatable_del:
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 65d8ff4849..153ece8176 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1664,23 +1664,17 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
 void hmp_object_add(Monitor *mon, const QDict *qdict)
 {
 Error *err = NULL;
-QemuOpts *opts;
-Object *obj = NULL;
 
-opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
-if (err) {
-goto end;
+if (is_help_option(qdict_get_str(qdict, "qom-type"))) {
+user_creatable_print_types();
+return;
 }
-
-obj = user_creatable_add_opts(opts, &err);
-qemu_opts_del(opts);
-
-end:
-hmp_handle_error(mon, err);
-
-if (obj) {
-object_unref(obj);
+if (qdict_haskey(qdict, "help")) {
+user_creatable_print_help_from_qdict(qdict);
+return;
 }
+user_creatable_add_dict((QDict *)qdict, true, &err);
+hmp_handle_error(mon, err);
 }
 
 void hmp_getfd(Monitor *mon, const QDict *qdict)
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 34edc3d1d8..f7dcdf18e2 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -280,7 +280,7 @@ bool user_creatable_print_help(const char *type, QemuOpts 
*opts)
 return false;
 }
 
-void user_creatable_print_help_from_qdict(QDict *args)
+void user_creatable_print_help_from_qdict(const QDict *args)
 {
 const char *type = qdict_get_try_str(args, "qom-type");
 
-- 
2.26.2

[PATCH 13/28] remove -writeconfig

[Paolo Bonzini]

Like -set and -readconfig, it would not really be too hard to
extend -writeconfig to parsing mechanisms other than QemuOpts.
However, the uses of -writeconfig are substantially more
limited, as it is generally easier to write the configuration
by hand in the first place.  In addition, -writeconfig does
not even try to detect cases where it prints incorrect
syntax (for example if values have a quote in them, since
qemu_config_parse does not support any kind of escaping.
Just remove it.

Signed-off-by: Paolo Bonzini 
---
 include/qemu/config-file.h |  1 -
 qemu-options.hx| 13 ++--
 softmmu/vl.c   | 19 -
 util/qemu-config.c | 42 --
 4 files changed, 2 insertions(+), 73 deletions(-)

diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index 29226107bd..7d26fe3816 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -10,7 +10,6 @@ void qemu_add_opts(QemuOptsList *list);
 void qemu_add_drive_opts(QemuOptsList *list);
 int qemu_global_option(const char *str);
 
-void qemu_config_write(FILE *fp);
 int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname);
 
 int qemu_read_config_file(const char *filename);
diff --git a/qemu-options.hx b/qemu-options.hx
index e60ad42976..408d9c2def 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -4278,23 +4278,14 @@ SRST
 ERST
 
 DEF("readconfig", HAS_ARG, QEMU_OPTION_readconfig,
-"-readconfig \n", QEMU_ARCH_ALL)
+"-readconfig \n",
+"read config file\n", QEMU_ARCH_ALL)
 SRST
 ``-readconfig file``
 Read device configuration from file. This approach is useful when
 you want to spawn QEMU process with many command line options but
 you don't want to exceed the command line character limit.
 ERST
-DEF("writeconfig", HAS_ARG, QEMU_OPTION_writeconfig,
-"-writeconfig \n"
-"read/write config file\n", QEMU_ARCH_ALL)
-SRST
-``-writeconfig file``
-Write device configuration to file. The file can be either filename
-to save command line and device configuration into file or dash
-``-``) character to print the output to stdout. This can be later
-used as input file for ``-readconfig`` option.
-ERST
 
 DEF("no-user-config", 0, QEMU_OPTION_nouserconfig,
 "-no-user-config\n"
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 023c16245b..4039bf3a39 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3309,25 +3309,6 @@ void qemu_init(int argc, char **argv, char **envp)
 }
 display_remote++;
 break;
-case QEMU_OPTION_writeconfig:
-{
-FILE *fp;
-if (strcmp(optarg, "-") == 0) {
-fp = stdout;
-} else {
-fp = fopen(optarg, "w");
-if (fp == NULL) {
-error_report("open %s: %s", optarg,
- strerror(errno));
-exit(1);
-}
-}
-qemu_config_write(fp);
-if (fp != stdout) {
-fclose(fp);
-}
-break;
-}
 case QEMU_OPTION_qtest:
 qtest_chrdev = optarg;
 break;
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 725e3d7e4b..cc5be3c779 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -313,48 +313,6 @@ void qemu_add_opts(QemuOptsList *list)
 abort();
 }
 
-struct ConfigWriteData {
-QemuOptsList *list;
-FILE *fp;
-};
-
-static int config_write_opt(void *opaque, const char *name, const char *value,
-Error **errp)
-{
-struct ConfigWriteData *data = opaque;
-
-fprintf(data->fp, "  %s = \"%s\"\n", name, value);
-return 0;
-}
-
-static int config_write_opts(void *opaque, QemuOpts *opts, Error **errp)
-{
-struct ConfigWriteData *data = opaque;
-const char *id = qemu_opts_id(opts);
-
-if (id) {
-fprintf(data->fp, "[%s \"%s\"]\n", data->list->name, id);
-} else {
-fprintf(data->fp, "[%s]\n", data->list->name);
-}
-qemu_opt_foreach(opts, config_write_opt, data, NULL);
-fprintf(data->fp, "\n");
-return 0;
-}
-
-void qemu_config_write(FILE *fp)
-{
-struct ConfigWriteData data = { .fp = fp };
-QemuOptsList **lists = vm_config_groups;
-int i;
-
-fprintf(fp, "# qemu config file\n\n");
-for (i = 0; lists[i] != NULL; i++) {
-data.list = lists[i];
-qemu_opts_foreach(data.list, config_write_opts, &data, NULL);
-}
-}
-
 /* Returns number of config groups on success, -errno on error */
 int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname)
 {
-- 
2.26.2

[PATCH 21/28] qemu-img: use keyval for -object parsing

[Paolo Bonzini]

Enable creation of object with non-scalar properties.

Signed-off-by: Paolo Bonzini 
---
 qemu-img.c | 258 +++--
 1 file changed, 52 insertions(+), 206 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 8bdea40b58..a91bbba4c6 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -227,21 +227,27 @@ static void QEMU_NORETURN help(void)
 exit(EXIT_SUCCESS);
 }
 
-static QemuOptsList qemu_object_opts = {
-.name = "object",
-.implied_opt_name = "qom-type",
-.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-.desc = {
-{ }
-},
-};
-
-static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
+static void qemu_img_object_parse(const char *optarg, int exit_code)
 {
-if (user_creatable_print_help(type, opts)) {
-exit(0);
+QDict *args;
+bool help;
+Error *local_error = NULL;
+
+args = keyval_parse(optarg, "qom-type", &help, &local_error);
+if (local_error) {
+error_report_err(local_error);
+exit(exit_code);
 }
-return true;
+if (help) {
+user_creatable_print_help_from_qdict(args);
+exit(EXIT_SUCCESS);
+}
+user_creatable_add_dict(args, true, &local_error);
+if (local_error) {
+error_report_err(local_error);
+exit(exit_code);
+}
+qobject_unref(args);
 }
 
 /*
@@ -567,14 +573,9 @@ static int img_create(int argc, char **argv)
 case 'u':
 flags |= BDRV_O_NO_BACKING;
 break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(&qemu_object_opts,
-   optarg, true);
-if (!opts) {
-goto fail;
-}
-}   break;
+case OPTION_OBJECT:
+qemu_img_object_parse(optarg, 1);
+break;
 }
 }
 
@@ -590,12 +591,6 @@ static int img_create(int argc, char **argv)
 }
 optind++;
 
-if (qemu_opts_foreach(&qemu_object_opts,
-  user_creatable_add_opts_foreach,
-  qemu_img_object_print_help, &error_fatal)) {
-goto fail;
-}
-
 /* Get image size, if specified */
 if (optind < argc) {
 int64_t sval;
@@ -805,14 +800,9 @@ static int img_check(int argc, char **argv)
 case 'U':
 force_share = true;
 break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(&qemu_object_opts,
-   optarg, true);
-if (!opts) {
-return 1;
-}
-}   break;
+case OPTION_OBJECT:
+qemu_img_object_parse(optarg, 1);
+break;
 case OPTION_IMAGE_OPTS:
 image_opts = true;
 break;
@@ -832,12 +822,6 @@ static int img_check(int argc, char **argv)
 return 1;
 }
 
-if (qemu_opts_foreach(&qemu_object_opts,
-  user_creatable_add_opts_foreach,
-  qemu_img_object_print_help, &error_fatal)) {
-return 1;
-}
-
 ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
 if (ret < 0) {
 error_report("Invalid source cache option: %s", cache);
@@ -1035,14 +1019,9 @@ static int img_commit(int argc, char **argv)
 return 1;
 }
 break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(&qemu_object_opts,
-   optarg, true);
-if (!opts) {
-return 1;
-}
-}   break;
+case OPTION_OBJECT:
+qemu_img_object_parse(optarg, 1);
+break;
 case OPTION_IMAGE_OPTS:
 image_opts = true;
 break;
@@ -1059,12 +1038,6 @@ static int img_commit(int argc, char **argv)
 }
 filename = argv[optind++];
 
-if (qemu_opts_foreach(&qemu_object_opts,
-  user_creatable_add_opts_foreach,
-  qemu_img_object_print_help, &error_fatal)) {
-return 1;
-}
-
 flags = BDRV_O_RDWR | BDRV_O_UNMAP;
 ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
 if (ret < 0) {
@@ -1424,15 +1397,9 @@ static int img_compare(int argc, char **argv)
 case 'U':
 force_share = true;
 break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(&qemu_object_opts,
-   optarg, true);
-if (!opts) {
-ret = 2;
-goto out4;
-}
-}   break;
+case OPTION_OBJECT:
+qemu_img_object_parse(optarg, 2);
+break;
 case OPTION_IMAGE_OPTS:
 image_opts = true;
 break;
@@ -1451,13 +1418,6 @@ static 

[PATCH 10/28] hmp: replace "O" parser with keyval

[Paolo Bonzini]

HMP is using QemuOpts to parse free-form commands device_add,
netdev_add and object_add.  However, none of these need QemuOpts
for validation (these three QemuOptsLists are all of the catch-all
kind), and keyval is already able to parse into QDict.  So use
keyval directly, avoiding the detour from
string to QemuOpts to QDict.

The args_type now stores the implied key.  This arguably makes more
sense than storing the QemuOptsList name; at least, it _is_ a key
that might end up in the arguments QDict.

Signed-off-by: Paolo Bonzini 
---
 hmp-commands.hx |  6 +++---
 monitor/hmp.c   | 20 +---
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 737c647c46..b3e54de27a 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -676,7 +676,7 @@ ERST
 
 {
 .name   = "device_add",
-.args_type  = "device:O",
+.args_type  = "driver:O",
 .params = "driver[,prop=value][,...]",
 .help   = "add device, like -device on the command line",
 .cmd= hmp_device_add,
@@ -1322,7 +1322,7 @@ ERST
 
 {
 .name   = "netdev_add",
-.args_type  = "netdev:O",
+.args_type  = "type:O",
 .params = 
"[user|tap|socket|vde|bridge|hubport|netmap|vhost-user],id=str[,prop=value][,...]",
 .help   = "add host network device",
 .cmd= hmp_netdev_add,
@@ -1352,7 +1352,7 @@ ERST
 
 {
 .name   = "object_add",
-.args_type  = "object:O",
+.args_type  = "qom-type:O",
 .params = "[qom-type=]type,id=str[,prop=value][,...]",
 .help   = "create QOM object",
 .cmd= hmp_object_add,
diff --git a/monitor/hmp.c b/monitor/hmp.c
index 6c0b33a0b1..d2cb886da5 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -744,13 +744,9 @@ static QDict *monitor_parse_arguments(Monitor *mon,
 break;
 case 'O':
 {
-QemuOptsList *opts_list;
-QemuOpts *opts;
+Error *errp;
+bool help;
 
-opts_list = qemu_find_opts(key);
-if (!opts_list || opts_list->desc->name) {
-goto bad_type;
-}
 while (qemu_isspace(*p)) {
 p++;
 }
@@ -760,12 +756,14 @@ static QDict *monitor_parse_arguments(Monitor *mon,
 if (get_str(buf, sizeof(buf), &p) < 0) {
 goto fail;
 }
-opts = qemu_opts_parse_noisily(opts_list, buf, true);
-if (!opts) {
-goto fail;
+keyval_parse_into(qdict, buf, key, &help, &errp);
+if (help) {
+if (qdict_haskey(qdict, key)) {
+qdict_put_bool(qdict, "help", true);
+} else {
+qdict_put_str(qdict, key, "help");
+}
 }
-qemu_opts_to_qdict(opts, qdict);
-qemu_opts_del(opts);
 }
 break;
 case '/':
-- 
2.26.2

[PATCH 19/28] qemu-io: use keyval for -object parsing

[Paolo Bonzini]

Enable creation of object with non-scalar properties.

Signed-off-by: Paolo Bonzini 
---
 qemu-io.c | 42 +-
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index ac88d8bd40..306086f767 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -477,23 +477,6 @@ enum {
 OPTION_IMAGE_OPTS = 257,
 };
 
-static QemuOptsList qemu_object_opts = {
-.name = "object",
-.implied_opt_name = "qom-type",
-.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-.desc = {
-{ }
-},
-};
-
-static bool qemu_io_object_print_help(const char *type, QemuOpts *opts)
-{
-if (user_creatable_print_help(type, opts)) {
-exit(0);
-}
-return true;
-}
-
 static QemuOptsList file_opts = {
 .name = "file",
 .implied_opt_name = "file",
@@ -550,7 +533,6 @@ int main(int argc, char **argv)
 qcrypto_init(&error_fatal);
 
 module_call_init(MODULE_INIT_QOM);
-qemu_add_opts(&qemu_object_opts);
 qemu_add_opts(&qemu_trace_opts);
 bdrv_init();
 
@@ -612,14 +594,20 @@ int main(int argc, char **argv)
 case 'U':
 force_share = true;
 break;
-case OPTION_OBJECT: {
-QemuOpts *qopts;
-qopts = qemu_opts_parse_noisily(&qemu_object_opts,
-optarg, true);
-if (!qopts) {
-exit(1);
+case OPTION_OBJECT:
+{
+QDict *args;
+bool help;
+
+args = keyval_parse(optarg, "qom-type", &help, &error_fatal);
+if (help) {
+user_creatable_print_help_from_qdict(args);
+exit(EXIT_SUCCESS);
+}
+user_creatable_add_dict(args, true, &error_fatal);
+qobject_unref(args);
+break;
 }
-}   break;
 case OPTION_IMAGE_OPTS:
 imageOpts = true;
 break;
@@ -644,10 +632,6 @@ int main(int argc, char **argv)
 exit(1);
 }
 
-qemu_opts_foreach(&qemu_object_opts,
-  user_creatable_add_opts_foreach,
-  qemu_io_object_print_help, &error_fatal);
-
 if (!trace_init_backends()) {
 exit(1);
 }
-- 
2.26.2

[PATCH 17/28] vl: plumb keyval-based options into -set and -readconfig

[Paolo Bonzini]

Add generic machinery to support parsing command line options with
keyval in -set and -readconfig, choosing between QDict and
QemuOpts as the underlying data structure.

The keyval_merge function is slightly heavyweight as a way to
do qemu_set_option for QDict-based options, but it will be put
to further use later to merge entire -readconfig sections together
with their command-line equivalents.

Signed-off-by: Paolo Bonzini 
---
 include/block/qdict.h|   2 -
 include/qapi/qmp/qdict.h |   3 +
 include/qemu/option.h|   1 +
 softmmu/vl.c | 131 ---
 tests/test-keyval.c  |  32 ++
 util/keyval.c|  36 +++
 6 files changed, 182 insertions(+), 23 deletions(-)

diff --git a/include/block/qdict.h b/include/block/qdict.h
index d8cb502d7d..ced2acfb92 100644
--- a/include/block/qdict.h
+++ b/include/block/qdict.h
@@ -20,8 +20,6 @@ void qdict_join(QDict *dest, QDict *src, bool overwrite);
 void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
 void qdict_array_split(QDict *src, QList **dst);
 int qdict_array_entries(QDict *src, const char *subqdict);
-QObject *qdict_crumple(const QDict *src, Error **errp);
-void qdict_flatten(QDict *qdict);
 
 typedef struct QDictRenames {
 const char *from;
diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
index da942347a7..8eb1dc9500 100644
--- a/include/qapi/qmp/qdict.h
+++ b/include/qapi/qmp/qdict.h
@@ -66,4 +66,7 @@ const char *qdict_get_try_str(const QDict *qdict, const char 
*key);
 
 QDict *qdict_clone_shallow(const QDict *src);
 
+QObject *qdict_crumple(const QDict *src, Error **errp);
+void qdict_flatten(QDict *qdict);
+
 #endif /* QDICT_H */
diff --git a/include/qemu/option.h b/include/qemu/option.h
index 092e291c37..fffb03d848 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -151,5 +151,6 @@ QDict *keyval_parse_into(QDict *qdict, const char *params, 
const char *implied_k
  bool *p_help, Error **errp);
 QDict *keyval_parse(const char *params, const char *implied_key,
 bool *help, Error **errp);
+void keyval_merge(QDict *old, const QDict *new, Error **errp);
 
 #endif
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 771af8bb85..de3e22c9eb 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -115,6 +115,7 @@
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-commands-misc.h"
 #include "qapi/qapi-commands-ui.h"
+#include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
 #include "sysemu/iothread.h"
 #include "qemu/guest-random.h"
@@ -2045,13 +2046,94 @@ static int global_init_func(void *opaque, QemuOpts 
*opts, Error **errp)
 return 0;
 }
 
+/*
+ * Return whether configuration group @group is stored in QemuOpts or
+ * in a list of QDicts.
+ */
+static bool is_qemuopts_group(const char *group)
+{
+return true;
+}
+
+/*
+ * Return a pointer to a list of QDicts, used to store options for
+ * "-set GROUP.*".
+ */
+static GSList **qemu_config_list(const char *group)
+{
+return NULL;
+}
+
+/*
+ * Return a pointer to a QDict inside the list starting at @head,
+ * used to store options for "-GROUP id=...".
+ */
+static QDict *qemu_find_config(GSList *head, const char *id)
+{
+assert(id);
+while (head) {
+QDict *dict = head->data;
+if (g_strcmp0(qdict_get_try_str(dict, "id"), id) == 0) {
+return dict;
+}
+head = head->next;
+}
+return NULL;
+}
+
+static void qemu_record_config_group(const char *group, QDict *dict, Error 
**errp)
+{
+GSList **p_head;
+
+p_head = qemu_config_list(group);
+if (p_head) {
+*p_head = g_slist_prepend(*p_head, dict);
+} else {
+abort();
+}
+}
+
+static void qemu_set_qdict_option(QDict *dict, const char *key, const char 
*value,
+  Error **errp)
+{
+QDict *merge_dict;
+
+merge_dict = qdict_new();
+qdict_put_str(merge_dict, key, value);
+keyval_merge(dict, merge_dict, errp);
+qobject_unref(merge_dict);
+}
+
+/*
+ * Parse non-QemuOpts config file groups, pass the rest to
+ * qemu_config_do_parse.
+ */
+static void qemu_parse_config_group(const char *group, QDict *qdict,
+void *opaque, Error **errp)
+{
+if (is_qemuopts_group(group)) {
+QObject *crumpled = qdict_crumple(qdict, errp);
+if (!crumpled) {
+return;
+}
+if (qobject_type(crumpled) != QTYPE_QDICT) {
+assert(qobject_type(crumpled) == QTYPE_QLIST);
+error_setg(errp, "Lists cannot be at top level of a configuration 
section");
+return;
+}
+qemu_record_config_group(group, qobject_to(QDict, crumpled), errp);
+} else {
+qemu_config_do_parse(group, qdict, opaque, errp);
+}
+}
+
 static void qemu_read_default_config_file(Error **errp)
 {
 int ret;
 Error *local_err = NULL;
 g_autofree char *file = get_reloca

[PATCH 16/28] qemu-config: parse configuration files to a QDict

[Paolo Bonzini]

Change the parser to put the values into a QDict and pass them
to a callback.  qemu_config_parse's QemuOpts creation is
itself turned into a callback function.

Signed-off-by: Paolo Bonzini 
---
 include/qemu/config-file.h |  6 ++-
 softmmu/vl.c   |  4 +-
 util/qemu-config.c | 91 +-
 3 files changed, 68 insertions(+), 33 deletions(-)

diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index da6f4690b7..dcf2948435 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -1,6 +1,7 @@
 #ifndef QEMU_CONFIG_FILE_H
 #define QEMU_CONFIG_FILE_H
 
+typedef void QEMUConfigCB(const char *group, QDict *qdict, void *opaque, Error 
**errp);
 
 QemuOptsList *qemu_find_opts(const char *group);
 QemuOptsList *qemu_find_opts_err(const char *group, Error **errp);
@@ -12,7 +13,10 @@ int qemu_global_option(const char *str);
 
 int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error 
**errp);
 
-int qemu_read_config_file(const char *filename, Error **errp);
+/* A default callback for qemu_read_config_file().  */
+void qemu_config_do_parse(const char *group, QDict *qdict, void *opaque, Error 
**errp);
+
+int qemu_read_config_file(const char *filename, QEMUConfigCB *f, Error **errp);
 
 /* Parse QDict options as a replacement for a config file (allowing multiple
enumerated (0..(n-1)) configuration "sections") */
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 16a5cd1046..771af8bb85 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2051,7 +2051,7 @@ static void qemu_read_default_config_file(Error **errp)
 Error *local_err = NULL;
 g_autofree char *file = get_relocated_path(CONFIG_QEMU_CONFDIR 
"/qemu.conf");
 
-ret = qemu_read_config_file(file, &local_err);
+ret = qemu_read_config_file(file, qemu_config_do_parse, &local_err);
 if (ret < 0) {
 if (ret == -ENOENT) {
 error_free(local_err);
@@ -3285,7 +3285,7 @@ void qemu_init(int argc, char **argv, char **envp)
 qemu_plugin_opt_parse(optarg, &plugin_list);
 break;
 case QEMU_OPTION_readconfig:
-qemu_read_config_file(optarg, &error_fatal);
+qemu_read_config_file(optarg, qemu_config_do_parse, 
&error_fatal);
 break;
 case QEMU_OPTION_spice:
 olist = qemu_find_opts_err("spice", NULL);
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 85f20079de..32d3d6e954 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -314,19 +314,19 @@ void qemu_add_opts(QemuOptsList *list)
 }
 
 /* Returns number of config groups on success, -errno on error */
-int qemu_config_parse(FILE *fp, QemuOptsList **lists, const char *fname, Error 
**errp)
+static int qemu_config_foreach(FILE *fp, QEMUConfigCB *cb, void *opaque,
+   const char *fname, Error **errp)
 {
-char line[1024], group[64], id[64], arg[64], value[1024];
+char line[1024], prev_group[64], group[64], arg[64], value[1024];
 Location loc;
-QemuOptsList *list = NULL;
 Error *local_err = NULL;
-QemuOpts *opts = NULL;
+QDict *qdict = NULL;
 int res = -EINVAL, lno = 0;
 int count = 0;
 
 loc_push_none(&loc);
 while (fgets(line, sizeof(line), fp) != NULL) {
-loc_set_file(fname, ++lno);
+++lno;
 if (line[0] == '\n') {
 /* skip empty lines */
 continue;
@@ -335,39 +335,39 @@ int qemu_config_parse(FILE *fp, QemuOptsList **lists, 
const char *fname, Error *
 /* comment */
 continue;
 }
-if (sscanf(line, "[%63s \"%63[^\"]\"]", group, id) == 2) {
-/* group with id */
-list = find_list(lists, group, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+if (line[0] == '[') {
+QDict *prev = qdict;
+if (sscanf(line, "[%63s \"%63[^\"]\"]", group, value) == 2) {
+qdict = qdict_new();
+qdict_put_str(qdict, "id", value);
+count++;
+} else if (sscanf(line, "[%63[^]]]", group) == 1) {
+qdict = qdict_new();
+count++;
 }
-opts = qemu_opts_create(list, id, 1, NULL);
-count++;
-continue;
-}
-if (sscanf(line, "[%63[^]]]", group) == 1) {
-/* group without id */
-list = find_list(lists, group, &local_err);
-if (local_err) {
-error_propagate(errp, local_err);
-goto out;
+if (qdict != prev) {
+if (prev) {
+cb(prev_group, prev, opaque, &local_err);
+qobject_unref(prev);
+if (local_err) {
+error_propagate(errp, local_err);
+goto out;
+}
+ 

[PATCH 00/28] qemu-option, keyval, vl: switch -object/-M/-accel to keyval parsing

[Paolo Bonzini]

This series switches -object, -M and -accel from QemuOpts to keyval.
Monitor commands device_add and netdev_add are also switched to keyval,
though -device and -netdev for now are not.

Along the way, the syntax of keyval and QemuOpts becomes more consistent
and support for keyval-based options is added to -readconfig.  -writeconfig
instead is removed (see patch 13 for rationale).

The reason to do this is:

- to make qemu-io, qemu-nbd, qemu-img and QEMU's parsing of -object
  consistent with qemu-storage-daemon's

- to allow using compound properties in -object, -M and -accel

Patch 1-5: make QemuOpts parsing a bit more restrictive, warning for
short-form boolean options and removing weird ways to request help
such as "help=foo" or "no?".

Patch 6-12: let keyval accept escaped commas in implied options,
switch comma-separated syntax for HMP from QemuOpts to keyval,
add help support to object_add

Patch 13-18: plumbing for reading keyval-based options in vl.c,
including -set and -readconfig.

Patch 19-23: switch -object to keyval everywhere

Patch 24-28: switch -M and -accel to keyval

Paolo Bonzini (28):
  qemu-option: simplify search for end of key
  qemu-option: pass QemuOptsList to opts_accepts_any
  qemu-option: clean up id vs. list->merge_lists
  qemu-option: move help handling to get_opt_name_value
  qemu-option: warn for short-form boolean options
  keyval: accept escaped commas in implied option
  keyval: simplify keyval_parse_one
  tests: convert check-qom-proplist to keyval
  keyval: introduce keyval_parse_into
  hmp: replace "O" parser with keyval
  qom: use qemu_printf to print help for user-creatable objects
  hmp: special case help options for object_add
  remove -writeconfig
  qemu-config: add error propagation to qemu_config_parse
  qemu-option: support accept-any QemuOptsList in qemu_opts_absorb_qdict
  qemu-config: parse configuration files to a QDict
  vl: plumb keyval-based options into -set and -readconfig
  qom: do not modify QDict argument in user_creatable_add_dict
  qemu-io: use keyval for -object parsing
  qemu-nbd: use keyval for -object parsing
  qemu-img: use keyval for -object parsing
  qemu: use keyval for -object parsing
  storage-daemon: do not register the "object" group with QemuOpts
  qom: export more functions for use with non-UserCreatable objects
  vl: rename local variable in configure_accelerators
  vl: switch -M parsing to keyval
  qemu-option: remove now-dead code
  vl: switch -accel parsing to keyval

 accel/accel.c|   6 +
 block/blkdebug.c |   3 +-
 docs/system/deprecated.rst   |   6 +
 hmp-commands.hx  |   6 +-
 include/block/qdict.h|   2 -
 include/qapi/qmp/qdict.h |   3 +
 include/qemu/config-file.h   |   9 +-
 include/qemu/help_option.h   |  11 -
 include/qemu/option.h|   6 +-
 include/qom/object.h |  21 +
 include/qom/object_interfaces.h  |  68 +--
 include/sysemu/accel.h   |   1 +
 monitor/hmp-cmds.c   |  22 +-
 monitor/hmp.c|  20 +-
 qemu-img.c   | 258 ++
 qemu-io.c|  42 +-
 qemu-nbd.c   |  42 +-
 qemu-options.hx  |  13 +-
 qom/object_interfaces.c  | 152 ++
 softmmu/vl.c | 687 ++-
 storage-daemon/qemu-storage-daemon.c |  10 -
 tests/check-qom-proplist.c   |  58 ++-
 tests/test-keyval.c  |  53 ++-
 tests/test-qemu-opts.c   |  37 +-
 util/keyval.c| 230 +
 util/qemu-config.c   | 141 +++---
 util/qemu-option.c   | 184 ---
 27 files changed, 946 insertions(+), 1145 deletions(-)

-- 
2.26.2

[PATCH 09/28] keyval: introduce keyval_parse_into

[Paolo Bonzini]

Allow parsing multiple keyval sequences into the same dictionary.

Signed-off-by: Paolo Bonzini 
---
 include/qemu/option.h |  2 ++
 util/keyval.c | 39 ---
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index f73e0dc7d9..092e291c37 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -147,6 +147,8 @@ void qemu_opts_print_help(QemuOptsList *list, bool 
print_caption);
 void qemu_opts_free(QemuOptsList *list);
 QemuOptsList *qemu_opts_append(QemuOptsList *dst, QemuOptsList *list);
 
+QDict *keyval_parse_into(QDict *qdict, const char *params, const char 
*implied_key,
+ bool *p_help, Error **errp);
 QDict *keyval_parse(const char *params, const char *implied_key,
 bool *help, Error **errp);
 
diff --git a/util/keyval.c b/util/keyval.c
index e7f708cd1e..1d4ca12129 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -436,13 +436,12 @@ static QObject *keyval_listify(QDict *cur, GSList 
*key_of_cur, Error **errp)
  * If @p_help is not NULL, store whether help is requested there.
  * If @p_help is NULL and help is requested, fail.
  *
- * On success, return a dictionary of the parsed keys and values.
+ * On success, return @dict, now filled with the parsed keys and values.
  * On failure, store an error through @errp and return NULL.
  */
-QDict *keyval_parse(const char *params, const char *implied_key,
-bool *p_help, Error **errp)
+QDict *keyval_parse_into(QDict *qdict, const char *params, const char 
*implied_key,
+ bool *p_help, Error **errp)
 {
-QDict *qdict = qdict_new();
 QObject *listified;
 g_autofree char *dup;
 char *s;
@@ -452,7 +451,6 @@ QDict *keyval_parse(const char *params, const char 
*implied_key,
 while (*s) {
 s = keyval_parse_one(qdict, s, implied_key, &help, errp);
 if (!s) {
-qobject_unref(qdict);
 return NULL;
 }
 implied_key = NULL;
@@ -462,15 +460,42 @@ QDict *keyval_parse(const char *params, const char 
*implied_key,
 *p_help = help;
 } else if (help) {
 error_setg(errp, "Help is not available for this option");
-qobject_unref(qdict);
 return NULL;
 }
 
 listified = keyval_listify(qdict, NULL, errp);
 if (!listified) {
-qobject_unref(qdict);
 return NULL;
 }
 assert(listified == QOBJECT(qdict));
 return qdict;
 }
+
+/*
+ * Parse @params in QEMU's traditional KEY=VALUE,... syntax.
+ *
+ * If @implied_key, the first KEY= can be omitted.  @implied_key is
+ * implied then, and VALUE can't be empty or contain ',' or '='.
+ *
+ * A parameter "help" or "?" without a value isn't added to the
+ * resulting dictionary, but instead is interpreted as help request.
+ * All other options are parsed and returned normally so that context
+ * specific help can be printed.
+ *
+ * If @p_help is not NULL, store whether help is requested there.
+ * If @p_help is NULL and help is requested, fail.
+ *
+ * On success, return a dictionary of the parsed keys and values.
+ * On failure, store an error through @errp and return NULL.
+ */
+QDict *keyval_parse(const char *params, const char *implied_key,
+bool *p_help, Error **errp)
+{
+QDict *qdict = qdict_new();
+QDict *ret = keyval_parse_into(qdict, params, implied_key, p_help, errp);
+
+if (!ret) {
+qobject_unref(qdict);
+}
+return ret;
+}
-- 
2.26.2

[PATCH 07/28] keyval: simplify keyval_parse_one

[Paolo Bonzini]

Now that the key is NULL terminated, we can remove some of the contortions
that were done to operate on possibly '='-terminated strings in
keyval_parse_one.

Signed-off-by: Paolo Bonzini 
---
 util/keyval.c | 27 ++-
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/util/keyval.c b/util/keyval.c
index eb9b9c55ec..e7f708cd1e 100644
--- a/util/keyval.c
+++ b/util/keyval.c
@@ -170,11 +170,10 @@ static QObject *keyval_parse_put(QDict *cur,
  *
  * On return:
  * - either NUL or the separator (comma or equal sign) is returned.
- * - the length of the string is stored in @len.
  * - @start is advanced to either the NUL or the first character past the
  *   separator.
  */
-static char keyval_fetch_string(char **start, size_t *len, bool key)
+static char keyval_fetch_string(char **start, bool key)
 {
 char sep;
 char *p, *unescaped;
@@ -197,7 +196,6 @@ static char keyval_fetch_string(char **start, size_t *len, 
bool key)
 }
 
 *unescaped = 0;
-*len = unescaped - *start;
 *start = p;
 return sep;
 }
@@ -219,7 +217,7 @@ static char *keyval_parse_one(QDict *qdict, char *params,
   const char *implied_key, bool *help,
   Error **errp)
 {
-const char *key, *key_end, *s, *end;
+const char *key, *s, *end;
 const char *val = NULL;
 char sep;
 size_t len;
@@ -229,8 +227,8 @@ static char *keyval_parse_one(QDict *qdict, char *params,
 QObject *next;
 
 key = params;
-sep = keyval_fetch_string(¶ms, &len, true);
-if (!len) {
+sep = keyval_fetch_string(¶ms, true);
+if (!*key) {
 if (sep) {
 error_setg(errp, "Expected parameter before '%c%s'", sep, params);
 } else {
@@ -247,13 +245,11 @@ static char *keyval_parse_one(QDict *qdict, char *params,
 /* Desugar implied key */
 val = key;
 key = implied_key;
-len = strlen(implied_key);
 } else {
 error_setg(errp, "No implicit parameter name for value '%s'", key);
 return NULL;
 }
 }
-key_end = key + len;
 
 /*
  * Loop over key fragments: @s points to current fragment, it
@@ -269,24 +265,21 @@ static char *keyval_parse_one(QDict *qdict, char *params,
 ret = parse_qapi_name(s, false);
 len = ret < 0 ? 0 : ret;
 }
-assert(s + len <= key_end);
-if (!len || (s + len < key_end && s[len] != '.')) {
+if (!len || (s[len] != '\0' && s[len] != '.')) {
 assert(key != implied_key);
-error_setg(errp, "Invalid parameter '%.*s'",
-   (int)(key_end - key), key);
+error_setg(errp, "Invalid parameter '%s'", key);
 return NULL;
 }
 if (len >= sizeof(key_in_cur)) {
 assert(key != implied_key);
 error_setg(errp, "Parameter%s '%.*s' is too long",
-   s != key || s + len != key_end ? " fragment" : "",
+   s != key || s[len] == '.' ? " fragment" : "",
(int)len, s);
 return NULL;
 }
 
 if (s != key) {
-next = keyval_parse_put(cur, key_in_cur, NULL,
-key, s - 1, errp);
+next = keyval_parse_put(cur, key_in_cur, NULL, key, s - 1, errp);
 if (!next) {
 return NULL;
 }
@@ -301,9 +294,9 @@ static char *keyval_parse_one(QDict *qdict, char *params,
 
 if (key != implied_key) {
 val = params;
-keyval_fetch_string(¶ms, &len, false);
+keyval_fetch_string(¶ms, false);
 }
-if (!keyval_parse_put(cur, key_in_cur, val, key, key_end, errp)) {
+if (!keyval_parse_put(cur, key_in_cur, val, key, s - 1, errp)) {
 return NULL;
 }
 return params;
-- 
2.26.2

[PATCH 18/28] qom: do not modify QDict argument in user_creatable_add_dict

[Paolo Bonzini]

-object will process its QDicts in two steps, first for the "early" objects and
then for the "late" objects.  If qom-type is removed by the "early" pass, the
late pass fails.  So just create a shallow copy of the QDict in
user_creatable_add_dict.

Signed-off-by: Paolo Bonzini 
---
 include/qom/object_interfaces.h |  2 +-
 qom/object_interfaces.c | 11 +++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index abb23eaea3..ed0d7d663b 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -102,7 +102,7 @@ Object *user_creatable_add_type(const char *type, const 
char *id,
  *
  * Returns: %true on success, %false on failure.
  */
-bool user_creatable_add_dict(QDict *qdict, bool keyval, Error **errp);
+bool user_creatable_add_dict(const QDict *qdict, bool keyval, Error **errp);
 
 /**
  * user_creatable_add_opts:
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index f7dcdf18e2..80814ae7b5 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -106,24 +106,25 @@ out:
 return obj;
 }
 
-bool user_creatable_add_dict(QDict *qdict, bool keyval, Error **errp)
+bool user_creatable_add_dict(const QDict *dict, bool keyval, Error **errp)
 {
 Visitor *v;
-Object *obj;
+Object *obj = NULL;
+QDict *qdict = qdict_clone_shallow(dict);
 g_autofree char *type = NULL;
 g_autofree char *id = NULL;
 
 type = g_strdup(qdict_get_try_str(qdict, "qom-type"));
 if (!type) {
 error_setg(errp, QERR_MISSING_PARAMETER, "qom-type");
-return false;
+goto out;
 }
 qdict_del(qdict, "qom-type");
 
 id = g_strdup(qdict_get_try_str(qdict, "id"));
 if (!id) {
 error_setg(errp, QERR_MISSING_PARAMETER, "id");
-return false;
+goto out;
 }
 qdict_del(qdict, "id");
 
@@ -135,6 +136,8 @@ bool user_creatable_add_dict(QDict *qdict, bool keyval, 
Error **errp)
 obj = user_creatable_add_type(type, id, qdict, v, errp);
 visit_free(v);
 object_unref(obj);
+out:
+qobject_unref(qdict);
 return !!obj;
 }
 
-- 
2.26.2

[PATCH 05/28] qemu-option: warn for short-form boolean options

[Paolo Bonzini]

Options such as "server" or "nowait", that are commonly found in -chardev,
are sugar for "server=on" and "wait=off".  This is quite surprising and
also does not have any notion of typing attached.  It is even possible to
do "-device e1000,noid" and get a device with "id=off".

Deprecate it and print a warning when it is encountered.  In general,
this short form for boolean options only seems to be in wide use for
-chardev and -spice.

Signed-off-by: Paolo Bonzini 
---
 docs/system/deprecated.rst |  6 ++
 tests/test-qemu-opts.c |  2 +-
 util/qemu-option.c | 29 ++---
 3 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 565389697e..ced4fa23a5 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -146,6 +146,12 @@ Drives with interface types other than ``if=none`` are for 
onboard
 devices.  It is possible to use drives the board doesn't pick up with
 -device.  This usage is now deprecated.  Use ``if=none`` instead.
 
+Short-form boolean options (since 5.2)
+''
+
+Boolean options such as ``share=on``/``share=off`` can be written
+in short form as ``share`` and ``noshare``.  This is deprecated
+and will cause a warning.
 
 QEMU Machine Protocol (QMP) commands
 
diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 2aab831d10..8bbb17b1c7 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -515,7 +515,7 @@ static void test_opts_parse(void)
 error_free_or_abort(&err);
 g_assert(!opts);
 
-/* Implied value */
+/* Implied value (qemu_opts_parse warns but accepts it) */
 opts = qemu_opts_parse(&opts_list_03, "an,noaus,noaus=",
false, &error_abort);
 g_assert_cmpuint(opts_count(opts), ==, 3);
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 5f27d4369d..40564a12eb 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -756,10 +756,12 @@ void qemu_opts_print(QemuOpts *opts, const char 
*separator)
 
 static const char *get_opt_name_value(const char *params,
   const char *firstname,
+  bool warn_on_flag,
   bool *help_wanted,
   char **name, char **value)
 {
 const char *p;
+const char *prefix = "";
 size_t len;
 bool is_help = false;
 
@@ -776,10 +778,15 @@ static const char *get_opt_name_value(const char *params,
 if (strncmp(*name, "no", 2) == 0) {
 memmove(*name, *name + 2, strlen(*name + 2) + 1);
 *value = g_strdup("off");
+prefix = "no";
 } else {
 *value = g_strdup("on");
 is_help = is_help_option(*name);
 }
+if (!is_help && warn_on_flag) {
+warn_report("short-form boolean option '%s%s' deprecated", 
prefix, *name);
+error_printf("Please use %s=%s instead\n", *name, *value);
+}
 }
 } else {
 /* found "foo=bar,more" */
@@ -801,14 +808,14 @@ static const char *get_opt_name_value(const char *params,
 
 static bool opts_do_parse(QemuOpts *opts, const char *params,
   const char *firstname, bool prepend,
-  bool *help_wanted, Error **errp)
+  bool warn_on_flag, bool *help_wanted, Error **errp)
 {
 char *option, *value;
 const char *p;
 QemuOpt *opt;
 
 for (p = params; *p;) {
-p = get_opt_name_value(p, firstname, help_wanted, &option, &value);
+p = get_opt_name_value(p, firstname, warn_on_flag, help_wanted, 
&option, &value);
 if (help_wanted && *help_wanted) {
 g_free(option);
 g_free(value);
@@ -839,7 +846,7 @@ static char *opts_parse_id(const char *params)
 char *name, *value;
 
 for (p = params; *p;) {
-p = get_opt_name_value(p, NULL, NULL, &name, &value);
+p = get_opt_name_value(p, NULL, false, NULL, &name, &value);
 if (!strcmp(name, "id")) {
 g_free(name);
 return value;
@@ -858,7 +865,7 @@ bool has_help_option(const char *params)
 bool ret = false;
 
 for (p = params; *p;) {
-p = get_opt_name_value(p, NULL, &ret, &name, &value);
+p = get_opt_name_value(p, NULL, false, &ret, &name, &value);
 g_free(name);
 g_free(value);
 if (ret) {
@@ -878,12 +885,12 @@ bool has_help_option(const char *params)
 bool qemu_opts_do_parse(QemuOpts *opts, const char *params,
const char *firstname, Error **errp)
 {
-return opts_do_parse(opts, params, firstname, false, NULL, errp);
+return opts_do_parse(opts, params, firstname, false, false, NULL, errp);
 }
 
 static QemuOpts *opts_parse(QemuOptsList *list, const char *par

[PATCH 01/28] qemu-option: simplify search for end of key

[Paolo Bonzini]

Use strcspn to find an equal or comma value, and pass the result directly
to get_opt_name to avoid another strchr.

Signed-off-by: Paolo Bonzini 
---
 util/qemu-option.c | 35 +--
 1 file changed, 13 insertions(+), 22 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 25792159ba..6bd654a473 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -38,27 +38,19 @@
 #include "qemu/help_option.h"
 
 /*
- * Extracts the name of an option from the parameter string (p points at the
+ * Extracts the name of an option from the parameter string (@p points at the
  * first byte of the option name)
  *
- * The option name is delimited by delim (usually , or =) or the string end
- * and is copied into option. The caller is responsible for free'ing option
- * when no longer required.
+ * The option name is @len characters long and is copied into @option. The
+ * caller is responsible for free'ing @option when no longer required.
  *
  * The return value is the position of the delimiter/zero byte after the option
- * name in p.
+ * name in @p.
  */
-static const char *get_opt_name(const char *p, char **option, char delim)
+static const char *get_opt_name(const char *p, char **option, size_t len)
 {
-char *offset = strchr(p, delim);
-
-if (offset) {
-*option = g_strndup(p, offset - p);
-return offset;
-} else {
-*option = g_strdup(p);
-return p + strlen(p);
-}
+*option = g_strndup(p, len);
+return p + len;
 }
 
 /*
@@ -766,12 +758,11 @@ static const char *get_opt_name_value(const char *params,
   const char *firstname,
   char **name, char **value)
 {
-const char *p, *pe, *pc;
-
-pe = strchr(params, '=');
-pc = strchr(params, ',');
+const char *p;
+size_t len;
 
-if (!pe || (pc && pc < pe)) {
+len = strcspn(params, "=,");
+if (params[len] != '=') {
 /* found "foo,more" */
 if (firstname) {
 /* implicitly named first option */
@@ -779,7 +770,7 @@ static const char *get_opt_name_value(const char *params,
 p = get_opt_value(params, value);
 } else {
 /* option without value, must be a flag */
-p = get_opt_name(params, name, ',');
+p = get_opt_name(params, name, len);
 if (strncmp(*name, "no", 2) == 0) {
 memmove(*name, *name + 2, strlen(*name + 2) + 1);
 *value = g_strdup("off");
@@ -789,7 +780,7 @@ static const char *get_opt_name_value(const char *params,
 }
 } else {
 /* found "foo=bar,more" */
-p = get_opt_name(params, name, '=');
+p = get_opt_name(params, name, len);
 assert(*p == '=');
 p++;
 p = get_opt_value(p, value);
-- 
2.26.2

[PATCH 03/28] qemu-option: clean up id vs. list->merge_lists

[Paolo Bonzini]

Looking at all merge-lists QemuOptsList, here is how they access their
QemuOpts:

reopen_opts in qemu-io-cmds.c ("qemu-img reopen -o")
qemu_opts_find(&reopen_opts, NULL)

empty_opts in qemu-io.c ("qemu-io open -o")
qemu_opts_find(&empty_opts, NULL)

qemu_rtc_opts ("-rtc")
qemu_find_opts_singleton("rtc")

qemu_machine_opts ("-M")
qemu_find_opts_singleton("machine")

qemu_boot_opts ("-boot")
QTAILQ_FIRST(&qemu_find_opts("bootopts")->head)

qemu_name_opts ("-name")
qemu_opts_foreach->parse_name
parse_name does not use id

qemu_mem_opts ("-m")
qemu_find_opts_singleton("memory")

qemu_icount_opts ("-icount")
qemu_opts_foreach->do_configuree_icount
do_configure_icount->icount_configure
icount_configure does not use id

qemu_smp_opts ("-smp")
qemu_opts_find(qemu_find_opts("smp-opts"), NULL)

qemu_spice_opts ("-spice")
QTAILQ_FIRST(&qemu_spice_opts.head)

i.e. they don't need an id.  Sometimes its presence is ignored
(e.g. when using qemu_opts_foreach), sometimes all the options
with the id are skipped, sometimes only the first option on the
command line is considered.  With this patch we just forbid id
on merge-lists QemuOptsLists; if the command line still works,
it has the same semantics as before.

qemu_opts_create's fail_if_exists parameter is now unnecessary:

- it is unused if id is NULL

- opts_parse only passes false if reached from qemu_opts_set_defaults,
in which case this patch enforces that id must be NULL

- other callers that can pass a non-NULL id always set it to true

Assert that it is true in the only case where "fail_if_exists" matters,
i.e. "id && !lists->merge_lists".  This means that if an id is present,
duplicates are always forbidden, which was already the status quo.

Discounting the case that aborts as it's not user-controlled (it's
"just" a matter of inspecting qemu_opts_create callers), the paths
through qemu_opts_create can be summarized as:

- merge_lists = true: singleton opts with NULL id; non-NULL id fails

- merge_lists = false: always return new opts; non-NULL id fails if dup

Signed-off-by: Paolo Bonzini 
---
 util/qemu-option.c | 27 ++-
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index c88e159f18..91f4120ce1 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -619,7 +619,17 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char 
*id,
 {
 QemuOpts *opts = NULL;
 
-if (id) {
+if (list->merge_lists) {
+if (id) {
+error_setg(errp, QERR_INVALID_PARAMETER, "id");
+return NULL;
+}
+opts = qemu_opts_find(list, NULL);
+if (opts) {
+return opts;
+}
+} else if (id) {
+assert(fail_if_exists);
 if (!id_wellformed(id)) {
 error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
"an identifier");
@@ -629,17 +639,8 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char 
*id,
 }
 opts = qemu_opts_find(list, id);
 if (opts != NULL) {
-if (fail_if_exists && !list->merge_lists) {
-error_setg(errp, "Duplicate ID '%s' for %s", id, list->name);
-return NULL;
-} else {
-return opts;
-}
-}
-} else if (list->merge_lists) {
-opts = qemu_opts_find(list, NULL);
-if (opts) {
-return opts;
+error_setg(errp, "Duplicate ID '%s' for %s", id, list->name);
+return NULL;
 }
 }
 opts = g_malloc0(sizeof(*opts));
@@ -893,7 +894,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char 
*params,
  * (if unlikely) future misuse:
  */
 assert(!defaults || list->merge_lists);
-opts = qemu_opts_create(list, id, !defaults, errp);
+opts = qemu_opts_create(list, id, !list->merge_lists, errp);
 g_free(id);
 if (opts == NULL) {
 return NULL;
-- 
2.26.2

[PATCH 25/28] vl: rename local variable in configure_accelerators

[Paolo Bonzini]

Silly patch extracted from the next one, which is already big enough.

Because there are already local variables named "accel", we will name
the global vl.c variable for "-M accel" accelerators instead.  Rename
it already in configure_accelerators to be ready.

Signed-off-by: Paolo Bonzini 
---
 softmmu/vl.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index f083881f21..a942ce2004 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2251,17 +2251,17 @@ static int do_configure_accelerator(void *opaque, 
QemuOpts *opts, Error **errp)
 
 static void configure_accelerators(const char *progname)
 {
-const char *accel;
+const char *accelerators;
 bool init_failed = false;
 
 qemu_opts_foreach(qemu_find_opts("icount"),
   do_configure_icount, NULL, &error_fatal);
 
-accel = qemu_opt_get(qemu_get_machine_opts(), "accel");
+accelerators = qemu_opt_get(qemu_get_machine_opts(), "accel");
 if (QTAILQ_EMPTY(&qemu_accel_opts.head)) {
 char **accel_list, **tmp;
 
-if (accel == NULL) {
+if (accelerators == NULL) {
 /* Select the default accelerator */
 bool have_tcg = accel_find("tcg");
 bool have_kvm = accel_find("kvm");
@@ -2269,21 +2269,21 @@ static void configure_accelerators(const char *progname)
 if (have_tcg && have_kvm) {
 if (g_str_has_suffix(progname, "kvm")) {
 /* If the program name ends with "kvm", we prefer KVM */
-accel = "kvm:tcg";
+accelerators = "kvm:tcg";
 } else {
-accel = "tcg:kvm";
+accelerators = "tcg:kvm";
 }
 } else if (have_kvm) {
-accel = "kvm";
+accelerators = "kvm";
 } else if (have_tcg) {
-accel = "tcg";
+accelerators = "tcg";
 } else {
 error_report("No accelerator selected and"
  " no default accelerator available");
 exit(1);
 }
 }
-accel_list = g_strsplit(accel, ":", 0);
+accel_list = g_strsplit(accelerators, ":", 0);
 
 for (tmp = accel_list; *tmp; tmp++) {
 /*
@@ -2299,7 +2299,7 @@ static void configure_accelerators(const char *progname)
 }
 g_strfreev(accel_list);
 } else {
-if (accel != NULL) {
+if (accelerators != NULL) {
 error_report("The -accel and \"-machine accel=\" options are 
incompatible");
 exit(1);
 }
-- 
2.26.2

[PATCH 08/28] tests: convert check-qom-proplist to keyval

[Paolo Bonzini]

The command-line creation test is using QemuOpts.  Switch it to keyval,
since all the -object command line options will follow
qemu-storage-daemon and do the same.

Signed-off-by: Paolo Bonzini 
---
 tests/check-qom-proplist.c | 58 +-
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index 1b76581980..8dba26fb3c 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -21,6 +21,8 @@
 #include "qemu/osdep.h"
 
 #include "qapi/error.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/qmp/qobject.h"
 #include "qom/object.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
@@ -400,42 +402,58 @@ static void test_dummy_createlist(void)
 
 static void test_dummy_createcmdl(void)
 {
-QemuOpts *opts;
+QDict *qdict;
 DummyObject *dobj;
 Error *err = NULL;
+bool help;
 const char *params = TYPE_DUMMY \
  ",id=dev0," \
  "bv=yes,sv=Hiss hiss hiss,av=platypus";
 
-qemu_add_opts(&qemu_object_opts);
-opts = qemu_opts_parse(&qemu_object_opts, params, true, &err);
+qdict = keyval_parse(params, "qom-type", &help, &err);
 g_assert(err == NULL);
-g_assert(opts);
+g_assert(qdict);
+g_assert(!help);
 
-dobj = DUMMY_OBJECT(user_creatable_add_opts(opts, &err));
+g_assert(user_creatable_add_dict(qdict, true, &err));
 g_assert(err == NULL);
+qobject_unref(qdict);
+
+dobj = 
DUMMY_OBJECT(object_resolve_path_component(object_get_objects_root(),
+  "dev0"));
 g_assert(dobj);
 g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
 g_assert(dobj->bv == true);
 g_assert(dobj->av == DUMMY_PLATYPUS);
 
+qdict = keyval_parse(params, "qom-type", &help, &err);
+g_assert(!user_creatable_add_dict(qdict, true, &err));
+g_assert(err);
+g_assert(object_resolve_path_component(object_get_objects_root(), "dev0")
+ == OBJECT(dobj));
+qobject_unref(qdict);
+error_free(err);
+err = NULL;
+
+qdict = keyval_parse(params, "qom-type", &help, &err);
 user_creatable_del("dev0", &error_abort);
+g_assert(object_resolve_path_component(object_get_objects_root(), "dev0")
+ == NULL);
 
-object_unref(OBJECT(dobj));
-
-/*
- * cmdline-parsing via qemu_opts_parse() results in a QemuOpts entry
- * corresponding to the Object's ID to be added to the QemuOptsList
- * for objects. To avoid having this entry conflict with future
- * Objects using the same ID (which can happen in cases where
- * qemu_opts_parse() is used to parse the object params, such as
- * with hmp_object_add() at the time of this comment), we need to
- * check for this in user_creatable_del() and remove the QemuOpts if
- * it is present.
- *
- * The below check ensures this works as expected.
- */
-g_assert_null(qemu_opts_find(&qemu_object_opts, "dev0"));
+g_assert(user_creatable_add_dict(qdict, true, &err));
+g_assert(err == NULL);
+qobject_unref(qdict);
+
+dobj = 
DUMMY_OBJECT(object_resolve_path_component(object_get_objects_root(),
+  "dev0"));
+g_assert(dobj);
+g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
+g_assert(dobj->bv == true);
+g_assert(dobj->av == DUMMY_PLATYPUS);
+g_assert(object_resolve_path_component(object_get_objects_root(), "dev0")
+ == OBJECT(dobj));
+
+object_unparent(OBJECT(dobj));
 }
 
 static void test_dummy_badenum(void)
-- 
2.26.2