[PATCH 0/3] OF: base: cleanup and allow empty properties

[PATCH 0/3] OF: base: cleanup and allow empty properties

[Sebastian Hesselbarth]

This patch set cleans Barebox OF API with respect to properies by using
internal functions for creating/deleting properties instead of
allocating/freeing in different places.

Also, with the consolidated property handling, we can now add a bogus
property value pointer to distinguish empty/boolean properties from
non-existing properties.

The patch set is based on the latest OF API patches sent some days
ago with Sascha Hauers fixes applied.

Sebastian Hesselbarth (3):
  OF: base: add sanity checks to of_new/delete_property
  OF: base: use of_delete_property where applicable
  OF: base: add empty property value pointer

 drivers/of/base.c |  107 ++++++++++++++++++++++++-----------------------------
 1 files changed, 48 insertions(+), 59 deletions(-)

---
Cc: barebox@lists.infradead.org
-- 
1.7.2.5


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/3] OF: base: add sanity checks to of_new/delete_property

[Sebastian Hesselbarth]

This adds some sanity checks to of_new_property and of_delete_property.
Also, data passed to of_new_property is only copied if non-NULL.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: barebox@lists.infradead.org
---
 drivers/of/base.c |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 1b351ee..bcfd73a 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1527,21 +1527,37 @@ struct property *of_new_property(struct device_node *node, const char *name,
 	struct property *prop;
 
 	prop = xzalloc(sizeof(*prop));
+	if (!prop)
+		return NULL;
 
 	prop->name = strdup(name);
+	if (!prop->name)
+		goto bail_out;
+
 	prop->length = len;
 	if (len) {
 		prop->value = xzalloc(len);
-		memcpy(prop->value, data, len);
+		if (!prop->value)
+			goto bail_out;
 	}
 
+	if (len && data)
+		memcpy(prop->value, data, len);
+
 	list_add_tail(&prop->list, &node->properties);
 
 	return prop;
+
+bail_out:
+	of_delete_property(prop);
+	return NULL;
 }
 
 void of_delete_property(struct property *pp)
 {
+	if (!pp)
+		return;
+
 	list_del(&pp->list);
 
 	free(pp->name);
-- 
1.7.2.5


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] OF: base: use of_delete_property where applicable

[Sebastian Hesselbarth]

