[Qemu-devel] [PATCH 0/4][RFC] Add module infrastructure to QEMU

[Qemu-devel] [PATCH 0/4][RFC] Add module infrastructure to QEMU

[Anthony Liguori]

This is the current state of a patch set to introduce a module infrastructure to
QEMU.  It depends on GCC constructors and GNU ld's --whole-archive.

The dependency on GNU ld's --whole-archive is a problem, but it can be addressed
in a number of ways:

 1) Avoid creating a shared archive and just use object files.  I see no real
    downside to this except that we're just ignoring this problem.

 2) Switch to using dynamic shared libraries.  This has the benefit of reducing
    the QEMU install size.  This is attractive except for the fact that creating
    dynamic shared libraries across multiple host architectures is a pain.

 3) For platforms that don't use GNU ld (Solaris, AIX, ???), add platform
    specific equivalents.  For instance, -z allextract should work for Solaris.

I'm leaning toward #3 as I think it's a reasonable compromise.

This patch series introduces a number of other concepts though beyond
constructors.  It decentralizes the block driver code and moves it to a separate
subdirectory.  It also introduces also decentralizes the Makefile for the block
drivers.

Finally, it allows for a config file to be used during build time to
selectively choose what block drivers are compiled in.  This is done in a
fashion similar to how the Linux kernel's build system works.

There are a number of reasons to allow this level of build customization.  One
reason is that it makes per-platform support more understandable.  Instead of
having #ifdefs all through the code, you have a central place to disable
features for particular platforms.

It also has the advantage of allowing an individual to build a minimal version
of QEMU to support a particular type of machine with a particular set of
functionality.  This is useful for embedded environments and for security
sensitive environments.

I have additional patches that split up qemu-char.c and introduce a similar
structure there but they are not quite ready yet.

[Qemu-devel] [PATCH 1/4] Add module infrastructure to QEMU

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/Makefile b/Makefile
index 8649938..5c53801 100644
--- a/Makefile
+++ b/Makefile
@@ -63,7 +63,7 @@ recurse-all: $(SUBDIR_RULES)
 #######################################################################
 # BLOCK_OBJS is code used by both qemu system emulation and qemu-img
 
-BLOCK_OBJS=cutils.o cache-utils.o qemu-malloc.o
+BLOCK_OBJS=cutils.o cache-utils.o qemu-malloc.o module.o
 BLOCK_OBJS+=block-cow.o block-qcow.o aes.o block-vmdk.o block-cloop.o
 BLOCK_OBJS+=block-dmg.o block-bochs.o block-vpc.o block-vvfat.o
 BLOCK_OBJS+=block-qcow2.o block-parallels.o block-nbd.o
diff --git a/Makefile.target b/Makefile.target
index fe83837..c08c6fd 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -340,14 +340,13 @@ ifeq ($(TARGET_ARCH), m68k)
 OBJS+= m68k-sim.o m68k-semi.o
 endif
 
-OBJS+= libqemu.a
-
 # Note: this is a workaround. The real fix is to avoid compiling
 # cpu_signal_handler() in cpu-exec.c.
 signal.o: CFLAGS += $(HELPER_CFLAGS)
 
-$(QEMU_PROG): $(OBJS) ../libqemu_user.a
-	$(LINK)
+$(QEMU_PROG): ARLIBS=../libqemu_user.a libqemu.a
+$(QEMU_PROG): $(OBJS) ../libqemu_user.a libqemu.a
+	$(call LINK,$(OBJS))
 ifeq ($(ARCH),alpha)
 # Mark as 32 bit binary, i. e. it will be mapped into the low 31 bit of
 # the address space (31 bit so sign extending doesn't matter)
@@ -372,14 +371,13 @@ LIBS+=-lmx
 OBJS= main.o commpage.o machload.o mmap.o signal.o syscall.o thunk.o \
       gdbstub.o gdbstub-xml.o
 
-OBJS+= libqemu.a
-
 # Note: this is a workaround. The real fix is to avoid compiling
 # cpu_signal_handler() in cpu-exec.c.
 signal.o: CFLAGS += $(HELPER_CFLAGS)
 
-$(QEMU_PROG): $(OBJS)
-	$(LINK)
+$(QEMU_PROG): ARLIBS=libqemu.a
+$(QEMU_PROG): $(OBJS) libqemu.a
+	$(call LINK,$(OBJS))
 
 endif #CONFIG_DARWIN_USER
 
@@ -473,14 +471,13 @@ OBJS= main.o bsdload.o elfload.o mmap.o path.o signal.o strace.o syscall.o \
       gdbstub.o gdbstub-xml.o
 OBJS+= uaccess.o
 
-OBJS+= libqemu.a
-
 # Note: this is a workaround. The real fix is to avoid compiling
 # cpu_signal_handler() in cpu-exec.c.
 signal.o: CFLAGS += $(HELPER_CFLAGS)
 
-$(QEMU_PROG): $(OBJS) ../libqemu_user.a
-	$(LINK)
+$(QEMU_PROG): ARLIBS=libqemu.a ../libqemu_user.a
+$(QEMU_PROG): $(OBJS) libqemu.a ../libqemu_user.a
+	$(call LINK,$(OBJS))
 
 endif #CONFIG_BSD_USER
 
@@ -497,14 +494,6 @@ OBJS+=fw_cfg.o
 ifdef CONFIG_KVM
 OBJS+=kvm.o kvm-all.o
 endif
-ifdef CONFIG_WIN32
-OBJS+=block-raw-win32.o
-else
-ifdef CONFIG_AIO
-OBJS+=posix-aio-compat.o
-endif
-OBJS+=block-raw-posix.o
-endif
 
 LIBS+=-lz
 ifdef CONFIG_ALSA
@@ -724,9 +713,9 @@ endif
 vl.o: qemu-options.h
 
 $(QEMU_PROG): LIBS += $(SDL_LIBS) $(COCOA_LIBS) $(CURSES_LIBS) $(BRLAPI_LIBS) $(VDE_LIBS)
-
+$(QEMU_PROG): ARLIBS=../libqemu_common.a libqemu.a
 $(QEMU_PROG): $(OBJS) ../libqemu_common.a libqemu.a
-	$(LINK)
+	$(call LINK,$(OBJS))
 
 endif # !CONFIG_USER_ONLY
 
diff --git a/module.c b/module.c
new file mode 100644
index 0000000..2db2dad
--- /dev/null
+++ b/module.c
@@ -0,0 +1,137 @@
+/*
+ * QEMU Module Infrastructure
+ *
+ * Copyright IBM, Corp. 2009
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu-common.h"
+#include "sys-queue.h"
+#include "module.h"
+
+typedef struct ModuleEntry
+{
+    int priority;
+    int is_init;
+    union {
+        int (*init)(void);
+        void (*exit)(void);
+    };
+    TAILQ_ENTRY(ModuleEntry) node;
+} ModuleEntry;
+
+typedef struct ModulePriorityList
+{
+    int priority;
+    TAILQ_HEAD(, ModuleEntry) entry_list;
+    TAILQ_ENTRY(ModulePriorityList) node;
+} ModulePriorityList;
+
+static TAILQ_HEAD(, ModulePriorityList) priority_list;
+
+static ModulePriorityList *find_priority_or_alloc(int priority, int alloc)
+{
+    ModulePriorityList *n;
+
+    TAILQ_FOREACH(n, &priority_list, node) {
+        if (priority >= n->priority)
+            break;
+    }
+
+    if (!n || n->priority != priority) {
+        ModulePriorityList *o;
+
+        if (!alloc)
+            return NULL;
+
+        o = qemu_mallocz(sizeof(*o));
+        o->priority = priority;
+        TAILQ_INIT(&o->entry_list);
+
+        if (n) {
+            TAILQ_INSERT_AFTER(&priority_list, n, o, node);
+        } else {
+            TAILQ_INSERT_HEAD(&priority_list, o, node);
+        }
+
+        n = o;
+    }
+
+    return n;
+}
+
+void register_module_init(int (*fn)(void), int priority)
+{
+    ModuleEntry *e;
+    ModulePriorityList *l;
+
+    e = qemu_mallocz(sizeof(*e));
+    e->is_init = 1;
+    e->init = fn;
+
+    l = find_priority_or_alloc(priority, 1);
+
+    TAILQ_INSERT_TAIL(&l->entry_list, e, node);
+}
+
+void register_module_exit(void (*fn)(void), int priority)
+{
+    ModuleEntry *e;
+    ModulePriorityList *l;
+
+    e = qemu_mallocz(sizeof(*e));
+    e->is_init = 0;
+    e->exit = fn;
+
+    l = find_priority_or_alloc(priority, 1);
+
+    TAILQ_INSERT_TAIL(&l->entry_list, e, node);
+}
+
+int module_call_init(int priority)
+{
+    ModulePriorityList *l;
+    ModuleEntry *e;
+
+    l = find_priority_or_alloc(priority, 0);
+    if (!l) {
+        return 0;
+    }
+
+    TAILQ_FOREACH(e, &l->entry_list, node) {
+        int ret;
+
+        if (!e->is_init) {
+            continue;
+        }
+
+        ret = e->init();
+        if (ret != 0)
+            return ret;
+    }
+
+    return 0;
+}
+
+void module_call_exit(int priority)
+{
+    ModulePriorityList *l;
+    ModuleEntry *e;
+
+    l = find_priority_or_alloc(priority, 0);
+    if (!l) {
+        return;
+    }
+
+    TAILQ_FOREACH(e, &l->entry_list, node) {
+        if (!e->is_init) {
+            e->exit();
+        }
+    }
+}
diff --git a/module.h b/module.h
new file mode 100644
index 0000000..5aa3eaa
--- /dev/null
+++ b/module.h
@@ -0,0 +1,41 @@
+/*
+ * QEMU Module Infrastructure
+ *
+ * Copyright IBM, Corp. 2009
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2.  See
+ * the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef QEMU_MODULE_H
+#define QEMU_MODULE_H
+
+#define module_init(function, priority)                                  \
+static void __attribute__((constructor)) qemu_init_ ## function(void) {  \
+   register_module_init(function, priority);                             \
+}
+
+#define module_exit(function, priority)                                  \
+static void __attribute__((constructor)) qemu_exit_ ## function(void) {  \
+   register_module_exit(function, priority);                             \
+}
+
+#define MOD_PRI_HIGHEST    0
+#define MOD_PRI_BLOCK      (MOD_PRI_HIGHEST + 1)
+
+#define block_init(function) module_init(function, MOD_PRI_BLOCK)
+#define block_exit(function) module_exit(function, MOD_PRI_BLOCK)
+
+void register_module_init(int (*fn)(void), int priority);
+
+void register_module_exit(void (*fn)(void), int priority);
+
+int module_call_init(int priority);
+
+void module_call_exit(int priority);
+
+#endif
diff --git a/rules.mak b/rules.mak
index a75a93b..8471d40 100644
--- a/rules.mak
+++ b/rules.mak
@@ -8,10 +8,13 @@
 %.o: %.m
 	$(call quiet-command,$(CC) $(CFLAGS) $(CPPFLAGS) -c -o $@ $<,"  OBJC  $(TARGET_DIR)$@")
 
-LINK = $(call quiet-command,$(CC) $(LDFLAGS) -o $@ $^ $(LIBS),"  LINK  $(TARGET_DIR)$@")
+WAS=-Wl,--whole-archive
+WAE=-Wl,--no-whole-archive
+
+LINK = $(call quiet-command,$(CC) $(LDFLAGS) -o $@ $(1) $(LIBS) $(WAS) $(ARLIBS) $(WAE),"  LINK  $(TARGET_DIR)$@")
 
 %$(EXESUF): %.o
-	$(LINK)
+	$(call LINK,$^)
 
 %.a:
 	$(call quiet-command,rm -f $@ && $(AR) rcs $@ $^,"  AR    $(TARGET_DIR)$@")
-- 
1.6.0.6

[Qemu-devel] [PATCH 2/4] Convert block infrastructure to use new module init functionality

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/block-bochs.c b/block-bochs.c
index 7a75412..5f7db83 100644
--- a/block-bochs.c
+++ b/block-bochs.c
@@ -24,6 +24,7 @@
  */
 #include "qemu-common.h"
 #include "block_int.h"
+#include "module.h"
 
 /**************************************************************/
 
@@ -241,7 +242,7 @@ static void bochs_close(BlockDriverState *bs)
     close(s->fd);
 }
 
