Re: [Qemu-block] [PATCH v0 2/2] qmp: add block-set-copy-on-read command

[Alberto Garcia]

On Thu 14 Jun 2018 11:00:55 AM CEST, Kevin Wolf  wrote:
> Am 13.06.2018 um 18:41 hat Max Reitz geschrieben:
>> On 2018-06-13 18:02, Eric Blake wrote:
>> > On 06/13/2018 10:47 AM, Denis Plotnikov wrote:
>> >> The command enables/disables copy-on-read mode for VM's disk while
>> >> VM is running.
>> >>
>> >> This is needed when using external disk readers to shape access pattern
>> >> to the disk backend.
>> >>
>> >> Signed-off-by: Denis Plotnikov 
>> >> ---
>> > 
>> > Deferring thoughts on the actual design for later;
>> 
>> But why? ;-)
>> 
>> This patch would definitely be superseded by a block reconfiguration
>> command (i.e. presumably one that makes reopen accessible over QMP).
>> With such a command, you could insert or remove a copy-on-read filter
>> node at any point in time.
>> 
>> Since we definitely want block graph configuration, I don't think we
>> want to add special commands now.
>
> I agree, and it seems that we get more and more use cases for a block
> reconfiguration commands. Only yesterday we talked about the "true"
> blockdev way for libvirt to implement I/O throttling. The result was
> that without reopen, we still need to use the old QMP command to set
> BlockBackend-based I/O throttling instead of using a filter node.
>
> So I'm eagerly awaiting Berto's promised patches for it.

They're coming, it's just a bit tricker than I initially though. I'll
probably send the first series as an RFC if I don't manage to fix all
problems, so we can discuss them a bit.

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Thu, Jun 14, 2018 at 10:40:58AM +0200, Kevin Wolf wrote:
>> Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
>> > Kevin Wolf  writes:
>> > 
>> > > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
>> > >> When you mix scalar and non-scalar keys, whether you get an "already
>> > >> set as scalar" or an "already set as dict" error depends on qdict
>> > >> iteration order.  Neither message makes much sense.  Replace by
>> > >> ""Cannot mix scalar and non-scalar keys".  This is similar to the
>> > >> message we get for mixing list and non-list keys.
>> > >> 
>> > >> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
>> > >> and add a comment.
>> > >> 
>> > >> Signed-off-by: Markus Armbruster 
>> > >
>> > > To be honest, I found the old version of the loop more obvious.
>> > >
>> > >>  qobject/block-qdict.c | 42 +-
>> > >>  1 file changed, 21 insertions(+), 21 deletions(-)
>> > >> 
>> > >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
>> > >> index a4e1c8d08f..35e9052816 100644
>> > >> --- a/qobject/block-qdict.c
>> > >> +++ b/qobject/block-qdict.c
>> > >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error 
>> > >> **errp)
>> > >>  QObject *qdict_crumple(const QDict *src, Error **errp)
>> > >>  {
>> > >>  const QDictEntry *ent;
>> > >> -QDict *two_level, *multi_level = NULL;
>> > >> +QDict *two_level, *multi_level = NULL, *child_dict;
>> > >>  QObject *dst = NULL, *child;
>> > >>  size_t i;
>> > >>  char *prefix = NULL;
>> > >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error 
>> > >> **errp)
>> > >>  }
>> > >>  
>> > >>  qdict_split_flat_key(ent->key, , );
>> > >> -
>> > >>  child = qdict_get(two_level, prefix);
>> > >> +child_dict = qobject_to(QDict, child);
>> > >> +
>> > >> +if (child) {
>> > >> +/*
>> > >> + * An existing child must be a dict and @ent must be a
>> > >> + * dict member (i.e. suffix not null), or else @ent
>> > >> + * clashes.
>> > >> + */
>> > >> +if (!child_dict || !suffix) {
>> > >> +error_setg(errp,
>> > >> +   "Cannot mix scalar and non-scalar keys");
>> > >> +goto error;
>> > >> +}
>> > >> +} else if (suffix) {
>> > >> +child_dict = qdict_new();
>> > >> +qdict_put(two_level, prefix, child_dict);
>> > >> +} else {
>> > >> +qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
>> > >> +}
>> > >
>> > > At least, can you please move the else branch to the if below so that
>> > > the addition of the new entry is symmetrical for both scalars and dicts?
>> > > As the code is, it mixes the conflict check, creation of the child dict
>> > > and addition of scalars (but not to the child dict) in a weird way in a
>> > > single if block.
>> > >
>> > > Or maybe organise it like this:
>> > >
>> > > if (child && !(child_dict && suffix)) {
>> > > error
>> > > }
>> > >
>> > > if (suffix) {
>> > > if (!child_dict) {
>> > > create it
>> > > add it to two_level
>> > > }
>> > > add entry to child_dict
>> > > } else {
>> > > add entry to two_level
>> > > }
>> > 
>> > Fleshing out...
>> > 
>> > if (child && !child_dict) {
>> > /*
>> >  * @prefix already exists and it's a non-dictionary,
>> >  * i.e. we've seen a scalar with key @prefix.  The same
>> >  * key can't occur twice, therefore suffix must be
>> >  * non-null.
>> >  */
>> > assert(suffix);
>> > /*
>> >  * @ent has key @prefix.@suffix, but we've already seen
>> >  * key @prefix: clash.
>> >  */
>> > error_setg(errp, "Cannot mix scalar and non-scalar keys");
>> > goto error;
>> > }
>> 
>> This catches "foo.bar" after "foo", but not "foo" after "foo.bar".
>> 
>> > if (suffix) {
>> > if (!child_dict) {
>> > child_dict = qdict_new();
>> > qdict_put(two_level, prefix, child_dict);
>> > }
>> > qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
>> > } else {
>> > assert(!child);
>> 
>> So "foo" after "foo.bar" would fail this assertion.
>
> We got coverage of bad inputs in tests/check-qdict.c
>
> If the qdict_crumple_test_bad_inputs doesn't already cover these scenarios
> we should extend it to make sure we detect them during testing

Depends on qdict_first() / qdict_next() traversal order, which makes it
awkward.

Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/4] block: Remove deprecated -drive geometry options

[Markus Armbruster]

Kevin Wolf  writes:

> The -drive options cyls, heads, secs and trans were deprecated in
> QEMU 2.10. It's time to remove them.
>
> hd-geo-test tested both the old version with geometry options in -drive
> and the new one with -device. Therefore the code using -drive doesn't
> have to be replaced there, we just need to remove the -drive test cases.
> This in turn allows some simplification of the code.
>
> Signed-off-by: Kevin Wolf 

Reviewed-by: Markus Armbruster 

Re: [Qemu-block] [PATCH v0 2/2] qmp: add block-set-copy-on-read command

[Kevin Wolf]

Am 13.06.2018 um 18:41 hat Max Reitz geschrieben:
> On 2018-06-13 18:02, Eric Blake wrote:
> > On 06/13/2018 10:47 AM, Denis Plotnikov wrote:
> >> The command enables/disables copy-on-read mode for VM's disk while
> >> VM is running.
> >>
> >> This is needed when using external disk readers to shape access pattern
> >> to the disk backend.
> >>
> >> Signed-off-by: Denis Plotnikov 
> >> ---
> > 
> > Deferring thoughts on the actual design for later;
> 
> But why? ;-)
> 
> This patch would definitely be superseded by a block reconfiguration
> command (i.e. presumably one that makes reopen accessible over QMP).
> With such a command, you could insert or remove a copy-on-read filter
> node at any point in time.
> 
> Since we definitely want block graph configuration, I don't think we
> want to add special commands now.

I agree, and it seems that we get more and more use cases for a block
reconfiguration commands. Only yesterday we talked about the "true"
blockdev way for libvirt to implement I/O throttling. The result was
that without reopen, we still need to use the old QMP command to set
BlockBackend-based I/O throttling instead of using a filter node.

So I'm eagerly awaiting Berto's promised patches for it.

Kevin


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH 2/3] qcow2: refactor data compression

[Kevin Wolf]

Am 08.06.2018 um 21:20 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Make a separate function for compression to be parallelized later.
>  - use .avail_aut field instead of .next_out to calculate size of

s/avail_aut/avail_out/

>compressed data. It looks more natural and it allows to keep dest to
>be void pointer
>  - set avail_out to be at least one byte less than input, to be sure
>avoid inefficient compression earlier
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

>  block/qcow2.c | 74 
> +--
>  1 file changed, 47 insertions(+), 27 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 549fee9b69..d4dbe329ab 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -22,11 +22,13 @@
>   * THE SOFTWARE.
>   */
>  
> +#define ZLIB_CONST
> +#include 

The first #include must always be "qemu/osdep.h". If you want to
separate zlib.h from the internal headers, you can move it down instead.

>  #include "qemu/osdep.h"
>  #include "block/block_int.h"
>  #include "sysemu/block-backend.h"
>  #include "qemu/module.h"
> -#include 
>  #include "qcow2.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
> @@ -3674,6 +3676,45 @@ static int qcow2_truncate(BlockDriverState *bs, 
> int64_t offset,
>  return 0;
>  }
>  
> +/* qcow2_compress()

The first line of the comment should contain only the /*

> + *
> + * @dest - destination buffer, at least of @size-1 bytes
> + * @src - source buffer, @size bytes
> + *
> + * Returns: compressed size on success
> + *  -1 if compression is inefficient
> + *  -2 on any other error
> + */

The logic looks fine.

Initially I intended to request splitting the code motion from the
changes, but I see that this would probably only make things more
complicated, so I'm okay with leaving that as it is.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/4] block: Remove deprecated -drive option serial

[Richard W.M. Jones]

On Thu, Jun 14, 2018 at 11:55:43AM +0200, Kevin Wolf wrote:
> The -drive option serial was deprecated in QEMU 2.10. It's time to
> remove it.
> 
> Tests need to be updated to set the serial number with -global instead
> of using the -drive option.

Libguestfs uses this option to set the disk serial/label.  What is
the alternate syntax?

Rich.

> Signed-off-by: Kevin Wolf 
> Reviewed-by: Markus Armbruster 
> Reviewed-by: Jeff Cody 
> Signed-off-by: Kevin Wolf 
> ---
>  include/hw/block/block.h  |  1 -
>  include/sysemu/blockdev.h |  1 -
>  block/block-backend.c |  1 -
>  blockdev.c| 10 --
>  hw/block/block.c  | 13 -
>  hw/block/nvme.c   |  1 -
>  hw/block/virtio-blk.c |  1 -
>  hw/ide/qdev.c |  1 -
>  hw/scsi/scsi-disk.c   |  1 -
>  hw/usb/dev-storage.c  |  1 -
>  tests/ahci-test.c |  6 +++---
>  tests/ide-test.c  |  8 
>  qemu-doc.texi |  5 -
>  qemu-options.hx   |  6 +-
>  14 files changed, 8 insertions(+), 48 deletions(-)
> 
> diff --git a/include/hw/block/block.h b/include/hw/block/block.h
> index d4f4dfffab..e9f9e2223f 100644
> --- a/include/hw/block/block.h
> +++ b/include/hw/block/block.h
> @@ -72,7 +72,6 @@ static inline unsigned int get_physical_block_exp(BlockConf 
> *conf)
>  
>  /* Configuration helpers */
>  
> -void blkconf_serial(BlockConf *conf, char **serial);
>  bool blkconf_geometry(BlockConf *conf, int *trans,
>unsigned cyls_max, unsigned heads_max, unsigned 
> secs_max,
>Error **errp);
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index c0ae3700ec..24954b94e0 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -35,7 +35,6 @@ struct DriveInfo {
>  bool is_default;/* Added by default_drive() ?  */
>  int media_cd;
>  QemuOpts *opts;
> -char *serial;
>  QTAILQ_ENTRY(DriveInfo) next;
>  };
>  
> diff --git a/block/block-backend.c b/block/block-backend.c
> index d55c328736..2d1a3463e8 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -419,7 +419,6 @@ static void drive_info_del(DriveInfo *dinfo)
>  return;
>  }
>  qemu_opts_del(dinfo->opts);
> -g_free(dinfo->serial);
>  g_free(dinfo);
>  }
>  
> diff --git a/blockdev.c b/blockdev.c
> index 83b3cc12e9..695e10d398 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -730,10 +730,6 @@ QemuOptsList qemu_legacy_drive_opts = {
>  .type = QEMU_OPT_STRING,
>  .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
>  },{
> -.name = "serial",
> -.type = QEMU_OPT_STRING,
> -.help = "disk serial number",
> -},{
>  .name = "file",
>  .type = QEMU_OPT_STRING,
>  .help = "file name",
> @@ -775,12 +771,10 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>  const char *werror, *rerror;
>  bool read_only = false;
>  bool copy_on_read;
> -const char *serial;
>  const char *filename;
>  Error *local_err = NULL;
>  int i;
>  const char *deprecated[] = {
> -"serial"
>  };
>  
>  /* Change legacy command line options into QMP ones */
> @@ -948,9 +942,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>  goto fail;
>  }
>  
> -/* Serial number */
> -serial = qemu_opt_get(legacy_opts, "serial");
> -
>  /* no id supplied -> create one */
>  if (qemu_opts_id(all_opts) == NULL) {
>  char *new_id;
> @@ -1025,7 +1016,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
> BlockInterfaceType block_default_type)
>  dinfo->type = type;
>  dinfo->bus = bus_id;
>  dinfo->unit = unit_id;
> -dinfo->serial = g_strdup(serial);
>  
>  blk_set_legacy_dinfo(blk, dinfo);
>  
> diff --git a/hw/block/block.c b/hw/block/block.c
> index b6c80ab0b7..cf0eb826f1 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -15,19 +15,6 @@
>  #include "qapi/qapi-types-block.h"
>  #include "qemu/error-report.h"
>  
> -void blkconf_serial(BlockConf *conf, char **serial)
> -{
> -DriveInfo *dinfo;
> -
> -if (!*serial) {
> -/* try to fall back to value set with legacy -drive serial=... */
> -dinfo = blk_legacy_dinfo(conf->blk);
> -if (dinfo) {
> -*serial = g_strdup(dinfo->serial);
> -}
> -}
> -}
> -
>  void blkconf_blocksizes(BlockConf *conf)
>  {
>  BlockBackend *blk = conf->blk;
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 811084b6a7..d5bf95b79b 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -1215,7 +1215,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error 
> **errp)
>  return;
>  }
>  
> -blkconf_serial(>conf, >serial);
>  if (!n->serial) {
>  error_setg(errp, "serial property not set");

Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/4] block: Remove dead deprecation warning code

[Markus Armbruster]

Kevin Wolf  writes:

> We removed all options from the 'deprecated' array, so the code is dead
> and can be removed as well.
>
> Signed-off-by: Kevin Wolf 

Reviewed-by: Markus Armbruster 

Re: [Qemu-block] [PATCH 3/3] qcow2: add compress threads

[Denis V. Lunev]

On 06/14/2018 04:16 PM, Kevin Wolf wrote:
> Am 08.06.2018 um 21:20 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Do data compression in separate threads. This significantly improve
>> performance for qemu-img convert with -W (allow async writes) and -c
>> (compressed) options.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> Looks correct to me, but why do we introduce a separate
> MAX_COMPRESS_THREADS? Can't we simply leave the maximum number of
> threads to the thread poll?
>
> I see that you chose a much smaller number here (4 vs. 64), but is there
> actually a good reason for this?
>
> Kevin
yes. In the other case the guest will suffer much more from this increased
activity and load on the host.

Den

Re: [Qemu-block] [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit

[Daniel P . Berrangé]

On Thu, Jun 14, 2018 at 10:40:58AM +0200, Kevin Wolf wrote:
> Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
> > Kevin Wolf  writes:
> > 
> > > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
> > >> When you mix scalar and non-scalar keys, whether you get an "already
> > >> set as scalar" or an "already set as dict" error depends on qdict
> > >> iteration order.  Neither message makes much sense.  Replace by
> > >> ""Cannot mix scalar and non-scalar keys".  This is similar to the
> > >> message we get for mixing list and non-list keys.
> > >> 
> > >> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
> > >> and add a comment.
> > >> 
> > >> Signed-off-by: Markus Armbruster 
> > >
> > > To be honest, I found the old version of the loop more obvious.
> > >
> > >>  qobject/block-qdict.c | 42 +-
> > >>  1 file changed, 21 insertions(+), 21 deletions(-)
> > >> 
> > >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> > >> index a4e1c8d08f..35e9052816 100644
> > >> --- a/qobject/block-qdict.c
> > >> +++ b/qobject/block-qdict.c
> > >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error 
> > >> **errp)
> > >>  QObject *qdict_crumple(const QDict *src, Error **errp)
> > >>  {
> > >>  const QDictEntry *ent;
> > >> -QDict *two_level, *multi_level = NULL;
> > >> +QDict *two_level, *multi_level = NULL, *child_dict;
> > >>  QObject *dst = NULL, *child;
> > >>  size_t i;
> > >>  char *prefix = NULL;
> > >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error 
> > >> **errp)
> > >>  }
> > >>  
> > >>  qdict_split_flat_key(ent->key, , );
> > >> -
> > >>  child = qdict_get(two_level, prefix);
> > >> +child_dict = qobject_to(QDict, child);
> > >> +
> > >> +if (child) {
> > >> +/*
> > >> + * An existing child must be a dict and @ent must be a
> > >> + * dict member (i.e. suffix not null), or else @ent
> > >> + * clashes.
> > >> + */
> > >> +if (!child_dict || !suffix) {
> > >> +error_setg(errp,
> > >> +   "Cannot mix scalar and non-scalar keys");
> > >> +goto error;
> > >> +}
> > >> +} else if (suffix) {
> > >> +child_dict = qdict_new();
> > >> +qdict_put(two_level, prefix, child_dict);
> > >> +} else {
> > >> +qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
> > >> +}
> > >
> > > At least, can you please move the else branch to the if below so that
> > > the addition of the new entry is symmetrical for both scalars and dicts?
> > > As the code is, it mixes the conflict check, creation of the child dict
> > > and addition of scalars (but not to the child dict) in a weird way in a
> > > single if block.
> > >
> > > Or maybe organise it like this:
> > >
> > > if (child && !(child_dict && suffix)) {
> > > error
> > > }
> > >
> > > if (suffix) {
> > > if (!child_dict) {
> > > create it
> > > add it to two_level
> > > }
> > > add entry to child_dict
> > > } else {
> > > add entry to two_level
> > > }
> > 
> > Fleshing out...
> > 
> > if (child && !child_dict) {
> > /*
> >  * @prefix already exists and it's a non-dictionary,
> >  * i.e. we've seen a scalar with key @prefix.  The same
> >  * key can't occur twice, therefore suffix must be
> >  * non-null.
> >  */
> > assert(suffix);
> > /*
> >  * @ent has key @prefix.@suffix, but we've already seen
> >  * key @prefix: clash.
> >  */
> > error_setg(errp, "Cannot mix scalar and non-scalar keys");
> > goto error;
> > }
> 
> This catches "foo.bar" after "foo", but not "foo" after "foo.bar".
> 
> > if (suffix) {
> > if (!child_dict) {
> > child_dict = qdict_new();
> > qdict_put(two_level, prefix, child_dict);
> > }
> > qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
> > } else {
> > assert(!child);
> 
> So "foo" after "foo.bar" would fail this assertion.

We got coverage of bad inputs in tests/check-qdict.c

If the qdict_crumple_test_bad_inputs doesn't already cover these scenarios
we should extend it to make sure we detect them during testing


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

[Qemu-block] [PATCH v2 2/4] block: Remove deprecated -drive option addr

[Kevin Wolf]

The -drive option addr was deprecated in QEMU 2.10. It's time to remove
it.

Signed-off-by: Kevin Wolf 
Reviewed-by: Markus Armbruster 
Reviewed-by: Jeff Cody 
---
 include/sysemu/blockdev.h |  1 -
 blockdev.c| 17 +
 device-hotplug.c  |  4 
 qemu-doc.texi |  5 -
 qemu-options.hx   |  5 +
 5 files changed, 2 insertions(+), 30 deletions(-)

diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 37ea39719e..c0ae3700ec 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -28,7 +28,6 @@ typedef enum {
 } BlockInterfaceType;
 
 struct DriveInfo {
-const char *devaddr;
 BlockInterfaceType type;
 int bus;
 int unit;
diff --git a/blockdev.c b/blockdev.c
index 9c891706ef..83b3cc12e9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -730,10 +730,6 @@ QemuOptsList qemu_legacy_drive_opts = {
 .type = QEMU_OPT_STRING,
 .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
 },{
-.name = "addr",
-.type = QEMU_OPT_STRING,
-.help = "pci address (virtio only)",
-},{
 .name = "serial",
 .type = QEMU_OPT_STRING,
 .help = "disk serial number",
@@ -776,7 +772,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType 
block_default_type)
 DriveMediaType media = MEDIA_DISK;
 BlockInterfaceType type;
 int max_devs, bus_id, unit_id, index;
-const char *devaddr;
 const char *werror, *rerror;
 bool read_only = false;
 bool copy_on_read;
@@ -785,7 +780,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType 
block_default_type)
 Error *local_err = NULL;
 int i;
 const char *deprecated[] = {
-"serial", "addr"
+"serial"
 };
 
 /* Change legacy command line options into QMP ones */
@@ -975,12 +970,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type)
 }
 
 /* Add virtio block device */
-devaddr = qemu_opt_get(legacy_opts, "addr");
-if (devaddr && type != IF_VIRTIO) {
-error_report("addr is not supported by this bus type");
-goto fail;
-}
-
 if (type == IF_VIRTIO) {
 QemuOpts *devopts;
 devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
@@ -992,9 +981,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType 
block_default_type)
 }
 qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
  _abort);
-if (devaddr) {
-qemu_opt_set(devopts, "addr", devaddr, _abort);
-}
 }
 
 filename = qemu_opt_get(legacy_opts, "file");
@@ -1039,7 +1025,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type)
 dinfo->type = type;
 dinfo->bus = bus_id;
 dinfo->unit = unit_id;
-dinfo->devaddr = devaddr;
 dinfo->serial = g_strdup(serial);
 
 blk_set_legacy_dinfo(blk, dinfo);
diff --git a/device-hotplug.c b/device-hotplug.c
index 23fd6656f1..cd427e2c76 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -69,10 +69,6 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
 if (!dinfo) {
 goto err;
 }
-if (dinfo->devaddr) {
-monitor_printf(mon, "Parameter addr not supported\n");
-goto err;
-}
 
 switch (dinfo->type) {
 case IF_NONE:
diff --git a/qemu-doc.texi b/qemu-doc.texi
index ab95bffc74..338477725f 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2855,11 +2855,6 @@ provided per NIC.
 The drive serial argument is replaced by the the serial argument
 that can be specified with the ``-device'' parameter.
 
-@subsection -drive addr=... (since 2.10.0)
-
-The drive addr argument is replaced by the the addr argument
-that can be specified with the ``-device'' parameter.
-
 @subsection -usbdevice (since 2.10.0)
 
 The ``-usbdevice DEV'' argument is now a synonym for setting
diff --git a/qemu-options.hx b/qemu-options.hx
index a14b9655c5..c2531e2f3c 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -805,7 +805,7 @@ ETEXI
 DEF("drive", HAS_ARG, QEMU_OPTION_drive,
 "-drive [file=file][,if=type][,bus=n][,unit=m][,media=d][,index=i]\n"
 "   
[,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
-"   
[,snapshot=on|off][,serial=s][,addr=A][,rerror=ignore|stop|report]\n"
+"   [,snapshot=on|off][,serial=s][,rerror=ignore|stop|report]\n"
 "   
[,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
 "   [,readonly=on|off][,copy-on-read=on|off]\n"
 "   [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
@@ -883,9 +883,6 @@ an untrusted format header.
 This option specifies the serial number to assign to the device. This
 parameter is deprecated, use the corresponding parameter of @code{-device}
 instead.
-@item addr=@var{addr}
-Specify the controller's PCI address (if=virtio only). This 

[Qemu-block] [PATCH v2 4/4] block: Remove dead deprecation warning code

[Kevin Wolf]

We removed all options from the 'deprecated' array, so the code is dead
and can be removed as well.

Signed-off-by: Kevin Wolf 
---
 blockdev.c | 12 
 1 file changed, 12 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 695e10d398..4ab3d6ec0b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -774,8 +774,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType 
block_default_type)
 const char *filename;
 Error *local_err = NULL;
 int i;
-const char *deprecated[] = {
-};
 
 /* Change legacy command line options into QMP ones */
 static const struct {
@@ -852,16 +850,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type)
 goto fail;
 }
 
-/* Other deprecated options */
-if (!qtest_enabled()) {
-for (i = 0; i < ARRAY_SIZE(deprecated); i++) {
-if (qemu_opt_get(legacy_opts, deprecated[i]) != NULL) {
-error_report("'%s' is deprecated, please use the corresponding 
"
- "option of '-device' instead", deprecated[i]);
-}
-}
-}
-
 /* Media type */
 value = qemu_opt_get(legacy_opts, "media");
 if (value) {
-- 
2.13.6

[Qemu-block] [PATCH v2 1/4] block: Remove deprecated -drive geometry options

[Kevin Wolf]

The -drive options cyls, heads, secs and trans were deprecated in
QEMU 2.10. It's time to remove them.

hd-geo-test tested both the old version with geometry options in -drive
and the new one with -device. Therefore the code using -drive doesn't
have to be replaced there, we just need to remove the -drive test cases.
This in turn allows some simplification of the code.

Signed-off-by: Kevin Wolf 
---
 include/sysemu/blockdev.h |  1 -
 blockdev.c| 75 +--
 hw/block/block.c  | 14 -
 tests/hd-geo-test.c   | 37 +--
 hmp-commands.hx   |  1 -
 qemu-doc.texi |  5 
 qemu-options.hx   |  7 +
 7 files changed, 9 insertions(+), 131 deletions(-)

diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index ac22f2ae1f..37ea39719e 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -35,7 +35,6 @@ struct DriveInfo {
 int auto_del;   /* see blockdev_mark_auto_del() */
 bool is_default;/* Added by default_drive() ?  */
 int media_cd;
-int cyls, heads, secs, trans;
 QemuOpts *opts;
 char *serial;
 QTAILQ_ENTRY(DriveInfo) next;
diff --git a/blockdev.c b/blockdev.c
index 4862323012..9c891706ef 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -730,22 +730,6 @@ QemuOptsList qemu_legacy_drive_opts = {
 .type = QEMU_OPT_STRING,
 .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
 },{
-.name = "cyls",
-.type = QEMU_OPT_NUMBER,
-.help = "number of cylinders (ide disk geometry)",
-},{
-.name = "heads",
-.type = QEMU_OPT_NUMBER,
-.help = "number of heads (ide disk geometry)",
-},{
-.name = "secs",
-.type = QEMU_OPT_NUMBER,
-.help = "number of sectors (ide disk geometry)",
-},{
-.name = "trans",
-.type = QEMU_OPT_STRING,
-.help = "chs translation (auto, lba, none)",
-},{
 .name = "addr",
 .type = QEMU_OPT_STRING,
 .help = "pci address (virtio only)",
@@ -791,7 +775,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType 
block_default_type)
 QemuOpts *legacy_opts;
 DriveMediaType media = MEDIA_DISK;
 BlockInterfaceType type;
-int cyls, heads, secs, translation;
 int max_devs, bus_id, unit_id, index;
 const char *devaddr;
 const char *werror, *rerror;
@@ -802,7 +785,7 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType 
block_default_type)
 Error *local_err = NULL;
 int i;
 const char *deprecated[] = {
-"serial", "trans", "secs", "heads", "cyls", "addr"
+"serial", "addr"
 };
 
 /* Change legacy command line options into QMP ones */
@@ -931,57 +914,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type)
 type = block_default_type;
 }
 
-/* Geometry */
-cyls  = qemu_opt_get_number(legacy_opts, "cyls", 0);
-heads = qemu_opt_get_number(legacy_opts, "heads", 0);
-secs  = qemu_opt_get_number(legacy_opts, "secs", 0);
-
-if (cyls || heads || secs) {
-if (cyls < 1) {
-error_report("invalid physical cyls number");
-goto fail;
-}
-if (heads < 1) {
-error_report("invalid physical heads number");
-goto fail;
-}
-if (secs < 1) {
-error_report("invalid physical secs number");
-goto fail;
-}
-}
-
-translation = BIOS_ATA_TRANSLATION_AUTO;
-value = qemu_opt_get(legacy_opts, "trans");
-if (value != NULL) {
-if (!cyls) {
-error_report("'%s' trans must be used with cyls, heads and secs",
- value);
-goto fail;
-}
-if (!strcmp(value, "none")) {
-translation = BIOS_ATA_TRANSLATION_NONE;
-} else if (!strcmp(value, "lba")) {
-translation = BIOS_ATA_TRANSLATION_LBA;
-} else if (!strcmp(value, "large")) {
-translation = BIOS_ATA_TRANSLATION_LARGE;
-} else if (!strcmp(value, "rechs")) {
-translation = BIOS_ATA_TRANSLATION_RECHS;
-} else if (!strcmp(value, "auto")) {
-translation = BIOS_ATA_TRANSLATION_AUTO;
-} else {
-error_report("'%s' invalid translation type", value);
-goto fail;
-}
-}
-
-if (media == MEDIA_CDROM) {
-if (cyls || secs || heads) {
-error_report("CHS can't be set with media=cdrom");
-goto fail;
-}
-}
-
 /* Device address specified by bus/unit or index.
  * If none was specified, try to find the first free one. */
 bus_id  = qemu_opt_get_number(legacy_opts, "bus", 0);
@@ -1104,11 +1036,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, 

[Qemu-block] [PATCH v2 3/4] block: Remove deprecated -drive option serial

[Kevin Wolf]

The -drive option serial was deprecated in QEMU 2.10. It's time to
remove it.

Tests need to be updated to set the serial number with -global instead
of using the -drive option.

Signed-off-by: Kevin Wolf 
Reviewed-by: Markus Armbruster 
Reviewed-by: Jeff Cody 
Signed-off-by: Kevin Wolf 
---
 include/hw/block/block.h  |  1 -
 include/sysemu/blockdev.h |  1 -
 block/block-backend.c |  1 -
 blockdev.c| 10 --
 hw/block/block.c  | 13 -
 hw/block/nvme.c   |  1 -
 hw/block/virtio-blk.c |  1 -
 hw/ide/qdev.c |  1 -
 hw/scsi/scsi-disk.c   |  1 -
 hw/usb/dev-storage.c  |  1 -
 tests/ahci-test.c |  6 +++---
 tests/ide-test.c  |  8 
 qemu-doc.texi |  5 -
 qemu-options.hx   |  6 +-
 14 files changed, 8 insertions(+), 48 deletions(-)

diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index d4f4dfffab..e9f9e2223f 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -72,7 +72,6 @@ static inline unsigned int get_physical_block_exp(BlockConf 
*conf)
 
 /* Configuration helpers */
 
-void blkconf_serial(BlockConf *conf, char **serial);
 bool blkconf_geometry(BlockConf *conf, int *trans,
   unsigned cyls_max, unsigned heads_max, unsigned secs_max,
   Error **errp);
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index c0ae3700ec..24954b94e0 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -35,7 +35,6 @@ struct DriveInfo {
 bool is_default;/* Added by default_drive() ?  */
 int media_cd;
 QemuOpts *opts;
-char *serial;
 QTAILQ_ENTRY(DriveInfo) next;
 };
 
diff --git a/block/block-backend.c b/block/block-backend.c
index d55c328736..2d1a3463e8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -419,7 +419,6 @@ static void drive_info_del(DriveInfo *dinfo)
 return;
 }
 qemu_opts_del(dinfo->opts);
-g_free(dinfo->serial);
 g_free(dinfo);
 }
 
diff --git a/blockdev.c b/blockdev.c
index 83b3cc12e9..695e10d398 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -730,10 +730,6 @@ QemuOptsList qemu_legacy_drive_opts = {
 .type = QEMU_OPT_STRING,
 .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
 },{
-.name = "serial",
-.type = QEMU_OPT_STRING,
-.help = "disk serial number",
-},{
 .name = "file",
 .type = QEMU_OPT_STRING,
 .help = "file name",
@@ -775,12 +771,10 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type)
 const char *werror, *rerror;
 bool read_only = false;
 bool copy_on_read;
-const char *serial;
 const char *filename;
 Error *local_err = NULL;
 int i;
 const char *deprecated[] = {
-"serial"
 };
 
 /* Change legacy command line options into QMP ones */
@@ -948,9 +942,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, BlockInterfaceType 
block_default_type)
 goto fail;
 }
 
-/* Serial number */
-serial = qemu_opt_get(legacy_opts, "serial");
-
 /* no id supplied -> create one */
 if (qemu_opts_id(all_opts) == NULL) {
 char *new_id;
@@ -1025,7 +1016,6 @@ DriveInfo *drive_new(QemuOpts *all_opts, 
BlockInterfaceType block_default_type)
 dinfo->type = type;
 dinfo->bus = bus_id;
 dinfo->unit = unit_id;
-dinfo->serial = g_strdup(serial);
 
 blk_set_legacy_dinfo(blk, dinfo);
 
diff --git a/hw/block/block.c b/hw/block/block.c
index b6c80ab0b7..cf0eb826f1 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -15,19 +15,6 @@
 #include "qapi/qapi-types-block.h"
 #include "qemu/error-report.h"
 
-void blkconf_serial(BlockConf *conf, char **serial)
-{
-DriveInfo *dinfo;
-
-if (!*serial) {
-/* try to fall back to value set with legacy -drive serial=... */
-dinfo = blk_legacy_dinfo(conf->blk);
-if (dinfo) {
-*serial = g_strdup(dinfo->serial);
-}
-}
-}
-
 void blkconf_blocksizes(BlockConf *conf)
 {
 BlockBackend *blk = conf->blk;
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 811084b6a7..d5bf95b79b 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1215,7 +1215,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 return;
 }
 
-blkconf_serial(>conf, >serial);
 if (!n->serial) {
 error_setg(errp, "serial property not set");
 return;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 50b5c869e3..225fe44b7a 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -935,7 +935,6 @@ static void virtio_blk_device_realize(DeviceState *dev, 
Error **errp)
 return;
 }
 
-blkconf_serial(>conf, >serial);
 if (!blkconf_apply_backend_options(>conf,
blk_is_read_only(conf->conf.blk), true,
 

[Qemu-block] [PATCH v2 0/4] block: Remove deprecated -drive options

[Kevin Wolf]

We deprecated a bunch of -drive options in 2.10, so let's remove them
for 3.0.

v2:
- Simplified hd-geo-test code after removing test cases [Markus]
- Split patch 3 and 4 so that the deprecation warning code can be easily
  restored if we ever need it again [Markus]

Kevin Wolf (4):
  block: Remove deprecated -drive geometry options
  block: Remove deprecated -drive option addr
  block: Remove deprecated -drive option serial
  block: Remove dead deprecation warning code

 include/hw/block/block.h  |   1 -
 include/sysemu/blockdev.h |   3 --
 block/block-backend.c |   1 -
 blockdev.c| 110 --
 device-hotplug.c  |   4 --
 hw/block/block.c  |  27 
 hw/block/nvme.c   |   1 -
 hw/block/virtio-blk.c |   1 -
 hw/ide/qdev.c |   1 -
 hw/scsi/scsi-disk.c   |   1 -
 hw/usb/dev-storage.c  |   1 -
 tests/ahci-test.c |   6 +--
 tests/hd-geo-test.c   |  37 +++-
 tests/ide-test.c  |   8 ++--
 hmp-commands.hx   |   1 -
 qemu-doc.texi |  15 ---
 qemu-options.hx   |  14 +-
 17 files changed, 15 insertions(+), 217 deletions(-)

-- 
2.13.6

Re: [Qemu-block] bug in reopen arch

[Kevin Wolf]

Am 12.06.2018 um 20:57 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Hi all!
> 
> I've faced the following problem:
> 
>     1. create image with dirty bitmap, a.qcow2 (start qemu and run qmp
>     command block-dirty-bitmap-add)
> 
>     2. run the following commands:
> 
>     qemu-img create -f qcow2 -b a.qcow2 b.qcow2 10M
>     qemu-io -c 'write 0 512' b.qcow2
>     qemu-img commit b.qcow2
> 
>     3. last command fails with the following output:
> 
> Formatting 'b.qcow2', fmt=qcow2 size=68719476736 backing_file=a.qcow2
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> wrote 512/512 bytes at offset 0
> 512 bytes, 1 ops; 0.0953 sec (5.243 KiB/sec and 10.4867 ops/sec)
> qemu-img: #block397: Failed to make dirty bitmaps writable: Can't update
> bitmap directory: Operation not permitted
> qemu-img: Block job failed: Operation not permitted
> 
> And problem is that children are reopened _after_ parent. But qcow2 reopen
> needs write access to its file, to write IN_USE flag to dirty-bitmaps
> extension.

I was aware of a different instance of this problem: Assume a qcow2
image with an unknown autoclear flag (so it will be cleared on r/w
open), which is first opened r/o and then reopened r/w. This will fail
because .bdrv_reopen_prepare doesn't have the permissions yet.

Simply changing the order won't fix this because in the r/w -> r/o, the
driver will legitimately flush its caches in .bdrv_reopen_prepare, and
for this it still needs to be able to write.

We may need to have a way for nodes to access both the old and the new
state of their children. I'm not completely sure how to achieve this
best, though.

When I thought only of permissions, the obvious and simple thing to do
was to just get combined permissions for the old and new state, i.e.
'old_perm | new_perm' and 'old_shared & new_shared'. But I don't think
this is actually enough when the child node switches between a r/w and
a r/o file descriptor because even though QEMU's permission system would
allow the write, you still can't successfully write to a r/o file
descriptor.

Kevin

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

[Peter Maydell]

On 13 June 2018 at 15:53, Jeff Cody  wrote:
> The following changes since commit 2ab09bf2f9f55b9fb8d2de6eb2ba2a8570e268e2:
>
>   Merge remote-tracking branch 
> 'remotes/kraxel/tags/usb-20180612-pull-request' into staging (2018-06-12 
> 15:34:34 +0100)
>
> are available in the git repository at:
>
>   git://github.com/codyprime/qemu-kvm-jtc.git tags/block-pull-request
>
> for you to fetch changes up to b61acdecbf19c3c5a327baec1e8e4c06d0da68b7:
>
>   block: Ignore generated job QAPI files (2018-06-13 10:51:49 -0400)
>
> 
> Block patch for .gitignore
> 
>
> Eric Blake (1):
>   block: Ignore generated job QAPI files
>
>  .gitignore | 4 
>  1 file changed, 4 insertions(+)
>

Applied, thanks.

-- PMM

Re: [Qemu-block] [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit

[Kevin Wolf]

Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
> >> When you mix scalar and non-scalar keys, whether you get an "already
> >> set as scalar" or an "already set as dict" error depends on qdict
> >> iteration order.  Neither message makes much sense.  Replace by
> >> ""Cannot mix scalar and non-scalar keys".  This is similar to the
> >> message we get for mixing list and non-list keys.
> >> 
> >> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
> >> and add a comment.
> >> 
> >> Signed-off-by: Markus Armbruster 
> >
> > To be honest, I found the old version of the loop more obvious.
> >
> >>  qobject/block-qdict.c | 42 +-
> >>  1 file changed, 21 insertions(+), 21 deletions(-)
> >> 
> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> >> index a4e1c8d08f..35e9052816 100644
> >> --- a/qobject/block-qdict.c
> >> +++ b/qobject/block-qdict.c
> >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error 
> >> **errp)
> >>  QObject *qdict_crumple(const QDict *src, Error **errp)
> >>  {
> >>  const QDictEntry *ent;
> >> -QDict *two_level, *multi_level = NULL;
> >> +QDict *two_level, *multi_level = NULL, *child_dict;
> >>  QObject *dst = NULL, *child;
> >>  size_t i;
> >>  char *prefix = NULL;
> >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error 
> >> **errp)
> >>  }
> >>  
> >>  qdict_split_flat_key(ent->key, , );
> >> -
> >>  child = qdict_get(two_level, prefix);
> >> +child_dict = qobject_to(QDict, child);
> >> +
> >> +if (child) {
> >> +/*
> >> + * An existing child must be a dict and @ent must be a
> >> + * dict member (i.e. suffix not null), or else @ent
> >> + * clashes.
> >> + */
> >> +if (!child_dict || !suffix) {
> >> +error_setg(errp,
> >> +   "Cannot mix scalar and non-scalar keys");
> >> +goto error;
> >> +}
> >> +} else if (suffix) {
> >> +child_dict = qdict_new();
> >> +qdict_put(two_level, prefix, child_dict);
> >> +} else {
> >> +qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
> >> +}
> >
> > At least, can you please move the else branch to the if below so that
> > the addition of the new entry is symmetrical for both scalars and dicts?
> > As the code is, it mixes the conflict check, creation of the child dict
> > and addition of scalars (but not to the child dict) in a weird way in a
> > single if block.
> >
> > Or maybe organise it like this:
> >
> > if (child && !(child_dict && suffix)) {
> > error
> > }
> >
> > if (suffix) {
> > if (!child_dict) {
> > create it
> > add it to two_level
> > }
> > add entry to child_dict
> > } else {
> > add entry to two_level
> > }
> 
> Fleshing out...
> 
> if (child && !child_dict) {
> /*
>  * @prefix already exists and it's a non-dictionary,
>  * i.e. we've seen a scalar with key @prefix.  The same
>  * key can't occur twice, therefore suffix must be
>  * non-null.
>  */
> assert(suffix);
> /*
>  * @ent has key @prefix.@suffix, but we've already seen
>  * key @prefix: clash.
>  */
> error_setg(errp, "Cannot mix scalar and non-scalar keys");
> goto error;
> }

This catches "foo.bar" after "foo", but not "foo" after "foo.bar".

> if (suffix) {
> if (!child_dict) {
> child_dict = qdict_new();
> qdict_put(two_level, prefix, child_dict);
> }
> qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
> } else {
> assert(!child);

So "foo" after "foo.bar" would fail this assertion.

> qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
> }
> 
> Okay?

Kevin

Re: [Qemu-block] [PATCH v0 2/2] qmp: add block-set-copy-on-read command

[Kevin Wolf]

Am 14.06.2018 um 11:03 hat Alberto Garcia geschrieben:
> On Thu 14 Jun 2018 11:00:55 AM CEST, Kevin Wolf  wrote:
> > Am 13.06.2018 um 18:41 hat Max Reitz geschrieben:
> >> On 2018-06-13 18:02, Eric Blake wrote:
> >> > On 06/13/2018 10:47 AM, Denis Plotnikov wrote:
> >> >> The command enables/disables copy-on-read mode for VM's disk while
> >> >> VM is running.
> >> >>
> >> >> This is needed when using external disk readers to shape access pattern
> >> >> to the disk backend.
> >> >>
> >> >> Signed-off-by: Denis Plotnikov 
> >> >> ---
> >> > 
> >> > Deferring thoughts on the actual design for later;
> >> 
> >> But why? ;-)
> >> 
> >> This patch would definitely be superseded by a block reconfiguration
> >> command (i.e. presumably one that makes reopen accessible over QMP).
> >> With such a command, you could insert or remove a copy-on-read filter
> >> node at any point in time.
> >> 
> >> Since we definitely want block graph configuration, I don't think we
> >> want to add special commands now.
> >
> > I agree, and it seems that we get more and more use cases for a block
> > reconfiguration commands. Only yesterday we talked about the "true"
> > blockdev way for libvirt to implement I/O throttling. The result was
> > that without reopen, we still need to use the old QMP command to set
> > BlockBackend-based I/O throttling instead of using a filter node.
> >
> > So I'm eagerly awaiting Berto's promised patches for it.
> 
> They're coming, it's just a bit tricker than I initially though. I'll
> probably send the first series as an RFC if I don't manage to fix all
> problems, so we can discuss them a bit.

Sounds good to me. It's not completely surprising that there are some
tricky spots there, we've been postponing this for a reason...

Kevin

Re: [Qemu-block] [PATCH 3/3] qcow2: add compress threads

[Kevin Wolf]

Am 08.06.2018 um 21:20 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Do data compression in separate threads. This significantly improve
> performance for qemu-img convert with -W (allow async writes) and -c
> (compressed) options.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 

Looks correct to me, but why do we introduce a separate
MAX_COMPRESS_THREADS? Can't we simply leave the maximum number of
threads to the thread poll?

I see that you chose a much smaller number here (4 vs. 64), but is there
actually a good reason for this?

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/4] block: Remove deprecated -drive option serial

[Kevin Wolf]

Am 14.06.2018 um 15:09 hat Richard W.M. Jones geschrieben:
> On Thu, Jun 14, 2018 at 11:55:43AM +0200, Kevin Wolf wrote:
> > The -drive option serial was deprecated in QEMU 2.10. It's time to
> > remove it.
> > 
> > Tests need to be updated to set the serial number with -global instead
> > of using the -drive option.
> 
> Libguestfs uses this option to set the disk serial/label.  What is
> the alternate syntax?

Just set it in -device (for the disk, e.g. the one for ide-hd) rather
than -drive.

Kevin

[Qemu-block] [RFC PATCH 09/10] block: Add a 'x-blockdev-reopen' QMP command

[Alberto Garcia]

This command allows reopening an arbitrary BlockDriverState with a new
set of options. Some options (e.g node-name) cannot be changed and
some block drivers don't allow reopening, but otherwise this command
is modelled after 'blockdev-add' and the state of the reopened
BlockDriverState should generally be the same as if it had just been
added by 'blockdev-add' with the same set of options.

One notable exception is that when the 'backing' option is omitted
then 'blockdev-add' opens the image's default backing file, whereas
'x-blockdev-reopen' simply keeps the current one.

This command allows reconfiguring the graph by using the appropriate
options to change the children of a node. At the moment it's possible
to change a backing file by setting the 'backing' option to the name
of the new node, but it should also be possible to add a similar
functionality to other block drivers (e.g. Quorum, blkverify).

Although the API is unlikely to change, this command is marked
experimental for the time being so there's room to see if the
semantics need changes.

Signed-off-by: Alberto Garcia 
---
 blockdev.c   | 57 
 qapi/block-core.json | 29 ++
 2 files changed, 86 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 4862323012..1502747483 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -4236,6 +4236,63 @@ fail:
 visit_free(v);
 }
 
+void qmp_x_blockdev_reopen(BlockdevOptions *options, Error **errp)
+{
+BlockDriverState *bs;
+QObject *obj;
+Visitor *v = qobject_output_visitor_new();
+Error *local_err = NULL;
+int flags;
+BlockReopenQueue *queue;
+QDict *qdict;
+
+/* Check for the selected node name */
+if (!options->has_node_name) {
+error_setg(errp, "Node name not specified");
+goto fail;
+}
+
+bs = bdrv_find_node(options->node_name);
+if (!bs) {
+error_setg(errp, "Cannot find node named '%s'", options->node_name);
+goto fail;
+}
+
+/* Put all options in a QDict and flatten it */
+visit_type_BlockdevOptions(v, NULL, , _err);
+if (local_err) {
+error_propagate(errp, local_err);
+goto fail;
+}
+
+visit_complete(v, );
+qdict = qobject_to(QDict, obj);
+
+qdict_flatten(qdict);
+
+/* Set some default values */
+qdict_put_bool(qdict, BDRV_OPT_CACHE_DIRECT,
+   qdict_get_try_bool(qdict, BDRV_OPT_CACHE_DIRECT, false));
+qdict_put_bool(qdict, BDRV_OPT_CACHE_NO_FLUSH,
+   qdict_get_try_bool(qdict, BDRV_OPT_CACHE_NO_FLUSH, false));
+qdict_put_bool(qdict, BDRV_OPT_READ_ONLY,
+   qdict_get_try_bool(qdict, BDRV_OPT_READ_ONLY, false));
+
+flags = 0;
+if (!qdict_get_bool(qdict, BDRV_OPT_READ_ONLY)) {
+flags |= (BDRV_O_RDWR | BDRV_O_ALLOW_RDWR);
+}
+
+/* Perform the reopen operation */
+bdrv_subtree_drained_begin(bs);
+queue = bdrv_reopen_queue(NULL, bs, qdict, flags, false);
+bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, errp);
+bdrv_subtree_drained_end(bs);
+
+fail:
+visit_free(v);
+}
+
 void qmp_blockdev_del(const char *node_name, Error **errp)
 {
 AioContext *aio_context;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index fff23fc82b..ee8bf7c0ea 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3651,6 +3651,35 @@
 { 'command': 'blockdev-add', 'data': 'BlockdevOptions', 'boxed': true }
 
 ##
+# @x-blockdev-reopen:
+#
+# Reopens a block device using the given set of options. Any option
+# not specified will be reset to its default value regardless of its
+# previous status. If an option cannot be changed or a particular
+# driver does not support reopening the command will return an error.
+#
+# The top-level @node-name option (from BlockdevOptions) must be
+# specified and is used to select the block device to be reopened.
+# Other @node-name options must be either omitted or set to the
+# current name of the appropriate node. This command won't change any
+# node name and any attempt to do it will result in an error.
+#
+# The @backing option can always be omitted: any block device can be
+# reopened by specifying only its own options, without having to
+# include any of its backing files.
+#
+# Unlike blockdev-add, leaving out the @backing option will simply
+# keep the current backing file for that image and will not try to
+# open the default one. The way to replace a backing file is by
+# passing a reference to the new one in the @backing parameter.
+# If @backing is null then the current backing file will be removed.
+#
+# Since: 2.13
+##
+{ 'command': 'x-blockdev-reopen',
+  'data': 'BlockdevOptions', 'boxed': true }
+
+##
 # @blockdev-del:
 #
 # Deletes a block device that has been added using blockdev-add.
-- 
2.11.0

[Qemu-block] [RFC PATCH 08/10] block: Add bdrv_reset_options_allowed()

[Alberto Garcia]

bdrv_reopen_prepare() receives a BDRVReopenState with (among other
things) a new set of options to be applied to that BlockDriverState.

If an option is missing then it means that we want to reset it to its
default value rather than keeping the previous one. This way the state
of the block device after being reopened is comparable to that of a
device added with "blockdev-add" using the same set of options.

Not all options from all drivers can be changed, however. If the user
attempts to reset an immutable option to its default value using this
method then we must forbid it.

This new function takes a QemuOptsList with the options of a block
driver and checks if there's any that was explicitly set but is not
present in the BDRVReopenState.

If the option is present in both sets we don't need to check that it
keeps the same value. The loop at the end of bdrv_reopen_prepare()
already takes care of that.

Signed-off-by: Alberto Garcia 
---
 block.c | 49 +
 1 file changed, 49 insertions(+)

diff --git a/block.c b/block.c
index 91fff6361f..4aa40f7f10 100644
--- a/block.c
+++ b/block.c
@@ -2823,6 +2823,43 @@ BlockDriverState *bdrv_open(const char *filename, const 
char *reference,
 }
 
 /*
+ * For every option in @list, check that if it is present in
+ * @state->bs->explicit_options then it is also present in
+ * @state->options. Options present in @mutable_opts are skipped.
+ *
+ * @mutable_opts is either NULL or a NULL-terminated array of option
+ * names.
+ *
+ * Return 0 on success, -EINVAL otherwise.
+ */
+static int bdrv_reset_options_allowed(BDRVReopenState *state,
+  QemuOptsList *list,
+  const char *const mutable_opts[],
+  Error **errp)
+{
+QemuOptDesc *desc;
+for (desc = list->desc; desc && desc->name; desc++) {
+unsigned i;
+bool skip_option = false;
+for (i = 0; mutable_opts && mutable_opts[i] != 0; i++) {
+if (!strcmp(desc->name, mutable_opts[i])) {
+skip_option = true;
+break;
+}
+}
+
+if (!skip_option && !qdict_haskey(state->options, desc->name) &&
+qdict_haskey(state->bs->explicit_options, desc->name)) {
+error_setg(errp, "Option '%s' can't be reset to its default value",
+   desc->name);
+return -EINVAL;
+}
+}
+
+return 0;
+}
+
+/*
  * Returns true if @child can be reached recursively from @bs
  */
 static bool bdrv_recurse_has_child(BlockDriverState *bs,
@@ -3254,6 +3291,18 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 }
 
 if (drv->bdrv_reopen_prepare) {
+/* If a driver-specified option is missing, it means that we
+ * should reset it to its default value. Not all options can
+ * be modified, so we need to check that first */
+if (drv->runtime_opts) {
+ret = bdrv_reset_options_allowed(reopen_state, drv->runtime_opts,
+ drv->mutable_opts, _err);
+if (ret) {
+error_propagate(errp, local_err);
+goto error;
+}
+}
+
 ret = drv->bdrv_reopen_prepare(reopen_state, queue, _err);
 if (ret) {
 if (local_err != NULL) {
-- 
2.11.0

[Qemu-block] [RFC PATCH 03/10] block: Allow changing 'detect-zeroes' on reopen

[Alberto Garcia]

'detect-zeroes' is one of the basic BlockdevOptions available for all
drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
the user cannot change it and doesn't get an error explaining that it
can't be changed.

Since there's no reason why we shouldn't allow changing it and the
implementation is trivial, let's just do it.

Signed-off-by: Alberto Garcia 
---
 block.c   | 54 ---
 include/block/block.h |  1 +
 2 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/block.c b/block.c
index e470db7e5e..4c186e54b6 100644
--- a/block.c
+++ b/block.c
@@ -759,6 +759,29 @@ static void bdrv_join_options(BlockDriverState *bs, QDict 
*options,
 }
 }
 
+static BlockdevDetectZeroesOptions bdrv_parse_detect_zeroes(const char *value,
+int open_flags,
+Error **errp)
+{
+Error *local_err = NULL;
+BlockdevDetectZeroesOptions detect_zeroes =
+qapi_enum_parse(_lookup, value,
+BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return detect_zeroes;
+}
+
+if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
+!(open_flags & BDRV_O_UNMAP))
+{
+error_setg(errp, "setting detect-zeroes to unmap is not allowed "
+   "without setting discard operation to unmap");
+}
+
+return detect_zeroes;
+}
+
 /**
  * Set open flags for a given discard mode
  *
@@ -1390,27 +1413,14 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockBackend *file,
 
 detect_zeroes = qemu_opt_get(opts, "detect-zeroes");
 if (detect_zeroes) {
-BlockdevDetectZeroesOptions value =
-qapi_enum_parse(_lookup,
-detect_zeroes,
-BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
-_err);
+bs->detect_zeroes = bdrv_parse_detect_zeroes(detect_zeroes,
+ bs->open_flags,
+ _err);
 if (local_err) {
 error_propagate(errp, local_err);
 ret = -EINVAL;
 goto fail_opts;
 }
-
-if (value == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
-!(bs->open_flags & BDRV_O_UNMAP))
-{
-error_setg(errp, "setting detect-zeroes to unmap is not allowed "
- "without setting discard operation to unmap");
-ret = -EINVAL;
-goto fail_opts;
-}
-
-bs->detect_zeroes = value;
 }
 
 if (filename != NULL) {
@@ -3148,6 +3158,17 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 }
 }
 
+value = qemu_opt_get(opts, "detect-zeroes");
+if (value) {
+reopen_state->detect_zeroes =
+bdrv_parse_detect_zeroes(value, reopen_state->flags, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto error;
+}
+}
+
 /* node-name and driver must be unchanged. Put them back into the QDict, so
  * that they are checked at the end of this function. */
 value = qemu_opt_get(opts, "node-name");
@@ -3278,6 +3299,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 bs->explicit_options   = reopen_state->explicit_options;
 bs->open_flags = reopen_state->flags;
 bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
+bs->detect_zeroes  = reopen_state->detect_zeroes;
 
 bdrv_refresh_limits(bs, NULL);
 
diff --git a/include/block/block.h b/include/block/block.h
index e677080c4e..1e78587331 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -159,6 +159,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, 
BlockReopenQueueEntry) BlockReopenQueue;
 typedef struct BDRVReopenState {
 BlockDriverState *bs;
 int flags;
+BlockdevDetectZeroesOptions detect_zeroes;
 uint64_t perm, shared_perm;
 QDict *options;
 QDict *explicit_options;
-- 
2.11.0

[Qemu-block] [RFC PATCH 01/10] file-posix: Forbid trying to change unsupported options during reopen

[Alberto Garcia]

The file-posix code is used for the "file", "host_device" and
"host_cdrom" drivers, and it allows reopening images. However the only
option that is actually processed is "x-check-cache-dropped", and
changes in all other options (e.g. "filename") are silently ignored.

While we could allow changing some of the other options, let's keep
things as they are for now but return an error if the user tries to
change any of them.

Signed-off-by: Alberto Garcia 
---
 block/file-posix.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 07bb061fe4..1511a4d69d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -813,8 +813,13 @@ static int raw_reopen_prepare(BDRVReopenState *state,
 goto out;
 }
 
-rs->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped",
-s->check_cache_dropped);
+rs->check_cache_dropped =
+qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
+
+/* This driver's reopen function doesn't currently allow changing
+ * other options, so let's put them back in the original QDict and
+ * bdrv_reopen_prepare() will detect changes and complain. */
+qemu_opts_to_qdict(opts, state->options);
 
 if (s->type == FTYPE_CD) {
 rs->open_flags |= O_NONBLOCK;
-- 
2.11.0

[Qemu-block] [RFC PATCH 05/10] block: Add 'keep_old_opts' parameter to bdrv_reopen_queue()

[Alberto Garcia]

The bdrv_reopen_queue() function is used to create a queue with the
BDSs that are going to be reopened and their new options. Once the
queue is ready bdrv_reopen_multiple() is called to perform the
operation.

The original options from each one of the BDSs are kept, with the new
options passed to bdrv_reopen_queue() applied on top of them.

For "x-blockdev-reopen" we want a function that behaves much like
"blockdev-add". We want to ignore the previous set of options so that
only the ones actually specified by the user are applied, with the
rest having their default values.

We can achieve this by adding a new parameter to bdrv_reopen_queue()
that specifies whether the old set of options is kept or discarded
when building the reopen queue. All current callers will set that
parameter to true, but x-blockdev-reopen will set it to false.

Signed-off-by: Alberto Garcia 
---
 block.c   | 34 +++---
 block/replication.c   |  4 ++--
 include/block/block.h |  3 ++-
 qemu-io-cmds.c|  2 +-
 4 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/block.c b/block.c
index a741300fae..0b9268a48d 100644
--- a/block.c
+++ b/block.c
@@ -2850,7 +2850,8 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
  int flags,
  const BdrvChildRole *role,
  QDict *parent_options,
- int parent_flags)
+ int parent_flags,
+ bool keep_old_opts)
 {
 assert(bs != NULL);
 
@@ -2899,13 +2900,13 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 }
 
 /* Old explicitly set values (don't overwrite by inherited value) */
-if (bs_entry) {
-old_options = qdict_clone_shallow(bs_entry->state.explicit_options);
-} else {
-old_options = qdict_clone_shallow(bs->explicit_options);
+if (bs_entry || keep_old_opts) {
+old_options = qdict_clone_shallow(bs_entry ?
+  bs_entry->state.explicit_options :
+  bs->explicit_options);
+bdrv_join_options(bs, options, old_options);
+qobject_unref(old_options);
 }
-bdrv_join_options(bs, options, old_options);
-qobject_unref(old_options);
 
 explicit_options = qdict_clone_shallow(options);
 
@@ -2923,10 +2924,12 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 qobject_unref(options_copy);
 }
 
-/* Old values are used for options that aren't set yet */
-old_options = qdict_clone_shallow(bs->options);
-bdrv_join_options(bs, options, old_options);
-qobject_unref(old_options);
+if (keep_old_opts) {
+/* Old values are used for options that aren't set yet */
+old_options = qdict_clone_shallow(bs->options);
+bdrv_join_options(bs, options, old_options);
+qobject_unref(old_options);
+}
 
 /* bdrv_open_inherit() sets and clears some additional flags internally */
 flags &= ~BDRV_O_PROTOCOL;
@@ -2967,7 +2970,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 g_free(child_key_dot);
 
 bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
-child->role, options, flags);
+child->role, options, flags, keep_old_opts);
 }
 
 return bs_queue;
@@ -2975,10 +2978,11 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
 BlockDriverState *bs,
-QDict *options, int flags)
+QDict *options, int flags,
+bool keep_old_opts)
 {
 return bdrv_reopen_queue_child(bs_queue, bs, options, flags,
-   NULL, NULL, 0);
+   NULL, NULL, 0, keep_old_opts);
 }
 
 /*
@@ -3049,7 +3053,7 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, 
Error **errp)
 
 bdrv_subtree_drained_begin(bs);
 
-queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags);
+queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags, true);
 ret = bdrv_reopen_multiple(bdrv_get_aio_context(bs), queue, _err);
 if (local_err != NULL) {
 error_propagate(errp, local_err);
diff --git a/block/replication.c b/block/replication.c
index 826db7b304..af984c29dd 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -401,12 +401,12 @@ static void reopen_backing_file(BlockDriverState *bs, 
bool writable,
 
 if (orig_hidden_flags != new_hidden_flags) {
 reopen_queue = 

Re: [Qemu-block] [Qemu-devel] [PATCH 12/18] block-qdict: Clean up qdict_crumple() a bit

[Kevin Wolf]

Am 14.06.2018 um 13:52 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Am 13.06.2018 um 17:23 hat Markus Armbruster geschrieben:
> >> Kevin Wolf  writes:
> >> 
> >> > Am 12.06.2018 um 14:58 hat Markus Armbruster geschrieben:
> >> >> When you mix scalar and non-scalar keys, whether you get an "already
> >> >> set as scalar" or an "already set as dict" error depends on qdict
> >> >> iteration order.  Neither message makes much sense.  Replace by
> >> >> ""Cannot mix scalar and non-scalar keys".  This is similar to the
> >> >> message we get for mixing list and non-list keys.
> >> >> 
> >> >> I find qdict_crumple()'s first loop hard to understand.  Rearrange it
> >> >> and add a comment.
> >> >> 
> >> >> Signed-off-by: Markus Armbruster 
> >> >
> >> > To be honest, I found the old version of the loop more obvious.
> >> >
> >> >>  qobject/block-qdict.c | 42 +-
> >> >>  1 file changed, 21 insertions(+), 21 deletions(-)
> >> >> 
> >> >> diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
> >> >> index a4e1c8d08f..35e9052816 100644
> >> >> --- a/qobject/block-qdict.c
> >> >> +++ b/qobject/block-qdict.c
> >> >> @@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error 
> >> >> **errp)
> >> >>  QObject *qdict_crumple(const QDict *src, Error **errp)
> >> >>  {
> >> >>  const QDictEntry *ent;
> >> >> -QDict *two_level, *multi_level = NULL;
> >> >> +QDict *two_level, *multi_level = NULL, *child_dict;
> >> >>  QObject *dst = NULL, *child;
> >> >>  size_t i;
> >> >>  char *prefix = NULL;
> >> >> @@ -422,29 +422,29 @@ QObject *qdict_crumple(const QDict *src, Error 
> >> >> **errp)
> >> >>  }
> >> >>  
> >> >>  qdict_split_flat_key(ent->key, , );
> >> >> -
> >> >>  child = qdict_get(two_level, prefix);
> >> >> +child_dict = qobject_to(QDict, child);
> >> >> +
> >> >> +if (child) {
> >> >> +/*
> >> >> + * An existing child must be a dict and @ent must be a
> >> >> + * dict member (i.e. suffix not null), or else @ent
> >> >> + * clashes.
> >> >> + */
> >> >> +if (!child_dict || !suffix) {
> >> >> +error_setg(errp,
> >> >> +   "Cannot mix scalar and non-scalar keys");
> >> >> +goto error;
> >> >> +}
> >> >> +} else if (suffix) {
> >> >> +child_dict = qdict_new();
> >> >> +qdict_put(two_level, prefix, child_dict);
> >> >> +} else {
> >> >> +qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
> >> >> +}
> >> >
> >> > At least, can you please move the else branch to the if below so that
> >> > the addition of the new entry is symmetrical for both scalars and dicts?
> >> > As the code is, it mixes the conflict check, creation of the child dict
> >> > and addition of scalars (but not to the child dict) in a weird way in a
> >> > single if block.
> >> >
> >> > Or maybe organise it like this:
> >> >
> >> > if (child && !(child_dict && suffix)) {
> >> > error
> >> > }
> >> >
> >> > if (suffix) {
> >> > if (!child_dict) {
> >> > create it
> >> > add it to two_level
> >> > }
> >> > add entry to child_dict
> >> > } else {
> >> > add entry to two_level
> >> > }
> >> 
> >> Fleshing out...
> >> 
> >> if (child && !child_dict) {
> >> /*
> >>  * @prefix already exists and it's a non-dictionary,
> >>  * i.e. we've seen a scalar with key @prefix.  The same
> >>  * key can't occur twice, therefore suffix must be
> >>  * non-null.
> >>  */
> >> assert(suffix);
> >> /*
> >>  * @ent has key @prefix.@suffix, but we've already seen
> >>  * key @prefix: clash.
> >>  */
> >> error_setg(errp, "Cannot mix scalar and non-scalar keys");
> >> goto error;
> >> }
> >
> > This catches "foo.bar" after "foo", but not "foo" after "foo.bar".
> >
> >> if (suffix) {
> >> if (!child_dict) {
> >> child_dict = qdict_new();
> >> qdict_put(two_level, prefix, child_dict);
> >> }
> >> qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
> >> } else {
> >> assert(!child);
> >
> > So "foo" after "foo.bar" would fail this assertion.
> >
> >> qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
> >> }
> >> 
> >> Okay?
> >
> > Kevin
> 
> I feel dumb.  Next try:
> 
> if (child) {
> /*
>  * If @child_dict, then all previous keys with this prefix
>  * had a suffix.  If @suffix, this one has one as well,
>  * and we're good, else there's a clash.
>  */
> if (!child_dict || !suffix) {
> error_setg(errp, "Cannot mix 

Re: [Qemu-block] [PATCH 3/3] qcow2: add compress threads

[Kevin Wolf]

Am 14.06.2018 um 15:19 hat Denis V. Lunev geschrieben:
> On 06/14/2018 04:16 PM, Kevin Wolf wrote:
> > Am 08.06.2018 um 21:20 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> Do data compression in separate threads. This significantly improve
> >> performance for qemu-img convert with -W (allow async writes) and -c
> >> (compressed) options.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> > Looks correct to me, but why do we introduce a separate
> > MAX_COMPRESS_THREADS? Can't we simply leave the maximum number of
> > threads to the thread poll?
> >
> > I see that you chose a much smaller number here (4 vs. 64), but is there
> > actually a good reason for this?
> >
> > Kevin
> yes. In the other case the guest will suffer much more from this increased
> activity and load on the host.

Ah, your primary motivation is use in a backup block job? I completely
forgot about that one (and qemu-img shouldn't care because there is no
guest), but that makes some sense.

Makes me wonder whether this value should be configurable. But that can
come later.

Kevin

[Qemu-block] [PATCH] block/crypto: Simplify block_crypto_{open, create}_opts_init()

[Markus Armbruster]

block_crypto_open_opts_init() and block_crypto_create_opts_init()
contain a virtual visit of QCryptoBlockOptions and
QCryptoBlockCreateOptions less member "format", respectively.

Change their callers to put member "format" in the QDict, so they can
use the generated visitors for these types instead.

Signed-off-by: Markus Armbruster 
---
 block/crypto.c | 95 ++
 block/crypto.h |  8 ++---
 block/qcow.c   |  5 ++-
 block/qcow2.c  | 10 +++---
 4 files changed, 18 insertions(+), 100 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index bc322b50f5..ea3b4aeb34 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -148,102 +148,26 @@ static QemuOptsList block_crypto_create_opts_luks = {
 
 
 QCryptoBlockOpenOptions *
-block_crypto_open_opts_init(QCryptoBlockFormat format,
-QDict *opts,
-Error **errp)
+block_crypto_open_opts_init(QDict *opts, Error **errp)
 {
 Visitor *v;
-QCryptoBlockOpenOptions *ret = NULL;
-Error *local_err = NULL;
-
-ret = g_new0(QCryptoBlockOpenOptions, 1);
-ret->format = format;
+QCryptoBlockOpenOptions *ret;
 
 v = qobject_input_visitor_new_keyval(QOBJECT(opts));
-
-visit_start_struct(v, NULL, NULL, 0, _err);
-if (local_err) {
-goto out;
-}
-
-switch (format) {
-case Q_CRYPTO_BLOCK_FORMAT_LUKS:
-visit_type_QCryptoBlockOptionsLUKS_members(
-v, >u.luks, _err);
-break;
-
-case Q_CRYPTO_BLOCK_FORMAT_QCOW:
-visit_type_QCryptoBlockOptionsQCow_members(
-v, >u.qcow, _err);
-break;
-
-default:
-error_setg(_err, "Unsupported block format %d", format);
-break;
-}
-if (!local_err) {
-visit_check_struct(v, _err);
-}
-
-visit_end_struct(v, NULL);
-
- out:
-if (local_err) {
-error_propagate(errp, local_err);
-qapi_free_QCryptoBlockOpenOptions(ret);
-ret = NULL;
-}
+visit_type_QCryptoBlockOpenOptions(v, NULL, , errp);
 visit_free(v);
 return ret;
 }
 
 
 QCryptoBlockCreateOptions *
-block_crypto_create_opts_init(QCryptoBlockFormat format,
-  QDict *opts,
-  Error **errp)
+block_crypto_create_opts_init(QDict *opts, Error **errp)
 {
 Visitor *v;
-QCryptoBlockCreateOptions *ret = NULL;
-Error *local_err = NULL;
-
-ret = g_new0(QCryptoBlockCreateOptions, 1);
-ret->format = format;
+QCryptoBlockCreateOptions *ret;
 
 v = qobject_input_visitor_new_keyval(QOBJECT(opts));
-
-visit_start_struct(v, NULL, NULL, 0, _err);
-if (local_err) {
-goto out;
-}
-
-switch (format) {
-case Q_CRYPTO_BLOCK_FORMAT_LUKS:
-visit_type_QCryptoBlockCreateOptionsLUKS_members(
-v, >u.luks, _err);
-break;
-
-case Q_CRYPTO_BLOCK_FORMAT_QCOW:
-visit_type_QCryptoBlockOptionsQCow_members(
-v, >u.qcow, _err);
-break;
-
-default:
-error_setg(_err, "Unsupported block format %d", format);
-break;
-}
-if (!local_err) {
-visit_check_struct(v, _err);
-}
-
-visit_end_struct(v, NULL);
-
- out:
-if (local_err) {
-error_propagate(errp, local_err);
-qapi_free_QCryptoBlockCreateOptions(ret);
-ret = NULL;
-}
+visit_type_QCryptoBlockCreateOptions(v, NULL, , errp);
 visit_free(v);
 return ret;
 }
@@ -281,8 +205,9 @@ static int block_crypto_open_generic(QCryptoBlockFormat 
format,
 }
 
 cryptoopts = qemu_opts_to_qdict(opts, NULL);
+qdict_put_str(cryptoopts, "format", QCryptoBlockFormat_str(format));
 
-open_opts = block_crypto_open_opts_init(format, cryptoopts, errp);
+open_opts = block_crypto_open_opts_init(cryptoopts, errp);
 if (!open_opts) {
 goto cleanup;
 }
@@ -605,8 +530,8 @@ static int coroutine_fn 
block_crypto_co_create_opts_luks(const char *filename,
  _crypto_create_opts_luks,
  true);
 
-create_opts = block_crypto_create_opts_init(Q_CRYPTO_BLOCK_FORMAT_LUKS,
-cryptoopts, errp);
+qdict_put_str(cryptoopts, "format", "luks");
+create_opts = block_crypto_create_opts_init(cryptoopts, errp);
 if (!create_opts) {
 ret = -EINVAL;
 goto fail;
diff --git a/block/crypto.h b/block/crypto.h
index 0f985ea4e2..dd7d47903c 100644
--- a/block/crypto.h
+++ b/block/crypto.h
@@ -89,13 +89,9 @@
 }
 
 QCryptoBlockCreateOptions *
-block_crypto_create_opts_init(QCryptoBlockFormat format,
-  QDict *opts,
-  Error **errp);
+block_crypto_create_opts_init(QDict *opts, Error **errp);
 
 QCryptoBlockOpenOptions *
-block_crypto_open_opts_init(QCryptoBlockFormat format,
-QDict *opts,
-Error 

Re: [Qemu-block] [PATCH v5 02/14] block/mirror: Convert to coroutines

[Kevin Wolf]

Am 13.06.2018 um 20:18 hat Max Reitz geschrieben:
> In order to talk to the source BDS (and maybe in the future to the
> target BDS as well) directly, we need to convert our existing AIO
> requests into coroutine I/O requests.
> 
> Signed-off-by: Max Reitz 
> Reviewed-by: Fam Zheng 
> ---
>  block/mirror.c | 152 +
>  1 file changed, 90 insertions(+), 62 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index f5e240611b..47122482ff 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -78,6 +78,10 @@ typedef struct MirrorOp {
>  QEMUIOVector qiov;
>  int64_t offset;
>  uint64_t bytes;
> +
> +/* The pointee is set by mirror_co_read(), mirror_co_zero(), and
> + * mirror_co_discard() before yielding for the first time */
> +int64_t *bytes_handled;

Should we at least document that this becomes invalid after the
MirrorOp coroutine has yielded for the first time because it points to a
stack variable of the caller which returns then?

>  } MirrorOp;
>  
>  typedef enum MirrorMethod {
> @@ -99,7 +103,7 @@ static BlockErrorAction mirror_error_action(MirrorBlockJob 
> *s, bool read,
>  }
>  }
>  
> -static void mirror_iteration_done(MirrorOp *op, int ret)
> +static void coroutine_fn mirror_iteration_done(MirrorOp *op, int ret)
>  {
>  MirrorBlockJob *s = op->s;
>  struct iovec *iov;
> @@ -136,9 +140,8 @@ static void mirror_iteration_done(MirrorOp *op, int ret)
>  }
>  }
>  
> -static void mirror_write_complete(void *opaque, int ret)
> +static void coroutine_fn mirror_write_complete(MirrorOp *op, int ret)
>  {
> -MirrorOp *op = opaque;
>  MirrorBlockJob *s = op->s;
>  
>  aio_context_acquire(blk_get_aio_context(s->common.blk));
> @@ -155,9 +158,8 @@ static void mirror_write_complete(void *opaque, int ret)
>  aio_context_release(blk_get_aio_context(s->common.blk));
>  }

One of the effects of this conversion is that we hold the AioContext
lock for s->common.blk recursively multiple times. I don't think
anything calls drain in such sections (which would hang), so it's
probably okay, but it could turn out to be a trap in the long run.

> -static void mirror_read_complete(void *opaque, int ret)
> +static void coroutine_fn mirror_read_complete(MirrorOp *op, int ret)
>  {
> -MirrorOp *op = opaque;
>  MirrorBlockJob *s = op->s;
>  
>  aio_context_acquire(blk_get_aio_context(s->common.blk));
> @@ -172,8 +174,9 @@ static void mirror_read_complete(void *opaque, int ret)
>  
>  mirror_iteration_done(op, ret);
>  } else {
> -blk_aio_pwritev(s->target, op->offset, >qiov,
> -0, mirror_write_complete, op);
> +ret = blk_co_pwritev(s->target, op->offset,
> + op->qiov.size, >qiov, 0);
> +mirror_write_complete(op, ret);
>  }
>  aio_context_release(blk_get_aio_context(s->common.blk));
>  }
> @@ -230,60 +233,57 @@ static inline void mirror_wait_for_io(MirrorBlockJob *s)
>  s->waiting_for_io = false;
>  }
>  
> -/* Submit async read while handling COW.
> - * Returns: The number of bytes copied after and including offset,
> - *  excluding any bytes copied prior to offset due to alignment.
> - *  This will be @bytes if no alignment is necessary, or
> - *  (new_end - offset) if tail is rounded up or down due to
> - *  alignment or buffer limit.
> +/* Perform a mirror copy operation.
> + *
> + * *op->bytes_handled is set to the number of bytes copied after and
> + * including offset, excluding any bytes copied prior to offset due
> + * to alignment.  This will be op->bytes if no alignment is necessary,
> + * or (new_end - op->offset) if the tail is rounded up or down due to
> + * alignment or buffer limit.
>   */
> -static uint64_t mirror_do_read(MirrorBlockJob *s, int64_t offset,
> -   uint64_t bytes)
> +static void coroutine_fn mirror_co_read(void *opaque)
>  {
> +MirrorOp *op = opaque;
> +MirrorBlockJob *s = op->s;
>  BlockBackend *source = s->common.blk;
>  int nb_chunks;
>  uint64_t ret;
> -MirrorOp *op;
>  uint64_t max_bytes;
>  
>  max_bytes = s->granularity * s->max_iov;
>  
>  /* We can only handle as much as buf_size at a time. */
> -bytes = MIN(s->buf_size, MIN(max_bytes, bytes));
> -assert(bytes);
> -assert(bytes < BDRV_REQUEST_MAX_BYTES);
> -ret = bytes;
> +op->bytes = MIN(s->buf_size, MIN(max_bytes, op->bytes));
> +assert(op->bytes);
> +assert(op->bytes < BDRV_REQUEST_MAX_BYTES);
> +*op->bytes_handled = op->bytes;
>  
>  if (s->cow_bitmap) {
> -ret += mirror_cow_align(s, , );
> +*op->bytes_handled += mirror_cow_align(s, >offset, >bytes);
>  }

Actually, if we factor out this block, call it from mirror_perform() and
only assert the conditions here, we could get completely rid of that
peculiar op->bytes_handled.

Kevin

[Qemu-block] [RFC PATCH 02/10] block: Allow changing 'discard' on reopen

[Alberto Garcia]

'discard' is one of the basic BlockdevOptions available for all
drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
the user cannot change it and doesn't get an error explaining that it
can't be changed.

Since there's no reason why we shouldn't allow changing it and the
implementation is trivial, let's just do it.

Signed-off-by: Alberto Garcia 
---
 block.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/block.c b/block.c
index 50887087f3..e470db7e5e 100644
--- a/block.c
+++ b/block.c
@@ -3139,6 +3139,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 
 update_flags_from_options(_state->flags, opts);
 
+value = qemu_opt_get(opts, "discard");
+if (value != NULL) {
+if (bdrv_parse_discard_flags(value, _state->flags) != 0) {
+error_setg(errp, "Invalid discard option");
+ret = -EINVAL;
+goto error;
+}
+}
+
 /* node-name and driver must be unchanged. Put them back into the QDict, so
  * that they are checked at the end of this function. */
 value = qemu_opt_get(opts, "node-name");
-- 
2.11.0

[Qemu-block] ping Re: [PATCH v2] util/async: avoid NULL pointer dereference

[WangJie (Pluto)]

ping

On 2018/6/12 11:32, Jeff Cody wrote:
> On Tue, Jun 12, 2018 at 07:26:25AM +0800, Jie Wang wrote:
>> if laio_init create linux_aio failed and return NULL, NULL pointer
>> dereference will occur when laio_attach_aio_context dereference
>> linux_aio in aio_get_linux_aio. Let's avoid it and report error.
>>
>> Signed-off-by: Jie Wang 
> 
> Reviewed-by: Jeff Cody 
> 
>> ---
>>  block/file-posix.c | 19 +--
>>  util/async.c   |  5 -
>>  2 files changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 513d371bb1..653017d7a5 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1665,6 +1665,11 @@ static int coroutine_fn raw_co_prw(BlockDriverState 
>> *bs, uint64_t offset,
>>  #ifdef CONFIG_LINUX_AIO
>>  } else if (s->use_linux_aio) {
>>  LinuxAioState *aio = 
>> aio_get_linux_aio(bdrv_get_aio_context(bs));
>> +if (!aio) {
>> +s->use_linux_aio = false;
>> +error_report("Failed to get linux aio");
>> +return -EIO;
>> +}
>>  assert(qiov->size == bytes);
>>  return laio_co_submit(bs, aio, s->fd, offset, qiov, type);
>>  #endif
>> @@ -1695,7 +1700,12 @@ static void raw_aio_plug(BlockDriverState *bs)
>>  BDRVRawState *s = bs->opaque;
>>  if (s->use_linux_aio) {
>>  LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
>> -laio_io_plug(bs, aio);
>> +if (aio) {
>> +laio_io_plug(bs, aio);
>> +} else {
>> +s->use_linux_aio = false;
>> +error_report("Failed to get linux aio");
>> +}
>>  }
>>  #endif
>>  }
>> @@ -1706,7 +1716,12 @@ static void raw_aio_unplug(BlockDriverState *bs)
>>  BDRVRawState *s = bs->opaque;
>>  if (s->use_linux_aio) {
>>  LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
>> -laio_io_unplug(bs, aio);
>> +if (aio) {
>> +laio_io_unplug(bs, aio);
>> +} else {
>> +s->use_linux_aio = false;
>> +error_report("Failed to get linux aio");
>> +}
>>  }
>>  #endif
>>  }
>> diff --git a/util/async.c b/util/async.c
>> index 03f62787f2..08d71340f8 100644
>> --- a/util/async.c
>> +++ b/util/async.c
>> @@ -327,8 +327,11 @@ LinuxAioState *aio_get_linux_aio(AioContext *ctx)
>>  {
>>  if (!ctx->linux_aio) {
>>  ctx->linux_aio = laio_init();
>> -laio_attach_aio_context(ctx->linux_aio, ctx);
>> +if (ctx->linux_aio) {
>> +laio_attach_aio_context(ctx->linux_aio, ctx);
>> +}
>>  }
>> +
>>  return ctx->linux_aio;
>>  }
>>  #endif
>> -- 
>> 2.15.0.windows.1
>>
>>
> 
> .
> 

[Qemu-block] [RFC PATCH 04/10] block: Allow changing 'force-share' on reopen

[Alberto Garcia]

'force-share' is one of the basic BlockdevOptions available for all
drivers, but it's silently ignored by bdrv_reopen_prepare/commit(), so
the user cannot change it and doesn't get an error explaining that it
can't be changed.

Since there's no reason why we shouldn't allow changing it and the
implementation is trivial, let's just do it.

Signed-off-by: Alberto Garcia 
---
 block.c   | 11 +++
 include/block/block.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/block.c b/block.c
index 4c186e54b6..a741300fae 100644
--- a/block.c
+++ b/block.c
@@ -3169,6 +3169,16 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 }
 }
 
+reopen_state->force_share = qemu_opt_get_bool(opts, BDRV_OPT_FORCE_SHARE,
+  false);
+if (reopen_state->force_share && (reopen_state->flags & BDRV_O_RDWR)) {
+error_setg(errp,
+   BDRV_OPT_FORCE_SHARE
+   "=on can only be used with read-only images");
+ret = -EINVAL;
+goto error;
+}
+
 /* node-name and driver must be unchanged. Put them back into the QDict, so
  * that they are checked at the end of this function. */
 value = qemu_opt_get(opts, "node-name");
@@ -3300,6 +3310,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 bs->open_flags = reopen_state->flags;
 bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
 bs->detect_zeroes  = reopen_state->detect_zeroes;
+bs->force_share= reopen_state->force_share;
 
 bdrv_refresh_limits(bs, NULL);
 
diff --git a/include/block/block.h b/include/block/block.h
index 1e78587331..9306c986ef 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -160,6 +160,7 @@ typedef struct BDRVReopenState {
 BlockDriverState *bs;
 int flags;
 BlockdevDetectZeroesOptions detect_zeroes;
+bool force_share;
 uint64_t perm, shared_perm;
 QDict *options;
 QDict *explicit_options;
-- 
2.11.0

[Qemu-block] [RFC PATCH 00/10] Add a 'x-blockdev-reopen' QMP command

[Alberto Garcia]

Hi,

here's my first attempt to implement a bdrv_reopen() QMP command. I'm
tagging this as RFC because there are still several important things
that need to be sorted out before I consider this stable enough. And
I'd still prefix the command with an "x-" for a while. But let's get
to the details.

The new command is called x-blockdev-reopen, and it's designed to be
similar to blockdev-add. It also takes BlockdevOptions as a
parameter. The "node-name" field of BlockdevOptions must be set, and
it is used to select the BlockDriverState to reopen.

In this example "hd0" is reopened with the given set of options:

{ "execute": "x-blockdev-reopen",
  "arguments": {
  "driver": "qcow2",
  "node-name": "hd0",
  "file": {
  "driver": "file",
  "filename": "hd0.qcow2",
  "node-name": "hd0f"
  },
  "l2-cache-size": 524288
   }
}

Changing options

The general idea is quite straightforward, but the devil is in the
detail. Since this command tries to mimic blockdev-add, the state of
the block device after being reopened should generally be equivalent
to that of a block device added with the same set of options.

There are two important things with this:

  a) Some options cannot be changed (most drivers don't allow that, in
 fact).
  b) If an option is missing, it should be reset to its default value
 (rather than keeping its previous value).

Example: the default value of l2-cache-size is 1MB. If we set a
different value (2MB) on blockdev-add but then omit the option in
x-blockdev-reopen, then it should be reset back to 1MB. The current
bdrv_reopen() code keeps the previous value.

Trying to change an option that doesn't allow it is already being
handled by the code. The loop at the end of bdrv_reopen_prepare()
checks all options that were not handled by the block driver and sees
if any of them has been modified. There was at least one case where a
block driver silently ignores new values for some options (I fixed
that in the series) but otherwise this works generally well.

However, as I explained earlier in (b), omitting an option is supposed
to reset it to its default value, so it's also a way of changing
it. If the option cannot be changed then we should detect it and
return an error. What I'm doing in this series is making every driver
publish its list of run-time options, and which ones of them can be
modified.

Backing files
=
This command allows reconfigurations in the node graph. Currently it
only allows changing an image's backing file, but it should be
possible to implement similar functionalities in drivers that have
other children, like blkverify or quorum.

Let's add an image with its backing file (hd1 <- hd0) like this:

{ "execute": "blockdev-add",
  "arguments": {
  "driver": "qcow2",
  "node-name": "hd0",
  "file": {
  "driver": "file",
  "filename": "hd0.qcow2",
  "node-name": "hd0f"
  },
  "backing": {
  "driver": "qcow2",
  "node-name": "hd1",
  "file": {
  "driver": "file",
  "filename": "hd1.qcow2",
  "node-name": "hd1f"
  }
  }
   }
}

Removing the backing file can be done by simply passing the option
{ ..., "backing": null } to x-blockdev-reopen.

Replacing it is not so straightforward. If we pass something like
{ ..., "backing": { "driver": "foo" ... } } then x-blockdev-reopen
will assume that we're trying to change the options of the existing
backing file (and it will fail in most cases because it'll likely
think that we're trying to change the node name -among other options).

So I decided to use a reference instead: { ..., "backing": "hd2" },
where "hd2" is of an existing node that has been added previously.

If the "backing" option is left out then we simply keep the existing
backing file. I have doubts about this, because it differs from what
blockdev-add would do (open the default backing file stored in the
image header), but I don't think we want to do that on reopen. Perhaps
we could force the user to specify "backing" in these cases? I haven't
explored this option much yet.

There are some tricky things with this: bs->options keeps the
effective set of options for a given BlockDriverState (*), including
the options of the backing file.

(*) not quite, because that dict is not updated at all in
bdrv_reopen_commit(). That looks like a bug to me and I'll
probably need to fix that.

So on one hand the options for a given BDS can appear at least twice:
on the BDS itself an on its parent(s). If you reopen the child then
both sets are out of sync. On the other hand the graph can be
reconfigured by means other than x-blockdev-reopen: if you do
block-stream you are changing a BDS's backing file. That command
includes a call to bdrv_reopen(), keeping the 

[Qemu-block] [RFC PATCH 06/10] block: Allow changing the backing file on reopen

[Alberto Garcia]

This patch allows the user to change the backing file of an image that
is being reopened. Here's what it does:

 - In bdrv_reopen_queue_child(): if the 'backing' option points to an
   image different from the current backing file then it means that
   the latter is going be detached so it must not be put in the reopen
   queue.

 - In bdrv_reopen_prepare(): check that the value of 'backing' points
   to an existing node or is null. If it points to an existing node it
   also needs to make sure that replacing the backing file will not
   create a cycle in the node graph (i.e. you cannot reach the parent
   from the new backing file).

 - In bdrv_reopen_commit(): perform the actual node replacement by
   calling bdrv_set_backing_hd(). It may happen that there are
   temporary implicit nodes between the BDS that we are reopening and
   the backing file that we want to replace (e.g. a commit fiter node),
   so we must skip them.

Although x-blockdev-reopen is meant to be used like blockdev-add,
there's two important things that must be taken into account:

1) The only way to set a new backing file is by using a reference to
   an existing node (previously added with e.g. blockdev-add).
   If 'backing' contains a dictionary with a new set of options
   ({"driver": "qcow2", "file": { ... }}) then it is interpreted that
   the _existing_ backing file must be reopened with those options.

2) If the 'backing' option is omitted altogether then the existing
   backing file (if any) is kept. Unlike blockdev-add this will _not_
   open the default backing file specified in the image header.

Signed-off-by: Alberto Garcia 
---
 block.c | 86 +++--
 1 file changed, 84 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 0b9268a48d..91fff6361f 100644
--- a/block.c
+++ b/block.c
@@ -2823,6 +2823,27 @@ BlockDriverState *bdrv_open(const char *filename, const 
char *reference,
 }
 
 /*
+ * Returns true if @child can be reached recursively from @bs
+ */
+static bool bdrv_recurse_has_child(BlockDriverState *bs,
+   BlockDriverState *child)
+{
+BdrvChild *c;
+
+if (bs == child) {
+return true;
+}
+
+QLIST_FOREACH(c, >children, next) {
+if (bdrv_recurse_has_child(c->bs, child)) {
+return true;
+}
+}
+
+return false;
+}
+
+/*
  * Adds a BlockDriverState to a simple queue for an atomic, transactional
  * reopen of multiple devices.
  *
@@ -2957,6 +2978,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 QLIST_FOREACH(child, >children, next) {
 QDict *new_child_options;
 char *child_key_dot;
+bool child_keep_old = keep_old_opts;
 
 /* reopen can only change the options of block devices that were
  * implicitly created and inherited options. For other (referenced)
@@ -2965,12 +2987,28 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 continue;
 }
 
+/* If the 'backing' option is set and it points to a different
+ * node then it means that we want to replace the current one,
+ * so we shouldn't put it in the reopen queue */
+if (child->role == _backing && qdict_haskey(options, "backing")) 
{
+const char *backing = qdict_get_try_str(options, "backing");
+if (g_strcmp0(backing, child->bs->node_name)) {
+continue;
+}
+}
+
 child_key_dot = g_strdup_printf("%s.", child->name);
 qdict_extract_subqdict(options, _child_options, child_key_dot);
 g_free(child_key_dot);
 
+/* If the 'backing' option was omitted then we keep the old
+ * set of options */
+if (child->role == _backing && !qdict_size(new_child_options)) {
+child_keep_old = true;
+}
+
 bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
-child->role, options, flags, keep_old_opts);
+child->role, options, flags, child_keep_old);
 }
 
 return bs_queue;
@@ -3262,7 +3300,34 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
  * the user can simply omit options which cannot be changed anyway,
  * so they will stay unchanged.
  */
-if (!qobject_is_equal(new, old)) {
+/*
+ * Allow changing the 'backing' option. The new value can be
+ * either a reference to an existing node (using its node name)
+ * or NULL to simply detach the current backing file.
+ */
+if (!strcmp(entry->key, "backing")) {
+switch (qobject_type(new)) {
+case QTYPE_QNULL:
+break;
+case QTYPE_QSTRING: {
+const char *str = 

[Qemu-block] [RFC PATCH 07/10] block: Add 'runtime_opts' and 'mutable_opts' fields to BlockDriver

[Alberto Garcia]

This patch adds two new fields to BlockDriver:

   - runtime_opts: list of runtime options for a particular block
 driver. We'll use this list later to detect what options are
 missing when we try to reopen a block device.

   - mutable_opts: names of the runtime options that can be modified
 after a block device has been added. This way an option can not
 be changed unless we explicitly allow it.

This also sets runtime_opts (and mutable_opts where appropriate) in
all drivers that allow reopening. Most of those drivers don't actually
support changing any of their options. If the user specifies a new
value for an option that can't be changed then we already detect that
and forbid it (in bdrv_reopen_prepare()). But if the user omits an
option in order to try to reset it to its default value we need to
detect that, so we'll use these two new fields for that.

Signed-off-by: Alberto Garcia 
---
 block/blkdebug.c  |  1 +
 block/crypto.c|  1 +
 block/file-posix.c| 10 ++
 block/iscsi.c |  2 ++
 block/null.c  |  2 ++
 block/nvme.c  |  1 +
 block/qcow.c  |  1 +
 block/rbd.c   |  1 +
 block/sheepdog.c  |  2 ++
 block/vpc.c   |  1 +
 include/block/block_int.h |  4 
 11 files changed, 26 insertions(+)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 526af2a808..9b52465642 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -893,6 +893,7 @@ static BlockDriver bdrv_blkdebug = {
 .protocol_name  = "blkdebug",
 .instance_size  = sizeof(BDRVBlkdebugState),
 .is_filter  = true,
+.runtime_opts   = _opts,
 
 .bdrv_parse_filename= blkdebug_parse_filename,
 .bdrv_file_open = blkdebug_open,
diff --git a/block/crypto.c b/block/crypto.c
index bc322b50f5..5da97799b2 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -696,6 +696,7 @@ BlockDriver bdrv_crypto_luks = {
 .bdrv_co_create_opts = block_crypto_co_create_opts_luks,
 .bdrv_truncate  = block_crypto_truncate,
 .create_opts= _crypto_create_opts_luks,
+.runtime_opts   = _crypto_runtime_opts_luks,
 
 .bdrv_reopen_prepare = block_crypto_reopen_prepare,
 .bdrv_refresh_limits = block_crypto_refresh_limits,
diff --git a/block/file-posix.c b/block/file-posix.c
index 1511a4d69d..d6fe8d17cf 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -429,6 +429,8 @@ static QemuOptsList raw_runtime_opts = {
 },
 };
 
+static const char *const mutable_opts[] = { "x-check-cache-dropped", NULL };
+
 static int raw_open_common(BlockDriverState *bs, QDict *options,
int bdrv_flags, int open_flags, Error **errp)
 {
@@ -2582,6 +2584,8 @@ BlockDriver bdrv_file = {
 .format_name = "file",
 .protocol_name = "file",
 .instance_size = sizeof(BDRVRawState),
+.runtime_opts = _runtime_opts,
+.mutable_opts = mutable_opts,
 .bdrv_needs_filename = true,
 .bdrv_probe = NULL, /* no probe for protocols */
 .bdrv_parse_filename = raw_parse_filename,
@@ -3064,6 +3068,8 @@ static BlockDriver bdrv_host_device = {
 .format_name= "host_device",
 .protocol_name= "host_device",
 .instance_size  = sizeof(BDRVRawState),
+.runtime_opts = _runtime_opts,
+.mutable_opts = mutable_opts,
 .bdrv_needs_filename = true,
 .bdrv_probe_device  = hdev_probe_device,
 .bdrv_parse_filename = hdev_parse_filename,
@@ -3189,6 +3195,8 @@ static BlockDriver bdrv_host_cdrom = {
 .format_name= "host_cdrom",
 .protocol_name  = "host_cdrom",
 .instance_size  = sizeof(BDRVRawState),
+.runtime_opts = _runtime_opts,
+.mutable_opts = mutable_opts,
 .bdrv_needs_filename = true,
 .bdrv_probe_device = cdrom_probe_device,
 .bdrv_parse_filename = cdrom_parse_filename,
@@ -3321,6 +3329,8 @@ static BlockDriver bdrv_host_cdrom = {
 .format_name= "host_cdrom",
 .protocol_name  = "host_cdrom",
 .instance_size  = sizeof(BDRVRawState),
+.runtime_opts = _runtime_opts,
+.mutable_opts = mutable_opts,
 .bdrv_needs_filename = true,
 .bdrv_probe_device = cdrom_probe_device,
 .bdrv_parse_filename = cdrom_parse_filename,
diff --git a/block/iscsi.c b/block/iscsi.c
index c2fbd8a8aa..5f68d11176 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2443,6 +2443,7 @@ static BlockDriver bdrv_iscsi = {
 .bdrv_close = iscsi_close,
 .bdrv_co_create_opts= iscsi_co_create_opts,
 .create_opts= _create_opts,
+.runtime_opts   = _opts,
 .bdrv_reopen_prepare= iscsi_reopen_prepare,
 .bdrv_reopen_commit = iscsi_reopen_commit,
 .bdrv_co_invalidate_cache = iscsi_co_invalidate_cache,
@@ -2480,6 +2481,7 @@ static BlockDriver bdrv_iser = {
 .bdrv_close = iscsi_close,
 .bdrv_co_create_opts= iscsi_co_create_opts,
 .create_opts   

[Qemu-block] [RFC PATCH 10/10] qemu-iotests: Test the x-blockdev-reopen QMP command

[Alberto Garcia]

This patch adds several tests for the x-blockdev-reopen QMP command.

Signed-off-by: Alberto Garcia 
---
 tests/qemu-iotests/220 | 724 +
 tests/qemu-iotests/220.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 730 insertions(+)
 create mode 100755 tests/qemu-iotests/220
 create mode 100644 tests/qemu-iotests/220.out

diff --git a/tests/qemu-iotests/220 b/tests/qemu-iotests/220
new file mode 100755
index 00..ac737845d8
--- /dev/null
+++ b/tests/qemu-iotests/220
@@ -0,0 +1,724 @@
+#!/usr/bin/env python
+#
+# Test cases for the QMP 'x-blockdev-reopen' command
+#
+# Copyright (C) 2018 Igalia, S.L.
+# Author: Alberto Garcia 
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+import os
+import re
+import iotests
+import copy
+import json
+from iotests import qemu_img, qemu_io
+
+hd_path = [
+os.path.join(iotests.test_dir, 'hd0.img'),
+os.path.join(iotests.test_dir, 'hd1.img'),
+os.path.join(iotests.test_dir, 'hd2.img')
+]
+
+def hd_opts(idx):
+return {'driver': iotests.imgfmt,
+'node-name': 'hd%d' % idx,
+'file': {'driver': 'file',
+ 'node-name': 'hd%d-file' % idx,
+ 'filename':  hd_path[idx] } }
+
+class TestBlockdevReopen(iotests.QMPTestCase):
+total_io_cmds = 0
+
+def setUp(self):
+qemu_img('create', '-f', iotests.imgfmt, hd_path[0], '3M')
+qemu_img('create', '-f', iotests.imgfmt, '-b', hd_path[0], hd_path[1])
+qemu_img('create', '-f', iotests.imgfmt, hd_path[2], '3M')
+qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xa0  0 1M', hd_path[0])
+qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xa1 1M 1M', hd_path[1])
+qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xa2 2M 1M', hd_path[2])
+self.vm = iotests.VM()
+self.vm.launch()
+
+def tearDown(self):
+self.vm.shutdown()
+self.check_qemu_io_errors()
+os.remove(hd_path[0])
+os.remove(hd_path[1])
+os.remove(hd_path[2])
+
+# The output of qemu-io is not returned by vm.hmp_qemu_io() but
+# it's stored in the log and can only be read when the VM has been
+# shut down. This function runs qemu-io and keeps track of the
+# number of times it's been called.
+def run_qemu_io(self, img, cmd):
+result = self.vm.hmp_qemu_io(img, cmd)
+self.assert_qmp(result, 'return', '')
+self.total_io_cmds += 1
+
+# Once the VM is shut down we can parse the log and see if qemu-io
+# ran without errors.
+def check_qemu_io_errors(self):
+self.assertFalse(self.vm.is_running())
+found = 0
+log = self.vm.get_log()
+for line in log.split("\n"):
+if line.startswith("Pattern verification failed"):
+raise Exception("%s (command #%d)" % (line, found))
+if re.match("read .*/.* bytes at offset", line):
+found += 1
+self.assertEqual(found, self.total_io_cmds,
+ "Expected output of %d qemu-io commands, found %d" %
+ (found, self.total_io_cmds))
+
+# Run x-blockdev-reopen with 'opts' but applying 'newopts'
+# on top of it. The original 'opts' dict is unmodified
+def reopen(self, opts, newopts = {}, errmsg = None):
+opts = copy.deepcopy(opts)
+
+# Apply the changes from 'newopts' on top of 'opts'
+for key in newopts:
+value = newopts[key]
+# If key has the form "foo.bar" then we need to do
+# opts["foo"]["bar"] = value, not opts["foo.bar"] = value
+subdict = opts
+while key.find('.') != -1:
+[prefix, key] = key.split('.', 1)
+subdict = opts[prefix]
+subdict[key] = value
+
+result = self.vm.qmp('x-blockdev-reopen', conv_keys = False, **opts)
+if errmsg:
+self.assert_qmp(result, 'error/class', 'GenericError')
+self.assert_qmp(result, 'error/desc', errmsg)
+else:
+self.assert_qmp(result, 'return', {})
+
+
+# Run query-named-block-nodes and return the specified entry
+def get_node(self, node_name):
+result = self.vm.qmp('query-named-block-nodes')
+for node in result['return']:
+if node['node-name'] == node_name:
+

Re: [Qemu-block] [PATCH v5 11/14] job: Add job_progress_increase_remaining()

[Kevin Wolf]

Am 13.06.2018 um 20:18 hat Max Reitz geschrieben:
> Signed-off-by: Max Reitz 

Reviewed-by: Kevin Wolf 

[Qemu-block] [PATCH v2 09/18] block: Make remaining uses of qobject input visitor more robust

[Markus Armbruster]

Remaining uses of qobject_input_visitor_new_keyval() in the block
subsystem:

* block_crypto_create_opts_init()
  Currently doesn't visit any non-string scalars, thus safe.  It's
  called from
  - block_crypto_open_luks()
Creates the QDict with qemu_opts_to_qdict_filtered(), which
creates only string scalars, but has a TODO asking for other types.
  - qcow_open()
  - qcow2_open(), qcow2_co_invalidate_cache(), qcow2_reopen_prepare()

* block_crypto_create_opts_init(), called from
  - block_crypto_co_create_opts_luks()
Also creates the QDict with qemu_opts_to_qdict_filtered().

* vdi_co_create_opts()
  Also creates the QDict with qemu_opts_to_qdict_filtered().

Replace these uses by qobject_input_visitor_new_flat_confused() for
robustness.  This adds crumpling.  Right now, that's a no-op, but if
we ever extend these things in non-flat ways, crumpling will be
needed.

Signed-off-by: Markus Armbruster 
---
 block/crypto.c | 12 +---
 block/vdi.c|  8 ++--
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/block/crypto.c b/block/crypto.c
index bc322b50f5..82091c5f70 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -21,11 +21,11 @@
 #include "qemu/osdep.h"
 
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "crypto/block.h"
 #include "qapi/opts-visitor.h"
 #include "qapi/qapi-visit-crypto.h"
-#include "qapi/qmp/qdict.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/error.h"
 #include "qemu/option.h"
@@ -159,7 +159,10 @@ block_crypto_open_opts_init(QCryptoBlockFormat format,
 ret = g_new0(QCryptoBlockOpenOptions, 1);
 ret->format = format;
 
-v = qobject_input_visitor_new_keyval(QOBJECT(opts));
+v = qobject_input_visitor_new_flat_confused(opts, _err);
+if (local_err) {
+goto out;
+}
 
 visit_start_struct(v, NULL, NULL, 0, _err);
 if (local_err) {
@@ -210,7 +213,10 @@ block_crypto_create_opts_init(QCryptoBlockFormat format,
 ret = g_new0(QCryptoBlockCreateOptions, 1);
 ret->format = format;
 
-v = qobject_input_visitor_new_keyval(QOBJECT(opts));
+v = qobject_input_visitor_new_flat_confused(opts, _err);
+if (local_err) {
+goto out;
+}
 
 visit_start_struct(v, NULL, NULL, 0, _err);
 if (local_err) {
diff --git a/block/vdi.c b/block/vdi.c
index 668af0a828..1d8ed67dbf 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -51,10 +51,10 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
-#include "qapi/qmp/qdict.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-visit-block-core.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
@@ -934,7 +934,11 @@ static int coroutine_fn vdi_co_create_opts(const char 
*filename, QemuOpts *opts,
 }
 
 /* Get the QAPI object */
-v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
+v = qobject_input_visitor_new_flat_confused(qdict, errp);
+if (!v) {
+ret = -EINVAL;
+goto done;
+}
 visit_type_BlockdevCreateOptions(v, NULL, _options, _err);
 visit_free(v);
 
-- 
2.17.1

[Qemu-block] [PATCH v2 01/18] rbd: Drop deprecated -drive parameter "filename"

[Markus Armbruster]

Parameter "filename" is deprecated since commit 91589d9e5ca, v2.10.0.
Time to get rid of it.

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block/rbd.c | 16 
 1 file changed, 16 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index a16431e267..40c6e4185f 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -632,25 +632,9 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict 
*options, int flags,
 QObject *crumpled = NULL;
 const QDictEntry *e;
 Error *local_err = NULL;
-const char *filename;
 char *keypairs, *secretid;
 int r;
 
-/* If we are given a filename, parse the filename, with precedence given to
- * filename encoded options */
-filename = qdict_get_try_str(options, "filename");
-if (filename) {
-warn_report("'filename' option specified. "
-"This is an unsupported option, and may be deprecated "
-"in the future");
-qemu_rbd_parse_filename(filename, options, _err);
-qdict_del(options, "filename");
-if (local_err) {
-error_propagate(errp, local_err);
-return -EINVAL;
-}
-}
-
 keypairs = g_strdup(qdict_get_try_str(options, "=keyvalue-pairs"));
 if (keypairs) {
 qdict_del(options, "=keyvalue-pairs");
-- 
2.17.1

[Qemu-block] [PATCH v2 13/18] block-qdict: Simplify qdict_is_list() some

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 qobject/block-qdict.c | 27 +++
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index 36cf58acc8..e51a3d2c0f 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -317,27 +317,22 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
 
 for (ent = qdict_first(maybe_list); ent != NULL;
  ent = qdict_next(maybe_list, ent)) {
+int is_index = !qemu_strtoi64(ent->key, NULL, 10, );
 
-if (qemu_strtoi64(ent->key, NULL, 10, ) == 0) {
-if (is_list == -1) {
-is_list = 1;
-} else if (!is_list) {
-error_setg(errp,
-   "Cannot mix list and non-list keys");
-return -1;
-}
+if (is_list == -1) {
+is_list = is_index;
+}
+
+if (is_index != is_list) {
+error_setg(errp, "Cannot mix list and non-list keys");
+return -1;
+}
+
+if (is_index) {
 len++;
 if (val > max) {
 max = val;
 }
-} else {
-if (is_list == -1) {
-is_list = 0;
-} else if (is_list) {
-error_setg(errp,
-   "Cannot mix list and non-list keys");
-return -1;
-}
 }
 }
 
-- 
2.17.1

[Qemu-block] [PATCH v2 05/18] block: Fix -blockdev for certain non-string scalars

[Markus Armbruster]

Configuration flows through the block subsystem in a rather peculiar
way.  Configuration made with -drive enters it as QemuOpts.
Configuration made with -blockdev / blockdev-add enters it as QAPI
type BlockdevOptions.  The block subsystem uses QDict, QemuOpts and
QAPI types internally.  The precise flow is next to impossible to
explain (I tried for this commit message, but gave up after wasting
several hours).  What I can explain is a flaw in the BlockDriver
interface that leads to this bug:

$ qemu-system-x86_64 -blockdev 
node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234
qemu-system-x86_64: -blockdev 
node-name=n1,driver=nfs,server.type=inet,server.host=localhost,path=/foo/bar,user=1234:
 Internal error: parameter user invalid

QMP blockdev-add is broken the same way.

Here's what happens.  The block layer passes configuration represented
as flat QDict (with dotted keys) to BlockDriver methods
.bdrv_file_open().  The QDict's members are typed according to the
QAPI schema.

nfs_file_open() converts it to QAPI type BlockdevOptionsNfs, with
qdict_crumple() and a qobject input visitor.

This visitor comes in two flavors.  The plain flavor requires scalars
to be typed according to the QAPI schema.  That's the case here.  The
keyval flavor requires string scalars.  That's not the case here.
nfs_file_open() uses the latter, and promptly falls apart for members
@user, @group, @tcp-syn-count, @readahead-size, @page-cache-size,
@debug.

Switching to the plain flavor would fix -blockdev, but break -drive,
because there the scalars arrive in nfs_file_open() as strings.

The proper fix would be to replace the QDict by QAPI type
BlockdevOptions in the BlockDriver interface.  Sadly, that's beyond my
reach right now.

Next best would be to fix the block layer to always pass correctly
typed QDicts to the BlockDriver methods.  Also beyond my reach.

What I can do is throw another hack onto the pile: have
nfs_file_open() convert all members to string, so use of the keyval
flavor actually works, by replacing qdict_crumple() by new function
qdict_crumple_for_keyval_qiv().

The pattern "pass result of qdict_crumple() to
qobject_input_visitor_new_keyval()" occurs several times more:

* qemu_rbd_open()

  Same issue as nfs_file_open(), but since BlockdevOptionsRbd has only
  string members, its only a latent bug.  Fix it anyway.

* parallels_co_create_opts(), qcow_co_create_opts(),
  qcow2_co_create_opts(), bdrv_qed_co_create_opts(),
  sd_co_create_opts(), vhdx_co_create_opts(), vpc_co_create_opts()

  These work, because they create the QDict with
  qemu_opts_to_qdict_filtered(), which creates only string scalars.
  The function sports a TODO comment asking for better typing; that's
  going to be fun.  Use qdict_crumple_for_keyval_qiv() to be safe.

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block/nfs.c   |  2 +-
 block/parallels.c |  2 +-
 block/qcow.c  |  2 +-
 block/qcow2.c |  2 +-
 block/qed.c   |  2 +-
 block/rbd.c   |  2 +-
 block/sheepdog.c  |  2 +-
 block/vhdx.c  |  2 +-
 block/vpc.c   |  2 +-
 include/block/qdict.h |  1 +
 qobject/block-qdict.c | 57 +++
 11 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/block/nfs.c b/block/nfs.c
index 3170b059b3..6935b611cc 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -561,7 +561,7 @@ static BlockdevOptionsNfs *nfs_options_qdict_to_qapi(QDict 
*options,
 const QDictEntry *e;
 Error *local_err = NULL;
 
-crumpled = qdict_crumple(options, errp);
+crumpled = qdict_crumple_for_keyval_qiv(options, errp);
 if (crumpled == NULL) {
 return NULL;
 }
diff --git a/block/parallels.c b/block/parallels.c
index c1d9643498..695899fc4b 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -653,7 +653,7 @@ static int coroutine_fn parallels_co_create_opts(const char 
*filename,
 qdict_put_str(qdict, "driver", "parallels");
 qdict_put_str(qdict, "file", bs->node_name);
 
-qobj = qdict_crumple(qdict, errp);
+qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
 qobject_unref(qdict);
 qdict = qobject_to(QDict, qobj);
 if (qdict == NULL) {
diff --git a/block/qcow.c b/block/qcow.c
index 8c08908fd8..860b058240 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -997,7 +997,7 @@ static int coroutine_fn qcow_co_create_opts(const char 
*filename,
 qdict_put_str(qdict, "driver", "qcow");
 qdict_put_str(qdict, "file", bs->node_name);
 
-qobj = qdict_crumple(qdict, errp);
+qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
 qobject_unref(qdict);
 qdict = qobject_to(QDict, qobj);
 if (qdict == NULL) {
diff --git a/block/qcow2.c b/block/qcow2.c
index d2d955f984..0a27afa2ef 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3152,7 +3152,7 @@ static int coroutine_fn qcow2_co_create_opts(const char 
*filename, QemuOpts *opt
 qdict_put_str(qdict, 

[Qemu-block] [PATCH v2 17/18] rbd: New parameter auth-client-required

[Markus Armbruster]

Parameter auth-client-required lets you configure authentication
methods.  We tried to provide that in v2.9.0, but backed out due to
interface design doubts (commit 46fcc16).

This commit is similar to what we backed out, but simpler: we use a
list of enumeration values instead of a list of objects with a member
of enumeration type.

Let's review our reasons for backing out the first try, as stated in
the commit message:

* The implementation uses deprecated rados_conf_set() key
  "auth_supported".  No biggie.

Fixed: we use "auth-client-required".

* The implementation makes -drive silently ignore invalid parameters
  "auth" and "auth-supported.*.X" where X isn't "auth".  Fixable (in
  fact I'm going to fix similar bugs around parameter server), so
  again no biggie.

That fix is commit 2836284db60.  This commit doesn't bring the bugs
back.

* BlockdevOptionsRbd member @password-secret applies only to
  authentication method cephx.  Should it be a variant member of
  RbdAuthMethod?

We've had time to ponder, and we decided to stick to the way Ceph
configuration works: the key configured separately, and silently
ignored if the authentication method doesn't use it.

* BlockdevOptionsRbd member @user could apply to both methods cephx
  and none, but I'm not sure it's actually used with none.  If it
  isn't, should it be a variant member of RbdAuthMethod?

Likewise.

* The client offers a *set* of authentication methods, not a list.
  Should the methods be optional members of BlockdevOptionsRbd instead
  of members of list @auth-supported?  The latter begs the question
  what multiple entries for the same method mean.  Trivial question
  now that RbdAuthMethod contains nothing but @type, but less so when
  RbdAuthMethod acquires other members, such the ones discussed above.

Again, we decided to stick to the way Ceph configuration works, except
we make auth-client-required a list of enumeration values instead of a
string containing keywords separated by delimiters.

* How BlockdevOptionsRbd member @auth-supported interacts with
  settings from a configuration file specified with @conf is
  undocumented.  I suspect it's untested, too.

Not actually true, the documentation for @conf says "Values in the
configuration file will be overridden by options specified via QAPI",
and we've tested this.

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block/rbd.c  | 42 --
 qapi/block-core.json | 13 +
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 82346a2a5e..ea0575d068 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -240,20 +240,42 @@ static void qemu_rbd_refresh_limits(BlockDriverState *bs, 
Error **errp)
 
 
 static int qemu_rbd_set_auth(rados_t cluster, const char *secretid,
+ BlockdevOptionsRbd *opts,
  Error **errp)
 {
-if (secretid == 0) {
-return 0;
-}
+char *acr;
+int r;
+GString *accu;
+RbdAuthModeList *auth;
+
+if (secretid) {
+gchar *secret = qcrypto_secret_lookup_as_base64(secretid,
+errp);
+if (!secret) {
+return -1;
+}
 
-gchar *secret = qcrypto_secret_lookup_as_base64(secretid,
-errp);
-if (!secret) {
-return -1;
+rados_conf_set(cluster, "key", secret);
+g_free(secret);
 }
 
-rados_conf_set(cluster, "key", secret);
-g_free(secret);
+if (opts->has_auth_client_required) {
+accu = g_string_new("");
+for (auth = opts->auth_client_required; auth; auth = auth->next) {
+if (accu->str[0]) {
+g_string_append_c(accu, ';');
+}
+g_string_append(accu, RbdAuthMode_str(auth->value));
+}
+acr = g_string_free(accu, FALSE);
+r = rados_conf_set(cluster, "auth_client_required", acr);
+g_free(acr);
+if (r < 0) {
+error_setg_errno(errp, -r,
+ "Could not set 'auth_client_required'");
+return r;
+}
+}
 
 return 0;
 }
@@ -585,7 +607,7 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t 
*io_ctx,
 }
 }
 
-if (qemu_rbd_set_auth(*cluster, secretid, errp) < 0) {
+if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) {
 r = -EIO;
 goto failed_shutdown;
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index fff23fc82b..0f68ca56f3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3177,6 +3177,14 @@
 '*timeout': 'int' } }
 
 
+##
+# @RbdAuthMode:
+#
+# Since: 3.0
+##
+{ 'enum': 'RbdAuthMode',
+  'data': [ 'cephx', 'none' ] }
+
 ##
 # @BlockdevOptionsRbd:
 #
@@ -3192,6 +3200,10 @@
 #
 # 

[Qemu-block] [PATCH v2 06/18] block: Fix -drive for certain non-string scalars

[Markus Armbruster]

The previous commit fixed -blockdev breakage due to misuse of the
qobject input visitor's keyval flavor in bdrv_file_open().  The commit
message explain why using the plain flavor would be just as wrong; it
would break -drive.  Turns out we break it in three places:
nbd_open(), sd_open() and ssh_file_open().  They are even marked
FIXME.  Example breakage:

$ qemu-system-x86 -drive 
node-name=n1,driver=nbd,server.type=inet,server.host=localhost,server.port=1234,server.numeric=off
qemu-system-x86: -drive 
node-name=n1,driver=nbd,server.type=inet,server.host=localhost,server.port=1234,server.numeric=off:
 Invalid parameter type for 'numeric', expected: boolean

Fix it the same way: replace qdict_crumple() by
qdict_crumple_for_keyval_qiv(), and switch from plain to the keyval
flavor.

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block/nbd.c  | 12 ++--
 block/sheepdog.c | 12 ++--
 block/ssh.c  | 12 ++--
 3 files changed, 6 insertions(+), 30 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index d6c4c4ddbc..614dd9fec0 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -273,20 +273,12 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict 
*options,
 goto done;
 }
 
-crumpled_addr = qdict_crumple(addr, errp);
+crumpled_addr = qdict_crumple_for_keyval_qiv(addr, errp);
 if (!crumpled_addr) {
 goto done;
 }
 
-/*
- * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
- * server.type=inet.  .to doesn't matter, it's ignored anyway.
- * That's because when @options come from -blockdev or
- * blockdev_add, members are typed according to the QAPI schema,
- * but when they come from -drive, they're all QString.  The
- * visitor expects the former.
- */
-iv = qobject_input_visitor_new(crumpled_addr);
+iv = qobject_input_visitor_new_keyval(crumpled_addr);
 visit_type_SocketAddress(iv, NULL, , _err);
 if (local_err) {
 error_propagate(errp, local_err);
diff --git a/block/sheepdog.c b/block/sheepdog.c
index a93f93d360..29e3e1eaaa 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -546,20 +546,12 @@ static SocketAddress *sd_server_config(QDict *options, 
Error **errp)
 
 qdict_extract_subqdict(options, , "server.");
 
-crumpled_server = qdict_crumple(server, errp);
+crumpled_server = qdict_crumple_for_keyval_qiv(server, errp);
 if (!crumpled_server) {
 goto done;
 }
 
-/*
- * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive
- * server.type=inet.  .to doesn't matter, it's ignored anyway.
- * That's because when @options come from -blockdev or
- * blockdev_add, members are typed according to the QAPI schema,
- * but when they come from -drive, they're all QString.  The
- * visitor expects the former.
- */
-iv = qobject_input_visitor_new(crumpled_server);
+iv = qobject_input_visitor_new_keyval(crumpled_server);
 visit_type_SocketAddress(iv, NULL, , _err);
 if (local_err) {
 error_propagate(errp, local_err);
diff --git a/block/ssh.c b/block/ssh.c
index eec37dd27c..bd85d989d5 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -623,20 +623,12 @@ static BlockdevOptionsSsh *ssh_parse_options(QDict 
*options, Error **errp)
 }
 
 /* Create the QAPI object */
-crumpled = qdict_crumple(options, errp);
+crumpled = qdict_crumple_for_keyval_qiv(options, errp);
 if (crumpled == NULL) {
 goto fail;
 }
 
-/*
- * FIXME .numeric, .to, .ipv4 or .ipv6 don't work with -drive.
- * .to doesn't matter, it's ignored anyway.
- * That's because when @options come from -blockdev or
- * blockdev_add, members are typed according to the QAPI schema,
- * but when they come from -drive, they're all QString.  The
- * visitor expects the former.
- */
-v = qobject_input_visitor_new(crumpled);
+v = qobject_input_visitor_new_keyval(crumpled);
 visit_type_BlockdevOptionsSsh(v, NULL, , _err);
 visit_free(v);
 qobject_unref(crumpled);
-- 
2.17.1

[Qemu-block] [PATCH v2 11/18] block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist()

[Markus Armbruster]

qdict_flatten_qdict() skips copying scalars from @qdict to @target
when the two are the same.  Fair enough, but it uses a non-obvious
test for "same".  Replace it by the obvious one.  While there, improve
comments.

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 qobject/block-qdict.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index f32df343e8..a4e1c8d08f 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -71,12 +71,15 @@ static void qdict_flatten_qlist(QList *qlist, QDict 
*target, const char *prefix)
 value = qlist_entry_obj(entry);
 new_key = g_strdup_printf("%s.%i", prefix, i);
 
+/*
+ * Flatten non-empty QDict and QList recursively into @target,
+ * copy other objects to @target
+ */
 if (qobject_type(value) == QTYPE_QDICT) {
 qdict_flatten_qdict(qobject_to(QDict, value), target, new_key);
 } else if (qobject_type(value) == QTYPE_QLIST) {
 qdict_flatten_qlist(qobject_to(QList, value), target, new_key);
 } else {
-/* All other types are moved to the target unchanged. */
 qdict_put_obj(target, new_key, qobject_ref(value));
 }
 
@@ -101,9 +104,11 @@ static void qdict_flatten_qdict(QDict *qdict, QDict 
*target, const char *prefix)
 new_key = g_strdup_printf("%s.%s", prefix, entry->key);
 }
 
+/*
+ * Flatten non-empty QDict and QList recursively into @target,
+ * copy other objects to @target
+ */
 if (qobject_type(value) == QTYPE_QDICT) {
-/* Entries of QDicts are processed recursively, the QDict object
- * itself disappears. */
 qdict_flatten_qdict(qobject_to(QDict, value), target,
 new_key ? new_key : entry->key);
 qdict_del(qdict, entry->key);
@@ -111,8 +116,7 @@ static void qdict_flatten_qdict(QDict *qdict, QDict 
*target, const char *prefix)
 qdict_flatten_qlist(qobject_to(QList, value), target,
 new_key ? new_key : entry->key);
 qdict_del(qdict, entry->key);
-} else if (prefix) {
-/* All other objects are moved to the target unchanged. */
+} else if (target != qdict) {
 qdict_put_obj(target, new_key, qobject_ref(value));
 qdict_del(qdict, entry->key);
 }
-- 
2.17.1

[Qemu-block] [PATCH v2 16/18] block: Fix -blockdev / blockdev-add for empty objects and arrays

[Markus Armbruster]

-blockdev and blockdev-add silently ignore empty objects and arrays in
their argument.  That's because qmp_blockdev_add() converts the
argument to a flat QDict, and qdict_flatten() eats empty QDict and
QList members.  For instance, we ignore an empty BlockdevOptions
member @cache.  No real harm, as absent means the same as empty there.

Thus, the flaw puts an artificial restriction on the QAPI schema: we
can't have potentially empty objects and arrays within
BlockdevOptions, except when they're optional and "empty" has the same
meaning as "absent".

Our QAPI schema satisfies this restriction (I checked), but it's a
trap for the unwary, and a temptation to employ awkward workarounds
for the wary.  Let's get rid of it.

Change qdict_flatten() and qdict_crumple() to treat empty dictionaries
and lists exactly like scalars.

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 qobject/block-qdict.c | 54 ---
 tests/check-block-qdict.c | 38 +--
 2 files changed, 63 insertions(+), 29 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index e51a3d2c0f..df833083a7 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -56,6 +56,8 @@ static void qdict_flatten_qlist(QList *qlist, QDict *target, 
const char *prefix)
 {
 QObject *value;
 const QListEntry *entry;
+QDict *dict_val;
+QList *list_val;
 char *new_key;
 int i;
 
@@ -69,16 +71,18 @@ static void qdict_flatten_qlist(QList *qlist, QDict 
*target, const char *prefix)
 
 for (i = 0; entry; entry = qlist_next(entry), i++) {
 value = qlist_entry_obj(entry);
+dict_val = qobject_to(QDict, value);
+list_val = qobject_to(QList, value);
 new_key = g_strdup_printf("%s.%i", prefix, i);
 
 /*
  * Flatten non-empty QDict and QList recursively into @target,
  * copy other objects to @target
  */
-if (qobject_type(value) == QTYPE_QDICT) {
-qdict_flatten_qdict(qobject_to(QDict, value), target, new_key);
-} else if (qobject_type(value) == QTYPE_QLIST) {
-qdict_flatten_qlist(qobject_to(QList, value), target, new_key);
+if (dict_val && qdict_size(dict_val)) {
+qdict_flatten_qdict(dict_val, target, new_key);
+} else if (list_val && !qlist_empty(list_val)) {
+qdict_flatten_qlist(list_val, target, new_key);
 } else {
 qdict_put_obj(target, new_key, qobject_ref(value));
 }
@@ -91,6 +95,8 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, 
const char *prefix)
 {
 QObject *value;
 const QDictEntry *entry, *next;
+QDict *dict_val;
+QList *list_val;
 char *new_key;
 
 entry = qdict_first(qdict);
@@ -98,6 +104,8 @@ static void qdict_flatten_qdict(QDict *qdict, QDict *target, 
const char *prefix)
 while (entry != NULL) {
 next = qdict_next(qdict, entry);
 value = qdict_entry_value(entry);
+dict_val = qobject_to(QDict, value);
+list_val = qobject_to(QList, value);
 new_key = NULL;
 
 if (prefix) {
@@ -108,12 +116,12 @@ static void qdict_flatten_qdict(QDict *qdict, QDict 
*target, const char *prefix)
  * Flatten non-empty QDict and QList recursively into @target,
  * copy other objects to @target
  */
-if (qobject_type(value) == QTYPE_QDICT) {
-qdict_flatten_qdict(qobject_to(QDict, value), target,
+if (dict_val && qdict_size(dict_val)) {
+qdict_flatten_qdict(dict_val, target,
 new_key ? new_key : entry->key);
 qdict_del(qdict, entry->key);
-} else if (qobject_type(value) == QTYPE_QLIST) {
-qdict_flatten_qlist(qobject_to(QList, value), target,
+} else if (list_val && !qlist_empty(list_val)) {
+qdict_flatten_qlist(list_val, target,
 new_key ? new_key : entry->key);
 qdict_del(qdict, entry->key);
 } else if (target != qdict) {
@@ -127,10 +135,11 @@ static void qdict_flatten_qdict(QDict *qdict, QDict 
*target, const char *prefix)
 }
 
 /**
- * qdict_flatten(): For each nested QDict with key x, all fields with key y
- * are moved to this QDict and their key is renamed to "x.y". For each nested
- * QList with key x, the field at index y is moved to this QDict with the key
- * "x.y" (i.e., the reverse of what qdict_array_split() does).
+ * qdict_flatten(): For each nested non-empty QDict with key x, all
+ * fields with key y are moved to this QDict and their key is renamed
+ * to "x.y". For each nested non-empty QList with key x, the field at
+ * index y is moved to this QDict with the key "x.y" (i.e., the
+ * reverse of what qdict_array_split() does).
  * This operation is applied recursively for nested QDicts and QLists.
  */
 void qdict_flatten(QDict *qdict)
@@ -361,8 +370,8 @@ static int qdict_is_list(QDict 

[Qemu-block] [PATCH v2 12/18] block-qdict: Clean up qdict_crumple() a bit

[Markus Armbruster]

When you mix scalar and non-scalar keys, whether you get an "already
set as scalar" or an "already set as dict" error depends on qdict
iteration order.  Neither message makes much sense.  Replace by
""Cannot mix scalar and non-scalar keys".  This is similar to the
message we get for mixing list and non-list keys.

I find qdict_crumple()'s first loop hard to understand.  Rearrange it
and add a comment.

Signed-off-by: Markus Armbruster 
---
 qobject/block-qdict.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
index a4e1c8d08f..36cf58acc8 100644
--- a/qobject/block-qdict.c
+++ b/qobject/block-qdict.c
@@ -403,7 +403,7 @@ static int qdict_is_list(QDict *maybe_list, Error **errp)
 QObject *qdict_crumple(const QDict *src, Error **errp)
 {
 const QDictEntry *ent;
-QDict *two_level, *multi_level = NULL;
+QDict *two_level, *multi_level = NULL, *child_dict;
 QObject *dst = NULL, *child;
 size_t i;
 char *prefix = NULL;
@@ -422,28 +422,28 @@ QObject *qdict_crumple(const QDict *src, Error **errp)
 }
 
 qdict_split_flat_key(ent->key, , );
-
 child = qdict_get(two_level, prefix);
+child_dict = qobject_to(QDict, child);
+
+if (child) {
+/*
+ * If @child_dict, then all previous keys with this prefix
+ * had a suffix.  If @suffix, this one has one as well,
+ * and we're good, else there's a clash.
+ */
+if (!child_dict || !suffix) {
+error_setg(errp, "Cannot mix scalar and non-scalar keys");
+goto error;
+}
+}
+
 if (suffix) {
-QDict *child_dict = qobject_to(QDict, child);
 if (!child_dict) {
-if (child) {
-error_setg(errp, "Key %s prefix is already set as a 
scalar",
-   prefix);
-goto error;
-}
-
 child_dict = qdict_new();
-qdict_put_obj(two_level, prefix, QOBJECT(child_dict));
+qdict_put(two_level, prefix, child_dict);
 }
-
 qdict_put_obj(child_dict, suffix, qobject_ref(ent->value));
 } else {
-if (child) {
-error_setg(errp, "Key %s prefix is already set as a dict",
-   prefix);
-goto error;
-}
 qdict_put_obj(two_level, prefix, qobject_ref(ent->value));
 }
 
-- 
2.17.1

[Qemu-block] [PATCH v2 18/18] rbd: New parameter key-secret

[Markus Armbruster]

Legacy -drive supports "password-secret" parameter that isn't
available with -blockdev / blockdev-add.  That's because we backed out
our first try to provide it there due to interface design doubts, in
commit 577d8c9a811, v2.9.0.

This is the second try.  It brings back the parameter, except it's
named "key-secret" now.

Let's review our reasons for backing out the first try, as stated in
the commit message:

* BlockdevOptionsRbd member @password-secret isn't actually a
  password, it's a key generated by Ceph.

Addressed by the rename.

* We're not sure where member @password-secret belongs (see the
  previous commit).

See previous commit.

* How @password-secret interacts with settings from a configuration
  file specified with @conf is undocumented.

Not actually true, the documentation for @conf says "Values in the
configuration file will be overridden by options specified via QAPI",
and we've tested this.

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block/rbd.c  | 41 +
 qapi/block-core.json |  6 ++
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index ea0575d068..f2c6965418 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -239,24 +239,25 @@ static void qemu_rbd_refresh_limits(BlockDriverState *bs, 
Error **errp)
 }
 
 
-static int qemu_rbd_set_auth(rados_t cluster, const char *secretid,
- BlockdevOptionsRbd *opts,
+static int qemu_rbd_set_auth(rados_t cluster, BlockdevOptionsRbd *opts,
  Error **errp)
 {
-char *acr;
+char *key, *acr;
 int r;
 GString *accu;
 RbdAuthModeList *auth;
 
-if (secretid) {
-gchar *secret = qcrypto_secret_lookup_as_base64(secretid,
-errp);
-if (!secret) {
-return -1;
+if (opts->key_secret) {
+key = qcrypto_secret_lookup_as_base64(opts->key_secret, errp);
+if (!key) {
+return -EIO;
+}
+r = rados_conf_set(cluster, "key", key);
+g_free(key);
+if (r < 0) {
+error_setg_errno(errp, -r, "Could not set 'key'");
+return r;
 }
-
-rados_conf_set(cluster, "key", secret);
-g_free(secret);
 }
 
 if (opts->has_auth_client_required) {
@@ -367,9 +368,7 @@ static QemuOptsList runtime_opts = {
 },
 };
 
-/* FIXME Deprecate and remove keypairs or make it available in QMP.
- * password_secret should eventually be configurable in opts->location. Support
- * for it in .bdrv_open will make it work here as well. */
+/* FIXME Deprecate and remove keypairs or make it available in QMP. */
 static int qemu_rbd_do_create(BlockdevCreateOptions *options,
   const char *keypairs, const char 
*password_secret,
   Error **errp)
@@ -575,6 +574,16 @@ static int qemu_rbd_connect(rados_t *cluster, 
rados_ioctx_t *io_ctx,
 Error *local_err = NULL;
 int r;
 
+if (secretid) {
+if (opts->key_secret) {
+error_setg(errp,
+   "Legacy 'password-secret' clashes with 'key-secret'");
+return -EINVAL;
+}
+opts->key_secret = g_strdup(secretid);
+opts->has_key_secret = true;
+}
+
 mon_host = qemu_rbd_mon_host(opts, _err);
 if (local_err) {
 error_propagate(errp, local_err);
@@ -607,8 +616,8 @@ static int qemu_rbd_connect(rados_t *cluster, rados_ioctx_t 
*io_ctx,
 }
 }
 
-if (qemu_rbd_set_auth(*cluster, secretid, opts, errp) < 0) {
-r = -EIO;
+r = qemu_rbd_set_auth(*cluster, opts, errp);
+if (r < 0) {
 goto failed_shutdown;
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0f68ca56f3..ab629d1647 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3204,6 +3204,11 @@
 #  This maps to Ceph configuration option
 #  "auth_client_required".  (Since 3.0)
 #
+# @key-secret: ID of a QCryptoSecret object providing a key
+#  for cephx authentication.
+#  This maps to Ceph configuration option
+#  "key".  (Since 3.0)
+#
 # @server: Monitor host address and port.  This maps
 #  to the "mon_host" Ceph option.
 #
@@ -3216,6 +3221,7 @@
 '*snapshot': 'str',
 '*user': 'str',
 '*auth-client-required': ['RbdAuthMode'],
+'*key-secret': 'str',
 '*server': ['InetSocketAddressBase'] } }
 
 ##
-- 
2.17.1

[Qemu-block] [PATCH v2 08/18] block: Factor out qobject_input_visitor_new_flat_confused()

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block/nbd.c   |  7 ++-
 block/nfs.c   |  7 ++-
 block/parallels.c |  7 ++-
 block/qcow.c  |  7 ++-
 block/qcow2.c |  7 ++-
 block/qed.c   |  7 ++-
 block/rbd.c   |  7 ++-
 block/sheepdog.c  | 14 --
 block/ssh.c   |  7 ++-
 block/vhdx.c  |  7 ++-
 block/vpc.c   |  7 ++-
 include/block/qdict.h |  3 ++-
 qobject/block-qdict.c | 28 +++-
 13 files changed, 53 insertions(+), 62 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index 614dd9fec0..13db4030e6 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -263,7 +263,6 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict 
*options,
 {
 SocketAddress *saddr = NULL;
 QDict *addr = NULL;
-QObject *crumpled_addr = NULL;
 Visitor *iv = NULL;
 Error *local_err = NULL;
 
@@ -273,12 +272,11 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict 
*options,
 goto done;
 }
 
-crumpled_addr = qdict_crumple_for_keyval_qiv(addr, errp);
-if (!crumpled_addr) {
+iv = qobject_input_visitor_new_flat_confused(addr, errp);
+if (!iv) {
 goto done;
 }
 
-iv = qobject_input_visitor_new_keyval(crumpled_addr);
 visit_type_SocketAddress(iv, NULL, , _err);
 if (local_err) {
 error_propagate(errp, local_err);
@@ -287,7 +285,6 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict 
*options,
 
 done:
 qobject_unref(addr);
-qobject_unref(crumpled_addr);
 visit_free(iv);
 return saddr;
 }
diff --git a/block/nfs.c b/block/nfs.c
index 6935b611cc..743ca0450e 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -556,20 +556,17 @@ static BlockdevOptionsNfs 
*nfs_options_qdict_to_qapi(QDict *options,
  Error **errp)
 {
 BlockdevOptionsNfs *opts = NULL;
-QObject *crumpled = NULL;
 Visitor *v;
 const QDictEntry *e;
 Error *local_err = NULL;
 
-crumpled = qdict_crumple_for_keyval_qiv(options, errp);
-if (crumpled == NULL) {
+v = qobject_input_visitor_new_flat_confused(options, errp);
+if (!v) {
 return NULL;
 }
 
-v = qobject_input_visitor_new_keyval(crumpled);
 visit_type_BlockdevOptionsNfs(v, NULL, , _err);
 visit_free(v);
-qobject_unref(crumpled);
 
 if (local_err) {
 error_propagate(errp, local_err);
diff --git a/block/parallels.c b/block/parallels.c
index ceb7a15d62..fd215e202a 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -617,7 +617,6 @@ static int coroutine_fn parallels_co_create_opts(const char 
*filename,
 Error *local_err = NULL;
 BlockDriverState *bs = NULL;
 QDict *qdict;
-QObject *qobj;
 Visitor *v;
 int ret;
 
@@ -653,14 +652,12 @@ static int coroutine_fn parallels_co_create_opts(const 
char *filename,
 qdict_put_str(qdict, "driver", "parallels");
 qdict_put_str(qdict, "file", bs->node_name);
 
-qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-if (!qobj) {
+v = qobject_input_visitor_new_flat_confused(qdict, errp);
+if (!v) {
 ret = -EINVAL;
 goto done;
 }
 
-v = qobject_input_visitor_new_keyval(qobj);
-qobject_unref(qobj);
 visit_type_BlockdevCreateOptions(v, NULL, _options, _err);
 visit_free(v);
 
diff --git a/block/qcow.c b/block/qcow.c
index 2f81f081fd..5532731b9f 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -947,7 +947,6 @@ static int coroutine_fn qcow_co_create_opts(const char 
*filename,
 BlockdevCreateOptions *create_options = NULL;
 BlockDriverState *bs = NULL;
 QDict *qdict;
-QObject *qobj;
 Visitor *v;
 const char *val;
 Error *local_err = NULL;
@@ -997,14 +996,12 @@ static int coroutine_fn qcow_co_create_opts(const char 
*filename,
 qdict_put_str(qdict, "driver", "qcow");
 qdict_put_str(qdict, "file", bs->node_name);
 
-qobj = qdict_crumple_for_keyval_qiv(qdict, errp);
-if (!qobj) {
+v = qobject_input_visitor_new_flat_confused(qdict, errp);
+if (!v) {
 ret = -EINVAL;
 goto fail;
 }
 
-v = qobject_input_visitor_new_keyval(qobj);
-qobject_unref(qobj);
 visit_type_BlockdevCreateOptions(v, NULL, _options, _err);
 visit_free(v);
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 8c338661db..945132f692 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3081,7 +3081,6 @@ static int coroutine_fn qcow2_co_create_opts(const char 
*filename, QemuOpts *opt
 {
 BlockdevCreateOptions *create_options = NULL;
 QDict *qdict;
-QObject *qobj;
 Visitor *v;
 BlockDriverState *bs = NULL;
 Error *local_err = NULL;
@@ -3152,14 +3151,12 @@ static int coroutine_fn qcow2_co_create_opts(const char 
*filename, QemuOpts *opt
 qdict_put_str(qdict, "file", bs->node_name);
 
 /* Now get the QAPI type BlockdevCreateOptions */
-qobj = 

[Qemu-block] [PATCH v2 02/18] iscsi: Drop deprecated -drive parameter "filename"

[Markus Armbruster]

Parameter "filename" is deprecated since commit 5c3ad1a6a8f, v2.10.0.
Time to get rid of it.

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block/iscsi.c | 23 ++-
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/block/iscsi.c b/block/iscsi.c
index c2fbd8a8aa..7e3ea72bd2 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1713,10 +1713,6 @@ static QemuOptsList runtime_opts = {
 .name = "timeout",
 .type = QEMU_OPT_NUMBER,
 },
-{
-.name = "filename",
-.type = QEMU_OPT_STRING,
-},
 { /* end of list */ }
 },
 };
@@ -1756,27 +1752,12 @@ static int iscsi_open(BlockDriverState *bs, QDict 
*options, int flags,
 char *initiator_name = NULL;
 QemuOpts *opts;
 Error *local_err = NULL;
-const char *transport_name, *portal, *target, *filename;
+const char *transport_name, *portal, *target;
 #if LIBISCSI_API_VERSION >= (20160603)
 enum iscsi_transport_type transport;
 #endif
 int i, ret = 0, timeout = 0, lun;
 
-/* If we are given a filename, parse the filename, with precedence given to
- * filename encoded options */
-filename = qdict_get_try_str(options, "filename");
-if (filename) {
-warn_report("'filename' option specified. "
-"This is an unsupported option, and may be deprecated "
-"in the future");
-iscsi_parse_filename(filename, options, _err);
-if (local_err) {
-ret = -EINVAL;
-error_propagate(errp, local_err);
-goto exit;
-}
-}
-
 opts = qemu_opts_create(_opts, NULL, 0, _abort);
 qemu_opts_absorb_qdict(opts, options, _err);
 if (local_err) {
@@ -2006,7 +1987,7 @@ out:
 }
 memset(iscsilun, 0, sizeof(IscsiLun));
 }
-exit:
+
 return ret;
 }
 
-- 
2.17.1

[Qemu-block] [PATCH v2 14/18] check-block-qdict: Rename qdict_flatten()'s variables for clarity

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 tests/check-block-qdict.c | 57 ---
 1 file changed, 29 insertions(+), 28 deletions(-)

diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
index 5b9f4d506e..29f58a2d3d 100644
--- a/tests/check-block-qdict.c
+++ b/tests/check-block-qdict.c
@@ -37,11 +37,11 @@ static void qdict_defaults_test(void)
 
 static void qdict_flatten_test(void)
 {
-QList *list1 = qlist_new();
-QList *list2 = qlist_new();
-QDict *dict1 = qdict_new();
-QDict *dict2 = qdict_new();
-QDict *dict3 = qdict_new();
+QList *e_1 = qlist_new();
+QList *e = qlist_new();
+QDict *e_1_2 = qdict_new();
+QDict *f = qdict_new();
+QDict *root = qdict_new();
 
 /*
  * Test the flattening of
@@ -79,35 +79,36 @@ static void qdict_flatten_test(void)
  * }
  */
 
-qdict_put_int(dict1, "a", 0);
-qdict_put_int(dict1, "b", 1);
+qdict_put_int(e_1_2, "a", 0);
+qdict_put_int(e_1_2, "b", 1);
 
-qlist_append_int(list1, 23);
-qlist_append_int(list1, 66);
-qlist_append(list1, dict1);
-qlist_append_int(list2, 42);
-qlist_append(list2, list1);
+qlist_append_int(e_1, 23);
+qlist_append_int(e_1, 66);
+qlist_append(e_1, e_1_2);
+qlist_append_int(e, 42);
+qlist_append(e, e_1);
 
-qdict_put_int(dict2, "c", 2);
-qdict_put_int(dict2, "d", 3);
-qdict_put(dict3, "e", list2);
-qdict_put(dict3, "f", dict2);
-qdict_put_int(dict3, "g", 4);
+qdict_put_int(f, "c", 2);
+qdict_put_int(f, "d", 3);
 
-qdict_flatten(dict3);
+qdict_put(root, "e", e);
+qdict_put(root, "f", f);
+qdict_put_int(root, "g", 4);
 
-g_assert(qdict_get_int(dict3, "e.0") == 42);
-g_assert(qdict_get_int(dict3, "e.1.0") == 23);
-g_assert(qdict_get_int(dict3, "e.1.1") == 66);
-g_assert(qdict_get_int(dict3, "e.1.2.a") == 0);
-g_assert(qdict_get_int(dict3, "e.1.2.b") == 1);
-g_assert(qdict_get_int(dict3, "f.c") == 2);
-g_assert(qdict_get_int(dict3, "f.d") == 3);
-g_assert(qdict_get_int(dict3, "g") == 4);
+qdict_flatten(root);
 
-g_assert(qdict_size(dict3) == 8);
+g_assert(qdict_get_int(root, "e.0") == 42);
+g_assert(qdict_get_int(root, "e.1.0") == 23);
+g_assert(qdict_get_int(root, "e.1.1") == 66);
+g_assert(qdict_get_int(root, "e.1.2.a") == 0);
+g_assert(qdict_get_int(root, "e.1.2.b") == 1);
+g_assert(qdict_get_int(root, "f.c") == 2);
+g_assert(qdict_get_int(root, "f.d") == 3);
+g_assert(qdict_get_int(root, "g") == 4);
 
-qobject_unref(dict3);
+g_assert(qdict_size(root) == 8);
+
+qobject_unref(root);
 }
 
 static void qdict_array_split_test(void)
-- 
2.17.1

[Qemu-block] [PATCH v2 15/18] check-block-qdict: Cover flattening of empty lists and dictionaries

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 tests/check-block-qdict.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tests/check-block-qdict.c b/tests/check-block-qdict.c
index 29f58a2d3d..2da16f01a6 100644
--- a/tests/check-block-qdict.c
+++ b/tests/check-block-qdict.c
@@ -41,6 +41,8 @@ static void qdict_flatten_test(void)
 QList *e = qlist_new();
 QDict *e_1_2 = qdict_new();
 QDict *f = qdict_new();
+QList *y = qlist_new();
+QDict *z = qdict_new();
 QDict *root = qdict_new();
 
 /*
@@ -62,7 +64,9 @@ static void qdict_flatten_test(void)
  * "c": 2,
  * "d": 3,
  * },
- * "g": 4
+ * "g": 4,
+ * "y": [{}],
+ * "z": {"a": []}
  * }
  *
  * to
@@ -77,6 +81,8 @@ static void qdict_flatten_test(void)
  * "f.d": 3,
  * "g": 4
  * }
+ *
+ * Note that "y" and "z" get eaten.
  */
 
 qdict_put_int(e_1_2, "a", 0);
@@ -91,9 +97,15 @@ static void qdict_flatten_test(void)
 qdict_put_int(f, "c", 2);
 qdict_put_int(f, "d", 3);
 
+qlist_append(y, qdict_new());
+
+qdict_put(z, "a", qlist_new());
+
 qdict_put(root, "e", e);
 qdict_put(root, "f", f);
 qdict_put_int(root, "g", 4);
+qdict_put(root, "y", y);
+qdict_put(root, "z", z);
 
 qdict_flatten(root);
 
-- 
2.17.1

[Qemu-block] [PATCH v2 00/18] block: Configuration fixes and rbd authentication

[Markus Armbruster]

PATCH 01-17 are configuration fixes and cleanup, in particular
-blockdev driver=nfs,... and -drive driver=(nbd|sheepdog|ssh),... with
non-string scalars.

PATCH 18-19 provide support for configuring rbd authentication.

I'm happy to split the series if that helps.

Jeff Cody tested the RFC on his Ceph rig.  All results match
expectations.  Since code changes since then have been minimal, I
believe his test results remain valid.

v2:
* PATCH 03+05: Fix typos and pastos in commit message [Kevin]
* PATCH 09: Fix error handling [Kevin]
* PATCH 12: Redo for clarity [Kevin]

Markus Armbruster (17):
  rbd: Drop deprecated -drive parameter "filename"
  iscsi: Drop deprecated -drive parameter "filename"
  qobject: Move block-specific qdict code to block-qdict.c
  block: Fix -blockdev for certain non-string scalars
  block: Fix -drive for certain non-string scalars
  block: Clean up a misuse of qobject_to() in .bdrv_co_create_opts()
  block: Factor out qobject_input_visitor_new_flat_confused()
  block: Make remaining uses of qobject input visitor more robust
  block-qdict: Simplify qdict_flatten_qdict()
  block-qdict: Tweak qdict_flatten_qdict(), qdict_flatten_qlist()
  block-qdict: Clean up qdict_crumple() a bit
  block-qdict: Simplify qdict_is_list() some
  check-block-qdict: Rename qdict_flatten()'s variables for clarity
  check-block-qdict: Cover flattening of empty lists and dictionaries
  block: Fix -blockdev / blockdev-add for empty objects and arrays
  rbd: New parameter auth-client-required
  rbd: New parameter key-secret

Max Reitz (1):
  block: Add block-specific QDict header

 MAINTAINERS   |   2 +
 block.c   |   1 +
 block/crypto.c|  12 +-
 block/gluster.c   |   1 +
 block/iscsi.c |  24 +-
 block/nbd.c   |  16 +-
 block/nfs.c   |   8 +-
 block/parallels.c |  11 +-
 block/qcow.c  |  11 +-
 block/qcow2.c |  11 +-
 block/qed.c   |  11 +-
 block/quorum.c|   1 +
 block/rbd.c   |  85 +++--
 block/sheepdog.c  |  23 +-
 block/snapshot.c  |   1 +
 block/ssh.c   |  16 +-
 block/vdi.c   |   8 +-
 block/vhdx.c  |  11 +-
 block/vpc.c   |  11 +-
 block/vvfat.c |   1 +
 block/vxhs.c  |   1 +
 blockdev.c|   1 +
 include/block/qdict.h |  34 ++
 include/qapi/qmp/qdict.h  |  17 -
 qapi/block-core.json  |  19 +
 qobject/Makefile.objs |   1 +
 qobject/block-qdict.c | 722 ++
 qobject/qdict.c   | 628 -
 tests/Makefile.include|   4 +
 tests/check-block-qdict.c | 690 
 tests/check-qdict.c   | 641 -
 tests/check-qobject.c |   1 +
 tests/test-replication.c  |   1 +
 util/qemu-config.c|   1 +
 34 files changed, 1587 insertions(+), 1439 deletions(-)
 create mode 100644 include/block/qdict.h
 create mode 100644 qobject/block-qdict.c
 create mode 100644 tests/check-block-qdict.c

-- 
2.17.1

[Qemu-block] [PATCH v2 03/18] block: Add block-specific QDict header

[Markus Armbruster]

From: Max Reitz 

There are numerous QDict functions that have been introduced for and are
used only by the block layer.  Move their declarations into an own
header file to reflect that.

While qdict_extract_subqdict() is in fact used outside of the block
layer (in util/qemu-config.c), it is still a function related very
closely to how the block layer works with nested QDicts, namely by
sometimes flattening them.  Therefore, its declaration is put into this
header as well and util/qemu-config.c includes it with a comment stating
exactly which function it needs.

Suggested-by: Markus Armbruster 
Signed-off-by: Max Reitz 
Message-Id: <20180509165530.29561-7-mre...@redhat.com>
[Copyright note tweaked, superfluous includes dropped]
Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 block.c  |  1 +
 block/gluster.c  |  1 +
 block/iscsi.c|  1 +
 block/nbd.c  |  1 +
 block/nfs.c  |  1 +
 block/parallels.c|  1 +
 block/qcow.c |  1 +
 block/qcow2.c|  1 +
 block/qed.c  |  1 +
 block/quorum.c   |  1 +
 block/rbd.c  |  1 +
 block/sheepdog.c |  1 +
 block/snapshot.c |  1 +
 block/ssh.c  |  1 +
 block/vhdx.c |  1 +
 block/vpc.c  |  1 +
 block/vvfat.c|  1 +
 block/vxhs.c |  1 +
 blockdev.c   |  1 +
 include/block/qdict.h| 32 
 include/qapi/qmp/qdict.h | 17 -
 qobject/qdict.c  |  1 +
 tests/check-qdict.c  |  1 +
 tests/check-qobject.c|  1 +
 tests/test-replication.c |  1 +
 util/qemu-config.c   |  1 +
 26 files changed, 56 insertions(+), 17 deletions(-)
 create mode 100644 include/block/qdict.h

diff --git a/block.c b/block.c
index 50887087f3..afe30caac3 100644
--- a/block.c
+++ b/block.c
@@ -27,6 +27,7 @@
 #include "block/block_int.h"
 #include "block/blockjob.h"
 #include "block/nbd.h"
+#include "block/qdict.h"
 #include "qemu/error-report.h"
 #include "module_block.h"
 #include "qemu/module.h"
diff --git a/block/gluster.c b/block/gluster.c
index 9900b6420c..b5fe7f3e87 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -11,6 +11,7 @@
 #include "qemu/osdep.h"
 #include 
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qapi/error.h"
 #include "qapi/qmp/qdict.h"
 #include "qapi/qmp/qerror.h"
diff --git a/block/iscsi.c b/block/iscsi.c
index 7e3ea72bd2..9f00fb47a5 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -33,6 +33,7 @@
 #include "qemu/bitops.h"
 #include "qemu/bitmap.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "scsi/constants.h"
 #include "qemu/iov.h"
 #include "qemu/option.h"
diff --git a/block/nbd.c b/block/nbd.c
index ff8333e3c1..d6c4c4ddbc 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -28,6 +28,7 @@
 
 #include "qemu/osdep.h"
 #include "nbd-client.h"
+#include "block/qdict.h"
 #include "qapi/error.h"
 #include "qemu/uri.h"
 #include "block/block_int.h"
diff --git a/block/nfs.c b/block/nfs.c
index 3349b67a76..3170b059b3 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -29,6 +29,7 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "trace.h"
 #include "qemu/iov.h"
 #include "qemu/option.h"
diff --git a/block/parallels.c b/block/parallels.c
index 6e9c37f44e..c1d9643498 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -31,6 +31,7 @@
 #include "qemu/osdep.h"
 #include "qapi/error.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
diff --git a/block/qcow.c b/block/qcow.c
index 1f866af0d3..8c08908fd8 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -26,6 +26,7 @@
 #include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
diff --git a/block/qcow2.c b/block/qcow2.c
index 6fa5e1d71a..d2d955f984 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include 
diff --git a/block/qed.c b/block/qed.c
index 65cfe92393..324a953cbc 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -13,6 +13,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "block/qdict.h"
 #include "qapi/error.h"
 #include "qemu/timer.h"
 #include "qemu/bswap.h"
diff --git a/block/quorum.c b/block/quorum.c
index b6476c405a..9152da8c58 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -17,6 +17,7 @@
 #include "qemu/cutils.h"
 #include "qemu/option.h"
 #include "block/block_int.h"
+#include "block/qdict.h"
 #include "qapi/error.h"
 #include "qapi/qapi-events-block.h"
 #include "qapi/qmp/qdict.h"
diff --git a/block/rbd.c b/block/rbd.c
index 40c6e4185f..9659c7361f 100644

[Qemu-block] [PATCH v2 04/18] qobject: Move block-specific qdict code to block-qdict.c

[Markus Armbruster]

Pure code motion, except for two brace placements and a comment
tweaked to appease checkpatch.

Signed-off-by: Markus Armbruster 
Reviewed-by: Kevin Wolf 
---
 MAINTAINERS   |   2 +
 qobject/Makefile.objs |   1 +
 qobject/block-qdict.c | 640 +
 qobject/qdict.c   | 629 
 tests/Makefile.include|   4 +
 tests/check-block-qdict.c | 655 ++
 tests/check-qdict.c   | 642 -
 7 files changed, 1302 insertions(+), 1271 deletions(-)
 create mode 100644 qobject/block-qdict.c
 create mode 100644 tests/check-block-qdict.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8a94517e9e..0fb5f38f9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1369,6 +1369,8 @@ F: qemu-img*
 F: qemu-io*
 F: tests/qemu-iotests/
 F: util/qemu-progress.c
+F: qobject/block-qdict.c
+F: test/check-block-qdict.c
 T: git git://repo.or.cz/qemu/kevin.git block
 
 Block I/O path
diff --git a/qobject/Makefile.objs b/qobject/Makefile.objs
index 002d25873a..7b12c9cacf 100644
--- a/qobject/Makefile.objs
+++ b/qobject/Makefile.objs
@@ -1,2 +1,3 @@
 util-obj-y = qnull.o qnum.o qstring.o qdict.o qlist.o qbool.o qlit.o
 util-obj-y += qjson.o qobject.o json-lexer.o json-streamer.o json-parser.o
+util-obj-y += block-qdict.o
diff --git a/qobject/block-qdict.c b/qobject/block-qdict.c
new file mode 100644
index 00..fb92010dc5
--- /dev/null
+++ b/qobject/block-qdict.c
@@ -0,0 +1,640 @@
+/*
+ * Special QDict functions used by the block layer
+ *
+ * Copyright (c) 2013-2018 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "block/qdict.h"
+#include "qapi/qmp/qlist.h"
+#include "qemu/cutils.h"
+#include "qapi/error.h"
+
+/**
+ * qdict_copy_default(): If no entry mapped by 'key' exists in 'dst' yet, the
+ * value of 'key' in 'src' is copied there (and the refcount increased
+ * accordingly).
+ */
+void qdict_copy_default(QDict *dst, QDict *src, const char *key)
+{
+QObject *val;
+
+if (qdict_haskey(dst, key)) {
+return;
+}
+
+val = qdict_get(src, key);
+if (val) {
+qdict_put_obj(dst, key, qobject_ref(val));
+}
+}
+
+/**
+ * qdict_set_default_str(): If no entry mapped by 'key' exists in 'dst' yet, a
+ * new QString initialised by 'val' is put there.
+ */
+void qdict_set_default_str(QDict *dst, const char *key, const char *val)
+{
+if (qdict_haskey(dst, key)) {
+return;
+}
+
+qdict_put_str(dst, key, val);
+}
+
+static void qdict_flatten_qdict(QDict *qdict, QDict *target,
+const char *prefix);
+
+static void qdict_flatten_qlist(QList *qlist, QDict *target, const char 
*prefix)
+{
+QObject *value;
+const QListEntry *entry;
+char *new_key;
+int i;
+
+/* This function is never called with prefix == NULL, i.e., it is always
+ * called from within qdict_flatten_q(list|dict)(). Therefore, it does not
+ * need to remove list entries during the iteration (the whole list will be
+ * deleted eventually anyway from qdict_flatten_qdict()). */
+assert(prefix);
+
+entry = qlist_first(qlist);
+
+for (i = 0; entry; entry = qlist_next(entry), i++) {
+value = qlist_entry_obj(entry);
+new_key = g_strdup_printf("%s.%i", prefix, i);
+
+if (qobject_type(value) == QTYPE_QDICT) {
+qdict_flatten_qdict(qobject_to(QDict, value), target, new_key);
+} else if (qobject_type(value) == QTYPE_QLIST) {
+qdict_flatten_qlist(qobject_to(QList, value), target, new_key);
+} else {
+/* All other types are moved to the target unchanged. */
+qdict_put_obj(target, new_key, qobject_ref(value));
+}
+
+g_free(new_key);
+}
+}
+
+static void qdict_flatten_qdict(QDict *qdict, QDict *target, const char 
*prefix)
+{
+QObject *value;
+const QDictEntry *entry, *next;
+char *new_key;
+bool delete;
+
+entry = qdict_first(qdict);
+
+while (entry != NULL) {
+
+next = qdict_next(qdict, entry);
+value = qdict_entry_value(entry);
+new_key = NULL;
+delete = false;
+
+if (prefix) {
+new_key = g_strdup_printf("%s.%s", prefix, entry->key);
+}
+
+if (qobject_type(value) == QTYPE_QDICT) {
+/* Entries of QDicts are processed recursively, the QDict object
+ * itself disappears. */
+qdict_flatten_qdict(qobject_to(QDict, value), target,
+new_key ? new_key : entry->key);
+delete = true;
+} else if (qobject_type(value) == QTYPE_QLIST) {
+qdict_flatten_qlist(qobject_to(QList, value), target,
+new_key ? new_key : entry->key);
+delete = true;
+   

Re: [Qemu-block] [Qemu-devel] [PATCH] [RFC] aio: properly bubble up errors from initialization

[no-reply]

Hi,

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

Type: series
Message-id: 20180614232119.31669-1-naravamu...@digitalocean.com
Subject: [Qemu-devel] [PATCH] [RFC] aio: properly bubble up errors from 
initialization

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   
patchew/20180614232119.31669-1-naravamu...@digitalocean.com -> 
patchew/20180614232119.31669-1-naravamu...@digitalocean.com
Switched to a new branch 'test'
e72bf49783 aio: properly bubble up errors from initialization

=== OUTPUT BEGIN ===
Checking PATCH 1/1: aio: properly bubble up errors from initialization...
ERROR: do not use C99 // comments
#146: FILE: block/io.c:2788:
+// XXX: do we need to undo the successful setups if we fail midway?

ERROR: do not use C99 // comments
#157: FILE: block/io.c:2799:
+// XXX: do we need to undo the successful setups if we fail midway?

total: 2 errors, 0 warnings, 258 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

[Qemu-block] [PATCH] [RFC] aio: properly bubble up errors from initialization

[Nishanth Aravamudan]

laio_init() can fail for a couple of reasons, which will lead to a NULL
pointer dereference in laio_attach_aio_context().

To solve this, add a aio_linux_aio_setup() path which is called where
aio_get_linux_aio() is called currently, but can propogate errors up.

virtio-block and virtio-scsi call this new function before calling
blk_io_plug() (which eventually calls aio_get_linux_aio). This is
necessary because plug/unplug currently assume they do not fail.

It is trivial to make qemu segfault in my testing. Set
/proc/sys/fs/aio-max-nr to 0 and start a guest with
aio=native,cache=directsync. With this patch, the guest successfully
starts (but obviously isn't using native AIO). Setting aio-max-nr back
up to a reasonable value, AIO contexts are consumed normally.

Signed-off-by: Nishanth Aravamudan 
---
 block/block-backend.c  | 10 ++
 block/file-posix.c | 27 +++
 block/io.c | 27 +++
 block/linux-aio.c  | 15 ++-
 hw/block/virtio-blk.c  |  4 
 hw/scsi/virtio-scsi.c  |  4 
 include/block/aio.h|  3 +++
 include/block/block.h  |  1 +
 include/block/block_int.h  |  1 +
 include/block/raw-aio.h|  2 +-
 include/sysemu/block-backend.h |  1 +
 stubs/linux-aio.c  |  2 +-
 util/async.c   | 14 +++---
 13 files changed, 101 insertions(+), 10 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index d55c328736..2a18bb2af4 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1946,6 +1946,16 @@ void blk_add_insert_bs_notifier(BlockBackend *blk, 
Notifier *notify)
 notifier_list_add(>insert_bs_notifiers, notify);
 }
 
+int blk_io_plug_setup(BlockBackend *blk)
+{
+BlockDriverState *bs = blk_bs(blk);
+
+if (bs) {
+return bdrv_io_plug_setup(bs);
+}
+return 0;
+}
+
 void blk_io_plug(BlockBackend *blk)
 {
 BlockDriverState *bs = blk_bs(blk);
diff --git a/block/file-posix.c b/block/file-posix.c
index 07bb061fe4..adaba7c366 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1665,6 +1665,12 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, 
uint64_t offset,
 type |= QEMU_AIO_MISALIGNED;
 #ifdef CONFIG_LINUX_AIO
 } else if (s->use_linux_aio) {
+int rc;
+rc = aio_linux_aio_setup(bdrv_get_aio_context(bs));
+if (rc != 0) {
+s->use_linux_aio = 0;
+return rc;
+}
 LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
 assert(qiov->size == bytes);
 return laio_co_submit(bs, aio, s->fd, offset, qiov, type);
@@ -1690,12 +1696,28 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState 
*bs, uint64_t offset,
 return raw_co_prw(bs, offset, bytes, qiov, QEMU_AIO_WRITE);
 }
 
+static int raw_aio_plug_setup(BlockDriverState *bs)
+{
+int rc = 0;
+#ifdef CONFIG_LINUX_AIO
+BDRVRawState *s = bs->opaque;
+if (s->use_linux_aio) {
+rc = aio_linux_aio_setup(bdrv_get_aio_context(bs));
+if (rc != 0) {
+s->use_linux_aio = 0;
+}
+}
+#endif
+return rc;
+}
+
 static void raw_aio_plug(BlockDriverState *bs)
 {
 #ifdef CONFIG_LINUX_AIO
 BDRVRawState *s = bs->opaque;
 if (s->use_linux_aio) {
 LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
+assert(aio != NULL);
 laio_io_plug(bs, aio);
 }
 #endif
@@ -1707,6 +1729,7 @@ static void raw_aio_unplug(BlockDriverState *bs)
 BDRVRawState *s = bs->opaque;
 if (s->use_linux_aio) {
 LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
+assert(aio != NULL);
 laio_io_unplug(bs, aio);
 }
 #endif
@@ -2599,6 +2622,7 @@ BlockDriver bdrv_file = {
 .bdrv_co_copy_range_from = raw_co_copy_range_from,
 .bdrv_co_copy_range_to  = raw_co_copy_range_to,
 .bdrv_refresh_limits = raw_refresh_limits,
+.bdrv_io_plug_setup = raw_aio_plug_setup,
 .bdrv_io_plug = raw_aio_plug,
 .bdrv_io_unplug = raw_aio_unplug,
 
@@ -3079,6 +3103,7 @@ static BlockDriver bdrv_host_device = {
 .bdrv_co_copy_range_from = raw_co_copy_range_from,
 .bdrv_co_copy_range_to  = raw_co_copy_range_to,
 .bdrv_refresh_limits = raw_refresh_limits,
+.bdrv_io_plug_setup = raw_aio_plug_setup,
 .bdrv_io_plug = raw_aio_plug,
 .bdrv_io_unplug = raw_aio_unplug,
 
@@ -3201,6 +3226,7 @@ static BlockDriver bdrv_host_cdrom = {
 .bdrv_co_pwritev= raw_co_pwritev,
 .bdrv_aio_flush= raw_aio_flush,
 .bdrv_refresh_limits = raw_refresh_limits,
+.bdrv_io_plug_setup = raw_aio_plug_setup,
 .bdrv_io_plug = raw_aio_plug,
 .bdrv_io_unplug = raw_aio_unplug,
 
@@ -3331,6 +3357,7 @@ static BlockDriver bdrv_host_cdrom = {
 .bdrv_co_pwritev= raw_co_pwritev,
 .bdrv_aio_flush= raw_aio_flush,
 

Re: [Qemu-block] [Qemu-devel] [PATCH v3 0/8] discard blockstats

[Anton Nefedov]

well, build tests failed due to the lack of qapi patches this series is
based on :)
(http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg03638.html)

On 13/6/2018 9:44 PM, no-re...@patchew.org wrote:

Hi,

This series failed build test on s390x host. Please find the details below.

Type: series
Message-id: 1528911866-37489-1-git-send-email-anton.nefe...@virtuozzo.com
Subject: [Qemu-devel] [PATCH v3 0/8] discard blockstats

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e
echo "=== ENV ==="
env
echo "=== PACKAGES ==="
rpm -qa
echo "=== TEST BEGIN ==="
CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
echo -n "Using CC: "
realpath $CC
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
 From https://github.com/patchew-project/qemu
  * [new tag]   
patchew/1528911866-37489-1-git-send-email-anton.nefe...@virtuozzo.com -> 
patchew/1528911866-37489-1-git-send-email-anton.nefe...@virtuozzo.com
Switched to a new branch 'test'
ffdbc6a304 qapi: query-blockstat: add driver specific file-posix stats
e083b61534 file-posix: account discard operations
7f94677e54 scsi: account unmap operations
5319508883 scsi: move unmap error checking to the complete callback
71d39aee6c scsi: store unmap offset and nb_sectors in request struct
ea700b6ca5 ide: account UNMAP (TRIM) operations
4877c986b5 qapi: add unmap to BlockDeviceStats
e04d7101a1 qapi: group BlockDeviceStats fields

=== OUTPUT BEGIN ===
=== ENV ===
LANG=en_US.UTF-8
XDG_SESSION_ID=225076
USER=fam
PWD=/var/tmp/patchew-tester-tmp-w5gg517r/src
HOME=/home/fam
SHELL=/bin/sh
SHLVL=2
PATCHEW=/home/fam/patchew/patchew-cli -s http://patchew.org --nodebug
LOGNAME=fam
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1012/bus
XDG_RUNTIME_DIR=/run/user/1012
PATH=/usr/bin:/bin
_=/usr/bin/env
=== PACKAGES ===
gpg-pubkey-873529b8-54e386ff
glibc-debuginfo-common-2.24-10.fc25.s390x
fedora-release-26-1.noarch
dejavu-sans-mono-fonts-2.35-4.fc26.noarch
xemacs-filesystem-21.5.34-22.20170124hgf412e9f093d4.fc26.noarch
bash-4.4.12-7.fc26.s390x
libSM-1.2.2-5.fc26.s390x
libmpc-1.0.2-6.fc26.s390x
libaio-0.3.110-7.fc26.s390x
libverto-0.2.6-7.fc26.s390x
perl-Scalar-List-Utils-1.48-1.fc26.s390x
iptables-libs-1.6.1-2.fc26.s390x
tcl-8.6.6-2.fc26.s390x
libxshmfence-1.2-4.fc26.s390x
expect-5.45-23.fc26.s390x
perl-Thread-Queue-3.12-1.fc26.noarch
perl-encoding-2.19-6.fc26.s390x
keyutils-1.5.10-1.fc26.s390x
gmp-devel-6.1.2-4.fc26.s390x
enchant-1.6.0-16.fc26.s390x
python-gobject-base-3.24.1-1.fc26.s390x
python3-enchant-1.6.10-1.fc26.noarch
python-lockfile-0.11.0-6.fc26.noarch
python2-pyparsing-2.1.10-3.fc26.noarch
python2-lxml-4.1.1-1.fc26.s390x
librados2-10.2.7-2.fc26.s390x
trousers-lib-0.3.13-7.fc26.s390x
libdatrie-0.2.9-4.fc26.s390x
libsoup-2.58.2-1.fc26.s390x
passwd-0.79-9.fc26.s390x
bind99-libs-9.9.10-3.P3.fc26.s390x
python3-rpm-4.13.0.2-1.fc26.s390x
systemd-233-7.fc26.s390x
virglrenderer-0.6.0-1.20170210git76b3da97b.fc26.s390x
s390utils-ziomon-1.36.1-3.fc26.s390x
s390utils-osasnmpd-1.36.1-3.fc26.s390x
libXrandr-1.5.1-2.fc26.s390x
libglvnd-glx-1.0.0-1.fc26.s390x
texlive-ifxetex-svn19685.0.5-33.fc26.2.noarch
texlive-psnfss-svn33946.9.2a-33.fc26.2.noarch
texlive-dvipdfmx-def-svn40328-33.fc26.2.noarch
texlive-natbib-svn20668.8.31b-33.fc26.2.noarch
texlive-xdvi-bin-svn40750-33.20160520.fc26.2.s390x
texlive-cm-svn32865.0-33.fc26.2.noarch
texlive-beton-svn15878.0-33.fc26.2.noarch
texlive-fpl-svn15878.1.002-33.fc26.2.noarch
texlive-mflogo-svn38628-33.fc26.2.noarch
texlive-texlive-docindex-svn41430-33.fc26.2.noarch
texlive-luaotfload-bin-svn34647.0-33.20160520.fc26.2.noarch
texlive-koma-script-svn41508-33.fc26.2.noarch
texlive-pst-tree-svn24142.1.12-33.fc26.2.noarch
texlive-breqn-svn38099.0.98d-33.fc26.2.noarch
texlive-xetex-svn41438-33.fc26.2.noarch
gstreamer1-plugins-bad-free-1.12.3-1.fc26.s390x
xorg-x11-font-utils-7.5-33.fc26.s390x
ghostscript-fonts-5.50-36.fc26.noarch
libXext-devel-1.3.3-5.fc26.s390x
libusbx-devel-1.0.21-2.fc26.s390x
libglvnd-devel-1.0.0-1.fc26.s390x
emacs-25.3-3.fc26.s390x
alsa-lib-devel-1.1.4.1-1.fc26.s390x
kbd-2.0.4-2.fc26.s390x
dconf-0.26.0-2.fc26.s390x
mc-4.8.19-5.fc26.s390x
doxygen-1.8.13-9.fc26.s390x
dpkg-1.18.24-1.fc26.s390x
libtdb-1.3.13-1.fc26.s390x
python2-pynacl-1.1.1-1.fc26.s390x
perl-Filter-1.58-1.fc26.s390x
python2-pip-9.0.1-11.fc26.noarch
dnf-2.7.5-2.fc26.noarch
bind-license-9.11.2-1.P1.fc26.noarch
libtasn1-4.13-1.fc26.s390x
cpp-7.3.1-2.fc26.s390x
pkgconf-1.3.12-2.fc26.s390x
python2-fedora-0.10.0-1.fc26.noarch
cmake-filesystem-3.10.1-11.fc26.s390x
python3-requests-kerberos-0.12.0-1.fc26.noarch
libmicrohttpd-0.9.59-1.fc26.s390x
GeoIP-GeoLite-data-2018.01-1.fc26.noarch
python2-libs-2.7.14-7.fc26.s390x
libidn2-2.0.4-3.fc26.s390x

Re: [Qemu-block] [PATCH v4 00/10] New block driver: blklogwrites

[Ari Sundholm]

ping. Any comments or suggestions would be welcome whenever you have the 
time. :)


Thank you,
Ari Sundholm
a...@tuxera.com


On 06/08/2018 03:32 PM, Ari Sundholm wrote:

This patch series adds a new block driver, blklogwrites, to QEMU. The
driver is given two block devices: a raw device backed by an image or a
host block device, and a log device, typically backed by a file, on
which writes to the raw device are logged.

The logging format used is the same as in the dm-log-writes target of
the Linux kernel device mapper. The log reflects the writes that have
been performed on the guest block device and flushed. To be strict, the
log may contain writes that have not been flushed yet, but they are
technically outside the bounds of the log until the next flush updates
the metadata in the log superblock. We believe these semantics to be
useful even though they may not be completely identical to those of
dm-log-writes.

This functionality can be used for crash consistency and fs consistency
testing in filesystem drivers, including on non-Linux guests and older
guests running Linux versions without support for dm-log-writes. This
is simple and useful. Admittedly this and more advanced things could
perhaps be done by extending the quorum driver, but this approach would
require re-engineering the functionality and involve a more complicated
setup, so we offer this simple solution which we have found useful
internally.

In the first patch of this series, two block permission constants are
moved from block.c to include/block/block.h to make them available
outside of block.c. The next patch uses these constants.

The driver requires all requests to be aligned to the sector size. In
the second patch of this series, which includes the bulk of the
blklogwrites driver, this sector size is assumed to be equal to
BDRV_SECTOR_SIZE for simplicity.

In patches 3-8, a mechanism to pass the block configuration of a block
backend to a block driver is introduced, and this mechanism is
integrated in various hw/block drivers.

In patch 9, blklogwrites is changed to use the block configuration of
the relevant block backend to set its block limits.

Finally, in patch 10, blklogwrites is improved to use the logical
sector size of the block backend for logging. This means that the
sector size in the log metadata will be set to the logical sector size,
and offsets and sizes of writes will be expressed as a multiple of that
instead of BDRV_SECTOR_SIZE.

v4:
   - Check return value of snprintf() (flagged by patchew)
   - Don't use C99 for loop initial declaration (flagged by patchew, but
 wasn't QEMU supposed to be C99? Oh well.)
   - Add proper "Since" fields for blklogwrites in block-core.json
   - Rebase on current upstream master

v3:
   - Rebase on current upstream master

v2:
   - Incorporate review feedback
   - Add patches to apply block configurations in more block device
 drivers

Aapo Vienamo (1):
   block: Add blklogwrites

Ari Sundholm (9):
   block: Move two block permission constants to the relevant enum
   block: Add a mechanism for passing a block driver a block
 configuration
   hw/scsi/scsi-disk: Always apply block configuration to block driver
   hw/ide/qdev: Always apply block configuration to block driver
   hw/block/virtio-blk: Always apply block configuration to block driver
   hw/block/nvme: Always apply block configuration to block driver
   hw/block/fdc: Always apply block configuration to block driver
   block/blklogwrites: Use block limits from the backend block
 configuration
   block/blklogwrites: Use the block device logical sector size when
 logging writes

  MAINTAINERS   |   6 +
  block.c   |   6 -
  block/Makefile.objs   |   1 +
  block/blklogwrites.c  | 426 ++
  block/io.c|  22 +++
  hw/block/block.c  |  12 +-
  hw/block/fdc.c|   4 +
  hw/block/nvme.c   |   1 +
  hw/block/virtio-blk.c |   1 +
  hw/ide/qdev.c |   2 +
  hw/scsi/scsi-disk.c   |   1 +
  include/block/block.h |  17 ++
  include/block/block_int.h |   9 +
  include/hw/block/block.h  |   1 +
  qapi/block-core.json  |  30 +++-
  15 files changed, 526 insertions(+), 13 deletions(-)
  create mode 100644 block/blklogwrites.c