[U-Boot] [PATCH v2 1/4] core: Add functions to set properties in live-tree

[Simon Glass]

Hi Mario,

On 4 May 2018 at 01:14, Mario Six <mario.six at gdsys.cc> wrote:
> Hi Simon,
>
> On Tue, May 1, 2018 at 1:13 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Mario,
>>
>> On 27 April 2018 at 06:51, Mario Six <mario.six at gdsys.cc> wrote:
>>>
>>> Implement a set of functions to manipulate properties in a live device
>>> tree:
>>>
>>> * ofnode_set_property() to set generic properties of a node
>>> * ofnode_write_string() to set string properties of a node
>>> * ofnode_set_enabled() to either enable or disable a node
>>>
>>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>>>
>>> ---
>>>
>>> v1 -> v2:
>>> * Fix potential NULL pointer dereference in ofnode_write_property
>>>
>>> ---
>>>  drivers/core/ofnode.c | 58
+++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/dm/ofnode.h   | 37 ++++++++++++++++++++++++++++++++
>>>  2 files changed, 95 insertions(+)
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>> But please see below.
>>
>>>
>>> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
>>> index 5909a25f85..a55aa75e5b 100644
>>> --- a/drivers/core/ofnode.c
>>> +++ b/drivers/core/ofnode.c
>>> @@ -687,3 +687,61 @@ u64 ofnode_translate_address(ofnode node, const
fdt32_t *in_addr)
>>>         else
>>>                 return fdt_translate_address(gd->fdt_blob,
ofnode_to_offset(node), in_addr);
>>>  }
>>> +
>>> +#ifdef CONFIG_OF_LIVE
>>> +int ofnode_write_property(ofnode node, const char *propname, int len,
>>> +                         const void *value)
>>> +{
>>> +       const struct device_node *np = ofnode_to_np(node);
>>> +       struct property *pp;
>>> +       struct property *pp_last = NULL;
>>> +       struct property *new;
>>> +
>>> +       if (!np)
>>> +               return -EINVAL;
>>> +
>>> +       for (pp = np->properties; pp; pp = pp->next) {
>>> +               if (strcmp(pp->name, propname) == 0) {
>>> +                       /* Property exists -> change value */
>>> +                       pp->value = (void *)value;
>>> +                       pp->length = len;
>>> +                       return 0;
>>> +               }
>>> +               pp_last = pp;
>>> +       }
>>> +
>>> +       if (!pp_last)
>>> +               return -ENOENT;
>>> +
>>> +       /* Property does not exist -> append new property */
>>> +       new = malloc(sizeof(struct property));
>>> +
>>> +       new->name = strdup(propname);
>>> +       new->value = malloc(len);
>>> +       memcpy(new->value, value, len);
>>
>> To me it seems odd that you allocate space for the value here, but
>> above (in the loop) you just assign it.
>>
>
> Right, just the pointer in the property itself is allocated; just
assigning the
> pointer to the property value with the one passed in might lead to it
being
> deallocated.

Who will ever deallocate it? To me these cases are the same and I can't see
why an existing property should be simply assigned, but a new property must
be allocated. At the very lease that seems like a confusing thing to
explain in the function comment you're going to add :-)

>
> Unfortunately a "free and malloc" or "realloc" won't work, since the
unflatten
> procedure in lib/of_live.c allocates the memory for the whole device tree
as
> one chunk, which cannot be partially freed. So the only choice would
either be
> only a "malloc" without prior freeing (which would leak memory if the
property
> is set multiple times), or require that the property value passed in is
valid
> forever (i.e. the caller has to malloc it himself), which would make the
> interface more complicated to use, and also pushes the responsibility of
> freeing the value onto the caller, who probably cannot safely decide when
to
> free it anyway.
>
> Another idea would be to find out the size of the unflattened device
tree; that
> way one could decide whether the property value pointer points into the
> allocated memory for the tree (in that case, just allocate a new
property), or
> if it points into a different memory region, which would indicate that the
> property was changed before already (in that case, free and allocate new
> property or reallocate). I don't know how complicated that would be,
though.
> I'll investigate.

Hmm, I think just documenting behaviour clearly is good enough. How about
we don't allocate the memory here. The caller can do it if necessary.

Regards,
Simon