All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [RFC 0/8] CPU DeviceState v9
@ 2012-12-04 13:19 Eduardo Habkost
  2012-12-04 13:19 ` [Qemu-devel] [RFC 1/8] move -I$(SRC_PATH)/include compiler flag to Makefile.objs Eduardo Habkost
                   ` (8 more replies)
  0 siblings, 9 replies; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 13:19 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mammedov, Andreas Färber, Anthony Liguori, Paolo Bonzini

Changes on v9:
 - Instead of moving qemu_[un]register_reset() to reset.c and including
   it on *-user, create stubs for them on libqemustub.a
 - This is based on afaerber's qom-cpu branch, that has some header cleanup
   changes. You can get the complete series in a git tree at:
   https://github.com/ehabkost/qemu-hacks/tree/cpu_qdev.v9
   git://github.com/ehabkost/qemu-hacks.git cpu_qdev.v9

v8:
 - Use a simpler copyright header on qdev-properties-system.c
 - Use the new libqemustub.a mechanism instead of the (now exting)
   QEMU_WEAK_ALIAS mechanism
 - Move the reset-handler registration code to a new hw/reset.c file

v7:
 - Use the new QEMU_WEAK_ALIAS mechanism instead of the (now extinct)
   GCC_WEAK attribute (patches 20 and 21)

v6:
 - Simple rebase against latest qemu.git master
 - Patch 13: some new typedefs were added and others were removed
 - Patch 19: trivial rebase
v5:
 - Tons of header cleanups just to eliminate qlist.h <-> cpu-common.h circular
   dependency (patches 1-17)
 - Add copyright/license information to qdev-properties.c (patch 17)
 - Add copyright/license information to qdev-properties-system.c (patch 22)
 - use error_report()+abort() instead of hw_error() on qdev.c (patch 18)
 - Move qemu_[un]register_reset() and qemu_devices_reset() to qdev-core.c
   (patch 19)
 - Make vmstate_[un]register() weak stubs, instead of a new function (patch 20)
 - Make sysbus_get_default() weak stub, instead of new qbus reset (un)register
   functions (patch 21)
 - Eliminate qdev-system.c (all code is kept on qdev.c, now) (patch 22)
v4:
  - Add GCC_WEAK_DECL to functions that have GCC_WEAK versions
  - Updated the qdev_init_gpio_in() code on qdev-system.c to current version
  - Patch description updates (moved changelog below "---" and/or move info
    about changes made by different authors between SoB lines)
v3 (submitted by Igor):
  - rebased on top of 8b4a3df (today's master)
  - slight code reshuffling in (see commit's changelog)
     "qdev: separate core from the code used only by qemu-system-*"
     "move qemu_irq typedef out of cpu-common.h"
  - commit messages cleanup
v2:
  Removes the CONFIG_USER_ONLY ifdefs, and use weak symbols to move
  the vmstate and qemu_register_reset() handling to qdev-system.c

git tree for testing:
  https://github.com/ehabkost/qemu-hacks/tree/cpu_qdev.v9
  git://github.com/ehabkost/qemu-hacks.git cpu_qdev.v9

References to previous versions:
  v8: http://article.gmane.org/gmane.comp.emulators.qemu/182589
  v7: http://article.gmane.org/gmane.comp.emulators.qemu/179969
  v6: http://article.gmane.org/gmane.comp.emulators.qemu/179918
  v5: http://article.gmane.org/gmane.comp.emulators.qemu/177426
  v4: http://article.gmane.org/gmane.comp.emulators.qemu/176127
  v3: http://article.gmane.org/gmane.comp.emulators.qemu/175980
  v2: http://article.gmane.org/gmane.comp.emulators.qemu/173909
  v1: http://article.gmane.org/gmane.comp.emulators.qemu/166630


Eduardo Habkost (7):
  move -I$(SRC_PATH)/include compiler flag to Makefile.objs
  qdev: qdev_create(): use error_report() instead of hw_error()
  libqemustub: add qemu_[un]register_reset() stubs
  libqemustub: vmstate register/unregister stubs
  libqemustub: sysbus_get_default() stub
  qdev-properties.c: separate core from the code used only by
    qemu-system-*
  include qdev code into *-user, too

Igor Mammedov (1):
  qom: make CPU a child of DeviceState

 Makefile                    |   1 -
 Makefile.objs               |  23 ++-
 hw/Makefile.objs            |  10 +-
 hw/qdev-properties-system.c | 352 ++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev-properties.c        | 321 +---------------------------------------
 hw/qdev-properties.h        |   1 +
 hw/qdev.c                   |  21 +--
 include/qemu/cpu.h          |   6 +-
 qom/cpu.c                   |   3 +-
 stubs/Makefile.objs         |   3 +
 stubs/reset.c               |  13 ++
 stubs/sysbus.c              |   6 +
 stubs/vmstate.c             |  17 +++
 13 files changed, 428 insertions(+), 349 deletions(-)
 create mode 100644 hw/qdev-properties-system.c
 create mode 100644 stubs/reset.c
 create mode 100644 stubs/sysbus.c
 create mode 100644 stubs/vmstate.c

-- 
1.7.11.7

^ permalink raw reply	[flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC 1/8] move -I$(SRC_PATH)/include compiler flag to Makefile.objs
  2012-12-04 13:19 [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Eduardo Habkost
@ 2012-12-04 13:19 ` Eduardo Habkost
  2012-12-04 16:03   ` Andreas Färber
  2012-12-04 13:19 ` [Qemu-devel] [RFC 2/8] qdev: qdev_create(): use error_report() instead of hw_error() Eduardo Habkost
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 13:19 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mammedov, Andreas Färber, Anthony Liguori, Paolo Bonzini

The flag is necessary for code that doesn't use the variables from
Makefile (but use Makefile.objs), like libcacard/ and stubs/.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 Makefile      |  1 -
 Makefile.objs | 15 +++++++++------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index 9ecbcbb..739d9cd 100644
--- a/Makefile
+++ b/Makefile
@@ -145,7 +145,6 @@ audio/audio.o audio/fmodaudio.o: QEMU_CFLAGS += $(FMOD_CFLAGS)
 
 QEMU_CFLAGS+=$(CURL_CFLAGS)
 
-QEMU_CFLAGS += -I$(SRC_PATH)/include
 
 ui/cocoa.o: ui/cocoa.m
 
diff --git a/Makefile.objs b/Makefile.objs
index 3c7abca..0a0a33a 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -1,4 +1,13 @@
 #######################################################################
+# general compiler flags
+
+QEMU_CFLAGS += $(GLIB_CFLAGS)
+QEMU_CFLAGS += -I$(SRC_PATH)/include
+
+vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
+vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
+
+#######################################################################
 # Stub library, linked in tools
 stub-obj-y = stubs/
 
@@ -236,12 +245,6 @@ universal-obj-y += $(qapi-obj-y)
 qga-obj-y = qga/ qemu-ga.o module.o qemu-tool.o
 qga-obj-$(CONFIG_POSIX) += qemu-sockets.o qemu-option.o
 
-vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
-
-vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
-
-QEMU_CFLAGS+=$(GLIB_CFLAGS)
-
 nested-vars += \
 	stub-obj-y \
 	qga-obj-y \
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC 2/8] qdev: qdev_create(): use error_report() instead of hw_error()
  2012-12-04 13:19 [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Eduardo Habkost
  2012-12-04 13:19 ` [Qemu-devel] [RFC 1/8] move -I$(SRC_PATH)/include compiler flag to Makefile.objs Eduardo Habkost
@ 2012-12-04 13:19 ` Eduardo Habkost
  2012-12-04 16:12   ` Andreas Färber
  2012-12-04 13:19 ` [Qemu-devel] [RFC 3/8] libqemustub: add qemu_[un]register_reset() stubs Eduardo Habkost
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 13:19 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mammedov, Andreas Färber, Anthony Liguori, Paolo Bonzini

hw_error() is specific for fatal hardware emulation errors, not for
internal errors related to the qdev object/class abstraction or object
initialization.

Replace it with an error_report() call, followed by abort().

This will also help reduce dependencies of the qdev code (as hw_error()
is from cpus.o, and depends on the CPU list from exec.o).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 hw/qdev.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 788b4da..599382c 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -109,10 +109,12 @@ DeviceState *qdev_create(BusState *bus, const char *name)
     dev = qdev_try_create(bus, name);
     if (!dev) {
         if (bus) {
-            hw_error("Unknown device '%s' for bus '%s'\n", name,
-                     object_get_typename(OBJECT(bus)));
+            error_report("Unknown device '%s' for bus '%s'\n", name,
+                         object_get_typename(OBJECT(bus)));
+            abort();
         } else {
-            hw_error("Unknown device '%s' for default sysbus\n", name);
+            error_report("Unknown device '%s' for default sysbus\n", name);
+            abort();
         }
     }
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC 3/8] libqemustub: add qemu_[un]register_reset() stubs
  2012-12-04 13:19 [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Eduardo Habkost
  2012-12-04 13:19 ` [Qemu-devel] [RFC 1/8] move -I$(SRC_PATH)/include compiler flag to Makefile.objs Eduardo Habkost
  2012-12-04 13:19 ` [Qemu-devel] [RFC 2/8] qdev: qdev_create(): use error_report() instead of hw_error() Eduardo Habkost
@ 2012-12-04 13:19 ` Eduardo Habkost
  2012-12-04 13:19 ` [Qemu-devel] [RFC 4/8] libqemustub: vmstate register/unregister stubs Eduardo Habkost
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 13:19 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mammedov, Andreas Färber, Anthony Liguori, Paolo Bonzini

This will be useful for code that don't call qemu_devices_reset() (e.g.
*-user). If qemu_devices_reset() is never called, it means we don't need
to keep track of the reset handler list.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 stubs/Makefile.objs |  1 +
 stubs/reset.c       | 13 +++++++++++++
 2 files changed, 14 insertions(+)
 create mode 100644 stubs/reset.c

diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 035b29a..00f0b64 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -5,4 +5,5 @@ stub-obj-y += fdset-get-fd.o
 stub-obj-y += fdset-remove-fd.o
 stub-obj-y += get-fd.o
 stub-obj-y += set-fd-handler.o
+stub-obj-y += reset.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
diff --git a/stubs/reset.c b/stubs/reset.c
new file mode 100644
index 0000000..ad28725
--- /dev/null
+++ b/stubs/reset.c
@@ -0,0 +1,13 @@
+#include "hw/hw.h"
+
+/* Stub functions for binaries that never call qemu_devices_reset(),
+ * and don't need to keep track of the reset handler list.
+ */
+
+void qemu_register_reset(QEMUResetHandler *func, void *opaque)
+{
+}
+
+void qemu_unregister_reset(QEMUResetHandler *func, void *opaque)
+{
+}
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC 4/8] libqemustub: vmstate register/unregister stubs
  2012-12-04 13:19 [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Eduardo Habkost
                   ` (2 preceding siblings ...)
  2012-12-04 13:19 ` [Qemu-devel] [RFC 3/8] libqemustub: add qemu_[un]register_reset() stubs Eduardo Habkost
@ 2012-12-04 13:19 ` Eduardo Habkost
  2012-12-04 13:19 ` [Qemu-devel] [RFC 5/8] libqemustub: sysbus_get_default() stub Eduardo Habkost
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 13:19 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mammedov, Andreas Färber, Anthony Liguori, Paolo Bonzini

Add vmstate stub functions, so that qdev.o can be used without savevm.o
when vmstate support is not necessary (i.e. by *-user).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Originally submitted as:
  Subject: qdev-core: isolate vmstate handling into separate functions

Changes v1 -> v2:
 - Add GCC_WEAK_DECL to function declarations

Changes v2 -> v3:
 - Subject: qdev: add weak aliases for vmstate handling on qdev.c
 - Make vmstate_register_with_alias_id()/vmstate_unregister()
   have GCC_WEAK versions, instead of creating a new function
 - Kept qdev_get_vmsd() inside qdev.c

Changss v3 -> v4:
 - Use the new QEMU_WEAK_ALIAS system instead of GCC_WEAK

Changes v4 -> v5:
 - Use the new libqemustub.a, instead of QEMU_WEAK_ALIAS

Changes v5 -> v6:
 - Cosmetic whitespace changes
---
 stubs/Makefile.objs |  1 +
 stubs/vmstate.c     | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)
 create mode 100644 stubs/vmstate.c

diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 00f0b64..ca2197e 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -6,4 +6,5 @@ stub-obj-y += fdset-remove-fd.o
 stub-obj-y += get-fd.o
 stub-obj-y += set-fd-handler.o
 stub-obj-y += reset.o
+stub-obj-y += vmstate.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
diff --git a/stubs/vmstate.c b/stubs/vmstate.c
new file mode 100644
index 0000000..badf79e
--- /dev/null
+++ b/stubs/vmstate.c
@@ -0,0 +1,17 @@
+#include "qemu-common.h"
+#include "vmstate.h"
+
+int vmstate_register_with_alias_id(DeviceState *dev,
+                                   int instance_id,
+                                   const VMStateDescription *vmsd,
+                                   void *base, int alias_id,
+                                   int required_for_version)
+{
+    return 0;
+}
+
+void vmstate_unregister(DeviceState *dev,
+                        const VMStateDescription *vmsd,
+                        void *opaque)
+{
+}
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC 5/8] libqemustub: sysbus_get_default() stub
  2012-12-04 13:19 [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Eduardo Habkost
                   ` (3 preceding siblings ...)
  2012-12-04 13:19 ` [Qemu-devel] [RFC 4/8] libqemustub: vmstate register/unregister stubs Eduardo Habkost
@ 2012-12-04 13:19 ` Eduardo Habkost
  2012-12-04 13:19 ` [Qemu-devel] [RFC 6/8] qdev-properties.c: separate core from the code used only by qemu-system-* Eduardo Habkost
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 13:19 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mammedov, Andreas Färber, Anthony Liguori, Paolo Bonzini

The stub will be used on cases where sysbus.c is not compiled in (e.g.
*-user).

Note that code that uses NULL as the bus with qdev{_try,}_create()
implicitly uses sysbus_get_default() as the bus, and will still require
sysbus.c to be compiled in.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 - Use the new QEMU_WEAK_ALIAS mechanism, instead of GCC_WEAK

Changes v2 -> v3:
 - Use the new libqemustub.a mechanism, instead of QEMU_WEAK_ALIAS
---
 stubs/Makefile.objs | 1 +
 stubs/sysbus.c      | 6 ++++++
 2 files changed, 7 insertions(+)
 create mode 100644 stubs/sysbus.c

diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index ca2197e..7672c69 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -7,4 +7,5 @@ stub-obj-y += get-fd.o
 stub-obj-y += set-fd-handler.o
 stub-obj-y += reset.o
 stub-obj-y += vmstate.o
+stub-obj-y += sysbus.o
 stub-obj-$(CONFIG_WIN32) += fd-register.o
diff --git a/stubs/sysbus.c b/stubs/sysbus.c
new file mode 100644
index 0000000..e134965
--- /dev/null
+++ b/stubs/sysbus.c
@@ -0,0 +1,6 @@
+#include "hw/qdev-core.h"
+
+BusState *sysbus_get_default(void)
+{
+    return NULL;
+}
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC 6/8] qdev-properties.c: separate core from the code used only by qemu-system-*
  2012-12-04 13:19 [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Eduardo Habkost
                   ` (4 preceding siblings ...)
  2012-12-04 13:19 ` [Qemu-devel] [RFC 5/8] libqemustub: sysbus_get_default() stub Eduardo Habkost
@ 2012-12-04 13:19 ` Eduardo Habkost
  2012-12-04 18:55   ` Blue Swirl
  2012-12-04 13:19 ` [Qemu-devel] [RFC 7/8] include qdev code into *-user, too Eduardo Habkost
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 13:19 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mammedov, Andreas Färber, Anthony Liguori, Paolo Bonzini

This separates the qdev properties code in two parts:
 - qdev-properties.c, that contains most of the qdev properties code;
 - qdev-properties-system.c for code specific for qemu-system-*,
   containing:
   - Property types: drive, chr, netdev, vlan, that depend on code that
     won't be included on *-user
   - qemu_add_globals(), that depends on qemu-config.o.

This change should help on two things:
 - Allowing DeviceState to be used by *-user without pulling
   dependencies that are specific for qemu-system-*;
 - Writing qdev unit tests without pulling too many dependencies.

The copyright/license header for the new file is directly copied from
qdev-properties.c.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Detailed changelog:

Changes v1 (ehabkost) -> v2 (imammedo):
 - keep qdev_get_child_bus() in hw/qdev.c
 - put qdev_set_nic_properties() in hw/qdev-properties-system.c

Changes v2 -> v3 (ehabkost):
 - updated the qdev_init_gpio_in() code on qdev-system.c to current
   version

Changes v3 -> v4 (ehabkost):
 - Added copyright/license information to qdev-properties-system.c
   (based on copyright/license of qdev-properties.c)
 - Whitespace change at the end of qdev-properties.c
 - Don't create qdev-system.c, now we can keep the qdev.c code as-is
   as the qdev.c dependencies were reduced
 - Rewrite patch description

Changes v4 -> v5 (ehabkost):
 - Remove large copyright header and instead just point to the original
   file it was based on

Changes v5 -> v6 (ehabkost):
 - Removed inter-SoB line changelog from commit message
---
 hw/Makefile.objs            |   1 +
 hw/qdev-properties-system.c | 352 ++++++++++++++++++++++++++++++++++++++++++++
 hw/qdev-properties.c        | 321 +---------------------------------------
 hw/qdev-properties.h        |   1 +
 hw/qdev.c                   |  13 --
 5 files changed, 355 insertions(+), 333 deletions(-)
 create mode 100644 hw/qdev-properties-system.c

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index d581d8d..96a8365 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -185,6 +185,7 @@ common-obj-y += bt.o bt-l2cap.o bt-sdp.o bt-hci.o bt-hid.o
 common-obj-y += bt-hci-csr.o
 common-obj-y += msmouse.o ps2.o
 common-obj-y += qdev.o qdev-properties.o qdev-monitor.o
+common-obj-y += qdev-properties-system.o
 common-obj-$(CONFIG_BRLAPI) += baum.o
 
 # xen backend driver support
diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
new file mode 100644
index 0000000..9a7e0b3
--- /dev/null
+++ b/hw/qdev-properties-system.c
@@ -0,0 +1,352 @@
+/*
+ * qdev property parsing and global properties
+ * (parts specific for qemu-system-*)
+ *
+ * This file is based on code from hw/qdev-properties.c from
+ * commit 4e68f7a0819f179c2ff90a60611806c789911cc2,
+ * Copyright (c) Gerd Hoffmann <kraxel@redhat.com> and other contributors.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "net.h"
+#include "qdev.h"
+#include "qerror.h"
+#include "blockdev.h"
+#include "hw/block-common.h"
+#include "net/hub.h"
+#include "qapi/qapi-visit-core.h"
+
+static void get_pointer(Object *obj, Visitor *v, Property *prop,
+                        const char *(*print)(void *ptr),
+                        const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    void **ptr = qdev_get_prop_ptr(dev, prop);
+    char *p;
+
+    p = (char *) (*ptr ? print(*ptr) : "");
+    visit_type_str(v, &p, name, errp);
+}
+
+static void set_pointer(Object *obj, Visitor *v, Property *prop,
+                        int (*parse)(DeviceState *dev, const char *str,
+                                     void **ptr),
+                        const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Error *local_err = NULL;
+    void **ptr = qdev_get_prop_ptr(dev, prop);
+    char *str;
+    int ret;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_str(v, &str, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (!*str) {
+        g_free(str);
+        *ptr = NULL;
+        return;
+    }
+    ret = parse(dev, str, ptr);
+    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
+    g_free(str);
+}
+
+/* --- drive --- */
+
+static int parse_drive(DeviceState *dev, const char *str, void **ptr)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_find(str);
+    if (bs == NULL)
+        return -ENOENT;
+    if (bdrv_attach_dev(bs, dev) < 0)
+        return -EEXIST;
+    *ptr = bs;
+    return 0;
+}
+
+static void release_drive(Object *obj, const char *name, void *opaque)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
+
+    if (*ptr) {
+        bdrv_detach_dev(*ptr, dev);
+        blockdev_auto_del(*ptr);
+    }
+}
+
+static const char *print_drive(void *ptr)
+{
+    return bdrv_get_device_name(ptr);
+}
+
+static void get_drive(Object *obj, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    get_pointer(obj, v, opaque, print_drive, name, errp);
+}
+
+static void set_drive(Object *obj, Visitor *v, void *opaque,
+                      const char *name, Error **errp)
+{
+    set_pointer(obj, v, opaque, parse_drive, name, errp);
+}
+
+PropertyInfo qdev_prop_drive = {
+    .name  = "drive",
+    .get   = get_drive,
+    .set   = set_drive,
+    .release = release_drive,
+};
+
+/* --- character device --- */
+
+static int parse_chr(DeviceState *dev, const char *str, void **ptr)
+{
+    CharDriverState *chr = qemu_chr_find(str);
+    if (chr == NULL) {
+        return -ENOENT;
+    }
+    if (chr->avail_connections < 1) {
+        return -EEXIST;
+    }
+    *ptr = chr;
+    --chr->avail_connections;
+    return 0;
+}
+
+static void release_chr(Object *obj, const char *name, void *opaque)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
+
+    if (*ptr) {
+        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
+    }
+}
+
+
+static const char *print_chr(void *ptr)
+{
+    CharDriverState *chr = ptr;
+
+    return chr->label ? chr->label : "";
+}
+
+static void get_chr(Object *obj, Visitor *v, void *opaque,
+                    const char *name, Error **errp)
+{
+    get_pointer(obj, v, opaque, print_chr, name, errp);
+}
+
+static void set_chr(Object *obj, Visitor *v, void *opaque,
+                    const char *name, Error **errp)
+{
+    set_pointer(obj, v, opaque, parse_chr, name, errp);
+}
+
+PropertyInfo qdev_prop_chr = {
+    .name  = "chr",
+    .get   = get_chr,
+    .set   = set_chr,
+    .release = release_chr,
+};
+
+/* --- netdev device --- */
+
+static int parse_netdev(DeviceState *dev, const char *str, void **ptr)
+{
+    NetClientState *netdev = qemu_find_netdev(str);
+
+    if (netdev == NULL) {
+        return -ENOENT;
+    }
+    if (netdev->peer) {
+        return -EEXIST;
+    }
+    *ptr = netdev;
+    return 0;
+}
+
+static const char *print_netdev(void *ptr)
+{
+    NetClientState *netdev = ptr;
+
+    return netdev->name ? netdev->name : "";
+}
+
+static void get_netdev(Object *obj, Visitor *v, void *opaque,
+                       const char *name, Error **errp)
+{
+    get_pointer(obj, v, opaque, print_netdev, name, errp);
+}
+
+static void set_netdev(Object *obj, Visitor *v, void *opaque,
+                       const char *name, Error **errp)
+{
+    set_pointer(obj, v, opaque, parse_netdev, name, errp);
+}
+
+PropertyInfo qdev_prop_netdev = {
+    .name  = "netdev",
+    .get   = get_netdev,
+    .set   = set_netdev,
+};
+
+void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
+{
+    qdev_prop_set_macaddr(dev, "mac", nd->macaddr.a);
+    if (nd->netdev)
+        qdev_prop_set_netdev(dev, "netdev", nd->netdev);
+    if (nd->nvectors != DEV_NVECTORS_UNSPECIFIED &&
+        object_property_find(OBJECT(dev), "vectors", NULL)) {
+        qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
+    }
+    nd->instantiated = 1;
+}
+
+/* --- vlan --- */
+
+static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
+{
+    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
+
+    if (*ptr) {
+        int id;
+        if (!net_hub_id_for_client(*ptr, &id)) {
+            return snprintf(dest, len, "%d", id);
+        }
+    }
+
+    return snprintf(dest, len, "<null>");
+}
+
+static void get_vlan(Object *obj, Visitor *v, void *opaque,
+                     const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
+    int32_t id = -1;
+
+    if (*ptr) {
+        int hub_id;
+        if (!net_hub_id_for_client(*ptr, &hub_id)) {
+            id = hub_id;
+        }
+    }
+
+    visit_type_int32(v, &id, name, errp);
+}
+
+static void set_vlan(Object *obj, Visitor *v, void *opaque,
+                     const char *name, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
+    Error *local_err = NULL;
+    int32_t id;
+    NetClientState *hubport;
+
+    if (dev->state != DEV_STATE_CREATED) {
+        error_set(errp, QERR_PERMISSION_DENIED);
+        return;
+    }
+
+    visit_type_int32(v, &id, name, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    if (id == -1) {
+        *ptr = NULL;
+        return;
+    }
+
+    hubport = net_hub_port_find(id);
+    if (!hubport) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  name, prop->info->name);
+        return;
+    }
+    *ptr = hubport;
+}
+
+PropertyInfo qdev_prop_vlan = {
+    .name  = "vlan",
+    .print = print_vlan,
+    .get   = get_vlan,
+    .set   = set_vlan,
+};
+
+
+int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
+{
+    Error *errp = NULL;
+    const char *bdrv_name = value ? bdrv_get_device_name(value) : "";
+    object_property_set_str(OBJECT(dev), bdrv_name,
+                            name, &errp);
+    if (errp) {
+        qerror_report_err(errp);
+        error_free(errp);
+        return -1;
+    }
+    return 0;
+}
+
+void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value)
+{
+    if (qdev_prop_set_drive(dev, name, value) < 0) {
+        exit(1);
+    }
+}
+
+void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value)
+{
+    Error *errp = NULL;
+    assert(!value || value->label);
+    object_property_set_str(OBJECT(dev),
+                            value ? value->label : "", name, &errp);
+    assert_no_error(errp);
+}
+
+void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value)
+{
+    Error *errp = NULL;
+    assert(!value || value->name);
+    object_property_set_str(OBJECT(dev),
+                            value ? value->name : "", name, &errp);
+    assert_no_error(errp);
+}
+
+static int qdev_add_one_global(QemuOpts *opts, void *opaque)
+{
+    GlobalProperty *g;
+
+    g = g_malloc0(sizeof(*g));
+    g->driver   = qemu_opt_get(opts, "driver");
+    g->property = qemu_opt_get(opts, "property");
+    g->value    = qemu_opt_get(opts, "value");
+    qdev_prop_register_global(g);
+    return 0;
+}
+
+void qemu_add_globals(void)
+{
+    qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
+}
diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index 81d901c..9ec04be 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -13,49 +13,6 @@ void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
     return ptr;
 }
 
