Re: [Qemu-devel] [PATCH 3/6] device_tree: introduce qemu_fdt_node_path

[Eric Auger <eric.auger@linaro.org>]

Hi Peter,
On 12/18/2015 03:23 PM, Peter Maydell wrote:
> On 17 December 2015 at 12:29, Eric Auger <eric.auger@linaro.org> 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 <eric.auger@linaro.org>
>>
>> ---
>>
>> 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.
sure
> 
>> +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.
OK
> 
>> +
>> +    *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.)
My first understanding of the API was fdt_node_offset_by_compatible
would increment the offset even if an error occurred; so I envisioned to
continue parsing the tree, looking for another node with same features.
But I think it is overkill anyway and I will abort.
> 
>> +        }
>> +        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').
yes will set offset to len and then break;
> 
>> +        }
>> +
>> +        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').
true strcmp is OK

Thanks

Eric
> 
>> +            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
>