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

[Andrew Lunn <andrew at lunn dot ch>]

> No two bits for "valid or in_progress or empty" is not enough. In
> the case that power is lost while writing exactly these two bits the
> state of these two bits is undefined. So the probability that they
> end up as valid when I actually wanted to write in_progress is 25%,
> at least >>0. If the valid_flag is 32 bits and only one combination
> is considered valid, then the probability is 1/(2**32).

Maybe im missing something here....

You have three states:

Empty        11b
in_progress  01b
valid        00b

Since only one bit is ever changed you should always have a defined
state. The write either happened, or it did not. 

> Two bits for the version are with my proposed scheme also not
> enough. The old table will never be touched, the new one will
> increase the version of the old table by one. So no wrap-around may
> happen. This won't happen with 32 bits.

640K is enought memory for any system.
4294967295 is enought IP addresses.
512 files in the root directory should be enough.

Why not just do it right and use modular arithmatic to handle wrap
around?

diff -rbup current.orig/cdl/redboot.cdl current/cdl/redboot.cdl
--- current.orig/cdl/redboot.cdl	Sun Sep 19 19:49:59 2004
+++ current/cdl/redboot.cdl	Tue Oct 19 17:16:18 2004
@@ -638,6 +638,19 @@ cdl_package CYGPKG_REDBOOT {
                       Negative block numbers count backwards from the last block.
                       eg 2 means block 2, -2 means the last but one block."
                 }
+
+					  cdl_option CYGNUM_REDBOOT_FIS_ADDITIONAL_DIRECTORY_BLOCK {
+                    display         "Flash block containing the backup Directory"
+                    flavor          data
+                    default_value   (-1)
+                    description "

Setting the default as -1 is not a good idea. Thats the same as the
primary block. You probably want a requires statement that the
additional block is different from the primary block just to stop
people doing this....

+#define EFIS_MAGIC "$_FisValid_

Any particular reason for the $_ and _?

+struct fis_valid_info
+{
+   unsigned char magic_name[12];
+   unsigned long valid_flag;
+   CYG_ADDRESS unused_flash_base;
+   unsigned long version_count;
+};

This does not look good. The version_count is in the same place as the
size. I expect uses are going to wonder why the size keeps
increasing. I would try to hide this inside the name.

Its also probably safer to use the existing struct fis_image_desc if
you can get it all inside the name. It means less trouble if somebody
comes along and adds a new field at the beginning etc.

+    memcpy(fvi->magic_name, EFIS_MAGIC, 12);

Using a memcpy on a string is not a good idea. Its much safer to do a
strcpy or strncpy.

+        if ((memcmp(fvi0.magic_name, EFIS_MAGIC, 12)==0) || (memcmp(fvi1.magic_name, EFIS_MAGIC, 12)==0)) {
+           if (get_valid_buf(&fvi0, &fvi1)==0)
+              fis_addr=fis_addr0;

This looks ugly. I would put the memcmp (which should be strncpy())
inside get_valid_buf().

fis init should also probably erase the secondary fis directory just
to be safe.

        Andrew