Re: [Qemu-devel] [PATCH 1/6] blockdev: Orphaned drive search

[John Snow]

On 09/24/2014 10:06 AM, Markus Armbruster wrote:
John Snow <js...@redhat.com> writes:

When users use command line options like -hda, -cdrom,
or even -drive if=ide, it is up to the board initialization
routines to pick up these drives and create backing
devices for them.

Some boards, like Q35, have not been doing this.
However, there is no warning explaining why certain
drive specifications are just silently ignored,
so this function adds a check to print some warnings
to assist users in debugging these sorts of issues
in the future.

This patch will warn about drives added with if_none,

Judging from my testing, I suspect you mean "will not warn" ;)

A fun story: I did actually mean what I wrote, but in my own testing, 
found that this produced warnings for the test suite, so I decided to 
change the behavior to not warn for IF_NONE.

I figured it would make sense if it gave a little warning that amounted 
to "Don't forget to add a device for this drive!" but I didn't want to 
see any more output on the test suite, so I nixed it.

I amended my commit message for V2.

for which it is not possible to tell in advance if
the omission of a backing device is an issue.

A warning in these cases is considered appropriate.

Signed-off-by: John Snow <js...@redhat.com>
---
  blockdev.c                | 21 +++++++++++++++++++++
  include/sysemu/blockdev.h |  2 ++
  vl.c                      | 12 +++++++++++-
  3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index b361fbb..81398e7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -166,6 +166,27 @@ DriveInfo *drive_get(BlockInterfaceType type, int bus, int 
unit)
      return NULL;
  }

+bool drive_check_orphaned(void)
+{
+    DriveInfo *dinfo;
+    bool rs = false;
+
+    QTAILQ_FOREACH(dinfo, &drives, next) {
+        /* If dinfo->bdrv->dev is NULL, it has no device attached. */
+        /* Unless this is a default drive, this may be an oversight. */
+        if (!dinfo->bdrv->dev && !dinfo->is_default &&
+            dinfo->type != IF_NONE) {
+            fprintf(stderr, "Warning: Orphaned drive without device: "
+                    "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
+                    dinfo->id, dinfo->bdrv->filename, if_name[dinfo->type],
+                    dinfo->bus, dinfo->unit);
+            rs = true;
+        }
+    }
+
+    return rs;
+}
+
  DriveInfo *drive_get_by_index(BlockInterfaceType type, int index)
  {
      return drive_get(type,
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 23a5d10..80f768d 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -38,6 +38,7 @@ struct DriveInfo {
      int unit;
      int auto_del;               /* see blockdev_mark_auto_del() */
      bool enable_auto_del;       /* Only for legacy drive_new() */
+    bool is_default;            /* Added by default_drive() ?  */
      int media_cd;
      int cyls, heads, secs, trans;
      QemuOpts *opts;
@@ -46,6 +47,7 @@ struct DriveInfo {
  };

  DriveInfo *drive_get(BlockInterfaceType type, int bus, int unit);
+bool drive_check_orphaned(void);
  DriveInfo *drive_get_by_index(BlockInterfaceType type, int index);
  int drive_get_max_bus(BlockInterfaceType type);
  DriveInfo *drive_get_next(BlockInterfaceType type);
diff --git a/vl.c b/vl.c
index dc792fe..eaef240 100644
--- a/vl.c
+++ b/vl.c
@@ -1168,6 +1168,7 @@ static void default_drive(int enable, int snapshot, 
BlockInterfaceType type,
                            int index, const char *optstr)
  {
      QemuOpts *opts;
+    DriveInfo *dinfo;

      if (!enable || drive_get_by_index(type, index)) {
          return;
@@ -1177,9 +1178,13 @@ static void default_drive(int enable, int snapshot, 
BlockInterfaceType type,
      if (snapshot) {
          drive_enable_snapshot(opts, NULL);
      }
-    if (!drive_new(opts, type)) {
+
+    dinfo = drive_new(opts, type);
+    if (!dinfo) {
          exit(1);
      }
+    dinfo->is_default = true;
+
  }

  void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
@@ -4457,6 +4462,11 @@ int main(int argc, char **argv, char **envp)
      if (qemu_opts_foreach(qemu_find_opts("device"), device_init_func, NULL, 
1) != 0)
          exit(1);

+    /* anybody left over? */
+    if (drive_check_orphaned()) {
+        fprintf(stderr, "Warning: found drives without a backing device.\n");
+    }
+
      net_check_clients();

      ds = init_displaystate();

Terminology: the device model is the front end, the thing created by
-drive is the back end.  I'd simply  drop this message.  If you want to
keep it, rephrase it to avoid "backing device".


--
—js