All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work
@ 2016-03-16 18:54 Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 01/17] monitor: Use BB list for BB name completion Max Reitz
                   ` (17 more replies)
  0 siblings, 18 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

This series appears to reinvent itself with every revision. This time,
its main implication is that BBs are no longer automatically treated as
monitor-owned, and that a BB's name is tightly tied to the monitor
reference (it is considered equivalent to that reference).


v4:
- Tightly tied a BB's name to the monitor reference [Kevin]
  - Added two patches from my "blockdev: (Nearly) free clean-up work"
    series which are required so a BB does not need to have a name when
    bdrv_open() is invoked for its BDS tree
- Added a missing monitor_remove_blk() to the error path of
  hmp_drive_add() [Kevin]
- v3 just made blk_backends contain all BBs and then added another
  list called monitor_block_backends. v4 instead renames blk_backends to
  monitor_block_backends (because both are supposed to be the same,
  basically) and then adds a new list containing all BBs.
  [Markus/Kevin]
- blk_hide_on_behalf_of_hmp_drive_del() does a bit more than what
  monitor_remove_blk() does; namely, it invokes bdrv_make_anon() on the
  BDS. We should probably continue to do so.
- Use blk_is_inserted() instead of blk_is_available() for
  blk_commit_all() [Kevin]


Patch justifications:
- 1: General clean-up.
- 2: Clean-up which is nice to have for patch 7 which renames
     blk_backends to monitor_block_backends.
- 3: In order to move bdrv_commit_all() to the BB level (patch 12),
     first we need to introduce a BB-level function...
- 4: ...and then we have to use it.
- 5: After this series, BBs will no longer have a name before
     monitor_add_blk() is called; therefore, we should not try to print
     the device name when an image is opened (the filename is more
     interesting anyway)
- 6: Similarily to patch 5, this removes the device name from an error
     message emitted during image opening.
- 7: blk_backends to me personally implies this was a list of all
     BlockBackends. It actually is not. Therefore, this patch renames it
     to a more precise monitor_block_backends.
- 8: As this series makes it more clear that not all BlockBackends need
     to be referenced by the monitor, this patch consequently adds a
     list of truly and unconditionally all BlockBackends for whenever we
     need to iterate through really all of them.
- 9: This patch moves the name management for BlockBackends from
     blk_new(), blk_delete() and blk_hide_on_behalf_of_hmp_drive_del()
     into new functions monitor_add_blk() and monitor_remove_blk().
     These functions are still invoked in blk_new() etc., so nothing
     changes to the outside yet.
- 10: This patch drops the implicit calls of monitor_add_blk() and
      monitor_remove_blk() and thus makes the distinction between a BB's
      existence and whether it is owned by the monitor externally
      visible.
- 11: Clean-up patch.
- 12: More or less a clean-up patch.
- 13: Required so we can drop bdrv_states.
- 14: Required so we can drop bdrv_states.
- 15: Required so we can drop bdrv_states.
- 16: Clean-up and required so we can drop bdrv_states.
- 17: Clean-up and preparation for the follow-up series.


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/17:[----] [--] 'monitor: Use BB list for BB name completion'
002/17:[down] 'block: Use blk_next() in block-backend.c'
003/17:[----] [--] 'block: Add blk_commit_all()'
004/17:[----] [--] 'block: Use blk_{commit,flush}_all() consistently'
005/17:[down] 'qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATURE'
006/17:[down] 'block: Drop BB name from bad option error'
007/17:[down] 'blockdev: Rename blk_backends'
008/17:[down] 'blockdev: Add list of all BlockBackends'
009/17:[down] 'blockdev: Separate BB name management'
010/17:[down] 'blockdev: Split monitor reference from BB creation'
011/17:[0014] [FC] 'blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()'
012/17:[0002] [FC] 'block: Move some bdrv_*_all() functions to BB'
013/17:[0002] [FC] 'block: Add bdrv_next_monitor_owned()'
014/17:[----] [-C] 'block: Add blk_next_root_bs()'
015/17:[----] [--] 'block: Rewrite bdrv_next()'
016/17:[----] [--] 'block: Use bdrv_next() instead of bdrv_states'
017/17:[----] [-C] 'block: Remove bdrv_states list'


Max Reitz (17):
  monitor: Use BB list for BB name completion
  block: Use blk_next() in block-backend.c
  block: Add blk_commit_all()
  block: Use blk_{commit,flush}_all() consistently
  qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATURE
  block: Drop BB name from bad option error
  blockdev: Rename blk_backends
  blockdev: Add list of all BlockBackends
  blockdev: Separate BB name management
  blockdev: Split monitor reference from BB creation
  blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()
  block: Move some bdrv_*_all() functions to BB
  block: Add bdrv_next_monitor_owned()
  block: Add blk_next_root_bs()
  block: Rewrite bdrv_next()
  block: Use bdrv_next() instead of bdrv_states
  block: Remove bdrv_states list

 block.c                                       |  86 +++-------
 block/block-backend.c                         | 228 ++++++++++++++++++--------
 block/io.c                                    |  20 ---
 block/parallels.c                             |   2 +-
 block/qcow.c                                  |   8 +-
 block/qcow2.c                                 |  30 +---
 block/qed.c                                   |   9 +-
 block/sheepdog.c                              |   4 +-
 block/vdi.c                                   |   2 +-
 block/vhdx.c                                  |   2 +-
 block/vmdk.c                                  |  13 +-
 block/vpc.c                                   |   2 +-
 blockdev.c                                    |  44 +++--
 cpus.c                                        |   5 +-
 device-hotplug.c                              |   4 +-
 hw/block/xen_disk.c                           |   2 +-
 include/block/block.h                         |   4 +-
 include/block/block_int.h                     |   4 -
 include/qapi/qmp/qerror.h                     |   3 -
 include/sysemu/block-backend.h                |  15 +-
 monitor.c                                     |   7 +-
 qemu-char.c                                   |   3 +-
 qemu-img.c                                    |  50 +++---
 qemu-io.c                                     |   2 +-
 qemu-nbd.c                                    |   4 +-
 stubs/Makefile.objs                           |   3 +-
 stubs/bdrv-next-monitor-owned.c               |   8 +
 stubs/{bdrv-commit-all.c => blk-commit-all.c} |   4 +-
 tests/qemu-iotests/036.out                    |  16 +-
 tests/qemu-iotests/051.out                    |   8 +-
 tests/qemu-iotests/051.pc.out                 |   8 +-
 tests/qemu-iotests/087.out                    |   2 +-
 32 files changed, 310 insertions(+), 292 deletions(-)
 create mode 100644 stubs/bdrv-next-monitor-owned.c
 rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%)

-- 
2.7.3

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 01/17] monitor: Use BB list for BB name completion
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 02/17] block: Use blk_next() in block-backend.c Max Reitz
                   ` (16 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 monitor.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/monitor.c b/monitor.c
index 894f862..4c02f0f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -42,6 +42,7 @@
 #include "ui/console.h"
 #include "ui/input.h"
 #include "sysemu/blockdev.h"
+#include "sysemu/block-backend.h"
 #include "audio/audio.h"
 #include "disas/disas.h"
 #include "sysemu/balloon.h"
@@ -3483,7 +3484,7 @@ static void monitor_find_completion_by_table(Monitor *mon,
     int i;
     const char *ptype, *str, *name;
     const mon_cmd_t *cmd;
-    BlockDriverState *bs;
+    BlockBackend *blk = NULL;
 
     if (nb_args <= 1) {
         /* command completion */
@@ -3538,8 +3539,8 @@ static void monitor_find_completion_by_table(Monitor *mon,
         case 'B':
             /* block device name completion */
             readline_set_completion_index(mon->rs, strlen(str));
-            for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
-                name = bdrv_get_device_name(bs);
+            while ((blk = blk_next(blk)) != NULL) {
+                name = blk_name(blk);
                 if (str[0] == '\0' ||
                     !strncmp(name, str, strlen(str))) {
                     readline_add_completion(mon->rs, name);
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 02/17] block: Use blk_next() in block-backend.c
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 01/17] monitor: Use BB list for BB name completion Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 03/17] block: Add blk_commit_all() Max Reitz
                   ` (15 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Instead of iterating directly through blk_backends, we can use
blk_next() instead. This gives us some abstraction from the list itself
which we can use to rename it, for example.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 03e71b4..7a04e10 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -227,9 +227,9 @@ void blk_unref(BlockBackend *blk)
 
 void blk_remove_all_bs(void)
 {
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
 
-    QTAILQ_FOREACH(blk, &blk_backends, link) {
+    while ((blk = blk_next(blk)) != NULL) {
         AioContext *ctx = blk_get_aio_context(blk);
 
         aio_context_acquire(ctx);
@@ -271,10 +271,10 @@ const char *blk_name(BlockBackend *blk)
  */
 BlockBackend *blk_by_name(const char *name)
 {
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
 
     assert(name);
-    QTAILQ_FOREACH(blk, &blk_backends, link) {
+    while ((blk = blk_next(blk)) != NULL) {
         if (!strcmp(name, blk->name)) {
             return blk;
         }
@@ -332,9 +332,9 @@ DriveInfo *blk_set_legacy_dinfo(BlockBackend *blk, DriveInfo *dinfo)
  */
 BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
 {
-    BlockBackend *blk;
+    BlockBackend *blk = NULL;
 
-    QTAILQ_FOREACH(blk, &blk_backends, link) {
+    while ((blk = blk_next(blk)) != NULL) {
         if (blk->legacy_dinfo == dinfo) {
             return blk;
         }
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 03/17] block: Add blk_commit_all()
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 01/17] monitor: Use BB list for BB name completion Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 02/17] block: Use blk_next() in block-backend.c Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 04/17] block: Use blk_{commit, flush}_all() consistently Max Reitz
                   ` (14 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Later, we will remove bdrv_commit_all() and move its contents here, and
in order to replace bdrv_commit_all() calls by calls to blk_commit_all()
before doing so, we need to add it as an alias now.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 5 +++++
 include/sysemu/block-backend.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 7a04e10..e75b8fe 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1329,3 +1329,8 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
 {
     return &blk->root_state;
 }
+
+int blk_commit_all(void)
+{
+    return bdrv_commit_all();
+}
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 00d69ba..84612ce 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -127,6 +127,7 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
 int blk_co_flush(BlockBackend *blk);
 int blk_flush(BlockBackend *blk);
 int blk_flush_all(void);
+int blk_commit_all(void);
 void blk_drain(BlockBackend *blk);
 void blk_drain_all(void);
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 04/17] block: Use blk_{commit, flush}_all() consistently
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (2 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 03/17] block: Add blk_commit_all() Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 05/17] qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATURE Max Reitz
                   ` (13 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Replace bdrv_commmit_all() and bdrv_flush_all() by their BlockBackend
equivalents.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c  | 2 +-
 cpus.c      | 5 +++--
 qemu-char.c | 3 ++-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 50410bf..5be7d4b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1173,7 +1173,7 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
     int ret;
 
     if (!strcmp(device, "all")) {
-        ret = bdrv_commit_all();
+        ret = blk_commit_all();
     } else {
         BlockDriverState *bs;
         AioContext *aio_context;
diff --git a/cpus.c b/cpus.c
index 4052be5..23cf7aa 100644
--- a/cpus.c
+++ b/cpus.c
@@ -29,6 +29,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
 #include "exec/gdbstub.h"
 #include "sysemu/dma.h"
 #include "sysemu/kvm.h"
@@ -734,7 +735,7 @@ static int do_vm_stop(RunState state)
     }
 
     bdrv_drain_all();
-    ret = bdrv_flush_all();
+    ret = blk_flush_all();
 
     return ret;
 }
@@ -1433,7 +1434,7 @@ int vm_stop_force_state(RunState state)
         bdrv_drain_all();
         /* Make sure to return an error if the flush in a previous vm_stop()
          * failed. */
-        return bdrv_flush_all();
+        return blk_flush_all();
     }
 }
 
diff --git a/qemu-char.c b/qemu-char.c
index 0a14e57..bfcf80d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -25,6 +25,7 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 #include "sysemu/sysemu.h"
+#include "sysemu/block-backend.h"
 #include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "sysemu/char.h"
@@ -628,7 +629,7 @@ static int mux_proc_byte(CharDriverState *chr, MuxDriver *d, int ch)
                  break;
             }
         case 's':
-            bdrv_commit_all();
+            blk_commit_all();
             break;
         case 'b':
             qemu_chr_be_event(chr, CHR_EVENT_BREAK);
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 05/17] qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATURE
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (3 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 04/17] block: Use blk_{commit, flush}_all() consistently Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 06/17] block: Drop BB name from bad option error Max Reitz
                   ` (12 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Just specifying a custom string is simpler in basically all places that
used it, and in addition, specifying the BB or node name is something we
generally do not do in other error messages when opening a BDS, so we
should not do it here.

This changes the output for iotest 036 (to the better, in my opinion),
so the reference output needs to be changed accordingly.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow.c               |  6 +-----
 block/qcow2.c              | 24 +++++-------------------
 block/qed.c                |  7 ++-----
 block/vmdk.c               |  7 ++-----
 include/qapi/qmp/qerror.h  |  3 ---
 tests/qemu-iotests/036.out | 16 ++++++++--------
 6 files changed, 18 insertions(+), 45 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 2fd5ee6..c09cb79 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -121,11 +121,7 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
     if (header.version != QCOW_VERSION) {
-        char version[64];
-        snprintf(version, sizeof(version), "QCOW version %" PRIu32,
-                 header.version);
-        error_setg(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-                   bdrv_get_device_or_node_name(bs), "qcow", version);
+        error_setg(errp, "Unsupported qcow version %" PRIu32, header.version);
         ret = -ENOTSUP;
         goto fail;
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 1ce6264..3fd664b 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -198,22 +198,8 @@ static void cleanup_unknown_header_ext(BlockDriverState *bs)
     }
 }
 
-static void GCC_FMT_ATTR(3, 4) report_unsupported(BlockDriverState *bs,
-    Error **errp, const char *fmt, ...)
-{
-    char msg[64];
-    va_list ap;
-
-    va_start(ap, fmt);
-    vsnprintf(msg, sizeof(msg), fmt, ap);
-    va_end(ap);
-
-    error_setg(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-               bdrv_get_device_or_node_name(bs), "qcow2", msg);
-}
-
-static void report_unsupported_feature(BlockDriverState *bs,
-    Error **errp, Qcow2Feature *table, uint64_t mask)
+static void report_unsupported_feature(Error **errp, Qcow2Feature *table,
+                                       uint64_t mask)
 {
     char *features = g_strdup("");
     char *old;
@@ -238,7 +224,7 @@ static void report_unsupported_feature(BlockDriverState *bs,
         g_free(old);
     }
 
-    report_unsupported(bs, errp, "%s", features);
+    error_setg(errp, "Unsupported qcow2 feature(s): %s", features);
     g_free(features);
 }
 
@@ -855,7 +841,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
     if (header.version < 2 || header.version > 3) {
-        report_unsupported(bs, errp, "QCOW version %" PRIu32, header.version);
+        error_setg(errp, "Unsupported qcow2 version %" PRIu32, header.version);
         ret = -ENOTSUP;
         goto fail;
     }
@@ -935,7 +921,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         void *feature_table = NULL;
         qcow2_read_extensions(bs, header.header_length, ext_end,
                               &feature_table, NULL);
-        report_unsupported_feature(bs, errp, feature_table,
+        report_unsupported_feature(errp, feature_table,
                                    s->incompatible_features &
                                    ~QCOW2_INCOMPAT_MASK);
         ret = -ENOTSUP;
diff --git a/block/qed.c b/block/qed.c
index 8de7dd0..76563ee 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -412,11 +412,8 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
     }
     if (s->header.features & ~QED_FEATURE_MASK) {
         /* image uses unsupported feature bits */
-        char buf[64];
-        snprintf(buf, sizeof(buf), "%" PRIx64,
-            s->header.features & ~QED_FEATURE_MASK);
-        error_setg(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-                   bdrv_get_device_or_node_name(bs), "QED", buf);
+        error_setg(errp, "Unsupported QED features: %" PRIx64,
+                   s->header.features & ~QED_FEATURE_MASK);
         return -ENOTSUP;
     }
     if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
diff --git a/block/vmdk.c b/block/vmdk.c
index 23bd57e..2f8e6cf 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -661,11 +661,8 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     compressed =
         le16_to_cpu(header.compressAlgorithm) == VMDK4_COMPRESSION_DEFLATE;
     if (le32_to_cpu(header.version) > 3) {
-        char buf[64];
-        snprintf(buf, sizeof(buf), "VMDK version %" PRId32,
-                 le32_to_cpu(header.version));
-        error_setg(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-                   bdrv_get_device_or_node_name(bs), "vmdk", buf);
+        error_setg(errp, "Unsupported VMDK version %" PRIu32,
+                   le32_to_cpu(header.version));
         return -ENOTSUP;
     } else if (le32_to_cpu(header.version) == 3 && (flags & BDRV_O_RDWR) &&
                !compressed) {
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index f601499..d08652a 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -100,9 +100,6 @@
 #define QERR_UNDEFINED_ERROR \
     "An undefined error has occurred"
 
-#define QERR_UNKNOWN_BLOCK_FORMAT_FEATURE \
-    "'%s' uses a %s feature which is not supported by this qemu version: %s"
-
 #define QERR_UNSUPPORTED \
     "this feature or command is not currently supported"
 
diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
index f443635..9b009b8 100644
--- a/tests/qemu-iotests/036.out
+++ b/tests/qemu-iotests/036.out
@@ -22,18 +22,18 @@ autoclear_features        0x0
 refcount_order            4
 header_length             104
 
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: Unknown incompatible feature: 8000000000000000
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: Test feature
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: 8000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Test feature
 
 === Image with multiple incompatible feature bits ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: Unknown incompatible feature: e000000000000000
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: Test feature, Unknown incompatible feature: 6000000000000000
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: Test feature, Unknown incompatible feature: c000000000000000
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: test1, test2, Unknown incompatible feature: 8000000000000000
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: test1, test2, test3
-qemu-img: Could not open 'TEST_DIR/t.IMGFMT': 'image' uses a IMGFMT feature which is not supported by this qemu version: test2, Unknown incompatible feature: a000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Unknown incompatible feature: e000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Test feature, Unknown incompatible feature: 6000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): Test feature, Unknown incompatible feature: c000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): test1, test2, Unknown incompatible feature: 8000000000000000
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): test1, test2, test3
+qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT feature(s): test2, Unknown incompatible feature: a000000000000000
 === Create image with unknown autoclear feature bit ===
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 06/17] block: Drop BB name from bad option error
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (4 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 05/17] qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATURE Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 07/17] blockdev: Rename blk_backends Max Reitz
                   ` (11 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

The information which BB is concerned does not seem useful enough to
justify its existence in most other place (which may be related to qemu
printing the -drive parameter in question anyway, and for blockdev-add
the attribution is naturally unambiguous). Furthermore, as of a future
patch, bdrv_get_device_name(bs) will always return the empty string
before bdrv_open_inherit() returns.

Therefore, just dropping that information seems to be the best course of
action.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                       | 6 +++---
 tests/qemu-iotests/051.out    | 8 ++++----
 tests/qemu-iotests/051.pc.out | 8 ++++----
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/block.c b/block.c
index 59a18a3..582a9e8 100644
--- a/block.c
+++ b/block.c
@@ -1671,9 +1671,9 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
             error_setg(errp, "Block protocol '%s' doesn't support the option "
                        "'%s'", drv->format_name, entry->key);
         } else {
-            error_setg(errp, "Block format '%s' used by device '%s' doesn't "
-                       "support the option '%s'", drv->format_name,
-                       bdrv_get_device_name(bs), entry->key);
+            error_setg(errp,
+                       "Block format '%s' does not support the option '%s'",
+                       drv->format_name, entry->key);
         }
 
         ret = -EINVAL;
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 0f8a8d3..c1291ff 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -5,16 +5,16 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
 === Unknown option ===
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=,if=none,id=drive0
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=,if=none,id=drive0: Block format 'qcow2' used by device 'drive0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=,if=none,id=drive0: Block format 'qcow2' does not support the option 'unknown_opt'
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on,if=none,id=drive0
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on,if=none,id=drive0: Block format 'qcow2' used by device 'drive0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on,if=none,id=drive0: Block format 'qcow2' does not support the option 'unknown_opt'
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234,if=none,id=drive0
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234,if=none,id=drive0: Block format 'qcow2' used by device 'drive0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234,if=none,id=drive0: Block format 'qcow2' does not support the option 'unknown_opt'
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo,if=none,id=drive0
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo,if=none,id=drive0: Block format 'qcow2' used by device 'drive0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo,if=none,id=drive0: Block format 'qcow2' does not support the option 'unknown_opt'
 
 
 === Unknown protocol option ===
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index 85fc05d..73cc15a 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -5,16 +5,16 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
 === Unknown option ===
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=,if=none,id=drive0
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=,if=none,id=drive0: Block format 'qcow2' used by device 'drive0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=,if=none,id=drive0: Block format 'qcow2' does not support the option 'unknown_opt'
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on,if=none,id=drive0
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on,if=none,id=drive0: Block format 'qcow2' used by device 'drive0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on,if=none,id=drive0: Block format 'qcow2' does not support the option 'unknown_opt'
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234,if=none,id=drive0
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234,if=none,id=drive0: Block format 'qcow2' used by device 'drive0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234,if=none,id=drive0: Block format 'qcow2' does not support the option 'unknown_opt'
 
 Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo,if=none,id=drive0
-QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo,if=none,id=drive0: Block format 'qcow2' used by device 'drive0' doesn't support the option 'unknown_opt'
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo,if=none,id=drive0: Block format 'qcow2' does not support the option 'unknown_opt'
 
 
 === Unknown protocol option ===
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 07/17] blockdev: Rename blk_backends
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (5 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 06/17] block: Drop BB name from bad option error Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 08/17] blockdev: Add list of all BlockBackends Max Reitz
                   ` (10 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

The blk_backends list does not contain all BlockBackends but only the
ones which are referenced by the monitor, and that is not necessarily
true for every BlockBackend. Rename the list to monitor_block_backends
to make that fact clear.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index e75b8fe..3bb2a6a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -29,7 +29,7 @@ struct BlockBackend {
     int refcnt;
     BlockDriverState *bs;
     DriveInfo *legacy_dinfo;    /* null unless created by drive_new() */
-    QTAILQ_ENTRY(BlockBackend) link; /* for blk_backends */
+    QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */
 
     void *dev;                  /* attached device model, if any */
     /* TODO change to DeviceState when all users are qdevified */
@@ -69,9 +69,10 @@ static const AIOCBInfo block_backend_aiocb_info = {
 
 static void drive_info_del(DriveInfo *dinfo);
 
-/* All the BlockBackends (except for hidden ones) */
-static QTAILQ_HEAD(, BlockBackend) blk_backends =
-    QTAILQ_HEAD_INITIALIZER(blk_backends);
+/* All BlockBackends referenced by the monitor and which are iterated through by
+ * blk_next() */
+static QTAILQ_HEAD(, BlockBackend) monitor_block_backends =
+    QTAILQ_HEAD_INITIALIZER(monitor_block_backends);
 
 /*
  * Create a new BlockBackend with @name, with a reference count of one.
@@ -105,7 +106,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
     blk->refcnt = 1;
     notifier_list_init(&blk->remove_bs_notifiers);
     notifier_list_init(&blk->insert_bs_notifiers);
-    QTAILQ_INSERT_TAIL(&blk_backends, blk, link);
+    QTAILQ_INSERT_TAIL(&monitor_block_backends, blk, monitor_link);
     return blk;
 }
 
@@ -178,7 +179,7 @@ static void blk_delete(BlockBackend *blk)
     }
     /* Avoid double-remove after blk_hide_on_behalf_of_hmp_drive_del() */
     if (blk->name[0]) {
-        QTAILQ_REMOVE(&blk_backends, blk, link);
+        QTAILQ_REMOVE(&monitor_block_backends, blk, monitor_link);
     }
     g_free(blk->name);
     drive_info_del(blk->legacy_dinfo);
@@ -241,7 +242,7 @@ void blk_remove_all_bs(void)
 }
 
 /*
- * Return the BlockBackend after @blk.
+ * Return the monitor-owned BlockBackend after @blk.
  * If @blk is null, return the first one.
  * Else, return @blk's next sibling, which may be null.
  *
@@ -252,7 +253,8 @@ void blk_remove_all_bs(void)
  */
 BlockBackend *blk_next(BlockBackend *blk)
 {
-    return blk ? QTAILQ_NEXT(blk, link) : QTAILQ_FIRST(&blk_backends);
+    return blk ? QTAILQ_NEXT(blk, monitor_link)
+               : QTAILQ_FIRST(&monitor_block_backends);
 }
 
 /*
@@ -353,7 +355,7 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
  */
 void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
 {
-    QTAILQ_REMOVE(&blk_backends, blk, link);
+    QTAILQ_REMOVE(&monitor_block_backends, blk, monitor_link);
     blk->name[0] = 0;
     if (blk->bs) {
         bdrv_make_anon(blk->bs);
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 08/17] blockdev: Add list of all BlockBackends
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (6 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 07/17] blockdev: Rename blk_backends Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 09/17] blockdev: Separate BB name management Max Reitz
                   ` (9 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

While monitor_block_backends contains nearly all BBs, we sometimes
really need all BBs. To this end, this patch adds the block_backend
list.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 3bb2a6a..35206be 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -29,6 +29,7 @@ struct BlockBackend {
     int refcnt;
     BlockDriverState *bs;
     DriveInfo *legacy_dinfo;    /* null unless created by drive_new() */
+    QTAILQ_ENTRY(BlockBackend) link;         /* for block_backends */
     QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */
 
     void *dev;                  /* attached device model, if any */
@@ -69,6 +70,10 @@ static const AIOCBInfo block_backend_aiocb_info = {
 
 static void drive_info_del(DriveInfo *dinfo);
 
+/* All BlockBackends */
+static QTAILQ_HEAD(, BlockBackend) block_backends =
+    QTAILQ_HEAD_INITIALIZER(block_backends);
+
 /* All BlockBackends referenced by the monitor and which are iterated through by
  * blk_next() */
 static QTAILQ_HEAD(, BlockBackend) monitor_block_backends =
@@ -106,7 +111,10 @@ BlockBackend *blk_new(const char *name, Error **errp)
     blk->refcnt = 1;
     notifier_list_init(&blk->remove_bs_notifiers);
     notifier_list_init(&blk->insert_bs_notifiers);
+
+    QTAILQ_INSERT_TAIL(&block_backends, blk, link);
     QTAILQ_INSERT_TAIL(&monitor_block_backends, blk, monitor_link);
+
     return blk;
 }
 
@@ -177,11 +185,15 @@ static void blk_delete(BlockBackend *blk)
         g_free(blk->root_state.throttle_group);
         throttle_group_unref(blk->root_state.throttle_state);
     }
+
     /* Avoid double-remove after blk_hide_on_behalf_of_hmp_drive_del() */
     if (blk->name[0]) {
         QTAILQ_REMOVE(&monitor_block_backends, blk, monitor_link);
     }
     g_free(blk->name);
+
+    QTAILQ_REMOVE(&block_backends, blk, link);
+
     drive_info_del(blk->legacy_dinfo);
     block_acct_cleanup(&blk->stats);
     g_free(blk);
@@ -226,11 +238,21 @@ void blk_unref(BlockBackend *blk)
     }
 }
 
+/*
+ * Behaves similarly to blk_next() but iterates over all BlockBackends, even the
+ * ones which are hidden (i.e. are not referenced by the monitor).
+ */
+static BlockBackend *blk_all_next(BlockBackend *blk)
+{
+    return blk ? QTAILQ_NEXT(blk, link)
+               : QTAILQ_FIRST(&block_backends);
+}
+
 void blk_remove_all_bs(void)
 {
     BlockBackend *blk = NULL;
 
-    while ((blk = blk_next(blk)) != NULL) {
+    while ((blk = blk_all_next(blk)) != NULL) {
         AioContext *ctx = blk_get_aio_context(blk);
 
         aio_context_acquire(ctx);
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 09/17] blockdev: Separate BB name management
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (7 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 08/17] blockdev: Add list of all BlockBackends Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 10/17] blockdev: Split monitor reference from BB creation Max Reitz
                   ` (8 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Introduce separate functions (monitor_add_blk() and
monitor_remove_blk()) which set or unset a BB name. Since the name is
equivalent to the monitor's reference to a BB, adding a name the same as
declaring the BB to be monitor-owned and removing it revokes this
status, hence the function names.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 92 ++++++++++++++++++++++++++++--------------
 include/sysemu/block-backend.h |  2 +
 2 files changed, 63 insertions(+), 31 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 35206be..a5b950c 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -90,30 +90,17 @@ BlockBackend *blk_new(const char *name, Error **errp)
 {
     BlockBackend *blk;
 
-    assert(name && name[0]);
-    if (!id_wellformed(name)) {
-        error_setg(errp, "Invalid device name");
-        return NULL;
-    }
-    if (blk_by_name(name)) {
-        error_setg(errp, "Device with id '%s' already exists", name);
-        return NULL;
-    }
-    if (bdrv_find_node(name)) {
-        error_setg(errp,
-                   "Device name '%s' conflicts with an existing node name",
-                   name);
-        return NULL;
-    }
-
     blk = g_new0(BlockBackend, 1);
-    blk->name = g_strdup(name);
     blk->refcnt = 1;
     notifier_list_init(&blk->remove_bs_notifiers);
     notifier_list_init(&blk->insert_bs_notifiers);
 
     QTAILQ_INSERT_TAIL(&block_backends, blk, link);
-    QTAILQ_INSERT_TAIL(&monitor_block_backends, blk, monitor_link);
+
+    if (!monitor_add_blk(blk, name, errp)) {
+        blk_unref(blk);
+        return NULL;
+    }
 
     return blk;
 }
@@ -174,7 +161,10 @@ BlockBackend *blk_new_open(const char *name, const char *filename,
 
 static void blk_delete(BlockBackend *blk)
 {
+    monitor_remove_blk(blk);
+
     assert(!blk->refcnt);
+    assert(!blk->name);
     assert(!blk->dev);
     if (blk->bs) {
         blk_remove_bs(blk);
@@ -185,15 +175,7 @@ static void blk_delete(BlockBackend *blk)
         g_free(blk->root_state.throttle_group);
         throttle_group_unref(blk->root_state.throttle_state);
     }
-
-    /* Avoid double-remove after blk_hide_on_behalf_of_hmp_drive_del() */
-    if (blk->name[0]) {
-        QTAILQ_REMOVE(&monitor_block_backends, blk, monitor_link);
-    }
-    g_free(blk->name);
-
     QTAILQ_REMOVE(&block_backends, blk, link);
-
     drive_info_del(blk->legacy_dinfo);
     block_acct_cleanup(&blk->stats);
     g_free(blk);
@@ -280,13 +262,62 @@ BlockBackend *blk_next(BlockBackend *blk)
 }
 
 /*
+ * Add a BlockBackend into the list of backends referenced by the monitor, with
+ * the given @name acting as the handle for the monitor.
+ * Strictly for use by blockdev.c.
+ *
+ * @name must not be null or empty.
+ *
+ * Returns true on success and false on failure. In the latter case, an Error
+ * object is returned through @errp.
+ */
+bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp)
+{
+    assert(!blk->name);
+    assert(name && name[0]);
+
+    if (!id_wellformed(name)) {
+        error_setg(errp, "Invalid device name");
+        return false;
+    }
+    if (blk_by_name(name)) {
+        error_setg(errp, "Device with id '%s' already exists", name);
+        return false;
+    }
+    if (bdrv_find_node(name)) {
+        error_setg(errp,
+                   "Device name '%s' conflicts with an existing node name",
+                   name);
+        return false;
+    }
+
+    blk->name = g_strdup(name);
+    QTAILQ_INSERT_TAIL(&monitor_block_backends, blk, monitor_link);
+    return true;
+}
+
+/*
+ * Remove a BlockBackend from the list of backends referenced by the monitor.
+ * Strictly for use by blockdev.c.
+ */
+void monitor_remove_blk(BlockBackend *blk)
+{
+    if (!blk->name) {
+        return;
+    }
+
+    QTAILQ_REMOVE(&monitor_block_backends, blk, monitor_link);
+    g_free(blk->name);
+    blk->name = NULL;
+}
+
+/*
  * Return @blk's name, a non-null string.
- * Wart: the name is empty iff @blk has been hidden with
- * blk_hide_on_behalf_of_hmp_drive_del().
+ * Returns an empty string iff @blk is not referenced by the monitor.
  */
 const char *blk_name(BlockBackend *blk)
 {
-    return blk->name;
+    return blk->name ?: "";
 }
 
 /*
@@ -377,8 +408,7 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
  */
 void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
 {
-    QTAILQ_REMOVE(&monitor_block_backends, blk, monitor_link);
-    blk->name[0] = 0;
+    monitor_remove_blk(blk);
     if (blk->bs) {
         bdrv_make_anon(blk->bs);
     }
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 84612ce..c906c20 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -71,6 +71,8 @@ void blk_remove_all_bs(void);
 const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
+bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp);
+void monitor_remove_blk(BlockBackend *blk);
 
 BlockDriverState *blk_bs(BlockBackend *blk);
 void blk_remove_bs(BlockBackend *blk);
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 10/17] blockdev: Split monitor reference from BB creation
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (8 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 09/17] blockdev: Separate BB name management Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 11/17] blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del() Max Reitz
                   ` (7 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Before this patch, blk_new() automatically assigned a name to the new
BlockBackend and considered it referenced by the monitor. This patch
removes the implicit monitor_add_blk() call from blk_new() (and
consequently the monitor_remove_blk() call from blk_delete(), too) and
thus blk_new() (and related functions) no longer take a BB name
argument.

In fact, there is only a single point where blk_new()/blk_new_open() is
called and the new BB is monitor-owned, and that is in blockdev_init().
Besides thus relieving us from having to invent names for all of the BBs
we use in qemu-img, this fixes a bug where qemu cannot create a new
image if there already is a monitor-owned BB named "image".

If a BB and its BDS tree are created in a single operation, as of this
patch the BDS tree will be created before the BB is given a name
(whereas it was the other way around before). This results in minor
change to the output of iotest 087, whose reference output is amended
accordingly.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 26 ++++++----------------
 block/parallels.c              |  2 +-
 block/qcow.c                   |  2 +-
 block/qcow2.c                  |  6 ++---
 block/qed.c                    |  2 +-
 block/sheepdog.c               |  4 ++--
 block/vdi.c                    |  2 +-
 block/vhdx.c                   |  2 +-
 block/vmdk.c                   |  6 ++---
 block/vpc.c                    |  2 +-
 blockdev.c                     | 17 +++++++++++---
 device-hotplug.c               |  4 +++-
 hw/block/xen_disk.c            |  2 +-
 include/sysemu/block-backend.h |  9 ++++----
 qemu-img.c                     | 50 ++++++++++++++++++------------------------
 qemu-io.c                      |  2 +-
 qemu-nbd.c                     |  4 ++--
 tests/qemu-iotests/087.out     |  2 +-
 18 files changed, 68 insertions(+), 76 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index a5b950c..9ed3912 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -80,13 +80,11 @@ static QTAILQ_HEAD(, BlockBackend) monitor_block_backends =
     QTAILQ_HEAD_INITIALIZER(monitor_block_backends);
 
 /*
- * Create a new BlockBackend with @name, with a reference count of one.
- * @name must not be null or empty.
- * Fail if a BlockBackend with this name already exists.
+ * Create a new BlockBackend with a reference count of one.
  * Store an error through @errp on failure, unless it's null.
  * Return the new BlockBackend on success, null on failure.
  */
-BlockBackend *blk_new(const char *name, Error **errp)
+BlockBackend *blk_new(Error **errp)
 {
     BlockBackend *blk;
 
@@ -94,14 +92,7 @@ BlockBackend *blk_new(const char *name, Error **errp)
     blk->refcnt = 1;
     notifier_list_init(&blk->remove_bs_notifiers);
     notifier_list_init(&blk->insert_bs_notifiers);
-
     QTAILQ_INSERT_TAIL(&block_backends, blk, link);
-
-    if (!monitor_add_blk(blk, name, errp)) {
-        blk_unref(blk);
-        return NULL;
-    }
-
     return blk;
 }
 
@@ -109,12 +100,12 @@ BlockBackend *blk_new(const char *name, Error **errp)
  * Create a new BlockBackend with a new BlockDriverState attached.
  * Otherwise just like blk_new(), which see.
  */
-BlockBackend *blk_new_with_bs(const char *name, Error **errp)
+BlockBackend *blk_new_with_bs(Error **errp)
 {
     BlockBackend *blk;
     BlockDriverState *bs;
 
-    blk = blk_new(name, errp);
+    blk = blk_new(errp);
     if (!blk) {
         return NULL;
     }
@@ -137,14 +128,13 @@ BlockBackend *blk_new_with_bs(const char *name, Error **errp)
  * though, so callers of this function have to be able to specify @filename and
  * @flags.
  */
-BlockBackend *blk_new_open(const char *name, const char *filename,
-                           const char *reference, QDict *options, int flags,
-                           Error **errp)
+BlockBackend *blk_new_open(const char *filename, const char *reference,
+                           QDict *options, int flags, Error **errp)
 {
     BlockBackend *blk;
     int ret;
 
-    blk = blk_new_with_bs(name, errp);
+    blk = blk_new_with_bs(errp);
     if (!blk) {
         QDECREF(options);
         return NULL;
@@ -161,8 +151,6 @@ BlockBackend *blk_new_open(const char *name, const char *filename,
 
 static void blk_delete(BlockBackend *blk)
 {
-    monitor_remove_blk(blk);
-
     assert(!blk->refcnt);
     assert(!blk->name);
     assert(!blk->dev);
diff --git a/block/parallels.c b/block/parallels.c
index 0d1a60c..b322d05 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -478,7 +478,7 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
         return ret;
     }
 
-    file = blk_new_open("image", filename, NULL, NULL,
+    file = blk_new_open(filename, NULL, NULL,
                         BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
                         &local_err);
     if (file == NULL) {
diff --git a/block/qcow.c b/block/qcow.c
index c09cb79..73cf8a7 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -793,7 +793,7 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
         goto cleanup;
     }
 
-    qcow_blk = blk_new_open("image", filename, NULL, NULL,
+    qcow_blk = blk_new_open(filename, NULL, NULL,
                             BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
                             &local_err);
     if (qcow_blk == NULL) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 3fd664b..5f4fea6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2159,7 +2159,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         return ret;
     }
 
-    blk = blk_new_open("image", filename, NULL, NULL,
+    blk = blk_new_open(filename, NULL, NULL,
                        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
                        &local_err);
     if (blk == NULL) {
@@ -2224,7 +2224,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
      */
     options = qdict_new();
     qdict_put(options, "driver", qstring_from_str("qcow2"));
-    blk = blk_new_open("image-qcow2", filename, NULL, options,
+    blk = blk_new_open(filename, NULL, options,
                        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH,
                        &local_err);
     if (blk == NULL) {
@@ -2286,7 +2286,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     /* Reopen the image without BDRV_O_NO_FLUSH to flush it before returning */
     options = qdict_new();
     qdict_put(options, "driver", qstring_from_str("qcow2"));
-    blk = blk_new_open("image-flush", filename, NULL, options,
+    blk = blk_new_open(filename, NULL, options,
                        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_BACKING,
                        &local_err);
     if (blk == NULL) {
diff --git a/block/qed.c b/block/qed.c
index 76563ee..9f3dd2a 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -586,7 +586,7 @@ static int qed_create(const char *filename, uint32_t cluster_size,
         return ret;
     }
 
-    blk = blk_new_open("image", filename, NULL, NULL,
+    blk = blk_new_open(filename, NULL, NULL,
                        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
                        &local_err);
     if (blk == NULL) {
diff --git a/block/sheepdog.c b/block/sheepdog.c
index a6e98a5..a24732d 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1646,7 +1646,7 @@ static int sd_prealloc(const char *filename, Error **errp)
     void *buf = NULL;
     int ret;
 
-    blk = blk_new_open("image-prealloc", filename, NULL, NULL,
+    blk = blk_new_open(filename, NULL, NULL,
                        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
                        errp);
     if (blk == NULL) {
@@ -1843,7 +1843,7 @@ static int sd_create(const char *filename, QemuOpts *opts,
             goto out;
         }
 
-        blk = blk_new_open("backing", backing_file, NULL, NULL,
+        blk = blk_new_open(backing_file, NULL, NULL,
                            BDRV_O_PROTOCOL | BDRV_O_CACHE_WB, errp);
         if (blk == NULL) {
             ret = -EIO;
diff --git a/block/vdi.c b/block/vdi.c
index 662d14b..df9fa47 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -768,7 +768,7 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
         goto exit;
     }
 
-    blk = blk_new_open("image", filename, NULL, NULL,
+    blk = blk_new_open(filename, NULL, NULL,
                        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
                        &local_err);
     if (blk == NULL) {
diff --git a/block/vhdx.c b/block/vhdx.c
index e15020c..78fe56c 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1838,7 +1838,7 @@ static int vhdx_create(const char *filename, QemuOpts *opts, Error **errp)
         goto exit;
     }
 
-    blk = blk_new_open("image", filename, NULL, NULL,
+    blk = blk_new_open(filename, NULL, NULL,
                        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
                        &local_err);
     if (blk == NULL) {
diff --git a/block/vmdk.c b/block/vmdk.c
index 2f8e6cf..80f0338 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1661,7 +1661,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
         goto exit;
     }
 
-    blk = blk_new_open("extent", filename, NULL, NULL,
+    blk = blk_new_open(filename, NULL, NULL,
                        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
                        &local_err);
     if (blk == NULL) {
@@ -1946,7 +1946,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
             goto exit;
         }
 
-        blk = blk_new_open("backing", full_backing, NULL, NULL,
+        blk = blk_new_open(full_backing, NULL, NULL,
                            BDRV_O_NO_BACKING | BDRV_O_CACHE_WB, errp);
         g_free(full_backing);
         if (blk == NULL) {
@@ -2018,7 +2018,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
         }
     }
 
-    new_blk = blk_new_open("descriptor", filename, NULL, NULL,
+    new_blk = blk_new_open(filename, NULL, NULL,
                            BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
                            &local_err);
     if (new_blk == NULL) {
diff --git a/block/vpc.c b/block/vpc.c
index 0d1524d..8435205 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -888,7 +888,7 @@ static int vpc_create(const char *filename, QemuOpts *opts, Error **errp)
         goto out;
     }
 
-    blk = blk_new_open("image", filename, NULL, NULL,
+    blk = blk_new_open(filename, NULL, NULL,
                        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_PROTOCOL,
                        &local_err);
     if (blk == NULL) {
diff --git a/blockdev.c b/blockdev.c
index 5be7d4b..d0e3d9c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -147,6 +147,7 @@ void blockdev_auto_del(BlockBackend *blk)
     DriveInfo *dinfo = blk_legacy_dinfo(blk);
 
     if (dinfo && dinfo->auto_del) {
+        monitor_remove_blk(blk);
         blk_unref(blk);
     }
 }
@@ -561,7 +562,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     if ((!file || !*file) && !qdict_size(bs_opts)) {
         BlockBackendRootState *blk_rs;
 
-        blk = blk_new(qemu_opts_id(opts), errp);
+        blk = blk_new(errp);
         if (!blk) {
             goto early_err;
         }
@@ -597,8 +598,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
             bdrv_flags |= BDRV_O_INACTIVE;
         }
 
-        blk = blk_new_open(qemu_opts_id(opts), file, NULL, bs_opts, bdrv_flags,
-                           errp);
+        blk = blk_new_open(file, NULL, bs_opts, bdrv_flags, errp);
         if (!blk) {
             goto err_no_bs_opts;
         }
@@ -630,6 +630,12 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
 
     blk_set_on_error(blk, on_read_error, on_write_error);
 
+    if (!monitor_add_blk(blk, qemu_opts_id(opts), errp)) {
+        blk_unref(blk);
+        blk = NULL;
+        goto err_no_bs_opts;
+    }
+
 err_no_bs_opts:
     qemu_opts_del(opts);
     QDECREF(interval_dict);
@@ -2859,6 +2865,8 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
         blk_remove_bs(blk);
     }
 
+    monitor_remove_blk(blk);
+
     /* if we have a device attached to this BlockDriverState
      * then we need to make the drive anonymous until the device
      * can be removed.  If this is a drive with no device backing
@@ -3976,6 +3984,7 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 
     if (bs && bdrv_key_required(bs)) {
         if (blk) {
+            monitor_remove_blk(blk);
             blk_unref(blk);
         } else {
             QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
@@ -4005,6 +4014,7 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
     }
 
     if (has_id) {
+        /* blk_by_name() never returns a BB that is not owned by the monitor */
         blk = blk_by_name(id);
         if (!blk) {
             error_setg(errp, "Cannot find block backend %s", id);
@@ -4052,6 +4062,7 @@ void qmp_x_blockdev_del(bool has_id, const char *id,
     }
 
     if (blk) {
+        monitor_remove_blk(blk);
         blk_unref(blk);
     } else {
         QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list);
diff --git a/device-hotplug.c b/device-hotplug.c
index 3e5cdaa..126f73c 100644
--- a/device-hotplug.c
+++ b/device-hotplug.c
@@ -84,6 +84,8 @@ void hmp_drive_add(Monitor *mon, const QDict *qdict)
 
 err:
     if (dinfo) {
-        blk_unref(blk_by_legacy_dinfo(dinfo));
+        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
+        monitor_remove_blk(blk);
+        blk_unref(blk);
     }
 }
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 7bd5bde..635328f 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -917,7 +917,7 @@ static int blk_connect(struct XenDevice *xendev)
 
         /* setup via xenbus -> create new block driver instance */
         xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
-        blkdev->blk = blk_new_open(blkdev->dev, blkdev->filename, NULL, options,
+        blkdev->blk = blk_new_open(blkdev->filename, NULL, options,
                                    qflags, &local_err);
         if (!blkdev->blk) {
             xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c906c20..5edc427 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -59,11 +59,10 @@ typedef struct BlockDevOps {
     void (*resize_cb)(void *opaque);
 } BlockDevOps;
 
-BlockBackend *blk_new(const char *name, Error **errp);
-BlockBackend *blk_new_with_bs(const char *name, Error **errp);
-BlockBackend *blk_new_open(const char *name, const char *filename,
-                           const char *reference, QDict *options, int flags,
-                           Error **errp);
+BlockBackend *blk_new(Error **errp);
+BlockBackend *blk_new_with_bs(Error **errp);
+BlockBackend *blk_new_open(const char *filename, const char *reference,
+                           QDict *options, int flags, Error **errp);
 int blk_get_refcnt(BlockBackend *blk);
 void blk_ref(BlockBackend *blk);
 void blk_unref(BlockBackend *blk);
diff --git a/qemu-img.c b/qemu-img.c
index 3103150..29eae2a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -245,8 +245,7 @@ static int img_open_password(BlockBackend *blk, const char *filename,
 }
 
 
-static BlockBackend *img_open_opts(const char *id,
-                                   const char *optstr,
+static BlockBackend *img_open_opts(const char *optstr,
                                    QemuOpts *opts, int flags,
                                    bool require_io, bool quiet)
 {
@@ -254,7 +253,7 @@ static BlockBackend *img_open_opts(const char *id,
     Error *local_err = NULL;
     BlockBackend *blk;
     options = qemu_opts_to_qdict(opts, NULL);
-    blk = blk_new_open(id, NULL, NULL, options, flags, &local_err);
+    blk = blk_new_open(NULL, NULL, options, flags, &local_err);
     if (!blk) {
         error_reportf_err(local_err, "Could not open '%s'", optstr);
         return NULL;
@@ -267,7 +266,7 @@ static BlockBackend *img_open_opts(const char *id,
     return blk;
 }
 
-static BlockBackend *img_open_file(const char *id, const char *filename,
+static BlockBackend *img_open_file(const char *filename,
                                    const char *fmt, int flags,
                                    bool require_io, bool quiet)
 {
@@ -280,7 +279,7 @@ static BlockBackend *img_open_file(const char *id, const char *filename,
         qdict_put(options, "driver", qstring_from_str(fmt));
     }
 
-    blk = blk_new_open(id, filename, NULL, options, flags, &local_err);
+    blk = blk_new_open(filename, NULL, options, flags, &local_err);
     if (!blk) {
         error_reportf_err(local_err, "Could not open '%s': ", filename);
         return NULL;
@@ -294,8 +293,7 @@ static BlockBackend *img_open_file(const char *id, const char *filename,
 }
 
 
-static BlockBackend *img_open(const char *id,
-                              bool image_opts,
+static BlockBackend *img_open(bool image_opts,
                               const char *filename,
                               const char *fmt, int flags,
                               bool require_io, bool quiet)
@@ -312,9 +310,9 @@ static BlockBackend *img_open(const char *id,
         if (!opts) {
             return NULL;
         }
-        blk = img_open_opts(id, filename, opts, flags, true, quiet);
+        blk = img_open_opts(filename, opts, flags, true, quiet);
     } else {
-        blk = img_open_file(id, filename, fmt, flags, true, quiet);
+        blk = img_open_file(filename, fmt, flags, true, quiet);
     }
     return blk;
 }
@@ -686,7 +684,7 @@ static int img_check(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open("image", image_opts, filename, fmt, flags, true, quiet);
+    blk = img_open(image_opts, filename, fmt, flags, true, quiet);
     if (!blk) {
         return 1;
     }
@@ -878,7 +876,7 @@ static int img_commit(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open("image", image_opts, filename, fmt, flags, true, quiet);
+    blk = img_open(image_opts, filename, fmt, flags, true, quiet);
     if (!blk) {
         return 1;
     }
@@ -1212,13 +1210,13 @@ static int img_compare(int argc, char **argv)
         goto out3;
     }
 
-    blk1 = img_open("image_1", image_opts, filename1, fmt1, flags, true, quiet);
+    blk1 = img_open(image_opts, filename1, fmt1, flags, true, quiet);
     if (!blk1) {
         ret = 2;
         goto out3;
     }
 
-    blk2 = img_open("image_2", image_opts, filename2, fmt2, flags, true, quiet);
+    blk2 = img_open(image_opts, filename2, fmt2, flags, true, quiet);
     if (!blk2) {
         ret = 2;
         goto out2;
@@ -1899,11 +1897,8 @@ static int img_convert(int argc, char **argv)
 
     total_sectors = 0;
     for (bs_i = 0; bs_i < bs_n; bs_i++) {
-        char *id = bs_n > 1 ? g_strdup_printf("source_%d", bs_i)
-                            : g_strdup("source");
-        blk[bs_i] = img_open(id, image_opts, argv[optind + bs_i],
+        blk[bs_i] = img_open(image_opts, argv[optind + bs_i],
                              fmt, src_flags, true, quiet);
-        g_free(id);
         if (!blk[bs_i]) {
             ret = -1;
             goto out;
@@ -2048,8 +2043,7 @@ static int img_convert(int argc, char **argv)
      * the bdrv_create() call which takes different params.
      * Not critical right now, so fix can wait...
      */
-    out_blk = img_open_file("target", out_filename,
-                            out_fmt, flags, true, quiet);
+    out_blk = img_open_file(out_filename, out_fmt, flags, true, quiet);
     if (!out_blk) {
         ret = -1;
         goto out;
@@ -2240,7 +2234,7 @@ static ImageInfoList *collect_image_info_list(bool image_opts,
         }
         g_hash_table_insert(filenames, (gpointer)filename, NULL);
 
-        blk = img_open("image", image_opts, filename, fmt,
+        blk = img_open(image_opts, filename, fmt,
                        BDRV_O_FLAGS | BDRV_O_NO_BACKING,
                        false, false);
         if (!blk) {
@@ -2572,8 +2566,7 @@ static int img_map(int argc, char **argv)
         return 1;
     }
 
-    blk = img_open("image", image_opts, filename, fmt,
-                   BDRV_O_FLAGS, true, false);
+    blk = img_open(image_opts, filename, fmt, BDRV_O_FLAGS, true, false);
     if (!blk) {
         return 1;
     }
@@ -2718,8 +2711,7 @@ static int img_snapshot(int argc, char **argv)
     }
 
     /* Open the image */
-    blk = img_open("image", image_opts, filename, NULL,
-                   bdrv_oflags, true, quiet);
+    blk = img_open(image_opts, filename, NULL, bdrv_oflags, true, quiet);
     if (!blk) {
         return 1;
     }
@@ -2890,7 +2882,7 @@ static int img_rebase(int argc, char **argv)
      * Ignore the old backing file for unsafe rebase in case we want to correct
      * the reference to a renamed or moved backing file.
      */
-    blk = img_open("image", image_opts, filename, fmt, flags, true, quiet);
+    blk = img_open(image_opts, filename, fmt, flags, true, quiet);
     if (!blk) {
         ret = -1;
         goto out;
@@ -2916,7 +2908,7 @@ static int img_rebase(int argc, char **argv)
         }
 
         bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
-        blk_old_backing = blk_new_open("old_backing", backing_name, NULL,
+        blk_old_backing = blk_new_open(backing_name, NULL,
                                        options, src_flags, &local_err);
         if (!blk_old_backing) {
             error_reportf_err(local_err,
@@ -2933,7 +2925,7 @@ static int img_rebase(int argc, char **argv)
                 options = NULL;
             }
 
-            blk_new_backing = blk_new_open("new_backing", out_baseimg, NULL,
+            blk_new_backing = blk_new_open(out_baseimg, NULL,
                                            options, src_flags, &local_err);
             if (!blk_new_backing) {
                 error_reportf_err(local_err,
@@ -3227,7 +3219,7 @@ static int img_resize(int argc, char **argv)
     n = qemu_opt_get_size(param, BLOCK_OPT_SIZE, 0);
     qemu_opts_del(param);
 
-    blk = img_open("image", image_opts, filename, fmt,
+    blk = img_open(image_opts, filename, fmt,
                    BDRV_O_FLAGS | BDRV_O_RDWR, true, quiet);
     if (!blk) {
         ret = -1;
@@ -3387,7 +3379,7 @@ static int img_amend(int argc, char **argv)
         goto out;
     }
 
-    blk = img_open("image", image_opts, filename, fmt, flags, true, quiet);
+    blk = img_open(image_opts, filename, fmt, flags, true, quiet);
     if (!blk) {
         ret = -1;
         goto out;
diff --git a/qemu-io.c b/qemu-io.c
index 8c31257..d7c2f26 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -61,7 +61,7 @@ static int openfile(char *name, int flags, QDict *opts)
         return 1;
     }
 
-    qemuio_blk = blk_new_open("hda", name, NULL, opts, flags, &local_err);
+    qemuio_blk = blk_new_open(name, NULL, opts, flags, &local_err);
     if (!qemuio_blk) {
         error_reportf_err(local_err, "can't open%s%s: ",
                           name ? " device " : "", name ?: "");
diff --git a/qemu-nbd.c b/qemu-nbd.c
index a5c1d95..d5f8473 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -831,13 +831,13 @@ int main(int argc, char **argv)
         }
         options = qemu_opts_to_qdict(opts, NULL);
         qemu_opts_reset(&file_opts);
-        blk = blk_new_open("hda", NULL, NULL, options, flags, &local_err);
+        blk = blk_new_open(NULL, NULL, options, flags, &local_err);
     } else {
         if (fmt) {
             options = qdict_new();
             qdict_put(options, "driver", qstring_from_str(fmt));
         }
-        blk = blk_new_open("hda", srcpath, NULL, options, flags, &local_err);
+        blk = blk_new_open(srcpath, NULL, options, flags, &local_err);
     }
 
     if (!blk) {
diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
index 7d62cd5..d0662f9 100644
--- a/tests/qemu-iotests/087.out
+++ b/tests/qemu-iotests/087.out
@@ -21,7 +21,7 @@ QMP_VERSION
 {"error": {"class": "GenericError", "desc": "Device name 'test-node' conflicts with an existing node name"}}
 {"error": {"class": "GenericError", "desc": "node-name=disk is conflicting with a device id"}}
 {"error": {"class": "GenericError", "desc": "Duplicate node name"}}
-{"error": {"class": "GenericError", "desc": "node-name=disk3 is conflicting with a device id"}}
+{"error": {"class": "GenericError", "desc": "Device name 'disk3' conflicts with an existing node name"}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
 
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 11/17] blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del()
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (9 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 10/17] blockdev: Split monitor reference from BB creation Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 12/17] block: Move some bdrv_*_all() functions to BB Max Reitz
                   ` (6 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

We can basically inline it in hmp_drive_del(); monitor_remove_blk() is
called already, so we just need to call bdrv_make_anon(), too.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 17 -----------------
 blockdev.c                     | 11 ++++++-----
 include/sysemu/block-backend.h |  2 --
 3 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 9ed3912..68f3662 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -386,23 +386,6 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
 }
 
 /*
- * Hide @blk.
- * @blk must not have been hidden already.
- * Make attached BlockDriverState, if any, anonymous.
- * Once hidden, @blk is invisible to all functions that don't receive
- * it as argument.  For example, blk_by_name() won't return it.
- * Strictly for use by do_drive_del().
- * TODO get rid of it!
- */
-void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
-{
-    monitor_remove_blk(blk);
-    if (blk->bs) {
-        bdrv_make_anon(blk->bs);
-    }
-}
-
-/*
  * Disassociates the currently associated BlockDriverState from @blk.
  */
 void blk_remove_bs(BlockBackend *blk)
diff --git a/blockdev.c b/blockdev.c
index d0e3d9c..b6d2444 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2865,15 +2865,16 @@ void hmp_drive_del(Monitor *mon, const QDict *qdict)
         blk_remove_bs(blk);
     }
 
