Re: AW: contributing a failsafe update meachanism for FIS from within ecos applications

[Andrew Lunn <andrew at lunn dot ch>]

> Andrew: in your patch you removed the magic_name field from the valid_info struct. While this works, it requires quite a lot more typing:
> 
> struct fis_image_desc* img0=(struct fis_image_desc*)buf0;
> struct fis_image_desc* img1=(struct fis_image_desc*)buf1;
> struct fis_valid_info* fvi0
>      =(struct fis_valid_info*)img0->name+sizeof(CYG_REDBOOT_VALID_MAGIC);
> struct fis_valid_info* fvi1
>      =(struct fis_valid_info*)img1->name+sizeof(CYG_REDBOOT_VALID_MAGIC);

This is wrong and dangerous and i do not do that in my code. Becasue
sizeof(CYG_REDBOOT_VALID_MAGIC) == 10, your fvi is not word
aligned. When you then access fvi0->valid_flag you do a none aligned
access which on ARM and probably other targets will throw an BUS
exception. That why my code does a memcpy() from the tail of the name
array into fvi.
 
> instead of:
> 
> struct fis_valid_info* fvi0 =(struct fis_valid_info*)buf0;
> struct fis_valid_info* fvi1 =(struct fis_valid_info*)buf1;

What im trying to do is use fis_image_desc as much as possible becasue
thats what we have to be compatible with. By defining a new structure
there is the danger that fis_image_desc gets changed and
fis_valid_info does not. 

> which IMHO makes it much easier to mix things up and introduce bugs,
> while it doesn't help against the padding/alignment problem (AFAIK).

It should help the padding/allignment problem becasue my structure is
aligned where as yours is not. I also memcpy it back and for which is
always safe to do. And the assert() i added should tell us if its gone
wrong.

> This doesn't only apply to the redboot code, but even more to the
> fisfs implementation, there this has to be done more often.

I took a quick look at your fisfs code. We need to talk about that...

        Andrew