This is the mail archive of the ecos-devel@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi,

> Von: Andrew Lunn [mailto:andrew@lunn.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:
> > 
...
> 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.

And that's why I changed from 
unsigned short valid_flag;
to 
unsigned char valid_flag1;
unsigned char valid_flag2;

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 fvi1;
memcpy(&fvi0, img0->name+sizeof(CYG_REDBOOT_VALID_MAGIC), sizeof(CYG_REDBOOT_VALID_MAGIC));
memcpy(&fvi1, img1->name+sizeof(CYG_REDBOOT_VALID_MAGIC), sizeof(CYG_REDBOOT_VALID_MAGIC));

vs. 

struct fis_valid_info* fvi0 =(struct fis_valid_info*)buf0;
struct fis_valid_info* fvi1 =(struct fis_valid_info*)buf1;

which is IMO easier readable and has less potential for bugs, while also having no alignment problems (with my change to two characters).
 
> 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. 

This has to be kept in mind anyway when changing the binary format of fis_image_desc. Changing this format means caring about compatibility with previous redboot versions, so it doesn't happen easily (as we are doing right now ;-)
Maybe something with a union would make it more explicit ?

union
{
   struct fis_image_desc img;
   struct fis_valid_info valid;
} give_me_a_name;
 
> > 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...

Yes, sure :-)

(I may have mentioned it already: the current implementation is not intended to go straight into ecos cvs, the conversion to an ecos fs will happen around end of november)
Issues I know of: eocs package and cdl-optins, namespace to ecos fs conversion incl. data structures, adding special function for update.

Bye
Alex


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]