+    /* Make the BlockBackend and the attached BlockDriverState anonymous */
     monitor_remove_blk(blk);
+    if (blk_bs(blk)) {
+        bdrv_make_anon(blk_bs(blk));
+    }
 
-    /* if we have a device attached to this BlockDriverState
-     * then we need to make the drive anonymous until the device
-     * can be removed.  If this is a drive with no device backing
-     * then we can just get rid of the block driver state right here.
+    /* If this BlockBackend has a device attached to it, its refcount will be
+     * decremented when the device is removed; otherwise we have to do so here.
      */
     if (blk_get_attached_dev(blk)) {
-        blk_hide_on_behalf_of_hmp_drive_del(blk);
         /* Further I/O must not pause the guest */
         blk_set_on_error(blk, BLOCKDEV_ON_ERROR_REPORT,
                          BLOCKDEV_ON_ERROR_REPORT);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 5edc427..60c4b07 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -77,8 +77,6 @@ BlockDriverState *blk_bs(BlockBackend *blk);
 void blk_remove_bs(BlockBackend *blk);
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
 
-void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk);
-
 void blk_set_allow_write_beyond_eof(BlockBackend *blk, bool allow);
 void blk_iostatus_enable(BlockBackend *blk);
 bool blk_iostatus_is_enabled(const BlockBackend *blk);
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 12/17] block: Move some bdrv_*_all() functions to BB
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (10 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 11/17] blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del() Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 13/17] block: Add bdrv_next_monitor_owned() Max Reitz
                   ` (5 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Move bdrv_commit_all() and bdrv_flush_all() to the BlockBackend level.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                                       | 20 ------------
 block/block-backend.c                         | 44 +++++++++++++++++++++++----
 block/io.c                                    | 20 ------------
 include/block/block.h                         |  2 --
 stubs/Makefile.objs                           |  2 +-
 stubs/{bdrv-commit-all.c => blk-commit-all.c} |  4 +--
 6 files changed, 41 insertions(+), 51 deletions(-)
 rename stubs/{bdrv-commit-all.c => blk-commit-all.c} (53%)

diff --git a/block.c b/block.c
index 582a9e8..91c006a 100644
--- a/block.c
+++ b/block.c
@@ -2521,26 +2521,6 @@ ro_cleanup:
     return ret;
 }
 
-int bdrv_commit_all(void)
-{
-    BlockDriverState *bs;
-
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-
-        aio_context_acquire(aio_context);
-        if (bs->drv && bs->backing) {
-            int ret = bdrv_commit(bs);
-            if (ret < 0) {
-                aio_context_release(aio_context);
-                return ret;
-            }
-        }
-        aio_context_release(aio_context);
-    }
-    return 0;
-}
-
 /*
  * Return values:
  * 0        - success
diff --git a/block/block-backend.c b/block/block-backend.c
index 68f3662..b3c3d39 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -904,11 +904,6 @@ int blk_flush(BlockBackend *blk)
     return bdrv_flush(blk->bs);
 }
 
-int blk_flush_all(void)
-{
-    return bdrv_flush_all();
-}
-
 void blk_drain(BlockBackend *blk)
 {
     if (blk->bs) {
@@ -1357,5 +1352,42 @@ BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
 
 int blk_commit_all(void)
 {
-    return bdrv_commit_all();
+    BlockBackend *blk = NULL;
+
+    while ((blk = blk_all_next(blk)) != NULL) {
+        AioContext *aio_context = blk_get_aio_context(blk);
+
+        aio_context_acquire(aio_context);
+        if (blk_is_inserted(blk) && blk->bs->backing) {
+            int ret = bdrv_commit(blk->bs);
+            if (ret < 0) {
+                aio_context_release(aio_context);
+                return ret;
+            }
+        }
+        aio_context_release(aio_context);
+    }
+    return 0;
+}
+
+int blk_flush_all(void)
+{
+    BlockBackend *blk = NULL;
+    int result = 0;
+
+    while ((blk = blk_all_next(blk)) != NULL) {
+        AioContext *aio_context = blk_get_aio_context(blk);
+        int ret;
+
+        aio_context_acquire(aio_context);
+        if (blk_is_inserted(blk)) {
+            ret = blk_flush(blk);
+            if (ret < 0 && !result) {
+                result = ret;
+            }
+        }
+        aio_context_release(aio_context);
+    }
+
+    return result;
 }
diff --git a/block/io.c b/block/io.c
index a69bfc4..5f9b6d6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1445,26 +1445,6 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDriverState *bs,
                              BDRV_REQ_ZERO_WRITE | flags);
 }
 
-int bdrv_flush_all(void)
-{
-    BlockDriverState *bs = NULL;
-    int result = 0;
-
-    while ((bs = bdrv_next(bs))) {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-        int ret;
-
-        aio_context_acquire(aio_context);
-        ret = bdrv_flush(bs);
-        if (ret < 0 && !result) {
-            result = ret;
-        }
-        aio_context_release(aio_context);
-    }
-
-    return result;
-}
-
 typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
diff --git a/include/block/block.h b/include/block/block.h
index eaa6426..ea5be0f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -274,7 +274,6 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
 void bdrv_refresh_limits(BlockDriverState *bs, Error **errp);
 int bdrv_commit(BlockDriverState *bs);
-int bdrv_commit_all(void);
 int bdrv_change_backing_file(BlockDriverState *bs,
     const char *backing_file, const char *backing_fmt);
 void bdrv_register(BlockDriver *bdrv);
@@ -373,7 +372,6 @@ int bdrv_inactivate_all(void);
 /* Ensure contents are flushed to disk.  */
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
-int bdrv_flush_all(void);
 void bdrv_close_all(void);
 void bdrv_drain(BlockDriverState *bs);
 void bdrv_drain_all(void);
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index e922de9..9d9f1d0 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,5 +1,5 @@
 stub-obj-y += arch-query-cpu-def.o
-stub-obj-y += bdrv-commit-all.o
+stub-obj-y += blk-commit-all.o
 stub-obj-y += blockdev-close-all-bdrv-states.o
 stub-obj-y += clock-warp.o
 stub-obj-y += cpu-get-clock.o
diff --git a/stubs/bdrv-commit-all.c b/stubs/blk-commit-all.c
similarity index 53%
rename from stubs/bdrv-commit-all.c
rename to stubs/blk-commit-all.c
index bf84a1d..c82fb7f 100644
--- a/stubs/bdrv-commit-all.c
+++ b/stubs/blk-commit-all.c
@@ -1,8 +1,8 @@
 #include "qemu/osdep.h"
 #include "qemu-common.h"
-#include "block/block.h"
+#include "sysemu/block-backend.h"
 
-int bdrv_commit_all(void)
+int blk_commit_all(void)
 {
     return 0;
 }
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 13/17] block: Add bdrv_next_monitor_owned()
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (11 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 12/17] block: Move some bdrv_*_all() functions to BB Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 14/17] block: Add blk_next_root_bs() Max Reitz
                   ` (4 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Add a function for iterating over all monitor-owned BlockDriverStates so
the generic block layer can do so.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c                      | 7 +++++++
 include/block/block.h           | 1 +
 stubs/Makefile.objs             | 1 +
 stubs/bdrv-next-monitor-owned.c | 8 ++++++++
 4 files changed, 17 insertions(+)
 create mode 100644 stubs/bdrv-next-monitor-owned.c

diff --git a/blockdev.c b/blockdev.c
index b6d2444..a5df7e7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -723,6 +723,13 @@ void blockdev_close_all_bdrv_states(void)
     }
 }
 
