[Qemu-devel] [PATCH v2 0/6] block: Move BB options from blockdev-add to qdev

[Qemu-devel] [PATCH v2 0/6] block: Move BB options from blockdev-add to qdev

[Kevin Wolf]

This series adds some device level options (write-cache, werror, rerror)
to qdev that used to be specified with -drive and made their way into
blockdev-add. They are at the same time removed from blockdev-add, which
is going to be changed in a later patch series so that it only creates
block nodes without a BlockBackend.

The -device drive=... option is also changed to allow specifying a
node-name rather than a BlockBackend name. In this case, an anonymous
BlockBackend is created internally.

Kevin Wolf (6):
  block/qdev: Allow node name for drive properties
  block/qdev: Allow configuring WCE with qdev properties
  commit: Fix use of error handling policy
  block/qdev: Allow configuring rerror/werror with qdev properties
  qemu-iotests: Test setting WCE with qdev
  block: Remove BB options from blockdev-add

 block/block-backend.c            |  1 +
 block/commit.c                   |  6 +--
 blockjob.c                       |  1 +
 hw/block/block.c                 | 28 ++++++++++++
 hw/block/nvme.c                  |  1 +
 hw/block/virtio-blk.c            |  2 +
 hw/core/qdev-properties-system.c | 22 ++++++++--
 hw/core/qdev-properties.c        | 13 ++++++
 hw/ide/qdev.c                    |  2 +
 hw/scsi/scsi-disk.c              |  2 +
 hw/usb/dev-storage.c             |  1 +
 include/hw/block/block.h         | 13 +++++-
 include/hw/qdev-properties.h     |  4 ++
 qapi/block-core.json             | 22 ++--------
 tests/qemu-iotests/157           | 92 ++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/157.out       | 22 ++++++++++
 tests/qemu-iotests/group         |  1 +
 17 files changed, 208 insertions(+), 25 deletions(-)
 create mode 100755 tests/qemu-iotests/157
 create mode 100644 tests/qemu-iotests/157.out

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/6] block/qdev: Allow node name for drive properties

[Kevin Wolf]

If a node name instead of a BlockBackend name is specified as the driver
for a guest device, an anonymous BlockBackend is created now.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/core/qdev-properties-system.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index df38b8a..69b451e 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -72,12 +72,21 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
                         const char *propname, Error **errp)
 {
     BlockBackend *blk;
+    bool blk_created = false;
 
     blk = blk_by_name(str);
     if (!blk) {
+        BlockDriverState *bs = bdrv_lookup_bs(NULL, str, NULL);
+        if (bs) {
+            blk = blk_new();
+            blk_insert_bs(blk, bs);
+            blk_created = true;
+        }
+    }
+    if (!blk) {
         error_setg(errp, "Property '%s.%s' can't find value '%s'",
                    object_get_typename(OBJECT(dev)), propname, str);
-        return;
+        goto fail;
     }
     if (blk_attach_dev(blk, dev) < 0) {
         DriveInfo *dinfo = blk_legacy_dinfo(blk);
@@ -91,9 +100,16 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr,
             error_setg(errp, "Drive '%s' is already in use by another device",
                        str);
         }
-        return;
+        goto fail;
     }
+
     *ptr = blk;
+
+fail:
+    if (blk_created) {
+        /* If we need to keep a reference, blk_attach_dev() took it */
+        blk_unref(blk);
+    }
 }
 
 static void release_drive(Object *obj, const char *name, void *opaque)
@@ -127,7 +143,7 @@ static void set_drive(Object *obj, Visitor *v, const char *name, void *opaque,
 
 PropertyInfo qdev_prop_drive = {
     .name  = "str",
-    .description = "ID of a drive to use as a backend",
+    .description = "Node name or ID of a block device to use as a backend",
     .get   = get_drive,
     .set   = set_drive,
     .release = release_drive,
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/6] block/qdev: Allow configuring WCE with qdev properties

[Kevin Wolf]

As cache.writeback is a BlockBackend property and as such more related
to the guest device than the BlockDriverState, we already removed it
from the blockdev-add interface. This patch adds the new way to set it,
as a qdev property of the corresponding guest device.

For example: -drive if=none,file=test.img,node-name=img
             -device ide-hd,drive=img,write-cache=off

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/block.c         | 16 ++++++++++++++++
 hw/block/nvme.c          |  1 +
 hw/block/virtio-blk.c    |  1 +
 hw/ide/qdev.c            |  1 +
 hw/scsi/scsi-disk.c      |  1 +
 hw/usb/dev-storage.c     |  1 +
 include/hw/block/block.h |  5 ++++-
 7 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/hw/block/block.c b/hw/block/block.c
index 97a59d4..396b0d5 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -51,6 +51,22 @@ void blkconf_blocksizes(BlockConf *conf)
     }
 }
 
+void blkconf_apply_backend_options(BlockConf *conf)
+{
+    BlockBackend *blk = conf->blk;
+    bool wce;
+
+    switch (conf->wce) {
+    case ON_OFF_AUTO_ON:    wce = true; break;
+    case ON_OFF_AUTO_OFF:   wce = false; break;
+    case ON_OFF_AUTO_AUTO:  wce = blk_enable_write_cache(blk); break;
+    default:
+        abort();
+    }
+
+    blk_set_enable_write_cache(blk, wce);
+}
+
 void blkconf_geometry(BlockConf *conf, int *ptrans,
                       unsigned cyls_max, unsigned heads_max, unsigned secs_max,
                       Error **errp)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 9faad29..8234669 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -803,6 +803,7 @@ static int nvme_init(PCIDevice *pci_dev)
         return -1;
     }
     blkconf_blocksizes(&n->conf);
+    blkconf_apply_backend_options(&n->conf);
 
     pci_conf = pci_dev->config;
     pci_conf[PCI_INTERRUPT_PIN] = 1;
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index ae86e94..5eb6ba1 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -904,6 +904,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
     blkconf_blocksizes(&conf->conf);
+    blkconf_apply_backend_options(&conf->conf);
 
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
                 sizeof(struct virtio_blk_config));
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 6842a55..e2badc3 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -180,6 +180,7 @@ static int ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind)
             return -1;
         }
     }
+    blkconf_apply_backend_options(&dev->conf);
 
     if (ide_init_drive(s, dev->conf.blk, kind,
                        dev->version, dev->serial, dev->model, dev->wwn,
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 36f8a85..8c26a4e 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2309,6 +2309,7 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
             return;
         }
     }
+    blkconf_apply_backend_options(&dev->conf);
 
     if (s->qdev.conf.discard_granularity == -1) {
         s->qdev.conf.discard_granularity =
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 4d605b8..5938c59 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -603,6 +603,7 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
 
     blkconf_serial(&s->conf, &dev->serial);
     blkconf_blocksizes(&s->conf);
+    blkconf_apply_backend_options(&s->conf);
 
     /*
      * Hack alert: this pretends to be a block device, but it's really
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 984660e..e511dc3 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -25,6 +25,7 @@ typedef struct BlockConf {
     uint32_t discard_granularity;
     /* geometry, not all devices use this */
     uint32_t cyls, heads, secs;
+    OnOffAuto wce;
 } BlockConf;
 
 static inline unsigned int get_physical_block_exp(BlockConf *conf)
@@ -49,7 +50,8 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
     DEFINE_PROP_UINT16("min_io_size", _state, _conf.min_io_size, 0),  \
     DEFINE_PROP_UINT32("opt_io_size", _state, _conf.opt_io_size, 0),    \
     DEFINE_PROP_UINT32("discard_granularity", _state, \
-                       _conf.discard_granularity, -1)
+                       _conf.discard_granularity, -1), \
+    DEFINE_PROP_ON_OFF_AUTO("write-cache", _state, _conf.wce, ON_OFF_AUTO_AUTO)
 
 #define DEFINE_BLOCK_CHS_PROPERTIES(_state, _conf)      \
     DEFINE_PROP_UINT32("cyls", _state, _conf.cyls, 0),  \
@@ -63,6 +65,7 @@ void blkconf_geometry(BlockConf *conf, int *trans,
                       unsigned cyls_max, unsigned heads_max, unsigned secs_max,
                       Error **errp);
 void blkconf_blocksizes(BlockConf *conf);
+void blkconf_apply_backend_options(BlockConf *conf);
 
 /* Hard disk geometry */
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 3/6] commit: Fix use of error handling policy

[Kevin Wolf]

