Re: [Qemu-devel] [RFC for-2.7 01/11] qdt: IEEE1275-style device tree utility code

[Markus Armbruster]

David Gibson <address@hidden> writes:

> On Tue, Apr 26, 2016 at 01:00:06PM +0200, Thomas Huth wrote:
>> On 20.04.2016 04:33, David Gibson wrote:
>> ...
>> > This patch introduces a new utility library "qdt" for runtime
>> > manipulation of device trees.  The intention is that machine type code
>> > can use these routines to construct the device tree conveniently,
>> > using a pointer-based representation doesn't have the limitations
>> > above.  They can then use qdt_flatten() to convert the completed tree
>> > to fdt format as a single O(n) operation to pass to the guest.
>> 
>> Good idea, the FDT format itself is really not very well suited for
>> dynamic manipulations...
>> 
>> ...
>> > diff --git a/util/qdt.c b/util/qdt.c
>> > new file mode 100644
>> > index 0000000..e3a449a
>> > --- /dev/null
>> > +++ b/util/qdt.c
>> > @@ -0,0 +1,262 @@
>> > +/*
>> > + * Functions for manipulating IEEE1275 (Open Firmware) style device
>> > + * trees.
>> 
>> What does QDT stand for? Maybe add that in the description here.
>
> "QEMU Device Tree" I guess?  Really I was just looking for something
> similar but not the same as fdt, and short.
>
>> > + * Copyright David Gibson, Red Hat Inc. 2016
>> > + *
>> > + * This work is licensed unter the GNU GPL version 2 or (at your
>> > + * option) any later version.
>> > + */
>> > +
>> > +#include <libfdt.h>
>> > +#include <stdbool.h>
>> > +
>> > +#include "qemu/osdep.h"
>> > +#include "qapi/error.h"
>> > +#include "qemu/qdt.h"
>> > +#include "qemu/error-report.h"
>> > +
>> > +/*
>> > + * Node functions
>> > + */
>> > +
>> > +QDTNode *qdt_new_node(const gchar *name)
>> > +{
>> > +    QDTNode *node = g_new0(QDTNode, 1);
>> > +
>> > +    g_assert(!strchr(name, '/'));
>> > +
>> > +    node->name = g_strdup(name);
>> > +    QTAILQ_INIT(&node->properties);
>> > +    QTAILQ_INIT(&node->children);
>> > +
>> > +    return node;
>> > +}
>> > +
>> > +static QDTNode *get_subnode(QDTNode *parent, const gchar *name, size_t 
>> > namelen)
>> > +{
>> > +    QDTNode *child;
>> > +
>> > +    g_assert(!memchr(name, '/', namelen));
>> > +
>> > +    QTAILQ_FOREACH(child, &parent->children, sibling) {
>> > +        if ((strlen(child->name) == namelen)
>> > +            && (memcmp(child->name, name, namelen) == 0)) {
>> 
>> Too many parenthesis for my taste ;-)

Mine too.

>> > +            return child;
>> > +        }
>> > +    }
>> > +
>> > +    return NULL;
>> > +}
>> > +
>> > +QDTNode *qdt_get_node_relative(QDTNode *node, const gchar *path)
>> > +{
>> > +    const gchar *slash;
>> > +    gsize seglen;
>> > +
>> > +    do {
>> > +        slash = strchr(path, '/');
>> > +        seglen = slash ? slash - path : strlen(path);
>> > +
>> > +        node = get_subnode(node, path, seglen);
>> > +        path += seglen + 1;
>> > +    } while (node && slash);
>> > +
>> > +    return node;
>> > +}
>> > +
>> > +QDTNode *qdt_get_node(QDTNode *root, const gchar *path)
>> > +{
>> > +    g_assert(!root->parent);
>> > +    g_assert(path[0] == '/');
>> > +    return qdt_get_node_relative(root, path + 1);
>> > +}
>> > +
>> > +QDTNode *qdt_add_subnode(QDTNode *parent, const gchar *name)
>> > +{
>> > +    QDTNode *new = qdt_new_node(name);
>> > +
>> > +    new->parent = parent;
>> > +    QTAILQ_INSERT_TAIL(&parent->children, new, sibling);
>> > +    return new;
>> > +}
>> 
>> In case somebody ever tries to compile this with a C++ compiler ... it's
>> maybe better avoid using "new" as name for a variable.
>
> Good point, will change.

My answer to "what if somebody tries to compile this code with a
compiler for a different language" is "hope it won't compile then, for
the innocents' sake".

>> > +/*
>> > + * Property functions
>> > + */
>> > +
>> > +QDTProperty *qdt_new_property(const gchar *name, gconstpointer val, gsize 
>> > len)
>> > +{
>> > +    QDTProperty *prop = g_malloc0(sizeof(*prop) + len);
>> > +
>> > +    prop->name = g_strdup(name);
>> > +    prop->len = len;
>> > +    memcpy(prop->val, val, len);
>> > +    return prop;
>> > +}
>> > +
>> > +static QDTProperty *getprop_(const QDTNode *node, const gchar *name)
>> 
>> Underscore at the end looks somewhat strange ... can't you simply drop that?
>
> Well.. the idea was that the _ versions are the "internal" ones,
> whereas external users will generally use the non-underscore version

I've seen that convention used before.  It's fine with me.

> (in this case the only difference is that the external one returns a
> const pointer).
>
> I don't particularly like that convention, so feel free to suggest
> something better.

Consider getprop_internal() if the length isn't bothersome.  It is when
the name is used all over the place.

do_getprop() would be shorter.  I don't like do_verb names myself.

[...]