On Thu, 2016-01-21 at 11:13 +0100, Uwe Kleine-König wrote:
> > And having compared the version tag to 1, couldn't you then cast img
> > into a struct main_hdr_v1 *? That would avoid all the hard coded magic
> > offsets in the rest of the code. Unless img isn't aligned?
>
> ... or the build machine is big endian. (Note in this case kwbimage is
> broken, too. And given that catching such an error independently is IMHO
> a feature.) I'd prefer to keep it as is at least for now for a few
> reasons (endianess, alignment, bigger reshuffling of code because said
> struct is defined in a different .c file)
Endianess shouldn't make a difference when it comes to using the struct.
Seems like it would be nice to have a common definition of the structs
between the code that makes the image and the code the reads it.
> >> +
> >> + image_size = img[0x4] | (img[0x5] << 8) |
> >> + (img[0x6] << 16) | (img[0x7] << 24);
> >
> > struct main_hdr_v1 *hdr = (const struct main_hdr
> > image_size = le32_to_cpu(hdr->blocksize);
> > or if unaligned:
> > image_size = get_unaligned_le32(&img[0x4]);
>
> Of the three offered versions I'd prefer mine ... It's simple and easy
> to verify when comparing to the reference manual.
>
> >> +
> >> + header_size = (img[0x9] << 16) | img[0xa] | (img[0xb] << 8);
> >
> > header_size = hdr->headersz_msb << 16 | le16_to_cpu(hdr->headersz_lsb);
> >
> >> +
> >> + if (header_size + image_size != size) {
> >> + printf("Size mismatch (%zu + %zu != %zu)\n",
> >> + header_size, image_size, size);
> >> + goto err;
> >> + } else {
> >
> > Don't really need the else block here since the failure above exits,
> > just like the two failure checks before this one.
>
> right, this is a relict of v1 where I didn't jump in the first block.
>
> >> + for (i = 0; i < header_size; ++i)
> >> + csum += img[i];
> >
> > csum = image_checksum8(img, header_size)
>
> This is again in kwbimage.c
>
> Best regards
> Uwe
>
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