+/* Iterates over the list of monitor-owned BlockDriverStates */
+BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs)
+{
+    return bs ? QTAILQ_NEXT(bs, monitor_list)
+              : QTAILQ_FIRST(&monitor_bdrv_states);
+}
+
 static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
                             Error **errp)
 {
diff --git a/include/block/block.h b/include/block/block.h
index ea5be0f..09272c3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -412,6 +412,7 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
 bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
 BlockDriverState *bdrv_next_node(BlockDriverState *bs);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
+BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
 int bdrv_is_encrypted(BlockDriverState *bs);
 int bdrv_key_required(BlockDriverState *bs);
 int bdrv_set_key(BlockDriverState *bs, const char *key);
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 9d9f1d0..b6d1e65 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -1,4 +1,5 @@
 stub-obj-y += arch-query-cpu-def.o
+stub-obj-y += bdrv-next-monitor-owned.o
 stub-obj-y += blk-commit-all.o
 stub-obj-y += blockdev-close-all-bdrv-states.o
 stub-obj-y += clock-warp.o
diff --git a/stubs/bdrv-next-monitor-owned.c b/stubs/bdrv-next-monitor-owned.c
new file mode 100644
index 0000000..2acf6c3
--- /dev/null
+++ b/stubs/bdrv-next-monitor-owned.c
@@ -0,0 +1,8 @@
+#include "qemu/osdep.h"
+#include "qemu-common.h"
+#include "block/block.h"
+
+BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs)
+{
+    return NULL;
+}
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 14/17] block: Add blk_next_root_bs()
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (12 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 13/17] block: Add bdrv_next_monitor_owned() Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 15/17] block: Rewrite bdrv_next() Max Reitz
                   ` (3 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

This function iterates over all BDSs attached to a BB. We are going to
need it when rewriting bdrv_next() so it no longer uses bdrv_states.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 24 ++++++++++++++++++++++++
 include/sysemu/block-backend.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index b3c3d39..7818aa2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -250,6 +250,30 @@ BlockBackend *blk_next(BlockBackend *blk)
 }
 
 /*
+ * Iterates over all BlockDriverStates which are attached to a BlockBackend.
+ * This function is for use by bdrv_next().
+ *
+ * @bs must be NULL or a BDS that is attached to a BB.
+ */
+BlockDriverState *blk_next_root_bs(BlockDriverState *bs)
+{
+    BlockBackend *blk;
+
+    if (bs) {
+        assert(bs->blk);
+        blk = bs->blk;
+    } else {
+        blk = NULL;
+    }
+
+    do {
+        blk = blk_all_next(blk);
+    } while (blk && !blk->bs);
+
+    return blk ? blk->bs : NULL;
+}
+
+/*
  * Add a BlockBackend into the list of backends referenced by the monitor, with
  * the given @name acting as the handle for the monitor.
  * Strictly for use by blockdev.c.
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 60c4b07..d839bff 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -70,6 +70,7 @@ void blk_remove_all_bs(void);
 const char *blk_name(BlockBackend *blk);
 BlockBackend *blk_by_name(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
+BlockDriverState *blk_next_root_bs(BlockDriverState *bs);
 bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp);
 void monitor_remove_blk(BlockBackend *blk);
 
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 15/17] block: Rewrite bdrv_next()
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (13 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 14/17] block: Add blk_next_root_bs() Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 16/17] block: Use bdrv_next() instead of bdrv_states Max Reitz
                   ` (2 subsequent siblings)
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Instead of using the bdrv_states list, iterate over all the
BlockDriverStates attached to BlockBackends, and over all the
monitor-owned BDSs afterwards (except for those attached to a BB).

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 91c006a..8f700f1 100644
--- a/block.c
+++ b/block.c
@@ -2969,12 +2969,23 @@ BlockDriverState *bdrv_next_node(BlockDriverState *bs)
     return QTAILQ_NEXT(bs, node_list);
 }
 
