Re: [Qemu-devel] [libvirt] [PATCH RFC] blockdev: copy legacy and common opts to qemu_drive_opts

[Amos Kong <akong@redhat.com>]

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

Hi Kevin,

On Mon, Nov 04, 2013 at 12:27:10PM +0100, Kevin Wolf wrote:
> Am 04.11.2013 um 08:01 hat Amos Kong geschrieben:
> > Currently we have three QemuOptsList (qemu_common_drive_opts,
> > qemu_legacy_drive_opts, and qemu_drive_opts), only qemu_drive_opts
> > is added to vm_config_groups[].
> > 
> > We query commandline options by checking information in
> > vm_config_groups[], so we can only get a NULL parameter list now.
> > 
> > This patch copied desc items of qemu_legacy_drive_opts and
> > qemu_common_drive_opts to qemu_drive_opts.
> > 
> > Signed-off-by: Amos Kong <akong@redhat.com>
> 
> This breaks driver-specific options because they aren't (and cannot be)
> listed in the QemuOptsList.
> 
> For example:
> 
> $ x86_64-softmmu/qemu-system-x86_64 -drive file.driver=nbd,file.host=localhost
> qemu-system-x86_64: -drive file.driver=nbd,file.host=localhost: Invalid parameter 'file.driver'
> 
> query-command-line-options isn't an appropriate API to query the -drive
> capabilities in the blockdev-add world. You really want to have
> introspection for that.
> 
> For compatibility, we might want to at least expose part of the provided
> options there. In this case you should modify the monitor command to
> access the local QemuOptsLists of drive_init() and blockdev_init() for
> option="drive".

It's to access the local QemuOptsLists of drive_init() and
blockdev_init() for option="drive".

I saw there are two repeat items ("copy-on-read", "read-only")
I will merge the two desc lists together, remove the repeat items,
and output once.

Attached my draft patch, I can use it to compile qemu binary, but
touched following error, any note?

------------------------
[amos@amosk qemu]$ make
  CC    util/qemu-config.o
  AR    libqemuutil.a
  LINK  qemu-ga
  LINK  qemu-nbd
  LINK  qemu-img
  LINK  qemu-io
libqemuutil.a(qemu-config.o): In function `qmp_query_command_line_options':
/home/devel/qemu/util/qemu-config.c:141: undefined reference to `qemu_legacy_drive_opts'
/home/devel/qemu/util/qemu-config.c:142: undefined reference to `qemu_common_drive_opts'
/home/devel/qemu/util/qemu-config.c:144: undefined reference to `qemu_drive_opts'
collect2: error: ld returned 1 exit status
make: *** [qemu-nbd] Error 1
make: *** Waiting for unfinished jobs....
libqemuutil.a(qemu-config.o): In function `qmp_query_command_line_options':
/home/devel/qemu/util/qemu-config.c:141: undefined reference to `qemu_legacy_drive_opts'
/home/devel/qemu/util/qemu-config.c:142: undefined reference to `qemu_common_drive_opts'
/home/devel/qemu/util/qemu-config.c:144: undefined reference to `qemu_drive_opts'
collect2: error: ld returned 1 exit status
make: *** [qemu-img] Error 1
libqemuutil.a(qemu-config.o): In function `qmp_query_command_line_options':
/home/devel/qemu/util/qemu-config.c:141: undefined reference to `qemu_legacy_drive_opts'
/home/devel/qemu/util/qemu-config.c:142: undefined reference to `qemu_common_drive_opts'
/home/devel/qemu/util/qemu-config.c:144: undefined reference to `qemu_drive_opts'
collect2: error: ld returned 1 exit status
make: *** [qemu-io] Error 1
  LINK  x86_64-softmmu/qemu-system-x86_64


-- 
			Amos.