-static void get_pointer(Object *obj, Visitor *v, Property *prop,
-                        const char *(*print)(void *ptr),
-                        const char *name, Error **errp)
-{
-    DeviceState *dev = DEVICE(obj);
-    void **ptr = qdev_get_prop_ptr(dev, prop);
-    char *p;
-
-    p = (char *) (*ptr ? print(*ptr) : "");
-    visit_type_str(v, &p, name, errp);
-}
-
-static void set_pointer(Object *obj, Visitor *v, Property *prop,
-                        int (*parse)(DeviceState *dev, const char *str,
-                                     void **ptr),
-                        const char *name, Error **errp)
-{
-    DeviceState *dev = DEVICE(obj);
-    Error *local_err = NULL;
-    void **ptr = qdev_get_prop_ptr(dev, prop);
-    char *str;
-    int ret;
-
-    if (dev->state != DEV_STATE_CREATED) {
-        error_set(errp, QERR_PERMISSION_DENIED);
-        return;
-    }
-
-    visit_type_str(v, &str, name, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-    if (!*str) {
-        g_free(str);
-        *ptr = NULL;
-        return;
-    }
-    ret = parse(dev, str, ptr);
-    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
-    g_free(str);
-}
-
 static void get_enum(Object *obj, Visitor *v, void *opaque,
                      const char *name, Error **errp)
 {
@@ -476,227 +433,6 @@ PropertyInfo qdev_prop_string = {
     .set   = set_string,
 };
 
-/* --- drive --- */
-
-static int parse_drive(DeviceState *dev, const char *str, void **ptr)
-{
-    BlockDriverState *bs;
-
-    bs = bdrv_find(str);
-    if (bs == NULL)
-        return -ENOENT;
-    if (bdrv_attach_dev(bs, dev) < 0)
-        return -EEXIST;
-    *ptr = bs;
-    return 0;
-}
-
-static void release_drive(Object *obj, const char *name, void *opaque)
-{
-    DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
-    BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
-
-    if (*ptr) {
-        bdrv_detach_dev(*ptr, dev);
-        blockdev_auto_del(*ptr);
-    }
-}
-
-static const char *print_drive(void *ptr)
-{
-    return bdrv_get_device_name(ptr);
-}
-
-static void get_drive(Object *obj, Visitor *v, void *opaque,
-                      const char *name, Error **errp)
-{
-    get_pointer(obj, v, opaque, print_drive, name, errp);
-}
-
-static void set_drive(Object *obj, Visitor *v, void *opaque,
-                      const char *name, Error **errp)
-{
-    set_pointer(obj, v, opaque, parse_drive, name, errp);
-}
-
-PropertyInfo qdev_prop_drive = {
-    .name  = "drive",
-    .get   = get_drive,
-    .set   = set_drive,
-    .release = release_drive,
-};
-
-/* --- character device --- */
-
-static int parse_chr(DeviceState *dev, const char *str, void **ptr)
-{
-    CharDriverState *chr = qemu_chr_find(str);
-    if (chr == NULL) {
-        return -ENOENT;
-    }
-    if (chr->avail_connections < 1) {
-        return -EEXIST;
-    }
-    *ptr = chr;
-    --chr->avail_connections;
-    return 0;
-}
-
-static void release_chr(Object *obj, const char *name, void *opaque)
-{
-    DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
-    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
-
-    if (*ptr) {
-        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
-    }
-}
-
-
-static const char *print_chr(void *ptr)
-{
-    CharDriverState *chr = ptr;
-
-    return chr->label ? chr->label : "";
-}
-
-static void get_chr(Object *obj, Visitor *v, void *opaque,
-                    const char *name, Error **errp)
-{
-    get_pointer(obj, v, opaque, print_chr, name, errp);
-}
-
-static void set_chr(Object *obj, Visitor *v, void *opaque,
-                    const char *name, Error **errp)
-{
-    set_pointer(obj, v, opaque, parse_chr, name, errp);
-}
-
-PropertyInfo qdev_prop_chr = {
-    .name  = "chr",
-    .get   = get_chr,
-    .set   = set_chr,
-    .release = release_chr,
-};
-
-/* --- netdev device --- */
-
-static int parse_netdev(DeviceState *dev, const char *str, void **ptr)
-{
-    NetClientState *netdev = qemu_find_netdev(str);
-
-    if (netdev == NULL) {
-        return -ENOENT;
-    }
-    if (netdev->peer) {
-        return -EEXIST;
-    }
-    *ptr = netdev;
-    return 0;
-}
-
-static const char *print_netdev(void *ptr)
-{
-    NetClientState *netdev = ptr;
-
-    return netdev->name ? netdev->name : "";
-}
-
-static void get_netdev(Object *obj, Visitor *v, void *opaque,
-                       const char *name, Error **errp)
-{
-    get_pointer(obj, v, opaque, print_netdev, name, errp);
-}
-
-static void set_netdev(Object *obj, Visitor *v, void *opaque,
-                       const char *name, Error **errp)
-{
-    set_pointer(obj, v, opaque, parse_netdev, name, errp);
-}
-
-PropertyInfo qdev_prop_netdev = {
-    .name  = "netdev",
-    .get   = get_netdev,
-    .set   = set_netdev,
-};
-
-/* --- vlan --- */
-
-static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
-{
-    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
-
-    if (*ptr) {
-        int id;
-        if (!net_hub_id_for_client(*ptr, &id)) {
-            return snprintf(dest, len, "%d", id);
-        }
-    }
-
-    return snprintf(dest, len, "<null>");
-}
-
-static void get_vlan(Object *obj, Visitor *v, void *opaque,
-                     const char *name, Error **errp)
-{
-    DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
-    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
-    int32_t id = -1;
-
-    if (*ptr) {
-        int hub_id;
-        if (!net_hub_id_for_client(*ptr, &hub_id)) {
-            id = hub_id;
-        }
-    }
-
-    visit_type_int32(v, &id, name, errp);
-}
-
-static void set_vlan(Object *obj, Visitor *v, void *opaque,
-                     const char *name, Error **errp)
-{
-    DeviceState *dev = DEVICE(obj);
-    Property *prop = opaque;
-    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
-    Error *local_err = NULL;
-    int32_t id;
-    NetClientState *hubport;
-
-    if (dev->state != DEV_STATE_CREATED) {
-        error_set(errp, QERR_PERMISSION_DENIED);
-        return;
-    }
-
-    visit_type_int32(v, &id, name, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-    if (id == -1) {
-        *ptr = NULL;
-        return;
-    }
-
-    hubport = net_hub_port_find(id);
-    if (!hubport) {
-        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
-                  name, prop->info->name);
-        return;
-    }
-    *ptr = hubport;
-}
-
-PropertyInfo qdev_prop_vlan = {
-    .name  = "vlan",
-    .print = print_vlan,
-    .get   = get_vlan,
-    .set   = set_vlan,
-};
-
 /* --- pointer --- */
 
 /* Not a proper property, just for dirty hacks.  TODO Remove it!  */
@@ -1158,44 +894,6 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value)
     assert_no_error(errp);
 }
 
-int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
-{
-    Error *errp = NULL;
-    const char *bdrv_name = value ? bdrv_get_device_name(value) : "";
-    object_property_set_str(OBJECT(dev), bdrv_name,
-                            name, &errp);
-    if (errp) {
-        qerror_report_err(errp);
-        error_free(errp);
-        return -1;
-    }
-    return 0;
-}
-
-void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value)
-{
-    if (qdev_prop_set_drive(dev, name, value) < 0) {
-        exit(1);
-    }
-}
-void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value)
-{
-    Error *errp = NULL;
-    assert(!value || value->label);
-    object_property_set_str(OBJECT(dev),
-                            value ? value->label : "", name, &errp);
-    assert_no_error(errp);
-}
-
-void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value)
-{
-    Error *errp = NULL;
-    assert(!value || value->name);
-    object_property_set_str(OBJECT(dev),
-                            value ? value->name : "", name, &errp);
-    assert_no_error(errp);
-}
-
 void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value)
 {
     Error *errp = NULL;
@@ -1231,7 +929,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
 
 static QTAILQ_HEAD(, GlobalProperty) global_props = QTAILQ_HEAD_INITIALIZER(global_props);
 
-static void qdev_prop_register_global(GlobalProperty *prop)
+void qdev_prop_register_global(GlobalProperty *prop)
 {
     QTAILQ_INSERT_TAIL(&global_props, prop, next);
 }
@@ -1262,20 +960,3 @@ void qdev_prop_set_globals(DeviceState *dev)
         class = object_class_get_parent(class);
     } while (class);
 }
-
-static int qdev_add_one_global(QemuOpts *opts, void *opaque)
-{
-    GlobalProperty *g;
-
-    g = g_malloc0(sizeof(*g));
-    g->driver   = qemu_opt_get(opts, "driver");
-    g->property = qemu_opt_get(opts, "property");
-    g->value    = qemu_opt_get(opts, "value");
-    qdev_prop_register_global(g);
-    return 0;
-}
-
-void qemu_add_globals(void)
-{
-    qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
-}
diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
index 5b046ab..ddcf774 100644
--- a/hw/qdev-properties.h
+++ b/hw/qdev-properties.h
@@ -116,6 +116,7 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
 /* FIXME: Remove opaque pointer properties.  */
 void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
 
+void qdev_prop_register_global(GlobalProperty *prop);
 void qdev_prop_register_global_list(GlobalProperty *props);
 void qdev_prop_set_globals(DeviceState *dev);
 void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
diff --git a/hw/qdev.c b/hw/qdev.c
index 599382c..fa0af21 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -25,7 +25,6 @@
    inherit from a particular bus (e.g. PCI or I2C) rather than
    this API directly.  */
 
-#include "net.h"
 #include "qdev.h"
 #include "sysemu.h"
 #include "error.h"
@@ -312,18 +311,6 @@ void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
     dev->gpio_out[n] = pin;
 }
 
