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

Dear Prafulla Wadaskar,

In message 1248804270-13715-4-git-send-email-prafulla@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