Dear Prafulla Wadaskar,
In message <1248804270-13715-4-git-send-email-prafu...@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,
> - ¶ms))) /* old-style image */
> - image_print_contents ((image_header_t *)ptr);
> + ¶ms);
> +
> + 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, ¶ms);
> + tparams->set_header (ptr, &sbuf, ifd, ¶ms);
>
> - 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: w...@denx.de
Technology is dominated by those who manage what they do not under-
stand.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