-void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
-{
-    qdev_prop_set_macaddr(dev, "mac", nd->macaddr.a);
-    if (nd->netdev)
-        qdev_prop_set_netdev(dev, "netdev", nd->netdev);
-    if (nd->nvectors != DEV_NVECTORS_UNSPECIFIED &&
-        object_property_find(OBJECT(dev), "vectors", NULL)) {
-        qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
-    }
-    nd->instantiated = 1;
-}
-
 BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
 {
     BusState *bus;
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC 7/8] include qdev code into *-user, too
  2012-12-04 13:19 [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Eduardo Habkost
                   ` (5 preceding siblings ...)
  2012-12-04 13:19 ` [Qemu-devel] [RFC 6/8] qdev-properties.c: separate core from the code used only by qemu-system-* Eduardo Habkost
@ 2012-12-04 13:19 ` Eduardo Habkost
  2012-12-04 13:19 ` [Qemu-devel] [RFC 8/8] qom: make CPU a child of DeviceState Eduardo Habkost
  2012-12-04 15:59 ` [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Andreas Färber
  8 siblings, 0 replies; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 13:19 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mammedov, Andreas Färber, Anthony Liguori, Paolo Bonzini

The code depends on some functions from qemu-option.o, so add
qemu-option.o to universal-obj-y to make sure it's included.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 - Keep files on the hw/ directory
   (it's simply easier to keep them there, as qdev.o depends on irq.o)
 - Add a $(hw-core-obj-y) variable to Makefile.objs for the qdev core code
 - Add irq.o to the list of core qdev files
   (as now the gpio code is being kept inside qdev.c)

Changes v2 -> v3:
 - Add reset.o to hw-core-obj-y

Changes v3 -> v4:
 - Removed reset.o again (it was replaced by stubs on libqemustub.a)
---
 Makefile.objs    | 8 ++++++++
 hw/Makefile.objs | 9 +++++++--
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/Makefile.objs b/Makefile.objs
index 0a0a33a..8fe4991 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -31,6 +31,13 @@ qom-obj-y = qom/
 universal-obj-y += $(qom-obj-y)
 
 #######################################################################
+# Core hw code (qdev core)
+hw-core-obj-y += hw/
+hw-core-obj-y += qemu-option.o
+
+universal-obj-y += $(hw-core-obj-y)
+
+#######################################################################
 # oslib-obj-y is code depending on the OS (win32 vs posix)
 oslib-obj-y = osdep.o cutils.o qemu-timer-common.o
 oslib-obj-$(CONFIG_WIN32) += oslib-win32.o qemu-thread-win32.o
@@ -253,5 +260,6 @@ nested-vars += \
 	block-obj-y \
 	user-obj-y \
 	common-obj-y \
+	hw-core-obj-y \
 	extra-obj-y
 dummy := $(call unnest-vars)
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 96a8365..1815536 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -1,3 +1,9 @@
+# core qdev-related obj files, also used by *-user:
+hw-core-obj-y += qdev.o qdev-properties.o
+# irq.o needed for qdev GPIO handling:
+hw-core-obj-y += irq.o
+
+
 common-obj-y = usb/ ide/
 common-obj-y += loader.o
 common-obj-$(CONFIG_VIRTIO) += virtio-console.o
@@ -158,7 +164,6 @@ common-obj-$(CONFIG_SOUND) += $(sound-obj-y)
 common-obj-$(CONFIG_REALLY_VIRTFS) += 9pfs/
 
 common-obj-y += usb/
-common-obj-y += irq.o
 common-obj-$(CONFIG_PTIMER) += ptimer.o
 common-obj-$(CONFIG_MAX7310) += max7310.o
 common-obj-$(CONFIG_WM8750) += wm8750.o
@@ -184,7 +189,7 @@ common-obj-$(CONFIG_SD) += sd.o
 common-obj-y += bt.o bt-l2cap.o bt-sdp.o bt-hci.o bt-hid.o
 common-obj-y += bt-hci-csr.o
 common-obj-y += msmouse.o ps2.o
-common-obj-y += qdev.o qdev-properties.o qdev-monitor.o
+common-obj-y += qdev-monitor.o
 common-obj-y += qdev-properties-system.o
 common-obj-$(CONFIG_BRLAPI) += baum.o
 
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [Qemu-devel] [RFC 8/8] qom: make CPU a child of DeviceState
  2012-12-04 13:19 [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Eduardo Habkost
                   ` (6 preceding siblings ...)
  2012-12-04 13:19 ` [Qemu-devel] [RFC 7/8] include qdev code into *-user, too Eduardo Habkost
@ 2012-12-04 13:19 ` Eduardo Habkost
  2012-12-05 14:48   ` Andreas Färber
  2012-12-04 15:59 ` [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Andreas Färber
  8 siblings, 1 reply; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 13:19 UTC (permalink / raw)
  To: qemu-devel
  Cc: Igor Mammedov, Andreas Färber, Anthony Liguori, Paolo Bonzini

From: Igor Mammedov <imammedo@redhat.com>

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
[ehabkost: change CPU type declaration to hae TYPE_DEVICE as parent]
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Yes, there is "changelog" data before the "---" mark, but I believe that
in this case they are important to indicate authorship and the scope of
the Signed-off-by lines (so they need to get into the git commit
message).
---
 include/qemu/cpu.h | 6 +++---
 qom/cpu.c          | 3 ++-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
index 61b7698..bc004fd 100644
--- a/include/qemu/cpu.h
+++ b/include/qemu/cpu.h
@@ -20,7 +20,7 @@
 #ifndef QEMU_CPU_H
 #define QEMU_CPU_H
 
-#include "qemu/object.h"
+#include "hw/qdev-core.h"
 #include "qemu-thread.h"
 
 /**
@@ -46,7 +46,7 @@ typedef struct CPUState CPUState;
  */
 typedef struct CPUClass {
     /*< private >*/
-    ObjectClass parent_class;
+    DeviceClass parent_class;
     /*< public >*/
 
     void (*reset)(CPUState *cpu);
@@ -62,7 +62,7 @@ typedef struct CPUClass {
  */
 struct CPUState {
     /*< private >*/
-    Object parent_obj;
+    DeviceState parent_obj;
     /*< public >*/
 
     struct QemuThread *thread;
diff --git a/qom/cpu.c b/qom/cpu.c
index 5b36046..f59db7d 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -20,6 +20,7 @@
 
 #include "qemu/cpu.h"
 #include "qemu-common.h"
+#include "hw/qdev-core.h"
 
 void cpu_reset(CPUState *cpu)
 {
@@ -43,7 +44,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
 
 static TypeInfo cpu_type_info = {
     .name = TYPE_CPU,
-    .parent = TYPE_OBJECT,
+    .parent = TYPE_DEVICE,
     .instance_size = sizeof(CPUState),
     .abstract = true,
     .class_size = sizeof(CPUClass),
-- 
1.7.11.7

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 0/8] CPU DeviceState v9
  2012-12-04 13:19 [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Eduardo Habkost
                   ` (7 preceding siblings ...)
  2012-12-04 13:19 ` [Qemu-devel] [RFC 8/8] qom: make CPU a child of DeviceState Eduardo Habkost
@ 2012-12-04 15:59 ` Andreas Färber
  2012-12-04 16:40   ` Eduardo Habkost
  8 siblings, 1 reply; 22+ messages in thread
From: Andreas Färber @ 2012-12-04 15:59 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Peter Maydell, Riku Voipio, qemu-devel, Anthony Liguori,
	Paolo Bonzini, Igor Mammedov

Am 04.12.2012 14:19, schrieb Eduardo Habkost:
> Changes on v9:
>  - Instead of moving qemu_[un]register_reset() to reset.c and including
>    it on *-user, create stubs for them on libqemustub.a

We compile cpu.c twice. Can't we do the same for qdev.c or whatever uses
those functions? I feel they have no business being used in *-user.
CC'ing Riku and Peter.

Andreas

>  - This is based on afaerber's qom-cpu branch, that has some header cleanup
>    changes. You can get the complete series in a git tree at:
>    https://github.com/ehabkost/qemu-hacks/tree/cpu_qdev.v9
>    git://github.com/ehabkost/qemu-hacks.git cpu_qdev.v9
> 
> v8:
>  - Use a simpler copyright header on qdev-properties-system.c
>  - Use the new libqemustub.a mechanism instead of the (now exting)
>    QEMU_WEAK_ALIAS mechanism
>  - Move the reset-handler registration code to a new hw/reset.c file
> 
> v7:
>  - Use the new QEMU_WEAK_ALIAS mechanism instead of the (now extinct)
>    GCC_WEAK attribute (patches 20 and 21)
> 
> v6:
>  - Simple rebase against latest qemu.git master
>  - Patch 13: some new typedefs were added and others were removed
>  - Patch 19: trivial rebase
> v5:
>  - Tons of header cleanups just to eliminate qlist.h <-> cpu-common.h circular
>    dependency (patches 1-17)
>  - Add copyright/license information to qdev-properties.c (patch 17)
>  - Add copyright/license information to qdev-properties-system.c (patch 22)
>  - use error_report()+abort() instead of hw_error() on qdev.c (patch 18)
>  - Move qemu_[un]register_reset() and qemu_devices_reset() to qdev-core.c
>    (patch 19)
>  - Make vmstate_[un]register() weak stubs, instead of a new function (patch 20)
>  - Make sysbus_get_default() weak stub, instead of new qbus reset (un)register
>    functions (patch 21)
>  - Eliminate qdev-system.c (all code is kept on qdev.c, now) (patch 22)
> v4:
>   - Add GCC_WEAK_DECL to functions that have GCC_WEAK versions
>   - Updated the qdev_init_gpio_in() code on qdev-system.c to current version
>   - Patch description updates (moved changelog below "---" and/or move info
>     about changes made by different authors between SoB lines)
> v3 (submitted by Igor):
>   - rebased on top of 8b4a3df (today's master)
>   - slight code reshuffling in (see commit's changelog)
>      "qdev: separate core from the code used only by qemu-system-*"
>      "move qemu_irq typedef out of cpu-common.h"
>   - commit messages cleanup
> v2:
>   Removes the CONFIG_USER_ONLY ifdefs, and use weak symbols to move
>   the vmstate and qemu_register_reset() handling to qdev-system.c
> 
> git tree for testing:
>   https://github.com/ehabkost/qemu-hacks/tree/cpu_qdev.v9
>   git://github.com/ehabkost/qemu-hacks.git cpu_qdev.v9
> 
> References to previous versions:
>   v8: http://article.gmane.org/gmane.comp.emulators.qemu/182589
>   v7: http://article.gmane.org/gmane.comp.emulators.qemu/179969
>   v6: http://article.gmane.org/gmane.comp.emulators.qemu/179918
>   v5: http://article.gmane.org/gmane.comp.emulators.qemu/177426
>   v4: http://article.gmane.org/gmane.comp.emulators.qemu/176127
>   v3: http://article.gmane.org/gmane.comp.emulators.qemu/175980
>   v2: http://article.gmane.org/gmane.comp.emulators.qemu/173909
>   v1: http://article.gmane.org/gmane.comp.emulators.qemu/166630
> 
> 
> Eduardo Habkost (7):
>   move -I$(SRC_PATH)/include compiler flag to Makefile.objs
>   qdev: qdev_create(): use error_report() instead of hw_error()
>   libqemustub: add qemu_[un]register_reset() stubs
>   libqemustub: vmstate register/unregister stubs
>   libqemustub: sysbus_get_default() stub
>   qdev-properties.c: separate core from the code used only by
>     qemu-system-*
>   include qdev code into *-user, too
> 
> Igor Mammedov (1):
>   qom: make CPU a child of DeviceState
> 
>  Makefile                    |   1 -
>  Makefile.objs               |  23 ++-
>  hw/Makefile.objs            |  10 +-
>  hw/qdev-properties-system.c | 352 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev-properties.c        | 321 +---------------------------------------
>  hw/qdev-properties.h        |   1 +
>  hw/qdev.c                   |  21 +--
>  include/qemu/cpu.h          |   6 +-
>  qom/cpu.c                   |   3 +-
>  stubs/Makefile.objs         |   3 +
>  stubs/reset.c               |  13 ++
>  stubs/sysbus.c              |   6 +
>  stubs/vmstate.c             |  17 +++
>  13 files changed, 428 insertions(+), 349 deletions(-)
>  create mode 100644 hw/qdev-properties-system.c
>  create mode 100644 stubs/reset.c
>  create mode 100644 stubs/sysbus.c
>  create mode 100644 stubs/vmstate.c
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 1/8] move -I$(SRC_PATH)/include compiler flag to Makefile.objs
  2012-12-04 13:19 ` [Qemu-devel] [RFC 1/8] move -I$(SRC_PATH)/include compiler flag to Makefile.objs Eduardo Habkost
@ 2012-12-04 16:03   ` Andreas Färber
  2012-12-04 16:22     ` Eduardo Habkost
  0 siblings, 1 reply; 22+ messages in thread
From: Andreas Färber @ 2012-12-04 16:03 UTC (permalink / raw)
  To: Eduardo Habkost, Paolo Bonzini; +Cc: Igor Mammedov, qemu-devel, Anthony Liguori

Am 04.12.2012 14:19, schrieb Eduardo Habkost:
> The flag is necessary for code that doesn't use the variables from
> Makefile (but use Makefile.objs), like libcacard/ and stubs/.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

I don't quite understand the rationale of this patch.
libcacard/ and stubs/ shouldn't need vl.o, do they? The CFLAGS move
makes more sense to me.

Paolo, can you take a look please?

Thanks,
Andreas

> ---
>  Makefile      |  1 -
>  Makefile.objs | 15 +++++++++------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 9ecbcbb..739d9cd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -145,7 +145,6 @@ audio/audio.o audio/fmodaudio.o: QEMU_CFLAGS += $(FMOD_CFLAGS)
>  
>  QEMU_CFLAGS+=$(CURL_CFLAGS)
>  
> -QEMU_CFLAGS += -I$(SRC_PATH)/include
>  
>  ui/cocoa.o: ui/cocoa.m
>  
> diff --git a/Makefile.objs b/Makefile.objs
> index 3c7abca..0a0a33a 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -1,4 +1,13 @@
>  #######################################################################
> +# general compiler flags
> +
> +QEMU_CFLAGS += $(GLIB_CFLAGS)
> +QEMU_CFLAGS += -I$(SRC_PATH)/include
> +
> +vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
> +vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
> +
> +#######################################################################
>  # Stub library, linked in tools
>  stub-obj-y = stubs/
>  
> @@ -236,12 +245,6 @@ universal-obj-y += $(qapi-obj-y)
>  qga-obj-y = qga/ qemu-ga.o module.o qemu-tool.o
>  qga-obj-$(CONFIG_POSIX) += qemu-sockets.o qemu-option.o
>  
> -vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
> -
> -vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
> -
> -QEMU_CFLAGS+=$(GLIB_CFLAGS)
> -
>  nested-vars += \
>  	stub-obj-y \
>  	qga-obj-y \

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 2/8] qdev: qdev_create(): use error_report() instead of hw_error()
  2012-12-04 13:19 ` [Qemu-devel] [RFC 2/8] qdev: qdev_create(): use error_report() instead of hw_error() Eduardo Habkost
@ 2012-12-04 16:12   ` Andreas Färber
  0 siblings, 0 replies; 22+ messages in thread
From: Andreas Färber @ 2012-12-04 16:12 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Igor Mammedov, qemu-devel, Anthony Liguori, Paolo Bonzini

Am 04.12.2012 14:19, schrieb Eduardo Habkost:
> hw_error() is specific for fatal hardware emulation errors, not for
> internal errors related to the qdev object/class abstraction or object
> initialization.
> 
> Replace it with an error_report() call, followed by abort().
> 
> This will also help reduce dependencies of the qdev code (as hw_error()
> is from cpus.o, and depends on the CPU list from exec.o).
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

This was already posted as a PATCH and I thought I had acked it ...
seems I didn't, so I re-checked cpus.c:hw_error() to abort() as well:

Acked-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 1/8] move -I$(SRC_PATH)/include compiler flag to Makefile.objs
  2012-12-04 16:03   ` Andreas Färber
@ 2012-12-04 16:22     ` Eduardo Habkost
  0 siblings, 0 replies; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 16:22 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Paolo Bonzini, qemu-devel, Anthony Liguori, Igor Mammedov

On Tue, Dec 04, 2012 at 05:03:36PM +0100, Andreas Färber wrote:
> Am 04.12.2012 14:19, schrieb Eduardo Habkost:
> > The flag is necessary for code that doesn't use the variables from
> > Makefile (but use Makefile.objs), like libcacard/ and stubs/.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> I don't quite understand the rationale of this patch.
> libcacard/ and stubs/ shouldn't need vl.o, do they? The CFLAGS move
> makes more sense to me.

The change is not about vl.o, is about having having the header files from
include/qemu available when building libcacard and libqemustub.

I had to do that because now some files included by files in libcacard/ and
stubs/ end up including header files from include/qemu. e.g.:

 lt CC stubs/sysbus.lo
In file included from /home/ehabkost/pessoal/proj/virt/qemu/stubs/sysbus.c:1:0:
/home/ehabkost/pessoal/proj/virt/qemu/hw/qdev-core.h:7:25: fatal error: qemu/object.h: No such file or directory
compilation terminated.
make[1]: *** [stubs/sysbus.lo] Error 1
make: *** [subdir-libcacard] Error 2

I remember seeing a similar error when building libcacard while working in this
series, but now I can't trigger it anymore.


> 
> Paolo, can you take a look please?
> 
> Thanks,
> Andreas
> 
> > ---
> >  Makefile      |  1 -
> >  Makefile.objs | 15 +++++++++------
> >  2 files changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 9ecbcbb..739d9cd 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -145,7 +145,6 @@ audio/audio.o audio/fmodaudio.o: QEMU_CFLAGS += $(FMOD_CFLAGS)
> >  
> >  QEMU_CFLAGS+=$(CURL_CFLAGS)
> >  
> > -QEMU_CFLAGS += -I$(SRC_PATH)/include
> >  
> >  ui/cocoa.o: ui/cocoa.m
> >  
> > diff --git a/Makefile.objs b/Makefile.objs
> > index 3c7abca..0a0a33a 100644
> > --- a/Makefile.objs
> > +++ b/Makefile.objs
> > @@ -1,4 +1,13 @@
> >  #######################################################################
> > +# general compiler flags
> > +
> > +QEMU_CFLAGS += $(GLIB_CFLAGS)
> > +QEMU_CFLAGS += -I$(SRC_PATH)/include
> > +
> > +vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
> > +vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
> > +
> > +#######################################################################
> >  # Stub library, linked in tools
> >  stub-obj-y = stubs/
> >  
> > @@ -236,12 +245,6 @@ universal-obj-y += $(qapi-obj-y)
> >  qga-obj-y = qga/ qemu-ga.o module.o qemu-tool.o
> >  qga-obj-$(CONFIG_POSIX) += qemu-sockets.o qemu-option.o
> >  
> > -vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
> > -
> > -vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
> > -
> > -QEMU_CFLAGS+=$(GLIB_CFLAGS)
> > -
> >  nested-vars += \
> >  	stub-obj-y \
> >  	qga-obj-y \
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 0/8] CPU DeviceState v9
  2012-12-04 15:59 ` [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Andreas Färber
@ 2012-12-04 16:40   ` Eduardo Habkost
  2012-12-05 14:50     ` Andreas Färber
  0 siblings, 1 reply; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 16:40 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Peter Maydell, Riku Voipio, qemu-devel, Anthony Liguori,
	Paolo Bonzini, Igor Mammedov

On Tue, Dec 04, 2012 at 04:59:38PM +0100, Andreas Färber wrote:
> Am 04.12.2012 14:19, schrieb Eduardo Habkost:
> > Changes on v9:
> >  - Instead of moving qemu_[un]register_reset() to reset.c and including
> >    it on *-user, create stubs for them on libqemustub.a
> 
> We compile cpu.c twice. Can't we do the same for qdev.c or whatever uses
> those functions? I feel they have no business being used in *-user.
> CC'ing Riku and Peter.

I don't understand what exactly you are suggesting. You suggest adding
#ifdefs to qdev.c to compile out the qemu_[un]register_reset() calls?

I had a version of this series that did exactly that[1], but IIRC
somebody suggested using stub functions instead. And I agree with
whoever suggested it, I believe stub functions are cleaner when the the
stub version still have the semantics expected by the caller[2].

[1] http://article.gmane.org/gmane.comp.emulators.xen.devel/137686
[2] e.g. a no-op qemu_register_reset() still does the job it's supposed
    to do (making sure a function to be called when qemu_devices_reset()
    is called), if we know qemu_devices_reset() is never called.


> 
> Andreas
> 
> >  - This is based on afaerber's qom-cpu branch, that has some header cleanup
> >    changes. You can get the complete series in a git tree at:
> >    https://github.com/ehabkost/qemu-hacks/tree/cpu_qdev.v9
> >    git://github.com/ehabkost/qemu-hacks.git cpu_qdev.v9
> > 
> > v8:
> >  - Use a simpler copyright header on qdev-properties-system.c
> >  - Use the new libqemustub.a mechanism instead of the (now exting)
> >    QEMU_WEAK_ALIAS mechanism
> >  - Move the reset-handler registration code to a new hw/reset.c file
> > 
> > v7:
> >  - Use the new QEMU_WEAK_ALIAS mechanism instead of the (now extinct)
> >    GCC_WEAK attribute (patches 20 and 21)
> > 
> > v6:
> >  - Simple rebase against latest qemu.git master
> >  - Patch 13: some new typedefs were added and others were removed
> >  - Patch 19: trivial rebase
> > v5:
> >  - Tons of header cleanups just to eliminate qlist.h <-> cpu-common.h circular
> >    dependency (patches 1-17)
> >  - Add copyright/license information to qdev-properties.c (patch 17)
> >  - Add copyright/license information to qdev-properties-system.c (patch 22)
> >  - use error_report()+abort() instead of hw_error() on qdev.c (patch 18)
> >  - Move qemu_[un]register_reset() and qemu_devices_reset() to qdev-core.c
> >    (patch 19)
> >  - Make vmstate_[un]register() weak stubs, instead of a new function (patch 20)
> >  - Make sysbus_get_default() weak stub, instead of new qbus reset (un)register
> >    functions (patch 21)
> >  - Eliminate qdev-system.c (all code is kept on qdev.c, now) (patch 22)
> > v4:
> >   - Add GCC_WEAK_DECL to functions that have GCC_WEAK versions
> >   - Updated the qdev_init_gpio_in() code on qdev-system.c to current version
> >   - Patch description updates (moved changelog below "---" and/or move info
> >     about changes made by different authors between SoB lines)
> > v3 (submitted by Igor):
> >   - rebased on top of 8b4a3df (today's master)
> >   - slight code reshuffling in (see commit's changelog)
> >      "qdev: separate core from the code used only by qemu-system-*"
> >      "move qemu_irq typedef out of cpu-common.h"
> >   - commit messages cleanup
> > v2:
> >   Removes the CONFIG_USER_ONLY ifdefs, and use weak symbols to move
> >   the vmstate and qemu_register_reset() handling to qdev-system.c
> > 
> > git tree for testing:
> >   https://github.com/ehabkost/qemu-hacks/tree/cpu_qdev.v9
> >   git://github.com/ehabkost/qemu-hacks.git cpu_qdev.v9
> > 
> > References to previous versions:
> >   v8: http://article.gmane.org/gmane.comp.emulators.qemu/182589
> >   v7: http://article.gmane.org/gmane.comp.emulators.qemu/179969
> >   v6: http://article.gmane.org/gmane.comp.emulators.qemu/179918
> >   v5: http://article.gmane.org/gmane.comp.emulators.qemu/177426
> >   v4: http://article.gmane.org/gmane.comp.emulators.qemu/176127
> >   v3: http://article.gmane.org/gmane.comp.emulators.qemu/175980
> >   v2: http://article.gmane.org/gmane.comp.emulators.qemu/173909
> >   v1: http://article.gmane.org/gmane.comp.emulators.qemu/166630
> > 
> > 
> > Eduardo Habkost (7):
> >   move -I$(SRC_PATH)/include compiler flag to Makefile.objs
> >   qdev: qdev_create(): use error_report() instead of hw_error()
> >   libqemustub: add qemu_[un]register_reset() stubs
> >   libqemustub: vmstate register/unregister stubs
> >   libqemustub: sysbus_get_default() stub
> >   qdev-properties.c: separate core from the code used only by
> >     qemu-system-*
> >   include qdev code into *-user, too
> > 
> > Igor Mammedov (1):
> >   qom: make CPU a child of DeviceState
> > 
> >  Makefile                    |   1 -
> >  Makefile.objs               |  23 ++-
> >  hw/Makefile.objs            |  10 +-
> >  hw/qdev-properties-system.c | 352 ++++++++++++++++++++++++++++++++++++++++++++
> >  hw/qdev-properties.c        | 321 +---------------------------------------
> >  hw/qdev-properties.h        |   1 +
> >  hw/qdev.c                   |  21 +--
> >  include/qemu/cpu.h          |   6 +-
> >  qom/cpu.c                   |   3 +-
> >  stubs/Makefile.objs         |   3 +
> >  stubs/reset.c               |  13 ++
> >  stubs/sysbus.c              |   6 +
> >  stubs/vmstate.c             |  17 +++
> >  13 files changed, 428 insertions(+), 349 deletions(-)
> >  create mode 100644 hw/qdev-properties-system.c
> >  create mode 100644 stubs/reset.c
> >  create mode 100644 stubs/sysbus.c
> >  create mode 100644 stubs/vmstate.c
> > 
> 
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 6/8] qdev-properties.c: separate core from the code used only by qemu-system-*
  2012-12-04 13:19 ` [Qemu-devel] [RFC 6/8] qdev-properties.c: separate core from the code used only by qemu-system-* Eduardo Habkost
@ 2012-12-04 18:55   ` Blue Swirl
  2012-12-04 19:01     ` Eduardo Habkost
  0 siblings, 1 reply; 22+ messages in thread
From: Blue Swirl @ 2012-12-04 18:55 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Igor Mammedov, Paolo Bonzini, qemu-devel, Anthony Liguori,
	Andreas Färber

On Tue, Dec 4, 2012 at 1:19 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> This separates the qdev properties code in two parts:
>  - qdev-properties.c, that contains most of the qdev properties code;
>  - qdev-properties-system.c for code specific for qemu-system-*,
>    containing:
>    - Property types: drive, chr, netdev, vlan, that depend on code that
>      won't be included on *-user
>    - qemu_add_globals(), that depends on qemu-config.o.
>
> This change should help on two things:
>  - Allowing DeviceState to be used by *-user without pulling
>    dependencies that are specific for qemu-system-*;
>  - Writing qdev unit tests without pulling too many dependencies.
>
> The copyright/license header for the new file is directly copied from
> qdev-properties.c.
>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Detailed changelog:
>
> Changes v1 (ehabkost) -> v2 (imammedo):
>  - keep qdev_get_child_bus() in hw/qdev.c
>  - put qdev_set_nic_properties() in hw/qdev-properties-system.c
>
> Changes v2 -> v3 (ehabkost):
>  - updated the qdev_init_gpio_in() code on qdev-system.c to current
>    version
>
> Changes v3 -> v4 (ehabkost):
>  - Added copyright/license information to qdev-properties-system.c
>    (based on copyright/license of qdev-properties.c)
>  - Whitespace change at the end of qdev-properties.c
>  - Don't create qdev-system.c, now we can keep the qdev.c code as-is
>    as the qdev.c dependencies were reduced
>  - Rewrite patch description
>
> Changes v4 -> v5 (ehabkost):
>  - Remove large copyright header and instead just point to the original
>    file it was based on
>
> Changes v5 -> v6 (ehabkost):
>  - Removed inter-SoB line changelog from commit message
> ---
>  hw/Makefile.objs            |   1 +
>  hw/qdev-properties-system.c | 352 ++++++++++++++++++++++++++++++++++++++++++++
>  hw/qdev-properties.c        | 321 +---------------------------------------
>  hw/qdev-properties.h        |   1 +
>  hw/qdev.c                   |  13 --
>  5 files changed, 355 insertions(+), 333 deletions(-)
>  create mode 100644 hw/qdev-properties-system.c
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index d581d8d..96a8365 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -185,6 +185,7 @@ common-obj-y += bt.o bt-l2cap.o bt-sdp.o bt-hci.o bt-hid.o
>  common-obj-y += bt-hci-csr.o
>  common-obj-y += msmouse.o ps2.o
>  common-obj-y += qdev.o qdev-properties.o qdev-monitor.o
> +common-obj-y += qdev-properties-system.o
>  common-obj-$(CONFIG_BRLAPI) += baum.o
>
>  # xen backend driver support
> diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
> new file mode 100644
> index 0000000..9a7e0b3
> --- /dev/null
> +++ b/hw/qdev-properties-system.c
> @@ -0,0 +1,352 @@
> +/*
> + * qdev property parsing and global properties
> + * (parts specific for qemu-system-*)
> + *
> + * This file is based on code from hw/qdev-properties.c from
> + * commit 4e68f7a0819f179c2ff90a60611806c789911cc2,
> + * Copyright (c) Gerd Hoffmann <kraxel@redhat.com> and other contributors.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "net.h"
> +#include "qdev.h"
> +#include "qerror.h"
> +#include "blockdev.h"
> +#include "hw/block-common.h"
> +#include "net/hub.h"
> +#include "qapi/qapi-visit-core.h"
> +
> +static void get_pointer(Object *obj, Visitor *v, Property *prop,
> +                        const char *(*print)(void *ptr),
> +                        const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    void **ptr = qdev_get_prop_ptr(dev, prop);
> +    char *p;
> +
> +    p = (char *) (*ptr ? print(*ptr) : "");
> +    visit_type_str(v, &p, name, errp);
> +}
> +
> +static void set_pointer(Object *obj, Visitor *v, Property *prop,
> +                        int (*parse)(DeviceState *dev, const char *str,
> +                                     void **ptr),
> +                        const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Error *local_err = NULL;
> +    void **ptr = qdev_get_prop_ptr(dev, prop);
> +    char *str;
> +    int ret;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_str(v, &str, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (!*str) {
> +        g_free(str);
> +        *ptr = NULL;
> +        return;
> +    }
> +    ret = parse(dev, str, ptr);
> +    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
> +    g_free(str);
> +}
> +
> +/* --- drive --- */
> +
> +static int parse_drive(DeviceState *dev, const char *str, void **ptr)
> +{
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_find(str);
> +    if (bs == NULL)

Please add braces, use checkpatch.pl.

> +        return -ENOENT;
> +    if (bdrv_attach_dev(bs, dev) < 0)

Also here.

> +        return -EEXIST;
> +    *ptr = bs;
> +    return 0;
> +}
> +
> +static void release_drive(Object *obj, const char *name, void *opaque)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    if (*ptr) {
> +        bdrv_detach_dev(*ptr, dev);
> +        blockdev_auto_del(*ptr);
> +    }
> +}
> +
> +static const char *print_drive(void *ptr)
> +{
> +    return bdrv_get_device_name(ptr);
> +}
> +
> +static void get_drive(Object *obj, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    get_pointer(obj, v, opaque, print_drive, name, errp);
> +}
> +
> +static void set_drive(Object *obj, Visitor *v, void *opaque,
> +                      const char *name, Error **errp)
> +{
> +    set_pointer(obj, v, opaque, parse_drive, name, errp);
> +}
> +
> +PropertyInfo qdev_prop_drive = {
> +    .name  = "drive",
> +    .get   = get_drive,
> +    .set   = set_drive,
> +    .release = release_drive,
> +};
> +
> +/* --- character device --- */
> +
> +static int parse_chr(DeviceState *dev, const char *str, void **ptr)
> +{
> +    CharDriverState *chr = qemu_chr_find(str);
> +    if (chr == NULL) {
> +        return -ENOENT;
> +    }
> +    if (chr->avail_connections < 1) {
> +        return -EEXIST;
> +    }
> +    *ptr = chr;
> +    --chr->avail_connections;
> +    return 0;
> +}
> +
> +static void release_chr(Object *obj, const char *name, void *opaque)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    if (*ptr) {
> +        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
> +    }
> +}
> +
> +
> +static const char *print_chr(void *ptr)
> +{
> +    CharDriverState *chr = ptr;
> +
> +    return chr->label ? chr->label : "";
> +}
> +
> +static void get_chr(Object *obj, Visitor *v, void *opaque,
> +                    const char *name, Error **errp)
> +{
> +    get_pointer(obj, v, opaque, print_chr, name, errp);
> +}
> +
> +static void set_chr(Object *obj, Visitor *v, void *opaque,
> +                    const char *name, Error **errp)
> +{
> +    set_pointer(obj, v, opaque, parse_chr, name, errp);
> +}
> +
> +PropertyInfo qdev_prop_chr = {
> +    .name  = "chr",
> +    .get   = get_chr,
> +    .set   = set_chr,
> +    .release = release_chr,
> +};
> +
> +/* --- netdev device --- */
> +
> +static int parse_netdev(DeviceState *dev, const char *str, void **ptr)
> +{
> +    NetClientState *netdev = qemu_find_netdev(str);
> +
> +    if (netdev == NULL) {
> +        return -ENOENT;
> +    }
> +    if (netdev->peer) {
> +        return -EEXIST;
> +    }
> +    *ptr = netdev;
> +    return 0;
> +}
> +
> +static const char *print_netdev(void *ptr)
> +{
> +    NetClientState *netdev = ptr;
> +
> +    return netdev->name ? netdev->name : "";
> +}
> +
> +static void get_netdev(Object *obj, Visitor *v, void *opaque,
> +                       const char *name, Error **errp)
> +{
> +    get_pointer(obj, v, opaque, print_netdev, name, errp);
> +}
> +
> +static void set_netdev(Object *obj, Visitor *v, void *opaque,
> +                       const char *name, Error **errp)
> +{
> +    set_pointer(obj, v, opaque, parse_netdev, name, errp);
> +}
> +
> +PropertyInfo qdev_prop_netdev = {
> +    .name  = "netdev",
> +    .get   = get_netdev,
> +    .set   = set_netdev,
> +};
> +
> +void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
> +{
> +    qdev_prop_set_macaddr(dev, "mac", nd->macaddr.a);
> +    if (nd->netdev)
> +        qdev_prop_set_netdev(dev, "netdev", nd->netdev);

Ditto.

> +    if (nd->nvectors != DEV_NVECTORS_UNSPECIFIED &&
> +        object_property_find(OBJECT(dev), "vectors", NULL)) {
> +        qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
> +    }
> +    nd->instantiated = 1;
> +}
> +
> +/* --- vlan --- */
> +
> +static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
> +{
> +    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> +
> +    if (*ptr) {
> +        int id;
> +        if (!net_hub_id_for_client(*ptr, &id)) {
> +            return snprintf(dest, len, "%d", id);
> +        }
> +    }
> +
> +    return snprintf(dest, len, "<null>");
> +}
> +
> +static void get_vlan(Object *obj, Visitor *v, void *opaque,
> +                     const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> +    int32_t id = -1;
> +
> +    if (*ptr) {
> +        int hub_id;
> +        if (!net_hub_id_for_client(*ptr, &hub_id)) {
> +            id = hub_id;
> +        }
> +    }
> +
> +    visit_type_int32(v, &id, name, errp);
> +}
> +
> +static void set_vlan(Object *obj, Visitor *v, void *opaque,
> +                     const char *name, Error **errp)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    Property *prop = opaque;
> +    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> +    Error *local_err = NULL;
> +    int32_t id;
> +    NetClientState *hubport;
> +
> +    if (dev->state != DEV_STATE_CREATED) {
> +        error_set(errp, QERR_PERMISSION_DENIED);
> +        return;
> +    }
> +
> +    visit_type_int32(v, &id, name, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    if (id == -1) {
> +        *ptr = NULL;
> +        return;
> +    }
> +
> +    hubport = net_hub_port_find(id);
> +    if (!hubport) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                  name, prop->info->name);
> +        return;
> +    }
> +    *ptr = hubport;
> +}
> +
> +PropertyInfo qdev_prop_vlan = {
> +    .name  = "vlan",
> +    .print = print_vlan,
> +    .get   = get_vlan,
> +    .set   = set_vlan,
> +};
> +
> +
> +int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
> +{
> +    Error *errp = NULL;
> +    const char *bdrv_name = value ? bdrv_get_device_name(value) : "";
> +    object_property_set_str(OBJECT(dev), bdrv_name,
> +                            name, &errp);
> +    if (errp) {
> +        qerror_report_err(errp);
> +        error_free(errp);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value)
> +{
> +    if (qdev_prop_set_drive(dev, name, value) < 0) {
> +        exit(1);
> +    }
> +}
> +
> +void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value)
> +{
> +    Error *errp = NULL;
> +    assert(!value || value->label);
> +    object_property_set_str(OBJECT(dev),
> +                            value ? value->label : "", name, &errp);
> +    assert_no_error(errp);
> +}
> +
> +void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value)
> +{
> +    Error *errp = NULL;
> +    assert(!value || value->name);
> +    object_property_set_str(OBJECT(dev),
> +                            value ? value->name : "", name, &errp);
> +    assert_no_error(errp);
> +}
> +
> +static int qdev_add_one_global(QemuOpts *opts, void *opaque)
> +{
> +    GlobalProperty *g;
> +
> +    g = g_malloc0(sizeof(*g));
> +    g->driver   = qemu_opt_get(opts, "driver");
> +    g->property = qemu_opt_get(opts, "property");
> +    g->value    = qemu_opt_get(opts, "value");
> +    qdev_prop_register_global(g);
> +    return 0;
> +}
> +
> +void qemu_add_globals(void)
> +{
> +    qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
> +}
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 81d901c..9ec04be 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -13,49 +13,6 @@ void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
>      return ptr;
>  }
>
> -static void get_pointer(Object *obj, Visitor *v, Property *prop,
> -                        const char *(*print)(void *ptr),
> -                        const char *name, Error **errp)
> -{
> -    DeviceState *dev = DEVICE(obj);
> -    void **ptr = qdev_get_prop_ptr(dev, prop);
> -    char *p;
> -
> -    p = (char *) (*ptr ? print(*ptr) : "");
> -    visit_type_str(v, &p, name, errp);
> -}
> -
> -static void set_pointer(Object *obj, Visitor *v, Property *prop,
> -                        int (*parse)(DeviceState *dev, const char *str,
> -                                     void **ptr),
> -                        const char *name, Error **errp)
> -{
> -    DeviceState *dev = DEVICE(obj);
> -    Error *local_err = NULL;
> -    void **ptr = qdev_get_prop_ptr(dev, prop);
> -    char *str;
> -    int ret;
> -
> -    if (dev->state != DEV_STATE_CREATED) {
> -        error_set(errp, QERR_PERMISSION_DENIED);
> -        return;
> -    }
> -
> -    visit_type_str(v, &str, name, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -    if (!*str) {
> -        g_free(str);
> -        *ptr = NULL;
> -        return;
> -    }
> -    ret = parse(dev, str, ptr);
> -    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
> -    g_free(str);
> -}
> -
>  static void get_enum(Object *obj, Visitor *v, void *opaque,
>                       const char *name, Error **errp)
>  {
> @@ -476,227 +433,6 @@ PropertyInfo qdev_prop_string = {
>      .set   = set_string,
>  };
>
> -/* --- drive --- */
> -
> -static int parse_drive(DeviceState *dev, const char *str, void **ptr)
> -{
> -    BlockDriverState *bs;
> -
> -    bs = bdrv_find(str);
> -    if (bs == NULL)
> -        return -ENOENT;
> -    if (bdrv_attach_dev(bs, dev) < 0)
> -        return -EEXIST;
> -    *ptr = bs;
> -    return 0;
> -}
> -
> -static void release_drive(Object *obj, const char *name, void *opaque)
> -{
> -    DeviceState *dev = DEVICE(obj);
> -    Property *prop = opaque;
> -    BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
> -
> -    if (*ptr) {
> -        bdrv_detach_dev(*ptr, dev);
> -        blockdev_auto_del(*ptr);
> -    }
> -}
> -
> -static const char *print_drive(void *ptr)
> -{
> -    return bdrv_get_device_name(ptr);
> -}
> -
> -static void get_drive(Object *obj, Visitor *v, void *opaque,
> -                      const char *name, Error **errp)
> -{
> -    get_pointer(obj, v, opaque, print_drive, name, errp);
> -}
> -
> -static void set_drive(Object *obj, Visitor *v, void *opaque,
> -                      const char *name, Error **errp)
> -{
> -    set_pointer(obj, v, opaque, parse_drive, name, errp);
> -}
> -
> -PropertyInfo qdev_prop_drive = {
> -    .name  = "drive",
> -    .get   = get_drive,
> -    .set   = set_drive,
> -    .release = release_drive,
> -};
> -
> -/* --- character device --- */
> -
> -static int parse_chr(DeviceState *dev, const char *str, void **ptr)
> -{
> -    CharDriverState *chr = qemu_chr_find(str);
> -    if (chr == NULL) {
> -        return -ENOENT;
> -    }
> -    if (chr->avail_connections < 1) {
> -        return -EEXIST;
> -    }
> -    *ptr = chr;
> -    --chr->avail_connections;
> -    return 0;
> -}
> -
> -static void release_chr(Object *obj, const char *name, void *opaque)
> -{
> -    DeviceState *dev = DEVICE(obj);
> -    Property *prop = opaque;
> -    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
> -
> -    if (*ptr) {
> -        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
> -    }
> -}
> -
> -
> -static const char *print_chr(void *ptr)
> -{
> -    CharDriverState *chr = ptr;
> -
> -    return chr->label ? chr->label : "";
> -}
> -
> -static void get_chr(Object *obj, Visitor *v, void *opaque,
> -                    const char *name, Error **errp)
> -{
> -    get_pointer(obj, v, opaque, print_chr, name, errp);
> -}
> -
> -static void set_chr(Object *obj, Visitor *v, void *opaque,
> -                    const char *name, Error **errp)
> -{
> -    set_pointer(obj, v, opaque, parse_chr, name, errp);
> -}
> -
> -PropertyInfo qdev_prop_chr = {
> -    .name  = "chr",
> -    .get   = get_chr,
> -    .set   = set_chr,
> -    .release = release_chr,
> -};
> -
> -/* --- netdev device --- */
> -
> -static int parse_netdev(DeviceState *dev, const char *str, void **ptr)
> -{
> -    NetClientState *netdev = qemu_find_netdev(str);
> -
> -    if (netdev == NULL) {
> -        return -ENOENT;
> -    }
> -    if (netdev->peer) {
> -        return -EEXIST;
> -    }
> -    *ptr = netdev;
> -    return 0;
> -}
> -
> -static const char *print_netdev(void *ptr)
> -{
> -    NetClientState *netdev = ptr;
> -
> -    return netdev->name ? netdev->name : "";
> -}
> -
> -static void get_netdev(Object *obj, Visitor *v, void *opaque,
> -                       const char *name, Error **errp)
> -{
> -    get_pointer(obj, v, opaque, print_netdev, name, errp);
> -}
> -
> -static void set_netdev(Object *obj, Visitor *v, void *opaque,
> -                       const char *name, Error **errp)
> -{
> -    set_pointer(obj, v, opaque, parse_netdev, name, errp);
> -}
> -
> -PropertyInfo qdev_prop_netdev = {
> -    .name  = "netdev",
> -    .get   = get_netdev,
> -    .set   = set_netdev,
> -};
> -
> -/* --- vlan --- */
> -
> -static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
> -{
> -    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> -
> -    if (*ptr) {
> -        int id;
> -        if (!net_hub_id_for_client(*ptr, &id)) {
> -            return snprintf(dest, len, "%d", id);
> -        }
> -    }
> -
> -    return snprintf(dest, len, "<null>");
> -}
> -
> -static void get_vlan(Object *obj, Visitor *v, void *opaque,
> -                     const char *name, Error **errp)
> -{
> -    DeviceState *dev = DEVICE(obj);
> -    Property *prop = opaque;
> -    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> -    int32_t id = -1;
> -
> -    if (*ptr) {
> -        int hub_id;
> -        if (!net_hub_id_for_client(*ptr, &hub_id)) {
> -            id = hub_id;
> -        }
> -    }
> -
> -    visit_type_int32(v, &id, name, errp);
> -}
> -
> -static void set_vlan(Object *obj, Visitor *v, void *opaque,
> -                     const char *name, Error **errp)
> -{
> -    DeviceState *dev = DEVICE(obj);
> -    Property *prop = opaque;
> -    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> -    Error *local_err = NULL;
> -    int32_t id;
> -    NetClientState *hubport;
> -
> -    if (dev->state != DEV_STATE_CREATED) {
> -        error_set(errp, QERR_PERMISSION_DENIED);
> -        return;
> -    }
> -
> -    visit_type_int32(v, &id, name, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> -    if (id == -1) {
> -        *ptr = NULL;
> -        return;
> -    }
> -
> -    hubport = net_hub_port_find(id);
> -    if (!hubport) {
> -        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> -                  name, prop->info->name);
> -        return;
> -    }
> -    *ptr = hubport;
> -}
> -
> -PropertyInfo qdev_prop_vlan = {
> -    .name  = "vlan",
> -    .print = print_vlan,
> -    .get   = get_vlan,
> -    .set   = set_vlan,
> -};
> -
>  /* --- pointer --- */
>
>  /* Not a proper property, just for dirty hacks.  TODO Remove it!  */
> @@ -1158,44 +894,6 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value)
>      assert_no_error(errp);
>  }
>
> -int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
> -{
> -    Error *errp = NULL;
> -    const char *bdrv_name = value ? bdrv_get_device_name(value) : "";
> -    object_property_set_str(OBJECT(dev), bdrv_name,
> -                            name, &errp);
> -    if (errp) {
> -        qerror_report_err(errp);
> -        error_free(errp);
> -        return -1;
> -    }
> -    return 0;
> -}
> -
> -void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value)
> -{
> -    if (qdev_prop_set_drive(dev, name, value) < 0) {
> -        exit(1);
> -    }
> -}
> -void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value)
> -{
> -    Error *errp = NULL;
> -    assert(!value || value->label);
> -    object_property_set_str(OBJECT(dev),
> -                            value ? value->label : "", name, &errp);
> -    assert_no_error(errp);
> -}
> -
> -void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value)
> -{
> -    Error *errp = NULL;
> -    assert(!value || value->name);
> -    object_property_set_str(OBJECT(dev),
> -                            value ? value->name : "", name, &errp);
> -    assert_no_error(errp);
> -}
> -
>  void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value)
>  {
>      Error *errp = NULL;
> @@ -1231,7 +929,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
>
>  static QTAILQ_HEAD(, GlobalProperty) global_props = QTAILQ_HEAD_INITIALIZER(global_props);
>
> -static void qdev_prop_register_global(GlobalProperty *prop)
> +void qdev_prop_register_global(GlobalProperty *prop)
>  {
>      QTAILQ_INSERT_TAIL(&global_props, prop, next);
>  }
> @@ -1262,20 +960,3 @@ void qdev_prop_set_globals(DeviceState *dev)
>          class = object_class_get_parent(class);
>      } while (class);
>  }
> -
> -static int qdev_add_one_global(QemuOpts *opts, void *opaque)
> -{
> -    GlobalProperty *g;
> -
> -    g = g_malloc0(sizeof(*g));
> -    g->driver   = qemu_opt_get(opts, "driver");
> -    g->property = qemu_opt_get(opts, "property");
> -    g->value    = qemu_opt_get(opts, "value");
> -    qdev_prop_register_global(g);
> -    return 0;
> -}
> -
> -void qemu_add_globals(void)
> -{
> -    qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
> -}
> diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
> index 5b046ab..ddcf774 100644
> --- a/hw/qdev-properties.h
> +++ b/hw/qdev-properties.h
> @@ -116,6 +116,7 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
>  /* FIXME: Remove opaque pointer properties.  */
>  void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
>
> +void qdev_prop_register_global(GlobalProperty *prop);
>  void qdev_prop_register_global_list(GlobalProperty *props);
>  void qdev_prop_set_globals(DeviceState *dev);
>  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 599382c..fa0af21 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -25,7 +25,6 @@
>     inherit from a particular bus (e.g. PCI or I2C) rather than
>     this API directly.  */
>
> -#include "net.h"
>  #include "qdev.h"
>  #include "sysemu.h"
>  #include "error.h"
> @@ -312,18 +311,6 @@ void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
>      dev->gpio_out[n] = pin;
>  }
>
> -void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
> -{
> -    qdev_prop_set_macaddr(dev, "mac", nd->macaddr.a);
> -    if (nd->netdev)
> -        qdev_prop_set_netdev(dev, "netdev", nd->netdev);
> -    if (nd->nvectors != DEV_NVECTORS_UNSPECIFIED &&
> -        object_property_find(OBJECT(dev), "vectors", NULL)) {
> -        qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
> -    }
> -    nd->instantiated = 1;
> -}
> -
>  BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
>  {
>      BusState *bus;
> --
> 1.7.11.7
>
>

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 6/8] qdev-properties.c: separate core from the code used only by qemu-system-*
  2012-12-04 18:55   ` Blue Swirl
@ 2012-12-04 19:01     ` Eduardo Habkost
  2012-12-04 19:04       ` Blue Swirl
  2012-12-04 19:05       ` Andreas Färber
  0 siblings, 2 replies; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 19:01 UTC (permalink / raw)
  To: Blue Swirl
  Cc: Igor Mammedov, Andreas Färber, qemu-devel, Anthony Liguori,
	Paolo Bonzini

On Tue, Dec 04, 2012 at 06:55:17PM +0000, Blue Swirl wrote:
> On Tue, Dec 4, 2012 at 1:19 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > This separates the qdev properties code in two parts:
> >  - qdev-properties.c, that contains most of the qdev properties code;
> >  - qdev-properties-system.c for code specific for qemu-system-*,
> >    containing:
> >    - Property types: drive, chr, netdev, vlan, that depend on code that
> >      won't be included on *-user
> >    - qemu_add_globals(), that depends on qemu-config.o.
> >
> > This change should help on two things:
> >  - Allowing DeviceState to be used by *-user without pulling
> >    dependencies that are specific for qemu-system-*;
> >  - Writing qdev unit tests without pulling too many dependencies.
> >
> > The copyright/license header for the new file is directly copied from
> > qdev-properties.c.
> >
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> > Detailed changelog:
> >
> > Changes v1 (ehabkost) -> v2 (imammedo):
> >  - keep qdev_get_child_bus() in hw/qdev.c
> >  - put qdev_set_nic_properties() in hw/qdev-properties-system.c
> >
> > Changes v2 -> v3 (ehabkost):
> >  - updated the qdev_init_gpio_in() code on qdev-system.c to current
> >    version
> >
> > Changes v3 -> v4 (ehabkost):
> >  - Added copyright/license information to qdev-properties-system.c
> >    (based on copyright/license of qdev-properties.c)
> >  - Whitespace change at the end of qdev-properties.c
> >  - Don't create qdev-system.c, now we can keep the qdev.c code as-is
> >    as the qdev.c dependencies were reduced
> >  - Rewrite patch description
> >
> > Changes v4 -> v5 (ehabkost):
> >  - Remove large copyright header and instead just point to the original
> >    file it was based on
> >
> > Changes v5 -> v6 (ehabkost):
> >  - Removed inter-SoB line changelog from commit message
> > ---
> >  hw/Makefile.objs            |   1 +
> >  hw/qdev-properties-system.c | 352 ++++++++++++++++++++++++++++++++++++++++++++
> >  hw/qdev-properties.c        | 321 +---------------------------------------
> >  hw/qdev-properties.h        |   1 +
> >  hw/qdev.c                   |  13 --
> >  5 files changed, 355 insertions(+), 333 deletions(-)
> >  create mode 100644 hw/qdev-properties-system.c
> >
> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> > index d581d8d..96a8365 100644
> > --- a/hw/Makefile.objs
> > +++ b/hw/Makefile.objs
> > @@ -185,6 +185,7 @@ common-obj-y += bt.o bt-l2cap.o bt-sdp.o bt-hci.o bt-hid.o
> >  common-obj-y += bt-hci-csr.o
> >  common-obj-y += msmouse.o ps2.o
> >  common-obj-y += qdev.o qdev-properties.o qdev-monitor.o
> > +common-obj-y += qdev-properties-system.o
> >  common-obj-$(CONFIG_BRLAPI) += baum.o
> >
> >  # xen backend driver support
> > diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
> > new file mode 100644
> > index 0000000..9a7e0b3
> > --- /dev/null
> > +++ b/hw/qdev-properties-system.c
> > @@ -0,0 +1,352 @@
> > +/*
> > + * qdev property parsing and global properties
> > + * (parts specific for qemu-system-*)
> > + *
> > + * This file is based on code from hw/qdev-properties.c from
> > + * commit 4e68f7a0819f179c2ff90a60611806c789911cc2,
> > + * Copyright (c) Gerd Hoffmann <kraxel@redhat.com> and other contributors.
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#include "net.h"
> > +#include "qdev.h"
> > +#include "qerror.h"
> > +#include "blockdev.h"
> > +#include "hw/block-common.h"
> > +#include "net/hub.h"
> > +#include "qapi/qapi-visit-core.h"
> > +
> > +static void get_pointer(Object *obj, Visitor *v, Property *prop,
> > +                        const char *(*print)(void *ptr),
> > +                        const char *name, Error **errp)
> > +{
> > +    DeviceState *dev = DEVICE(obj);
> > +    void **ptr = qdev_get_prop_ptr(dev, prop);
> > +    char *p;
> > +
> > +    p = (char *) (*ptr ? print(*ptr) : "");
> > +    visit_type_str(v, &p, name, errp);
> > +}
> > +
> > +static void set_pointer(Object *obj, Visitor *v, Property *prop,
> > +                        int (*parse)(DeviceState *dev, const char *str,
> > +                                     void **ptr),
> > +                        const char *name, Error **errp)
> > +{
> > +    DeviceState *dev = DEVICE(obj);
> > +    Error *local_err = NULL;
> > +    void **ptr = qdev_get_prop_ptr(dev, prop);
> > +    char *str;
> > +    int ret;
> > +
> > +    if (dev->state != DEV_STATE_CREATED) {
> > +        error_set(errp, QERR_PERMISSION_DENIED);
> > +        return;
> > +    }
> > +
> > +    visit_type_str(v, &str, name, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +    if (!*str) {
> > +        g_free(str);
> > +        *ptr = NULL;
> > +        return;
> > +    }
> > +    ret = parse(dev, str, ptr);
> > +    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
> > +    g_free(str);
> > +}
> > +
> > +/* --- drive --- */
> > +
> > +static int parse_drive(DeviceState *dev, const char *str, void **ptr)
> > +{
> > +    BlockDriverState *bs;
> > +
> > +    bs = bdrv_find(str);
> > +    if (bs == NULL)
> 
> Please add braces, use checkpatch.pl.
> 
> > +        return -ENOENT;
> > +    if (bdrv_attach_dev(bs, dev) < 0)
> 
> Also here.


This is pure code movement, and I won't introduce changes in the code
while it is being moved to not make patch review harder.

If you want to send coding style patches for that code after it is
moved, be my guest.


> 
> > +        return -EEXIST;
> > +    *ptr = bs;
> > +    return 0;
> > +}
> > +
> > +static void release_drive(Object *obj, const char *name, void *opaque)
> > +{
> > +    DeviceState *dev = DEVICE(obj);
> > +    Property *prop = opaque;
> > +    BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
> > +
> > +    if (*ptr) {
> > +        bdrv_detach_dev(*ptr, dev);
> > +        blockdev_auto_del(*ptr);
> > +    }
> > +}
> > +
> > +static const char *print_drive(void *ptr)
> > +{
> > +    return bdrv_get_device_name(ptr);
> > +}
> > +
> > +static void get_drive(Object *obj, Visitor *v, void *opaque,
> > +                      const char *name, Error **errp)
> > +{
> > +    get_pointer(obj, v, opaque, print_drive, name, errp);
> > +}
> > +
> > +static void set_drive(Object *obj, Visitor *v, void *opaque,
> > +                      const char *name, Error **errp)
> > +{
> > +    set_pointer(obj, v, opaque, parse_drive, name, errp);
> > +}
> > +
> > +PropertyInfo qdev_prop_drive = {
> > +    .name  = "drive",
> > +    .get   = get_drive,
> > +    .set   = set_drive,
> > +    .release = release_drive,
> > +};
> > +
> > +/* --- character device --- */
> > +
> > +static int parse_chr(DeviceState *dev, const char *str, void **ptr)
> > +{
> > +    CharDriverState *chr = qemu_chr_find(str);
> > +    if (chr == NULL) {
> > +        return -ENOENT;
> > +    }
> > +    if (chr->avail_connections < 1) {
> > +        return -EEXIST;
> > +    }
> > +    *ptr = chr;
> > +    --chr->avail_connections;
> > +    return 0;
> > +}
> > +
> > +static void release_chr(Object *obj, const char *name, void *opaque)
> > +{
> > +    DeviceState *dev = DEVICE(obj);
> > +    Property *prop = opaque;
> > +    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
> > +
> > +    if (*ptr) {
> > +        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
> > +    }
> > +}
> > +
> > +
> > +static const char *print_chr(void *ptr)
> > +{
> > +    CharDriverState *chr = ptr;
> > +
> > +    return chr->label ? chr->label : "";
> > +}
> > +
> > +static void get_chr(Object *obj, Visitor *v, void *opaque,
> > +                    const char *name, Error **errp)
> > +{
> > +    get_pointer(obj, v, opaque, print_chr, name, errp);
> > +}
> > +
> > +static void set_chr(Object *obj, Visitor *v, void *opaque,
> > +                    const char *name, Error **errp)
> > +{
> > +    set_pointer(obj, v, opaque, parse_chr, name, errp);
> > +}
> > +
> > +PropertyInfo qdev_prop_chr = {
> > +    .name  = "chr",
> > +    .get   = get_chr,
> > +    .set   = set_chr,
> > +    .release = release_chr,
> > +};
> > +
> > +/* --- netdev device --- */
> > +
> > +static int parse_netdev(DeviceState *dev, const char *str, void **ptr)
> > +{
> > +    NetClientState *netdev = qemu_find_netdev(str);
> > +
> > +    if (netdev == NULL) {
> > +        return -ENOENT;
> > +    }
> > +    if (netdev->peer) {
> > +        return -EEXIST;
> > +    }
> > +    *ptr = netdev;
> > +    return 0;
> > +}
> > +
> > +static const char *print_netdev(void *ptr)
> > +{
> > +    NetClientState *netdev = ptr;
> > +
> > +    return netdev->name ? netdev->name : "";
> > +}
> > +
> > +static void get_netdev(Object *obj, Visitor *v, void *opaque,
> > +                       const char *name, Error **errp)
> > +{
> > +    get_pointer(obj, v, opaque, print_netdev, name, errp);
> > +}
> > +
> > +static void set_netdev(Object *obj, Visitor *v, void *opaque,
> > +                       const char *name, Error **errp)
> > +{
> > +    set_pointer(obj, v, opaque, parse_netdev, name, errp);
> > +}
> > +
> > +PropertyInfo qdev_prop_netdev = {
> > +    .name  = "netdev",
> > +    .get   = get_netdev,
> > +    .set   = set_netdev,
> > +};
> > +
> > +void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
> > +{
> > +    qdev_prop_set_macaddr(dev, "mac", nd->macaddr.a);
> > +    if (nd->netdev)
> > +        qdev_prop_set_netdev(dev, "netdev", nd->netdev);
> 
> Ditto.
> 
> > +    if (nd->nvectors != DEV_NVECTORS_UNSPECIFIED &&
> > +        object_property_find(OBJECT(dev), "vectors", NULL)) {
> > +        qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
> > +    }
> > +    nd->instantiated = 1;
> > +}
> > +
> > +/* --- vlan --- */
> > +
> > +static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
> > +{
> > +    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> > +
> > +    if (*ptr) {
> > +        int id;
> > +        if (!net_hub_id_for_client(*ptr, &id)) {
> > +            return snprintf(dest, len, "%d", id);
> > +        }
> > +    }
> > +
> > +    return snprintf(dest, len, "<null>");
> > +}
> > +
> > +static void get_vlan(Object *obj, Visitor *v, void *opaque,
> > +                     const char *name, Error **errp)
> > +{
> > +    DeviceState *dev = DEVICE(obj);
> > +    Property *prop = opaque;
> > +    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> > +    int32_t id = -1;
> > +
> > +    if (*ptr) {
> > +        int hub_id;
> > +        if (!net_hub_id_for_client(*ptr, &hub_id)) {
> > +            id = hub_id;
> > +        }
> > +    }
> > +
> > +    visit_type_int32(v, &id, name, errp);
> > +}
> > +
> > +static void set_vlan(Object *obj, Visitor *v, void *opaque,
> > +                     const char *name, Error **errp)
> > +{
> > +    DeviceState *dev = DEVICE(obj);
> > +    Property *prop = opaque;
> > +    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> > +    Error *local_err = NULL;
> > +    int32_t id;
> > +    NetClientState *hubport;
> > +
> > +    if (dev->state != DEV_STATE_CREATED) {
> > +        error_set(errp, QERR_PERMISSION_DENIED);
> > +        return;
> > +    }
> > +
> > +    visit_type_int32(v, &id, name, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +    if (id == -1) {
> > +        *ptr = NULL;
> > +        return;
> > +    }
> > +
> > +    hubport = net_hub_port_find(id);
> > +    if (!hubport) {
> > +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> > +                  name, prop->info->name);
> > +        return;
> > +    }
> > +    *ptr = hubport;
> > +}
> > +
> > +PropertyInfo qdev_prop_vlan = {
> > +    .name  = "vlan",
> > +    .print = print_vlan,
> > +    .get   = get_vlan,
> > +    .set   = set_vlan,
> > +};
> > +
> > +
> > +int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
> > +{
> > +    Error *errp = NULL;
> > +    const char *bdrv_name = value ? bdrv_get_device_name(value) : "";
> > +    object_property_set_str(OBJECT(dev), bdrv_name,
> > +                            name, &errp);
> > +    if (errp) {
> > +        qerror_report_err(errp);
> > +        error_free(errp);
> > +        return -1;
> > +    }
> > +    return 0;
> > +}
> > +
> > +void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value)
> > +{
> > +    if (qdev_prop_set_drive(dev, name, value) < 0) {
> > +        exit(1);
> > +    }
> > +}
> > +
> > +void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value)
> > +{
> > +    Error *errp = NULL;
> > +    assert(!value || value->label);
> > +    object_property_set_str(OBJECT(dev),
> > +                            value ? value->label : "", name, &errp);
> > +    assert_no_error(errp);
> > +}
> > +
> > +void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value)
> > +{
> > +    Error *errp = NULL;
> > +    assert(!value || value->name);
> > +    object_property_set_str(OBJECT(dev),
> > +                            value ? value->name : "", name, &errp);
> > +    assert_no_error(errp);
> > +}
> > +
> > +static int qdev_add_one_global(QemuOpts *opts, void *opaque)
> > +{
> > +    GlobalProperty *g;
> > +
> > +    g = g_malloc0(sizeof(*g));
> > +    g->driver   = qemu_opt_get(opts, "driver");
> > +    g->property = qemu_opt_get(opts, "property");
> > +    g->value    = qemu_opt_get(opts, "value");
> > +    qdev_prop_register_global(g);
> > +    return 0;
> > +}
> > +
> > +void qemu_add_globals(void)
> > +{
> > +    qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
> > +}
> > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> > index 81d901c..9ec04be 100644
> > --- a/hw/qdev-properties.c
> > +++ b/hw/qdev-properties.c
> > @@ -13,49 +13,6 @@ void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
> >      return ptr;
> >  }
> >
> > -static void get_pointer(Object *obj, Visitor *v, Property *prop,
> > -                        const char *(*print)(void *ptr),
> > -                        const char *name, Error **errp)
> > -{
> > -    DeviceState *dev = DEVICE(obj);
> > -    void **ptr = qdev_get_prop_ptr(dev, prop);
> > -    char *p;
> > -
> > -    p = (char *) (*ptr ? print(*ptr) : "");
> > -    visit_type_str(v, &p, name, errp);
> > -}
> > -
> > -static void set_pointer(Object *obj, Visitor *v, Property *prop,
> > -                        int (*parse)(DeviceState *dev, const char *str,
> > -                                     void **ptr),
> > -                        const char *name, Error **errp)
> > -{
> > -    DeviceState *dev = DEVICE(obj);
> > -    Error *local_err = NULL;
> > -    void **ptr = qdev_get_prop_ptr(dev, prop);
> > -    char *str;
> > -    int ret;
> > -
> > -    if (dev->state != DEV_STATE_CREATED) {
> > -        error_set(errp, QERR_PERMISSION_DENIED);
> > -        return;
> > -    }
> > -
> > -    visit_type_str(v, &str, name, &local_err);
> > -    if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        return;
> > -    }
> > -    if (!*str) {
> > -        g_free(str);
> > -        *ptr = NULL;
> > -        return;
> > -    }
> > -    ret = parse(dev, str, ptr);
> > -    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
> > -    g_free(str);
> > -}
> > -
> >  static void get_enum(Object *obj, Visitor *v, void *opaque,
> >                       const char *name, Error **errp)
> >  {
> > @@ -476,227 +433,6 @@ PropertyInfo qdev_prop_string = {
> >      .set   = set_string,
> >  };
> >
> > -/* --- drive --- */
> > -
> > -static int parse_drive(DeviceState *dev, const char *str, void **ptr)
> > -{
> > -    BlockDriverState *bs;
> > -
> > -    bs = bdrv_find(str);
> > -    if (bs == NULL)
> > -        return -ENOENT;
> > -    if (bdrv_attach_dev(bs, dev) < 0)
> > -        return -EEXIST;
> > -    *ptr = bs;
> > -    return 0;
> > -}
> > -
> > -static void release_drive(Object *obj, const char *name, void *opaque)
> > -{
> > -    DeviceState *dev = DEVICE(obj);
> > -    Property *prop = opaque;
> > -    BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
> > -
> > -    if (*ptr) {
> > -        bdrv_detach_dev(*ptr, dev);
> > -        blockdev_auto_del(*ptr);
> > -    }
> > -}
> > -
> > -static const char *print_drive(void *ptr)
> > -{
> > -    return bdrv_get_device_name(ptr);
> > -}
> > -
> > -static void get_drive(Object *obj, Visitor *v, void *opaque,
> > -                      const char *name, Error **errp)
> > -{
> > -    get_pointer(obj, v, opaque, print_drive, name, errp);
> > -}
> > -
> > -static void set_drive(Object *obj, Visitor *v, void *opaque,
> > -                      const char *name, Error **errp)
> > -{
> > -    set_pointer(obj, v, opaque, parse_drive, name, errp);
> > -}
> > -
> > -PropertyInfo qdev_prop_drive = {
> > -    .name  = "drive",
> > -    .get   = get_drive,
> > -    .set   = set_drive,
> > -    .release = release_drive,
> > -};
> > -
> > -/* --- character device --- */
> > -
> > -static int parse_chr(DeviceState *dev, const char *str, void **ptr)
> > -{
> > -    CharDriverState *chr = qemu_chr_find(str);
> > -    if (chr == NULL) {
> > -        return -ENOENT;
> > -    }
> > -    if (chr->avail_connections < 1) {
> > -        return -EEXIST;
> > -    }
> > -    *ptr = chr;
> > -    --chr->avail_connections;
> > -    return 0;
> > -}
> > -
> > -static void release_chr(Object *obj, const char *name, void *opaque)
> > -{
> > -    DeviceState *dev = DEVICE(obj);
> > -    Property *prop = opaque;
> > -    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
> > -
> > -    if (*ptr) {
> > -        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
> > -    }
> > -}
> > -
> > -
> > -static const char *print_chr(void *ptr)
> > -{
> > -    CharDriverState *chr = ptr;
> > -
> > -    return chr->label ? chr->label : "";
> > -}
> > -
> > -static void get_chr(Object *obj, Visitor *v, void *opaque,
> > -                    const char *name, Error **errp)
> > -{
> > -    get_pointer(obj, v, opaque, print_chr, name, errp);
> > -}
> > -
> > -static void set_chr(Object *obj, Visitor *v, void *opaque,
> > -                    const char *name, Error **errp)
> > -{
> > -    set_pointer(obj, v, opaque, parse_chr, name, errp);
> > -}
> > -
> > -PropertyInfo qdev_prop_chr = {
> > -    .name  = "chr",
> > -    .get   = get_chr,
> > -    .set   = set_chr,
> > -    .release = release_chr,
> > -};
> > -
> > -/* --- netdev device --- */
> > -
> > -static int parse_netdev(DeviceState *dev, const char *str, void **ptr)
> > -{
> > -    NetClientState *netdev = qemu_find_netdev(str);
> > -
> > -    if (netdev == NULL) {
> > -        return -ENOENT;
> > -    }
> > -    if (netdev->peer) {
> > -        return -EEXIST;
> > -    }
> > -    *ptr = netdev;
> > -    return 0;
> > -}
> > -
> > -static const char *print_netdev(void *ptr)
> > -{
> > -    NetClientState *netdev = ptr;
> > -
> > -    return netdev->name ? netdev->name : "";
> > -}
> > -
> > -static void get_netdev(Object *obj, Visitor *v, void *opaque,
> > -                       const char *name, Error **errp)
> > -{
> > -    get_pointer(obj, v, opaque, print_netdev, name, errp);
> > -}
> > -
> > -static void set_netdev(Object *obj, Visitor *v, void *opaque,
> > -                       const char *name, Error **errp)
> > -{
> > -    set_pointer(obj, v, opaque, parse_netdev, name, errp);
> > -}
> > -
> > -PropertyInfo qdev_prop_netdev = {
> > -    .name  = "netdev",
> > -    .get   = get_netdev,
> > -    .set   = set_netdev,
> > -};
> > -
> > -/* --- vlan --- */
> > -
> > -static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
> > -{
> > -    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> > -
> > -    if (*ptr) {
> > -        int id;
> > -        if (!net_hub_id_for_client(*ptr, &id)) {
> > -            return snprintf(dest, len, "%d", id);
> > -        }
> > -    }
> > -
> > -    return snprintf(dest, len, "<null>");
> > -}
> > -
> > -static void get_vlan(Object *obj, Visitor *v, void *opaque,
> > -                     const char *name, Error **errp)
> > -{
> > -    DeviceState *dev = DEVICE(obj);
> > -    Property *prop = opaque;
> > -    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> > -    int32_t id = -1;
> > -
> > -    if (*ptr) {
> > -        int hub_id;
> > -        if (!net_hub_id_for_client(*ptr, &hub_id)) {
> > -            id = hub_id;
> > -        }
> > -    }
> > -
> > -    visit_type_int32(v, &id, name, errp);
> > -}
> > -
> > -static void set_vlan(Object *obj, Visitor *v, void *opaque,
> > -                     const char *name, Error **errp)
> > -{
> > -    DeviceState *dev = DEVICE(obj);
> > -    Property *prop = opaque;
> > -    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
> > -    Error *local_err = NULL;
> > -    int32_t id;
> > -    NetClientState *hubport;
> > -
> > -    if (dev->state != DEV_STATE_CREATED) {
> > -        error_set(errp, QERR_PERMISSION_DENIED);
> > -        return;
> > -    }
> > -
> > -    visit_type_int32(v, &id, name, &local_err);
> > -    if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        return;
> > -    }
> > -    if (id == -1) {
> > -        *ptr = NULL;
> > -        return;
> > -    }
> > -
> > -    hubport = net_hub_port_find(id);
> > -    if (!hubport) {
> > -        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> > -                  name, prop->info->name);
> > -        return;
> > -    }
> > -    *ptr = hubport;
> > -}
> > -
> > -PropertyInfo qdev_prop_vlan = {
> > -    .name  = "vlan",
> > -    .print = print_vlan,
> > -    .get   = get_vlan,
> > -    .set   = set_vlan,
> > -};
> > -
> >  /* --- pointer --- */
> >
> >  /* Not a proper property, just for dirty hacks.  TODO Remove it!  */
> > @@ -1158,44 +894,6 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value)
> >      assert_no_error(errp);
> >  }
> >
> > -int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
> > -{
> > -    Error *errp = NULL;
> > -    const char *bdrv_name = value ? bdrv_get_device_name(value) : "";
> > -    object_property_set_str(OBJECT(dev), bdrv_name,
> > -                            name, &errp);
> > -    if (errp) {
> > -        qerror_report_err(errp);
> > -        error_free(errp);
> > -        return -1;
> > -    }
> > -    return 0;
> > -}
> > -
> > -void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value)
> > -{
> > -    if (qdev_prop_set_drive(dev, name, value) < 0) {
> > -        exit(1);
> > -    }
> > -}
> > -void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value)
> > -{
> > -    Error *errp = NULL;
> > -    assert(!value || value->label);
> > -    object_property_set_str(OBJECT(dev),
> > -                            value ? value->label : "", name, &errp);
> > -    assert_no_error(errp);
> > -}
> > -
> > -void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value)
> > -{
> > -    Error *errp = NULL;
> > -    assert(!value || value->name);
> > -    object_property_set_str(OBJECT(dev),
> > -                            value ? value->name : "", name, &errp);
> > -    assert_no_error(errp);
> > -}
> > -
> >  void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value)
> >  {
> >      Error *errp = NULL;
> > @@ -1231,7 +929,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
> >
> >  static QTAILQ_HEAD(, GlobalProperty) global_props = QTAILQ_HEAD_INITIALIZER(global_props);
> >
> > -static void qdev_prop_register_global(GlobalProperty *prop)
> > +void qdev_prop_register_global(GlobalProperty *prop)
> >  {
> >      QTAILQ_INSERT_TAIL(&global_props, prop, next);
> >  }
> > @@ -1262,20 +960,3 @@ void qdev_prop_set_globals(DeviceState *dev)
> >          class = object_class_get_parent(class);
> >      } while (class);
> >  }
> > -
> > -static int qdev_add_one_global(QemuOpts *opts, void *opaque)
> > -{
> > -    GlobalProperty *g;
> > -
> > -    g = g_malloc0(sizeof(*g));
> > -    g->driver   = qemu_opt_get(opts, "driver");
> > -    g->property = qemu_opt_get(opts, "property");
> > -    g->value    = qemu_opt_get(opts, "value");
> > -    qdev_prop_register_global(g);
> > -    return 0;
> > -}
> > -
> > -void qemu_add_globals(void)
> > -{
> > -    qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
> > -}
> > diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
> > index 5b046ab..ddcf774 100644
> > --- a/hw/qdev-properties.h
> > +++ b/hw/qdev-properties.h
> > @@ -116,6 +116,7 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
> >  /* FIXME: Remove opaque pointer properties.  */
> >  void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
> >
> > +void qdev_prop_register_global(GlobalProperty *prop);
> >  void qdev_prop_register_global_list(GlobalProperty *props);
> >  void qdev_prop_set_globals(DeviceState *dev);
> >  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index 599382c..fa0af21 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> > @@ -25,7 +25,6 @@
> >     inherit from a particular bus (e.g. PCI or I2C) rather than
> >     this API directly.  */
> >
> > -#include "net.h"
> >  #include "qdev.h"
> >  #include "sysemu.h"
> >  #include "error.h"
> > @@ -312,18 +311,6 @@ void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
> >      dev->gpio_out[n] = pin;
> >  }
> >
> > -void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
> > -{
> > -    qdev_prop_set_macaddr(dev, "mac", nd->macaddr.a);
> > -    if (nd->netdev)
> > -        qdev_prop_set_netdev(dev, "netdev", nd->netdev);
> > -    if (nd->nvectors != DEV_NVECTORS_UNSPECIFIED &&
> > -        object_property_find(OBJECT(dev), "vectors", NULL)) {
> > -        qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
> > -    }
> > -    nd->instantiated = 1;
> > -}
> > -
> >  BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
> >  {
> >      BusState *bus;
> > --
> > 1.7.11.7
> >
> >
> 

-- 
Eduardo

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 6/8] qdev-properties.c: separate core from the code used only by qemu-system-*
  2012-12-04 19:01     ` Eduardo Habkost
@ 2012-12-04 19:04       ` Blue Swirl
  2012-12-04 19:05       ` Andreas Färber
  1 sibling, 0 replies; 22+ messages in thread
From: Blue Swirl @ 2012-12-04 19:04 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Igor Mammedov, Andreas Färber, qemu-devel, Anthony Liguori,
	Paolo Bonzini

On Tue, Dec 4, 2012 at 7:01 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Dec 04, 2012 at 06:55:17PM +0000, Blue Swirl wrote:
>> On Tue, Dec 4, 2012 at 1:19 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> > This separates the qdev properties code in two parts:
>> >  - qdev-properties.c, that contains most of the qdev properties code;
>> >  - qdev-properties-system.c for code specific for qemu-system-*,
>> >    containing:
>> >    - Property types: drive, chr, netdev, vlan, that depend on code that
>> >      won't be included on *-user
>> >    - qemu_add_globals(), that depends on qemu-config.o.
>> >
>> > This change should help on two things:
>> >  - Allowing DeviceState to be used by *-user without pulling
>> >    dependencies that are specific for qemu-system-*;
>> >  - Writing qdev unit tests without pulling too many dependencies.
>> >
>> > The copyright/license header for the new file is directly copied from
>> > qdev-properties.c.
>> >
>> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> > ---
>> > Detailed changelog:
>> >
>> > Changes v1 (ehabkost) -> v2 (imammedo):
>> >  - keep qdev_get_child_bus() in hw/qdev.c
>> >  - put qdev_set_nic_properties() in hw/qdev-properties-system.c
>> >
>> > Changes v2 -> v3 (ehabkost):
>> >  - updated the qdev_init_gpio_in() code on qdev-system.c to current
>> >    version
>> >
>> > Changes v3 -> v4 (ehabkost):
>> >  - Added copyright/license information to qdev-properties-system.c
>> >    (based on copyright/license of qdev-properties.c)
>> >  - Whitespace change at the end of qdev-properties.c
>> >  - Don't create qdev-system.c, now we can keep the qdev.c code as-is
>> >    as the qdev.c dependencies were reduced
>> >  - Rewrite patch description
>> >
>> > Changes v4 -> v5 (ehabkost):
>> >  - Remove large copyright header and instead just point to the original
>> >    file it was based on
>> >
>> > Changes v5 -> v6 (ehabkost):
>> >  - Removed inter-SoB line changelog from commit message
>> > ---
>> >  hw/Makefile.objs            |   1 +
>> >  hw/qdev-properties-system.c | 352 ++++++++++++++++++++++++++++++++++++++++++++
>> >  hw/qdev-properties.c        | 321 +---------------------------------------
>> >  hw/qdev-properties.h        |   1 +
>> >  hw/qdev.c                   |  13 --
>> >  5 files changed, 355 insertions(+), 333 deletions(-)
>> >  create mode 100644 hw/qdev-properties-system.c
>> >
>> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs
>> > index d581d8d..96a8365 100644
>> > --- a/hw/Makefile.objs
>> > +++ b/hw/Makefile.objs
>> > @@ -185,6 +185,7 @@ common-obj-y += bt.o bt-l2cap.o bt-sdp.o bt-hci.o bt-hid.o
>> >  common-obj-y += bt-hci-csr.o
>> >  common-obj-y += msmouse.o ps2.o
>> >  common-obj-y += qdev.o qdev-properties.o qdev-monitor.o
>> > +common-obj-y += qdev-properties-system.o
>> >  common-obj-$(CONFIG_BRLAPI) += baum.o
>> >
>> >  # xen backend driver support
>> > diff --git a/hw/qdev-properties-system.c b/hw/qdev-properties-system.c
>> > new file mode 100644
>> > index 0000000..9a7e0b3
>> > --- /dev/null
>> > +++ b/hw/qdev-properties-system.c
>> > @@ -0,0 +1,352 @@
>> > +/*
>> > + * qdev property parsing and global properties
>> > + * (parts specific for qemu-system-*)
>> > + *
>> > + * This file is based on code from hw/qdev-properties.c from
>> > + * commit 4e68f7a0819f179c2ff90a60611806c789911cc2,
>> > + * Copyright (c) Gerd Hoffmann <kraxel@redhat.com> and other contributors.
>> > + *
>> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> > + * See the COPYING file in the top-level directory.
>> > + */
>> > +
>> > +#include "net.h"
>> > +#include "qdev.h"
>> > +#include "qerror.h"
>> > +#include "blockdev.h"
>> > +#include "hw/block-common.h"
>> > +#include "net/hub.h"
>> > +#include "qapi/qapi-visit-core.h"
>> > +
>> > +static void get_pointer(Object *obj, Visitor *v, Property *prop,
>> > +                        const char *(*print)(void *ptr),
>> > +                        const char *name, Error **errp)
>> > +{
>> > +    DeviceState *dev = DEVICE(obj);
>> > +    void **ptr = qdev_get_prop_ptr(dev, prop);
>> > +    char *p;
>> > +
>> > +    p = (char *) (*ptr ? print(*ptr) : "");
>> > +    visit_type_str(v, &p, name, errp);
>> > +}
>> > +
>> > +static void set_pointer(Object *obj, Visitor *v, Property *prop,
>> > +                        int (*parse)(DeviceState *dev, const char *str,
>> > +                                     void **ptr),
>> > +                        const char *name, Error **errp)
>> > +{
>> > +    DeviceState *dev = DEVICE(obj);
>> > +    Error *local_err = NULL;
>> > +    void **ptr = qdev_get_prop_ptr(dev, prop);
>> > +    char *str;
>> > +    int ret;
>> > +
>> > +    if (dev->state != DEV_STATE_CREATED) {
>> > +        error_set(errp, QERR_PERMISSION_DENIED);
>> > +        return;
>> > +    }
>> > +
>> > +    visit_type_str(v, &str, name, &local_err);
>> > +    if (local_err) {
>> > +        error_propagate(errp, local_err);
>> > +        return;
>> > +    }
>> > +    if (!*str) {
>> > +        g_free(str);
>> > +        *ptr = NULL;
>> > +        return;
>> > +    }
>> > +    ret = parse(dev, str, ptr);
>> > +    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
>> > +    g_free(str);
>> > +}
>> > +
>> > +/* --- drive --- */
>> > +
>> > +static int parse_drive(DeviceState *dev, const char *str, void **ptr)
>> > +{
>> > +    BlockDriverState *bs;
>> > +
>> > +    bs = bdrv_find(str);
>> > +    if (bs == NULL)
>>
>> Please add braces, use checkpatch.pl.
>>
>> > +        return -ENOENT;
>> > +    if (bdrv_attach_dev(bs, dev) < 0)
>>
>> Also here.
>
>
> This is pure code movement, and I won't introduce changes in the code
> while it is being moved to not make patch review harder.

That's OK.

> If you want to send coding style patches for that code after it is
> moved, be my guest.

But that would not be OK because git blame information would be lost,
otherwise we could just reformat everything.

Please fix the code first as the first patch, then move.

>
>
>>
>> > +        return -EEXIST;
>> > +    *ptr = bs;
>> > +    return 0;
>> > +}
>> > +
>> > +static void release_drive(Object *obj, const char *name, void *opaque)
>> > +{
>> > +    DeviceState *dev = DEVICE(obj);
>> > +    Property *prop = opaque;
>> > +    BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>> > +
>> > +    if (*ptr) {
>> > +        bdrv_detach_dev(*ptr, dev);
>> > +        blockdev_auto_del(*ptr);
>> > +    }
>> > +}
>> > +
>> > +static const char *print_drive(void *ptr)
>> > +{
>> > +    return bdrv_get_device_name(ptr);
>> > +}
>> > +
>> > +static void get_drive(Object *obj, Visitor *v, void *opaque,
>> > +                      const char *name, Error **errp)
>> > +{
>> > +    get_pointer(obj, v, opaque, print_drive, name, errp);
>> > +}
>> > +
>> > +static void set_drive(Object *obj, Visitor *v, void *opaque,
>> > +                      const char *name, Error **errp)
>> > +{
>> > +    set_pointer(obj, v, opaque, parse_drive, name, errp);
>> > +}
>> > +
>> > +PropertyInfo qdev_prop_drive = {
>> > +    .name  = "drive",
>> > +    .get   = get_drive,
>> > +    .set   = set_drive,
>> > +    .release = release_drive,
>> > +};
>> > +
>> > +/* --- character device --- */
>> > +
>> > +static int parse_chr(DeviceState *dev, const char *str, void **ptr)
>> > +{
>> > +    CharDriverState *chr = qemu_chr_find(str);
>> > +    if (chr == NULL) {
>> > +        return -ENOENT;
>> > +    }
>> > +    if (chr->avail_connections < 1) {
>> > +        return -EEXIST;
>> > +    }
>> > +    *ptr = chr;
>> > +    --chr->avail_connections;
>> > +    return 0;
>> > +}
>> > +
>> > +static void release_chr(Object *obj, const char *name, void *opaque)
>> > +{
>> > +    DeviceState *dev = DEVICE(obj);
>> > +    Property *prop = opaque;
>> > +    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>> > +
>> > +    if (*ptr) {
>> > +        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
>> > +    }
>> > +}
>> > +
>> > +
>> > +static const char *print_chr(void *ptr)
>> > +{
>> > +    CharDriverState *chr = ptr;
>> > +
>> > +    return chr->label ? chr->label : "";
>> > +}
>> > +
>> > +static void get_chr(Object *obj, Visitor *v, void *opaque,
>> > +                    const char *name, Error **errp)
>> > +{
>> > +    get_pointer(obj, v, opaque, print_chr, name, errp);
>> > +}
>> > +
>> > +static void set_chr(Object *obj, Visitor *v, void *opaque,
>> > +                    const char *name, Error **errp)
>> > +{
>> > +    set_pointer(obj, v, opaque, parse_chr, name, errp);
>> > +}
>> > +
>> > +PropertyInfo qdev_prop_chr = {
>> > +    .name  = "chr",
>> > +    .get   = get_chr,
>> > +    .set   = set_chr,
>> > +    .release = release_chr,
>> > +};
>> > +
>> > +/* --- netdev device --- */
>> > +
>> > +static int parse_netdev(DeviceState *dev, const char *str, void **ptr)
>> > +{
>> > +    NetClientState *netdev = qemu_find_netdev(str);
>> > +
>> > +    if (netdev == NULL) {
>> > +        return -ENOENT;
>> > +    }
>> > +    if (netdev->peer) {
>> > +        return -EEXIST;
>> > +    }
>> > +    *ptr = netdev;
>> > +    return 0;
>> > +}
>> > +
>> > +static const char *print_netdev(void *ptr)
>> > +{
>> > +    NetClientState *netdev = ptr;
>> > +
>> > +    return netdev->name ? netdev->name : "";
>> > +}
>> > +
>> > +static void get_netdev(Object *obj, Visitor *v, void *opaque,
>> > +                       const char *name, Error **errp)
>> > +{
>> > +    get_pointer(obj, v, opaque, print_netdev, name, errp);
>> > +}
>> > +
>> > +static void set_netdev(Object *obj, Visitor *v, void *opaque,
>> > +                       const char *name, Error **errp)
>> > +{
>> > +    set_pointer(obj, v, opaque, parse_netdev, name, errp);
>> > +}
>> > +
>> > +PropertyInfo qdev_prop_netdev = {
>> > +    .name  = "netdev",
>> > +    .get   = get_netdev,
>> > +    .set   = set_netdev,
>> > +};
>> > +
>> > +void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
>> > +{
>> > +    qdev_prop_set_macaddr(dev, "mac", nd->macaddr.a);
>> > +    if (nd->netdev)
>> > +        qdev_prop_set_netdev(dev, "netdev", nd->netdev);
>>
>> Ditto.
>>
>> > +    if (nd->nvectors != DEV_NVECTORS_UNSPECIFIED &&
>> > +        object_property_find(OBJECT(dev), "vectors", NULL)) {
>> > +        qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
>> > +    }
>> > +    nd->instantiated = 1;
>> > +}
>> > +
>> > +/* --- vlan --- */
>> > +
>> > +static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
>> > +{
>> > +    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
>> > +
>> > +    if (*ptr) {
>> > +        int id;
>> > +        if (!net_hub_id_for_client(*ptr, &id)) {
>> > +            return snprintf(dest, len, "%d", id);
>> > +        }
>> > +    }
>> > +
>> > +    return snprintf(dest, len, "<null>");
>> > +}
>> > +
>> > +static void get_vlan(Object *obj, Visitor *v, void *opaque,
>> > +                     const char *name, Error **errp)
>> > +{
>> > +    DeviceState *dev = DEVICE(obj);
>> > +    Property *prop = opaque;
>> > +    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
>> > +    int32_t id = -1;
>> > +
>> > +    if (*ptr) {
>> > +        int hub_id;
>> > +        if (!net_hub_id_for_client(*ptr, &hub_id)) {
>> > +            id = hub_id;
>> > +        }
>> > +    }
>> > +
>> > +    visit_type_int32(v, &id, name, errp);
>> > +}
>> > +
>> > +static void set_vlan(Object *obj, Visitor *v, void *opaque,
>> > +                     const char *name, Error **errp)
>> > +{
>> > +    DeviceState *dev = DEVICE(obj);
>> > +    Property *prop = opaque;
>> > +    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
>> > +    Error *local_err = NULL;
>> > +    int32_t id;
>> > +    NetClientState *hubport;
>> > +
>> > +    if (dev->state != DEV_STATE_CREATED) {
>> > +        error_set(errp, QERR_PERMISSION_DENIED);
>> > +        return;
>> > +    }
>> > +
>> > +    visit_type_int32(v, &id, name, &local_err);
>> > +    if (local_err) {
>> > +        error_propagate(errp, local_err);
>> > +        return;
>> > +    }
>> > +    if (id == -1) {
>> > +        *ptr = NULL;
>> > +        return;
>> > +    }
>> > +
>> > +    hubport = net_hub_port_find(id);
>> > +    if (!hubport) {
>> > +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> > +                  name, prop->info->name);
>> > +        return;
>> > +    }
>> > +    *ptr = hubport;
>> > +}
>> > +
>> > +PropertyInfo qdev_prop_vlan = {
>> > +    .name  = "vlan",
>> > +    .print = print_vlan,
>> > +    .get   = get_vlan,
>> > +    .set   = set_vlan,
>> > +};
>> > +
>> > +
>> > +int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
>> > +{
>> > +    Error *errp = NULL;
>> > +    const char *bdrv_name = value ? bdrv_get_device_name(value) : "";
>> > +    object_property_set_str(OBJECT(dev), bdrv_name,
>> > +                            name, &errp);
>> > +    if (errp) {
>> > +        qerror_report_err(errp);
>> > +        error_free(errp);
>> > +        return -1;
>> > +    }
>> > +    return 0;
>> > +}
>> > +
>> > +void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value)
>> > +{
>> > +    if (qdev_prop_set_drive(dev, name, value) < 0) {
>> > +        exit(1);
>> > +    }
>> > +}
>> > +
>> > +void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value)
>> > +{
>> > +    Error *errp = NULL;
>> > +    assert(!value || value->label);
>> > +    object_property_set_str(OBJECT(dev),
>> > +                            value ? value->label : "", name, &errp);
>> > +    assert_no_error(errp);
>> > +}
>> > +
>> > +void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value)
>> > +{
>> > +    Error *errp = NULL;
>> > +    assert(!value || value->name);
>> > +    object_property_set_str(OBJECT(dev),
>> > +                            value ? value->name : "", name, &errp);
>> > +    assert_no_error(errp);
>> > +}
>> > +
>> > +static int qdev_add_one_global(QemuOpts *opts, void *opaque)
>> > +{
>> > +    GlobalProperty *g;
>> > +
>> > +    g = g_malloc0(sizeof(*g));
>> > +    g->driver   = qemu_opt_get(opts, "driver");
>> > +    g->property = qemu_opt_get(opts, "property");
>> > +    g->value    = qemu_opt_get(opts, "value");
>> > +    qdev_prop_register_global(g);
>> > +    return 0;
>> > +}
>> > +
>> > +void qemu_add_globals(void)
>> > +{
>> > +    qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
>> > +}
>> > diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
>> > index 81d901c..9ec04be 100644
>> > --- a/hw/qdev-properties.c
>> > +++ b/hw/qdev-properties.c
>> > @@ -13,49 +13,6 @@ void *qdev_get_prop_ptr(DeviceState *dev, Property *prop)
>> >      return ptr;
>> >  }
>> >
>> > -static void get_pointer(Object *obj, Visitor *v, Property *prop,
>> > -                        const char *(*print)(void *ptr),
>> > -                        const char *name, Error **errp)
>> > -{
>> > -    DeviceState *dev = DEVICE(obj);
>> > -    void **ptr = qdev_get_prop_ptr(dev, prop);
>> > -    char *p;
>> > -
>> > -    p = (char *) (*ptr ? print(*ptr) : "");
>> > -    visit_type_str(v, &p, name, errp);
>> > -}
>> > -
>> > -static void set_pointer(Object *obj, Visitor *v, Property *prop,
>> > -                        int (*parse)(DeviceState *dev, const char *str,
>> > -                                     void **ptr),
>> > -                        const char *name, Error **errp)
>> > -{
>> > -    DeviceState *dev = DEVICE(obj);
>> > -    Error *local_err = NULL;
>> > -    void **ptr = qdev_get_prop_ptr(dev, prop);
>> > -    char *str;
>> > -    int ret;
>> > -
>> > -    if (dev->state != DEV_STATE_CREATED) {
>> > -        error_set(errp, QERR_PERMISSION_DENIED);
>> > -        return;
>> > -    }
>> > -
>> > -    visit_type_str(v, &str, name, &local_err);
>> > -    if (local_err) {
>> > -        error_propagate(errp, local_err);
>> > -        return;
>> > -    }
>> > -    if (!*str) {
>> > -        g_free(str);
>> > -        *ptr = NULL;
>> > -        return;
>> > -    }
>> > -    ret = parse(dev, str, ptr);
>> > -    error_set_from_qdev_prop_error(errp, ret, dev, prop, str);
>> > -    g_free(str);
>> > -}
>> > -
>> >  static void get_enum(Object *obj, Visitor *v, void *opaque,
>> >                       const char *name, Error **errp)
>> >  {
>> > @@ -476,227 +433,6 @@ PropertyInfo qdev_prop_string = {
>> >      .set   = set_string,
>> >  };
>> >
>> > -/* --- drive --- */
>> > -
>> > -static int parse_drive(DeviceState *dev, const char *str, void **ptr)
>> > -{
>> > -    BlockDriverState *bs;
>> > -
>> > -    bs = bdrv_find(str);
>> > -    if (bs == NULL)
>> > -        return -ENOENT;
>> > -    if (bdrv_attach_dev(bs, dev) < 0)
>> > -        return -EEXIST;
>> > -    *ptr = bs;
>> > -    return 0;
>> > -}
>> > -
>> > -static void release_drive(Object *obj, const char *name, void *opaque)
>> > -{
>> > -    DeviceState *dev = DEVICE(obj);
>> > -    Property *prop = opaque;
>> > -    BlockDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>> > -
>> > -    if (*ptr) {
>> > -        bdrv_detach_dev(*ptr, dev);
>> > -        blockdev_auto_del(*ptr);
>> > -    }
>> > -}
>> > -
>> > -static const char *print_drive(void *ptr)
>> > -{
>> > -    return bdrv_get_device_name(ptr);
>> > -}
>> > -
>> > -static void get_drive(Object *obj, Visitor *v, void *opaque,
>> > -                      const char *name, Error **errp)
>> > -{
>> > -    get_pointer(obj, v, opaque, print_drive, name, errp);
>> > -}
>> > -
>> > -static void set_drive(Object *obj, Visitor *v, void *opaque,
>> > -                      const char *name, Error **errp)
>> > -{
>> > -    set_pointer(obj, v, opaque, parse_drive, name, errp);
>> > -}
>> > -
>> > -PropertyInfo qdev_prop_drive = {
>> > -    .name  = "drive",
>> > -    .get   = get_drive,
>> > -    .set   = set_drive,
>> > -    .release = release_drive,
>> > -};
>> > -
>> > -/* --- character device --- */
>> > -
>> > -static int parse_chr(DeviceState *dev, const char *str, void **ptr)
>> > -{
>> > -    CharDriverState *chr = qemu_chr_find(str);
>> > -    if (chr == NULL) {
>> > -        return -ENOENT;
>> > -    }
>> > -    if (chr->avail_connections < 1) {
>> > -        return -EEXIST;
>> > -    }
>> > -    *ptr = chr;
>> > -    --chr->avail_connections;
>> > -    return 0;
>> > -}
>> > -
>> > -static void release_chr(Object *obj, const char *name, void *opaque)
>> > -{
>> > -    DeviceState *dev = DEVICE(obj);
>> > -    Property *prop = opaque;
>> > -    CharDriverState **ptr = qdev_get_prop_ptr(dev, prop);
>> > -
>> > -    if (*ptr) {
>> > -        qemu_chr_add_handlers(*ptr, NULL, NULL, NULL, NULL);
>> > -    }
>> > -}
>> > -
>> > -
>> > -static const char *print_chr(void *ptr)
>> > -{
>> > -    CharDriverState *chr = ptr;
>> > -
>> > -    return chr->label ? chr->label : "";
>> > -}
>> > -
>> > -static void get_chr(Object *obj, Visitor *v, void *opaque,
>> > -                    const char *name, Error **errp)
>> > -{
>> > -    get_pointer(obj, v, opaque, print_chr, name, errp);
>> > -}
>> > -
>> > -static void set_chr(Object *obj, Visitor *v, void *opaque,
>> > -                    const char *name, Error **errp)
>> > -{
>> > -    set_pointer(obj, v, opaque, parse_chr, name, errp);
>> > -}
>> > -
>> > -PropertyInfo qdev_prop_chr = {
>> > -    .name  = "chr",
>> > -    .get   = get_chr,
>> > -    .set   = set_chr,
>> > -    .release = release_chr,
>> > -};
>> > -
>> > -/* --- netdev device --- */
>> > -
>> > -static int parse_netdev(DeviceState *dev, const char *str, void **ptr)
>> > -{
>> > -    NetClientState *netdev = qemu_find_netdev(str);
>> > -
>> > -    if (netdev == NULL) {
>> > -        return -ENOENT;
>> > -    }
>> > -    if (netdev->peer) {
>> > -        return -EEXIST;
>> > -    }
>> > -    *ptr = netdev;
>> > -    return 0;
>> > -}
>> > -
>> > -static const char *print_netdev(void *ptr)
>> > -{
>> > -    NetClientState *netdev = ptr;
>> > -
>> > -    return netdev->name ? netdev->name : "";
>> > -}
>> > -
>> > -static void get_netdev(Object *obj, Visitor *v, void *opaque,
>> > -                       const char *name, Error **errp)
>> > -{
>> > -    get_pointer(obj, v, opaque, print_netdev, name, errp);
>> > -}
>> > -
>> > -static void set_netdev(Object *obj, Visitor *v, void *opaque,
>> > -                       const char *name, Error **errp)
>> > -{
>> > -    set_pointer(obj, v, opaque, parse_netdev, name, errp);
>> > -}
>> > -
>> > -PropertyInfo qdev_prop_netdev = {
>> > -    .name  = "netdev",
>> > -    .get   = get_netdev,
>> > -    .set   = set_netdev,
>> > -};
>> > -
>> > -/* --- vlan --- */
>> > -
>> > -static int print_vlan(DeviceState *dev, Property *prop, char *dest, size_t len)
>> > -{
>> > -    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
>> > -
>> > -    if (*ptr) {
>> > -        int id;
>> > -        if (!net_hub_id_for_client(*ptr, &id)) {
>> > -            return snprintf(dest, len, "%d", id);
>> > -        }
>> > -    }
>> > -
>> > -    return snprintf(dest, len, "<null>");
>> > -}
>> > -
>> > -static void get_vlan(Object *obj, Visitor *v, void *opaque,
>> > -                     const char *name, Error **errp)
>> > -{
>> > -    DeviceState *dev = DEVICE(obj);
>> > -    Property *prop = opaque;
>> > -    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
>> > -    int32_t id = -1;
>> > -
>> > -    if (*ptr) {
>> > -        int hub_id;
>> > -        if (!net_hub_id_for_client(*ptr, &hub_id)) {
>> > -            id = hub_id;
>> > -        }
>> > -    }
>> > -
>> > -    visit_type_int32(v, &id, name, errp);
>> > -}
>> > -
>> > -static void set_vlan(Object *obj, Visitor *v, void *opaque,
>> > -                     const char *name, Error **errp)
>> > -{
>> > -    DeviceState *dev = DEVICE(obj);
>> > -    Property *prop = opaque;
>> > -    NetClientState **ptr = qdev_get_prop_ptr(dev, prop);
>> > -    Error *local_err = NULL;
>> > -    int32_t id;
>> > -    NetClientState *hubport;
>> > -
>> > -    if (dev->state != DEV_STATE_CREATED) {
>> > -        error_set(errp, QERR_PERMISSION_DENIED);
>> > -        return;
>> > -    }
>> > -
>> > -    visit_type_int32(v, &id, name, &local_err);
>> > -    if (local_err) {
>> > -        error_propagate(errp, local_err);
>> > -        return;
>> > -    }
>> > -    if (id == -1) {
>> > -        *ptr = NULL;
>> > -        return;
>> > -    }
>> > -
>> > -    hubport = net_hub_port_find(id);
>> > -    if (!hubport) {
>> > -        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> > -                  name, prop->info->name);
>> > -        return;
>> > -    }
>> > -    *ptr = hubport;
>> > -}
>> > -
>> > -PropertyInfo qdev_prop_vlan = {
>> > -    .name  = "vlan",
>> > -    .print = print_vlan,
>> > -    .get   = get_vlan,
>> > -    .set   = set_vlan,
>> > -};
>> > -
>> >  /* --- pointer --- */
>> >
>> >  /* Not a proper property, just for dirty hacks.  TODO Remove it!  */
>> > @@ -1158,44 +894,6 @@ void qdev_prop_set_string(DeviceState *dev, const char *name, const char *value)
>> >      assert_no_error(errp);
>> >  }
>> >
>> > -int qdev_prop_set_drive(DeviceState *dev, const char *name, BlockDriverState *value)
>> > -{
>> > -    Error *errp = NULL;
>> > -    const char *bdrv_name = value ? bdrv_get_device_name(value) : "";
>> > -    object_property_set_str(OBJECT(dev), bdrv_name,
>> > -                            name, &errp);
>> > -    if (errp) {
>> > -        qerror_report_err(errp);
>> > -        error_free(errp);
>> > -        return -1;
>> > -    }
>> > -    return 0;
>> > -}
>> > -
>> > -void qdev_prop_set_drive_nofail(DeviceState *dev, const char *name, BlockDriverState *value)
>> > -{
>> > -    if (qdev_prop_set_drive(dev, name, value) < 0) {
>> > -        exit(1);
>> > -    }
>> > -}
>> > -void qdev_prop_set_chr(DeviceState *dev, const char *name, CharDriverState *value)
>> > -{
>> > -    Error *errp = NULL;
>> > -    assert(!value || value->label);
>> > -    object_property_set_str(OBJECT(dev),
>> > -                            value ? value->label : "", name, &errp);
>> > -    assert_no_error(errp);
>> > -}
>> > -
>> > -void qdev_prop_set_netdev(DeviceState *dev, const char *name, NetClientState *value)
>> > -{
>> > -    Error *errp = NULL;
>> > -    assert(!value || value->name);
>> > -    object_property_set_str(OBJECT(dev),
>> > -                            value ? value->name : "", name, &errp);
>> > -    assert_no_error(errp);
>> > -}
>> > -
>> >  void qdev_prop_set_macaddr(DeviceState *dev, const char *name, uint8_t *value)
>> >  {
>> >      Error *errp = NULL;
>> > @@ -1231,7 +929,7 @@ void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value)
>> >
>> >  static QTAILQ_HEAD(, GlobalProperty) global_props = QTAILQ_HEAD_INITIALIZER(global_props);
>> >
>> > -static void qdev_prop_register_global(GlobalProperty *prop)
>> > +void qdev_prop_register_global(GlobalProperty *prop)
>> >  {
>> >      QTAILQ_INSERT_TAIL(&global_props, prop, next);
>> >  }
>> > @@ -1262,20 +960,3 @@ void qdev_prop_set_globals(DeviceState *dev)
>> >          class = object_class_get_parent(class);
>> >      } while (class);
>> >  }
>> > -
>> > -static int qdev_add_one_global(QemuOpts *opts, void *opaque)
>> > -{
>> > -    GlobalProperty *g;
>> > -
>> > -    g = g_malloc0(sizeof(*g));
>> > -    g->driver   = qemu_opt_get(opts, "driver");
>> > -    g->property = qemu_opt_get(opts, "property");
>> > -    g->value    = qemu_opt_get(opts, "value");
>> > -    qdev_prop_register_global(g);
>> > -    return 0;
>> > -}
>> > -
>> > -void qemu_add_globals(void)
>> > -{
>> > -    qemu_opts_foreach(qemu_find_opts("global"), qdev_add_one_global, NULL, 0);
>> > -}
>> > diff --git a/hw/qdev-properties.h b/hw/qdev-properties.h
>> > index 5b046ab..ddcf774 100644
>> > --- a/hw/qdev-properties.h
>> > +++ b/hw/qdev-properties.h
>> > @@ -116,6 +116,7 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
>> >  /* FIXME: Remove opaque pointer properties.  */
>> >  void qdev_prop_set_ptr(DeviceState *dev, const char *name, void *value);
>> >
>> > +void qdev_prop_register_global(GlobalProperty *prop);
>> >  void qdev_prop_register_global_list(GlobalProperty *props);
>> >  void qdev_prop_set_globals(DeviceState *dev);
>> >  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
>> > diff --git a/hw/qdev.c b/hw/qdev.c
>> > index 599382c..fa0af21 100644
>> > --- a/hw/qdev.c
>> > +++ b/hw/qdev.c
>> > @@ -25,7 +25,6 @@
>> >     inherit from a particular bus (e.g. PCI or I2C) rather than
>> >     this API directly.  */
>> >
>> > -#include "net.h"
>> >  #include "qdev.h"
>> >  #include "sysemu.h"
>> >  #include "error.h"
>> > @@ -312,18 +311,6 @@ void qdev_connect_gpio_out(DeviceState * dev, int n, qemu_irq pin)
>> >      dev->gpio_out[n] = pin;
>> >  }
>> >
>> > -void qdev_set_nic_properties(DeviceState *dev, NICInfo *nd)
>> > -{
>> > -    qdev_prop_set_macaddr(dev, "mac", nd->macaddr.a);
>> > -    if (nd->netdev)
>> > -        qdev_prop_set_netdev(dev, "netdev", nd->netdev);
>> > -    if (nd->nvectors != DEV_NVECTORS_UNSPECIFIED &&
>> > -        object_property_find(OBJECT(dev), "vectors", NULL)) {
>> > -        qdev_prop_set_uint32(dev, "vectors", nd->nvectors);
>> > -    }
>> > -    nd->instantiated = 1;
>> > -}
>> > -
>> >  BusState *qdev_get_child_bus(DeviceState *dev, const char *name)
>> >  {
>> >      BusState *bus;
>> > --
>> > 1.7.11.7
>> >
>> >
>>
>
> --
> Eduardo

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 6/8] qdev-properties.c: separate core from the code used only by qemu-system-*
  2012-12-04 19:01     ` Eduardo Habkost
  2012-12-04 19:04       ` Blue Swirl
@ 2012-12-04 19:05       ` Andreas Färber
  2012-12-04 19:13         ` Eduardo Habkost
  1 sibling, 1 reply; 22+ messages in thread
From: Andreas Färber @ 2012-12-04 19:05 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Blue Swirl, Igor Mammedov, qemu-devel, Anthony Liguori, Paolo Bonzini

Am 04.12.2012 20:01, schrieb Eduardo Habkost:
> On Tue, Dec 04, 2012 at 06:55:17PM +0000, Blue Swirl wrote:
>> On Tue, Dec 4, 2012 at 1:19 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> +static int parse_drive(DeviceState *dev, const char *str, void **ptr)
>>> +{
>>> +    BlockDriverState *bs;
>>> +
>>> +    bs = bdrv_find(str);
>>> +    if (bs == NULL)
>>
>> Please add braces, use checkpatch.pl.
>>
>>> +        return -ENOENT;
>>> +    if (bdrv_attach_dev(bs, dev) < 0)
>>
>> Also here.
> 
> 
> This is pure code movement, and I won't introduce changes in the code
> while it is being moved to not make patch review harder.

> If you want to send coding style patches for that code after it is
> moved, be my guest.

No. Patches are required to pass checkpatch.pl (i.e., + lines in the
patch need braces). That means if not in the same patch for movement
reasons, Coding Style cleanups need to be done in advance, not as
followup. (same issue as in his AREG0 series)

Andreas

P.S. Dropping unrelated parts of the quotes makes requested changes
easier to spot. There was a ditto hidden somewhere down. ;)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 6/8] qdev-properties.c: separate core from the code used only by qemu-system-*
  2012-12-04 19:05       ` Andreas Färber
@ 2012-12-04 19:13         ` Eduardo Habkost
  0 siblings, 0 replies; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-04 19:13 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Blue Swirl, Igor Mammedov, qemu-devel, Anthony Liguori, Paolo Bonzini