+/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by
+ * the monitor or attached to a BlockBackend */
 BlockDriverState *bdrv_next(BlockDriverState *bs)
 {
-    if (!bs) {
-        return QTAILQ_FIRST(&bdrv_states);
+    if (!bs || bs->blk) {
+        bs = blk_next_root_bs(bs);
+        if (bs) {
+            return bs;
+        }
     }
-    return QTAILQ_NEXT(bs, device_list);
+
+    /* Ignore all BDSs that are attached to a BlockBackend here; they have been
+     * handled by the above block already */
+    do {
+        bs = bdrv_next_monitor_owned(bs);
+    } while (bs && bs->blk);
+    return bs;
 }
 
 const char *bdrv_get_node_name(const BlockDriverState *bs)
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 16/17] block: Use bdrv_next() instead of bdrv_states
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (14 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 15/17] block: Rewrite bdrv_next() Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 17/17] block: Remove bdrv_states list Max Reitz
  2016-03-17 13:44 ` [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Kevin Wolf
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

There is no point in manually iterating through the bdrv_states list
when there is bdrv_next().

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 8f700f1..5d848fb 100644
--- a/block.c
+++ b/block.c
@@ -3293,10 +3293,10 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
 
 void bdrv_invalidate_cache_all(Error **errp)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
     Error *local_err = NULL;
 
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+    while ((bs = bdrv_next(bs)) != NULL) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
         aio_context_acquire(aio_context);
@@ -3326,10 +3326,10 @@ static int bdrv_inactivate(BlockDriverState *bs)
 
 int bdrv_inactivate_all(void)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
     int ret;
 
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+    while ((bs = bdrv_next(bs)) != NULL) {
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
         aio_context_acquire(aio_context);
@@ -3835,10 +3835,10 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
  */
 bool bdrv_is_first_non_filter(BlockDriverState *candidate)
 {
-    BlockDriverState *bs;
+    BlockDriverState *bs = NULL;
 
     /* walk down the bs forest recursively */
-    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
+    while ((bs = bdrv_next(bs)) != NULL) {
         bool perm;
 
         /* try to recurse in this top level bs */
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [Qemu-devel] [PATCH v4 17/17] block: Remove bdrv_states list
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (15 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 16/17] block: Use bdrv_next() instead of bdrv_states Max Reitz
@ 2016-03-16 18:54 ` Max Reitz
  2016-03-17 13:44 ` [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Kevin Wolf
  17 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-16 18:54 UTC (permalink / raw)
  To: qemu-block
  Cc: Kevin Wolf, Markus Armbruster, qemu-devel, Stefan Hajnoczi, Max Reitz

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block.c                   | 31 +++----------------------------
 blockdev.c                |  7 -------
 include/block/block.h     |  1 -
 include/block/block_int.h |  4 ----
 4 files changed, 3 insertions(+), 40 deletions(-)

diff --git a/block.c b/block.c
index 5d848fb..c47f99d 100644
--- a/block.c
+++ b/block.c
@@ -55,8 +55,6 @@
 
 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
 
-struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states);
-
 static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
     QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
 
@@ -226,10 +224,7 @@ void bdrv_register(BlockDriver *bdrv)
 
 BlockDriverState *bdrv_new_root(void)
 {
-    BlockDriverState *bs = bdrv_new();
-
-    QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
-    return bs;
+    return bdrv_new();
 }
 
 BlockDriverState *bdrv_new(void)
@@ -2230,26 +2225,10 @@ void bdrv_close_all(void)
     }
 }
 
-/* Note that bs->device_list.tqe_prev is initially null,
- * and gets set to non-null by QTAILQ_INSERT_TAIL().  Establish
- * the useful invariant "bs in bdrv_states iff bs->tqe_prev" by
- * resetting it to null on remove.  */
-void bdrv_device_remove(BlockDriverState *bs)
-{
-    QTAILQ_REMOVE(&bdrv_states, bs, device_list);
-    bs->device_list.tqe_prev = NULL;
-}
-
-/* make a BlockDriverState anonymous by removing from bdrv_state and
- * graph_bdrv_state list.
-   Also, NULL terminate the device_name to prevent double remove */
+/* make a BlockDriverState anonymous by removing from graph_bdrv_state list.
+ * Also, NULL terminate the device_name to prevent double remove */
 void bdrv_make_anon(BlockDriverState *bs)
 {
-    /* Take care to remove bs from bdrv_states only when it's actually
-     * in it. */
-    if (bs->device_list.tqe_prev) {
-        bdrv_device_remove(bs);
-    }
     if (bs->node_name[0] != '\0') {
         QTAILQ_REMOVE(&graph_bdrv_states, bs, node_list);
     }
@@ -2286,10 +2265,6 @@ static void change_parent_backing_link(BlockDriverState *from,
     }
     if (from->blk) {
         blk_set_bs(from->blk, to);
-        if (!to->device_list.tqe_prev) {
-            QTAILQ_INSERT_BEFORE(from, to, device_list);
-        }
-        bdrv_device_remove(from);
     }
 }
 
diff --git a/blockdev.c b/blockdev.c
index a5df7e7..85dee57 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2426,11 +2426,6 @@ void qmp_x_blockdev_remove_medium(const char *device, Error **errp)
         goto out;
     }
 
-    /* This follows the convention established by bdrv_make_anon() */
-    if (bs->device_list.tqe_prev) {
-        bdrv_device_remove(bs);
-    }
-
     blk_remove_bs(blk);
 
     if (!blk_dev_has_tray(blk)) {
@@ -2478,8 +2473,6 @@ static void qmp_blockdev_insert_anon_medium(const char *device,
 
     blk_insert_bs(blk, bs);
 
-    QTAILQ_INSERT_TAIL(&bdrv_states, bs, device_list);
-
     if (!blk_dev_has_tray(blk)) {
         /* For tray-less devices, blockdev-close-tray is a no-op (or may not be
          * called at all); therefore, the medium needs to be pushed into the
diff --git a/include/block/block.h b/include/block/block.h
index 09272c3..ea8ed04 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -201,7 +201,6 @@ int bdrv_create(BlockDriver *drv, const char* filename,
 int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
 BlockDriverState *bdrv_new_root(void);
 BlockDriverState *bdrv_new(void);
-void bdrv_device_remove(BlockDriverState *bs);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_replace_in_backing_chain(BlockDriverState *old,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dda5ba0..bbf617a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -442,8 +442,6 @@ struct BlockDriverState {
     char node_name[32];
     /* element of the list of named nodes building the graph */
     QTAILQ_ENTRY(BlockDriverState) node_list;
-    /* element of the list of "drives" the guest sees */
-    QTAILQ_ENTRY(BlockDriverState) device_list;
     /* element of the list of all BlockDriverStates (all_bdrv_states) */
     QTAILQ_ENTRY(BlockDriverState) bs_list;
     /* element of the list of monitor-owned BDS */
@@ -501,8 +499,6 @@ extern BlockDriver bdrv_file;
 extern BlockDriver bdrv_raw;
 extern BlockDriver bdrv_qcow2;
 
-extern QTAILQ_HEAD(BdrvStates, BlockDriverState) bdrv_states;
-
 /**
  * bdrv_setup_io_funcs:
  *
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work
  2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
                   ` (16 preceding siblings ...)
  2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 17/17] block: Remove bdrv_states list Max Reitz
