[U-Boot] [PATCH v2 3/4] tools: mkimage: kwbimage list command support

[Prafulla Wadaskar]

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de] 
> Sent: Wednesday, July 22, 2009 2:55 PM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Ashish Karkare; Prabhanjan Sarnaik
> Subject: Re: [U-Boot] [PATCH v2 3/4] tools: mkimage: kwbimage 
> list command support
> 
> Dear Prafulla Wadaskar,
> 
> In message 
> <1248220911-11378-1-git-send-email-prafulla at marvell.com> you wrote:
> > Signed-off-by: Prafulla Wadaskar <prafulla at marvell.com>
> ...
> > --- a/common/image.c
> > +++ b/common/image.c
> > @@ -159,7 +159,7 @@ static table_entry_t uimage_comp[] = {
> >  
> >  uint32_t crc32 (uint32_t, const unsigned char *, uint);  uint32_t 
> > crc32_wd (uint32_t, const unsigned char *, uint, uint); 
> -static void 
> > genimg_print_size (uint32_t size);
> > +void genimg_print_size (uint32_t size);
> 
> When exporting this function, then please add prototype to 
> header file.
I have followed function crc32, I will do it.
Shall I do it for crc32 and other functions too? I should not disturb them?

> 
> >  #if defined(CONFIG_TIMESTAMP) || defined(CONFIG_CMD_DATE) || 
> > defined(USE_HOSTCC)  static void genimg_print_time (time_t 
> timestamp);  
> > #endif @@ -473,7 +473,7 @@ void memmove_wd (void *to, void *from, 
> > size_t len, ulong chunksz)  }  #endif /* !USE_HOSTCC */
> >  
> > -static void genimg_print_size (uint32_t size)
> > +void genimg_print_size (uint32_t size)
> >  {
> >  #ifndef USE_HOSTCC
> >  	printf ("%d Bytes = ", size);
> > diff --git a/tools/kwbimage.c b/tools/kwbimage.c index 
> > f78d97a..9441a19 100644
> > --- a/tools/kwbimage.c
> > +++ b/tools/kwbimage.c
> > @@ -33,6 +33,8 @@
> >  #include <unistd.h>
> >  #include "kwbimage.h"
> >  
> > +extern void genimg_print_size (uint32_t size);
> 
> Please delete and use prototype from header file.
Same comments above

> 
> >  static struct kwb_header kwbimage_header;  static int 
> datacmd_cnt = 
> > 0;
> >  
> > @@ -262,3 +264,42 @@ void kwbimage_set_header (struct 
> kwb_header *hdr, struct stat *sbuf,
> >  	exthdr->checkSum = kwbimage_checksum8 ((void *)exthdr,
> >  						sizeof(extbhr_t), 0);
> >  }
> > +
> > +/* -l support functions */
> > +int kwbimage_check_header (struct kwb_header *hdr) {
> > +	bhr_t *mhdr = &hdr->kwb_hdr;
> > +	extbhr_t *exthdr = &hdr->kwb_exthdr;
> > +	uint8_t calc_hdrcsum;
> > +	uint8_t calc_exthdrcsum;
> > +
> > +	calc_hdrcsum = kwbimage_checksum8 ((void *)mhdr,
> > +			sizeof(bhr_t) - sizeof(uint8_t), 0);
> > +	if (calc_hdrcsum != mhdr->checkSum)
> > +		return MHDR_CSUM_NOT_MATCHED;
> > +
> > +	calc_exthdrcsum = kwbimage_checksum8 ((void *)exthdr,
> > +			sizeof(extbhr_t) - sizeof(uint8_t), 0);
> > +	if (calc_hdrcsum != mhdr->checkSum)
> > +		return MHDR_CSUM_NOT_MATCHED;
> > +
> > +	return HDR_CSUM_MATCHED;
> > +}
> > +
> > +void kwbimage_print_contents (struct kwb_header *hdr) {
> > +	bhr_t *mhdr = &hdr->kwb_hdr;
> > +	char * bootdev = "Unknown";
> > +
> > +	if (mhdr->blockid == IBR_HDR_SPI_ID)
> > +		bootdev = "spi";
> > +	if (mhdr->blockid == IBR_HDR_NAND_ID)
> > +		bootdev = "nand";
> > +	if (mhdr->blockid == IBR_HDR_SATA_ID)
> > +		bootdev = "sata";
> > +
> > +	printf ("Image Type:   Kirkwood Boot from %s Image\n", bootdev);
> > +	printf ("Data Size:    ");
> > +	genimg_print_size (mhdr->blocksize - sizeof(uint32_t));
> > +	printf ("Load Address: %08x\n", mhdr->destaddr);
> > +	printf ("Entry Point:  %08x\n", mhdr->execaddr); }
> > diff --git a/tools/kwbimage.h b/tools/kwbimage.h index 
> > 57db29f..d32f293 100644
> > --- a/tools/kwbimage.h
> > +++ b/tools/kwbimage.h
> > @@ -55,6 +55,12 @@ enum kwbimage_cmd {
> >  	CMD_DDR_DATA
> >  };
> >  
> > +enum kwbimage_err {
> > +	EHDR_CSUM_NOT_MATCHED = -2,
> > +	MHDR_CSUM_NOT_MATCHED,
> > +	HDR_CSUM_MATCHED
> > +};
> > +
> >  /* typedefs */
> >  typedef struct bhr_t {
> >  	uint8_t blockid;		/*0     */
> > diff --git a/tools/mkimage.c b/tools/mkimage.c index 
> d77d1d6..71076fa 
> > 100644
> > --- a/tools/mkimage.c
> > +++ b/tools/mkimage.c
> > @@ -166,7 +166,7 @@ NXTARG:		;
> >  		usage();
> >  
> >  	if (((argc != 1) && (opt_type == IH_TYPE_KWBIMAGE)) &&
> > -		(xflag || lflag))
> > +		(xflag))
> >  		usage();
> 
> Thanks. Please ignore my question in patch 2/4 :-)
That's pleasure, sometime main objective restricts us to do lot more things.
 
> 
> >  	if (!eflag) {
> > @@ -231,6 +231,11 @@ NXTARG:		;
> >  			exit (EXIT_FAILURE);
> >  		}
> >  
> > +		if (!(kwbimage_check_header ((struct kwb_header 
> *)ptr))) {
> > +			kwbimage_print_contents ((struct 
> kwb_header *)ptr);
> > +			exit (EXIT_SUCCESS);
> 
> I think you should add error checking here. For example, when 
> the code detects a corruption of the image (checksum error or 
> the like), it should return an error code here, so you can 
> use "mkimage -l" to actually verify the consistency of an 
> image (in automated scripts).
Currently mkimage checks first kwbimage header followed by ftd/fit header checks
In that case returning an error code will not be useful
On the other hand,
we should detect filename extension (img/kwb) from input file and then only do relevant image checks.

Filenames can be altered so I think current implementation is okay.
What do you think?

Regards..
Prafulla . .

> 
> 
> 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 Unix is like a toll road on which you have to stop 
> every 50  feet  to pay another nickel. But hey! You only feel 
> 5 cents poorer each time.
>                  - Larry Wall in <1992Aug13.192357.15731 at netlabs.com>
>