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

[Mario Six]

Hi Simon,

On Fri, May 4, 2018 at 11:37 PM, Simon Glass <sjg at chromium.org> wrote:
> 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.
>

OK, should not be too difficult. I'll use that approach for v3.

> Regards,
> Simon
>

Best regards,
Mario