[PATCH 1/5] dtc: Simplify asm_emit_string() implementation

[PATCH 1/5] dtc: Simplify asm_emit_string() implementation

[Nicolas Iooss]

Using %.*s format helps making asm_emit_string() not modify its "str"
parameter.

While at it, constify the "str" parameter of bin_emit_string() and
asm_emit_string(), as these function no longer modify it.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
---
 flattree.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/flattree.c b/flattree.c
index ebac548b3fa8..d8a118258e01 100644
--- a/flattree.c
+++ b/flattree.c
@@ -49,7 +49,7 @@ static struct version_info {
 
 struct emitter {
 	void (*cell)(void *, cell_t);
-	void (*string)(void *, char *, int);
+	void (*string)(void *, const char *, int);
 	void (*align)(void *, int);
 	void (*data)(void *, struct data);
 	void (*beginnode)(void *, struct label *labels);
@@ -64,7 +64,7 @@ static void bin_emit_cell(void *e, cell_t val)
 	*dtbuf = data_append_cell(*dtbuf, val);
 }
 
-static void bin_emit_string(void *e, char *str, int len)
+static void bin_emit_string(void *e, const char *str, int len)
 {
 	struct data *dtbuf = e;
 
@@ -144,22 +144,14 @@ static void asm_emit_cell(void *e, cell_t val)
 		(val >> 8) & 0xff, val & 0xff);
 }
 
-static void asm_emit_string(void *e, char *str, int len)
+static void asm_emit_string(void *e, const char *str, int len)
 {
 	FILE *f = e;
-	char c = 0;
 
-	if (len != 0) {
-		/* XXX: ewww */
-		c = str[len];
-		str[len] = '\0';
-	}
-
-	fprintf(f, "\t.string\t\"%s\"\n", str);
-
-	if (len != 0) {
-		str[len] = c;
-	}
+	if (len != 0)
+		fprintf(f, "\t.string\t\"%.*s\"\n", len, str);
+	else
+		fprintf(f, "\t.string\t\"%s\"\n", str);
 }
 
 static void asm_emit_align(void *e, int a)
-- 
2.11.1

[PATCH 2/5] libfdt: overlay: Check the value of the right variable

[Nicolas Iooss]

overlay_update_local_node_references() saves the result of
fdt_subnode_offset() into variable tree_child but checks for variable
ret afterwards. As this does not make sense, check tree_child instead of
ret.

This bug has been found by compiling with clang. The compiler reported
the following warning:

    libfdt/fdt_overlay.c:275:7: error: variable 'ret' may be
    uninitialized when used here
          [-Werror,-Wconditional-uninitialized]
                    if (ret == -FDT_ERR_NOTFOUND)
                        ^~~
    libfdt/fdt_overlay.c:210:9: note: initialize the variable 'ret' to
    silence this
          warning
            int ret;
                   ^
                    = 0

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
---
 libfdt/fdt_overlay.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index 56cb70ed4445..b2fb5d970ccc 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -272,7 +272,7 @@ static int overlay_update_local_node_references(void *fdto,
 
 		tree_child = fdt_subnode_offset(fdto, tree_node,
 						fixup_child_name);
-		if (ret == -FDT_ERR_NOTFOUND)
+		if (tree_child == -FDT_ERR_NOTFOUND)
 			return -FDT_ERR_BADOVERLAY;
 		if (tree_child < 0)
 			return tree_child;
-- 
2.11.1

Re: [PATCH 2/5] libfdt: overlay: Check the value of the right variable

[Simon Glass]

On 4 March 2017 at 06:26, Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> wrote:
> overlay_update_local_node_references() saves the result of
> fdt_subnode_offset() into variable tree_child but checks for variable
> ret afterwards. As this does not make sense, check tree_child instead of
> ret.
>
> This bug has been found by compiling with clang. The compiler reported
> the following warning:
>
>     libfdt/fdt_overlay.c:275:7: error: variable 'ret' may be
>     uninitialized when used here
>           [-Werror,-Wconditional-uninitialized]
>                     if (ret == -FDT_ERR_NOTFOUND)
>                         ^~~
>     libfdt/fdt_overlay.c:210:9: note: initialize the variable 'ret' to
>     silence this
>           warning
>             int ret;
>                    ^
>                     = 0
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> ---
>  libfdt/fdt_overlay.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

This suggests we need a new test case.

>
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 56cb70ed4445..b2fb5d970ccc 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -272,7 +272,7 @@ static int overlay_update_local_node_references(void *fdto,
>
>                 tree_child = fdt_subnode_offset(fdto, tree_node,
>                                                 fixup_child_name);
> -               if (ret == -FDT_ERR_NOTFOUND)
> +               if (tree_child == -FDT_ERR_NOTFOUND)
>                         return -FDT_ERR_BADOVERLAY;
>                 if (tree_child < 0)
>                         return tree_child;
> --
> 2.11.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] libfdt: overlay: Check the value of the right variable

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2229 bytes --]

