qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastruct


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 01/18] qom: add new dynamic property infrastructure based on Visitors
Date: Fri, 02 Dec 2011 12:47:10 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.21) Gecko/20110831 Lightning/1.0b2 Thunderbird/3.1.13

On 12/01/2011 09:52 AM, Kevin Wolf wrote:
Am 30.11.2011 22:03, schrieb Anthony Liguori:
+
+void qdev_property_add(DeviceState *dev, const char *name, const char *type,
+                       DevicePropertyEtter *get, DevicePropertyEtter *set,
+                       DevicePropertyRelease *release, void *opaque,
+                       Error **errp)

How about letting the caller pass in a DeviceProperty for improved
readability and usability? Instead of memorizing the order of currently
eight parameters (could probably become more in the future) you can use
proper C99 initializers then.

This ends up making the code much more complex for the client if you try to eliminate the opaque and replace it with the structure. It becomes necessary to do a dynamic allocation of the structure and then you also have to add a release function.

We could make the structure just contain the function pointers and not the opaque but that doesn't seem very helpful to me. It just adds a few extra lines to the client code without a lot of gain.

Regards,

Anthony Liguori


@@ -45,6 +82,7 @@ struct DeviceState {
      QTAILQ_ENTRY(DeviceState) sibling;
      int instance_id_alias;
      int alias_required_for_version;
+    GSList *properties;
  };

Why GSList instead of qemu-queue.h macros that would provide type safety?

I don't think a property can belong to multiple devices, can it?
qdev_property_add only refers to a single device, and nothing else adds
elements to the list.

Kevin





reply via email to

[Prev in Thread] Current Thread [Next in Thread]