[PATCH v2] fdtput: expand fdt if value does not fit (v2).

[PATCH v2] fdtput: expand fdt if value does not fit (v2).

[Srinivas KANDAGATLA]

From: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>

If you try to insert a new node or extend a property with large value,
using fdtput you will notice that it always fails.

example:
fdtput -v -p -ts ./tst.dtb "/node-1" "property-1" "value-1
Error at 'node-1': FDT_ERR_NOSPACE

or

fdtput -v -c ./tst.dtb "/node-1"
Error at 'node-1': FDT_ERR_NOSPACE

or

fdtput -v  -ts ./tst.dtb "/node" "property" "very big value"
Decoding value:
	string: 'very big value'
Value size 15
Error at 'property': FDT_ERR_NOSPACE

All these error are returned from libfdt, as the size of the fdt passed
has no space to accomdate these new properties.
This patch adds realloc functions in fdtput to allocate new space in fdt
when it detects a shortage in space for new value or node. With this
patch, fdtput can insert a new node or property or extend a property
with new value greater than original size. Also it packs the final blob
to clean up any extra padding.

Without this patch fdtput tool complains with FDT_ERR_NOSPACE when we
try to add a node/property or extend the value of a property.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
CC: David Gibson <dwg-8fk3Idey6ehBDgjK7y7TUQ@public.gmane.org>
---
 fdtput.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 72 insertions(+), 15 deletions(-)

diff --git a/fdtput.c b/fdtput.c
index f2197f5..3e17977 100644
--- a/fdtput.c
+++ b/fdtput.c
@@ -131,19 +131,72 @@ static int encode_value(struct display_info *disp, char **arg, int arg_count,
 	return 0;
 }
 
-static int store_key_value(void *blob, const char *node_name,
+#define ALIGN(x)		(((x) + (FDT_TAGSIZE) - 1) & ~((FDT_TAGSIZE) - 1))
+
+static char *_realloc_fdt(char *fdt, int delta)
+{
+	int new_sz;
+	char *new_fdt;
+
+	if (!delta)
+		return fdt;
+
+	new_sz = fdt_totalsize(fdt) + delta;
+	new_fdt = malloc(new_sz);
+	if (!new_fdt) {
+		fprintf(stderr, "Unable to allocate memory to new fdt\n");
+		return fdt;
+	}
+	fdt_open_into(fdt, new_fdt, new_sz);
+	free(fdt);
+	return new_fdt;
+}
+
+static char *realloc_node(char *fdt, const char *name)
+{
+	int delta = 0;
+	int newlen = strlen(name);
+	if (newlen)
+		delta = sizeof(struct fdt_node_header) +
+				ALIGN(newlen + 1) + FDT_TAGSIZE;
+
+	return _realloc_fdt(fdt, delta);
+}
+
+static char *realloc_property(char *fdt, int nodeoffset,
+		const char *name, int newlen)
+{
+	int delta = 0;
+	int oldlen = 0;
+
+	if (!fdt_get_property(fdt, nodeoffset, name, &oldlen))
+		/* strings + property header */
+		delta = sizeof(struct fdt_property) + strlen(name) + 1;
+
+	if (newlen > oldlen)
+		/* actual value in off_struct */
+		delta += ALIGN(newlen) - ALIGN(oldlen);
+
+	return _realloc_fdt(fdt, delta);
+}
+
+static int store_key_value(char **blob, const char *node_name,
 		const char *property, const char *buf, int len)
 {
 	int node;
 	int err;
 
-	node = fdt_path_offset(blob, node_name);
+	node = fdt_path_offset(*blob, node_name);
 	if (node < 0) {
 		report_error(node_name, -1, node);
 		return -1;
 	}
 
-	err = fdt_setprop(blob, node, property, buf, len);
+	err = fdt_setprop(*blob, node, property, buf, len);
+	if (err == -FDT_ERR_NOSPACE) {
+		*blob = realloc_property(*blob, node, property, len);
+		err = fdt_setprop(*blob, node, property, buf, len);
+	}
 	if (err) {
 		report_error(property, -1, err);
 		return -1;
@@ -161,7 +214,7 @@ static int store_key_value(void *blob, const char *node_name,
  * @param in_path	Path to process
  * @return 0 if ok, -1 on error
  */
-static int create_paths(void *blob, const char *in_path)
+static int create_paths(char **blob, const char *in_path)
 {
 	const char *path = in_path;
 	const char *sep;
@@ -177,10 +230,11 @@ static int create_paths(void *blob, const char *in_path)
 		if (!sep)
 			sep = path + strlen(path);
 
-		node = fdt_subnode_offset_namelen(blob, offset, path,
+		node = fdt_subnode_offset_namelen(*blob, offset, path,
 				sep - path);
 		if (node == -FDT_ERR_NOTFOUND) {
-			node = fdt_add_subnode_namelen(blob, offset, path,
+			*blob = realloc_node(*blob, path);
+			node = fdt_add_subnode_namelen(*blob, offset, path,
 						       sep - path);
 		}
 		if (node < 0) {
@@ -203,7 +257,7 @@ static int create_paths(void *blob, const char *in_path)
  * @param node_name	Name of node to create
  * @return new node offset if found, or -1 on failure
  */
-static int create_node(void *blob, const char *node_name)
+static int create_node(char **blob, const char *node_name)
 {
 	int node = 0;
 	char *p;
@@ -215,15 +269,17 @@ static int create_node(void *blob, const char *node_name)
 	}
 	*p = '\0';
 
+	*blob = realloc_node(*blob, p + 1);
+
 	if (p > node_name) {
-		node = fdt_path_offset(blob, node_name);
+		node = fdt_path_offset(*blob, node_name);
 		if (node < 0) {
 			report_error(node_name, -1, node);
 			return -1;
 		}
 	}
 
-	node = fdt_add_subnode(blob, node, p + 1);
+	node = fdt_add_subnode(*blob, node, p + 1);
 	if (node < 0) {
 		report_error(p + 1, -1, node);
 		return -1;
@@ -250,23 +306,25 @@ static int do_fdtput(struct display_info *disp, const char *filename,
 		 * store them into the property.
 		 */
 		assert(arg_count >= 2);
-		if (disp->auto_path && create_paths(blob, *arg))
+		if (disp->auto_path && create_paths(&blob, *arg))
 			return -1;
 		if (encode_value(disp, arg + 2, arg_count - 2, &value, &len) ||
-			store_key_value(blob, *arg, arg[1], value, len))
+			store_key_value(&blob, *arg, arg[1], value, len))
 			ret = -1;
 		break;
 	case OPER_CREATE_NODE:
 		for (; ret >= 0 && arg_count--; arg++) {
 			if (disp->auto_path)
-				ret = create_paths(blob, *arg);
+				ret = create_paths(&blob, *arg);
 			else
-				ret = create_node(blob, *arg);
+				ret = create_node(&blob, *arg);
 		}
 		break;
 	}
-	if (ret >= 0)
+	if (ret >= 0) {
+		fdt_pack(blob);
 		ret = utilfdt_write(filename, blob);
+	}
 
 	free(blob);
 	return ret;
@@ -317,7 +375,6 @@ int main(int argc, char *argv[])
 		 * - rename node
 		 * - pack fdt before writing
 		 * - set amount of free space when writing
-		 * - expand fdt if value doesn't fit
 		 */
 		switch (c) {
 		case 'c':
-- 
1.7.6.5

Re: [PATCH v2] fdtput: expand fdt if value does not fit (v2).

[Srinivas KANDAGATLA]

Hi David/Jon,

Are there any plans to take this patch?

Thanks,
srini
On 19/04/13 09:36, Srinivas KANDAGATLA wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
>
> If you try to insert a new node or extend a property with large value,
> using fdtput you will notice that it always fails.
>
> example:
> fdtput -v -p -ts ./tst.dtb "/node-1" "property-1" "value-1
> Error at 'node-1': FDT_ERR_NOSPACE
>
> or
>
> fdtput -v -c ./tst.dtb "/node-1"
> Error at 'node-1': FDT_ERR_NOSPACE
>
> or
>
> fdtput -v  -ts ./tst.dtb "/node" "property" "very big value"
> Decoding value:
> 	string: 'very big value'
> Value size 15
> Error at 'property': FDT_ERR_NOSPACE
>
> All these error are returned from libfdt, as the size of the fdt passed
> has no space to accomdate these new properties.
> This patch adds realloc functions in fdtput to allocate new space in fdt
> when it detects a shortage in space for new value or node. With this
> patch, fdtput can insert a new node or property or extend a property
> with new value greater than original size. Also it packs the final blob
> to clean up any extra padding.
>
> Without this patch fdtput tool complains with FDT_ERR_NOSPACE when we
> try to add a node/property or extend the value of a property.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
> CC: David Gibson <dwg-8fk3Idey6ehBDgjK7y7TUQ@public.gmane.org>
> ---
>  fdtput.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 72 insertions(+), 15 deletions(-)
>
> diff --git a/fdtput.c b/fdtput.c
> index f2197f5..3e17977 100644
> --- a/fdtput.c
> +++ b/fdtput.c
> @@ -131,19 +131,72 @@ static int encode_value(struct display_info *disp, char **arg, int arg_count,
>  	return 0;
>  }
>  
> -static int store_key_value(void *blob, const char *node_name,
> +#define ALIGN(x)		(((x) + (FDT_TAGSIZE) - 1) & ~((FDT_TAGSIZE) - 1))
> +
> +static char *_realloc_fdt(char *fdt, int delta)
> +{
> +	int new_sz;
> +	char *new_fdt;
> +
> +	if (!delta)
> +		return fdt;
> +
> +	new_sz = fdt_totalsize(fdt) + delta;
> +	new_fdt = malloc(new_sz);
> +	if (!new_fdt) {
> +		fprintf(stderr, "Unable to allocate memory to new fdt\n");
> +		return fdt;
> +	}
> +	fdt_open_into(fdt, new_fdt, new_sz);
> +	free(fdt);
> +	return new_fdt;
> +}
> +
> +static char *realloc_node(char *fdt, const char *name)
> +{
> +	int delta = 0;
> +	int newlen = strlen(name);
> +	if (newlen)
> +		delta = sizeof(struct fdt_node_header) +
> +				ALIGN(newlen + 1) + FDT_TAGSIZE;
> +
> +	return _realloc_fdt(fdt, delta);
> +}
> +
> +static char *realloc_property(char *fdt, int nodeoffset,
> +		const char *name, int newlen)
> +{
> +	int delta = 0;
> +	int oldlen = 0;
> +
> +	if (!fdt_get_property(fdt, nodeoffset, name, &oldlen))
> +		/* strings + property header */
> +		delta = sizeof(struct fdt_property) + strlen(name) + 1;
> +
> +	if (newlen > oldlen)
> +		/* actual value in off_struct */
> +		delta += ALIGN(newlen) - ALIGN(oldlen);
> +
> +	return _realloc_fdt(fdt, delta);
> +}
> +
> +static int store_key_value(char **blob, const char *node_name,
>  		const char *property, const char *buf, int len)
>  {
>  	int node;
>  	int err;
>  
> -	node = fdt_path_offset(blob, node_name);
> +	node = fdt_path_offset(*blob, node_name);
>  	if (node < 0) {
>  		report_error(node_name, -1, node);
>  		return -1;
>  	}
>  
> -	err = fdt_setprop(blob, node, property, buf, len);
> +	err = fdt_setprop(*blob, node, property, buf, len);
> +	if (err == -FDT_ERR_NOSPACE) {
> +		*blob = realloc_property(*blob, node, property, len);
> +		err = fdt_setprop(*blob, node, property, buf, len);
> +	}
>  	if (err) {
>  		report_error(property, -1, err);
>  		return -1;
> @@ -161,7 +214,7 @@ static int store_key_value(void *blob, const char *node_name,
>   * @param in_path	Path to process
>   * @return 0 if ok, -1 on error
>   */
> -static int create_paths(void *blob, const char *in_path)
> +static int create_paths(char **blob, const char *in_path)
>  {
>  	const char *path = in_path;
>  	const char *sep;
> @@ -177,10 +230,11 @@ static int create_paths(void *blob, const char *in_path)
>  		if (!sep)
>  			sep = path + strlen(path);
>  
> -		node = fdt_subnode_offset_namelen(blob, offset, path,
> +		node = fdt_subnode_offset_namelen(*blob, offset, path,
>  				sep - path);
>  		if (node == -FDT_ERR_NOTFOUND) {
> -			node = fdt_add_subnode_namelen(blob, offset, path,
> +			*blob = realloc_node(*blob, path);
> +			node = fdt_add_subnode_namelen(*blob, offset, path,
>  						       sep - path);
>  		}
>  		if (node < 0) {
> @@ -203,7 +257,7 @@ static int create_paths(void *blob, const char *in_path)
>   * @param node_name	Name of node to create
>   * @return new node offset if found, or -1 on failure
>   */
> -static int create_node(void *blob, const char *node_name)
> +static int create_node(char **blob, const char *node_name)
>  {
>  	int node = 0;
>  	char *p;
> @@ -215,15 +269,17 @@ static int create_node(void *blob, const char *node_name)
>  	}
>  	*p = '\0';
>  
> +	*blob = realloc_node(*blob, p + 1);
> +
>  	if (p > node_name) {
> -		node = fdt_path_offset(blob, node_name);
> +		node = fdt_path_offset(*blob, node_name);
>  		if (node < 0) {
>  			report_error(node_name, -1, node);
>  			return -1;
>  		}
>  	}
>  
> -	node = fdt_add_subnode(blob, node, p + 1);
> +	node = fdt_add_subnode(*blob, node, p + 1);
>  	if (node < 0) {
>  		report_error(p + 1, -1, node);
>  		return -1;
> @@ -250,23 +306,25 @@ static int do_fdtput(struct display_info *disp, const char *filename,
>  		 * store them into the property.
>  		 */
>  		assert(arg_count >= 2);
> -		if (disp->auto_path && create_paths(blob, *arg))
> +		if (disp->auto_path && create_paths(&blob, *arg))
>  			return -1;
>  		if (encode_value(disp, arg + 2, arg_count - 2, &value, &len) ||
> -			store_key_value(blob, *arg, arg[1], value, len))
> +			store_key_value(&blob, *arg, arg[1], value, len))
>  			ret = -1;
>  		break;
>  	case OPER_CREATE_NODE:
>  		for (; ret >= 0 && arg_count--; arg++) {
>  			if (disp->auto_path)
> -				ret = create_paths(blob, *arg);
> +				ret = create_paths(&blob, *arg);
>  			else
> -				ret = create_node(blob, *arg);
> +				ret = create_node(&blob, *arg);
>  		}
>  		break;
>  	}
> -	if (ret >= 0)
> +	if (ret >= 0) {
> +		fdt_pack(blob);
>  		ret = utilfdt_write(filename, blob);
> +	}
>  
>  	free(blob);
>  	return ret;
> @@ -317,7 +375,6 @@ int main(int argc, char *argv[])
>  		 * - rename node
>  		 * - pack fdt before writing
>  		 * - set amount of free space when writing
> -		 * - expand fdt if value doesn't fit
>  		 */
>  		switch (c) {
>  		case 'c':

Re: [PATCH v2] fdtput: expand fdt if value does not fit (v2).

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 7694 bytes --]

On Fri, Apr 19, 2013 at 09:36:36AM +0100, Srinivas KANDAGATLA wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
> 
> If you try to insert a new node or extend a property with large value,
> using fdtput you will notice that it always fails.
> 
> example:
> fdtput -v -p -ts ./tst.dtb "/node-1" "property-1" "value-1
> Error at 'node-1': FDT_ERR_NOSPACE
> 
> or
> 
> fdtput -v -c ./tst.dtb "/node-1"
> Error at 'node-1': FDT_ERR_NOSPACE
> 
> or
> 
> fdtput -v  -ts ./tst.dtb "/node" "property" "very big value"
> Decoding value:
> 	string: 'very big value'
> Value size 15
> Error at 'property': FDT_ERR_NOSPACE
> 
> All these error are returned from libfdt, as the size of the fdt passed
> has no space to accomdate these new properties.
> This patch adds realloc functions in fdtput to allocate new space in fdt
> when it detects a shortage in space for new value or node. With this
> patch, fdtput can insert a new node or property or extend a property
> with new value greater than original size. Also it packs the final blob
> to clean up any extra padding.
> 
> Without this patch fdtput tool complains with FDT_ERR_NOSPACE when we
> try to add a node/property or extend the value of a property.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
> CC: David Gibson <dwg-8fk3Idey6ehBDgjK7y7TUQ@public.gmane.org>
> ---
>  fdtput.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 72 insertions(+), 15 deletions(-)
> 
> diff --git a/fdtput.c b/fdtput.c
> index f2197f5..3e17977 100644
> --- a/fdtput.c
> +++ b/fdtput.c
> @@ -131,19 +131,72 @@ static int encode_value(struct display_info *disp, char **arg, int arg_count,
>  	return 0;
>  }
>  
> -static int store_key_value(void *blob, const char *node_name,
> +#define ALIGN(x)		(((x) + (FDT_TAGSIZE) - 1) & ~((FDT_TAGSIZE) - 1))
> +
> +static char *_realloc_fdt(char *fdt, int delta)
> +{
> +	int new_sz;
> +	char *new_fdt;
> +
> +	if (!delta)
> +		return fdt;
> +
> +	new_sz = fdt_totalsize(fdt) + delta;
> +	new_fdt = malloc(new_sz);

Might as well use realloc() here.  fdt_open_into() is (by design) safe
in both the overlapping and non-overlapping cases.

Also, use xrealloc()/xmalloc() instead of plain realloc()/malloc().
Your caller doesn't actually handle the failure case here when this
returns NULL, and there's not a whole lot you can do, so the abort()
is about the best you can do, and simplifies the code here.

> +	if (!new_fdt) {
> +		fprintf(stderr, "Unable to allocate memory to new fdt\n");
> +		return fdt;
> +	}
> +	fdt_open_into(fdt, new_fdt, new_sz);
> +	free(fdt);
> +	return new_fdt;
> +}
> +
> +static char *realloc_node(char *fdt, const char *name)
> +{
> +	int delta = 0;
> +	int newlen = strlen(name);
> +	if (newlen)

I don't see any point to this test.  Adding a node named "" would be
an error in other ways, but it would still require 8 bytes of extra
space.

> +		delta = sizeof(struct fdt_node_header) +
> +				ALIGN(newlen + 1) + FDT_TAGSIZE;

struct fdt_node_header already includes the FDT_BEGIN_NODE tag, so
you've allocated space for the tag twice.

> +
> +	return _realloc_fdt(fdt, delta);
> +}
> +
> +static char *realloc_property(char *fdt, int nodeoffset,
> +		const char *name, int newlen)
> +{
> +	int delta = 0;
> +	int oldlen = 0;
> +
> +	if (!fdt_get_property(fdt, nodeoffset, name, &oldlen))
> +		/* strings + property header */
> +		delta = sizeof(struct fdt_property) + strlen(name) + 1;
> +
> +	if (newlen > oldlen)
> +		/* actual value in off_struct */
> +		delta += ALIGN(newlen) - ALIGN(oldlen);
> +
> +	return _realloc_fdt(fdt, delta);
> +}
> +
> +static int store_key_value(char **blob, const char *node_name,
>  		const char *property, const char *buf, int len)
>  {
>  	int node;
>  	int err;
>  
> -	node = fdt_path_offset(blob, node_name);
> +	node = fdt_path_offset(*blob, node_name);
>  	if (node < 0) {
>  		report_error(node_name, -1, node);
>  		return -1;
>  	}
>  
> -	err = fdt_setprop(blob, node, property, buf, len);
> +	err = fdt_setprop(*blob, node, property, buf, len);
> +	if (err == -FDT_ERR_NOSPACE) {
> +		*blob = realloc_property(*blob, node, property, len);
> +		err = fdt_setprop(*blob, node, property, buf, len);
> +	}
>  	if (err) {
>  		report_error(property, -1, err);
>  		return -1;
> @@ -161,7 +214,7 @@ static int store_key_value(void *blob, const char *node_name,
>   * @param in_path	Path to process
>   * @return 0 if ok, -1 on error
>   */
> -static int create_paths(void *blob, const char *in_path)
> +static int create_paths(char **blob, const char *in_path)
>  {
>  	const char *path = in_path;
>  	const char *sep;
> @@ -177,10 +230,11 @@ static int create_paths(void *blob, const char *in_path)
>  		if (!sep)
>  			sep = path + strlen(path);
>  
> -		node = fdt_subnode_offset_namelen(blob, offset, path,
> +		node = fdt_subnode_offset_namelen(*blob, offset, path,
>  				sep - path);
>  		if (node == -FDT_ERR_NOTFOUND) {
> -			node = fdt_add_subnode_namelen(blob, offset, path,
> +			*blob = realloc_node(*blob, path);
> +			node = fdt_add_subnode_namelen(*blob, offset, path,
>  						       sep - path);
>  		}
>  		if (node < 0) {
> @@ -203,7 +257,7 @@ static int create_paths(void *blob, const char *in_path)
>   * @param node_name	Name of node to create
>   * @return new node offset if found, or -1 on failure
>   */
> -static int create_node(void *blob, const char *node_name)
> +static int create_node(char **blob, const char *node_name)
>  {
>  	int node = 0;
>  	char *p;
> @@ -215,15 +269,17 @@ static int create_node(void *blob, const char *node_name)
>  	}
>  	*p = '\0';
>  
> +	*blob = realloc_node(*blob, p + 1);
> +
>  	if (p > node_name) {
> -		node = fdt_path_offset(blob, node_name);
> +		node = fdt_path_offset(*blob, node_name);
>  		if (node < 0) {
>  			report_error(node_name, -1, node);
>  			return -1;
>  		}
>  	}
>  
> -	node = fdt_add_subnode(blob, node, p + 1);
> +	node = fdt_add_subnode(*blob, node, p + 1);
>  	if (node < 0) {
>  		report_error(p + 1, -1, node);
>  		return -1;
> @@ -250,23 +306,25 @@ static int do_fdtput(struct display_info *disp, const char *filename,
>  		 * store them into the property.
>  		 */
>  		assert(arg_count >= 2);
> -		if (disp->auto_path && create_paths(blob, *arg))
> +		if (disp->auto_path && create_paths(&blob, *arg))
>  			return -1;
>  		if (encode_value(disp, arg + 2, arg_count - 2, &value, &len) ||
> -			store_key_value(blob, *arg, arg[1], value, len))
> +			store_key_value(&blob, *arg, arg[1], value, len))
>  			ret = -1;
>  		break;
>  	case OPER_CREATE_NODE:
>  		for (; ret >= 0 && arg_count--; arg++) {
>  			if (disp->auto_path)
> -				ret = create_paths(blob, *arg);
> +				ret = create_paths(&blob, *arg);
>  			else
> -				ret = create_node(blob, *arg);
> +				ret = create_node(&blob, *arg);
>  		}
>  		break;
>  	}
> -	if (ret >= 0)
> +	if (ret >= 0) {
> +		fdt_pack(blob);
>  		ret = utilfdt_write(filename, blob);
> +	}
>  
>  	free(blob);
>  	return ret;
> @@ -317,7 +375,6 @@ int main(int argc, char *argv[])
>  		 * - rename node
>  		 * - pack fdt before writing
>  		 * - set amount of free space when writing
> -		 * - expand fdt if value doesn't fit
>  		 */
>  		switch (c) {
>  		case 'c':

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v2] fdtput: expand fdt if value does not fit (v2).

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 413 bytes --]

On Mon, Apr 29, 2013 at 07:39:58AM +0100, Srinivas KANDAGATLA wrote:
> Hi David/Jon,
> 
> Are there any plans to take this patch?

Sorry, been sidetracked by other things.  Just sent another round of
comments.

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v2] fdtput: expand fdt if value does not fit (v2).

[Srinivas KANDAGATLA]

On 29/04/13 10:08, David Gibson wrote:
> On Fri, Apr 19, 2013 at 09:36:36AM +0100, Srinivas KANDAGATLA wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
>>
>> If you try to insert a new node or extend a property with large value,
>> using fdtput you will notice that it always fails.
>>
>> example:
>> fdtput -v -p -ts ./tst.dtb "/node-1" "property-1" "value-1
>> Error at 'node-1': FDT_ERR_NOSPACE
>>
>> or
>>
>> fdtput -v -c ./tst.dtb "/node-1"
>> Error at 'node-1': FDT_ERR_NOSPACE
>>
>> or
>>
>> fdtput -v  -ts ./tst.dtb "/node" "property" "very big value"
>> Decoding value:
>> 	string: 'very big value'
>> Value size 15
>> Error at 'property': FDT_ERR_NOSPACE
>>
>> All these error are returned from libfdt, as the size of the fdt passed
>> has no space to accomdate these new properties.
>> This patch adds realloc functions in fdtput to allocate new space in fdt
>> when it detects a shortage in space for new value or node. With this
>> patch, fdtput can insert a new node or property or extend a property
>> with new value greater than original size. Also it packs the final blob
>> to clean up any extra padding.
>>
>> Without this patch fdtput tool complains with FDT_ERR_NOSPACE when we
>> try to add a node/property or extend the value of a property.
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
>> CC: David Gibson <dwg-8fk3Idey6ehBDgjK7y7TUQ@public.gmane.org>
>> ---
>>  fdtput.c |   87 +++++++++++++++++++++++++++++++++++++++++++++++++++----------
>>  1 files changed, 72 insertions(+), 15 deletions(-)
>>
>> diff --git a/fdtput.c b/fdtput.c
>> index f2197f5..3e17977 100644
>> --- a/fdtput.c
>> +++ b/fdtput.c
>> @@ -131,19 +131,72 @@ static int encode_value(struct display_info *disp, char **arg, int arg_count,
>>  	return 0;
>>  }
>>  
>> -static int store_key_value(void *blob, const char *node_name,
>> +#define ALIGN(x)		(((x) + (FDT_TAGSIZE) - 1) & ~((FDT_TAGSIZE) - 1))
>> +
>> +static char *_realloc_fdt(char *fdt, int delta)
>> +{
>> +	int new_sz;
>> +	char *new_fdt;
>> +
>> +	if (!delta)
>> +		return fdt;
>> +
>> +	new_sz = fdt_totalsize(fdt) + delta;
>> +	new_fdt = malloc(new_sz);
> Might as well use realloc() here.  fdt_open_into() is (by design) safe
> in both the overlapping and non-overlapping cases.
>
> Also, use xrealloc()/xmalloc() instead of plain realloc()/malloc().
> Your caller doesn't actually handle the failure case here when this
> returns NULL, and there's not a whole lot you can do, so the abort()
> is about the best you can do, and simplifies the code here.
Yes, you are right, we should about if we fail to allocate memory.
>> +	if (!new_fdt) {
>> +		fprintf(stderr, "Unable to allocate memory to new fdt\n");
>> +		return fdt;
>> +	}
>> +	fdt_open_into(fdt, new_fdt, new_sz);
>> +	free(fdt);
>> +	return new_fdt;
>> +}
>> +
>> +static char *realloc_node(char *fdt, const char *name)
>> +{
>> +	int delta = 0;
>> +	int newlen = strlen(name);
>> +	if (newlen)
> I don't see any point to this test.  Adding a node named "" would be
> an error in other ways, but it would still require 8 bytes of extra
> space.
Ok.
>
>> +		delta = sizeof(struct fdt_node_header) +
>> +				ALIGN(newlen + 1) + FDT_TAGSIZE;
> struct fdt_node_header already includes the FDT_BEGIN_NODE tag, so
> you've allocated space for the tag twice.
Here am allocating space for FDT_BEGIN_NODE + node name in off_struct +
FDT_END_NODE.
So am not allocating space for the tag twice.
Am I missing something?

thanks,
srini
>
>> +
>> +	return _realloc_fdt(fdt, delta);
>> +}
>> +
>> +static char *realloc_property(char *fdt, int nodeoffset,
>> +		const char *name, int newlen)
>> +{
>> +	int delta = 0;
>> +	int oldlen = 0;
>> +
>> +	if (!fdt_get_property(fdt, nodeoffset, name, &oldlen))
>> +		/* strings + property header */
>> +		delta = sizeof(struct fdt_property) + strlen(name) + 1;
>> +
>> +	if (newlen > oldlen)
>> +		/* actual value in off_struct */
>> +		delta += ALIGN(newlen) - ALIGN(oldlen);
>> +
>> +	return _realloc_fdt(fdt, delta);
>> +}
>> +
>> +static int store_key_value(char **blob, const char *node_name,
>>  		const char *property, const char *buf, int len)
>>  {
>>  	int node;
>>  	int err;
>>  
>> -	node = fdt_path_offset(blob, node_name);
>> +	node = fdt_path_offset(*blob, node_name);
>>  	if (node < 0) {
>>  		report_error(node_name, -1, node);
>>  		return -1;
>>  	}
>>  
>> -	err = fdt_setprop(blob, node, property, buf, len);
>> +	err = fdt_setprop(*blob, node, property, buf, len);
>> +	if (err == -FDT_ERR_NOSPACE) {
>> +		*blob = realloc_property(*blob, node, property, len);
>> +		err = fdt_setprop(*blob, node, property, buf, len);
>> +	}
>>  	if (err) {
>>  		report_error(property, -1, err);
>>  		return -1;
>> @@ -161,7 +214,7 @@ static int store_key_value(void *blob, const char *node_name,
>>   * @param in_path	Path to process
>>   * @return 0 if ok, -1 on error
>>   */
>> -static int create_paths(void *blob, const char *in_path)
>> +static int create_paths(char **blob, const char *in_path)
>>  {
>>  	const char *path = in_path;
>>  	const char *sep;
>> @@ -177,10 +230,11 @@ static int create_paths(void *blob, const char *in_path)
>>  		if (!sep)
>>  			sep = path + strlen(path);
>>  
>> -		node = fdt_subnode_offset_namelen(blob, offset, path,
>> +		node = fdt_subnode_offset_namelen(*blob, offset, path,
>>  				sep - path);
>>  		if (node == -FDT_ERR_NOTFOUND) {
>> -			node = fdt_add_subnode_namelen(blob, offset, path,
>> +			*blob = realloc_node(*blob, path);
>> +			node = fdt_add_subnode_namelen(*blob, offset, path,
>>  						       sep - path);
>>  		}
>>  		if (node < 0) {
>> @@ -203,7 +257,7 @@ static int create_paths(void *blob, const char *in_path)
>>   * @param node_name	Name of node to create
>>   * @return new node offset if found, or -1 on failure
>>   */
>> -static int create_node(void *blob, const char *node_name)
>> +static int create_node(char **blob, const char *node_name)
>>  {
>>  	int node = 0;
>>  	char *p;
>> @@ -215,15 +269,17 @@ static int create_node(void *blob, const char *node_name)
>>  	}
>>  	*p = '\0';
>>  
>> +	*blob = realloc_node(*blob, p + 1);
>> +
>>  	if (p > node_name) {
>> -		node = fdt_path_offset(blob, node_name);
>> +		node = fdt_path_offset(*blob, node_name);
>>  		if (node < 0) {
>>  			report_error(node_name, -1, node);
>>  			return -1;
>>  		}
>>  	}
>>  
>> -	node = fdt_add_subnode(blob, node, p + 1);
>> +	node = fdt_add_subnode(*blob, node, p + 1);
>>  	if (node < 0) {
>>  		report_error(p + 1, -1, node);
>>  		return -1;
>> @@ -250,23 +306,25 @@ static int do_fdtput(struct display_info *disp, const char *filename,
>>  		 * store them into the property.
>>  		 */
>>  		assert(arg_count >= 2);
>> -		if (disp->auto_path && create_paths(blob, *arg))
>> +		if (disp->auto_path && create_paths(&blob, *arg))
>>  			return -1;
>>  		if (encode_value(disp, arg + 2, arg_count - 2, &value, &len) ||
>> -			store_key_value(blob, *arg, arg[1], value, len))
>> +			store_key_value(&blob, *arg, arg[1], value, len))
>>  			ret = -1;
>>  		break;
>>  	case OPER_CREATE_NODE:
>>  		for (; ret >= 0 && arg_count--; arg++) {
>>  			if (disp->auto_path)
>> -				ret = create_paths(blob, *arg);
>> +				ret = create_paths(&blob, *arg);
>>  			else
>> -				ret = create_node(blob, *arg);
>> +				ret = create_node(&blob, *arg);
>>  		}
>>  		break;
>>  	}
>> -	if (ret >= 0)
>> +	if (ret >= 0) {
>> +		fdt_pack(blob);
>>  		ret = utilfdt_write(filename, blob);
>> +	}
>>  
>>  	free(blob);
>>  	return ret;
>> @@ -317,7 +375,6 @@ int main(int argc, char *argv[])
>>  		 * - rename node
>>  		 * - pack fdt before writing
>>  		 * - set amount of free space when writing
>> -		 * - expand fdt if value doesn't fit
>>  		 */
>>  		switch (c) {
>>  		case 'c':
>
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v2] fdtput: expand fdt if value does not fit (v2).

[David Gibson]

[-- Attachment #1.1: Type: text/plain, Size: 988 bytes --]

On Tue, Apr 30, 2013 at 07:49:16AM +0100, Srinivas KANDAGATLA wrote:
> On 29/04/13 10:08, David Gibson wrote:
> > On Fri, Apr 19, 2013 at 09:36:36AM +0100, Srinivas KANDAGATLA wrote:
> >> From: Srinivas Kandagatla <srinivas.kandagatla-qxv4g6HH51o@public.gmane.org>
[snip]
> >
> >> +		delta = sizeof(struct fdt_node_header) +
> >> +				ALIGN(newlen + 1) + FDT_TAGSIZE;
> > struct fdt_node_header already includes the FDT_BEGIN_NODE tag, so
> > you've allocated space for the tag twice.
> Here am allocating space for FDT_BEGIN_NODE + node name in off_struct +
> FDT_END_NODE.
> So am not allocating space for the tag twice.
> Am I missing something?

Oh, yes, sorry.  Maybe put /* FDT_END_NODE */ next to the extra
FDT_TAGSIZE to stop someone else missing this the way I did.

-- 
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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss