[PATCH v2 2/3] Add fdtgrep to grep and subset FDTs

[Mike Frysinger]

On Friday 15 February 2013 17:49:37 Simon Glass wrote:
> --- /dev/null
> +++ b/fdtgrep.c

hopefully my patchset gets merged soon in which case you should rebase on top 
of it to use the new common getopts code

> +/* Define DEBUG to get some debugging output on stderr */
> +#ifdef DEBUG
> +#define debug(a, b...) fprintf(stderr, a, ## b)
> +#else
> +#define debug(a, b...)
> +#endif

shouldn't this be in util.h ?  seems to be copied from dtc.h ...

> +static void report_error(const char *where, int err)
> +{
> +	fprintf(stderr, "Error at '%s': %s\n", where, fdt_strerror(err));
> +}

report_error should probably get pulled out of fdt{get,put}.c and this file and 
put into util.h.  it already has a bunch of helpers like die().

> +	str = strdup(str);

xstrdup()

> +	node = malloc(sizeof(*node));

xmalloc()

> +	if (!str || !node) {
> +		fprintf(stderr, "Out of memory\n");
> +		return -1;
> +	}

then you don't need this

> +static int do_fdtgrep(struct display_info *disp, const char *filename)
> +{
> +	struct fdt_region *region;
> +	int max_regions;
> +	int count = 100;
> +	char path[1024];

isn't there a constant somewhere to use rather than 1024 ?

> +		region = malloc(count * sizeof(struct fdt_region));
> +		if (!region) {
> +			fprintf(stderr, "Out of memory for %d regions\n",
> +				count);
> +			return -1;
> +		}

xmalloc(), then you can drop the error checking

> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
>
> +    if [ -z "$nodename" ]; then
> +	nodename=testing
> +    fi
> +    if [ -z "$base" ]; then
> +	base=$($DTGREP ${args} -O bin $tree | sha1sum)
> +    fi

you could one line these sort of defaults:
	: ${nodename:=testing}
	: ${base:=$($DTGREP ${args} -O bin $tree | sha1sum)}

> +    if [ "$base" == "$hash" ]; then
> +	if [ "$changed" == 1 ]; then

this is not valid POSIX shell.  you have to use "=".  you should update the 
other parts of your patch too

> +    lines=$($@ | wc -l)

you should quote that:
	lines=$("$@" | wc -l)

> +    bytes=$($@ | wc -c)

same here

> +    if $@ | grep -q $text; then

same here.  and you should probably quote that $text:
	if "$@" | grep -q "$text"; then

> +    tmp=/tmp/tests.$$

probably better to do:
	tmp=$(mktemp)

> +    addr=$($DTGREP -a $dtb | head -1 | tr -d : | awk '{print $1}')

you could do:
	addr=$($DTGREP -a $dtb | tr -d : | awk '{print $1; exit}')

> +    addr=$($DTGREP -a $dtb | tail -1 | tr -d : | awk '{print $1}')

same here

> +    last=$(printf "%#x" $(($dt_start + $dt_size - 8)))

for $((...)) operations, you don't need to expand the var yourself:
	last=$(printf "%#x" $((dt_start + dt_size - 8)))
seems to show up a few times in this patch

> +    cat >$tmp <<END
> +	#address-cells
> +	airline
> +	bootargs
> +	compatible
> +	linux,platform
> +	model
> +	#size-cells
> +	status
> +	weather
> +END

if you use - with the heredoc marker, you can indent the contents with tabs:
	cat >$tmp <<-END
	foo
	bar
	mew
	END

> +    lines=$(cat $tmp | wc -l)

avoid the cat/pipe:
	lines=$(wc -l < $tmp)

> +    lines=$(grep "=" $dts |wc -l)

needs a space after the pipe:
	lines=$(grep "=" $dts | wc -l)

> +    base=$(stat -c %s $dtb)

this isn't portable.  do this instead:
	base=$(wc -c < $dtb)

> +    rm -f $tmp

so failures in this test leaks the tmp file ?  should add a `trap` to rm it 
upon exit.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.ozlabs.org/pipermail/devicetree-discuss/attachments/20130415/605ed378/attachment.sig>