From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54869) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avJC5-0007hf-6M for qemu-devel@nongnu.org; Wed, 27 Apr 2016 02:43:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avJC0-0002gW-Ib for qemu-devel@nongnu.org; Wed, 27 Apr 2016 02:43:29 -0400 From: Markus Armbruster References: <1461119601-4936-1-git-send-email-david@gibson.dropbear.id.au> <1461119601-4936-2-git-send-email-david@gibson.dropbear.id.au> <571F4A36.7060106@redhat.com> <20160427060252.GB18476@voom.redhat.com> Date: Wed, 27 Apr 2016 08:43:20 +0200 In-Reply-To: <20160427060252.GB18476@voom.redhat.com> (David Gibson's message of "Wed, 27 Apr 2016 16:02:52 +1000") Message-ID: <87bn4v362f.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [RFC for-2.7 01/11] qdt: IEEE1275-style device tree utility code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: Thomas Huth , aik@ozlabs.ru, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de, crosthwaite.peter@gmail.com David Gibson 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 >> > +#include >> > + >> > +#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. [...]