This patch makes OF API use of_delete_property where applicable instead
of freeing allocated data in different places.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: barebox@lists.infradead.org
---
 drivers/of/base.c |   83 ++++++++++++++++------------------------------------
 1 files changed, 26 insertions(+), 57 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index bcfd73a..7926d5d 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -899,16 +899,11 @@ int of_property_write_u8_array(struct device_node *np,
 	struct property *prop = of_find_property(np, propname, NULL);
 	u8 *val;
 
-	if (!prop)
-		prop = of_new_property(np, propname, NULL, 0);
-	if (!prop)
-		return -ENOMEM;
-
-	free(prop->value);
+	if (prop)
+		of_delete_property(prop);
 
-	prop->length = sizeof(*val) * sz;
-	prop->value = malloc(prop->length);
-	if (!prop->value)
+	prop = of_new_property(np, propname, NULL, sizeof(*val) * sz);
+	if (!prop)
 		return -ENOMEM;
 
 	val = prop->value;
@@ -940,18 +935,13 @@ int of_property_write_u16_array(struct device_node *np,
 	struct property *prop = of_find_property(np, propname, NULL);
 	__be16 *val;
 
-	if (!prop)
-		prop = of_new_property(np, propname, NULL, 0);
+	if (prop)
+		of_delete_property(prop);
+		
+	prop = of_new_property(np, propname, NULL, sizeof(*val) * sz);
 	if (!prop)
 		return -ENOMEM;
 
-	free(prop->value);
-
-	prop->length = sizeof(*val) * sz;
-	prop->value = malloc(prop->length);
-	if (!prop->value)
-		return -ENOMEM;
-
 	val = prop->value;
 	while (sz--)
 		*val++ = cpu_to_be16(*values++);
@@ -981,16 +971,11 @@ int of_property_write_u32_array(struct device_node *np,
 	struct property *prop = of_find_property(np, propname, NULL);
 	__be32 *val;
 
-	if (!prop)
-		prop = of_new_property(np, propname, NULL, 0);
-	if (!prop)
-		return -ENOMEM;
-
-	free(prop->value);
+	if (prop)
+		of_delete_property(prop);
 
-	prop->length = sizeof(*val) * sz;
-	prop->value = malloc(prop->length);
-	if (!prop->value)
+	prop = of_new_property(np, propname, NULL, sizeof(*val) * sz);
+	if (!prop)
 		return -ENOMEM;
 
 	val = prop->value;
@@ -1022,16 +1007,11 @@ int of_property_write_u64_array(struct device_node *np,
 	struct property *prop = of_find_property(np, propname, NULL);
 	__be32 *val;
 
-	if (!prop)
-		prop = of_new_property(np, propname, NULL, 0);
-	if (!prop)
-		return -ENOMEM;
-
-	free(prop->value);
+	if (prop)
+		of_delete_property(prop);
 
-	prop->length = 2 * sizeof(*val) * sz;
-	prop->value = malloc(prop->length);
-	if (!prop->value)
+	prop = of_new_property(np, propname, NULL, 2 * sizeof(*val) * sz);
+	if (!prop)
 		return -ENOMEM;
 
 	val = prop->value;
@@ -1576,26 +1556,19 @@ void of_delete_property(struct property *pp)
 int of_set_property(struct device_node *np, const char *name, const void *val, int len,
 		int create)
 {
-	struct property *pp;
+	struct property *pp = of_find_property(np, name, NULL);
 
 	if (!np)
 		return -ENOENT;
 
-	pp = of_find_property(np, name, NULL);
-	if (pp) {
-		void *data;
-
-		free(pp->value);
-		data = xzalloc(len);
-		memcpy(data, val, len);
-		pp->value = data;
-		pp->length = len;
-	} else {
-		if (!create)
-			return -ENOENT;
+	if (!pp && !create)
+		return -ENOENT;
+		
+	of_delete_property(pp);
 
-		pp = of_new_property(np, name, val, len);
-	}
+	pp = of_new_property(np, name, val, len);
+	if (!pp)
+		return -ENOMEM;
 
 	return 0;
 }
@@ -1819,12 +1792,8 @@ void of_free(struct device_node *node)
 	if (!node)
 		return;
 
-	list_for_each_entry_safe(p, pt, &node->properties, list) {
-		list_del(&p->list);
-		free(p->name);
-		free(p->value);
-		free(p);
-	}
+	list_for_each_entry_safe(p, pt, &node->properties, list)
+		of_delete_property(p);
 
 	list_for_each_entry_safe(n, nt, &node->children, parent_list) {
 		of_free(n);
-- 
1.7.2.5


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] OF: base: add empty property value pointer

[Sebastian Hesselbarth]

Since property values can be empty, we need property value pointer to
be non-NULL to distinguish those properties from non-existing properties.
This adds a static u32 to which empty properties set their value pointer.
Also, the value memory is only freed, if property length is not zero.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: barebox@lists.infradead.org
---
 drivers/of/base.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 7926d5d..a100a17 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1501,6 +1501,8 @@ struct device_node *of_new_node(struct device_node *parent, const char *name)
 	return node;
 }
 
+static u32 empty_prop_value;
+
 struct property *of_new_property(struct device_node *node, const char *name,
 		const void *data, int len)
 {
@@ -1515,6 +1517,7 @@ struct property *of_new_property(struct device_node *node, const char *name,
 		goto bail_out;
 
 	prop->length = len;
+	prop->value = &empty_prop_value;
 	if (len) {
 		prop->value = xzalloc(len);
 		if (!prop->value)
@@ -1541,7 +1544,8 @@ void of_delete_property(struct property *pp)
 	list_del(&pp->list);
 
 	free(pp->name);
-	free(pp->value);
+	if (pp->length)
+		free(pp->value);
 	free(pp);
 }
 
-- 
1.7.2.5


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 1/3] OF: base: add sanity checks to of_new/delete_property

[Sascha Hauer]

On Fri, Jun 21, 2013 at 05:15:16PM +0200, Sebastian Hesselbarth wrote:
> This adds some sanity checks to of_new_property and of_delete_property.
> Also, data passed to of_new_property is only copied if non-NULL.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Cc: barebox@lists.infradead.org
> ---
>  drivers/of/base.c |   18 +++++++++++++++++-
>  1 files changed, 17 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 1b351ee..bcfd73a 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1527,21 +1527,37 @@ struct property *of_new_property(struct device_node *node, const char *name,
>  	struct property *prop;
>  
>  	prop = xzalloc(sizeof(*prop));
> +	if (!prop)
> +		return NULL;

xzalloc always returns valid memory. It's return-mem-or-panic.

>  
>  	prop->name = strdup(name);
> +	if (!prop->name)
> +		goto bail_out;
> +
>  	prop->length = len;
>  	if (len) {
>  		prop->value = xzalloc(len);
> -		memcpy(prop->value, data, len);
> +		if (!prop->value)
> +			goto bail_out;

ditto.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] OF: base: add empty property value pointer

[Sascha Hauer]

On Fri, Jun 21, 2013 at 05:15:18PM +0200, Sebastian Hesselbarth wrote:
> Since property values can be empty, we need property value pointer to
> be non-NULL to distinguish those properties from non-existing properties.
> This adds a static u32 to which empty properties set their value pointer.
> Also, the value memory is only freed, if property length is not zero.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Cc: barebox@lists.infradead.org
> ---
>  drivers/of/base.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 7926d5d..a100a17 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1501,6 +1501,8 @@ struct device_node *of_new_node(struct device_node *parent, const char *name)
>  	return node;
>  }
>  
> +static u32 empty_prop_value;
> +
>  struct property *of_new_property(struct device_node *node, const char *name,
>  		const void *data, int len)
>  {
> @@ -1515,6 +1517,7 @@ struct property *of_new_property(struct device_node *node, const char *name,
>  		goto bail_out;
>  
>  	prop->length = len;
> +	prop->value = &empty_prop_value;

This at least breaks of_set_property() and of_free(). Both unconditionally
do a free(pp->value).

Why do we need this anyway? We can always call of_find_property() to see
if a property exists.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] OF: base: add empty property value pointer

[Sebastian Hesselbarth]

On 06/23/2013 08:33 PM, Sascha Hauer wrote:
> On Fri, Jun 21, 2013 at 05:15:18PM +0200, Sebastian Hesselbarth wrote:
>> Since property values can be empty, we need property value pointer to
>> be non-NULL to distinguish those properties from non-existing properties.
>> This adds a static u32 to which empty properties set their value pointer.
>> Also, the value memory is only freed, if property length is not zero.
>>
>> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
>> ---
>> Cc: barebox@lists.infradead.org
>> ---
>>   drivers/of/base.c |    6 +++++-
>>   1 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 7926d5d..a100a17 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -1501,6 +1501,8 @@ struct device_node *of_new_node(struct device_node *parent, const char *name)
>>   	return node;
>>   }
>>
>> +static u32 empty_prop_value;
>> +
>>   struct property *of_new_property(struct device_node *node, const char *name,
>>   		const void *data, int len)
>>   {
>> @@ -1515,6 +1517,7 @@ struct property *of_new_property(struct device_node *node, const char *name,
>>   		goto bail_out;
>>
>>   	prop->length = len;
>> +	prop->value =&empty_prop_value;
>
> This at least breaks of_set_property() and of_free(). Both unconditionally
> do a free(pp->value).

Sascha,

this is patch 3/3, the two functions above are taken care of patch 2/3.

> Why do we need this anyway? We can always call of_find_property() to see
> if a property exists.

Actually, I was preparing to import drivers/of/address.c from linux.
of_translate_one uses of_get_property on "ranges". This returns
the property's value pointer instead of the property itself, which
is NULL for an empty ranges property. Now that you point it out,
using of_find_property is also an option.

Nevertheless, patches 1 (with your comments applied) and 2 seem
sensible to me. Patch 3 is an option to keep it in sync with Linux
OF API behavior but is optional.

Sebastian

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] OF: base: add empty property value pointer

[Sascha Hauer]

On Sun, Jun 23, 2013 at 10:11:39PM +0200, Sebastian Hesselbarth wrote:
> On 06/23/2013 08:33 PM, Sascha Hauer wrote:
> >On Fri, Jun 21, 2013 at 05:15:18PM +0200, Sebastian Hesselbarth wrote:
> >>Since property values can be empty, we need property value pointer to
> >>be non-NULL to distinguish those properties from non-existing properties.
> >>This adds a static u32 to which empty properties set their value pointer.
> >>Also, the value memory is only freed, if property length is not zero.
> >>
> >>Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com>
> >>---
> >>Cc: barebox@lists.infradead.org
> >>---
> >>  drivers/of/base.c |    6 +++++-
> >>  1 files changed, 5 insertions(+), 1 deletions(-)
> >>
> >>diff --git a/drivers/of/base.c b/drivers/of/base.c
> >>index 7926d5d..a100a17 100644
> >>--- a/drivers/of/base.c
> >>+++ b/drivers/of/base.c
> >>@@ -1501,6 +1501,8 @@ struct device_node *of_new_node(struct device_node *parent, const char *name)
> >>  	return node;
> >>  }
> >>
> >>+static u32 empty_prop_value;
> >>+
> >>  struct property *of_new_property(struct device_node *node, const char *name,
> >>  		const void *data, int len)
> >>  {
> >>@@ -1515,6 +1517,7 @@ struct property *of_new_property(struct device_node *node, const char *name,
> >>  		goto bail_out;
> >>
> >>  	prop->length = len;
> >>+	prop->value =&empty_prop_value;
> >
> >This at least breaks of_set_property() and of_free(). Both unconditionally
> >do a free(pp->value).
> 
> Sascha,
> 
> this is patch 3/3, the two functions above are taken care of patch 2/3.

Ah, sorry, I missed that.

> 
> >Why do we need this anyway? We can always call of_find_property() to see
> >if a property exists.
> 
> Actually, I was preparing to import drivers/of/address.c from linux.
> of_translate_one uses of_get_property on "ranges". This returns
> the property's value pointer instead of the property itself, which
> is NULL for an empty ranges property. Now that you point it out,
> using of_find_property is also an option.
> 
> Nevertheless, patches 1 (with your comments applied) and 2 seem
> sensible to me. Patch 3 is an option to keep it in sync with Linux
> OF API behavior but is optional.

How about allocating zero length property values with malloc(0)? This
makes them less special.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/3] OF: base: add empty property value pointer

[Sebastian Hesselbarth]

On 06/23/2013 10:26 PM, Sascha Hauer wrote:
> On Sun, Jun 23, 2013 at 10:11:39PM +0200, Sebastian Hesselbarth wrote:
>> On 06/23/2013 08:33 PM, Sascha Hauer wrote:
>>> Why do we need this anyway? We can always call of_find_property() to see
>>> if a property exists.
>>
>> Actually, I was preparing to import drivers/of/address.c from linux.
>> of_translate_one uses of_get_property on "ranges". This returns
>> the property's value pointer instead of the property itself, which
>> is NULL for an empty ranges property. Now that you point it out,
>> using of_find_property is also an option.
>>
>> Nevertheless, patches 1 (with your comments applied) and 2 seem
>> sensible to me. Patch 3 is an option to keep it in sync with Linux
>> OF API behavior but is optional.
>
> How about allocating zero length property values with malloc(0)? This
> makes them less special.

Agree, I will update the patch set and send a v2 hopefully soon.

Sebastian


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/2] OF: base: add sanity checks to of_new/delete_property

[Sebastian Hesselbarth]

This adds some sanity checks to of_new_property and of_delete_property.
Also, value pointer is always allocated even with zero length to allow
empty properties to be distinguished from non-existing properties.
Finally, data passed to of_new_property is only copied if non-NULL.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Note: Patch 3 of the former patch set has been dropped in favour of zero
length allocation here.

Changelog:
v1->v2:
- remove unneccesary NULL checks after xzalloc (Suggested by Sascha Hauer)
- allocate zero length value pointer (Suggested by Sascha Hauer)

Cc: barebox@lists.infradead.org
---
 drivers/of/base.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 1b351ee..e65cf85 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1527,13 +1527,17 @@ struct property *of_new_property(struct device_node *node, const char *name,
 	struct property *prop;
 
 	prop = xzalloc(sizeof(*prop));
-
 	prop->name = strdup(name);
+	if (!prop->name) {
+		free(prop);
+		return NULL;
+	}
+
 	prop->length = len;
-	if (len) {
-		prop->value = xzalloc(len);
+	prop->value = xzalloc(len);
+
+	if (data)
 		memcpy(prop->value, data, len);
-	}
 
 	list_add_tail(&prop->list, &node->properties);
 
@@ -1542,6 +1546,9 @@ struct property *of_new_property(struct device_node *node, const char *name,
 
 void of_delete_property(struct property *pp)
 {
+	if (!pp)
+		return;
+
 	list_del(&pp->list);
 
 	free(pp->name);
-- 
1.7.2.5


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/2] OF: base: use of_delete_property where applicable

[Sebastian Hesselbarth]

This patch makes OF API use of_delete_property where applicable instead
of freeing allocated data in different places.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
v1->v2:
- remove offending whitespace lines

Cc: barebox@lists.infradead.org
---
 drivers/of/base.c |   81 ++++++++++++++++------------------------------------
 1 files changed, 25 insertions(+), 56 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index e65cf85..63ff647 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -899,16 +899,11 @@ int of_property_write_u8_array(struct device_node *np,
 	struct property *prop = of_find_property(np, propname, NULL);
 	u8 *val;
 
-	if (!prop)
-		prop = of_new_property(np, propname, NULL, 0);
-	if (!prop)
-		return -ENOMEM;
-
-	free(prop->value);
+	if (prop)
+		of_delete_property(prop);
 
-	prop->length = sizeof(*val) * sz;
-	prop->value = malloc(prop->length);
-	if (!prop->value)
+	prop = of_new_property(np, propname, NULL, sizeof(*val) * sz);
+	if (!prop)
 		return -ENOMEM;
 
 	val = prop->value;
@@ -940,16 +935,11 @@ int of_property_write_u16_array(struct device_node *np,
 	struct property *prop = of_find_property(np, propname, NULL);
 	__be16 *val;
 
-	if (!prop)
-		prop = of_new_property(np, propname, NULL, 0);
-	if (!prop)
-		return -ENOMEM;
-
-	free(prop->value);
+	if (prop)
+		of_delete_property(prop);
 
-	prop->length = sizeof(*val) * sz;
-	prop->value = malloc(prop->length);
-	if (!prop->value)
+	prop = of_new_property(np, propname, NULL, sizeof(*val) * sz);
+	if (!prop)
 		return -ENOMEM;
 
 	val = prop->value;
@@ -981,16 +971,11 @@ int of_property_write_u32_array(struct device_node *np,
 	struct property *prop = of_find_property(np, propname, NULL);
 	__be32 *val;
 
-	if (!prop)
-		prop = of_new_property(np, propname, NULL, 0);
-	if (!prop)
-		return -ENOMEM;
+	if (prop)
+		of_delete_property(prop);
 
-	free(prop->value);
-
-	prop->length = sizeof(*val) * sz;
-	prop->value = malloc(prop->length);
-	if (!prop->value)
+	prop = of_new_property(np, propname, NULL, sizeof(*val) * sz);
+	if (!prop)
 		return -ENOMEM;
 
 	val = prop->value;
@@ -1022,16 +1007,11 @@ int of_property_write_u64_array(struct device_node *np,
 	struct property *prop = of_find_property(np, propname, NULL);
 	__be32 *val;
 
-	if (!prop)
-		prop = of_new_property(np, propname, NULL, 0);
-	if (!prop)
-		return -ENOMEM;
+	if (prop)
+		of_delete_property(prop);
 
-	free(prop->value);
-
-	prop->length = 2 * sizeof(*val) * sz;
-	prop->value = malloc(prop->length);
-	if (!prop->value)
+	prop = of_new_property(np, propname, NULL, 2 * sizeof(*val) * sz);
+	if (!prop)
 		return -ENOMEM;
 
 	val = prop->value;
@@ -1567,26 +1547,19 @@ void of_delete_property(struct property *pp)
 int of_set_property(struct device_node *np, const char *name, const void *val, int len,
 		int create)
 {
-	struct property *pp;
+	struct property *pp = of_find_property(np, name, NULL);
 
 	if (!np)
 		return -ENOENT;
 
-	pp = of_find_property(np, name, NULL);
-	if (pp) {
-		void *data;
+	if (!pp && !create)
+		return -ENOENT;
 
-		free(pp->value);
-		data = xzalloc(len);
-		memcpy(data, val, len);
-		pp->value = data;
-		pp->length = len;
-	} else {
-		if (!create)
-			return -ENOENT;
+	of_delete_property(pp);
 
-		pp = of_new_property(np, name, val, len);
-	}
+	pp = of_new_property(np, name, val, len);
+	if (!pp)
+		return -ENOMEM;
 
 	return 0;
 }
@@ -1810,12 +1783,8 @@ void of_free(struct device_node *node)
 	if (!node)
 		return;
 
-	list_for_each_entry_safe(p, pt, &node->properties, list) {
-		list_del(&p->list);
-		free(p->name);
-		free(p->value);
-		free(p);
-	}
+	list_for_each_entry_safe(p, pt, &node->properties, list)
+		of_delete_property(p);
 
 	list_for_each_entry_safe(n, nt, &node->children, parent_list) {
 		of_free(n);
-- 
1.7.2.5


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 1/2] OF: base: add sanity checks to of_new/delete_property

[Sascha Hauer]

On Mon, Jun 24, 2013 at 12:49:20PM +0200, Sebastian Hesselbarth wrote:
> This adds some sanity checks to of_new_property and of_delete_property.
> Also, value pointer is always allocated even with zero length to allow
> empty properties to be distinguished from non-existing properties.
> Finally, data passed to of_new_property is only copied if non-NULL.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

Applied both.

Thanks
 Sascha

> ---
> Note: Patch 3 of the former patch set has been dropped in favour of zero
> length allocation here.
> 
> Changelog:
> v1->v2:
> - remove unneccesary NULL checks after xzalloc (Suggested by Sascha Hauer)
> - allocate zero length value pointer (Suggested by Sascha Hauer)
> 
> Cc: barebox@lists.infradead.org
> ---
>  drivers/of/base.c |   15 +++++++++++----
>  1 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 1b351ee..e65cf85 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1527,13 +1527,17 @@ struct property *of_new_property(struct device_node *node, const char *name,
>  	struct property *prop;
>  
>  	prop = xzalloc(sizeof(*prop));
> -
>  	prop->name = strdup(name);
> +	if (!prop->name) {
> +		free(prop);
> +		return NULL;
> +	}
> +
>  	prop->length = len;
> -	if (len) {
> -		prop->value = xzalloc(len);
> +	prop->value = xzalloc(len);
> +
> +	if (data)
>  		memcpy(prop->value, data, len);
> -	}
>  
>  	list_add_tail(&prop->list, &node->properties);
>  
> @@ -1542,6 +1546,9 @@ struct property *of_new_property(struct device_node *node, const char *name,
>  
>  void of_delete_property(struct property *pp)
>  {
> +	if (!pp)
> +		return;
> +
>  	list_del(&pp->list);
>  
>  	free(pp->name);
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox