| Message ID | 1322687028-29714-16-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 30, 2011 at 03:03:45PM -0600, Anthony Liguori wrote: > @@ -1210,6 +1210,9 @@ void qdev_property_add_child(DeviceState *dev, const char *name, > qdev_property_add(dev, name, type, qdev_get_child_property, > NULL, NULL, child, errp); > > + g_assert(child->parent == NULL); > + child->parent = dev; The implications are: 1. A DeviceState must be a child or the root. It is not okay to create a DeviceState and inquire its canonical path before making it a child in the graph. 2. A DeviceState can only be the child of one parent. Since user-created devices are added to /peripheral or /peripheral-anon this means that the /i440fx only has links to them, never a parent-child relationship. Is this right? > + /* Do not, under any circumstance, use this parent link below anywhere > + * outside of qdev.c. You have been warned. */ > + DeviceState *parent; It would be nice to explain why parent is private to qdev.c.
On 12/01/2011 05:21 AM, Stefan Hajnoczi wrote: > On Wed, Nov 30, 2011 at 03:03:45PM -0600, Anthony Liguori wrote: >> @@ -1210,6 +1210,9 @@ void qdev_property_add_child(DeviceState *dev, const char *name, >> qdev_property_add(dev, name, type, qdev_get_child_property, >> NULL, NULL, child, errp); >> >> + g_assert(child->parent == NULL); >> + child->parent = dev; > > The implications are: > > 1. A DeviceState must be a child or the root. It is not okay to create > a DeviceState and inquire its canonical path before making it a child in > the graph. Correct. > > 2. A DeviceState can only be the child of one parent. Since > user-created devices are added to /peripheral or /peripheral-anon this > means that the /i440fx only has links to them, never a parent-child > relationship. Correct. /peripheral[-anon] are where user created devices live. Machine created devices live directly off of / Each "directory" ends up being a unique namespace. This ends up being a nice way to support compatibly since we have so many namespace today. I imagine, for instance, that we will end up with a /block directory too and that is where blockdev objects will live. > > Is this right? > >> + /* Do not, under any circumstance, use this parent link below anywhere >> + * outside of qdev.c. You have been warned. */ >> + DeviceState *parent; > > It would be nice to explain why parent is private to qdev.c. Composition is a unidirectional relationship. The parent pointer is there simply as an optimization. I'll make the comment a big scarier :-) Regards, Anthony Liguori
diff --git a/hw/qdev.c b/hw/qdev.c index b944108..7da7196 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -1210,6 +1210,9 @@ void qdev_property_add_child(DeviceState *dev, const char *name, qdev_property_add(dev, name, type, qdev_get_child_property, NULL, NULL, child, errp); + g_assert(child->parent == NULL); + child->parent = dev; + g_free(type); } @@ -1282,48 +1285,39 @@ void qdev_property_add_link(DeviceState *dev, const char *name, g_free(full_type); } -static gchar *qdev_get_path_in(DeviceState *parent, DeviceState *dev) +gchar *qdev_get_canonical_path(DeviceState *dev) { - GSList *i; + DeviceState *root = qdev_get_root(); + char *newpath = NULL, *path = NULL; - if (parent == dev) { - return g_strdup(""); - } - - for (i = parent->properties; i; i = i->next) { - DeviceProperty *prop = i->data; - gchar *subpath; - - if (!strstart(prop->type, "child<", NULL)) { - continue; - } + while (dev != root) { + GSList *i; - /* Check to see if the device is one of parent's children */ - if (prop->opaque == dev) { - return g_strdup(prop->name); - } + g_assert(dev->parent != NULL); - /* Check to see if the device is a child of our child */ - subpath = qdev_get_path_in(prop->opaque, dev); - if (subpath) { - gchar *path; + for (i = dev->parent->properties; i; i = i->next) { + DeviceProperty *prop = i->data; - path = g_strdup_printf("%s/%s", prop->name, subpath); - g_free(subpath); + if (!strstart(prop->type, "child<", NULL)) { + continue; + } - return path; + if (prop->opaque == dev) { + if (path) { + newpath = g_strdup_printf("%s/%s", prop->name, path); + g_free(path); + path = newpath; + } else { + path = g_strdup(prop->name); + } + break; + } } - } - return NULL; -} + g_assert(i != NULL); -gchar *qdev_get_canonical_path(DeviceState *dev) -{ - gchar *path, *newpath; - - path = qdev_get_path_in(qdev_get_root(), dev); - g_assert(path != NULL); + dev = dev->parent; + } newpath = g_strdup_printf("/%s", path); g_free(path); diff --git a/hw/qdev.h b/hw/qdev.h index e8c9e76..8eb7ce1 100644 --- a/hw/qdev.h +++ b/hw/qdev.h @@ -83,6 +83,10 @@ struct DeviceState { int instance_id_alias; int alias_required_for_version; GSList *properties; + + /* Do not, under any circumstance, use this parent link below anywhere + * outside of qdev.c. You have been warned. */ + DeviceState *parent; }; typedef void (*bus_dev_printfn)(Monitor *mon, DeviceState *dev, int indent);
The full tree search was a bit unreasonable. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- hw/qdev.c | 60 +++++++++++++++++++++++++++--------------------------------- hw/qdev.h | 4 ++++ 2 files changed, 31 insertions(+), 33 deletions(-)