Commit implemented the 'enospc' policy as 'ignore' if the error was not
ENOSPC. The QAPI documentation promises that it's treated as 'stop'.
Using the common block job error handling function fixes this.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 379efb7..cb6810d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -171,9 +171,9 @@ wait:
             bytes_written += n * BDRV_SECTOR_SIZE;
         }
         if (ret < 0) {
-            if (s->on_error == BLOCKDEV_ON_ERROR_STOP ||
-                s->on_error == BLOCKDEV_ON_ERROR_REPORT||
-                (s->on_error == BLOCKDEV_ON_ERROR_ENOSPC && ret == -ENOSPC)) {
+            BlockErrorAction action =
+                block_job_error_action(&s->common, false, s->on_error, -ret);
+            if (action == BLOCK_ERROR_ACTION_REPORT) {
                 goto out;
             } else {
                 n = 0;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 4/6] block/qdev: Allow configuring rerror/werror with qdev properties

[Kevin Wolf]

The rerror/werror policies are implemented in the devices, so that's
where they should be configured. In comparison to the old options in
-drive, the qdev properties are only added to those devices that
actually support them.

If the option isn't given (or "auto" is specified), the setting of the
BlockBackend is used for compatibility with the old options. For block
jobs, "auto" is the same as "enospc".

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c        |  1 +
 blockjob.c                   |  1 +
 hw/block/block.c             | 12 ++++++++++++
 hw/block/virtio-blk.c        |  1 +
 hw/core/qdev-properties.c    | 13 +++++++++++++
 hw/ide/qdev.c                |  1 +
 hw/scsi/scsi-disk.c          |  1 +
 include/hw/block/block.h     |  8 ++++++++
 include/hw/qdev-properties.h |  4 ++++
 qapi/block-core.json         |  4 +++-
 10 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index a862f65..4fedaf2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1173,6 +1173,7 @@ BlockErrorAction blk_get_error_action(BlockBackend *blk, bool is_read,
         return BLOCK_ERROR_ACTION_REPORT;
     case BLOCKDEV_ON_ERROR_IGNORE:
         return BLOCK_ERROR_ACTION_IGNORE;
+    case BLOCKDEV_ON_ERROR_AUTO:
     default:
         abort();
     }
diff --git a/blockjob.c b/blockjob.c
index 90c4e26..8c9ea35 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -523,6 +523,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
 
     switch (on_err) {
     case BLOCKDEV_ON_ERROR_ENOSPC:
+    case BLOCKDEV_ON_ERROR_AUTO:
         action = (error == ENOSPC) ?
                  BLOCK_ERROR_ACTION_STOP : BLOCK_ERROR_ACTION_REPORT;
         break;
diff --git a/hw/block/block.c b/hw/block/block.c
index 396b0d5..8dc9d84 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -54,6 +54,7 @@ void blkconf_blocksizes(BlockConf *conf)
 void blkconf_apply_backend_options(BlockConf *conf)
 {
     BlockBackend *blk = conf->blk;
+    BlockdevOnError rerror, werror;
     bool wce;
 
     switch (conf->wce) {
@@ -64,7 +65,18 @@ void blkconf_apply_backend_options(BlockConf *conf)
         abort();
     }
 
+    rerror = conf->rerror;
+    if (rerror == BLOCKDEV_ON_ERROR_AUTO) {
+        rerror = blk_get_on_error(blk, true);
+    }
+
+    werror = conf->werror;
+    if (werror == BLOCKDEV_ON_ERROR_AUTO) {
+        werror = blk_get_on_error(blk, false);
+    }
+
     blk_set_enable_write_cache(blk, wce);
+    blk_set_on_error(blk, rerror, werror);
 }
 
 void blkconf_geometry(BlockConf *conf, int *ptrans,
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 5eb6ba1..ef736fd 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -960,6 +960,7 @@ static void virtio_blk_instance_init(Object *obj)
 
 static Property virtio_blk_properties[] = {
     DEFINE_BLOCK_PROPERTIES(VirtIOBlock, conf.conf),
+    DEFINE_BLOCK_ERROR_PROPERTIES(VirtIOBlock, conf.conf),
     DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBlock, conf.conf),
     DEFINE_PROP_STRING("serial", VirtIOBlock, conf.serial),
     DEFINE_PROP_BIT("config-wce", VirtIOBlock, conf.config_wce, 0, true),
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index e3b2184..3deceee 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -539,6 +539,19 @@ PropertyInfo qdev_prop_losttickpolicy = {
     .set   = set_enum,
 };
 
+/* --- Block device error handling policy --- */
+
+QEMU_BUILD_BUG_ON(sizeof(BlockdevOnError) != sizeof(int));
+
+PropertyInfo qdev_prop_blockdev_on_error = {
+    .name = "BlockdevOnError",
+    .description = "Error handling policy, "
+                   "report/ignore/enospc/stop/auto",
+    .enum_table = BlockdevOnError_lookup,
+    .get = get_enum,
+    .set = set_enum,
+};
+
 /* --- BIOS CHS translation */
 
 QEMU_BUILD_BUG_ON(sizeof(BiosAtaTranslation) != sizeof(int));
diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index e2badc3..5b11f22 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -264,6 +264,7 @@ static int ide_drive_initfn(IDEDevice *dev)
 
 #define DEFINE_IDE_DEV_PROPERTIES()                     \
     DEFINE_BLOCK_PROPERTIES(IDEDrive, dev.conf),        \
+    DEFINE_BLOCK_ERROR_PROPERTIES(IDEDrive, dev.conf),  \
     DEFINE_PROP_STRING("ver",  IDEDrive, dev.version),  \
     DEFINE_PROP_UINT64("wwn",  IDEDrive, dev.wwn, 0),    \
     DEFINE_PROP_STRING("serial",  IDEDrive, dev.serial),\
diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 8c26a4e..8dbfc10 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2849,6 +2849,7 @@ static const TypeInfo scsi_disk_base_info = {
 
 #define DEFINE_SCSI_DISK_PROPERTIES()                                \
     DEFINE_BLOCK_PROPERTIES(SCSIDiskState, qdev.conf),               \
+    DEFINE_BLOCK_ERROR_PROPERTIES(SCSIDiskState, qdev.conf),         \
     DEFINE_PROP_STRING("ver", SCSIDiskState, version),               \
     DEFINE_PROP_STRING("serial", SCSIDiskState, serial),             \
     DEFINE_PROP_STRING("vendor", SCSIDiskState, vendor),             \
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index e511dc3..36dce84 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -26,6 +26,8 @@ typedef struct BlockConf {
     /* geometry, not all devices use this */
     uint32_t cyls, heads, secs;
     OnOffAuto wce;
+    BlockdevOnError rerror;
+    BlockdevOnError werror;
 } BlockConf;
 
 static inline unsigned int get_physical_block_exp(BlockConf *conf)
@@ -58,6 +60,12 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
     DEFINE_PROP_UINT32("heads", _state, _conf.heads, 0), \
     DEFINE_PROP_UINT32("secs", _state, _conf.secs, 0)
 
+#define DEFINE_BLOCK_ERROR_PROPERTIES(_state, _conf)                    \
+    DEFINE_PROP_BLOCKDEV_ON_ERROR("rerror", _state, _conf.rerror,       \
+                                  BLOCKDEV_ON_ERROR_AUTO),              \
+    DEFINE_PROP_BLOCKDEV_ON_ERROR("werror", _state, _conf.werror,       \
+                                  BLOCKDEV_ON_ERROR_AUTO)
+
 /* Configuration helpers */
 
 void blkconf_serial(BlockConf *conf, char **serial);
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 034b75a..2a9d2f9 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -20,6 +20,7 @@ extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_macaddr;
 extern PropertyInfo qdev_prop_on_off_auto;
 extern PropertyInfo qdev_prop_losttickpolicy;
+extern PropertyInfo qdev_prop_blockdev_on_error;
 extern PropertyInfo qdev_prop_bios_chs_trans;
 extern PropertyInfo qdev_prop_fdc_drive_type;
 extern PropertyInfo qdev_prop_drive;
@@ -161,6 +162,9 @@ extern PropertyInfo qdev_prop_arraylen;
 #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
                         LostTickPolicy)
+#define DEFINE_PROP_BLOCKDEV_ON_ERROR(_n, _s, _f, _d) \
+    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_blockdev_on_error, \
+                        BlockdevOnError)
 #define DEFINE_PROP_BIOS_CHS_TRANS(_n, _s, _f, _d) \
     DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_bios_chs_trans, int)
 #define DEFINE_PROP_BLOCKSIZE(_n, _s, _f) \
diff --git a/qapi/block-core.json b/qapi/block-core.json
index ac8f5f6..6008986 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -664,10 +664,12 @@
 # @stop: for guest operations, stop the virtual machine;
 #        for jobs, pause the job
 #
+# @auto: inherit the error handling policy of the backend (since: 2.7)
+#
 # Since: 1.3
 ##
 { 'enum': 'BlockdevOnError',
-  'data': ['report', 'ignore', 'enospc', 'stop'] }
+  'data': ['report', 'ignore', 'enospc', 'stop', 'auto'] }
 
 ##
 # @MirrorSyncMode:
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 5/6] qemu-iotests: Test setting WCE with qdev

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/157     | 92 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/157.out | 22 +++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 115 insertions(+)
 create mode 100755 tests/qemu-iotests/157
 create mode 100644 tests/qemu-iotests/157.out