[-- Attachment #2: 0001-qmp-access-the-local-QemuOptsLists-for-drive-option.patch --]
[-- Type: text/plain, Size: 4565 bytes --]

>From c95a05bde835481e861853c4365c768be26335e1 Mon Sep 17 00:00:00 2001
From: Amos Kong <akong@redhat.com>
Date: Tue, 5 Nov 2013 17:59:13 +0800
Subject: [PATCH] qmp: access the local QemuOptsLists for drive option

Currently we have three QemuOptsList (qemu_common_drive_opts,
qemu_legacy_drive_opts, and qemu_drive_opts), only qemu_drive_opts
is added to vm_config_groups[].

This patch changes query-command-line-options to access three list,
and merge the result together.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 blockdev.c              |  1 -
 include/sysemu/sysemu.h |  2 ++
 util/qemu-config.c      | 61 ++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b260477..f09f991 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -47,7 +47,6 @@
 #include "sysemu/arch_init.h"
 
 static QTAILQ_HEAD(drivelist, DriveInfo) drives = QTAILQ_HEAD_INITIALIZER(drives);
-extern QemuOptsList qemu_common_drive_opts;
 
 static const char *const if_name[IF_COUNT] = {
     [IF_NONE] = "none",
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index cd5791e..495dae8 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -193,6 +193,8 @@ QemuOpts *qemu_get_machine_opts(void);
 
 bool usb_enabled(bool default_usb);
 
+extern QemuOptsList qemu_legacy_drive_opts;
+extern QemuOptsList qemu_common_drive_opts;
 extern QemuOptsList qemu_drive_opts;
 extern QemuOptsList qemu_chardev_opts;
 extern QemuOptsList qemu_device_opts;
diff --git a/util/qemu-config.c b/util/qemu-config.c
index a59568d..573c5ad 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -6,6 +6,7 @@
 #include "hw/qdev.h"
 #include "qapi/error.h"
 #include "qmp-commands.h"
+#include "sysemu/sysemu.h"
 
 static QemuOptsList *vm_config_groups[32];
 
@@ -77,19 +78,77 @@ static CommandLineParameterInfoList *query_option_descs(const QemuOptDesc *desc)
     return param_list;
 }
 
+static void delete_infolist_entry(CommandLineParameterInfoList *head,
+                                  CommandLineParameterInfoList *del_entry)
+{
+    CommandLineParameterInfoList *cur;
+
+    cur = head;
+    while (cur->next) {
+        if (cur->next == del_entry) {
+            cur->next = del_entry->next;
+            g_free(del_entry);
+        }
+        cur = cur->next;
+    }
+}
+
+static void cleanup_infolist(CommandLineParameterInfoList *head)
+{
+    CommandLineParameterInfoList *pre_entry, *cur;
+
+    cur = head->next;
+    while (cur) {
+        pre_entry = head;
+        while (pre_entry != cur) {
+            if (!strcmp(pre_entry->value->name, cur->value->name)) {
+                delete_infolist_entry(head, cur);
+                break;
+            }
+            pre_entry = pre_entry->next;
+        }
+        cur = cur->next;
+    }
+}
+
+static void connect_infolist(CommandLineParameterInfoList *head,
+                             CommandLineParameterInfoList *new)
+{
+    CommandLineParameterInfoList *cur;
+
+    cur = head;
+    while (cur->next) {
+        cur = cur->next;
+    }
+    cur->next = new;
+}
+
 CommandLineOptionInfoList *qmp_query_command_line_options(bool has_option,
                                                           const char *option,
                                                           Error **errp)
 {
     CommandLineOptionInfoList *conf_list = NULL, *entry;
     CommandLineOptionInfo *info;
+    CommandLineParameterInfoList *new;
     int i;
 
     for (i = 0; vm_config_groups[i] != NULL; i++) {
         if (!has_option || !strcmp(option, vm_config_groups[i]->name)) {
             info = g_malloc0(sizeof(*info));
             info->option = g_strdup(vm_config_groups[i]->name);
-            info->parameters = query_option_descs(vm_config_groups[i]->desc);
+            if (!strcmp("drive", vm_config_groups[i]->name)) {
+                info->parameters =
+                    query_option_descs(qemu_legacy_drive_opts.desc);
+                new = query_option_descs(qemu_common_drive_opts.desc);
+                connect_infolist(info->parameters, new);
+                new = query_option_descs(qemu_drive_opts.desc);
+                connect_infolist(info->parameters, new);
+
+                cleanup_infolist(info->parameters);
+           } else {
+                info->parameters =
+                    query_option_descs(vm_config_groups[i]->desc);
+            }
             entry = g_malloc0(sizeof(*entry));
             entry->value = info;
             entry->next = conf_list;
-- 
1.8.3.1