On Wed, Mar 08, 2017 at 02:01:44PM -0700, Simon Glass wrote:
> On 4 March 2017 at 06:26, Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> wrote:
> > overlay_update_local_node_references() saves the result of
> > fdt_subnode_offset() into variable tree_child but checks for variable
> > ret afterwards. As this does not make sense, check tree_child instead of
> > ret.
> >
> > This bug has been found by compiling with clang. The compiler reported
> > the following warning:
> >
> >     libfdt/fdt_overlay.c:275:7: error: variable 'ret' may be
> >     uninitialized when used here
> >           [-Werror,-Wconditional-uninitialized]
> >                     if (ret == -FDT_ERR_NOTFOUND)
> >                         ^~~
> >     libfdt/fdt_overlay.c:210:9: note: initialize the variable 'ret' to
> >     silence this
> >           warning
> >             int ret;
> >                    ^
> >                     = 0
> >
> > Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> > ---
> >  libfdt/fdt_overlay.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> This suggests we need a new test case.

True.  I've already merged this but adding a testcase as a followup
would be most appreciated.

> 
> >
> > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> > index 56cb70ed4445..b2fb5d970ccc 100644
> > --- a/libfdt/fdt_overlay.c
> > +++ b/libfdt/fdt_overlay.c
> > @@ -272,7 +272,7 @@ static int overlay_update_local_node_references(void *fdto,
> >
> >                 tree_child = fdt_subnode_offset(fdto, tree_node,
> >                                                 fixup_child_name);
> > -               if (ret == -FDT_ERR_NOTFOUND)
> > +               if (tree_child == -FDT_ERR_NOTFOUND)
> >                         return -FDT_ERR_BADOVERLAY;
> >                 if (tree_child < 0)
> >                         return tree_child;
> >

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

[PATCH 3/5] tests: Make realloc_fdt() really allocate *fdt

[Nicolas Iooss]

This bug has been found by using clang Static Analyzer: it reported that
the value stored to fdt was never read.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
---
 tests/sw_tree1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/sw_tree1.c b/tests/sw_tree1.c
index 6d4c53102967..4887dc3e1172 100644
--- a/tests/sw_tree1.c
+++ b/tests/sw_tree1.c
@@ -42,14 +42,14 @@ static void realloc_fdt(void **fdt, size_t *size, bool created)
 	switch (alloc_mode) {
 	case FIXED:
 		if (!(*fdt))
-			fdt = xmalloc(*size);
+			*fdt = xmalloc(*size);
 		else
 			FAIL("Ran out of space");
 		return;
 
 	case RESIZE:
 		if (!(*fdt)) {
-			fdt = xmalloc(SPACE);
+			*fdt = xmalloc(SPACE);
 		} else if (*size < SPACE) {
 			*size += 1;
 			fdt_resize(*fdt, *fdt, *size);
-- 
2.11.1

Re: [PATCH 3/5] tests: Make realloc_fdt() really allocate *fdt

[Simon Glass]

On 4 March 2017 at 06:26, Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> wrote:
> This bug has been found by using clang Static Analyzer: it reported that
> the value stored to fdt was never read.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> ---
>  tests/sw_tree1.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

I suggest changing the code in main() so it starts with a 0 size. Then
the test would catch this problem.

>
> diff --git a/tests/sw_tree1.c b/tests/sw_tree1.c
> index 6d4c53102967..4887dc3e1172 100644
> --- a/tests/sw_tree1.c
> +++ b/tests/sw_tree1.c
> @@ -42,14 +42,14 @@ static void realloc_fdt(void **fdt, size_t *size, bool created)
>         switch (alloc_mode) {
>         case FIXED:
>                 if (!(*fdt))
> -                       fdt = xmalloc(*size);
> +                       *fdt = xmalloc(*size);
>                 else
>                         FAIL("Ran out of space");
>                 return;
>
>         case RESIZE:
>                 if (!(*fdt)) {
> -                       fdt = xmalloc(SPACE);
> +                       *fdt = xmalloc(SPACE);
>                 } else if (*size < SPACE) {
>                         *size += 1;
>                         fdt_resize(*fdt, *fdt, *size);
> --
> 2.11.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] tests: Make realloc_fdt() really allocate *fdt

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

On Wed, Mar 08, 2017 at 02:01:47PM -0700, Simon Glass wrote:
> On 4 March 2017 at 06:26, Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> wrote:
> > This bug has been found by using clang Static Analyzer: it reported that
> > the value stored to fdt was never read.
> >
> > Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> > ---
> >  tests/sw_tree1.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> I suggest changing the code in main() so it starts with a 0 size. Then
> the test would catch this problem.

Ah, yes, I was going to suggest this when I reviewed the series, but
then I forgot.

> 
> >
> > diff --git a/tests/sw_tree1.c b/tests/sw_tree1.c
> > index 6d4c53102967..4887dc3e1172 100644
> > --- a/tests/sw_tree1.c
> > +++ b/tests/sw_tree1.c
> > @@ -42,14 +42,14 @@ static void realloc_fdt(void **fdt, size_t *size, bool created)
> >         switch (alloc_mode) {
> >         case FIXED:
> >                 if (!(*fdt))
> > -                       fdt = xmalloc(*size);
> > +                       *fdt = xmalloc(*size);
> >                 else
> >                         FAIL("Ran out of space");
> >                 return;
> >
> >         case RESIZE:
> >                 if (!(*fdt)) {
> > -                       fdt = xmalloc(SPACE);
> > +                       *fdt = xmalloc(SPACE);
> >                 } else if (*size < SPACE) {
> >                         *size += 1;
> >                         fdt_resize(*fdt, *fdt, *size);
> >

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

[PATCH 4/5] fdtget: Use @return to document the return value

[Nicolas Iooss]

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
---
 fdtget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fdtget.c b/fdtget.c
index fb9d0e1730d7..2982a3e7d960 100644
--- a/fdtget.c
+++ b/fdtget.c
@@ -245,7 +245,7 @@ static int show_data_for_item(const void *blob, struct display_info *disp,
  * @param filename	Filename of blob file
  * @param arg		List of arguments to process
  * @param arg_count	Number of arguments
- * @param return 0 if ok, -ve on error
+ * @return 0 if ok, -ve on error
  */
 static int do_fdtget(struct display_info *disp, const char *filename,
 		     char **arg, int arg_count, int args_per_step)
-- 
2.11.1

Re: [PATCH 4/5] fdtget: Use @return to document the return value

[Simon Glass]

On 4 March 2017 at 06:26, Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> wrote:
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> ---
>  fdtget.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

[PATCH 5/5] fdtput: Remove star from value_len documentation

[Nicolas Iooss]

When clang checks the documentation tags (with -Wdocumentation flag), it
reports the following warning:

    fdtput.c:70:11: error: parameter '*value_len' not found in the
    function declaration [-Werror,-Wdocumentation]
     * @param *value_len    Returns length of value encoded
              ^~~~~~~~~~
    fdtput.c:70:11: note: did you mean 'value_len'?
     * @param *value_len    Returns length of value encoded
              ^~~~~~~~~~
              value_len

As this sounds reasonable, remove the star from the documentation tag.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
---
 fdtput.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fdtput.c b/fdtput.c
index db65e9613f29..971199813962 100644
--- a/fdtput.c
+++ b/fdtput.c
@@ -67,7 +67,7 @@ static void report_error(const char *name, int namelen, int err)
  * @param arg		List of arguments from command line
  * @param arg_count	Number of arguments (may be 0)
  * @param valuep	Returns buffer containing value
- * @param *value_len	Returns length of value encoded
+ * @param value_len	Returns length of value encoded
  */
 static int encode_value(struct display_info *disp, char **arg, int arg_count,
 			char **valuep, int *value_len)
-- 
2.11.1

Re: [PATCH 5/5] fdtput: Remove star from value_len documentation

[Simon Glass]

On 4 March 2017 at 06:26, Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org> wrote:
> When clang checks the documentation tags (with -Wdocumentation flag), it
> reports the following warning:
>
>     fdtput.c:70:11: error: parameter '*value_len' not found in the
>     function declaration [-Werror,-Wdocumentation]
>      * @param *value_len    Returns length of value encoded
>               ^~~~~~~~~~
>     fdtput.c:70:11: note: did you mean 'value_len'?
>      * @param *value_len    Returns length of value encoded
>               ^~~~~~~~~~
>               value_len
>
> As this sounds reasonable, remove the star from the documentation tag.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>
> ---
>  fdtput.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Re: [PATCH 1/5] dtc: Simplify asm_emit_string() implementation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2122 bytes --]

On Sat, Mar 04, 2017 at 02:26:43PM +0100, Nicolas Iooss wrote:
> Using %.*s format helps making asm_emit_string() not modify its "str"
> parameter.
> 
> While at it, constify the "str" parameter of bin_emit_string() and
> asm_emit_string(), as these function no longer modify it.
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss_linux-oWGTIYur0i8@public.gmane.org>

All applued, thanks.

> ---
>  flattree.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/flattree.c b/flattree.c
> index ebac548b3fa8..d8a118258e01 100644
> --- a/flattree.c
> +++ b/flattree.c
> @@ -49,7 +49,7 @@ static struct version_info {
>  
>  struct emitter {
>  	void (*cell)(void *, cell_t);
> -	void (*string)(void *, char *, int);
> +	void (*string)(void *, const char *, int);
>  	void (*align)(void *, int);
>  	void (*data)(void *, struct data);
>  	void (*beginnode)(void *, struct label *labels);
> @@ -64,7 +64,7 @@ static void bin_emit_cell(void *e, cell_t val)
>  	*dtbuf = data_append_cell(*dtbuf, val);
>  }
>  
> -static void bin_emit_string(void *e, char *str, int len)
> +static void bin_emit_string(void *e, const char *str, int len)
>  {
>  	struct data *dtbuf = e;
>  
> @@ -144,22 +144,14 @@ static void asm_emit_cell(void *e, cell_t val)
>  		(val >> 8) & 0xff, val & 0xff);
>  }
>  
> -static void asm_emit_string(void *e, char *str, int len)
> +static void asm_emit_string(void *e, const char *str, int len)
>  {
>  	FILE *f = e;
> -	char c = 0;
>  
> -	if (len != 0) {
> -		/* XXX: ewww */
> -		c = str[len];
> -		str[len] = '\0';
> -	}
> -
> -	fprintf(f, "\t.string\t\"%s\"\n", str);
> -
> -	if (len != 0) {
> -		str[len] = c;
> -	}
> +	if (len != 0)
> +		fprintf(f, "\t.string\t\"%.*s\"\n", len, str);
> +	else
> +		fprintf(f, "\t.string\t\"%s\"\n", str);
>  }
>  
>  static void asm_emit_align(void *e, int a)

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