[Qemu-block] [PATCH] Added iopmem device emulation

[Logan Gunthorpe]

An iopmem device is one which exposes volatile or non-volatile memory mapped
directly to a BAR region. One purpose is to provide buffers to do peer
to peer transfers on the bus. As such this device uses QEMU's drive
backing store to simulate non-volatile memory and provides through a
mapped BAR window.

This patch creates an emulated device which helps to test and debug the
kernel driver for iopmem while hardware availability is poor. A kernel
patch for a driver is being prepared simultaneously.

Signed-off-by: Logan Gunthorpe 
Signed-off-by: Stephen Bates 
---
 default-configs/pci.mak  |   1 +
 hw/block/Makefile.objs   |   1 +
 hw/block/iopmem.c| 141 +++
 include/hw/pci/pci_ids.h |   2 +
 4 files changed, 145 insertions(+)
 create mode 100644 hw/block/iopmem.c

diff --git a/default-configs/pci.mak b/default-configs/pci.mak
index fff7ce3..aef2b35 100644
--- a/default-configs/pci.mak
+++ b/default-configs/pci.mak
@@ -39,3 +39,4 @@ CONFIG_VGA=y
 CONFIG_VGA_PCI=y
 CONFIG_IVSHMEM=$(CONFIG_EVENTFD)
 CONFIG_ROCKER=y
+CONFIG_IOPMEM_PCI=y
diff --git a/hw/block/Makefile.objs b/hw/block/Makefile.objs
index d4c3ab7..1c8044b 100644
--- a/hw/block/Makefile.objs
+++ b/hw/block/Makefile.objs
@@ -8,6 +8,7 @@ common-obj-$(CONFIG_XEN_BACKEND) += xen_disk.o
 common-obj-$(CONFIG_ECC) += ecc.o
 common-obj-$(CONFIG_ONENAND) += onenand.o
 common-obj-$(CONFIG_NVME_PCI) += nvme.o
+common-obj-$(CONFIG_IOPMEM_PCI) += iopmem.o
 
 obj-$(CONFIG_SH4) += tc58128.o
 
diff --git a/hw/block/iopmem.c b/hw/block/iopmem.c
new file mode 100644
index 000..9c2d716
--- /dev/null
+++ b/hw/block/iopmem.c
@@ -0,0 +1,141 @@
+/*
+ * QEMU iopmem Controller
+ *
+ * Copyright (c) 2016, Microsemi Corporation
+ *
+ * Written by Logan Gunthorpe 
+ *
+ * This code is licensed under the GNU GPL v2 or later.
+ */
+
+
+/**
+ * Usage: add options:
+ *  -drive file=,if=none,id=
+ *  -device iopmem,drive=,id=
+ */
+
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "sysemu/block-backend.h"
+
+typedef struct IoPmemCtrl {
+PCIDeviceparent_obj;
+MemoryRegion iomem;
+BlockBackend *blk;
+uint64_t size;
+} IoPmemCtrl;
+
+#define TYPE_IOPMEM "iopmem"
+#define IOPMEM(obj) \
+OBJECT_CHECK(IoPmemCtrl, (obj), TYPE_IOPMEM)
+
+static uint64_t iopmem_bar_read(void *opaque, hwaddr addr, unsigned size)
+{
+IoPmemCtrl *ipm = (IoPmemCtrl *)opaque;
+uint64_t val;
+
+blk_pread(ipm->blk, addr, , size);
+
+return val;
+}
+
+static void iopmem_bar_write(void *opaque, hwaddr addr, uint64_t data,
+   unsigned size)
+{
+IoPmemCtrl *ipm = (IoPmemCtrl *)opaque;
+
+if (addr & 3) {
+return;
+}
+
+blk_pwrite(ipm->blk, addr, , size, 0);
+}
+
+static const MemoryRegionOps iopmem_bar_ops = {
+.read = iopmem_bar_read,
+.write = iopmem_bar_write,
+.endianness = DEVICE_LITTLE_ENDIAN,
+.impl = {
+.min_access_size = 4,
+.max_access_size = 8,
+},
+};
+
+static int iopmem_init(PCIDevice *pci_dev)
+{
+IoPmemCtrl *ipm = IOPMEM(pci_dev);
+
+if (!ipm->blk) {
+return -1;
+}
+
+ipm->size = blk_getlength(ipm->blk);
+
+pci_config_set_prog_interface(pci_dev->config, 0x2);
+pci_config_set_class(pci_dev->config, PCI_CLASS_STORAGE_OTHER);
+pcie_endpoint_cap_init(>parent_obj, 0x80);
+
+memory_region_init_io(>iomem, OBJECT(ipm), _bar_ops, ipm,
+  "iopmem", ipm->size);
+
+pci_register_bar(>parent_obj, 4,
+ PCI_BASE_ADDRESS_SPACE_MEMORY |
+ PCI_BASE_ADDRESS_MEM_PREFETCH |
+ PCI_BASE_ADDRESS_MEM_TYPE_64,
+ >iomem);
+
+return 0;
+}
+
+static void iopmem_exit(PCIDevice *pci_dev)
+{
+IoPmemCtrl *ipm = IOPMEM(pci_dev);
+
+if (ipm->blk) {
+blk_flush(ipm->blk);
+}
+}
+
+static Property iopmem_props[] = {
+DEFINE_PROP_DRIVE("drive", IoPmemCtrl, blk),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static const VMStateDescription iopmem_vmstate = {
+.name = "iopmem",
+.unmigratable = 1,
+};
+
+static void iopmem_class_init(ObjectClass *oc, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(oc);
+PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc);
+
+pc->init = iopmem_init;
+pc->exit = iopmem_exit;
+pc->class_id = PCI_CLASS_STORAGE_OTHER;
+pc->vendor_id = PCI_VENDOR_ID_PMC_SIERRA;
+pc->device_id = 0xf115;
+pc->revision = 2;
+pc->is_express = 1;
+
+set_bit(DEVICE_CATEGORY_STORAGE, dc->categories);
+dc->desc = "Non-Volatile IO Memory Storage";
+dc->props = iopmem_props;
+dc->vmsd = _vmstate;
+}
+
+static const TypeInfo iopmem_info = {
+.name  = "iopmem",
+.parent= TYPE_PCI_DEVICE,
+.instance_size = sizeof(IoPmemCtrl),
+.class_init= iopmem_class_init,
+};
+
+static void iopmem_register_types(void)
+{
+

Re: [Qemu-block] [PATCH v2 00/20] dataplane: remove RFifoLock

[Fam Zheng]

On Wed, 10/19 08:55, Fam Zheng wrote:
> Modulo the one harmful question, series:

s/harmful/unharmful/, apparently.

Fam

Re: [Qemu-block] [PATCH v2 00/20] dataplane: remove RFifoLock

[Fam Zheng]

On Mon, 10/17 15:54, Paolo Bonzini wrote:
> This patch reorganizes aio_poll callers to establish new rules for
> dataplane locking.  The idea is that I/O operations on a dataplane
> BDS (i.e. one where the AioContext is not the main one) do not call
> aio_poll anymore.  Instead, they wait for the operation to end in the
> other I/O thread, at which point the other I/O thread calls bdrv_wakeup
> to wake up the main thread.
> 
> With this change, only one thread runs aio_poll for an AioContext.
> While aio_context_acquire/release is still needed to protect the BDSes,
> it need not interrupt the other thread's event loop anymore, and therefore
> it does not need contention callbacks anymore.  Thus the patch can remove
> RFifoLock.  This fixes possible hangs in bdrv_drain_all, reproducible (for
> example) by unplugging a virtio-scsi-dataplane device while there is I/O
> going on for a virtio-blk-dataplane on the same I/O thread.
> 
> Patch 1 is a bugfix that I already posted.
> 
> Patch 2 makes blockjobs independent of aio_poll, the reason for which
> should be apparent from the explanation above.
> 
> Patch 3 is an independent mirror bugfix, that I wanted to submit separately
> but happens to fix a hang in COLO replication.  Like patch 1 I believe
> it's pre-existing and merely exposed by these patches.
> 
> Patches 4 to 10 introduce the infrastructure to wake up the main thread
> while bdrv_drain or other synchronous operations are running.  Patches 11
> to 16 do other changes to prepare for this.  Notably bdrv_drain_all
> needs to be called without holding any AioContext lock, so bdrv_reopen
> releases the lock temporarily (and callers of bdrv_reopen needs fixing).
> 
> Patch 17 then does the big change, after which there are just some
> cleanups left to do.
> 
> Paolo
> 
> Fam Zheng (1):
>   qed: Implement .bdrv_drain
> 
> Paolo Bonzini (19):
>   replication: interrupt failover if the main device is closed
>   blockjob: introduce .drain callback for jobs
>   mirror: use bdrv_drained_begin/bdrv_drained_end
>   block: add BDS field to count in-flight requests
>   block: change drain to look only at one child at a time
>   block: introduce BDRV_POLL_WHILE
>   nfs: move nfs_set_events out of the while loops
>   nfs: use BDRV_POLL_WHILE
>   sheepdog: use BDRV_POLL_WHILE
>   aio: introduce qemu_get_current_aio_context
>   iothread: detach all block devices before stopping them
>   replication: pass BlockDriverState to reopen_backing_file
>   block: prepare bdrv_reopen_multiple to release AioContext
>   qemu-io: acquire AioContext
>   qemu-img: call aio_context_acquire/release around block job
>   block: only call aio_poll on the current thread's AioContext
>   iothread: release AioContext around aio_poll
>   qemu-thread: introduce QemuRecMutex
>   aio: convert from RFifoLock to QemuRecMutex

Modulo the one harmful question, series:

Reviewed-by: Fam Zheng 

Re: [Qemu-block] [PATCH 16/20] qemu-img: call aio_context_acquire/release around block job

[Fam Zheng]

On Mon, 10/17 15:54, Paolo Bonzini wrote:
> This will be needed by bdrv_reopen_multiple, which calls
> bdrv_drain_all and thus will *release* the AioContext.

Looks okay, but I wonder how bdrv_drain_all releasing AioContext break anything?

Fam

> 
> Signed-off-by: Paolo Bonzini 
> ---
> v1->v2: new [qemu-iotests]
> 
>  qemu-img.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 02c07b9..ad7c964 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -795,6 +795,7 @@ static void run_block_job(BlockJob *job, Error **errp)
>  {
>  AioContext *aio_context = blk_get_aio_context(job->blk);
>  
> +aio_context_acquire(aio_context);
>  do {
>  aio_poll(aio_context, true);
>  qemu_progress_print(job->len ?
> @@ -802,6 +803,7 @@ static void run_block_job(BlockJob *job, Error **errp)
>  } while (!job->ready);
>  
>  block_job_complete_sync(job, errp);
> +aio_context_release(aio_context);
>  
>  /* A block job may finish instantaneously without publishing any 
> progress,
>   * so just signal completion here */
> @@ -819,6 +821,7 @@ static int img_commit(int argc, char **argv)
>  Error *local_err = NULL;
>  CommonBlockJobCBInfo cbi;
>  bool image_opts = false;
> +AioContext *aio_context;
>  
>  fmt = NULL;
>  cache = BDRV_DEFAULT_CACHE;
> @@ -928,8 +931,11 @@ static int img_commit(int argc, char **argv)
>  .bs   = bs,
>  };
>  
> +aio_context = bdrv_get_aio_context(bs);
> +aio_context_acquire(aio_context);
>  commit_active_start("commit", bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
>  common_block_job_cb, , _err, false);
> +aio_context_release(aio_context);
>  if (local_err) {
>  goto done;
>  }
> -- 
> 2.7.4
> 
> 

Re: [Qemu-block] [Qemu-devel] [PATCH v3 4/4] qemu-iotests: Test creating floppy drives

[Eric Blake]

On 10/18/2016 02:45 PM, John Snow wrote:
> 
> 
> On 10/18/2016 06:22 AM, Kevin Wolf wrote:
>> This tests the different supported methods to create floppy drives and
>> how they interact.
>>

>> +function check_floppy_qtree()
>> +{
>> +echo
>> +echo Testing: "$@" | _filter_testdir
>> +
>> +# QEMU_OPTIONS contains -nodefaults, we don't want that here
>> because the
>> +# defaults are part of what should be checked here
>> +echo "info qtree" |
>> +QEMU_OPTIONS="" do_run_qemu "$@" | _filter_win32 |
>> +grep -zo '[[:cntrl:]]\( *\)dev: isa-fdc.*\([[:cntrl:]]\1
>> .*\)*[[:cntrl:]] *dev:'
> 
> This grep invocation doesn't appear to actually terminate with the '-z'
> option here. Not sure why, I haven't looked into the bash framework
> much, hopefully it's not too hard for you to reproduce and correct.

Is 'grep -z' even portable to BSD, or is it just a GNU extension?  Would
it be better to run the output through tr to convert things to a saner
subset of characters before then grepping a text file?

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v3 4/4] qemu-iotests: Test creating floppy drives

[John Snow]

On 10/18/2016 06:22 AM, Kevin Wolf wrote:

This tests the different supported methods to create floppy drives and
how they interact.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/172 |  242 +
 tests/qemu-iotests/172.out | 1205 
 tests/qemu-iotests/group   |1 +
 3 files changed, 1448 insertions(+)
 create mode 100755 tests/qemu-iotests/172
 create mode 100644 tests/qemu-iotests/172.out

diff --git a/tests/qemu-iotests/172 b/tests/qemu-iotests/172
new file mode 100755
index 000..8bb6443
--- /dev/null
+++ b/tests/qemu-iotests/172
@@ -0,0 +1,242 @@
+#!/bin/bash
+#
+# Test floppy configuration
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
+_notrun "Requires a PC machine"
+fi
+
+function do_run_qemu()
+{
+(
+if ! test -t 0; then
+while read cmd; do
+echo $cmd
+done
+fi
+echo quit
+) | $QEMU -nographic -monitor stdio -serial none "$@"
+echo
+}
+
+function check_floppy_qtree()
+{
+echo
+echo Testing: "$@" | _filter_testdir
+
+# QEMU_OPTIONS contains -nodefaults, we don't want that here because the
+# defaults are part of what should be checked here
+echo "info qtree" |
+QEMU_OPTIONS="" do_run_qemu "$@" | _filter_win32 |
+grep -zo '[[:cntrl:]]\( *\)dev: isa-fdc.*\([[:cntrl:]]\1 .*\)*[[:cntrl:]] 
*dev:'


This grep invocation doesn't appear to actually terminate with the '-z' 
option here. Not sure why, I haven't looked into the bash framework 
much, hopefully it's not too hard for you to reproduce and correct.


...Sorry!

--js

Re: [Qemu-block] [PATCH v2] rbd: make the code more readable

[Jeff Cody]

On Sat, Oct 15, 2016 at 04:26:13PM +0800, Xiubo Li wrote:
> Make it a bit clearer and more readable.
> 
> Signed-off-by: Xiubo Li 
> CC: John Snow 
> ---
> 
> V2:
> - Advice from John Snow. Thanks.
> 
> 
>  block/rbd.c | 25 -
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 0a5840d..d0d4b39 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -366,45 +366,44 @@ static int qemu_rbd_create(const char *filename, 
> QemuOpts *opts, Error **errp)
>  rados_conf_read_file(cluster, NULL);
>  } else if (conf[0] != '\0' &&
> qemu_rbd_set_conf(cluster, conf, true, _err) < 0) {
> -rados_shutdown(cluster);
>  error_propagate(errp, local_err);
> -return -EIO;
> +ret = -EIO;
> +goto shutdown;
>  }
>  
>  if (conf[0] != '\0' &&
>  qemu_rbd_set_conf(cluster, conf, false, _err) < 0) {
> -rados_shutdown(cluster);
>  error_propagate(errp, local_err);
> -return -EIO;
> +ret = -EIO;
> +goto shutdown;
>  }
>  
>  if (qemu_rbd_set_auth(cluster, secretid, errp) < 0) {
> -rados_shutdown(cluster);
> -return -EIO;
> +ret = -EIO;
> +goto shutdown;
>  }
>  
>  ret = rados_connect(cluster);
>  if (ret < 0) {
>  error_setg_errno(errp, -ret, "error connecting");
> -rados_shutdown(cluster);
> -return ret;
> +goto shutdown;
>  }
>  
>  ret = rados_ioctx_create(cluster, pool, _ctx);
>  if (ret < 0) {
>  error_setg_errno(errp, -ret, "error opening pool %s", pool);
> -rados_shutdown(cluster);
> -return ret;
> +goto shutdown;
>  }
>  
>  ret = rbd_create(io_ctx, name, bytes, _order);
> -rados_ioctx_destroy(io_ctx);
> -rados_shutdown(cluster);
>  if (ret < 0) {
>  error_setg_errno(errp, -ret, "error rbd create");
> -return ret;
>  }
>  
> +rados_ioctx_destroy(io_ctx);
> +
> +shutdown:
> +rados_shutdown(cluster);
>  return ret;
>  }
>  
> -- 
> 1.8.3.1
> 
> 
> 
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff

Re: [Qemu-block] [Qemu-devel] [PATCH v14 14/21] qapi: allow repeated opts with qobject_input_visitor_new_opts

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> The qobject_input_visitor_new_opts() method gains a new
> parameter to control whether it allows repeated option
> keys in the input QemuOpts or not.
>
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qobject-input-visitor.h | 6 ++
>  qapi/qobject-input-visitor.c | 5 -
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/qobject-input-visitor.h 
> b/include/qapi/qobject-input-visitor.h
> index 242b767..bc5062a 100644
> --- a/include/qapi/qobject-input-visitor.h
> +++ b/include/qapi/qobject-input-visitor.h
> @@ -112,6 +112,11 @@ Visitor *qobject_input_visitor_new_autocast(QObject *obj,
>   * qobject_input_visitor_new_autocast() method. See the docs
>   * of that method for further details on processing behaviour.
>   *
> + * If the @permit_repeated_opts parameter is true, then the input
> + * @opts is allowed to contain repeated keys and they will be
> + * turned into a QList. If it is false, then repeated keys will
> + * result in an error being reported.
> + *
>   * The returned input visitor should be released by calling
>   * visit_free() when no longer required.
>   */
> @@ -119,6 +124,7 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts 
> *opts,
>  bool autocreate_list,
>  size_t autocreate_struct_levels,
>  bool permit_int_ranges,
> +bool permit_repeated_opts,
>  Error **errp);
>  
>  #endif
> diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> index 2287d11..5a3872c 100644
> --- a/qapi/qobject-input-visitor.c
> +++ b/qapi/qobject-input-visitor.c
> @@ -753,6 +753,7 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts 
> *opts,
>  bool autocreate_list,
>  size_t autocreate_struct_levels,
>  bool permit_int_ranges,
> +bool permit_repeated_opts,
>  Error **errp)
>  {
>  QDict *pdict;
> @@ -760,7 +761,9 @@ Visitor *qobject_input_visitor_new_opts(const QemuOpts 
> *opts,
>  Visitor *v = NULL;
>  
>  pdict = qemu_opts_to_qdict(opts, NULL,
> -   QEMU_OPTS_REPEAT_POLICY_LAST,
> +   permit_repeated_opts ?
> +   QEMU_OPTS_REPEAT_POLICY_ALL :
> +   QEMU_OPTS_REPEAT_POLICY_ERROR,
> errp);
>  if (!pdict) {
>  goto cleanup;

@permit_repeated_opts applies to the whole @opts.  However, the "repeat
to build a list" feature is actually used more selectively.  For
instance, -spice wants it for "tls-channel" and "plaintext-channel", but
not for its other keys.  There, the code uses qemu_opt_get() or similar,
which gets the last value.

The options visitor works the same.  Internally, it stores only lists.
When the visit is for a scalar, it gets the last value from the list.
When the visit is for a list, it sets up things so that the list element
visits get the values in order.

As long as no key is list-valued, you can keep @permit_repeated_opts
false, and have duplicate keys rejected, for whatever that's worth.

As soon as you have a list-valued key, you need to set
@permit_repeated_opts to true.  But then *no* duplicate keys are
rejected.  If consistency with @permit_repeated_opts = false is needed,
then something else needs to detect unwanted lists and reject them.

What use cases for @permit_repeated_opts = false do you envisage?

To serve as replacement for the options visitor, this needs to treat
duplicates the same.  As we saw in PATCH 12, QEMU_OPTS_REPEAT_POLICY_ALL
makes a list when the key is repeated, while the options visitor makes a
list always.  I guess some other patch hides this difference.

Re: [Qemu-block] [Qemu-devel] [PATCH v14 13/21] qdict: allow qdict_crumple to accept compound types as values

[Markus Armbruster]

Kevin Wolf  writes:

> Am 17.10.2016 um 16:50 hat Markus Armbruster geschrieben:
>> Kevin Wolf  writes:
>> 
>> > Am 13.10.2016 um 14:35 hat Markus Armbruster geschrieben:
>> >> Cc: Kevin for discussion of QemuOpts dotted key convention
>> >> 
>> >> "Daniel P. Berrange"  writes:
>> >> 
>> >> > Currently qdict_crumple requires a totally flat QDict as its
>> >> > input. i.e. all values in the QDict must be scalar types.
>> >> >
>> >> > In order to have backwards compatibility with the OptsVisitor,
>> >> > qemu_opt_to_qdict() has a new mode where it may return a QList
>> >> > for values in the QDict, if there was a repeated key. We thus
>> >> > need to allow compound types to appear as values in the input
>> >> > dict given to qdict_crumple().
>> >> >
>> >> > To avoid confusion, we sanity check that the user has not mixed
>> >> > the old and new syntax at the same time. e.g. these are allowed
>> >> >
>> >> >foo=hello,foo=world,foo=wibble
>> >> >foo.0=hello,foo.1=world,foo.2=wibble
>> >> >
>> >> > but this is forbidden
>> >> >
>> >> >foo=hello,foo=world,foo.2=wibble
>> >> 
>> >> I understand the need for foo.bar=val.  It makes it possible to specify
>> >> nested dictionaries with QemuOpts.
>> >> 
>> >> The case for foo.0=val is less clear.  QemuOpts already supports lists,
>> >> by repeating keys.  Why do we need a second, wordier way to specify
>> >> them?
>> >
>> > Probably primarily because someone didn't realise this when introducing
>> > the dotted syntax.
>> 
>> Can't even blame "someone" for that; it's an obscure, underdocumented
>> feature of an interface that's collapsing under its load of obscure,
>> underdocumented features.
>> 
>> On the other hand, that's not exactly a state that allows for *more*
>> obscure features.
>
> I don't really think we're introducing more obscure features here, but
> rather trying to implement a single, and rather straightforward, way as
> the standard.
>
> Dotted syntax for hierarchy has actually plenty of precedence in qemu if
> you look a bit closer (the block layer, -global, -device foo,help, even
> the bus names you mentioned below are really just flattened lists), so
> we're only making things more consistent.
>
>> >Also because flat QDicts can't represent this.
>> 
>> Explain?
>
> Repeated options are parsed into QLists. If you keep it at that without
> flattening you have at least a QDict that contains a QList that contains
> simple types. This is not flat any more.

*All* options are parsed into a (non-QList) list.  That's what is flat.

Only when you start crumpling things go beyond flat, and QDict/QList
come into play.

> Of course, you could argue that flat QDicts are the wrong data structure
> in the first place and instead of flatting everything we should have
> done the equivalent of qdict_crumple from the beginning, but they were
> the natural extension of what was already there and happened to work
> good enough, so this is what we're currently using.

We didn't "flatten everything", because QemuOpts is and has always been
flat to begin with.  Rather we've started to crumple a few things, and
in a separate layer.

I now think this is the wrong approach.  We have clearly outgrown flat
options.  We should redo QemuOpts to support what we need instead of
bolting on an optional crumpling layer.

I guess I reached the place you described, just on a different path :)

>> > But even if I realised that QemuOpts support this syntax, I think we
>> > would still have to use the dotted syntax because it's explicit about
>> > the index and we need that because the list can contains dicts.
>> >
>> > Compare this:
>> >
>> > driver=quorum,
>> > child.0.driver=file,child.0.filename=disk1.img,
>> > child.1.driver=host_device,child.1.filename=/dev/sdb,
>> > child.2.driver=nbd,child.2.host=localhost
>> >
>> > And this:
>> >
>> > driver=quorum,
>> > child.driver=file,child.filename=disk1.img,
>> > child.driver=host_device,child.filename=/dev/sdb,
>> > child.driver=nbd,child.host=localhost
>> 
>> Aside: both are about equally illegible, to be honest.
>
> Not sure about equally, but let's agree on "both are illegible".
>
>> > For starters, can we really trust the order in QemuOpts so that the
>> > right driver and filename are associated with each other?
>> 
>> The order is trustworthy, but...
>> 
>> > We would also have code to associate the third child.driver with the
>> > first child.host (because file and host_device don't have a host
>> > property). And this isn't even touching optional arguments yet. Would
>> > you really want to implement or review this?
>> 
>> ... you're right, doing lists by repeating keys breaks down when
>> combined with the dotted key convention's use of repetition to do
>> structured values.
>> 
>> Permit me to digress.
>> 
>> QemuOpts wasn't designed for list-values keys.  Doing lists by
>> repetition was clever.
>> 
>> 

Re: [Qemu-block] [Qemu-devel] block/nfs: Fine grained runtime options in nfs

[Ashijeet Acharya]

On Tue, Oct 18, 2016 at 9:43 PM, Ashijeet Acharya
 wrote:
> On Tue, Oct 18, 2016 at 7:03 PM, Kevin Wolf  wrote:
>> Am 18.10.2016 um 15:14 hat Ashijeet Acharya geschrieben:
>>> On Tue, Oct 18, 2016 at 6:34 PM, Kevin Wolf  wrote:
>>> > Am 18.10.2016 um 14:46 hat Ashijeet Acharya geschrieben:
>>> >> On Tue, Oct 18, 2016 at 4:11 PM, Peter Lieven  wrote:
>>> >> > Am 17.10.2016 um 21:34 schrieb Ashijeet Acharya:
>>> >> >>
>>> >> >> On Tue, Oct 18, 2016 at 12:59 AM, Eric Blake  
>>> >> >> wrote:
>>> >> >>>
>>> >> >>> On 10/17/2016 01:00 PM, Ashijeet Acharya wrote:
>>> >> >>>
>>> >>  One more relatively easy question though, will we include @port as 
>>> >>  an
>>> >>  option in runtime_opts while converting NFS to use several
>>> >>  runtime_opts? The reason I ask this because the uri syntax for NFS 
>>> >>  in
>>> >>  QEMU looks like this:
>>> >> 
>>> >> 
>>> >>  nfs:[?param=value[=value2[&...]]]
>>> >> >>>
>>> >> >>> It's actually nfs://[:port]/...
>>> >> >>>
>>> >> >>> so the URI syntax already supports port.
>>> >> >>
>>> >> >> But the commit message which added support for NFS had the uri which I
>>> >> >> mentioned above and the code for NFS does not make use of 'port'
>>> >> >> anywhere either, which is why I am a bit confused.
>>> >> >
>>> >> >
>>> >> > Hi Aschijeet,
>>> >> >
>>> >> > don't worry there is no port number when connecting to an NFS server.
>>> >> > The portmapper always listens on port 111. So theoretically we could
>>> >> > specifiy a port in the URL but it is ignored.
>>> >>
>>> >> So that means I will be including 'port' in runtime_opts and then just
>>> >> ignoring any value that comes through it?
>>> >
>>> > No, if there is nothing to configure there, leave it out. Adding an
>>> > option that doesn't do anything is not very useful.
>>> >
>>> Okay, understood.
>>>
>>> So this is what my runtime_opts look like now:
>>>
>>> static QemuOptsList runtime_opts = {
>>> .name = "nfs",
>>> .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>>> .desc = {
>>> {
>>> .name = "host",
>>> .type = QEMU_OPT_STRING,
>>> .help = "Host to connect to",
>>> },
>>> {
>>> .name = "export",
>>> .type = QEMU_OPT_STRING,
>>> .help = "Name of the NFS export to open",
>>> },
>>> {
>>> .name = "path",
>>> .type = QEMU_OPT_STRING,
>>> .help = "Path of the image on the host",
>>> },
>>
>> Does libnfs actually have separate export and path?
>>
>> If I understand correctly, we currently split the URL at the last / in
>> the string. So is export the part before that and path the part after
>> it?
>
> At the moment, I don't see any piece of code in NFS BLOCK DRIVER which
> does that to extract export from the URL. We actually do this at the
> moment:
>
> strp = strrchr(uri->path, '/');
>
> which I think is just to extract the name of the file from the URL and
> has nothing to do with export.
> Although NBD and gluster seem to extract export from the URL like this:
>
> p += strspn(p, "/");
>
> which actually splits the URL at the first appearance of "/", doesn't
> it? And then does after checking for p[0]:
>
> qdict_put(options, "export", qstring_from_str(p));
>
> which puts the value of key export in qdict as the part of the URL
> which occurs after the first appearance of "/", right?
>
>>
>> If so, does this mean that you can't currently access an image file in a
>> subdirectory of an NFS mount and adding the explicit options will fix
>> this?
>
> I am not sure of that :-/
>
But either way I think I will have to drop export and was a silly
mistake to include it, since it is extracted from path one way or
another.

Ashijeet

Re: [Qemu-block] [Qemu-devel] block/nfs: Fine grained runtime options in nfs

[Ashijeet Acharya]

On Tue, Oct 18, 2016 at 7:03 PM, Kevin Wolf  wrote:
> Am 18.10.2016 um 15:14 hat Ashijeet Acharya geschrieben:
>> On Tue, Oct 18, 2016 at 6:34 PM, Kevin Wolf  wrote:
>> > Am 18.10.2016 um 14:46 hat Ashijeet Acharya geschrieben:
>> >> On Tue, Oct 18, 2016 at 4:11 PM, Peter Lieven  wrote:
>> >> > Am 17.10.2016 um 21:34 schrieb Ashijeet Acharya:
>> >> >>
>> >> >> On Tue, Oct 18, 2016 at 12:59 AM, Eric Blake  wrote:
>> >> >>>
>> >> >>> On 10/17/2016 01:00 PM, Ashijeet Acharya wrote:
>> >> >>>
>> >>  One more relatively easy question though, will we include @port as an
>> >>  option in runtime_opts while converting NFS to use several
>> >>  runtime_opts? The reason I ask this because the uri syntax for NFS in
>> >>  QEMU looks like this:
>> >> 
>> >> 
>> >>  nfs:[?param=value[=value2[&...]]]
>> >> >>>
>> >> >>> It's actually nfs://[:port]/...
>> >> >>>
>> >> >>> so the URI syntax already supports port.
>> >> >>
>> >> >> But the commit message which added support for NFS had the uri which I
>> >> >> mentioned above and the code for NFS does not make use of 'port'
>> >> >> anywhere either, which is why I am a bit confused.
>> >> >
>> >> >
>> >> > Hi Aschijeet,
>> >> >
>> >> > don't worry there is no port number when connecting to an NFS server.
>> >> > The portmapper always listens on port 111. So theoretically we could
>> >> > specifiy a port in the URL but it is ignored.
>> >>
>> >> So that means I will be including 'port' in runtime_opts and then just
>> >> ignoring any value that comes through it?
>> >
>> > No, if there is nothing to configure there, leave it out. Adding an
>> > option that doesn't do anything is not very useful.
>> >
>> Okay, understood.
>>
>> So this is what my runtime_opts look like now:
>>
>> static QemuOptsList runtime_opts = {
>> .name = "nfs",
>> .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
>> .desc = {
>> {
>> .name = "host",
>> .type = QEMU_OPT_STRING,
>> .help = "Host to connect to",
>> },
>> {
>> .name = "export",
>> .type = QEMU_OPT_STRING,
>> .help = "Name of the NFS export to open",
>> },
>> {
>> .name = "path",
>> .type = QEMU_OPT_STRING,
>> .help = "Path of the image on the host",
>> },
>
> Does libnfs actually have separate export and path?
>
> If I understand correctly, we currently split the URL at the last / in
> the string. So is export the part before that and path the part after
> it?

At the moment, I don't see any piece of code in NFS BLOCK DRIVER which
does that to extract export from the URL. We actually do this at the
moment:

strp = strrchr(uri->path, '/');

which I think is just to extract the name of the file from the URL and
has nothing to do with export.
Although NBD and gluster seem to extract export from the URL like this:

p += strspn(p, "/");

which actually splits the URL at the first appearance of "/", doesn't
it? And then does after checking for p[0]:

qdict_put(options, "export", qstring_from_str(p));

which puts the value of key export in qdict as the part of the URL
which occurs after the first appearance of "/", right?

>
> If so, does this mean that you can't currently access an image file in a
> subdirectory of an NFS mount and adding the explicit options will fix
> this?

I am not sure of that :-/

Ashijeet
>
>> {
>> .name = "uid",
>> .type = QEMU_OPT_NUMBER,
>> .help = "UID value to use when talking to the server",
>> },
>> {
>> .name = "gid",
>> .type = QEMU_OPT_NUMBER,
>> .help = "GID value to use when talking to the server",
>> },
>> {
>> .name = "tcp-syncnt",
>> .type = QEMU_OPT_NUMBER,
>> .help = "Number of SYNs to send during the session establish",
>> },
>> {
>> .name = "readahead",
>> .type = QEMU_OPT_NUMBER,
>> .help = "Set the readahead size in bytes",
>> },
>> {
>> .name = "pagecache",
>> .type = QEMU_OPT_NUMBER,
>> .help = "Set the pagecache size in bytes",
>> },
>> {
>> .name = "debug",
>> .type = QEMU_OPT_NUMBER,
>> .help = "Set the NFS debug level (max 2)",
>> },
>> { /* end of list */ }
>> },
>> };
>>
>> Please check if I have missed out on anything.
>
> I don't see anything, but then I'm not an expert on NFS either. I hope
> Peter can take a look.
>
> Though maybe it's easier to see in the context of the full patch.
>
>> I have successfully converted NFS block driver to use this set of
>> runtime opts which I think is the required condition to add
>> blockdev-add compatibility later. Also, since I do not have 'port' as
>> a 

Re: [Qemu-block] [PATCH] qcow2: Optimize L2 table cache size based on image and cluster sizes

[Alberto Garcia]

On Fri 07 Oct 2016 03:58:29 PM CEST, Ed Swierk wrote:

>> I know the disk image size, and can set cache size in
>> bytes. l2-cache-size=max would be a convenience feature, *especially
>> if it could become the default*. Then I could forget thinking about
>> whether the image is larger than the current 8GB fully-cached
>> threshold, and only have to calculate cache size in bytes in case of
>> very large images, multiple backing files, or very tight memory.
>
> Same here, using libvirt. l2-cache-size=max would be ideal. Or if
> there were a cache-coverage-size option that takes an absolute number,
> libvirt could set it to the image size.

I can see the benefit of both approaches: setting the disk size covered
by the cache or setting a percentage, I personally like a bit more the
former but it wouldn't provide a way to say "create the largest cache
needed for this disk".

Would 'l2-cache-coverage-size' work for everyone? It would simply take
the disk size (16G, 1T, etc) and it would conflict with l2-cache-size.

Do we need something similar for the refcount cache?

Berto

Re: [Qemu-block] [Qemu-devel] [PATCH v14 02/21] qdict: implement a qdict_crumple method for un-flattening a dict

[Markus Armbruster]

"Daniel P. Berrange"  writes:

> The qdict_flatten() method will take a dict whose elements are
> further nested dicts/lists and flatten them by concatenating
> keys.
>
> The qdict_crumple() method aims to do the reverse, taking a flat
> qdict, and turning it into a set of nested dicts/lists. It will
> apply nesting based on the key name, with a '.' indicating a
> new level in the hierarchy. If the keys in the nested structure
> are all numeric, it will create a list, otherwise it will create
> a dict.
>
> If the keys are a mixture of numeric and non-numeric, or the
> numeric keys are not in strictly ascending order, an error will
> be reported.
>
> As an example, a flat dict containing
>
>  {
>'foo.0.bar': 'one',
>'foo.0.wizz': '1',
>'foo.1.bar': 'two',
>'foo.1.wizz': '2'
>  }
>
> will get turned into a dict with one element 'foo' whose
> value is a list. The list elements will each in turn be
> dicts.
>
>  {
>'foo': [
>  { 'bar': 'one', 'wizz': '1' },
>  { 'bar': 'two', 'wizz': '2' }
>],
>  }
>
> If the key is intended to contain a literal '.', then it must
> be escaped as '..'. ie a flat dict
>
>   {
>  'foo..bar': 'wizz',
>  'bar.foo..bar': 'eek',
>  'bar.hello': 'world'
>   }
>
> Will end up as
>
>   {
>  'foo.bar': 'wizz',
>  'bar': {
> 'foo.bar': 'eek',
> 'hello': 'world'
>  }
>   }
>
> The intent of this function is that it allows a set of QemuOpts
> to be turned into a nested data structure that mirrors the nesting
> used when the same object is defined over QMP.
>
> Reviewed-by: Eric Blake 
> Reviewed-by: Kevin Wolf 
> Reviewed-by: Marc-André Lureau 
> Signed-off-by: Daniel P. Berrange 
> ---
>  include/qapi/qmp/qdict.h |   1 +
>  qobject/qdict.c  | 289 
> +++
>  tests/check-qdict.c  | 261 ++
>  3 files changed, 551 insertions(+)
>
> diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> index 71b8eb0..e0d24e1 100644
> --- a/include/qapi/qmp/qdict.h
> +++ b/include/qapi/qmp/qdict.h
> @@ -73,6 +73,7 @@ void qdict_flatten(QDict *qdict);
>  void qdict_extract_subqdict(QDict *src, QDict **dst, const char *start);
>  void qdict_array_split(QDict *src, QList **dst);
>  int qdict_array_entries(QDict *src, const char *subqdict);
> +QObject *qdict_crumple(const QDict *src, bool recursive, Error **errp);
>  
>  void qdict_join(QDict *dest, QDict *src, bool overwrite);
>  
> diff --git a/qobject/qdict.c b/qobject/qdict.c
> index 60f158c..c38e90e 100644
> --- a/qobject/qdict.c
> +++ b/qobject/qdict.c
[...]
> +/**
> + * qdict_crumple:
> + * @src: the original flat dictionary (only scalar values) to crumple
> + * @recursive: true to recursively crumple nested dictionaries

Is recursive=false used outside tests in this series?

> + *
> + * Takes a flat dictionary whose keys use '.' separator to indicate
> + * nesting, and values are scalars, and crumples it into a nested
> + * structure. If the @recursive parameter is false, then only the
> + * first level of structure implied by the keys will be crumpled. If
> + * @recursive is true, then the input will be recursively crumpled to
> + * expand all levels of structure in the keys.
> + *
> + * To include a literal '.' in a key name, it must be escaped as '..'
> + *
> + * For example, an input of:
> + *
> + * { 'foo.0.bar': 'one', 'foo.0.wizz': '1',
> + *   'foo.1.bar': 'two', 'foo.1.wizz': '2' }
> + *
> + * will result in an output of:
> + *
> + * {
> + *   'foo': [
> + *  { 'bar': 'one', 'wizz': '1' },
> + *  { 'bar': 'two', 'wizz': '2' }
> + *   ],
> + * }
> + *
> + * The following scenarios in the input dict will result in an
> + * error being returned:
> + *
> + *  - Any values in @src are non-scalar types
> + *  - If keys in @src imply that a particular level is both a
> + *list and a dict. e.g., "foo.0.bar" and "foo.eek.bar".
> + *  - If keys in @src imply that a particular level is a list,
> + *but the indices are non-contiguous. e.g. "foo.0.bar" and
> + *"foo.2.bar" without any "foo.1.bar" present.
> + *  - If keys in @src represent list indexes, but are not in
> + *the "%zu" format. e.g. "foo.+0.bar"
> + *
> + * Returns: either a QDict or QList for the nested data structure, or NULL
> + * on error
> + */
> +QObject *qdict_crumple(const QDict *src, bool recursive, Error **errp)
[...]

Re: [Qemu-block] [Qemu-devel] block/nfs: Fine grained runtime options in nfs

[Eric Blake]

On 10/18/2016 08:33 AM, Kevin Wolf wrote:

>> I have successfully converted NFS block driver to use this set of
>> runtime opts which I think is the required condition to add
>> blockdev-add compatibility later. Also, since I do not have 'port' as
>> a runtime option, I can directly add blockdev-add compatibility after
>> this through qapi/block-core.json and will not have to go through the
>> tricky method we are implementing for NBD and SSH as there will be no
>> use of InetSocketAddress. Right?
> 
> Yes, InetSocketAddress is what makes things a bit tricky, and it doesn't
> seem to be useful with the API we get from libnfs, so just directly
> taking a host name should be okay. Then this one should be easier than
> SSH.
> 
> Eric, do you agree, or do you think we should take into account that
> libnfs might be extended one day to work on any socket?

Ideally, we want the valid JSON for ssh to be a subset of the valid JSON
for either InetSocketAddress, or for a flat counterpart (what we did for
gluster).  I kind of like the flat counterpart idea.  Yes, that probably
means we need to create a new QAPI type (comparable to the existing
types, but omitting port), rather than being able to reuse one; but as
long as the parameters are spelled the same, backwards-compatibility
states that we can later add fields, and that any two structs with
identical fields can be merged into one struct without breaking
backwards compatibility.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 0/2] Correct access to wrong BlockBackendPublic structures

[Kevin Wolf]

Am 17.10.2016 um 17:46 hat Alberto Garcia geschrieben:
> Hi all,
> 
> Paolo found that commit 27ccdd52598290f introduced a regression in the
> throttling code.
> 
> It can be easily reproduced in scenarios where you have a throttling
> group with several drives but you only write to one of them. In that
> case the round-robin algorithm can select the wrong drive all the time
> and the actual requests are never completed.
> 
> QEMU 2.7 is affected, here's the patch to fix it, plus a test case.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [PATCH v2] rbd: make the code more readable

[Kevin Wolf]

Am 15.10.2016 um 10:26 hat Xiubo Li geschrieben:
> Make it a bit clearer and more readable.
> 
> Signed-off-by: Xiubo Li 
> CC: John Snow 
> ---
> 
> V2:
> - Advice from John Snow. Thanks.

Copying the official maintainers:

$ scripts/get_maintainer.pl -f block/rbd.c
Josh Durgin  (supporter:RBD)
Jeff Cody  (supporter:RBD)
...

> 
>  block/rbd.c | 25 -
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 0a5840d..d0d4b39 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -366,45 +366,44 @@ static int qemu_rbd_create(const char *filename, 
> QemuOpts *opts, Error **errp)
>  rados_conf_read_file(cluster, NULL);
>  } else if (conf[0] != '\0' &&
> qemu_rbd_set_conf(cluster, conf, true, _err) < 0) {
> -rados_shutdown(cluster);
>  error_propagate(errp, local_err);
> -return -EIO;
> +ret = -EIO;
> +goto shutdown;
>  }
>  
>  if (conf[0] != '\0' &&
>  qemu_rbd_set_conf(cluster, conf, false, _err) < 0) {
> -rados_shutdown(cluster);
>  error_propagate(errp, local_err);
> -return -EIO;
> +ret = -EIO;
> +goto shutdown;
>  }
>  
>  if (qemu_rbd_set_auth(cluster, secretid, errp) < 0) {
> -rados_shutdown(cluster);
> -return -EIO;
> +ret = -EIO;
> +goto shutdown;
>  }
>  
>  ret = rados_connect(cluster);
>  if (ret < 0) {
>  error_setg_errno(errp, -ret, "error connecting");
> -rados_shutdown(cluster);
> -return ret;
> +goto shutdown;
>  }
>  
>  ret = rados_ioctx_create(cluster, pool, _ctx);
>  if (ret < 0) {
>  error_setg_errno(errp, -ret, "error opening pool %s", pool);
> -rados_shutdown(cluster);
> -return ret;
> +goto shutdown;
>  }
>  
>  ret = rbd_create(io_ctx, name, bytes, _order);
> -rados_ioctx_destroy(io_ctx);
> -rados_shutdown(cluster);
>  if (ret < 0) {
>  error_setg_errno(errp, -ret, "error rbd create");
> -return ret;
>  }
>  
> +rados_ioctx_destroy(io_ctx);
> +
> +shutdown:
> +rados_shutdown(cluster);
>  return ret;
>  }
>  
> -- 
> 1.8.3.1
> 
> 
> 
> 

Re: [Qemu-block] [Qemu-devel] block/nfs: Fine grained runtime options in nfs

[Kevin Wolf]

Am 18.10.2016 um 15:14 hat Ashijeet Acharya geschrieben:
> On Tue, Oct 18, 2016 at 6:34 PM, Kevin Wolf  wrote:
> > Am 18.10.2016 um 14:46 hat Ashijeet Acharya geschrieben:
> >> On Tue, Oct 18, 2016 at 4:11 PM, Peter Lieven  wrote:
> >> > Am 17.10.2016 um 21:34 schrieb Ashijeet Acharya:
> >> >>
> >> >> On Tue, Oct 18, 2016 at 12:59 AM, Eric Blake  wrote:
> >> >>>
> >> >>> On 10/17/2016 01:00 PM, Ashijeet Acharya wrote:
> >> >>>
> >>  One more relatively easy question though, will we include @port as an
> >>  option in runtime_opts while converting NFS to use several
> >>  runtime_opts? The reason I ask this because the uri syntax for NFS in
> >>  QEMU looks like this:
> >> 
> >> 
> >>  nfs:[?param=value[=value2[&...]]]
> >> >>>
> >> >>> It's actually nfs://[:port]/...
> >> >>>
> >> >>> so the URI syntax already supports port.
> >> >>
> >> >> But the commit message which added support for NFS had the uri which I
> >> >> mentioned above and the code for NFS does not make use of 'port'
> >> >> anywhere either, which is why I am a bit confused.
> >> >
> >> >
> >> > Hi Aschijeet,
> >> >
> >> > don't worry there is no port number when connecting to an NFS server.
> >> > The portmapper always listens on port 111. So theoretically we could
> >> > specifiy a port in the URL but it is ignored.
> >>
> >> So that means I will be including 'port' in runtime_opts and then just
> >> ignoring any value that comes through it?
> >
> > No, if there is nothing to configure there, leave it out. Adding an
> > option that doesn't do anything is not very useful.
> >
> Okay, understood.
> 
> So this is what my runtime_opts look like now:
> 
> static QemuOptsList runtime_opts = {
> .name = "nfs",
> .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> .desc = {
> {
> .name = "host",
> .type = QEMU_OPT_STRING,
> .help = "Host to connect to",
> },
> {
> .name = "export",
> .type = QEMU_OPT_STRING,
> .help = "Name of the NFS export to open",
> },
> {
> .name = "path",
> .type = QEMU_OPT_STRING,
> .help = "Path of the image on the host",
> },

Does libnfs actually have separate export and path?

If I understand correctly, we currently split the URL at the last / in
the string. So is export the part before that and path the part after
it?

If so, does this mean that you can't currently access an image file in a
subdirectory of an NFS mount and adding the explicit options will fix
this?

> {
> .name = "uid",
> .type = QEMU_OPT_NUMBER,
> .help = "UID value to use when talking to the server",
> },
> {
> .name = "gid",
> .type = QEMU_OPT_NUMBER,
> .help = "GID value to use when talking to the server",
> },
> {
> .name = "tcp-syncnt",
> .type = QEMU_OPT_NUMBER,
> .help = "Number of SYNs to send during the session establish",
> },
> {
> .name = "readahead",
> .type = QEMU_OPT_NUMBER,
> .help = "Set the readahead size in bytes",
> },
> {
> .name = "pagecache",
> .type = QEMU_OPT_NUMBER,
> .help = "Set the pagecache size in bytes",
> },
> {
> .name = "debug",
> .type = QEMU_OPT_NUMBER,
> .help = "Set the NFS debug level (max 2)",
> },
> { /* end of list */ }
> },
> };
> 
> Please check if I have missed out on anything.

I don't see anything, but then I'm not an expert on NFS either. I hope
Peter can take a look.

Though maybe it's easier to see in the context of the full patch.

> I have successfully converted NFS block driver to use this set of
> runtime opts which I think is the required condition to add
> blockdev-add compatibility later. Also, since I do not have 'port' as
> a runtime option, I can directly add blockdev-add compatibility after
> this through qapi/block-core.json and will not have to go through the
> tricky method we are implementing for NBD and SSH as there will be no
> use of InetSocketAddress. Right?

Yes, InetSocketAddress is what makes things a bit tricky, and it doesn't
seem to be useful with the API we get from libnfs, so just directly
taking a host name should be okay. Then this one should be easier than
SSH.

Eric, do you agree, or do you think we should take into account that
libnfs might be extended one day to work on any socket?

Kevin

Re: [Qemu-block] [Qemu-devel] block/nfs: Fine grained runtime options in nfs

[Ashijeet Acharya]

On Tue, Oct 18, 2016 at 6:34 PM, Kevin Wolf  wrote:
> Am 18.10.2016 um 14:46 hat Ashijeet Acharya geschrieben:
>> On Tue, Oct 18, 2016 at 4:11 PM, Peter Lieven  wrote:
>> > Am 17.10.2016 um 21:34 schrieb Ashijeet Acharya:
>> >>
>> >> On Tue, Oct 18, 2016 at 12:59 AM, Eric Blake  wrote:
>> >>>
>> >>> On 10/17/2016 01:00 PM, Ashijeet Acharya wrote:
>> >>>
>>  One more relatively easy question though, will we include @port as an
>>  option in runtime_opts while converting NFS to use several
>>  runtime_opts? The reason I ask this because the uri syntax for NFS in
>>  QEMU looks like this:
>> 
>> 
>>  nfs:[?param=value[=value2[&...]]]
>> >>>
>> >>> It's actually nfs://[:port]/...
>> >>>
>> >>> so the URI syntax already supports port.
>> >>
>> >> But the commit message which added support for NFS had the uri which I
>> >> mentioned above and the code for NFS does not make use of 'port'
>> >> anywhere either, which is why I am a bit confused.
>> >
>> >
>> > Hi Aschijeet,
>> >
>> > don't worry there is no port number when connecting to an NFS server.
>> > The portmapper always listens on port 111. So theoretically we could
>> > specifiy a port in the URL but it is ignored.
>>
>> So that means I will be including 'port' in runtime_opts and then just
>> ignoring any value that comes through it?
>
> No, if there is nothing to configure there, leave it out. Adding an
> option that doesn't do anything is not very useful.
>
Okay, understood.

So this is what my runtime_opts look like now:

static QemuOptsList runtime_opts = {
.name = "nfs",
.head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
.desc = {
{
.name = "host",
.type = QEMU_OPT_STRING,
.help = "Host to connect to",
},
{
.name = "export",
.type = QEMU_OPT_STRING,
.help = "Name of the NFS export to open",
},
{
.name = "path",
.type = QEMU_OPT_STRING,
.help = "Path of the image on the host",
},
{
.name = "uid",
.type = QEMU_OPT_NUMBER,
.help = "UID value to use when talking to the server",
},
{
.name = "gid",
.type = QEMU_OPT_NUMBER,
.help = "GID value to use when talking to the server",
},
{
.name = "tcp-syncnt",
.type = QEMU_OPT_NUMBER,
.help = "Number of SYNs to send during the session establish",
},
{
.name = "readahead",
.type = QEMU_OPT_NUMBER,
.help = "Set the readahead size in bytes",
},
{
.name = "pagecache",
.type = QEMU_OPT_NUMBER,
.help = "Set the pagecache size in bytes",
},
{
.name = "debug",
.type = QEMU_OPT_NUMBER,
.help = "Set the NFS debug level (max 2)",
},
{ /* end of list */ }
},
};

Please check if I have missed out on anything.
I have successfully converted NFS block driver to use this set of
runtime opts which I think is the required condition to add
blockdev-add compatibility later. Also, since I do not have 'port' as
a runtime option, I can directly add blockdev-add compatibility after
this through qapi/block-core.json and will not have to go through the
tricky method we are implementing for NBD and SSH as there will be no
use of InetSocketAddress. Right?

Ashijeet
> Kevin

Re: [Qemu-block] [Qemu-devel] block/nfs: Fine grained runtime options in nfs

[Kevin Wolf]

Am 18.10.2016 um 14:46 hat Ashijeet Acharya geschrieben:
> On Tue, Oct 18, 2016 at 4:11 PM, Peter Lieven  wrote:
> > Am 17.10.2016 um 21:34 schrieb Ashijeet Acharya:
> >>
> >> On Tue, Oct 18, 2016 at 12:59 AM, Eric Blake  wrote:
> >>>
> >>> On 10/17/2016 01:00 PM, Ashijeet Acharya wrote:
> >>>
>  One more relatively easy question though, will we include @port as an
>  option in runtime_opts while converting NFS to use several
>  runtime_opts? The reason I ask this because the uri syntax for NFS in
>  QEMU looks like this:
> 
> 
>  nfs:[?param=value[=value2[&...]]]
> >>>
> >>> It's actually nfs://[:port]/...
> >>>
> >>> so the URI syntax already supports port.
> >>
> >> But the commit message which added support for NFS had the uri which I
> >> mentioned above and the code for NFS does not make use of 'port'
> >> anywhere either, which is why I am a bit confused.
> >
> >
> > Hi Aschijeet,
> >
> > don't worry there is no port number when connecting to an NFS server.
> > The portmapper always listens on port 111. So theoretically we could
> > specifiy a port in the URL but it is ignored.
> 
> So that means I will be including 'port' in runtime_opts and then just
> ignoring any value that comes through it?

No, if there is nothing to configure there, leave it out. Adding an
option that doesn't do anything is not very useful.

Kevin

Re: [Qemu-block] [PATCH v2] raw_bsd: add offset and size options

[Kevin Wolf]

Am 18.10.2016 um 00:25 hat Tomáš Golembiovský geschrieben:
> Added two new options 'offset' and 'size'. This makes it possible to use
> only part of the file as a device. This can be used e.g. to limit the
> access only to single partition in a disk image or use a disk inside a
> tar archive (like OVA).
> 
> When 'size' is specified we do our best to honour it.
> 
> Signed-off-by: Tomáš Golembiovský 
> ---
>  block/raw_bsd.c  | 169 
> ++-
>  qapi/block-core.json |  16 -
>  2 files changed, 181 insertions(+), 4 deletions(-)
> 
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 588d408..3fb3f13 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -31,6 +31,30 @@
>  #include "qapi/error.h"
>  #include "qemu/option.h"
>  
> +typedef struct BDRVRawState {
> +uint64_t offset;
> +uint64_t size;
> +bool has_size;
> +} BDRVRawState;
> +
> +static QemuOptsList raw_runtime_opts = {
> +.name = "raw",
> +.head = QTAILQ_HEAD_INITIALIZER(raw_runtime_opts.head),
> +.desc = {
> +{
> +.name = "offset",
> +.type = QEMU_OPT_SIZE,
> +.help = "offset in the disk where the image starts",
> +},
> +{
> +.name = "size",
> +.type = QEMU_OPT_SIZE,
> +.help = "virtual disk size",
> +},
> +{ /* end of list */ }
> +},
> +};
> +
>  static QemuOptsList raw_create_opts = {
>  .name = "raw-create-opts",
>  .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
> @@ -44,17 +68,107 @@ static QemuOptsList raw_create_opts = {
>  }
>  };
>  
> +static int raw_read_options(QDict *options, BlockDriverState *bs,
> +BDRVRawState *s, Error **errp)
> +{
> +Error *local_err = NULL;
> +QemuOpts *opts = NULL;
> +int64_t real_size = 0;
> +int ret;
> +
> +real_size = bdrv_getlength(bs->file->bs);
> +if (real_size < 0) {
> +error_setg_errno(errp, -real_size, "Could not get image size");
> +return real_size;
> +}
> +
> +opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
> +qemu_opts_absorb_qdict(opts, options, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +ret = -EINVAL;
> +goto fail;
> +}
> +
> +s->offset = qemu_opt_get_size(opts, "offset", 0);
> +if (qemu_opt_find(opts, "size") != NULL) {
> +s->size = qemu_opt_get_size(opts, "size", 0);
> +s->has_size = true;
> +} else {
> +s->has_size = false;
> +s->size = real_size;
> +}
> +
> +/* Check size and offset */
> +if (real_size < s->offset || (real_size - s->offset) < s->size) {
> +error_setg(errp, "The sum of offset (%"PRIu64") and size "
> +"(%"PRIu64") has to be smaller or equal to the "
> +" actual size of the containing file (%"PRId64").",
> +s->offset, s->size, real_size);
> +ret = -EINVAL;
> +goto fail;
> +}
> +
> +/* Make sure size is multiple of BDRV_SECTOR_SIZE to prevent rounding
> + * up and leaking out of the specified area. */
> +if (s->size != QEMU_ALIGN_DOWN(s->size, BDRV_SECTOR_SIZE)) {
> +s->size = QEMU_ALIGN_DOWN(s->size, BDRV_SECTOR_SIZE);
> +fprintf(stderr,
> +"WARNING: Specified size is not multiple of %llu! "
> +"Rounding down to %"PRIu64 ". (End of the image will be "
> +"ignored.)\n",
> +BDRV_SECTOR_SIZE, s->size);

If we wanted this behaviour, this should use error_report() instead of
fprintf() so that the message is printed to the monitor if that's where
the request came from.

But as I already replied on the cover letter, I think we should just
make it a hard error.

> +}
> +
> +ret = 0;
> +
> +fail:
> +
> +qemu_opts_del(opts);
> +
> +return ret;
> +}
> +
>  static int raw_reopen_prepare(BDRVReopenState *reopen_state,
>BlockReopenQueue *queue, Error **errp)
>  {
> -return 0;
> +assert(reopen_state != NULL);
> +assert(reopen_state->bs != NULL);
> +
> +reopen_state->opaque = g_new0(BDRVRawState, 1);
> +
> +return raw_read_options(
> +reopen_state->options,
> +reopen_state->bs,
> +reopen_state->opaque,
> +errp);
> +}
> +
> +static void raw_reopen_commit(BDRVReopenState *state)
> +{
> +BDRVRawState *new_s = state->opaque;
> +BDRVRawState *s = state->bs->opaque;
> +
> +memcpy(s, new_s, sizeof(BDRVRawState));
> +
> +g_free(state->opaque);
> +state->opaque = NULL;
> +}
> +
> +static void raw_reopen_abort(BDRVReopenState *state)
> +{
> +g_free(state->opaque);
> +state->opaque = NULL;
>  }
>  
>  static int coroutine_fn raw_co_preadv(BlockDriverState *bs, uint64_t offset,
>uint64_t bytes, QEMUIOVector *qiov,
>int flags)
>  {
> +BDRVRawState 

Re: [Qemu-block] [Qemu-devel] block/nfs: Fine grained runtime options in nfs

[Ashijeet Acharya]

On Tue, Oct 18, 2016 at 4:11 PM, Peter Lieven  wrote:
> Am 17.10.2016 um 21:34 schrieb Ashijeet Acharya:
>>
>> On Tue, Oct 18, 2016 at 12:59 AM, Eric Blake  wrote:
>>>
>>> On 10/17/2016 01:00 PM, Ashijeet Acharya wrote:
>>>
 One more relatively easy question though, will we include @port as an
 option in runtime_opts while converting NFS to use several
 runtime_opts? The reason I ask this because the uri syntax for NFS in
 QEMU looks like this:


 nfs:[?param=value[=value2[&...]]]
>>>
>>> It's actually nfs://[:port]/...
>>>
>>> so the URI syntax already supports port.
>>
>> But the commit message which added support for NFS had the uri which I
>> mentioned above and the code for NFS does not make use of 'port'
>> anywhere either, which is why I am a bit confused.
>
>
> Hi Aschijeet,
>
> don't worry there is no port number when connecting to an NFS server.
> The portmapper always listens on port 111. So theoretically we could
> specifiy a port in the URL but it is ignored.

So that means I will be including 'port' in runtime_opts and then just
ignoring any value that comes through it?

Ashijeet
>
> BR,
> Peter

Re: [Qemu-block] [Qemu-devel] [PATCH v2] raw_bsd: add offset and size options

[Daniel P. Berrange]

On Tue, Oct 18, 2016 at 12:25:17AM +0200, Tomáš Golembiovský wrote:
> Added two new options 'offset' and 'size'. This makes it possible to use
> only part of the file as a device. This can be used e.g. to limit the
> access only to single partition in a disk image or use a disk inside a
> tar archive (like OVA).
> 
> When 'size' is specified we do our best to honour it.
> 
> Signed-off-by: Tomáš Golembiovský 
> ---
>  block/raw_bsd.c  | 169 
> ++-
>  qapi/block-core.json |  16 -
>  2 files changed, 181 insertions(+), 4 deletions(-)
> 
> diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> index 588d408..3fb3f13 100644
> --- a/block/raw_bsd.c
> +++ b/block/raw_bsd.c
> @@ -31,6 +31,30 @@
>  #include "qapi/error.h"
>  #include "qemu/option.h"
>  
> +typedef struct BDRVRawState {
> +uint64_t offset;
> +uint64_t size;
> +bool has_size;
> +} BDRVRawState;
> +
> +static QemuOptsList raw_runtime_opts = {
> +.name = "raw",
> +.head = QTAILQ_HEAD_INITIALIZER(raw_runtime_opts.head),
> +.desc = {
> +{
> +.name = "offset",
> +.type = QEMU_OPT_SIZE,
> +.help = "offset in the disk where the image starts",
> +},
> +{
> +.name = "size",
> +.type = QEMU_OPT_SIZE,
> +.help = "virtual disk size",
> +},
> +{ /* end of list */ }
> +},
> +};
> +
>  static QemuOptsList raw_create_opts = {
>  .name = "raw-create-opts",
>  .head = QTAILQ_HEAD_INITIALIZER(raw_create_opts.head),
> @@ -44,17 +68,107 @@ static QemuOptsList raw_create_opts = {
>  }
>  };
>  
> +static int raw_read_options(QDict *options, BlockDriverState *bs,
> +BDRVRawState *s, Error **errp)
> +{
> +Error *local_err = NULL;
> +QemuOpts *opts = NULL;
> +int64_t real_size = 0;
> +int ret;
> +
> +real_size = bdrv_getlength(bs->file->bs);
> +if (real_size < 0) {
> +error_setg_errno(errp, -real_size, "Could not get image size");
> +return real_size;
> +}
> +
> +opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
> +qemu_opts_absorb_qdict(opts, options, _err);
> +if (local_err) {
> +error_propagate(errp, local_err);
> +ret = -EINVAL;
> +goto fail;
> +}
> +
> +s->offset = qemu_opt_get_size(opts, "offset", 0);
> +if (qemu_opt_find(opts, "size") != NULL) {
> +s->size = qemu_opt_get_size(opts, "size", 0);
> +s->has_size = true;
> +} else {
> +s->has_size = false;
> +s->size = real_size;
> +}
> +
> +/* Check size and offset */
> +if (real_size < s->offset || (real_size - s->offset) < s->size) {
> +error_setg(errp, "The sum of offset (%"PRIu64") and size "
> +"(%"PRIu64") has to be smaller or equal to the "
> +" actual size of the containing file (%"PRId64").",
> +s->offset, s->size, real_size);
> +ret = -EINVAL;
> +goto fail;
> +}
> +
> +/* Make sure size is multiple of BDRV_SECTOR_SIZE to prevent rounding
> + * up and leaking out of the specified area. */
> +if (s->size != QEMU_ALIGN_DOWN(s->size, BDRV_SECTOR_SIZE)) {
> +s->size = QEMU_ALIGN_DOWN(s->size, BDRV_SECTOR_SIZE);
> +fprintf(stderr,
> +"WARNING: Specified size is not multiple of %llu! "
> +"Rounding down to %"PRIu64 ". (End of the image will be "
> +"ignored.)\n",
> +BDRV_SECTOR_SIZE, s->size);

IMHO you should be using error_setg here - I don't see a compelling
reason to let execution continue with incorrect values set.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://entangle-photo.org   -o-http://search.cpan.org/~danberr/ :|

Re: [Qemu-block] [PATCH v2] Add 'offset' and 'size' options

[Kevin Wolf]

Am 18.10.2016 um 00:25 hat Tomáš Golembiovský geschrieben:
> This is a follow-up to the patch:
> [PATCH] raw-posix: add 'offset' and 'size' options
> 
> The main changes are:
>  -  options were moved from 'file' driver into 'raw' driver as suggested
>  -  added support for writing, reopen and truncate when possible
> 
> If I forgot to address somebody's comments feel free to raise them again,
> please.
> 
> Some general notes to the code:
> 
> 1)  The size is rounded *down* to the 512 byte boundary. It's not that
> the raw driver really cares about this, but if we don't do it then 
> bdrv_getlength() will do that instead of us. The problem is that
> bdrv_getlength() does round *up* and this can lead to reads/writes
> outside the specified 'size'.

I think it might be better to just check whether offset/size are
correctly aligned and error out if they aren't.

Then once we made the necessary changes to allow byte alignment (I think
what prevents it is mostly bs->total_sectors, right?) we can allow that
in the raw format driver, which will only make previously failing
options work rather than changing the behaviour of already working
configurations (we can't do the latter without a very good justification
because of compatibility).

> 2)  We don't provide '.bdrv_get_allocated_file_size' function. As a
> result the information about allocated disk size reports size of the
> whole file. This is, rather confusingly, larger than the provided
> 'size'. But I don't think this matters much. Note that we don't have
> any easy way how to get the correct information here apart from
> checking all the block with bdrv_co_get_block_status() (as suggested
> by Kevin Wolf).
> 
> 3)  No options for raw_create(). The 'size' and 'offset' options were
> added only to open/reopen. In my opinion there is no real reason for
> them there. AFAIK you cannot create embeded QCOW2/VMDK/etc. image
> that way anyway.

These two things are fine with me.

Kevin

Re: [Qemu-block] [PATCH v2] qapi: fix memory leak in bdrv_image_info_specific_dump

[Kevin Wolf]

Am 18.10.2016 um 12:37 hat Pino Toscano geschrieben:
> The 'obj' result of the visitor was not properly freed, like done in
> other places doing a similar job.
> 
> Signed-off-by: Pino Toscano 

Thanks, applied to my block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v2] qapi: fix memory leak in bdrv_image_info_specific_dump

[Eric Blake]

On 10/18/2016 05:37 AM, Pino Toscano wrote:
> The 'obj' result of the visitor was not properly freed, like done in
> other places doing a similar job.
> 
> Signed-off-by: Pino Toscano 
> ---

Reviewed-by: Eric Blake 

> 
> Changes in v2:
> - added Signed-off-by
> 
>  block/qapi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 6f947e3..50d3090 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -698,6 +698,7 @@ void bdrv_image_info_specific_dump(fprintf_function 
> func_fprintf, void *f,
>  assert(qobject_type(obj) == QTYPE_QDICT);
>  data = qdict_get(qobject_to_qdict(obj), "data");
>  dump_qobject(func_fprintf, f, 1, data);
> +qobject_decref(obj);
>  visit_free(v);
>  }
>  
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v14 13/21] qdict: allow qdict_crumple to accept compound types as values

[Markus Armbruster]

Eric Blake  writes:

> On 10/17/2016 09:50 AM, Markus Armbruster wrote:
>>> But even if I realised that QemuOpts support this syntax, I think we
>>> would still have to use the dotted syntax because it's explicit about
>>> the index and we need that because the list can contains dicts.
>>>
>>> Compare this:
>>>
>>> driver=quorum,
>>> child.0.driver=file,child.0.filename=disk1.img,
>>> child.1.driver=host_device,child.1.filename=/dev/sdb,
>>> child.2.driver=nbd,child.2.host=localhost
>>>
>>> And this:
>>>
>>> driver=quorum,
>>> child.driver=file,child.filename=disk1.img,
>>> child.driver=host_device,child.filename=/dev/sdb,
>>> child.driver=nbd,child.host=localhost
>> 
>> Aside: both are about equally illegible, to be honest.
>
>> Permit me to digress.
>> 
>> QemuOpts wasn't designed for list-values keys.  Doing lists by
>> repetition was clever.
>> 
>> QemuOpts wasn't designed for structured values.  Doing structured values
>> by a dotted key convention plus repetition was clever.
>> 
>> And there's the problem: too much cleverness, not enough "this is being
>> pushed way beyond its design limits, time to replace it".
>> 
>> For me, a replacement should do structured values by providing suitable
>> *value* syntax instead of hacking it into the keys:
>> 
>> { "driver": "quorum",
>>   "child": [ { "driver": "file", "filename": "disk1.img" },
>>  { "driver": "host_device", "filename=/dev/sdb" },
>>  { "driver": "nbd", "host": "localhost" } ] }
>
> Possible hack solution:
>
> QemuOpts already special-cases id=.  What if we ALSO make it
> special-case a leading json=?  Shown here with shell quoting, the above
> example of creating a Quorum -drive argument could then be:
>
> -drive json='
> { "driver": "quorum",
>   "child": [ { "driver": "file", "filename": "disk1.img" },
>  { "driver": "host_device", "filename=/dev/sdb" },
>  { "driver": "nbd", "host": "localhost" } ] }
> '
>
> As far as I know, we don't have 'json' as any existing QemuOpts key (do
> we? A full audit may be better than my quick git grep '"json"').  Thus,
> if QemuOpts sees a leading json=, it hands off the rest of the string to
> the same parser as we use for QMP, where we no longer have to escape
> commas (even nicer than the drive hack where we support
> filename=json:{...} but have to double up all commas to make it through
> the QemuOpts layer).  Encountering json= as anything other than the
> first option would be an error, and you would be unable to combine a
> json= option with any other old-style option.  In other words, the use
> of leading json= would be the switch for whether to do old-style parsing
> or to use a saner syntax for everything else that needs structure, on a
> per-argument basis.

Slight variation: omit the 'json=' and recognize the '{':

-drive '{ "driver": "quorum",
  "child": [ { "driver": "file", "filename": "disk1.img" },
 { "driver": "host_device", "filename=/dev/sdb" },
 { "driver": "nbd", "host": "localhost" } ] }'

As always when extending haphazardly defined syntax, the question to ask
is whether this makes the syntax (more) ambiguous.

So what is the option argument syntax now?  The abstract syntax is
simple enough: "list of (key, value) pairs".  For the concrete syntax,
we need to study opts_do_parse().

Each iteration of its loop accepts an abstract (key, value).  It first
looks for the leftmost '=' and ','.  Cases:

1. There is no '=', or it is to the right of the leftmost ','.  In other
words, this (key, value) can only be key with an implied value or a
value with an implied key.

1a. If this is the first iteration, and we accept an implied key, this
is a value for the implied key.  We consume everything up to the first
non-escaped ','.  This may be more than the leftmost ',' we found above.
The consumed string with escapes processed is the value.

1b. Else, this is a key with an implied value.  We consume everything up
to the leftmost ',' (no escaping here).

1b1. If the consumed string starts with "no", the key is everything after
the "no" and the value is "off".

1b2. Else the key is the string and the value is "on".

2. This is a key and a value.  We first consume everything up to the
leftmost '=' (no escaping here).  The consumed string is the key.  We
then consume '='.  Finally, we consume everything up to the first
non-escaped ','The consumed string with escapes processed is the value.

Thus, the option argument starts either with a key (case 1b1, 2), "no"
(case 1b2) or a value (case 1a).

Adding JSON object syntax (which always starts with '{') is ambiguous
when a key can start with '{' (case 1b1, 2) or when a value can (case
1a).

Keys starting with '{' are basically foolish.  Let's outlaw them by
adopting QAPI's name rules.

Values starting with '{' are possible.  The implied keys I can remember
don't 

[Qemu-block] [PATCH v2] qapi: fix memory leak in bdrv_image_info_specific_dump

[Pino Toscano]

The 'obj' result of the visitor was not properly freed, like done in
other places doing a similar job.

Signed-off-by: Pino Toscano 
---

Changes in v2:
- added Signed-off-by

 block/qapi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qapi.c b/block/qapi.c
index 6f947e3..50d3090 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -698,6 +698,7 @@ void bdrv_image_info_specific_dump(fprintf_function 
func_fprintf, void *f,
 assert(qobject_type(obj) == QTYPE_QDICT);
 data = qdict_get(qobject_to_qdict(obj), "data");
 dump_qobject(func_fprintf, f, 1, data);
+qobject_decref(obj);
 visit_free(v);
 }
 
-- 
2.7.4

Re: [Qemu-block] [PATCH] qapi: fix memory leak in bdrv_image_info_specific_dump

