David Gibson <da...@gibson.dropbear.id.au> 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.
[...]
