Cleaned up a few additional commit corner cases

- General cleanup from integration, including cleaning up some older
  commit code
- Partial-prog tests do not make sense when prog_size == block_size
  (there can't be partial-progs!)
- Fixed signed-comparison issue in modified filebd

bd/lfs_filebd.c

@@ -97,7 +97,7 @@ int lfs_filebd_read(const struct lfs_config *cfg, lfs_block_t block,
     }
 
     // file truncated? zero for reproducability
-    if (res2 < size) {
+    if ((lfs_size_t)res2 < size) {
         memset((uint8_t*)buffer + res2, 0, size-res2);
     }
 

lfs.c

@@ -1005,12 +1005,13 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
                 // reset crc
                 crc = 0xffffffff;
 
+                if (lfs_tag_chunk(tag) == 3) {
                     // check if the next page is erased
                     //
-                // this may look inefficient, but it's surprisingly efficient
+                    // this may look inefficient, but since cache_size is
-                // since cache_size is probably > prog_size, so the data will
+                    // probably > prog_size, the data will always remain in
-                // always remain in cache for the next iteration
+                    // cache for the next iteration
-                if (lfs_tag_chunk(tag) == 3) {
+
                     // first we get a tag-worth of bits, this is so we can
                     // tweak our current tag to force future writes to be
                     // different than the erased state
@@ -1018,12 +1019,7 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
                     err = lfs_bd_read(lfs,
                             NULL, &lfs->rcache, lfs->cfg->block_size,
                             dir->pair[0], dir->off, &etag, sizeof(etag));
-                    if (err) {
+                    if (err && err != LFS_ERR_CORRUPT) {
-                        // TODO can we stop duplicating this error condition?
-                        if (err == LFS_ERR_CORRUPT) {
-                            dir->erased = false;
-                            break;
-                        }
                         return err;
                     }
 
@@ -1037,11 +1033,7 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
                     err = lfs_bd_crc(lfs,
                             NULL, &lfs->rcache, lfs->cfg->block_size,
                             dir->pair[0], dir->off, estate.size, &tcrc);
-                    if (err) {
+                    if (err && err != LFS_ERR_CORRUPT) {
-                        if (err == LFS_ERR_CORRUPT) {
-                            dir->erased = false;
-                            break;
-                        }
                         return err;
                     }
 
@@ -1049,16 +1041,16 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
                         dir->erased = true;
                         break;
                     }
-                } else {
+                } else if (lfs_tag_chunk(tag) == 2) {
                     // end of block commit
-                    // TODO handle backwards compat?
                     dir->erased = false;
                     break;
+                } else {
+                    // for backwards compatibility we fall through here on
+                    // unrecognized tags, leaving it up to the CRC to reject
+                    // bad commits
                 }
-
+            } else {
-                continue;
-            }
-
                 // crc the entry first, hopefully leaving it in the cache
                 err = lfs_bd_crc(lfs,
                         NULL, &lfs->rcache, lfs->cfg->block_size,
@@ -1125,11 +1117,13 @@ static lfs_stag_t lfs_dir_fetchmatch(lfs_t *lfs,
                         tempbesttag = -1;
                     } else if (res == LFS_CMP_GT &&
                             lfs_tag_id(tag) <= lfs_tag_id(tempbesttag)) {
-                    // found a greater match, keep track to keep things sorted
+                        // found a greater match, keep track to keep
+                        // things sorted
                         tempbesttag = tag | 0x80000000;
                     }
                 }
             }
+        }
 
         // consider what we have good enough
         if (dir->off > 0) {
@@ -1484,7 +1478,6 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
             int err = lfs_bd_read(lfs,
                     NULL, &lfs->rcache, esize,
                     commit->block, noff, &etag, sizeof(etag));
-            // TODO handle erased-as-corrupt correctly?
             if (err && err != LFS_ERR_CORRUPT) {
                 return err;
             }
@@ -1494,7 +1487,6 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
             err = lfs_bd_crc(lfs,
                     NULL, &lfs->rcache, esize,
                     commit->block, noff, esize, &ecrc);
-            // TODO handle erased-as-corrupt correctly?
             if (err && err != LFS_ERR_CORRUPT) {
                 return err;
             }
@@ -1540,44 +1532,34 @@ static int lfs_dir_commitcrc(lfs_t *lfs, struct lfs_commit *commit) {
     }
 
     // successful commit, check checksums to make sure
+    //
+    // note that we don't need to check padding commits, worst
+    // case if they are corrupted we would have had to compact anyways
     lfs_off_t off = commit->begin;
-    lfs_off_t noff = off1;
-    while (off < end) {
-        // TODO restructure to use lfs_bd_crc?
     uint32_t crc = 0xffffffff;
-        for (lfs_off_t i = off; i < noff+sizeof(uint32_t); i++) {
+    err = lfs_bd_crc(lfs,
-            // check against written crc, may catch blocks that
+            NULL, &lfs->rcache, off1+sizeof(uint32_t),
-            // become readonly and match our commit size exactly
+            commit->block, off, off1-off, &crc);
-            if (i == off1 && crc != crc1) {
-                return LFS_ERR_CORRUPT;
-            }
-
-            uint8_t dat;
-            err = lfs_bd_read(lfs,
-                    NULL, &lfs->rcache, noff+sizeof(uint32_t)-i,
-                    commit->block, i, &dat, 1);
     if (err) {
         return err;
     }
 
-            crc = lfs_crc(crc, &dat, 1);
+    // check against known crc for non-padding commits
-        }
+    if (crc != crc1) {
-
-        // detected write error?
-        if (crc != 0) {
         return LFS_ERR_CORRUPT;
     }
 
-        // skip padding, note that these always contain estate
+    // make sure to check crc in case we happened to pick
-        off = noff - 3*sizeof(uint32_t);
+    // up an unrelated crc (frozen block?)
-        off = lfs_min(end - off, 0x3fe) + off;
+    err = lfs_bd_crc(lfs,
-        if (off < end) {
+            NULL, &lfs->rcache, sizeof(uint32_t),
-            off = lfs_min(off, end - 2*sizeof(uint32_t));
+            commit->block, off1, sizeof(uint32_t), &crc);
-        }
+    if (err) {
-        noff = off + sizeof(uint32_t);
+        return err;
-        if (noff <= lfs->cfg->block_size - esize) {
-            noff += 2*sizeof(uint32_t);
     }
+
+    if (crc != 0) {
+        return LFS_ERR_CORRUPT;
     }
 
     return 0;

tests/test_powerloss.toml

@@ -84,6 +84,7 @@ code = '''
 '''
 
 [[case]] # partial prog, may not be byte in order!
+if = "LFS_PROG_SIZE < LFS_BLOCK_SIZE"
 define.BYTE_OFF = ["0", "LFS_PROG_SIZE-1", "LFS_PROG_SIZE/2"]
 define.BYTE_VALUE = [0x33, 0xcc]
 in = "lfs.c"