Re: [DTC][RFC] dtc: Allow better error reporting

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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

On Thu, Feb 11, 2021 at 04:57:21PM +0530, Viresh Kumar wrote:
> The dtc tool doesn't do very elaborate error reporting to keep the size
> of the executables small enough for all kind of applications.

dtc itself does plenty of error reporting, it's just libfdt that tries
to keep things minimal.

> This patch tries to provide a way to do better error reporting, without
> increasing the size of the executables for such cases (where we don't
> want elaborate error reporting).
> 
> The error reporting will only be enabled if 'VERBOSE' (as -DVERBOSE)
> flag is passed during compilation of the tools.
> 
> This also updates the fdtoverlay tool to do better error reporting,
> which is required by the Linux Kernel for now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> ---
> Hi David,
> 
> Unfortunately I am not a core DT guy and don't understand most of the
> stuff going on here. I tried to do minimal changes to get more
> information out on errors and it may require someone who understands the
> code well to write better error logs.
> 
> FWIW, I stumbled upon this because of the discussion that happened here:
> 
> https://lore.kernel.org/lkml/74f8aa8f-ffab-3b0f-186f-31fb7395ebbb@gmail.com/
> 
> Thanks.
> 
> ---
>  dtc.h                |   6 ++
>  fdtoverlay.c         |   1 +
>  libfdt/fdt_overlay.c | 156 ++++++++++++++++++++++++++++++++++---------
>  3 files changed, 132 insertions(+), 31 deletions(-)
> 
> diff --git a/dtc.h b/dtc.h
> index d3e82fb8e3db..b8ffec155263 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -29,6 +29,12 @@
>  #define debug(...)
>  #endif
>  
> +#ifdef VERBOSE
> +#define dtc_err(fmt, ...)	fprintf(stderr, "DTC: %s: %d: " fmt, __func__, __LINE__, ##__VA_ARGS__)
> +#else
> +#define dtc_err(fmt, ...)
> +#endif

Actually, the natural way to handle this is to make dtc_err() -
hrm... bad name, since this is libfdt - be something that must be
provided by libfdt_env.h.  The default version would have it be a
no-op, with a define that makes it use stdio.

This has the additional advantage that it would be relatively
straightfoward to enable the rich reporting in a non-POSIXish
environment (these should provide their own libfdt_env.h and it can
implement the error reporting callback in a way that makes sense in
that environment.

You will also definitely need Makefile changes, because you'll need to
make the fdtoverlay binary link to the verbose-compiled version of
libfdt not the normal one.

Except.... it might make more sense to do this in dtc rather than
libfdt, more on that in different mails.

> +
>  #define DEFAULT_FDT_VERSION	17
>  
>  /*
> diff --git a/fdtoverlay.c b/fdtoverlay.c
> index 5350af65679f..5f60ce4e4cea 100644
> --- a/fdtoverlay.c
> +++ b/fdtoverlay.c
> @@ -16,6 +16,7 @@
>  
>  #include <libfdt.h>
>  
> +#include "dtc.h"
>  #include "util.h"
>  
>  #define BUF_INCREMENT	65536
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index d217e79b6722..b24286ac8c6c 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -10,6 +10,7 @@
>  #include <libfdt.h>
>  
>  #include "libfdt_internal.h"
> +#include "../dtc.h"

Yeah, the libfdt code can't include dtc.h.

>  
>  /**
>   * overlay_get_target_phandle - retrieves the target phandle of a fragment
> @@ -160,12 +161,16 @@ static int overlay_adjust_node_phandles(void *fdto, int node,
>  	int ret;
>  
>  	ret = overlay_phandle_add_offset(fdto, node, "phandle", delta);
> -	if (ret && ret != -FDT_ERR_NOTFOUND)
> +	if (ret && ret != -FDT_ERR_NOTFOUND) {
> +		dtc_err("Failed to add phandle offset (%d: %s)\n", node, fdt_strerror(ret));
>  		return ret;
> +	}
>  
>  	ret = overlay_phandle_add_offset(fdto, node, "linux,phandle", delta);
> -	if (ret && ret != -FDT_ERR_NOTFOUND)
> +	if (ret && ret != -FDT_ERR_NOTFOUND) {
> +		dtc_err("Failed to add linux,phandle offset (%d: %s)\n", node, fdt_strerror(ret));
>  		return ret;
> +	}
>  
>  	fdt_for_each_subnode(child, fdto, node) {
>  		ret = overlay_adjust_node_phandles(fdto, child, delta);
> @@ -239,12 +244,17 @@ static int overlay_update_local_node_references(void *fdto,
>  		if (!fixup_val)
>  			return fixup_len;
>  
> -		if (fixup_len % sizeof(uint32_t))
> +		if (fixup_len % sizeof(uint32_t)) {
> +			dtc_err("Invalid fixup length\n");
>  			return -FDT_ERR_BADOVERLAY;
> +		}
>  		fixup_len /= sizeof(uint32_t);
>  
>  		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
>  		if (!tree_val) {
> +			dtc_err("Failed to get property: %s: %d\n", name,
> +				tree_len);
> +
>  			if (tree_len == -FDT_ERR_NOTFOUND)
>  				return -FDT_ERR_BADOVERLAY;
>  
> @@ -274,11 +284,17 @@ static int overlay_update_local_node_references(void *fdto,
>  								  poffset,
>  								  &adj_val,
>  								  sizeof(adj_val));
> -			if (ret == -FDT_ERR_NOSPACE)
> +			if (ret == -FDT_ERR_NOSPACE) {
> +				dtc_err("Failed to update property's name: %s\n",
> +					name);
>  				return -FDT_ERR_BADOVERLAY;
> +			}
>  
> -			if (ret)
> +			if (ret) {
> +				dtc_err("Failed to update property's name: %s: %s\n",
> +					name, fdt_strerror(ret));
>  				return ret;
> +			}
>  		}
>  	}
>  
> @@ -289,10 +305,16 @@ static int overlay_update_local_node_references(void *fdto,
>  
>  		tree_child = fdt_subnode_offset(fdto, tree_node,
>  						fixup_child_name);
> -		if (tree_child == -FDT_ERR_NOTFOUND)
> +		if (tree_child == -FDT_ERR_NOTFOUND) {
> +			dtc_err("Failed to find subnode: %s\n",
> +				fixup_child_name);
>  			return -FDT_ERR_BADOVERLAY;
> -		if (tree_child < 0)
> +		}
> +		if (tree_child < 0) {
> +			dtc_err("Failed to find subnode: %s: %s\n",
> +				fixup_child_name, fdt_strerror(tree_child));
>  			return tree_child;
> +		}
>  
>  		ret = overlay_update_local_node_references(fdto,
>  							   tree_child,
> @@ -332,6 +354,8 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>  		if (fixups == -FDT_ERR_NOTFOUND)
>  			return 0;
>  
> +		dtc_err("Failed to find local_fixups (%s)\n",
> +			fdt_strerror(fixups));
>  		return fixups;
>  	}
>  
> @@ -435,6 +459,8 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>  	value = fdt_getprop_by_offset(fdto, property,
>  				      &label, &len);
>  	if (!value) {
> +		dtc_err("Failed to get property by offset (%s)\n",
> +			fdt_strerror(len));
>  		if (len == -FDT_ERR_NOTFOUND)
>  			return -FDT_ERR_INTERNAL;
>  
> @@ -450,8 +476,10 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>  		int poffset, ret;
>  
>  		fixup_end = memchr(value, '\0', len);
> -		if (!fixup_end)
> +		if (!fixup_end) {
> +			dtc_err("fixup_end can't be 0: %s: %s\n", value, label);
>  			return -FDT_ERR_BADOVERLAY;
> +		}
>  		fixup_len = fixup_end - fixup_str;
>  
>  		len -= fixup_len + 1;
> @@ -459,32 +487,47 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>  
>  		path = fixup_str;
>  		sep = memchr(fixup_str, ':', fixup_len);
> -		if (!sep || *sep != ':')
> +		if (!sep || *sep != ':') {
> +			dtc_err("Missing ':' separator: %s: %s\n", value,
> +				label);
>  			return -FDT_ERR_BADOVERLAY;
> +		}
>  
>  		path_len = sep - path;
> -		if (path_len == (fixup_len - 1))
> +		if (path_len == (fixup_len - 1)) {
> +			dtc_err("Invalid path_len: %s: %s\n", value, label);
>  			return -FDT_ERR_BADOVERLAY;
> +		}
>  
>  		fixup_len -= path_len + 1;
>  		name = sep + 1;
>  		sep = memchr(name, ':', fixup_len);
> -		if (!sep || *sep != ':')
> +		if (!sep || *sep != ':') {
> +			dtc_err("Missing ':' separator: %s: %s\n", value,
> +				label);
>  			return -FDT_ERR_BADOVERLAY;
> +		}
>  
>  		name_len = sep - name;
> -		if (!name_len)
> +		if (!name_len) {
> +			dtc_err("name_len can't be 0: %s: %s\n", value, label);
>  			return -FDT_ERR_BADOVERLAY;
> +		}
>  
>  		poffset = strtoul(sep + 1, &endptr, 10);
> -		if ((*endptr != '\0') || (endptr <= (sep + 1)))
> +		if ((*endptr != '\0') || (endptr <= (sep + 1))) {
> +			dtc_err("Invalid name string: %s: %s\n", value, label);
>  			return -FDT_ERR_BADOVERLAY;
> +		}
>  
>  		ret = overlay_fixup_one_phandle(fdt, fdto, symbols_off,
>  						path, path_len, name, name_len,
>  						poffset, label);
> -		if (ret)
> +		if (ret) {
> +			dtc_err("failed to fixup one phandle: %s: %s: %s\n",
> +				value, label, fdt_strerror(ret));
>  			return ret;
> +		}
>  	} while (len > 0);
>  
>  	return 0;
> @@ -516,13 +559,19 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
>  	fixups_off = fdt_path_offset(fdto, "/__fixups__");
>  	if (fixups_off == -FDT_ERR_NOTFOUND)
>  		return 0; /* nothing to do */
> -	if (fixups_off < 0)
> +	if (fixups_off < 0) {
> +		dtc_err("Failed to get fixups offset (%s)\n",
> +			fdt_strerror(fixups_off));
>  		return fixups_off;
> +	}
>  
>  	/* And base DTs without symbols */
>  	symbols_off = fdt_path_offset(fdt, "/__symbols__");
> -	if ((symbols_off < 0 && (symbols_off != -FDT_ERR_NOTFOUND)))
> +	if ((symbols_off < 0 && (symbols_off != -FDT_ERR_NOTFOUND))) {
> +		dtc_err("Failed to get symbols offset (%s)\n",
> +			fdt_strerror(symbols_off));
>  		return symbols_off;
> +	}
>  
>  	fdt_for_each_property_offset(property, fdto, fixups_off) {
>  		int ret;
> @@ -633,16 +682,27 @@ static int overlay_merge(void *fdt, void *fdto)
>  		if (overlay == -FDT_ERR_NOTFOUND)
>  			continue;
>  
> -		if (overlay < 0)
> +		if (overlay < 0) {
> +			dtc_err("Failed to find __overlay__ tag (%d: %s)\n",
> +				fragment, fdt_strerror(overlay));
>  			return overlay;
> +		}
>  
>  		target = overlay_get_target(fdt, fdto, fragment, NULL);
> -		if (target < 0)
> +		if (target < 0) {
> +			dtc_err("Failed to retrieve fragment's target (%d: %s)\n",
> +				fragment, fdt_strerror(target));
>  			return target;
> +		}
>  
>  		ret = overlay_apply_node(fdt, target, fdto, overlay);
> -		if (ret)
> +		if (ret) {
> +			if (ret != -FDT_ERR_NOSPACE) {
> +				dtc_err("Failed to apply overlay node (%d: %d: %s)\n",
> +					fragment, target, fdt_strerror(ret));
> +			}
>  			return ret;
> +		}
>  	}
>  
>  	return 0;
> @@ -718,24 +778,35 @@ static int overlay_symbol_update(void *fdt, void *fdto)
>  		root_sym = fdt_add_subnode(fdt, 0, "__symbols__");
>  
>  	/* any error is fatal now */
> -	if (root_sym < 0)
> +	if (root_sym < 0) {
> +		dtc_err("Failed to get root __symbols__ (%s)\n",
> +			fdt_strerror(root_sym));
>  		return root_sym;
> +	}
>  
>  	/* iterate over each overlay symbol */
>  	fdt_for_each_property_offset(prop, fdto, ov_sym) {
>  		path = fdt_getprop_by_offset(fdto, prop, &name, &path_len);
> -		if (!path)
> +		if (!path) {
> +			dtc_err("Failed to get prop by offset (%s)\n",
> +				fdt_strerror(path_len));
>  			return path_len;
> +		}
>  
>  		/* verify it's a string property (terminated by a single \0) */
> -		if (path_len < 1 || memchr(path, '\0', path_len) != &path[path_len - 1])
> +		if (path_len < 1 || memchr(path, '\0', path_len) != &path[path_len - 1]) {
> +			dtc_err("Invalid property (%d)\n", path_len);
>  			return -FDT_ERR_BADVALUE;
> +		}
>  
>  		/* keep end marker to avoid strlen() */
>  		e = path + path_len;
>  
> -		if (*path != '/')
> +		if (*path != '/') {
> +			dtc_err("Path should start with '/' (%s : %s)\n", path,
> +				name);
>  			return -FDT_ERR_BADVALUE;
> +		}
>  
>  		/* get fragment name first */
>  		s = strchr(path + 1, '/');
> @@ -769,26 +840,38 @@ static int overlay_symbol_update(void *fdt, void *fdto)
>  		ret = fdt_subnode_offset_namelen(fdto, 0, frag_name,
>  					       frag_name_len);
>  		/* not found? */
> -		if (ret < 0)
> +		if (ret < 0) {
> +			dtc_err("Failed to find fragment index (%s: %s: %d)\n",
> +				path, frag_name, ret);
>  			return -FDT_ERR_BADOVERLAY;
> +		}
>  		fragment = ret;
>  
>  		/* an __overlay__ subnode must exist */
>  		ret = fdt_subnode_offset(fdto, fragment, "__overlay__");
> -		if (ret < 0)
> +		if (ret < 0) {
> +			dtc_err("Failed to find __overlay__ subnode (%s: %s: %d)\n",
> +				path, frag_name, ret);
>  			return -FDT_ERR_BADOVERLAY;
> +		}
>  
>  		/* get the target of the fragment */
>  		ret = overlay_get_target(fdt, fdto, fragment, &target_path);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			dtc_err("Failed to get target for the fragment (%s: %s: %d)\n",
> +				path, frag_name, ret);
>  			return ret;
> +		}
>  		target = ret;
>  
>  		/* if we have a target path use */
>  		if (!target_path) {
>  			ret = get_path_len(fdt, target);
> -			if (ret < 0)
> +			if (ret < 0) {
> +				dtc_err("Failed to get path length (%s: %s: %d)\n",
> +					path, name, ret);
>  				return ret;
> +			}
>  			len = ret;
>  		} else {
>  			len = strlen(target_path);
> @@ -796,14 +879,20 @@ static int overlay_symbol_update(void *fdt, void *fdto)
>  
>  		ret = fdt_setprop_placeholder(fdt, root_sym, name,
>  				len + (len > 1) + rel_path_len + 1, &p);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			dtc_err("Failed to set prop placeholder (%s: %s: %d)\n",
> +				path, name, ret);
>  			return ret;
> +		}
>  
>  		if (!target_path) {
>  			/* again in case setprop_placeholder changed it */
>  			ret = overlay_get_target(fdt, fdto, fragment, &target_path);
> -			if (ret < 0)
> +			if (ret < 0) {
> +				dtc_err("Failed to get target (%s: %s: %d)\n",
> +					path, name, ret);
>  				return ret;
> +			}
>  			target = ret;
>  		}
>  
> @@ -811,8 +900,11 @@ static int overlay_symbol_update(void *fdt, void *fdto)
>  		if (len > 1) { /* target is not root */
>  			if (!target_path) {
>  				ret = fdt_get_path(fdt, target, buf, len + 1);
> -				if (ret < 0)
> +				if (ret < 0) {
> +					dtc_err("Failed to get target path (%s: %s: %d)\n",
> +						path, name, ret);
>  					return ret;
> +				}
>  			} else
>  				memcpy(buf, target_path, len + 1);
>  
> @@ -836,8 +928,10 @@ int fdt_overlay_apply(void *fdt, void *fdto)
>  	FDT_RO_PROBE(fdto);
>  
>  	ret = fdt_find_max_phandle(fdt, &delta);
> -	if (ret)
> +	if (ret) {
> +		dtc_err("Failed to find max phandle (%s)\n", fdt_strerror(ret));
>  		goto err;
> +	}
>  
>  	ret = overlay_adjust_local_phandles(fdto, delta);
>  	if (ret)

-- 
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: 833 bytes --]