diff --git a/tests/qemu-iotests/157 b/tests/qemu-iotests/157
new file mode 100755
index 0000000..956cbdb
--- /dev/null
+++ b/tests/qemu-iotests/157
@@ -0,0 +1,92 @@
+#!/bin/bash
+#
+# Test command line configuration of block devices with qdev
+#
+# 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 <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@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 generic
+_supported_proto file
+_supported_os Linux
+
+function do_run_qemu()
+{
+    echo Testing: "$@"
+    (
+        if ! test -t 0; then
+            while read cmd; do
+                echo $cmd
+            done
+        fi
+        echo quit
+    ) | $QEMU -nodefaults -nographic -monitor stdio -serial none "$@"
+    echo
+}
+
+function run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_generated_node_ids
+}
+
+
+if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
+    _notrun "Test uses IDE devices"
+fi
+
+size=128M
+drive="if=none,file="$TEST_IMG",driver=$IMGFMT"
+
+_make_test_img $size
+
+echo
+echo "=== Setting WCE with qdev and with manually created BB ==="
+echo
+
+# The qdev option takes precedence, but if it isn't given or 'auto', the BB
+# option is used instead.
+
+for cache in "writeback" "writethrough"; do
+    for wce in "" ",write-cache=auto", ",write-cache=on", ",write-cache=off"; do
+        echo "info block" \
+            | run_qemu -drive "$drive,cache=$cache" \
+                       -device "ide-hd,drive=none0$wce" \
+            | grep -e "Testing" -e "Cache mode"
+    done
+done
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/157.out b/tests/qemu-iotests/157.out
new file mode 100644
index 0000000..2e41e83
--- /dev/null
+++ b/tests/qemu-iotests/157.out
@@ -0,0 +1,22 @@
+QA output created by 157
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+
+=== Setting WCE with qdev and with manually created BB ===
+
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device ide-hd,drive=none0
+    Cache mode:       writeback
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device ide-hd,drive=none0,write-cache=auto,
+    Cache mode:       writeback
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device ide-hd,drive=none0,write-cache=on,
+    Cache mode:       writeback
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device ide-hd,drive=none0,write-cache=off
+    Cache mode:       writethrough
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device ide-hd,drive=none0
+    Cache mode:       writethrough
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device ide-hd,drive=none0,write-cache=auto,
+    Cache mode:       writethrough
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device ide-hd,drive=none0,write-cache=on,
+    Cache mode:       writeback
+Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device ide-hd,drive=none0,write-cache=off
+    Cache mode:       writethrough
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1c6fcb6..3a3973e 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -156,3 +156,4 @@
 154 rw auto backing quick
 155 rw auto
 156 rw auto quick
+157 auto
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 6/6] block: Remove BB options from blockdev-add

[Kevin Wolf]

werror/rerror are now available as qdev options. The stats-* options are
removed without an existing replacement; they should probably be
configurable with a separate QMP command like I/O throttling settings.

Removing id is left for another day because this involves updating
qemu-iotests cases to use node-name for everything. Before we can do
that, however, all QMP commands must support node-name.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6008986..88b78a7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2051,20 +2051,8 @@
 # @discard:       #optional discard-related options (default: ignore)
 # @cache:         #optional cache-related options
 # @aio:           #optional AIO backend (default: threads)
-# @rerror:        #optional how to handle read errors on the device
-#                 (default: report)
-# @werror:        #optional how to handle write errors on the device
-#                 (default: enospc)
 # @read-only:     #optional whether the block device should be read-only
 #                 (default: false)
-# @stats-account-invalid: #optional whether to include invalid
-#                         operations when computing last access statistics
-#                         (default: true) (Since 2.5)
-# @stats-account-failed: #optional whether to include failed
-#                         operations when computing latency and last
-#                         access statistics (default: true) (Since 2.5)
-# @stats-intervals: #optional list of intervals for collecting I/O
-#                   statistics, in seconds (default: none) (Since 2.5)
 # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
 #                 (default: off)
 #
@@ -2074,17 +2062,13 @@
 ##
 { 'union': 'BlockdevOptions',
   'base': { 'driver': 'BlockdevDriver',
+# TODO 'id' is a BB-level option, remove it
             '*id': 'str',
             '*node-name': 'str',
             '*discard': 'BlockdevDiscardOptions',
             '*cache': 'BlockdevCacheOptions',
             '*aio': 'BlockdevAioOptions',
-            '*rerror': 'BlockdevOnError',
-            '*werror': 'BlockdevOnError',
             '*read-only': 'bool',
-            '*stats-account-invalid': 'bool',
-            '*stats-account-failed': 'bool',
-            '*stats-intervals': ['int'],
             '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
   'discriminator': 'driver',
   'data': {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/6] block/qdev: Allow node name for drive properties

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 406 bytes --]

On 30.06.2016 16:13, Kevin Wolf wrote:
> If a node name instead of a BlockBackend name is specified as the driver
> for a guest device, an anonymous BlockBackend is created now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/core/qdev-properties-system.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v2 1/6] block/qdev: Allow node name for drive properties

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1003 bytes --]

On 30.06.2016 16:13, Kevin Wolf wrote:
> If a node name instead of a BlockBackend name is specified as the driver
> for a guest device, an anonymous BlockBackend is created now.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/core/qdev-properties-system.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)

I just noticed this won't work with (at least) USB mass storage devices.
scsi_bus_legacy_add_drive() creates a new device and uses
qdev_prop_set_drive() to set its drive, and that function uses
blk_name() to get the required value for .drive.

That can of course be fixed by using bdrv_get_node_name(blk_bs(value))
there if blk_name(value) is empty. But in addition we need to make sure
that the BB is not deleted in usb_msd_realize_storage() (because that
function has to detach the device from the BB before creating the SCSI
disk), so we need to wrap the explicitly hacky block there in
blk_ref(blk) and blk_unref(blk).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v2 2/6] block/qdev: Allow configuring WCE with qdev properties

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 869 bytes --]

On 30.06.2016 16:13, Kevin Wolf wrote:
> As cache.writeback is a BlockBackend property and as such more related
> to the guest device than the BlockDriverState, we already removed it
> from the blockdev-add interface. This patch adds the new way to set it,
> as a qdev property of the corresponding guest device.
> 
> For example: -drive if=none,file=test.img,node-name=img
>              -device ide-hd,drive=img,write-cache=off
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/block/block.c         | 16 ++++++++++++++++
>  hw/block/nvme.c          |  1 +
>  hw/block/virtio-blk.c    |  1 +
>  hw/ide/qdev.c            |  1 +
>  hw/scsi/scsi-disk.c      |  1 +
>  hw/usb/dev-storage.c     |  1 +
>  include/hw/block/block.h |  5 ++++-
>  7 files changed, 25 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v2 3/6] commit: Fix use of error handling policy

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

On 30.06.2016 16:13, Kevin Wolf wrote:
> Commit implemented the 'enospc' policy as 'ignore' if the error was not
> ENOSPC. The QAPI documentation promises that it's treated as 'stop'.
> Using the common block job error handling function fixes this.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/commit.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

...and it fixes that no event was generated.

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v2 4/6] block/qdev: Allow configuring rerror/werror with qdev properties

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

On 30.06.2016 16:13, Kevin Wolf wrote:
> The rerror/werror policies are implemented in the devices, so that's
> where they should be configured. In comparison to the old options in
> -drive, the qdev properties are only added to those devices that
> actually support them.
> 
> If the option isn't given (or "auto" is specified), the setting of the
> BlockBackend is used for compatibility with the old options. For block
> jobs, "auto" is the same as "enospc".
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c        |  1 +
>  blockjob.c                   |  1 +
>  hw/block/block.c             | 12 ++++++++++++
>  hw/block/virtio-blk.c        |  1 +
>  hw/core/qdev-properties.c    | 13 +++++++++++++
>  hw/ide/qdev.c                |  1 +
>  hw/scsi/scsi-disk.c          |  1 +
>  include/hw/block/block.h     |  8 ++++++++
>  include/hw/qdev-properties.h |  4 ++++
>  qapi/block-core.json         |  4 +++-
>  10 files changed, 45 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v2 5/6] qemu-iotests: Test setting WCE with qdev

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 6166 bytes --]

On 30.06.2016 16:13, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/157     | 92 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/157.out | 22 +++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 115 insertions(+)
>  create mode 100755 tests/qemu-iotests/157
>  create mode 100644 tests/qemu-iotests/157.out
> 
> diff --git a/tests/qemu-iotests/157 b/tests/qemu-iotests/157
> new file mode 100755
> index 0000000..956cbdb
> --- /dev/null
> +++ b/tests/qemu-iotests/157
> @@ -0,0 +1,92 @@
> +#!/bin/bash
> +#
> +# Test command line configuration of block devices with qdev
> +#
> +# 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 <http://www.gnu.org/licenses/>.
> +#
> +
> +# creator
> +owner=kwolf@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 generic
> +_supported_proto file
> +_supported_os Linux
> +
> +function do_run_qemu()
> +{
> +    echo Testing: "$@"
> +    (
> +        if ! test -t 0; then
> +            while read cmd; do
> +                echo $cmd
> +            done
> +        fi
> +        echo quit
> +    ) | $QEMU -nodefaults -nographic -monitor stdio -serial none "$@"
> +    echo
> +}
> +
> +function run_qemu()
> +{
> +    do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | _filter_generated_node_ids
> +}
> +
> +
> +if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
> +    _notrun "Test uses IDE devices"