On Tue, Dec 04, 2012 at 08:05:43PM +0100, Andreas Färber wrote:
> Am 04.12.2012 20:01, schrieb Eduardo Habkost:
> > On Tue, Dec 04, 2012 at 06:55:17PM +0000, Blue Swirl wrote:
> >> On Tue, Dec 4, 2012 at 1:19 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>> +static int parse_drive(DeviceState *dev, const char *str, void **ptr)
> >>> +{
> >>> +    BlockDriverState *bs;
> >>> +
> >>> +    bs = bdrv_find(str);
> >>> +    if (bs == NULL)
> >>
> >> Please add braces, use checkpatch.pl.
> >>
> >>> +        return -ENOENT;
> >>> +    if (bdrv_attach_dev(bs, dev) < 0)
> >>
> >> Also here.
> > 
> > 
> > This is pure code movement, and I won't introduce changes in the code
> > while it is being moved to not make patch review harder.
> 
> > If you want to send coding style patches for that code after it is
> > moved, be my guest.
> 
> No. Patches are required to pass checkpatch.pl (i.e., + lines in the
> patch need braces). That means if not in the same patch for movement
> reasons, Coding Style cleanups need to be done in advance, not as
> followup. (same issue as in his AREG0 series)

I had the feeling that the amount of cleanup (included but not limited
to coding style changes) I was doing before introducing actual code
changes was making some of my series harder to review. If making coding
style changes first is preferred, I can do it. I will try to send v10 of
this series today.

> 
> Andreas
> 
> P.S. Dropping unrelated parts of the quotes makes requested changes
> easier to spot. There was a ditto hidden somewhere down. ;)

