Re: [PATCH v3 07/12] libfdt: Add fdt_setprop_inplace_by_index

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

On Fri, Jun 24, 2016 at 04:27:52PM +0200, Maxime Ripard wrote:
> Add a function to modify inplace a property starting from a given index.
> 
> This is especially useful when the property is an array of values, and you
> want to update one of them without changing the DT size.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  include/libfdt.h     | 34 +++++++++++++++++++++++++++++++---
>  lib/libfdt/fdt_wip.c | 13 ++++++++-----
>  2 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 4643be5adf39..2c8a42bcb667 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -1032,6 +1032,27 @@ int fdt_size_cells(const void *fdt, int nodeoffset);
>  /* Write-in-place functions                                           */
>  /**********************************************************************/
>  
> +/**
> + * fdt_setprop_inplace_namelen_by_index - change a property's value,
> + *                                        but not its size
> + * @fdt: pointer to the device tree blob
> + * @nodeoffset: offset of the node whose property to change
> + * @name: name of the property to change
> + * @namelen: number of characters of name to consider
> + * @index: index of the property to change in the array
> + * @val: pointer to data to replace the property value with
> + * @len: length of the property value
> + *
> + * Identical to fdt_setprop_inplace(), but modifies the given property
> + * starting from the given index, and using only the first characters
> + * of the name. It is useful when you want to manipulate only one value of
> + * an array and you have a string that doesn't end with \0.
> + */
> +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset,
> +					 const char *name, int namelen,
> +					 uint32_t index, const void *val,
> +					 int len);

This looks like a good addition to upstream, but I don't like the
name.  I'd suggest fdt_setprop_inplace_namelen_partial() because it
only overwrite part of the existing property value.

> +
>  /**
>   * fdt_setprop_inplace - change a property's value, but not its size
>   * @fdt: pointer to the device tree blob
> @@ -1060,8 +1081,13 @@ int fdt_size_cells(const void *fdt, int nodeoffset);
>   *	-FDT_ERR_BADSTRUCTURE,
>   *	-FDT_ERR_TRUNCATED, standard meanings
>   */
> -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
> -			const void *val, int len);
> +static inline int fdt_setprop_inplace(void *fdt, int nodeoffset,
> +				      const char *name, const void *val,
> +				      int len)
> +{
> +	return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset, name,
> +						    strlen(name), 0, val, len);

This effectively removes the error if len is not equal to the existing
property length, so that needs to be put back.

> +}
>  
>  /**
>   * fdt_setprop_inplace_u32 - change the value of a 32-bit integer property
> @@ -1095,7 +1121,9 @@ static inline int fdt_setprop_inplace_u32(void *fdt, int nodeoffset,
>  					  const char *name, uint32_t val)
>  {
>  	fdt32_t tmp = cpu_to_fdt32(val);
> -	return fdt_setprop_inplace(fdt, nodeoffset, name, &tmp, sizeof(tmp));
> +	return fdt_setprop_inplace_namelen_by_index(fdt, nodeoffset,
> +						    name, strlen(name),
> +						    0, &tmp, sizeof(tmp));
>  }
>  
>  /**
> diff --git a/lib/libfdt/fdt_wip.c b/lib/libfdt/fdt_wip.c
> index 9fe988655fe3..737769fa59e2 100644
> --- a/lib/libfdt/fdt_wip.c
> +++ b/lib/libfdt/fdt_wip.c
> @@ -14,20 +14,23 @@
>  
>  #include "libfdt_internal.h"
>  
> -int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
> -			const void *val, int len)
> +int fdt_setprop_inplace_namelen_by_index(void *fdt, int nodeoffset,
> +					 const char *name, int namelen,
> +					 uint32_t index, const void *val,
> +					 int len)
>  {
>  	void *propval;
>  	int proplen;
>  
> -	propval = fdt_getprop_w(fdt, nodeoffset, name, &proplen);
> +	propval = fdt_getprop_namelen_w(fdt, nodeoffset, name, namelen,
> +					&proplen);
>  	if (!propval)
>  		return proplen;
>  
> -	if (proplen != len)
> +	if (proplen < (len + index))
>  		return -FDT_ERR_NOSPACE;
>  
> -	memcpy(propval, val, len);
> +	memcpy(propval + index, val, len);
>  	return 0;
>  }
>  

-- 
David Gibson			| 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: 819 bytes --]