Why not just use virtio? Which brings me to my second question: Why does
this test fail with virtio (the BB always takes precedence then)? :-)

> +fi
> +
> +size=128M
> +drive="if=none,file="$TEST_IMG",driver=$IMGFMT"

I don't think the quotation marks around $TEST_IMG are right.


> +
> +_make_test_img $size
> +
> +echo
> +echo "=== Setting WCE with qdev and with manually created BB ==="
> +echo
> +
> +# The qdev option takes precedence, but if it isn't given or 'auto', the BB
> +# option is used instead.
> +
> +for cache in "writeback" "writethrough"; do
> +    for wce in "" ",write-cache=auto", ",write-cache=on", ",write-cache=off"; do

Commas between the values are wrong. They don't hurt, but they're part
of each value then.

> +        echo "info block" \
> +            | run_qemu -drive "$drive,cache=$cache" \
> +                       -device "ide-hd,drive=none0$wce" \
> +            | grep -e "Testing" -e "Cache mode"

Something interesting: If you'd specify the drive through a node name,
then the BDS tree has two BBs, one implicitly created with -drive (this
one is named (automatically) and owned by the monitor) and an anonymous
one for the device. If the device then overrides the cache mode, this
will not be reflected in the monitor-owned BB. "info block" (and
query-block) use the monitor BBs, however, so they'll report the BB on
top of the BDS tree in question to be in whatever mode has been
specified in -drive, whereas the mode the device uses for accessing that
BDS tree actually has nothing to do with that.

So the user has no way of inquiring the cache mode used by the device,
and they actually get presented misleading information.

Max

> +    done
> +done
> +
> +# success, all done
> +echo "*** done"
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/157.out b/tests/qemu-iotests/157.out
> new file mode 100644
> index 0000000..2e41e83
> --- /dev/null
> +++ b/tests/qemu-iotests/157.out
> @@ -0,0 +1,22 @@
> +QA output created by 157
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> +
> +=== Setting WCE with qdev and with manually created BB ===
> +
> +Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device ide-hd,drive=none0
> +    Cache mode:       writeback
> +Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device ide-hd,drive=none0,write-cache=auto,
> +    Cache mode:       writeback
> +Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device ide-hd,drive=none0,write-cache=on,
> +    Cache mode:       writeback
> +Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writeback -device ide-hd,drive=none0,write-cache=off
> +    Cache mode:       writethrough
> +Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device ide-hd,drive=none0
> +    Cache mode:       writethrough
> +Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device ide-hd,drive=none0,write-cache=auto,
> +    Cache mode:       writethrough
> +Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device ide-hd,drive=none0,write-cache=on,
> +    Cache mode:       writeback
> +Testing: -drive if=none,file=TEST_DIR/t.qcow2,driver=qcow2,cache=writethrough -device ide-hd,drive=none0,write-cache=off
> +    Cache mode:       writethrough
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 1c6fcb6..3a3973e 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -156,3 +156,4 @@
>  154 rw auto backing quick
>  155 rw auto
>  156 rw auto quick
> +157 auto
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v2 6/6] block: Remove BB options from blockdev-add

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3041 bytes --]

On 30.06.2016 16:13, Kevin Wolf wrote:
> werror/rerror are now available as qdev options. The stats-* options are
> removed without an existing replacement; they should probably be
> configurable with a separate QMP command like I/O throttling settings.

I'm not sure I agree with removing the stats-* options without a
replacement. If we'd get rid of @id in the process, fine. But we won't
get a pure blockdev-add before 2.7 anyway, so I'm not sure removing
stats-* now is necessary.

Max