ACK.  :)

-- 
Eduardo

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 8/8] qom: make CPU a child of DeviceState
  2012-12-04 13:19 ` [Qemu-devel] [RFC 8/8] qom: make CPU a child of DeviceState Eduardo Habkost
@ 2012-12-05 14:48   ` Andreas Färber
  2012-12-05 15:52     ` Eduardo Habkost
  0 siblings, 1 reply; 22+ messages in thread
From: Andreas Färber @ 2012-12-05 14:48 UTC (permalink / raw)
  To: Eduardo Habkost; +Cc: Igor Mammedov, qemu-devel, Anthony Liguori, Paolo Bonzini

Am 04.12.2012 14:19, schrieb Eduardo Habkost:
> From: Igor Mammedov <imammedo@redhat.com>
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> [ehabkost: change CPU type declaration to hae TYPE_DEVICE as parent]
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Yes, there is "changelog" data before the "---" mark, but I believe that
> in this case they are important to indicate authorship and the scope of
> the Signed-off-by lines (so they need to get into the git commit
> message).
> ---
>  include/qemu/cpu.h | 6 +++---
>  qom/cpu.c          | 3 ++-
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
> index 61b7698..bc004fd 100644
> --- a/include/qemu/cpu.h
> +++ b/include/qemu/cpu.h
> @@ -20,7 +20,7 @@
>  #ifndef QEMU_CPU_H
>  #define QEMU_CPU_H
>  
> -#include "qemu/object.h"
> +#include "hw/qdev-core.h"
>  #include "qemu-thread.h"
>  
>  /**
> @@ -46,7 +46,7 @@ typedef struct CPUState CPUState;
>   */
>  typedef struct CPUClass {
>      /*< private >*/
> -    ObjectClass parent_class;
> +    DeviceClass parent_class;
>      /*< public >*/
>  
>      void (*reset)(CPUState *cpu);
> @@ -62,7 +62,7 @@ typedef struct CPUClass {
>   */
>  struct CPUState {
>      /*< private >*/
> -    Object parent_obj;
> +    DeviceState parent_obj;
>      /*< public >*/
>  
>      struct QemuThread *thread;
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 5b36046..f59db7d 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -20,6 +20,7 @@
>  
>  #include "qemu/cpu.h"
>  #include "qemu-common.h"
> +#include "hw/qdev-core.h"
>  
>  void cpu_reset(CPUState *cpu)
>  {
> @@ -43,7 +44,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
>  
>  static TypeInfo cpu_type_info = {
>      .name = TYPE_CPU,
> -    .parent = TYPE_OBJECT,
> +    .parent = TYPE_DEVICE,
>      .instance_size = sizeof(CPUState),
>      .abstract = true,
>      .class_size = sizeof(CPUClass),

This makes the CPU a device, allowing the user to specify it with
-device. My preference would be to disable that at first[1] by setting
DeviceClass::no_user = 1.

Have you tested what happens if someone tries to hotplug a CPU device?
It may be the first device without bus...

[1] Anthony's and my idea was to handle hotplug at a higher level than
CPUState - X86Socket containing X86Core containing X86Thread or so. This
would require me (or someone) to refactor CPU_COMMON's numa_node (also
used in sPAPR), nr_cores, nr_threads (also used in mips/Malta) - in a
non-trivial way. We may need to go from CPU*State to CPUState (possible
so far) to Core to Socket, for which object_get_parent() would be
helpful. So far Object::parent is declared private.
Are we targetting to do this is two steps, using CPUState at first? Or
has one of you been investigating how involved this redesign would be?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 0/8] CPU DeviceState v9
  2012-12-04 16:40   ` Eduardo Habkost
