[U-Boot] [PATCH 4/8] tools: mkimage: Implemented callback function for default image support

[Wolfgang Denk]

Dear Prafulla Wadaskar,

In message <1248804270-13715-4-git-send-email-prafulla at marvell.com> you wrote:
> This is Third step towards cleaning mkimage code for kwbimage

BTW - I think "cleaning mkimage code" is not correct. This is not a
clean up, but a major rework.

> 1. callback functions are used in mkimage.c those are defined
> 	in defaut_image.c for currently supported images.

Typo...

> 2. scan loop implemented for image header verify and print

What does that mean? Please provide better documentation / comments so
we understand what you are doing.


>  void image_print_header (char *ptr)
>  {
> -	image_header_t * hdr = (image_header_t *)ptr;
> -
> -	image_print_contents (hdr);
> +	image_print_contents ((image_header_t *)ptr);
>  }

The new code looks uglier than the old one. Please don;t change.

> +/*
> + * FIT image support
> + */
> +int
> +fitimage_verify_header (char *ptr, int image_size,
> +			struct mkimage_params *params)
> +{
> +	fdt_check_header ((void *)ptr);
> +}

Do other image types need the provided parameters?

> +void fitimage_print_header (char *ptr)
> +{
> +	fit_print_contents ((void *)ptr);
> +}

Maybe we can omit this function, then?

> +struct image_type_params fitimage_params = {
> +	.header_size = sizeof(image_header_t),
> +	.hdr = (void*)&header,
> +	.check_params = image_check_params,
> +	.verify_header = fitimage_verify_header,
> +	.print_header = fitimage_print_header,

... and just use ".print_header = fit_print_contents," here?

...
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index d1636d8..3a3cb19 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -49,14 +49,28 @@ struct mkimage_params params = {
>  	.cmdname = "",
>  };
>  
> +static struct image_functions image_ftable[] = {
> +	{image_get_tparams,},	/* for IH_TYPE_INVALID */
> +	{image_get_tparams,},	/* for IH_TYPE_FILESYSTEM */
> +	{image_get_tparams,},	/* for IH_TYPE_FIRMWARE */
> +	{image_get_tparams,},	/* for IH_TYPE_KERNEL */
> +	{image_get_tparams,},	/* for IH_TYPE_MULTI */
> +	{image_get_tparams,},	/* for IH_TYPE_RAMDISK */
> +	{image_get_tparams,},	/* for IH_TYPE_SCRIPT */
> +	{image_get_tparams,},	/* for IH_TYPE_STANDALONE */
> +	{fitimage_get_tparams,},	/* for IH_TYPE_FLATDT */
> +};

What is this? Please explain. The table looks pretty bogus to me. If
all but one use the same function, why do we need such a table then?

>  int
>  main (int argc, char **argv)
>  {
>  	int ifd = -1;
>  	struct stat sbuf;
>  	unsigned char *ptr;
> -	int retval;
> +	int i, retval;
>  	struct image_type_params *tparams = image_get_tparams ();

Such initialization here looks bad to me, as it is in no way
correlated to the image_ftable[] settinga above.

> +	struct image_type_params *scantparams;
> +	int image_ftable_size = ARRAY_SIZE(image_ftable);
>  
>  	params.cmdname = *argv;
>  
> @@ -97,6 +111,15 @@ main (int argc, char **argv)
>  					(params.opt_type =
>  					genimg_get_type_id (*++argv)) < 0)
>  					usage ();
> +				if (image_ftable_size <= params.opt_type) {

Logic here looks inverted to me. And is the <= correct?

> +		/*
> +		 * Scan image headers for all supported types
> +		 * and print if veryfy_header sucessful

Typo.

I have to admit that I do not understand what you are actually doing
here.

> +		 */
> +		for (i = image_ftable_size - 1; i != IH_TYPE_INVALID; i--) {
> +			scantparams = image_ftable[i].get_tparams ();
> +			fprintf (stderr, "%s: Verify header for type=%s\n",
> +				params.cmdname, genimg_get_type_name (i));
> +
> +			retval = scantparams->verify_header (
>  				(char *)ptr, sbuf.st_size,
> -				&params))) /* old-style image */
> -			image_print_contents ((image_header_t *)ptr);
> +				&params);
> +
> +			if (retval == 0) {
> +				scantparams->print_header ((char *)ptr);
> +				break;
> +			}
> +		}

Maybe you can explain what you're doing?

>  		(void) munmap((void *)ptr, sbuf.st_size);
>  		(void) close (ifd);
> @@ -333,9 +368,9 @@ NXTARG:		;
>  		exit (EXIT_FAILURE);
>  	}
>  
> -	image_set_header (ptr, &sbuf, &params);
> +	tparams->set_header (ptr, &sbuf, ifd, &params);
>  
> -	image_print_header (ptr);
> +	tparams->print_header (ptr);

The longer I read this, the more I dislike the tparams-> code. I tend
to think the type switching should be transparent, and not visible in
the code.


> +/*
> + * Image type specific function pointers list
> + */
> +struct image_functions {
> +	void *	(*get_tparams) (void);
> +};

What's that?

> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))

Why do you need to declare this here? Please use standard features.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Technology is dominated by those who manage what they do  not  under-
stand.