-BlockDriver bdrv_bochs = {
+static BlockDriver bdrv_bochs = {
     .format_name	= "bochs",
     .instance_size	= sizeof(BDRVBochsState),
     .bdrv_probe		= bochs_probe,
@@ -249,3 +250,11 @@ BlockDriver bdrv_bochs = {
     .bdrv_read		= bochs_read,
     .bdrv_close		= bochs_close,
 };
+
+static int bdrv_bochs_init(void)
+{
+    bdrv_register(&bdrv_bochs);
+    return 0;
+}
+
+block_init(bdrv_bochs_init);
diff --git a/block-cloop.c b/block-cloop.c
index 9414d10..8af9773 100644
--- a/block-cloop.c
+++ b/block-cloop.c
@@ -23,6 +23,7 @@
  */
 #include "qemu-common.h"
 #include "block_int.h"
+#include "module.h"
 #include <zlib.h>
 
 typedef struct BDRVCloopState {
@@ -153,7 +154,7 @@ static void cloop_close(BlockDriverState *bs)
     inflateEnd(&s->zstream);
 }
 
-BlockDriver bdrv_cloop = {
+static BlockDriver bdrv_cloop = {
     .format_name	= "cloop",
     .instance_size	= sizeof(BDRVCloopState),
     .bdrv_probe		= cloop_probe,
@@ -161,3 +162,11 @@ BlockDriver bdrv_cloop = {
     .bdrv_read		= cloop_read,
     .bdrv_close		= cloop_close,
 };
+
+static int bdrv_cloop_init(void)
+{
+    bdrv_register(&bdrv_cloop);
+    return 0;
+}
+
+block_init(bdrv_cloop_init);
diff --git a/block-cow.c b/block-cow.c
index 17e3292..0a49491 100644
--- a/block-cow.c
+++ b/block-cow.c
@@ -24,6 +24,7 @@
 #ifndef _WIN32
 #include "qemu-common.h"
 #include "block_int.h"
+#include "module.h"
 #include <sys/mman.h>
 
 /**************************************************************/
@@ -252,7 +253,7 @@ static void cow_flush(BlockDriverState *bs)
     fsync(s->fd);
 }
 
-BlockDriver bdrv_cow = {
+static BlockDriver bdrv_cow = {
     .format_name	= "cow",
     .instance_size	= sizeof(BDRVCowState),
     .bdrv_probe		= cow_probe,
@@ -264,4 +265,12 @@ BlockDriver bdrv_cow = {
     .bdrv_flush		= cow_flush,
     .bdrv_is_allocated	= cow_is_allocated,
 };
+
+static int bdrv_cow_init(void)
+{
+    bdrv_register(&bdrv_cow);
+    return 0;
+}
+
+block_init(bdrv_cow_init);
 #endif
diff --git a/block-dmg.c b/block-dmg.c
index 82f6de1..6a78965 100644
--- a/block-dmg.c
+++ b/block-dmg.c
@@ -24,6 +24,7 @@
 #include "qemu-common.h"
 #include "block_int.h"
 #include "bswap.h"
+#include "module.h"
 #include <zlib.h>
 
 typedef struct BDRVDMGState {
@@ -92,7 +93,7 @@ static int dmg_open(BlockDriverState *bs, const char *filename, int flags)
 dmg_close:
 	close(s->fd);
 	/* open raw instead */
-	bs->drv=&bdrv_raw;
+	bs->drv=bdrv_find_format("raw");
 	return bs->drv->bdrv_open(bs, filename, flags);
     }
     info_begin=read_off(s->fd);
@@ -283,7 +284,7 @@ static void dmg_close(BlockDriverState *bs)
     inflateEnd(&s->zstream);
 }
 
-BlockDriver bdrv_dmg = {
+static BlockDriver bdrv_dmg = {
     .format_name	= "dmg",
     .instance_size	= sizeof(BDRVDMGState),
     .bdrv_probe		= dmg_probe,
@@ -291,3 +292,11 @@ BlockDriver bdrv_dmg = {
     .bdrv_read		= dmg_read,
     .bdrv_close		= dmg_close,
 };
+
+static int bdrv_dmg_init(void)
+{
+    bdrv_register(&bdrv_dmg);
+    return 0;
+}
+
+block_init(bdrv_dmg_init);
diff --git a/block-nbd.c b/block-nbd.c
index 632cb2d..59ceb4d 100644
--- a/block-nbd.c
+++ b/block-nbd.c
@@ -28,6 +28,7 @@
 
 #include "qemu-common.h"
 #include "nbd.h"
+#include "module.h"
 
 #include <sys/types.h>
 #include <unistd.h>
@@ -176,7 +177,7 @@ static int64_t nbd_getlength(BlockDriverState *bs)
     return s->size;
 }
 
-BlockDriver bdrv_nbd = {
+static BlockDriver bdrv_nbd = {
     .format_name	= "nbd",
     .instance_size	= sizeof(BDRVNBDState),
     .bdrv_open		= nbd_open,
@@ -186,3 +187,11 @@ BlockDriver bdrv_nbd = {
     .bdrv_getlength	= nbd_getlength,
     .protocol_name	= "nbd",
 };
+
+static int bdrv_nbd_init(void)
+{
+    bdrv_register(&bdrv_nbd);
+    return 0;
+}
+
+block_init(bdrv_nbd_init);
diff --git a/block-parallels.c b/block-parallels.c
index 18c3d83..3f46396 100644
--- a/block-parallels.c
+++ b/block-parallels.c
@@ -25,6 +25,7 @@
  */
 #include "qemu-common.h"
 #include "block_int.h"
+#include "module.h"
 
 /**************************************************************/
 
@@ -163,7 +164,7 @@ static void parallels_close(BlockDriverState *bs)
     close(s->fd);
 }
 
-BlockDriver bdrv_parallels = {
+static BlockDriver bdrv_parallels = {
     .format_name	= "parallels",
     .instance_size	= sizeof(BDRVParallelsState),
     .bdrv_probe		= parallels_probe,
@@ -171,3 +172,11 @@ BlockDriver bdrv_parallels = {
     .bdrv_read		= parallels_read,
     .bdrv_close		= parallels_close,
 };
+
+static int bdrv_parallels_init(void)
+{
+    bdrv_register(&bdrv_parallels);
+    return 0;
+}
+
+block_init(bdrv_parallels_init);
diff --git a/block-qcow.c b/block-qcow.c
index fc6b809..cc25f69 100644
--- a/block-qcow.c
+++ b/block-qcow.c
@@ -23,6 +23,7 @@
  */
 #include "qemu-common.h"
 #include "block_int.h"
+#include "module.h"
 #include <zlib.h>
 #include "aes.h"
 
@@ -917,7 +918,7 @@ static int qcow_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
     return 0;
 }
 
-BlockDriver bdrv_qcow = {
+static BlockDriver bdrv_qcow = {
     .format_name	= "qcow",
     .instance_size	= sizeof(BDRVQcowState),
     .bdrv_probe		= qcow_probe,
@@ -935,3 +936,11 @@ BlockDriver bdrv_qcow = {
     .bdrv_write_compressed = qcow_write_compressed,
     .bdrv_get_info	= qcow_get_info,
 };
+
+static int bdrv_qcow_init(void)
+{
+    bdrv_register(&bdrv_qcow);
+    return 0;
+}
+
+block_init(bdrv_qcow_init);
diff --git a/block-qcow2.c b/block-qcow2.c
index 9a49777..d8e5da3 100644
--- a/block-qcow2.c
+++ b/block-qcow2.c
@@ -23,6 +23,7 @@
  */
 #include "qemu-common.h"
 #include "block_int.h"
+#include "module.h"
 #include <zlib.h>
 #include "aes.h"
 #include <assert.h>
@@ -2892,7 +2893,7 @@ static int qcow_get_buffer(BlockDriverState *bs, uint8_t *buf,
     return ret;
 }
 
-BlockDriver bdrv_qcow2 = {
+static BlockDriver bdrv_qcow2 = {
     .format_name	= "qcow2",
     .instance_size	= sizeof(BDRVQcowState),
     .bdrv_probe		= qcow_probe,
@@ -2922,3 +2923,11 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_create2 = qcow_create2,
     .bdrv_check = qcow_check,
 };
+
+static int bdrv_qcow2_init(void)
+{
+    bdrv_register(&bdrv_qcow2);
+    return 0;
+}
+
+block_init(bdrv_qcow2_init);
diff --git a/block-raw-posix.c b/block-raw-posix.c
index 0663c06..74a64fc 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -25,6 +25,7 @@
 #include "qemu-timer.h"
 #include "qemu-char.h"
 #include "block_int.h"
+#include "module.h"
 #include <assert.h>
 #ifdef CONFIG_AIO
 #include "posix-aio-compat.h"
@@ -846,7 +847,7 @@ static void raw_flush(BlockDriverState *bs)
     fsync(s->fd);
 }
 
-BlockDriver bdrv_raw = {
+static BlockDriver bdrv_raw = {
     .format_name = "raw",
     .instance_size = sizeof(BDRVRawState),
     .bdrv_probe = NULL, /* no probe for protocols */
@@ -1398,7 +1399,7 @@ static int hdev_create(const char *filename, int64_t total_size,
 }
 #endif
 
-BlockDriver bdrv_host_device = {
+static BlockDriver bdrv_host_device = {
     .format_name	= "host_device",
     .instance_size	= sizeof(BDRVRawState),
     .bdrv_open		= hdev_open,
@@ -1428,3 +1429,12 @@ BlockDriver bdrv_host_device = {
     .bdrv_aio_ioctl	= raw_aio_ioctl,
 #endif
 };
+
+static int bdrv_raw_init(void)
+{
+    bdrv_register(&bdrv_raw);
+    bdrv_register(&bdrv_host_device);
+    return 0;
+}
+
+block_init(bdrv_raw_init);
diff --git a/block-raw-win32.c b/block-raw-win32.c
index b5287d2..e24adad 100644
--- a/block-raw-win32.c
+++ b/block-raw-win32.c
@@ -24,6 +24,7 @@
 #include "qemu-common.h"
 #include "qemu-timer.h"
 #include "block_int.h"
+#include "module.h"
 #include <assert.h>
 #include <windows.h>
 #include <winioctl.h>
@@ -228,7 +229,7 @@ static int raw_create(const char *filename, int64_t total_size,
     return 0;
 }
 
-BlockDriver bdrv_raw = {
+static BlockDriver bdrv_raw = {
     .format_name	= "raw",
     .instance_size	= sizeof(BDRVRawState),
     .bdrv_open		= raw_open,
@@ -372,7 +373,7 @@ static int raw_set_locked(BlockDriverState *bs, int locked)
 }
 #endif
 
-BlockDriver bdrv_host_device = {
+static BlockDriver bdrv_host_device = {
     .format_name	= "host_device",
     .instance_size	= sizeof(BDRVRawState),
     .bdrv_open		= hdev_open,
@@ -383,3 +384,11 @@ BlockDriver bdrv_host_device = {
     .bdrv_write	        = raw_write,
     .bdrv_getlength	= raw_getlength,
 };
+
+static int bdrv_raw_init(void)
+{
+    bdrv_register(&bdrv_raw);
+    bdrv_register(&bdrv_host_device);
+}
+
+block_init(bdrv_raw_init);
diff --git a/block-vmdk.c b/block-vmdk.c
index d47d483..35aca07 100644
--- a/block-vmdk.c
+++ b/block-vmdk.c
@@ -25,6 +25,7 @@
 
 #include "qemu-common.h"
 #include "block_int.h"
+#include "module.h"
 
 #define VMDK3_MAGIC (('C' << 24) | ('O' << 16) | ('W' << 8) | 'D')
 #define VMDK4_MAGIC (('K' << 24) | ('D' << 16) | ('M' << 8) | 'V')
@@ -811,7 +812,7 @@ static void vmdk_flush(BlockDriverState *bs)
     bdrv_flush(s->hd);
 }
 
-BlockDriver bdrv_vmdk = {
+static BlockDriver bdrv_vmdk = {
     .format_name	= "vmdk",
     .instance_size	= sizeof(BDRVVmdkState),
     .bdrv_probe		= vmdk_probe,
@@ -823,3 +824,11 @@ BlockDriver bdrv_vmdk = {
     .bdrv_flush		= vmdk_flush,
     .bdrv_is_allocated	= vmdk_is_allocated,
 };
+
+static int bdrv_vmdk_init(void)
+{
+    bdrv_register(&bdrv_vmdk);
+    return 0;
+}
+
+block_init(bdrv_vmdk_init);
diff --git a/block-vpc.c b/block-vpc.c
index 71a171d..0bbda1e 100644
--- a/block-vpc.c
+++ b/block-vpc.c
@@ -24,6 +24,7 @@
  */
 #include "qemu-common.h"
 #include "block_int.h"
+#include "module.h"
 
 /**************************************************************/
 
@@ -586,7 +587,7 @@ static void vpc_close(BlockDriverState *bs)
     bdrv_delete(s->hd);
 }
 
-BlockDriver bdrv_vpc = {
+static BlockDriver bdrv_vpc = {
     .format_name	= "vpc",
     .instance_size	= sizeof(BDRVVPCState),
     .bdrv_probe		= vpc_probe,
@@ -596,3 +597,11 @@ BlockDriver bdrv_vpc = {
     .bdrv_close		= vpc_close,
     .bdrv_create	= vpc_create,
 };
+
+static int bdrv_vpc_init(void)
+{
+    bdrv_register(&bdrv_vpc);
+    return 0;
+}
+
+block_init(bdrv_vpc_init);
diff --git a/block-vvfat.c b/block-vvfat.c
index 7905931..0a151f1 100644
--- a/block-vvfat.c
+++ b/block-vvfat.c
@@ -27,6 +27,7 @@
 #include <assert.h>
 #include "qemu-common.h"
 #include "block_int.h"
+#include "module.h"
 
 #ifndef S_IWGRP
 #define S_IWGRP 0
@@ -2777,7 +2778,7 @@ static int enable_write_target(BDRVVVFATState *s)
 
     s->qcow_filename = qemu_malloc(1024);
     get_tmp_filename(s->qcow_filename, 1024);
-    if (bdrv_create(&bdrv_qcow,
+    if (bdrv_create(bdrv_find_format("qcow"),
 		s->qcow_filename, s->sector_count, "fat:", 0) < 0)
 	return -1;
     s->qcow = bdrv_new("");
@@ -2807,7 +2808,7 @@ static void vvfat_close(BlockDriverState *bs)
         free(s->cluster_buffer);
 }
 
-BlockDriver bdrv_vvfat = {
+static BlockDriver bdrv_vvfat = {
     .format_name	= "vvfat",
     .instance_size	= sizeof(BDRVVVFATState),
     .bdrv_open		= vvfat_open,
@@ -2818,6 +2819,14 @@ BlockDriver bdrv_vvfat = {
     .protocol_name	= "fat",
 };
 
+static int bdrv_vvfat_init(void)
+{
+    bdrv_register(&bdrv_vvfat);
+    return 0;
+}
+
+block_init(bdrv_vvfat_init);
+
 #ifdef DEBUG
 static void checkpoint(void) {
     assert(((mapping_t*)array_get(&(vvv->mapping), 0))->end == 2);
diff --git a/block.c b/block.c
index acb8976..0dd7436 100644
--- a/block.c
+++ b/block.c
@@ -30,6 +30,7 @@
 #include "qemu-common.h"
 #include "monitor.h"
 #include "block_int.h"
+#include "module.h"
 
 #ifdef HOST_BSD
 #include <sys/types.h>
@@ -138,7 +139,7 @@ void path_combine(char *dest, int dest_size,
 }
 
 
-static void bdrv_register(BlockDriver *bdrv)
+void bdrv_register(BlockDriver *bdrv)
 {
     if (!bdrv->bdrv_aio_readv) {
         /* add AIO emulation layer */
@@ -259,11 +260,11 @@ static BlockDriver *find_protocol(const char *filename)
 #ifdef _WIN32
     if (is_windows_drive(filename) ||
         is_windows_drive_prefix(filename))
-        return &bdrv_raw;
+        return bdrv_find_format("raw");
 #endif
     p = strchr(filename, ':');
     if (!p)
-        return &bdrv_raw;
+        return bdrv_find_format("raw");
     len = p - filename;
     if (len > sizeof(protocol) - 1)
         len = sizeof(protocol) - 1;
@@ -289,23 +290,23 @@ static BlockDriver *find_image_format(const char *filename)
     /* detect host devices. By convention, /dev/cdrom[N] is always
        recognized as a host CDROM */
     if (strstart(filename, "/dev/cdrom", NULL))
-        return &bdrv_host_device;
+        return bdrv_find_format("host_device");
 #ifdef _WIN32
     if (is_windows_drive(filename))
-        return &bdrv_host_device;
+        return bdrv_find_format("host_device");
 #else
     {
         struct stat st;
         if (stat(filename, &st) >= 0 &&
             (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode))) {
-            return &bdrv_host_device;
+            return bdrv_find_format("host_device");
         }
     }
 #endif
 
     drv = find_protocol(filename);
     /* no need to test disk image formats for vvfat */
-    if (drv == &bdrv_vvfat)
+    if (strcmp(drv->format_name, "vvfat") == 0)
         return drv;
 
     ret = bdrv_file_open(&bs, filename, BDRV_O_RDONLY);
@@ -396,14 +397,14 @@ int bdrv_open2(BlockDriverState *bs, const char *filename, int flags,
         else
             realpath(filename, backing_filename);
 
-        ret = bdrv_create2(&bdrv_qcow2, tmp_filename,
+        ret = bdrv_create2(bdrv_find_format("qcow2"), tmp_filename,
                            total_size, backing_filename, 
                            (drv ? drv->format_name : NULL), 0);
         if (ret < 0) {
             return ret;
         }
         filename = tmp_filename;
-        drv = &bdrv_qcow2;
+        drv = bdrv_find_format("qcow2");
         bs->is_temporary = 1;
     }
 
@@ -1494,21 +1495,10 @@ static int bdrv_write_em(BlockDriverState *bs, int64_t sector_num,
 
 void bdrv_init(void)
 {
-    bdrv_register(&bdrv_raw);
-    bdrv_register(&bdrv_host_device);
-#ifndef _WIN32
-    bdrv_register(&bdrv_cow);
-#endif
-    bdrv_register(&bdrv_qcow);
-    bdrv_register(&bdrv_vmdk);
-    bdrv_register(&bdrv_cloop);
-    bdrv_register(&bdrv_dmg);
-    bdrv_register(&bdrv_bochs);
-    bdrv_register(&bdrv_vpc);
-    bdrv_register(&bdrv_vvfat);
-    bdrv_register(&bdrv_qcow2);
-    bdrv_register(&bdrv_parallels);
-    bdrv_register(&bdrv_nbd);
+    if (module_call_init(MOD_PRI_BLOCK) != 0) {
+        fprintf(stderr, "Failed to load one or more block drivers\n");
+        exit(1);
+    }
 }
 
 void aio_pool_init(AIOPool *pool, int aiocb_size,
diff --git a/block.h b/block.h
index 5aef076..22df8ca 100644
--- a/block.h
+++ b/block.h
@@ -7,20 +7,6 @@
 /* block.c */
 typedef struct BlockDriver BlockDriver;
 
-extern BlockDriver bdrv_raw;
-extern BlockDriver bdrv_host_device;
-extern BlockDriver bdrv_cow;
-extern BlockDriver bdrv_qcow;
-extern BlockDriver bdrv_vmdk;
-extern BlockDriver bdrv_cloop;
-extern BlockDriver bdrv_dmg;
-extern BlockDriver bdrv_bochs;
-extern BlockDriver bdrv_vpc;
-extern BlockDriver bdrv_vvfat;
-extern BlockDriver bdrv_qcow2;
-extern BlockDriver bdrv_parallels;
-extern BlockDriver bdrv_nbd;
-
 typedef struct BlockDriverInfo {
     /* in bytes, 0 if irrelevant */
     int cluster_size;
@@ -87,6 +73,8 @@ int64_t bdrv_getlength(BlockDriverState *bs);
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
 void bdrv_guess_geometry(BlockDriverState *bs, int *pcyls, int *pheads, int *psecs);
 int bdrv_commit(BlockDriverState *bs);
+void bdrv_register(BlockDriver *bdrv);
+
 /* async block I/O */
 typedef struct BlockDriverAIOCB BlockDriverAIOCB;
 typedef void BlockDriverCompletionFunc(void *opaque, int ret);
diff --git a/qemu-img.c b/qemu-img.c
index 29149a2..018d32b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -543,11 +543,11 @@ static int img_convert(int argc, char **argv)
     drv = bdrv_find_format(out_fmt);
     if (!drv)
         error("Unknown file format '%s'", out_fmt);
-    if (flags & BLOCK_FLAG_COMPRESS && drv != &bdrv_qcow && drv != &bdrv_qcow2)
+    if (flags & BLOCK_FLAG_COMPRESS && strcmp(drv->format_name, "qcow") && strcmp(drv->format_name, "qcow2"))
         error("Compression not supported for this file format");
-    if (flags & BLOCK_FLAG_ENCRYPT && drv != &bdrv_qcow && drv != &bdrv_qcow2)
+    if (flags & BLOCK_FLAG_ENCRYPT && strcmp(drv->format_name, "qcow") && strcmp(drv->format_name, "qcow2"))
         error("Encryption not supported for this file format");
-    if (flags & BLOCK_FLAG_COMPAT6 && drv != &bdrv_vmdk)
+    if (flags & BLOCK_FLAG_COMPAT6 && strcmp(drv->format_name, "vmdk"))
         error("Alternative compatibility level not supported for this file format");
     if (flags & BLOCK_FLAG_ENCRYPT && flags & BLOCK_FLAG_COMPRESS)
         error("Compression and encryption not supported at the same time");
@@ -656,7 +656,7 @@ static int img_convert(int argc, char **argv)
             if (n > bs_offset + bs_sectors - sector_num)
                 n = bs_offset + bs_sectors - sector_num;
 
-            if (drv != &bdrv_host_device) {
+            if (strcmp(drv->format_name, "host_device")) {
                 if (!bdrv_is_allocated(bs[bs_i], sector_num - bs_offset,
                                        n, &n1)) {
                     sector_num += n1;
@@ -683,7 +683,7 @@ static int img_convert(int argc, char **argv)
                    If the output is to a host device, we also write out
                    sectors that are entirely 0, since whatever data was
                    already there is garbage, not 0s. */
-                if (drv == &bdrv_host_device || out_baseimg ||
+                if (strcmp(drv->format_name, "host_device") == 0 || out_baseimg ||
                     is_allocated_sectors(buf1, n, &n1)) {
                     if (bdrv_write(out_bs, sector_num, buf1, n1) < 0)
                         error("error while writing");
-- 
1.6.0.6

[Qemu-devel] [PATCH 3/4] Move block drivers into their own directory

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/Makefile b/Makefile
index 5c53801..24b8c0d 100644
--- a/Makefile
+++ b/Makefile
@@ -64,18 +64,18 @@ recurse-all: $(SUBDIR_RULES)
 # BLOCK_OBJS is code used by both qemu system emulation and qemu-img
 
 BLOCK_OBJS=cutils.o cache-utils.o qemu-malloc.o module.o
-BLOCK_OBJS+=block-cow.o block-qcow.o aes.o block-vmdk.o block-cloop.o
-BLOCK_OBJS+=block-dmg.o block-bochs.o block-vpc.o block-vvfat.o
-BLOCK_OBJS+=block-qcow2.o block-parallels.o block-nbd.o
+BLOCK_OBJS+=block/cow.o block/qcow.o aes.o block/vmdk.o block/cloop.o
+BLOCK_OBJS+=block/dmg.o block/bochs.o block/vpc.o block/vvfat.o
+BLOCK_OBJS+=block/qcow2.o block/parallels.o block/nbd.o
 BLOCK_OBJS+=nbd.o block.o aio.o
 
 ifdef CONFIG_WIN32
-BLOCK_OBJS += block-raw-win32.o
+BLOCK_OBJS += block/raw-win32.o
 else
 ifdef CONFIG_AIO
 BLOCK_OBJS += posix-aio-compat.o
 endif
-BLOCK_OBJS += block-raw-posix.o
+BLOCK_OBJS += block/raw-posix.o
 endif
 
 ######################################################################
@@ -234,7 +234,7 @@ clean:
 # avoid old build problems by removing potentially incorrect old files
 	rm -f config.mak config.h op-i386.h opc-i386.h gen-op-i386.h op-arm.h opc-arm.h gen-op-arm.h
 	rm -f *.o *.d *.a $(TOOLS) TAGS cscope.* *.pod *~ */*~
-	rm -f slirp/*.o slirp/*.d audio/*.o audio/*.d
+	rm -f slirp/*.o slirp/*.d audio/*.o audio/*.d block/*.o block/*.d
 	$(MAKE) -C tests clean
 	for d in $(TARGET_DIRS); do \
 	$(MAKE) -C $$d $@ || exit 1 ; \
@@ -408,4 +408,4 @@ tarbin:
 	$(mandir)/man8/qemu-nbd.8
 
 # Include automatically generated dependency files
--include $(wildcard *.d audio/*.d slirp/*.d)
+-include $(wildcard *.d audio/*.d slirp/*.d block/*.d)
diff --git a/block-bochs.c b/block/bochs.c
similarity index 100%
rename from block-bochs.c
rename to block/bochs.c
diff --git a/block-cloop.c b/block/cloop.c
similarity index 100%
rename from block-cloop.c
rename to block/cloop.c
diff --git a/block-cow.c b/block/cow.c
similarity index 100%
rename from block-cow.c
rename to block/cow.c
diff --git a/block-dmg.c b/block/dmg.c
similarity index 100%
rename from block-dmg.c
rename to block/dmg.c
diff --git a/block-nbd.c b/block/nbd.c
similarity index 100%
rename from block-nbd.c
rename to block/nbd.c
diff --git a/block-parallels.c b/block/parallels.c
similarity index 100%
rename from block-parallels.c
rename to block/parallels.c
diff --git a/block-qcow.c b/block/qcow.c
similarity index 100%
rename from block-qcow.c
rename to block/qcow.c
diff --git a/block-qcow2.c b/block/qcow2.c
similarity index 100%
rename from block-qcow2.c
rename to block/qcow2.c
diff --git a/block-raw-posix.c b/block/raw-posix.c
similarity index 100%
rename from block-raw-posix.c
rename to block/raw-posix.c
diff --git a/block-raw-win32.c b/block/raw-win32.c
similarity index 100%
rename from block-raw-win32.c
rename to block/raw-win32.c
diff --git a/block-vmdk.c b/block/vmdk.c
similarity index 100%
rename from block-vmdk.c
rename to block/vmdk.c
diff --git a/block-vpc.c b/block/vpc.c
similarity index 100%
rename from block-vpc.c
rename to block/vpc.c
diff --git a/block-vvfat.c b/block/vvfat.c
similarity index 100%
rename from block-vvfat.c
rename to block/vvfat.c
diff --git a/configure b/configure
index b2a2540..1bd7e76 100755
--- a/configure
+++ b/configure
@@ -2028,7 +2028,7 @@ done # for target in $targets
 
 # build tree in object directory if source path is different from current one
 if test "$source_path_used" = "yes" ; then
-    DIRS="tests tests/cris slirp audio"
+    DIRS="tests tests/cris slirp audio block"
     FILES="Makefile tests/Makefile"
     FILES="$FILES tests/cris/Makefile tests/cris/.gdbinit"
     FILES="$FILES tests/test-mmap.c"
-- 
1.6.0.6

[Qemu-devel] [PATCH 4/4] Introduce global .config to selectively enable compile features

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/Makefile b/Makefile
index 24b8c0d..845c965 100644
--- a/Makefile
+++ b/Makefile
@@ -41,7 +41,16 @@ ifdef CONFIG_WIN32
 LIBS+=-lwinmm -lws2_32 -liphlpapi
 endif
 
-all: $(TOOLS) $(DOCS) recurse-all
+ifdef CONFIG_WIN32
+DEFCONFIG=$(SRC_PATH)/configs/win32/defconfig
+else
+DEFCONFIG=$(SRC_PATH)/configs/unix/defconfig
+endif
+
+all: .config $(TOOLS) $(DOCS) recurse-all
+
+.config: $(DEFCONFIG)
+	$(call quiet-command,cp $< $@,)
 
 config-host.mak: configure
 ifneq ($(wildcard config-host.mak),)
@@ -60,22 +69,22 @@ $(filter %-user,$(SUBDIR_RULES)): libqemu_user.a
 
 recurse-all: $(SUBDIR_RULES)
 
+-include .config
+
 #######################################################################
 # BLOCK_OBJS is code used by both qemu system emulation and qemu-img
 
-BLOCK_OBJS=cutils.o cache-utils.o qemu-malloc.o module.o
-BLOCK_OBJS+=block/cow.o block/qcow.o aes.o block/vmdk.o block/cloop.o
-BLOCK_OBJS+=block/dmg.o block/bochs.o block/vpc.o block/vvfat.o
-BLOCK_OBJS+=block/qcow2.o block/parallels.o block/nbd.o
-BLOCK_OBJS+=nbd.o block.o aio.o
+BLOCK_OBJS=cutils.o cache-utils.o qemu-malloc.o module.o aes.o nbd.o block.o
+BLOCK_OBJS+=aio.o
 
-ifdef CONFIG_WIN32
-BLOCK_OBJS += block/raw-win32.o
-else
+obj-y=
+include $(SRC_PATH)/block/Makefile
+BLOCK_OBJS+=$(addprefix block/, $(obj-y))
+
+ifndef CONFIG_WIN32
 ifdef CONFIG_AIO
 BLOCK_OBJS += posix-aio-compat.o
 endif
-BLOCK_OBJS += block/raw-posix.o
 endif
 
 ######################################################################
diff --git a/block/Makefile b/block/Makefile
new file mode 100644
index 0000000..f33c81d
--- /dev/null
+++ b/block/Makefile
@@ -0,0 +1,13 @@
+obj-$(CONFIG_BDRV_RAW_POSIX)	+= raw-posix.o
+obj-$(CONFIG_BDRV_RAW_WIN32)	+= raw-win32.o
+obj-$(CONFIG_BDRV_BOCHS)	+= bochs.o
+obj-$(CONFIG_BDRV_CLOOP)	+= cloop.o
+obj-$(CONFIG_BDRV_COW)		+= cow.o
+obj-$(CONFIG_BDRV_DMG)		+= dmg.o
+obj-$(CONFIG_BDRV_NBD)		+= nbd.o
+obj-$(CONFIG_BDRV_PARALLELS)	+= parallels.o
+obj-$(CONFIG_BDRV_QCOW2)	+= qcow2.o
+obj-$(CONFIG_BDRV_QCOW)		+= qcow.o
+obj-$(CONFIG_BDRV_VMDK)		+= vmdk.o
+obj-$(CONFIG_BDRV_VPC)		+= vpc.o
+obj-$(CONFIG_BDRV_VVFAT)	+= vvfat.o
diff --git a/block/cow.c b/block/cow.c
index 0a49491..bac1f07 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -21,7 +21,6 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
-#ifndef _WIN32
 #include "qemu-common.h"
 #include "block_int.h"
 #include "module.h"
@@ -273,4 +272,3 @@ static int bdrv_cow_init(void)
 }
 
 block_init(bdrv_cow_init);
-#endif
diff --git a/configs/unix/defconfig b/configs/unix/defconfig
new file mode 100644
index 0000000..d897462
--- /dev/null
+++ b/configs/unix/defconfig
@@ -0,0 +1,13 @@
+CONFIG_BDRV_RAW_POSIX=y
+# CONFIG_BDRV_RAW_WIN32 is not set
+CONFIG_BDRV_BOCHS=y
+CONFIG_BDRV_CLOOP=y
+CONFIG_BDRV_COW=y
+CONFIG_BDRV_DMG=y
+CONFIG_BDRV_NBD=y
+CONFIG_BDRV_PARALLELS=y
+CONFIG_BDRV_QCOW2=y
+CONFIG_BDRV_QCOW=y
+CONFIG_BDRV_VMDK=y
+CONFIG_BDRV_VPC=y
+CONFIG_BDRV_VVFAT=y
diff --git a/configs/win32/defconfig b/configs/win32/defconfig
new file mode 100644
index 0000000..29ed483
--- /dev/null
+++ b/configs/win32/defconfig
@@ -0,0 +1,13 @@
+# CONFIG_BDRV_RAW_POSIX is not set
+CONFIG_BDRV_RAW_WIN32=y
+CONFIG_BDRV_BOCHS=y
+CONFIG_BDRV_CLOOP=y
+# CONFIG_BDRV_COW is not set
+CONFIG_BDRV_DMG=y
+CONFIG_BDRV_NBD=y
+CONFIG_BDRV_PARALLELS=y
+CONFIG_BDRV_QCOW2=y
+CONFIG_BDRV_QCOW=y
+CONFIG_BDRV_VMDK=y
+CONFIG_BDRV_VPC=y
+CONFIG_BDRV_VVFAT=y
-- 
1.6.0.6

[Qemu-devel] Re: [PATCH 0/4][RFC] Add module infrastructure to QEMU

[Paul Brook]

On Monday 11 May 2009, Anthony Liguori wrote:
> This is the current state of a patch set to introduce a module
> infrastructure to QEMU.

I don't think numeric priorities are a good idea. If we have dependencies then 
we should be dealing with them properly, not hacking round the problem.

Also, there's no reason to have destructors. The init function can register 
these at runtime.

>  2) Switch to using dynamic shared libraries.  This has the benefit of
> reducing the QEMU install size.  This is attractive except for the fact
> that creating dynamic shared libraries across multiple host architectures
> is a pain.

In practice I'd expect the shared library overhead (both disk and RAM) to be 
significantly larger than the saving form omitting a few devices. As you said 
before, if we're not using something, why build it in the first place?

There's also the issue that shared libraries imply it's OK for third parties 
to ship binary plugins.

Paul

Re: [Qemu-devel] Re: [PATCH 0/4][RFC] Add module infrastructure to QEMU

[Avi Kivity]

Paul Brook wrote:
> On Monday 11 May 2009, Anthony Liguori wrote:
>   
>> This is the current state of a patch set to introduce a module
>> infrastructure to QEMU.
>>     
>
> I don't think numeric priorities are a good idea. If we have dependencies then 
> we should be dealing with them properly, not hacking round the problem.
>
> Also, there's no reason to have destructors. The init function can register 
> these at runtime.
>   

Agree to both.

> There's also the issue that shared libraries imply it's OK for third parties 
> to ship binary plugins.
>   

Can't we add wording to LICENSE to address this?  I'd really like to 
allow GPL plugins.  Linux and now gcc allow this.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH 4/4] Introduce global .config to selectively enable compile features

[Avi Kivity]

Anthony Liguori wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> diff --git a/Makefile b/Makefile
> index 24b8c0d..845c965 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -41,7 +41,16 @@ ifdef CONFIG_WIN32
>  LIBS+=-lwinmm -lws2_32 -liphlpapi
>  endif
>  
> -all: $(TOOLS) $(DOCS) recurse-all
> +ifdef CONFIG_WIN32
> +DEFCONFIG=$(SRC_PATH)/configs/win32/defconfig
> +else
> +DEFCONFIG=$(SRC_PATH)/configs/unix/defconfig
> +endif
> +
> +all: .config $(TOOLS) $(DOCS) recurse-all
> +
> +.config: $(DEFCONFIG)
> +	$(call quiet-command,cp $< $@,)
>  
>   

Please let's not make this a hidden file.


-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 0/4][RFC] Add module infrastructure to QEMU

[Anthony Liguori]

Paul Brook wrote:
> On Monday 11 May 2009, Anthony Liguori wrote:
>   
>> This is the current state of a patch set to introduce a module
>> infrastructure to QEMU.
>>     
>
> I don't think numeric priorities are a good idea. If we have dependencies then 
> we should be dealing with them properly, not hacking round the problem.
>   

The numeric priorities are an implementation detail.  No one should ever 
consume module_init() directly and I should add appropriately scary 
comments to that affect.

The numeric priorities are currently used to implement a simple 
dependency tree with each level of the tree being an integer priority.

We do have dependencies.  I'd like virtio to be a module, and I'd like 
virtio-net, virtio-blk, etc. to be modules.  This gets exposed as 
virtio.c being a bus_init() and virtio-*.c being virtio_init().  We use 
the integer priorities in module.h to express this.

We could get fancier and have explicit dependencies by name or something 
like that.  I'm not sure I think it's worth getting this complex though.

> Also, there's no reason to have destructors. The init function can register 
> these at runtime.
>   

I don't feel strongly here.

The one case I think it could prove useful is that a number of things 
require atexit functions today.  I think it would be good to use 
destructors to make these more standardized.  Also, deregistering 
components has the advantage of making valgrind much happier.

We don't deregister the block drivers in this patch series, but I can 
certainly add it.

>>  2) Switch to using dynamic shared libraries.  This has the benefit of
>> reducing the QEMU install size.  This is attractive except for the fact
>> that creating dynamic shared libraries across multiple host architectures
>> is a pain.
>>     
>
> In practice I'd expect the shared library overhead (both disk and RAM) to be 
> significantly larger than the saving form omitting a few devices. As you said 
> before, if we're not using something, why build it in the first place?
>
> There's also the issue that shared libraries imply it's OK for third parties 
> to ship binary plugins.
>   

I'm strictly talking about libqemu_common.a and libqemu_user.a.  
However, I really don't want to do shared libraries in the absence of 
automake because shared libraries require a lot of per-platform foo to 
create.

I'm happy ruling this out.

Regards,

Anthony Liguori

> Paul
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 0/4][RFC] Add module infrastructure toQEMU

[Anthony Liguori]

Avi Kivity wrote:
>> There's also the issue that shared libraries imply it's OK for third 
>> parties to ship binary plugins.
>>   
>
> Can't we add wording to LICENSE to address this?  I'd really like to 
> allow GPL plugins.  Linux and now gcc allow this.

There's a lot of technical work to actually support shared libraries on 
all the platforms QEMU supports.  Even if we had that, and I expect a 
big flame war wrt reinventing autoconf here too, in order to 
realistically talk about plugins, we would need a much more modular 
infrastructure than we have today to allow a config file to specify a 
particular device without any core knowledge in QEMU of that device.

It's an admirable goal but we're far away from that.  When we eventually 
get there, we still have the issue of stable ABI (and Linux has this too).

So, I don't think this is a debate that we need to have now.  There's 
plenty of work to do before we get there :-)

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 0/4][RFC] Add module infrastructure to QEMU

[Paul Brook]

On Monday 11 May 2009, Anthony Liguori wrote:
> Paul Brook wrote:
> > On Monday 11 May 2009, Anthony Liguori wrote:
> >> This is the current state of a patch set to introduce a module
> >> infrastructure to QEMU.
> >
> > I don't think numeric priorities are a good idea. If we have dependencies
> > then we should be dealing with them properly, not hacking round the
> > problem.
>
> The numeric priorities are an implementation detail.  No one should ever
> consume module_init() directly and I should add appropriately scary
> comments to that affect.

Having looked a bit deeper, you're not actually implementing priorities. 
You're implementing different categories of init function. So this should be 
an enum, and be renames to something less confusing.

> We do have dependencies.  I'd like virtio to be a module, and I'd like
> virtio-net, virtio-blk, etc. to be modules.  This gets exposed as
> virtio.c being a bus_init() and virtio-*.c being virtio_init().  We use
> the integer priorities in module.h to express this.

I'm not convinced virtio is as much of a separate entity as you think it is. 
It's certainly not a bus. It's an implementation detail that happens to be 
shared by several devices.

Paul

[Qemu-devel] Re: [PATCH 0/4][RFC] Add module infrastructure to QEMU

[Anthony Liguori]

Paul Brook wrote:
> On Monday 11 May 2009, Anthony Liguori wrote:
>   
>> The numeric priorities are an implementation detail.  No one should ever
>> consume module_init() directly and I should add appropriately scary
>> comments to that affect.
>>     
>
> Having looked a bit deeper, you're not actually implementing priorities. 
> You're implementing different categories of init function. So this should be 
> an enum, and be renames to something less confusing.
>   

Good point, will do.

>> We do have dependencies.  I'd like virtio to be a module, and I'd like
>> virtio-net, virtio-blk, etc. to be modules.  This gets exposed as
>> virtio.c being a bus_init() and virtio-*.c being virtio_init().  We use
>> the integer priorities in module.h to express this.
>>     
>
> I'm not convinced virtio is as much of a separate entity as you think it is. 
> It's certainly not a bus. It's an implementation detail that happens to be 
> shared by several devices.
>   

It is once you abstract out the transport API from the transport 
implementation.  We fall short here in QEMU today mainly for better 
integration with how machines are created today.  I'd like to refactor 
the QEMU virtio code though to be closer to the Linux side of things.  I 
see a module mechanism like this as a prereq for doing such a refactoring.

s390 uses a different transport implementation (there is no PCI on 
s390)for virtio so if QEMU ever were to support KVM with s390, we would 
need a way to support this.

> Paul
>   


-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH 0/4][RFC] Add module infrastructure to QEMU

[Paul Brook]

> > I'm not convinced virtio is as much of a separate entity as you think it
> > is. It's certainly not a bus. It's an implementation detail that happens
> > to be shared by several devices.
>
> It is once you abstract out the transport API from the transport
> implementation.  We fall short here in QEMU today mainly for better
> integration with how machines are created today.  I'd like to refactor
> the QEMU virtio code though to be closer to the Linux side of things.  I
> see a module mechanism like this as a prereq for doing such a refactoring.

I think it's only a prerequisite if you want a single "virtio" device that 
then magically morphs into a specific device based on its config.  In 
practice I don't think this is really useful, and the virtio init is driven 
directly from the host binding. i.e. you have separate virtio-net-pci, 
virtio-blk-pci, virtio-net-s390, etc. devices in the same way that other pci 
devices are top level entities.

I've already got a patch I'm hoping to push out soon that implements a 
different virtio binding.

Paul

Re: [Qemu-devel] Re: [PATCH 0/4][RFC] Add module infrastructure toQEMU

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>>> There's also the issue that shared libraries imply it's OK for third 
>>> parties to ship binary plugins.
>>>   
>>
>> Can't we add wording to LICENSE to address this?  I'd really like to 
>> allow GPL plugins.  Linux and now gcc allow this.
>
> There's a lot of technical work to actually support shared libraries 
> on all the platforms QEMU supports.  

We could enable that only on platforms that support it.

> Even if we had that, and I expect a big flame war wrt reinventing 
> autoconf here too, in order to realistically talk about plugins, we 
> would need a much more modular infrastructure than we have today to 
> allow a config file to specify a particular device without any core 
> knowledge in QEMU of that device.

No doubt.  But taking the first step may encourage people to work 
towards it.

>
> It's an admirable goal but we're far away from that.  When we 
> eventually get there, we still have the issue of stable ABI (and Linux 
> has this too).

That's fine, I think.  Not ideal but livable.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] [PATCH 4/4] Introduce global .config to selectively enable compile features

[Anthony Liguori]

Avi Kivity wrote: 
>> +.config: $(DEFCONFIG)
>> +    $(call quiet-command,cp $< $@,)
>>  
>>   
>
> Please let's not make this a hidden file.

I'm happy to call it whatever you'd like :-)

Some variation of config-host.{mak,h} probably makes sense.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/4] Introduce global .config to selectively enable compile features

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>>> +.config: $(DEFCONFIG)
>>> +    $(call quiet-command,cp $< $@,)
>>>  
>>>   
>>
>> Please let's not make this a hidden file.
>
> I'm happy to call it whatever you'd like :-)
>
> Some variation of config-host.{mak,h} probably makes sense.
>

components.mak?

build.mak?

modules.mak?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] [PATCH 1/4] Add module infrastructure to QEMU

[Jean-Christophe PLAGNIOL-VILLARD]

> diff --git a/module.h b/module.h
> new file mode 100644
> index 0000000..5aa3eaa
> --- /dev/null
> +++ b/module.h
> @@ -0,0 +1,41 @@
> +/*
> + * QEMU Module Infrastructure
> + *
> + * Copyright IBM, Corp. 2009
> + *
> + * Authors:
> + *  Anthony Liguori   <aliguori@us.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef QEMU_MODULE_H
> +#define QEMU_MODULE_H
> +
> +#define module_init(function, priority)                                  \
> +static void __attribute__((constructor)) qemu_init_ ## function(void) {  \
> +   register_module_init(function, priority);                             \
> +}
> +
> +#define module_exit(function, priority)                                  \
> +static void __attribute__((constructor)) qemu_exit_ ## function(void) {  \
constructor? ;-)

Best Regards,
J.

Re: [Qemu-devel] [PATCH 1/4] Add module infrastructure to QEMU

[Jean-Christophe PLAGNIOL-VILLARD]

> +#ifndef QEMU_MODULE_H
> +#define QEMU_MODULE_H
> +
> +#define module_init(function, priority)                                  \
> +static void __attribute__((constructor)) qemu_init_ ## function(void) {  \
> +   register_module_init(function, priority);                             \
> +}
why not use the constructor priority?

Best Regards,
J.

Re: [Qemu-devel] [PATCH 1/4] Add module infrastructure to QEMU

[Anthony Liguori]

Jean-Christophe PLAGNIOL-VILLARD wrote:
>> diff --git a/module.h b/module.h
>> new file mode 100644
>> index 0000000..5aa3eaa
>> --- /dev/null
>> +++ b/module.h
>> @@ -0,0 +1,41 @@
>> +/*
>> + * QEMU Module Infrastructure
>> + *
>> + * Copyright IBM, Corp. 2009
>> + *
>> + * Authors:
>> + *  Anthony Liguori   <aliguori@us.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef QEMU_MODULE_H
>> +#define QEMU_MODULE_H
>> +
>> +#define module_init(function, priority)                                  \
>> +static void __attribute__((constructor)) qemu_init_ ## function(void) {  \
>> +   register_module_init(function, priority);                             \
>> +}
>> +
>> +#define module_exit(function, priority)                                  \
>> +static void __attribute__((constructor)) qemu_exit_ ## function(void) {  \
>>     
> constructor? ;-)
>   

Yes, that's intentional.  These functions just register functions, they 
don't actually call those functions.  This allows us to explicitly 
invoke the constructors/destructors when we want to instead of relying 
on gcc doing it before and after main.


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/4] Add module infrastructure to QEMU

[Anthony Liguori]

Jean-Christophe PLAGNIOL-VILLARD wrote:
>> +#ifndef QEMU_MODULE_H
>> +#define QEMU_MODULE_H
>> +
>> +#define module_init(function, priority)                                  \
>> +static void __attribute__((constructor)) qemu_init_ ## function(void) {  \
>> +   register_module_init(function, priority);                             \
>> +}
>>     
> why not use the constructor priority?
>   

We're just using constructors to register our real constructors so 
priority wouldn't really help us in the gcc constructors.

> Best Regards,
> J.
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/4] Add module infrastructure to QEMU

[Gerd Hoffmann]

On 05/12/09 00:02, Anthony Liguori wrote:
> Jean-Christophe PLAGNIOL-VILLARD wrote:
>> why not use the constructor priority?
>
> We're just using constructors to register our real constructors so
> priority wouldn't really help us in the gcc constructors.

Hmm?  Just call the real constructors in the order they are registered 
by the (prioritized) gcc constructors should do the trick, no?

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/4] Add module infrastructure to QEMU

[Paul Brook]

> >> why not use the constructor priority?
> >
> > We're just using constructors to register our real constructors so
> > priority wouldn't really help us in the gcc constructors.
>
> Hmm?  Just call the real constructors in the order they are registered
> by the (prioritized) gcc constructors should do the trick, no?

You're missing the point. The "priority" field is mis-named. It's actually 
defining different categories of init function that are invoked when the 
corresponding bit of qemu want them (e.g. registration of block backends v.s. 
registration of character backends).

Paul

Re: [Qemu-devel] [PATCH 1/4] Add module infrastructure to QEMU

[Anthony Liguori]

Gerd Hoffmann wrote:
> On 05/12/09 00:02, Anthony Liguori wrote:
>> Jean-Christophe PLAGNIOL-VILLARD wrote:
>>> why not use the constructor priority?
>>
>> We're just using constructors to register our real constructors so
>> priority wouldn't really help us in the gcc constructors.
>
> Hmm?  Just call the real constructors in the order they are registered 
> by the (prioritized) gcc constructors should do the trick, no?

You don't necessarily want to run all of the constructors at once.  
Eventually, it would be nice to be generic enough that we could do that 
but I think it would be a bit difficult today.


> cheers,
>   Gerd
>


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 0/4][RFC] Add module infrastructure to QEMU

[Anthony Liguori]

Kazu wrote:
> Hi,
>
> I think using GCC extension __attribute__ (constractor) has a problem on
> MinGW. Does this infrastructure have effects on Windows?

It works for me.

> Also there is a project that ports QEMU to VisualC++.
> http://sourceforge.net/projects/winqemu/
>
> What do you think of portability?

It's been discussed before. QEMU has no intention of supporting MSVC as
a compiler.

-- 

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 1/4] Add module infrastructure to QEMU

[Anthony Liguori]

Kazu wrote:
> Hi,
>
> I tried your patch on MinGW but there is a link error.
>
> LINK i386-softmmu/qemu.exe
> gcc.exe: osdep.o: No such file or directory

I've fixed the build issue.

> make[1]: *** [qemu.exe] Error 1
> make: *** [subdir-i386-softmmu] Error 2
>
> Regards,
> kazu


-- 
Regards,

Anthony Liguori