@ 2012-12-05 14:50     ` Andreas Färber
  0 siblings, 0 replies; 22+ messages in thread
From: Andreas Färber @ 2012-12-05 14:50 UTC (permalink / raw)
  To: Eduardo Habkost
  Cc: Peter Maydell, Riku Voipio, qemu-devel, Anthony Liguori,
	Paolo Bonzini, Igor Mammedov

Am 04.12.2012 17:40, schrieb Eduardo Habkost:
> On Tue, Dec 04, 2012 at 04:59:38PM +0100, Andreas Färber wrote:
>> Am 04.12.2012 14:19, schrieb Eduardo Habkost:
>>> Changes on v9:
>>>  - Instead of moving qemu_[un]register_reset() to reset.c and including
>>>    it on *-user, create stubs for them on libqemustub.a
>>
>> We compile cpu.c twice. Can't we do the same for qdev.c or whatever uses
>> those functions? I feel they have no business being used in *-user.
>> CC'ing Riku and Peter.
> 
> I don't understand what exactly you are suggesting. You suggest adding
> #ifdefs to qdev.c to compile out the qemu_[un]register_reset() calls?

Yes, that was my thought. It would've spared us the include troubles in
stubs/ for now. ;)

Andreas

> I had a version of this series that did exactly that[1], but IIRC
> somebody suggested using stub functions instead. And I agree with
> whoever suggested it, I believe stub functions are cleaner when the the
> stub version still have the semantics expected by the caller[2].
> 
> [1] http://article.gmane.org/gmane.comp.emulators.xen.devel/137686
> [2] e.g. a no-op qemu_register_reset() still does the job it's supposed
>     to do (making sure a function to be called when qemu_devices_reset()
>     is called), if we know qemu_devices_reset() is never called.
> 
> 
>>
>> Andreas
>>
>>>  - This is based on afaerber's qom-cpu branch, that has some header cleanup
>>>    changes. You can get the complete series in a git tree at:
>>>    https://github.com/ehabkost/qemu-hacks/tree/cpu_qdev.v9
>>>    git://github.com/ehabkost/qemu-hacks.git cpu_qdev.v9
>>>
>>> v8:
>>>  - Use a simpler copyright header on qdev-properties-system.c
>>>  - Use the new libqemustub.a mechanism instead of the (now exting)
>>>    QEMU_WEAK_ALIAS mechanism
>>>  - Move the reset-handler registration code to a new hw/reset.c file
>>>
>>> v7:
>>>  - Use the new QEMU_WEAK_ALIAS mechanism instead of the (now extinct)
>>>    GCC_WEAK attribute (patches 20 and 21)
>>>
>>> v6:
>>>  - Simple rebase against latest qemu.git master
>>>  - Patch 13: some new typedefs were added and others were removed
>>>  - Patch 19: trivial rebase
>>> v5:
>>>  - Tons of header cleanups just to eliminate qlist.h <-> cpu-common.h circular
>>>    dependency (patches 1-17)
>>>  - Add copyright/license information to qdev-properties.c (patch 17)
>>>  - Add copyright/license information to qdev-properties-system.c (patch 22)
>>>  - use error_report()+abort() instead of hw_error() on qdev.c (patch 18)
>>>  - Move qemu_[un]register_reset() and qemu_devices_reset() to qdev-core.c
>>>    (patch 19)
>>>  - Make vmstate_[un]register() weak stubs, instead of a new function (patch 20)
>>>  - Make sysbus_get_default() weak stub, instead of new qbus reset (un)register
>>>    functions (patch 21)
>>>  - Eliminate qdev-system.c (all code is kept on qdev.c, now) (patch 22)
>>> v4:
>>>   - Add GCC_WEAK_DECL to functions that have GCC_WEAK versions
>>>   - Updated the qdev_init_gpio_in() code on qdev-system.c to current version
>>>   - Patch description updates (moved changelog below "---" and/or move info
>>>     about changes made by different authors between SoB lines)
>>> v3 (submitted by Igor):
>>>   - rebased on top of 8b4a3df (today's master)
>>>   - slight code reshuffling in (see commit's changelog)
>>>      "qdev: separate core from the code used only by qemu-system-*"
>>>      "move qemu_irq typedef out of cpu-common.h"
>>>   - commit messages cleanup
>>> v2:
>>>   Removes the CONFIG_USER_ONLY ifdefs, and use weak symbols to move
>>>   the vmstate and qemu_register_reset() handling to qdev-system.c

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [RFC 8/8] qom: make CPU a child of DeviceState
  2012-12-05 14:48   ` Andreas Färber
