From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49597) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9vwx-0002DG-3H for qemu-devel@nongnu.org; Fri, 18 Dec 2015 09:24:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a9vwv-0002nr-Sh for qemu-devel@nongnu.org; Fri, 18 Dec 2015 09:24:03 -0500 Received: from mail-vk0-x22b.google.com ([2607:f8b0:400c:c05::22b]:34053) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a9vwv-0002nm-N2 for qemu-devel@nongnu.org; Fri, 18 Dec 2015 09:24:01 -0500 Received: by mail-vk0-x22b.google.com with SMTP id j66so66057959vkg.1 for ; Fri, 18 Dec 2015 06:24:01 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <1450355367-14818-4-git-send-email-eric.auger@linaro.org> References: <1450355367-14818-1-git-send-email-eric.auger@linaro.org> <1450355367-14818-4-git-send-email-eric.auger@linaro.org> From: Peter Maydell Date: Fri, 18 Dec 2015 14:23:41 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH 3/6] device_tree: introduce qemu_fdt_node_path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Auger Cc: Baptiste Reynal , Thomas Huth , eric.auger@st.com, Patch Tracking , Peter Crosthwaite , QEMU Developers , Alex Williamson , Suravee Suthikulpanit , Paolo Bonzini , thomas.lendacky@amd.com, =?UTF-8?B?QWxleCBCZW5uw6ll?= , Christoffer Dall , David Gibson On 17 December 2015 at 12:29, Eric Auger wrote: > This new helper routine returns the node path of a device > referred to by its node name and compat string. > > Signed-off-by: Eric Auger > > --- > > RFC -> v1: > - improve error handling according to Alex' comments > --- > device_tree.c | 45 ++++++++++++++++++++++++++++++++++++++++++++ > include/sysemu/device_tree.h | 3 +++ > 2 files changed, 48 insertions(+) > > diff --git a/device_tree.c b/device_tree.c > index e556a99..b5d7e0b 100644 > --- a/device_tree.c > +++ b/device_tree.c > @@ -233,6 +233,51 @@ static int findnode_nofail(void *fdt, const char *node_path) > return offset; > } > > +/** > + * qemu_fdt_node_path > + * > + * return the node path of a device, given its node name and its > + * compat string > + * fdt: pointer to the dt blob > + * name: device node name > + * compat: compatibility string of the device > + * > + * upon success, the path is output at node_path address > + * returns 0 on success, < 0 on failure > + */ Can we put the doc comment in the header file, since this is a globally visible function? Also it would be nice to follow the doc-comment syntax standards about marking up arguments with '@' and so on. > +int qemu_fdt_node_path(void *fdt, const char *name, char *compat, > + char **node_path) > +{ > + int offset, len, ret; > + const char *iter_name; > + char path[256]; Rather than a fixed buffer size, we should check whether fdt_get_path returns -FDT_ERR_NOSPACE and if so enlarge the buffer and try again. > + > + *node_path = NULL; > + offset = fdt_node_offset_by_compatible(fdt, -1, compat); > + while (offset != -FDT_ERR_NOTFOUND) { > + if (offset < 0) { > + continue; I don't understand this continue -- if the fdt function returned any error other than -FDT_ERR_NOTFOUND then this will cause us to go into an infinite loop around this while(). Did you mean 'break' ? (Though if you just want to break then fixing the while condition would be better.) > + } > + iter_name = fdt_get_name(fdt, offset, &len); > + if (!iter_name) { > + continue; This also seems like it ought to be a break, except you need to set offset to the error code first (which fdt_get_name() will have returned in 'len'). > + } > + > + if (!strncmp(iter_name, name, len)) { Do we really want strncmp rather than strcmp here ? (ie "find first node whose name has 'name' as a prefix" rather than "find first node whose name matches 'name'). > + goto found; > + } > + offset = fdt_node_offset_by_compatible(fdt, offset, compat); > + } > + return offset; > + > +found: > + ret = fdt_get_path(fdt, offset, path, 256); > + if (!ret) { > + *node_path = g_strdup(path); > + } > + return ret; > +} > + > int qemu_fdt_setprop(void *fdt, const char *node_path, > const char *property, const void *val, int size) > { > diff --git a/include/sysemu/device_tree.h b/include/sysemu/device_tree.h > index 307e53d..f9e6e6e 100644 > --- a/include/sysemu/device_tree.h > +++ b/include/sysemu/device_tree.h > @@ -18,6 +18,9 @@ void *create_device_tree(int *sizep); > void *load_device_tree(const char *filename_path, int *sizep); > void *load_device_tree_from_sysfs(void); > > +int qemu_fdt_node_path(void *fdt, const char *name, char *compat, > + char **node_path); > + > int qemu_fdt_setprop(void *fdt, const char *node_path, > const char *property, const void *val, int size); > int qemu_fdt_setprop_cell(void *fdt, const char *node_path, > -- > 1.9.1 thanks -- PMM