@ 2016-03-17 13:44 ` Kevin Wolf
  2016-03-23 19:06   ` Max Reitz
  17 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2016-03-17 13:44 UTC (permalink / raw)
  To: Max Reitz; +Cc: Stefan Hajnoczi, qemu-devel, qemu-block, Markus Armbruster

Am 16.03.2016 um 19:54 hat Max Reitz geschrieben:
> This series appears to reinvent itself with every revision. This time,
> its main implication is that BBs are no longer automatically treated as
> monitor-owned, and that a BB's name is tightly tied to the monitor
> reference (it is considered equivalent to that reference).

Thanks, applied to the block branch.

> v4:
> - blk_hide_on_behalf_of_hmp_drive_del() does a bit more than what
>   monitor_remove_blk() does; namely, it invokes bdrv_make_anon() on the
>   BDS. We should probably continue to do so.

Probably not, removing the node-name of a BDS just because a BB
disappears certainly feels like a bug. Even more so now that node-names
are auto-generated and the general assumption is that all BDSes have
one.

But that's a preexisting problem, so we can fix it on top.

Kevin

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work
  2016-03-17 13:44 ` [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Kevin Wolf
@ 2016-03-23 19:06   ` Max Reitz
  0 siblings, 0 replies; 20+ messages in thread
