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: 4319 bytes --]

On Mon, Jun 27, 2016 at 11:16:52AM +0200, Maxime Ripard wrote:
> On Mon, Jun 27, 2016 at 01:45:11AM +1000, David Gibson wrote:
> > 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.
> 
> Ack.
> 
> > > +
> > >  /**
> > >   * 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.
> 
> So I'm not really sure what you want me to do.
> 
> I can't check for the property length, since it would prevent that
> function from working, I can't check for the length + index, since we
> might update only a few bytes in the middle, and we want to keep that
> error case.
> 
> Or should I just forgive about merging the two functions and just
> duplicate the two?

No, what I'm suggesting is that fdt_setprop_inplace(), whether or not
it is implemented using fdt_setprop_inplace_partial() *also* checks
that the existing property length exactly matches the replacement
length, and fails otherwise.

Doing this without too much code duplication might involve a private
internal function, or maybe it's just simpler to duplicate.  But those
are the semantics they should have.

-- 
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 --]