Re: [PATCH 2/5] libfdt: Add namelen variants for setprop

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6525 bytes --]

Sorry I've taken so long to reply.

On Sat, Nov 16, 2024 at 08:30:20PM +0530, Ayush Singh wrote:
> Helper functions to setproperty with length of property name similar to
> getprop_namelen variants.

The idea of this patch is good, independent of the rest of the series.
I'm kind of surprised we don't already have a setprop_namelen()
actually.

Unfortunately, this implementation isn't usable as is.

> 
> Signed-off-by: Ayush Singh <ayush@beagleboard.org>
> ---
>  libfdt/fdt_rw.c | 19 +++++++++++++++++
>  libfdt/libfdt.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  util.h          |  1 +
>  3 files changed, 83 insertions(+)
> 
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 3621d3651d3f4bd82b7af66c60d023e3139add03..9e66c2332bf4d3868c1388c294a195b152b6aefd 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -7,6 +7,8 @@
>  
>  #include <fdt.h>
>  #include <libfdt.h>
> +#include "../util.h"
> +#include <assert.h>

libfdt is designed to compile and work in limited environments (early
stage bootloaders, for example).  Therefore it's very limited in what
libc functions it's permitted to use: basically it can only use the
things listed in libfdt_env.h.

assert() is not ok, malloc() is right out.

>  #include "libfdt_internal.h"
>  
> @@ -288,6 +290,23 @@ int fdt_setprop(void *fdt, int nodeoffset, const char *name,
>  	return 0;
>  }
>  
> +int fdt_setprop_namelen(void *fdt, int nodeoffset, const char *name,
> +			size_t namelen, const void *val, int len)
> +{
> +	int ret;
> +	char *name_temp = xmalloc(namelen + 1);

As noted, we don't use an allocator in libfdt.  In fact that's a large
part of the reason the _namlen() functions exist in the first place.
To add this, you'll need to start with an fdt_get_property_namelen_w()
in terms of fdt_get_property_namelen(), then make
fdt_resize_property_() and fdt_add_property_() take the name length,
then finally fdt_setprop_placeholder_namelen() and
fdt_setprop_namelen().  The current fdt_setprop_placeholder() and
fdt_setprop() can then become trivial wrappers around the namelen
versions (like fdt_getprop() wraps fdt_getprop_namelen()).

> +
> +	assert(namelen <= strlen(name));
> +
> +	memcpy(name_temp, name, namelen);
> +	name_temp[namelen] = '\0';
> +	ret = fdt_setprop(fdt, nodeoffset, name_temp, val, len);
> +
> +	free(name_temp);
> +
> +	return ret;
> +}
> +
>  int fdt_appendprop(void *fdt, int nodeoffset, const char *name,
>  		   const void *val, int len)
>  {
> diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> index 96782bc57b8412d73dff92d6e0ad494ec0a23909..999b3800dff5c001b9c1babf370d45024c81a9aa 100644
> --- a/libfdt/libfdt.h
> +++ b/libfdt/libfdt.h
> @@ -1690,6 +1690,38 @@ int fdt_set_name(void *fdt, int nodeoffset, const char *name);
>  int fdt_setprop(void *fdt, int nodeoffset, const char *name,
>  		const void *val, int len);
>  
> +/**
> + * fdt_setprop_namelen - create or change a property
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node whose property to change
> + * @name: name of the property to change
> + * @namelen: length of the name
> + * @val: pointer to data to set the property value to
> + * @len: length of the property value
> + *
> + * fdt_setprop_namelen() sets the value of the named property in the given
> + * node to the given value and length, creating the property if it
> + * does not already exist.
> + *
> + * This function may insert or delete data from the blob, and will
> + * therefore change the offsets of some existing nodes.
> + *
> + * returns:
> + *	0, on success
> + *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
> + *		contain the new property value
> + *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +int fdt_setprop_namelen(void *fdt, int nodeoffset, const char *name,
> +			size_t namelen, const void *val, int len);
> +
>  /**
>   * fdt_setprop_placeholder - allocate space for a property
>   * @fdt: pointer to the device tree blob
> @@ -1839,6 +1871,37 @@ static inline int fdt_setprop_cell(void *fdt, int nodeoffset, const char *name,
>  #define fdt_setprop_string(fdt, nodeoffset, name, str) \
>  	fdt_setprop((fdt), (nodeoffset), (name), (str), strlen(str)+1)
>  
> +/**
> + * fdt_setprop_namelen_string - set a property to a string value
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node whose property to change
> + * @name: name of the property to change
> + * @str: string value for the property
> + *
> + * fdt_setprop_namelen_string() sets the value of the named property in the
> + * given node to the given string value (using the length of the
> + * string to determine the new length of the property), or creates a
> + * new property with that value if it does not already exist.
> + *
> + * This function may insert or delete data from the blob, and will
> + * therefore change the offsets of some existing nodes.
> + *
> + * returns:
> + *	0, on success
> + *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
> + *		contain the new property value
> + *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +#define fdt_setprop_namelen_string(fdt, nodeoffset, name, namelen, str)    \
> +	fdt_setprop_namelen((fdt), (nodeoffset), (name), (namelen), (str), \
> +			    strlen(str) + 1)
>  
>  /**
>   * fdt_setprop_empty - set a property to an empty value
> diff --git a/util.h b/util.h
> index 800f2e2c55b150d3c30101d1e1f1daaa0e4e3264..3e2ebaabe1e8c7e9ad6821ac2ca17a72e7f4f57d 100644
> --- a/util.h
> +++ b/util.h
> @@ -6,6 +6,7 @@
>  #include <stdarg.h>
>  #include <stdbool.h>
>  #include <getopt.h>
> +#include <stdio.h>
>  
>  /*
>   * Copyright 2011 The Chromium Authors, All Rights Reserved.
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]