From: Max Reitz @ 2016-03-23 19:06 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Stefan Hajnoczi, qemu-devel, qemu-block, Markus Armbruster


[-- Attachment #1.1: Type: text/plain, Size: 1067 bytes --]

On 17.03.2016 14:44, Kevin Wolf wrote:
> Am 16.03.2016 um 19:54 hat Max Reitz geschrieben:
>> This series appears to reinvent itself with every revision. This time,
>> its main implication is that BBs are no longer automatically treated as
>> monitor-owned, and that a BB's name is tightly tied to the monitor
>> reference (it is considered equivalent to that reference).
> 
> Thanks, applied to the block branch.

Great. :-)

>> v4:
>> - blk_hide_on_behalf_of_hmp_drive_del() does a bit more than what
>>   monitor_remove_blk() does; namely, it invokes bdrv_make_anon() on the
>>   BDS. We should probably continue to do so.
> 
> Probably not, removing the node-name of a BDS just because a BB
> disappears certainly feels like a bug. Even more so now that node-names
> are auto-generated and the general assumption is that all BDSes have
> one.

Yes, I meant in the sense of "It's what we did so far and I don't want
to mess with it in this series".

Max

> But that's a preexisting problem, so we can fix it on top.
> 
> Kevin
> 



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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2016-03-23 19:06 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-16 18:54 [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 01/17] monitor: Use BB list for BB name completion Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 02/17] block: Use blk_next() in block-backend.c Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 03/17] block: Add blk_commit_all() Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 04/17] block: Use blk_{commit, flush}_all() consistently Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 05/17] qapi: Drop QERR_UNKNOWN_BLOCK_FORMAT_FEATURE Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 06/17] block: Drop BB name from bad option error Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 07/17] blockdev: Rename blk_backends Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 08/17] blockdev: Add list of all BlockBackends Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 09/17] blockdev: Separate BB name management Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 10/17] blockdev: Split monitor reference from BB creation Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 11/17] blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del() Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 12/17] block: Move some bdrv_*_all() functions to BB Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 13/17] block: Add bdrv_next_monitor_owned() Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 14/17] block: Add blk_next_root_bs() Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 15/17] block: Rewrite bdrv_next() Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 16/17] block: Use bdrv_next() instead of bdrv_states Max Reitz
2016-03-16 18:54 ` [Qemu-devel] [PATCH v4 17/17] block: Remove bdrv_states list Max Reitz
2016-03-17 13:44 ` [Qemu-devel] [PATCH v4 00/17] blockdev: Further BlockBackend work Kevin Wolf
2016-03-23 19:06   ` Max Reitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.