@ 2012-12-05 15:52     ` Eduardo Habkost
  0 siblings, 0 replies; 22+ messages in thread
From: Eduardo Habkost @ 2012-12-05 15:52 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Igor Mammedov, qemu-devel, Anthony Liguori, Paolo Bonzini

On Wed, Dec 05, 2012 at 03:48:10PM +0100, Andreas Färber wrote:
> Am 04.12.2012 14:19, schrieb Eduardo Habkost:
[...]
> > @@ -43,7 +44,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
> >  
> >  static TypeInfo cpu_type_info = {
> >      .name = TYPE_CPU,
> > -    .parent = TYPE_OBJECT,
> > +    .parent = TYPE_DEVICE,
> >      .instance_size = sizeof(CPUState),
> >      .abstract = true,
> >      .class_size = sizeof(CPUClass),
> 
> This makes the CPU a device, allowing the user to specify it with
> -device. My preference would be to disable that at first[1] by setting
> DeviceClass::no_user = 1.

I didn't know no_user existed. It makes sense to set it by now, yes.

> 
> Have you tested what happens if someone tries to hotplug a CPU device?
> It may be the first device without bus...

I don't know, but I won't be surprised if stuff breaks horribly.

> 
> [1] Anthony's and my idea was to handle hotplug at a higher level than
> CPUState - X86Socket containing X86Core containing X86Thread or so.

Yes, and I agree with this approach.

> This
> would require me (or someone) to refactor CPU_COMMON's numa_node (also
> used in sPAPR), nr_cores, nr_threads (also used in mips/Malta) - in a
> non-trivial way. We may need to go from CPU*State to CPUState (possible
> so far) to Core to Socket, for which object_get_parent() would be
> helpful. So far Object::parent is declared private.

Why, exactly, the CPUX86State, CPUThread, or CPUCore objects would need
to access their parents directly? We could just make sure that the
parent provide the necessary information to its children, when creating
them.

The other side of that question is: why exactly we don't allow an object
to know its parent, by design? What's the right mechanism to be used
when a device really needs to send some signal to its parent?


> Are we targetting to do this is two steps, using CPUState at first? Or
> has one of you been investigating how involved this redesign would be?

What do you mean by "using CPUState at first"? I mean, I think we'll
always use CPUState in the code that represents a single VCPU/thread,
and it is not going away soon. The name "CPUState" may be a bit
confusing, though, as it contains the state of a single VCPU/thread (not
a CPU socket).

If you are asking about "qdevifying/subclassing CPUState first", I think
the answer is: yes, making it in two steps sounds better. If we use
no_user, we can more easily change the hierarchy later because there
would be nobody using "-device" to create CPUs yet. And if we have to
design and redo a whole socket/core/thread CPU class hierarchy first, I
don't expect us to finish doing this before QEMU 1.5[1].


When implementing an socket-based interface, I think we may end up with
something like:

- CPUSocket (or CPUPackage)
  - creates multiple CPUCore children

- CPUCore
  - creates multiple CPUThread children
  - maybe have CPUState children directly, to make things simpler

- CPUThread
  - creates one CPUState child
  - (maybe CPUState can be used directly)

- CPUState
  - the class we already have today
  - could be renamed to VCPUState or ThreadState, to make it clearer
  - CPUID data is handled here
  - CPU feature configuration is handled here
  - Will have one subclass for each CPU model


Some variations/alternatives I see:

* Just having two levels. e.g.:

- CPUSocket (or CPUPackage)
  - creates multiple CPUState children, depending on nr_cores/nr_threads
    configuration

- CPUState
  - the class we already have today
  - could be renamed to VCPUState or ThreadState, to make it clearer
  - CPUID data is handled here
  - CPU feature configuration is handled here
  - Will have one subclass for each CPU model


* Having CPU model subclasses at the CPUSocket level instead of
  CPUState level
  - It would make the CPUID code really messy: some CPUID bits would
    come from the CPUSocket subclass, other from the CPUState itself.
  - On the other hand, the external interface may make more sense
    if we do it that way. I mean: "create a 4-core SandyBridge CPU
    [package]" sounds more logical than "create a CPU package with 4
    SandyBridge threads inside it".


[1] Just to explain my expectations: what I _really_ want to have on
    QEMU 1.4 is:
    - A good machine-type compatibility mechanism that allows us to
      update CPU model definitions while keeping compatibility
      - Fortunately, the current global-variable-based approach kind-of
        works
      - But CPU subclasses/properties would give us a cleaner solution
        for free (as we could simply use machine-type global properties)
    - Any interface that libvirt can use to query for CPU model
      information, including:
      - Listing available CPU models
        - This exists, but it doesn't provide much detail
      - Checking which features are going to be enabled by each CPU model
      - Checking which features can really be enabled on a host
        (considering QEMU + kernel + hardware capabilities)
      - CPU subclasses/properties give us (most of) the above for free

  I wouldn't want to delay the CPU subclass/property work because of a
  CPU socket/core/thread modelling rework, as it would means delaying
  (again) the interfaces that libvirt really needs.

-- 
Eduardo

^ permalink raw reply	[flat|nested] 22+ messages in thread
end of thread, other threads:[~2012-12-05 15:51 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-04 13:19 [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Eduardo Habkost
2012-12-04 13:19 ` [Qemu-devel] [RFC 1/8] move -I$(SRC_PATH)/include compiler flag to Makefile.objs Eduardo Habkost
2012-12-04 16:03   ` Andreas Färber
2012-12-04 16:22     ` Eduardo Habkost
2012-12-04 13:19 ` [Qemu-devel] [RFC 2/8] qdev: qdev_create(): use error_report() instead of hw_error() Eduardo Habkost
2012-12-04 16:12   ` Andreas Färber
2012-12-04 13:19 ` [Qemu-devel] [RFC 3/8] libqemustub: add qemu_[un]register_reset() stubs Eduardo Habkost
2012-12-04 13:19 ` [Qemu-devel] [RFC 4/8] libqemustub: vmstate register/unregister stubs Eduardo Habkost
2012-12-04 13:19 ` [Qemu-devel] [RFC 5/8] libqemustub: sysbus_get_default() stub Eduardo Habkost
2012-12-04 13:19 ` [Qemu-devel] [RFC 6/8] qdev-properties.c: separate core from the code used only by qemu-system-* Eduardo Habkost
2012-12-04 18:55   ` Blue Swirl
2012-12-04 19:01     ` Eduardo Habkost
2012-12-04 19:04       ` Blue Swirl
2012-12-04 19:05       ` Andreas Färber
2012-12-04 19:13         ` Eduardo Habkost
2012-12-04 13:19 ` [Qemu-devel] [RFC 7/8] include qdev code into *-user, too Eduardo Habkost
2012-12-04 13:19 ` [Qemu-devel] [RFC 8/8] qom: make CPU a child of DeviceState Eduardo Habkost
2012-12-05 14:48   ` Andreas Färber
2012-12-05 15:52     ` Eduardo Habkost
2012-12-04 15:59 ` [Qemu-devel] [RFC 0/8] CPU DeviceState v9 Andreas Färber
2012-12-04 16:40   ` Eduardo Habkost
2012-12-05 14:50     ` Andreas Färber

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.