> 
> Removing id is left for another day because this involves updating
> qemu-iotests cases to use node-name for everything. Before we can do
> that, however, all QMP commands must support node-name.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/block-core.json | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6008986..88b78a7 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2051,20 +2051,8 @@
>  # @discard:       #optional discard-related options (default: ignore)
>  # @cache:         #optional cache-related options
>  # @aio:           #optional AIO backend (default: threads)
> -# @rerror:        #optional how to handle read errors on the device
> -#                 (default: report)
> -# @werror:        #optional how to handle write errors on the device
> -#                 (default: enospc)
>  # @read-only:     #optional whether the block device should be read-only
>  #                 (default: false)
> -# @stats-account-invalid: #optional whether to include invalid
> -#                         operations when computing last access statistics
> -#                         (default: true) (Since 2.5)
> -# @stats-account-failed: #optional whether to include failed
> -#                         operations when computing latency and last
> -#                         access statistics (default: true) (Since 2.5)
> -# @stats-intervals: #optional list of intervals for collecting I/O
> -#                   statistics, in seconds (default: none) (Since 2.5)
>  # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
>  #                 (default: off)
>  #
> @@ -2074,17 +2062,13 @@
>  ##
>  { 'union': 'BlockdevOptions',
>    'base': { 'driver': 'BlockdevDriver',
> +# TODO 'id' is a BB-level option, remove it
>              '*id': 'str',
>              '*node-name': 'str',
>              '*discard': 'BlockdevDiscardOptions',
>              '*cache': 'BlockdevCacheOptions',
>              '*aio': 'BlockdevAioOptions',
> -            '*rerror': 'BlockdevOnError',
> -            '*werror': 'BlockdevOnError',
>              '*read-only': 'bool',
> -            '*stats-account-invalid': 'bool',
> -            '*stats-account-failed': 'bool',
> -            '*stats-intervals': ['int'],
>              '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
>    'discriminator': 'driver',
>    'data': {
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v2 1/6] block/qdev: Allow node name for drive properties

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]

Am 02.07.2016 um 17:33 hat Max Reitz geschrieben:
> On 30.06.2016 16:13, Kevin Wolf wrote:
> > If a node name instead of a BlockBackend name is specified as the driver
> > for a guest device, an anonymous BlockBackend is created now.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  hw/core/qdev-properties-system.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> I just noticed this won't work with (at least) USB mass storage devices.
> scsi_bus_legacy_add_drive() creates a new device and uses
> qdev_prop_set_drive() to set its drive, and that function uses
> blk_name() to get the required value for .drive.
> 
> That can of course be fixed by using bdrv_get_node_name(blk_bs(value))
> there if blk_name(value) is empty. But in addition we need to make sure
> that the BB is not deleted in usb_msd_realize_storage() (because that
> function has to detach the device from the BB before creating the SCSI
> disk), so we need to wrap the explicitly hacky block there in
> blk_ref(blk) and blk_unref(blk).

I'm not sure if I even want to fix this here... Maybe it's better to
leave node names broken for usb-storage, as a motivation for someone to
try and clean up this mess for good. I mean, it's not the first time
that this hack breaks.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v2 5/6] qemu-iotests: Test setting WCE with qdev

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

Am 02.07.2016 um 18:15 hat Max Reitz geschrieben:
> On 30.06.2016 16:13, Kevin Wolf wrote:
> > +        echo "info block" \
> > +            | run_qemu -drive "$drive,cache=$cache" \
> > +                       -device "ide-hd,drive=none0$wce" \
> > +            | grep -e "Testing" -e "Cache mode"
> 
> Something interesting: If you'd specify the drive through a node name,
> then the BDS tree has two BBs, one implicitly created with -drive (this
> one is named (automatically) and owned by the monitor) and an anonymous
> one for the device. If the device then overrides the cache mode, this
> will not be reflected in the monitor-owned BB. "info block" (and
> query-block) use the monitor BBs, however, so they'll report the BB on
> top of the BDS tree in question to be in whatever mode has been
> specified in -drive, whereas the mode the device uses for accessing that
> BDS tree actually has nothing to do with that.
> 
> So the user has no way of inquiring the cache mode used by the device,
> and they actually get presented misleading information.

Yes, I know. Originally I wanted to add test cases that use node-name,
but then it occurred to me that this would be pretty pointless.

I'm not sure yet what the conclusion is. Change query-block to include
anonymous BBs that are owned by devices? A new query command? Add the
information to info qtree and whatever the QMP version of it is (if it
even exists)?

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v2 6/6] block: Remove BB options from blockdev-add

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1049 bytes --]

Am 02.07.2016 um 18:28 hat Max Reitz geschrieben:
> On 30.06.2016 16:13, Kevin Wolf wrote:
> > werror/rerror are now available as qdev options. The stats-* options are
> > removed without an existing replacement; they should probably be
> > configurable with a separate QMP command like I/O throttling settings.
> 
> I'm not sure I agree with removing the stats-* options without a
> replacement. If we'd get rid of @id in the process, fine. But we won't
> get a pure blockdev-add before 2.7 anyway, so I'm not sure removing
> stats-* now is necessary.

Actually, I'm not so sure about removing id only after 2.7. Basically
all that's missing is a conversion of block job commands to accept
node-name so that qemu-iotests cases can be converted. This is a mostly
mechanical conversion and I have part of it ready.

About keeping stats-*, what good is it to keep an option that we know
will go away sooner or later? For id there is a good reason, removing it
now would break test cases. But stats-*? I don't see a reason.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v2 1/6] block/qdev: Allow node name for drive properties

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]

On 04.07.2016 12:43, Kevin Wolf wrote:
> Am 02.07.2016 um 17:33 hat Max Reitz geschrieben:
>> On 30.06.2016 16:13, Kevin Wolf wrote:
>>> If a node name instead of a BlockBackend name is specified as the driver
>>> for a guest device, an anonymous BlockBackend is created now.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  hw/core/qdev-properties-system.c | 22 +++++++++++++++++++---
>>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> I just noticed this won't work with (at least) USB mass storage devices.
>> scsi_bus_legacy_add_drive() creates a new device and uses
>> qdev_prop_set_drive() to set its drive, and that function uses
>> blk_name() to get the required value for .drive.
>>
>> That can of course be fixed by using bdrv_get_node_name(blk_bs(value))
>> there if blk_name(value) is empty. But in addition we need to make sure
>> that the BB is not deleted in usb_msd_realize_storage() (because that
>> function has to detach the device from the BB before creating the SCSI
>> disk), so we need to wrap the explicitly hacky block there in
>> blk_ref(blk) and blk_unref(blk).
> 
> I'm not sure if I even want to fix this here... Maybe it's better to
> leave node names broken for usb-storage, as a motivation for someone to
> try and clean up this mess for good. I mean, it's not the first time
> that this hack breaks.

Well, you'll at least have to add the blk_ref()+blk_unref() wrapping, or
we'll get accesses to freed memory.

Realistically speaking, I doubt deliberately making USB mass storage
devices not support the new drive syntax would actually motivate someone
to clean the whole thing up.

In my opinion, just making qdev_prop_set_drive() support anonymous BBs
is simple enough, so it'd be worth it. I don't think we gain anything by
not doing it.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v2 5/6] qemu-iotests: Test setting WCE with qdev

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]

On 04.07.2016 12:50, Kevin Wolf wrote:
> Am 02.07.2016 um 18:15 hat Max Reitz geschrieben:
>> On 30.06.2016 16:13, Kevin Wolf wrote:
>>> +        echo "info block" \
>>> +            | run_qemu -drive "$drive,cache=$cache" \
>>> +                       -device "ide-hd,drive=none0$wce" \
>>> +            | grep -e "Testing" -e "Cache mode"
>>
>> Something interesting: If you'd specify the drive through a node name,
>> then the BDS tree has two BBs, one implicitly created with -drive (this
>> one is named (automatically) and owned by the monitor) and an anonymous
>> one for the device. If the device then overrides the cache mode, this
>> will not be reflected in the monitor-owned BB. "info block" (and
>> query-block) use the monitor BBs, however, so they'll report the BB on
>> top of the BDS tree in question to be in whatever mode has been
>> specified in -drive, whereas the mode the device uses for accessing that
>> BDS tree actually has nothing to do with that.
>>
>> So the user has no way of inquiring the cache mode used by the device,
>> and they actually get presented misleading information.
> 
> Yes, I know. Originally I wanted to add test cases that use node-name,
> but then it occurred to me that this would be pretty pointless.
> 
> I'm not sure yet what the conclusion is. Change query-block to include
> anonymous BBs that are owned by devices? A new query command? Add the
> information to info qtree and whatever the QMP version of it is (if it
> even exists)?

Well, since you are basically trying to purge the BB from the user's
field of view, it would probably make sense to display all the BB-level
information (like the writethrough mode) as part of the device
information; that is, probably in info qtree. That information should
then probably also include the node name of the root BDS, and the
user/management application can find out all about that with
query-named-block-nodes (although it'd probably makes sense to add a
query-block-node command for a single node).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v2 6/6] block: Remove BB options from blockdev-add

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1341 bytes --]

On 04.07.2016 12:58, Kevin Wolf wrote:
> Am 02.07.2016 um 18:28 hat Max Reitz geschrieben:
>> On 30.06.2016 16:13, Kevin Wolf wrote:
>>> werror/rerror are now available as qdev options. The stats-* options are
>>> removed without an existing replacement; they should probably be
>>> configurable with a separate QMP command like I/O throttling settings.
>>
>> I'm not sure I agree with removing the stats-* options without a
>> replacement. If we'd get rid of @id in the process, fine. But we won't
>> get a pure blockdev-add before 2.7 anyway, so I'm not sure removing
>> stats-* now is necessary.
> 
> Actually, I'm not so sure about removing id only after 2.7. Basically
> all that's missing is a conversion of block job commands to accept
> node-name so that qemu-iotests cases can be converted. This is a mostly
> mechanical conversion and I have part of it ready.
> 
> About keeping stats-*, what good is it to keep an option that we know
> will go away sooner or later? For id there is a good reason, removing it
> now would break test cases. But stats-*? I don't see a reason.

The reason would be "If someone is insane enough to use blockdev-add and
then wants to make use of those options".

I guess your reason for removing it would be "Don't encourage such
people further". Fine with me, then.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 498 bytes --]

Re: [Qemu-devel] [PATCH v2 5/6] qemu-iotests: Test setting WCE with qdev

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

On 07/05/2016 08:57 AM, Max Reitz wrote:

>> I'm not sure yet what the conclusion is. Change query-block to include
>> anonymous BBs that are owned by devices? A new query command? Add the
>> information to info qtree and whatever the QMP version of it is (if it
>> even exists)?
> 
> Well, since you are basically trying to purge the BB from the user's
> field of view, it would probably make sense to display all the BB-level
> information (like the writethrough mode) as part of the device
> information; that is, probably in info qtree. That information should
> then probably also include the node name of the root BDS, and the
> user/management application can find out all about that with
> query-named-block-nodes (although it'd probably makes sense to add a
> query-block-node command for a single node).

Or teach query-named-block-nodes to take an optional parameter for
filtering the results to a single node (either way is introspectible, so
I don't have a strong opinion which is nicer)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2 5/6] qemu-iotests: Test setting WCE with qdev

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

Am 02.07.2016 um 18:15 hat Max Reitz geschrieben:
> On 30.06.2016 16:13, Kevin Wolf wrote:
> > +if [ "$QEMU_DEFAULT_MACHINE" != "pc" ]; then
> > +    _notrun "Test uses IDE devices"
> 
> Why not just use virtio?

Hm, and I guess use virtio-blk rather than virtio-blk-pci so that it
works on all platforms?

> Which brings me to my second question: Why does this test fail with
> virtio (the BB always takes precedence then)? :-)

Because you didn't fix or even mention the bug in patch 2. :-)

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]