[Pino Toscano]

On Tuesday, 18 October 2016 11:44:26 CEST Kevin Wolf wrote:
> Am 18.10.2016 um 11:18 hat Pino Toscano geschrieben:
> > The 'obj' result of the visitor was not properly freed, like done in
> > other places doing a similar job.
> > ---
> >  block/qapi.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/block/qapi.c b/block/qapi.c
> > index 6f947e3..50d3090 100644
> > --- a/block/qapi.c
> > +++ b/block/qapi.c
> > @@ -698,6 +698,7 @@ void bdrv_image_info_specific_dump(fprintf_function 
> > func_fprintf, void *f,
> >  assert(qobject_type(obj) == QTYPE_QDICT);
> >  data = qdict_get(qobject_to_qdict(obj), "data");
> >  dump_qobject(func_fprintf, f, 1, data);
> > +qobject_decref(obj);
> >  visit_free(v);
> >  }
> 
> The change looks good, but without a Signed-off-by line I can't accept
> the patch: http://wiki.qemu.org/Contribute/SubmitAPatch

D'oh, sorry -- apparently I didn't send the right version... v2 coming.

Thanks,
-- 
Pino Toscano

signature.asc
Description: This is a digitally signed message part.

[Qemu-block] [PATCH v3 4/4] qemu-iotests: Test creating floppy drives

[Kevin Wolf]

This tests the different supported methods to create floppy drives and
how they interact.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/172 |  242 +
 tests/qemu-iotests/172.out | 1205 
 tests/qemu-iotests/group   |1 +
 3 files changed, 1448 insertions(+)
 create mode 100755 tests/qemu-iotests/172
 create mode 100644 tests/qemu-iotests/172.out

diff --git a/tests/qemu-iotests/172 b/tests/qemu-iotests/172
new file mode 100755
index 000..8bb6443
--- /dev/null
+++ b/tests/qemu-iotests/172
@@ -0,0 +1,242 @@
+#!/bin/bash
+#
+# Test floppy configuration
+#
+# Copyright (C) 2016 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+#
+
+# creator
+owner=kw...@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1   # failure is the default!
+
+_cleanup()
+{
+   _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
+_notrun "Requires a PC machine"
+fi
+
+function do_run_qemu()
+{
+(
+if ! test -t 0; then
+while read cmd; do
+echo $cmd
+done
+fi
+echo quit
+) | $QEMU -nographic -monitor stdio -serial none "$@"
+echo
+}
+
+function check_floppy_qtree()
+{
+echo
+echo Testing: "$@" | _filter_testdir
+
+# QEMU_OPTIONS contains -nodefaults, we don't want that here because the
+# defaults are part of what should be checked here
+echo "info qtree" |
+QEMU_OPTIONS="" do_run_qemu "$@" | _filter_win32 |
+grep -zo '[[:cntrl:]]\( *\)dev: isa-fdc.*\([[:cntrl:]]\1 .*\)*[[:cntrl:]] 
*dev:'
+}
+
+function check_cache_mode()
+{
+echo "info block none0" |
+QEMU_OPTIONS="" do_run_qemu -drive if=none,file="$TEST_IMG" "$@" |
+_filter_win32 | grep "Cache mode"
+}
+
+
+size=720k
+
+_make_test_img $size
+
+# Default drive semantics:
+#
+# By default you get a single empty floppy drive. You can override it with
+# -drive and using the same index, but if you use -drive to add a floppy to a
+# different index, you get both of them. However, as soon as you use any
+# '-device floppy', even to a different slot, the default drive is disabled.
+
+echo
+echo
+echo === Default ===
+
+check_floppy_qtree
+
+echo
+echo
+echo === Using -fda/-fdb options ===
+
+check_floppy_qtree -fda "$TEST_IMG"
+check_floppy_qtree -fdb "$TEST_IMG"
+check_floppy_qtree -fda "$TEST_IMG" -fdb "$TEST_IMG"
+
+
+echo
+echo
+echo === Using -drive options ===
+
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG"
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG",index=1
+check_floppy_qtree -drive if=floppy,file="$TEST_IMG" -drive 
if=floppy,file="$TEST_IMG",index=1
+
+echo
+echo
+echo === Using -drive if=none and -global ===
+
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveA=none0
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -global isa-fdc.driveB=none0
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+   -global isa-fdc.driveA=none0 -global isa-fdc.driveB=none1
+
+echo
+echo
+echo === Using -drive if=none and -device ===
+
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -device floppy,drive=none0
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0,unit=1
+check_floppy_qtree -drive if=none,file="$TEST_IMG" -drive 
if=none,file="$TEST_IMG" \
+   -device floppy,drive=none0 -device floppy,drive=none1,unit=1
+
+echo
+echo
+echo === Mixing -fdX and -global ===
+
+# Working
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
+
+# Conflicting (-fdX wins)
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveA=none0
+check_floppy_qtree -fdb "$TEST_IMG" -drive if=none,file="$TEST_IMG" -global 
isa-fdc.driveB=none0
+
+echo
+echo
+echo === Mixing -fdX and -device ===
+
+# Working
+check_floppy_qtree -fda "$TEST_IMG" -drive if=none,file="$TEST_IMG" -device 
floppy,drive=none0
+check_floppy_qtree -fda "$TEST_IMG" -drive 

[Qemu-block] [PATCH v3 1/4] fdc: Add a floppy qbus

[Kevin Wolf]

This adds a qbus to the floppy controller that should contain the floppy
drives eventually. At the moment it just exists and is empty.

Signed-off-by: Kevin Wolf 
Reviewed-by: John Snow 
---
 hw/block/fdc.c | 40 +++-
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index b79873a..a3afb62 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -52,6 +52,33 @@
 }   \
 } while (0)
 
+
+//
+/* qdev floppy bus  */
+
+#define TYPE_FLOPPY_BUS "Floppy"
+#define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
+
+typedef struct FDCtrl FDCtrl;
+
+typedef struct FloppyBus {
+BusState bus;
+FDCtrl *fdc;
+} FloppyBus;
+
+static const TypeInfo floppy_bus_info = {
+.name = TYPE_FLOPPY_BUS,
+.parent = TYPE_BUS,
+.instance_size = sizeof(FloppyBus),
+};
+
+static void floppy_bus_create(FDCtrl *fdc, FloppyBus *bus, DeviceState *dev)
+{
+qbus_create_inplace(bus, sizeof(FloppyBus), TYPE_FLOPPY_BUS, dev, NULL);
+bus->fdc = fdc;
+}
+
+
 //
 /* Floppy drive emulation   */
 
@@ -148,8 +175,6 @@ static FDriveSize drive_size(FloppyDriveType drive)
 #define FD_SECTOR_SC   2   /* Sector size code */
 #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */
 
-typedef struct FDCtrl FDCtrl;
-
 /* Floppy disk drive emulation */
 typedef enum FDiskFlags {
 FDISK_DBL_SIDES  = 0x01,
@@ -684,6 +709,7 @@ struct FDCtrl {
 /* Power down config (also with status regB access mode */
 uint8_t pwrd;
 /* Floppy drives */
+FloppyBus bus;
 uint8_t num_floppies;
 FDrive drives[MAX_FD];
 int reset_sensei;
@@ -2442,7 +2468,8 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
 *fdc_tc = qdev_get_gpio_in(dev, 0);
 }
 
-static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
+static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
+  Error **errp)
 {
 int i, j;
 static int command_tables_inited = 0;
@@ -2480,6 +2507,8 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error 
**errp)
 k->register_channel(fdctrl->dma, fdctrl->dma_chann,
 _transfer_handler, fdctrl);
 }
+
+floppy_bus_create(fdctrl, >bus, dev);
 fdctrl_connect_drives(fdctrl, errp);
 }
 
@@ -2508,7 +2537,7 @@ static void isabus_fdc_realize(DeviceState *dev, Error 
**errp)
 }
 
 qdev_set_legacy_instance_id(dev, isa->iobase, 2);
-fdctrl_realize_common(fdctrl, );
+fdctrl_realize_common(dev, fdctrl, );
 if (err != NULL) {
 error_propagate(errp, err);
 return;
@@ -2559,7 +2588,7 @@ static void sysbus_fdc_common_realize(DeviceState *dev, 
Error **errp)
 FDCtrlSysBus *sys = SYSBUS_FDC(dev);
 FDCtrl *fdctrl = >state;
 
-fdctrl_realize_common(fdctrl, errp);
+fdctrl_realize_common(dev, fdctrl, errp);
 }
 
 FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
@@ -2744,6 +2773,7 @@ static void fdc_register_types(void)
 type_register_static(_fdc_type_info);
 type_register_static(_fdc_info);
 type_register_static(_fdc_info);
+type_register_static(_bus_info);
 }
 
 type_init(fdc_register_types)
-- 
1.8.3.1

[Qemu-block] [PATCH v3 3/4] fdc: Move qdev properties to FloppyDrive

[Kevin Wolf]

This makes the FloppyDrive qdev object actually useful: Now that it has
all properties that don't belong to the controller, you can actually
use '-device floppy' and get a working result.

Command line semantics is consistent with CD-ROM drives: By default you
get a single empty floppy drive. You can override it with -drive and
using the same index, but if you use -drive to add a floppy to a
different index, you get both of them. However, as soon as you use any
'-device floppy', even to a different slot, the default drive is
disabled.

Using '-device floppy' without specifying the unit will choose the first
free slot on the controller.

Signed-off-by: Kevin Wolf 
---
 hw/block/fdc.c | 120 ++---
 vl.c   |   1 +
 2 files changed, 89 insertions(+), 32 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 5aa8e52..537b996 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -35,6 +35,7 @@
 #include "qemu/timer.h"
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
+#include "hw/block/block.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
@@ -487,12 +488,18 @@ static const BlockDevOps fd_block_ops = {
  OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
 
 typedef struct FloppyDrive {
-DeviceState qdev;
-uint32_tunit;
+DeviceState qdev;
+uint32_tunit;
+BlockConf   conf;
+FloppyDriveType type;
 } FloppyDrive;
 
 static Property floppy_drive_properties[] = {
 DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
+DEFINE_BLOCK_PROPERTIES(FloppyDrive, conf),
+DEFINE_PROP_DEFAULT("drive-type", FloppyDrive, type,
+FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+FloppyDriveType),
 DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -501,6 +508,7 @@ static int floppy_drive_init(DeviceState *qdev)
 FloppyDrive *dev = FLOPPY_DRIVE(qdev);
 FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
 FDrive *drive;
+int ret;
 
 if (dev->unit == -1) {
 for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
@@ -517,29 +525,57 @@ static int floppy_drive_init(DeviceState *qdev)
 return -1;
 }
 
-/* TODO Check whether unit is in use */
-
 drive = get_drv(bus->fdc, dev->unit);
-
 if (drive->blk) {
-if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
-error_report("fdc doesn't support drive option werror");
-return -1;
-}
-if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
-error_report("fdc doesn't support drive option rerror");
-return -1;
-}
-} else {
+error_report("Floppy unit %d is in use", dev->unit);
+return -1;
+}
+
+if (!dev->conf.blk) {
 /* Anonymous BlockBackend for an empty drive */
-drive->blk = blk_new();
+dev->conf.blk = blk_new();
+ret = blk_attach_dev(dev->conf.blk, qdev);
+assert(ret == 0);
 }
 
-fd_init(drive);
-if (drive->blk) {
-blk_set_dev_ops(drive->blk, _block_ops, drive);
-pick_drive_type(drive);
+blkconf_blocksizes(>conf);
+if (dev->conf.logical_block_size != 512 ||
+dev->conf.physical_block_size != 512)
+{
+error_report("Physical and logical block size must be 512 for floppy");
+return -1;
+}
+
+/* rerror/werror aren't supported by fdc and therefore not even registered
+ * with qdev. So set the defaults manually before they are used in
+ * blkconf_apply_backend_options(). */
+dev->conf.rerror = BLOCKDEV_ON_ERROR_AUTO;
+dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO;
+blkconf_apply_backend_options(>conf);
+
+/* 'enospc' is the default for -drive, 'report' is what blk_new() gives us
+ * for empty drives. */
+if (blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC &&
+blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option werror");
+return -1;
 }
+if (blk_get_on_error(dev->conf.blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option rerror");
+return -1;
+}
+
+drive->blk = dev->conf.blk;
+drive->fdctrl = bus->fdc;
+
+fd_init(drive);
+blk_set_dev_ops(drive->blk, _block_ops, drive);
+
+/* Keep 'type' qdev property and FDrive->drive in sync */
+drive->drive = dev->type;
+pick_drive_type(drive);
+dev->type = drive->drive;
+
 fd_revalidate(drive);
 
 return 0;
@@ -808,6 +844,10 @@ struct FDCtrl {
 FloppyBus bus;
 uint8_t num_floppies;
 FDrive drives[MAX_FD];
+struct {
+BlockBackend *blk;
+FloppyDriveType type;
+} qdev_for_drives[MAX_FD];
 int reset_sensei;
 uint32_t check_media_rate;
 FloppyDriveType fallback; /* 

[Qemu-block] [PATCH v3 0/4] fdc: Use separate qdev device for drives

[Kevin Wolf]

We have been complaining for a long time about how the floppy controller and
floppy drives are combined in a single qdev device and how this makes the
device awkward to work with because it behaves different from all other block
devices.

The latest reason to complain was when I noticed that using qdev device names
in QMP commands (e.g. for media change) doesn't really work when only the
controller is a qdev device, but the drives aren't.

So I decided to have a go at it, and this is the result.

It doesn't actually change any of the inner workings of the floppy controller,
but it wires things up differently on the qdev layer so that a floppy
controller now exposes a bus on which the floppy drives sit. This results in a
structure that is similar to IDE where the actual drive state is still in the
controller and the qdev device basically just contains the qdev properties -
not pretty, but quite workable.

The commit message of patch 3 explains how to use it. In short, there is a
'-device floppy' now and it does what you would expect if you ever used ide-cd.

The other problem is old command lines, especially those using things like
'-global isa-fdc,driveA=...'. In order to keep them working, we need to forward
the property to an internally created floppy drive device. This is a bit like
usb-storage, which we know is ugly, but works well enough in practice. The good
news here is that in contrast to usb-storage, the floppy controller only does
the forwarding for legacy configurations; as soon as you start using '-device
floppy', it doesn't happen any more.

So as you may have expected, this conversion doesn't result in a perfect
device, but I think it's definitely an improvement over the old state. I hope
you like it despite the warts. :-)

v3:
- Fixed omissons in the conversion sysbus-fdc and the Sun one. Nice, combining
  floppy controllers and weird platforms in a single series. [John]

v2:
- Added patch 4 (qemu-iotests case for floppy config on the command line)
- Patch 2: Create a floppy device only if a BlockBackend exists instead of
  always creating two of them
- Patch 2: Initialise drive->fdctrl even if no drive is attached, it is
  accessed anyway during migration
- Patch 3: Keep 'type' qdev property and FDrive->drive in sync
- Patch 3: Removed if with condition that is always true

Kevin Wolf (4):
  fdc: Add a floppy qbus
  fdc: Add a floppy drive qdev
  fdc: Move qdev properties to FloppyDrive
  qemu-iotests: Test creating floppy drives

 hw/block/fdc.c |  271 --
 tests/qemu-iotests/172 |  242 +
 tests/qemu-iotests/172.out | 1205 
 tests/qemu-iotests/group   |1 +
 vl.c   |1 +
 5 files changed, 1672 insertions(+), 48 deletions(-)
 create mode 100755 tests/qemu-iotests/172
 create mode 100644 tests/qemu-iotests/172.out

-- 
1.8.3.1

[Qemu-block] [PATCH v3 2/4] fdc: Add a floppy drive qdev

[Kevin Wolf]

Floppy controllers automatically create two floppy drive devices in qdev
now. (They always created two drives, but managed them only internally.)

Signed-off-by: Kevin Wolf 
Reviewed-by: John Snow 
---
 hw/block/fdc.c | 151 +
 1 file changed, 120 insertions(+), 31 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a3afb62..5aa8e52 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,6 +60,8 @@
 #define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
 
 typedef struct FDCtrl FDCtrl;
+typedef struct FDrive FDrive;
+static FDrive *get_drv(FDCtrl *fdctrl, int unit);
 
 typedef struct FloppyBus {
 BusState bus;
@@ -180,7 +182,7 @@ typedef enum FDiskFlags {
 FDISK_DBL_SIDES  = 0x01,
 } FDiskFlags;
 
-typedef struct FDrive {
+struct FDrive {
 FDCtrl *fdctrl;
 BlockBackend *blk;
 /* Drive status */
@@ -201,7 +203,7 @@ typedef struct FDrive {
 uint8_t media_rate;   /* Data rate of medium*/
 
 bool media_validated; /* Have we validated the media? */
-} FDrive;
+};
 
 
 static FloppyDriveType get_fallback_drive_type(FDrive *drv);
@@ -466,6 +468,100 @@ static void fd_revalidate(FDrive *drv)
 }
 }
 
+static void fd_change_cb(void *opaque, bool load)
+{
+FDrive *drive = opaque;
+
+drive->media_changed = 1;
+drive->media_validated = false;
+fd_revalidate(drive);
+}
+
+static const BlockDevOps fd_block_ops = {
+.change_media_cb = fd_change_cb,
+};
+
+
+#define TYPE_FLOPPY_DRIVE "floppy"
+#define FLOPPY_DRIVE(obj) \
+ OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
+
+typedef struct FloppyDrive {
+DeviceState qdev;
+uint32_tunit;
+} FloppyDrive;
+
+static Property floppy_drive_properties[] = {
+DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static int floppy_drive_init(DeviceState *qdev)
+{
+FloppyDrive *dev = FLOPPY_DRIVE(qdev);
+FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
+FDrive *drive;
+
+if (dev->unit == -1) {
+for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
+drive = get_drv(bus->fdc, dev->unit);
+if (!drive->blk) {
+break;
+}
+}
+}
+
+if (dev->unit >= MAX_FD) {
+error_report("Can't create floppy unit %d, bus supports only %d units",
+ dev->unit, MAX_FD);
+return -1;
+}
+
+/* TODO Check whether unit is in use */
+
+drive = get_drv(bus->fdc, dev->unit);
+
+if (drive->blk) {
+if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
+error_report("fdc doesn't support drive option werror");
+return -1;
+}
+if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
+error_report("fdc doesn't support drive option rerror");
+return -1;
+}
+} else {
+/* Anonymous BlockBackend for an empty drive */
+drive->blk = blk_new();
+}
+
+fd_init(drive);
+if (drive->blk) {
+blk_set_dev_ops(drive->blk, _block_ops, drive);
+pick_drive_type(drive);
+}
+fd_revalidate(drive);
+
+return 0;
+}
+
+static void floppy_drive_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *k = DEVICE_CLASS(klass);
+k->init = floppy_drive_init;
+set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
+k->bus_type = TYPE_FLOPPY_BUS;
+k->props = floppy_drive_properties;
+k->desc = "virtual floppy drive";
+}
+
+static const TypeInfo floppy_drive_info = {
+.name = TYPE_FLOPPY_DRIVE,
+.parent = TYPE_DEVICE,
+.instance_size = sizeof(FloppyDrive),
+.class_init = floppy_drive_class_init,
+};
+
 //
 /* Intel 82078 floppy disk controller emulation  */
 
@@ -1185,9 +1281,9 @@ static inline FDrive *drv3(FDCtrl *fdctrl)
 }
 #endif
 
-static FDrive *get_cur_drv(FDCtrl *fdctrl)
+static FDrive *get_drv(FDCtrl *fdctrl, int unit)
 {
-switch (fdctrl->cur_drv) {
+switch (unit) {
 case 0: return drv0(fdctrl);
 case 1: return drv1(fdctrl);
 #if MAX_FD == 4
@@ -1198,6 +1294,11 @@ static FDrive *get_cur_drv(FDCtrl *fdctrl)
 }
 }
 
+static FDrive *get_cur_drv(FDCtrl *fdctrl)
+{
+return get_drv(fdctrl, fdctrl->cur_drv);
+}
+
 /* Status A register : 0x00 (read-only) */
 static uint32_t fdctrl_read_statusA(FDCtrl *fdctrl)
 {
@@ -2357,46 +2458,33 @@ static void fdctrl_result_timer(void *opaque)
 }
 }
 
-static void fdctrl_change_cb(void *opaque, bool load)
-{
-FDrive *drive = opaque;
-
-drive->media_changed = 1;
-drive->media_validated = false;
-fd_revalidate(drive);
-}
-
-static const BlockDevOps fdctrl_block_ops = {
-.change_media_cb = fdctrl_change_cb,
-};
-
 /* Init functions */
 static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
 {
 

Re: [Qemu-block] [PATCH 15/18] block: only call aio_poll on the current thread's AioContext

[Stefan Hajnoczi]

On Mon, Oct 17, 2016 at 10:04:59AM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/10/2016 18:40, Stefan Hajnoczi wrote:
> > >  void bdrv_wakeup(BlockDriverState *bs)
> > >  {
> > > +if (bs->wakeup) {
> > > +aio_bh_schedule_oneshot(qemu_get_aio_context(), dummy_bh_cb, 
> > > NULL);
> > > +}
> > >  }
> > 
> > Why use a dummy BH instead of aio_notify()?
> 
> Originally I used aio_bh_schedule_oneshot() because aio_notify() is not
> enough for aio_poll() to return true.  It's also true that I am not
> using anymore the result of aio_poll, though.
> 
> Since this is not a fast path and it's not very much stressed by
> qemu-iotests, I think it's better if we can move towards making
> aio_notify() more or less an internal detail.  If you prefer
> aio_notify(), however, I can look into that as well.

I was just wondering if there is a reason that I missed.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [PATCH] qapi: fix memory leak in bdrv_image_info_specific_dump

[Kevin Wolf]

Am 18.10.2016 um 11:18 hat Pino Toscano geschrieben:
> The 'obj' result of the visitor was not properly freed, like done in
> other places doing a similar job.
> ---
>  block/qapi.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index 6f947e3..50d3090 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -698,6 +698,7 @@ void bdrv_image_info_specific_dump(fprintf_function 
> func_fprintf, void *f,
>  assert(qobject_type(obj) == QTYPE_QDICT);
>  data = qdict_get(qobject_to_qdict(obj), "data");
>  dump_qobject(func_fprintf, f, 1, data);
> +qobject_decref(obj);
>  visit_free(v);
>  }

The change looks good, but without a Signed-off-by line I can't accept
the patch: http://wiki.qemu.org/Contribute/SubmitAPatch

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v14 13/21] qdict: allow qdict_crumple to accept compound types as values

[Kevin Wolf]

Am 17.10.2016 um 16:50 hat Markus Armbruster geschrieben:
> Kevin Wolf  writes:
> 
> > Am 13.10.2016 um 14:35 hat Markus Armbruster geschrieben:
> >> Cc: Kevin for discussion of QemuOpts dotted key convention
> >> 
> >> "Daniel P. Berrange"  writes:
> >> 
> >> > Currently qdict_crumple requires a totally flat QDict as its
> >> > input. i.e. all values in the QDict must be scalar types.
> >> >
> >> > In order to have backwards compatibility with the OptsVisitor,
> >> > qemu_opt_to_qdict() has a new mode where it may return a QList
> >> > for values in the QDict, if there was a repeated key. We thus
> >> > need to allow compound types to appear as values in the input
> >> > dict given to qdict_crumple().
> >> >
> >> > To avoid confusion, we sanity check that the user has not mixed
> >> > the old and new syntax at the same time. e.g. these are allowed
> >> >
> >> >foo=hello,foo=world,foo=wibble
> >> >foo.0=hello,foo.1=world,foo.2=wibble
> >> >
> >> > but this is forbidden
> >> >
> >> >foo=hello,foo=world,foo.2=wibble
> >> 
> >> I understand the need for foo.bar=val.  It makes it possible to specify
> >> nested dictionaries with QemuOpts.
> >> 
> >> The case for foo.0=val is less clear.  QemuOpts already supports lists,
> >> by repeating keys.  Why do we need a second, wordier way to specify
> >> them?
> >
> > Probably primarily because someone didn't realise this when introducing
> > the dotted syntax.
> 
> Can't even blame "someone" for that; it's an obscure, underdocumented
> feature of an interface that's collapsing under its load of obscure,
> underdocumented features.
> 
> On the other hand, that's not exactly a state that allows for *more*
> obscure features.

I don't really think we're introducing more obscure features here, but
rather trying to implement a single, and rather straightforward, way as
the standard.

Dotted syntax for hierarchy has actually plenty of precedence in qemu if
you look a bit closer (the block layer, -global, -device foo,help, even
the bus names you mentioned below are really just flattened lists), so
we're only making things more consistent.

> >Also because flat QDicts can't represent this.
> 
> Explain?

Repeated options are parsed into QLists. If you keep it at that without
flattening you have at least a QDict that contains a QList that contains
simple types. This is not flat any more.

Of course, you could argue that flat QDicts are the wrong data structure
in the first place and instead of flatting everything we should have
done the equivalent of qdict_crumple from the beginning, but they were
the natural extension of what was already there and happened to work
good enough, so this is what we're currently using.

> > But even if I realised that QemuOpts support this syntax, I think we
> > would still have to use the dotted syntax because it's explicit about
> > the index and we need that because the list can contains dicts.
> >
> > Compare this:
> >
> > driver=quorum,
> > child.0.driver=file,child.0.filename=disk1.img,
> > child.1.driver=host_device,child.1.filename=/dev/sdb,
> > child.2.driver=nbd,child.2.host=localhost
> >
> > And this:
> >
> > driver=quorum,
> > child.driver=file,child.filename=disk1.img,
> > child.driver=host_device,child.filename=/dev/sdb,
> > child.driver=nbd,child.host=localhost
> 
> Aside: both are about equally illegible, to be honest.

Not sure about equally, but let's agree on "both are illegible".

> > For starters, can we really trust the order in QemuOpts so that the
> > right driver and filename are associated with each other?
> 
> The order is trustworthy, but...
> 
> > We would also have code to associate the third child.driver with the
> > first child.host (because file and host_device don't have a host
> > property). And this isn't even touching optional arguments yet. Would
> > you really want to implement or review this?
> 
> ... you're right, doing lists by repeating keys breaks down when
> combined with the dotted key convention's use of repetition to do
> structured values.
> 
> Permit me to digress.
> 
> QemuOpts wasn't designed for list-values keys.  Doing lists by
> repetition was clever.
> 
> QemuOpts wasn't designed for structured values.  Doing structured values
> by a dotted key convention plus repetition was clever.
> 
> And there's the problem: too much cleverness, not enough "this is being
> pushed way beyond its design limits, time to replace it".
> 
> For me, a replacement should do structured values by providing suitable
> *value* syntax instead of hacking it into the keys:
> 
> { "driver": "quorum",
>   "child": [ { "driver": "file", "filename": "disk1.img" },
>  { "driver": "host_device", "filename=/dev/sdb" },
>  { "driver": "nbd", "host": "localhost" } ] }
> 
> Note how the structure is obvious.  It isn't with dotted keys, not even
> if you order them sensibly (which users 

Re: [Qemu-block] [PATCH v4 0/3] iotests: Fix test 162

[Kevin Wolf]

Am 17.10.2016 um 19:07 hat Max Reitz geschrieben:
> On 28.09.2016 22:46, Max Reitz wrote:
> > 162 is potentially racy and makes some invalid assumptions about what
> > should happen when connecting to a non-existing domain name. This series
> > fixes both issues.
> > 
> > 
> > v4:
> > - Added documentation for the new --fork option [Kevin]
> > 
> > 
> > git-backport-diff against v3:
> > 
> > Key:
> > [] : patches are identical
> > [] : number of functional differences between upstream/downstream
> > patch
> > [down] : patch is downstream-only
> > The flags [FC] indicate (F)unctional and (C)ontextual differences,
> > respectively
> > 
> > 001/3:[0004] [FC] 'qemu-nbd: Add --fork option'
> > 002/3:[] [--] 'iotests: Remove raciness from 162'
> > 003/3:[] [--] 'iotests: Do not rely on unavailable domains in 162'
> > 
> > 
> > Max Reitz (3):
> >   qemu-nbd: Add --fork option
> >   iotests: Remove raciness from 162
> >   iotests: Do not rely on unavailable domains in 162
> > 
> >  qemu-nbd.c | 17 -
> >  qemu-nbd.texi  |  2 ++
> >  tests/qemu-iotests/162 | 22 --
> >  tests/qemu-iotests/162.out |  2 +-
> >  4 files changed, 35 insertions(+), 8 deletions(-)
> 
> Ping

I expected that Sascha would review this as he had comments on earlier
versions?

Kevin


pgp1d0jHZmnxr.pgp
Description: PGP signature

[Qemu-block] [PATCH] qapi: fix memory leak in bdrv_image_info_specific_dump

[Pino Toscano]

The 'obj' result of the visitor was not properly freed, like done in
other places doing a similar job.
---
 block/qapi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qapi.c b/block/qapi.c
index 6f947e3..50d3090 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -698,6 +698,7 @@ void bdrv_image_info_specific_dump(fprintf_function 
func_fprintf, void *f,
 assert(qobject_type(obj) == QTYPE_QDICT);
 data = qdict_get(qobject_to_qdict(obj), "data");
 dump_qobject(func_fprintf, f, 1, data);
+qobject_decref(obj);
 visit_free(v);
 }
 
-- 
2.7.4