From 7614cf29c1f79223af1088b78a15352c867dcb9b Mon Sep 17 00:00:00 2001
From: Christopher Haster <geky@geky.net>
Date: Wed, 9 Aug 2023 19:16:29 -0500
Subject: [PATCH] Adopted lfsr_data_t in low-level rbyd functions

The low-level rbyd functions need to parse things (mostly tags), so why
not use our parsers? In theory this offers a bit more code reuse.

In theory we can also rely on lfsr_data_t to do bounds checking of
offsets in the block, in practice we need to setup those bounds
correctly for lfsr_data_t, so not so much...

Code/stack cost:

            code          stack
  before:  21702           1992
  after:   21626 (-0.4%)   2024 (+1.6%)
---
 lfs.c | 331 ++++++++++++++++++++++++++++------------------------------
 1 file changed, 162 insertions(+), 169 deletions(-)

diff --git a/lfs.c b/lfs.c
index 0b027798..1d2441e7 100644
--- a/lfs.c
+++ b/lfs.c
@@ -908,116 +908,6 @@ static inline void lfsr_tag_trim2(
             lower_tag, upper_tag);
 }
 
-// support for encoding/decoding tags on disk
-
-// each piece of metadata in an rbyd tree is prefixed with a 4-piece tag:
-//
-// - 8-bit suptype     => 1 byte
-// - 8-bit subtype     => 1 byte
-// - 32-bit rid/weight => 5 byte leb128 (worst case)
-// - 32-bit size/jump  => 5 byte leb128 (worst case)
-//                     => 12 bytes total
-//
-#define LFSR_TAG_DSIZE (2+5+5)
-
-static lfs_ssize_t lfsr_bd_readtag(lfs_t *lfs,
-        lfs_block_t block, lfs_off_t off, lfs_size_t hint,
-        lfsr_tag_t *tag_, lfs_size_t *weight_, lfs_size_t *size_,
-        uint32_t *cksum_) {
-    // read the largest possible tag size
-    uint8_t tag_buf[LFSR_TAG_DSIZE];
-    lfs_size_t tag_dsize = lfs_min32(LFSR_TAG_DSIZE, lfs->cfg->block_size-off);
-    int err = lfsr_bd_read(lfs, block, off, hint, &tag_buf, tag_dsize);
-    if (err) {
-        return err;
-    }
-
-    if (tag_dsize < 2) {
-        return LFS_ERR_CORRUPT;
-    }
-    uint16_t tag
-            = ((lfsr_tag_t)tag_buf[0] << 8)
-            | ((lfsr_tag_t)tag_buf[1] << 0);
-    ssize_t d = 2;
-
-    if (cksum_) {
-        // on-disk, the tags valid bit must reflect the parity of the
-        // preceding data, fortunately for crc32c, this is the same as the
-        // parity of the crc
-        //
-        // note we need to do this before leb128 decoding as we may not have
-        // valid leb128 if we're erased, but we shouldn't treat a truncated
-        // leb128 here as corruption
-        if ((tag >> 15) != (lfs_popc(*cksum_) & 1)) {
-            return LFS_ERR_INVAL;
-        }
-    }
-
-    lfs_ssize_t weight;
-    lfs_ssize_t d_ = lfs_fromleb128(&weight, &tag_buf[d], tag_dsize-d);
-    if (d_ < 0) {
-        return d_;
-    }
-    d += d_;
-
-    lfs_ssize_t size;
-    d_ = lfs_fromleb128(&size, &tag_buf[d], tag_dsize-d);
-    if (d_ < 0) {
-        return d_;
-    }
-    d += d_;
-
-    // optional checksum
-    if (cksum_) {
-        *cksum_ = lfs_crc32c(*cksum_, tag_buf, d);
-    }
-
-    // save what we found, clearing the valid bit from the tag, note we
-    // checked this earlier
-    *tag_ = tag & 0x7fff;
-    *weight_ = weight;
-    *size_ = size;
-    return d;
-}
-
-static lfs_ssize_t lfsr_bd_progtag(lfs_t *lfs,
-        lfs_block_t block, lfs_off_t off,
-        lfsr_tag_t tag, lfs_size_t weight, lfs_size_t size,
-        uint32_t *cksum_) {
-    // check for underflow issues
-    LFS_ASSERT(weight < 0x80000000);
-    LFS_ASSERT(size < 0x80000000);
-
-    // make sure to include the parity of the current crc
-    tag |= (lfs_popc(*cksum_) & 1) << 15;
-
-    // encode into a be16 and pair of leb128s
-    uint8_t tag_buf[LFSR_TAG_DSIZE];
-    tag_buf[0] = (uint8_t)(tag >> 8);
-    tag_buf[1] = (uint8_t)(tag >> 0);
-
-    lfs_size_t d = 2;
-    ssize_t d_ = lfs_toleb128(weight, &tag_buf[d], 5);
-    if (d_ < 0) {
-        return d_;
-    }
-    d += d_;
-
-    d_ = lfs_toleb128(size, &tag_buf[d], 5);
-    if (d_ < 0) {
-        return d_;
-    }
-    d += d_;
-
-    int err = lfsr_bd_prog(lfs, block, off, &tag_buf, d, cksum_);
-    if (err) {
-        return err;
-    }
-
-    return d;
-}
-
-
 
 /// lfsr_data_t stuff ///
 
@@ -1310,6 +1200,121 @@ static int lfsr_bd_progdata(lfs_t *lfs,
 }
 
 
+// support for encoding/decoding tags on disk
+
+// each piece of metadata in an rbyd tree is prefixed with a 4-piece tag:
+//
+// - 8-bit suptype     => 1 byte
+// - 8-bit subtype     => 1 byte
+// - 32-bit rid/weight => 5 byte leb128 (worst case)
+// - 32-bit size/jump  => 5 byte leb128 (worst case)
+//                     => 12 bytes total
+//
+#define LFSR_TAG_DSIZE (2+5+5)
+
+static int lfsr_data_readtag(lfs_t *lfs, lfsr_data_t *data,
+        lfsr_tag_t *tag_, lfs_size_t *weight_, lfs_size_t *size_,
+        uint32_t *cksum_) {
+    // note we make sure not to update our data offset until after decoding
+    lfsr_data_t data_ = *data;
+
+    // read the largest possible tag size
+    uint8_t tag_buf[LFSR_TAG_DSIZE];
+    lfs_ssize_t tag_dsize = lfsr_data_read(lfs, &data_,
+            tag_buf, LFSR_TAG_DSIZE);
+    if (tag_dsize < 0) {
+        return tag_dsize;
+    }
+
+    if (tag_dsize < 2) {
+        return LFS_ERR_CORRUPT;
+    }
+    uint16_t tag
+            = ((lfsr_tag_t)tag_buf[0] << 8)
+            | ((lfsr_tag_t)tag_buf[1] << 0);
+    ssize_t d = 2;
+
+    if (cksum_) {
+        // on-disk, the tags valid bit must reflect the parity of the
+        // preceding data, fortunately for crc32c, this is the same as the
+        // parity of the crc
+        //
+        // note we need to do this before leb128 decoding as we may not have
+        // valid leb128 if we're erased, but we shouldn't treat a truncated
+        // leb128 here as corruption
+        if ((tag >> 15) != (lfs_popc(*cksum_) & 1)) {
+            return LFS_ERR_INVAL;
+        }
+    }
+
+    lfs_ssize_t weight;
+    lfs_ssize_t d_ = lfs_fromleb128(&weight, &tag_buf[d], tag_dsize-d);
+    if (d_ < 0) {
+        return d_;
+    }
+    d += d_;
+
+    lfs_ssize_t size;
+    d_ = lfs_fromleb128(&size, &tag_buf[d], tag_dsize-d);
+    if (d_ < 0) {
+        return d_;
+    }
+    d += d_;
+
+    // optional checksum
+    if (cksum_) {
+        *cksum_ = lfs_crc32c(*cksum_, tag_buf, d);
+    }
+
+    // save what we found, clearing the valid bit from the tag, note we
+    // checked this earlier
+    lfsr_data_add(data, d);
+    *tag_ = tag & 0x7fff;
+    *weight_ = weight;
+    *size_ = size;
+    return 0;
+}
+
+static lfs_ssize_t lfsr_bd_progtag(lfs_t *lfs,
+        lfs_block_t block, lfs_off_t off,
+        lfsr_tag_t tag, lfs_size_t weight, lfs_size_t size,
+        uint32_t *cksum_) {
+    // check for underflow issues
+    LFS_ASSERT(weight < 0x80000000);
+    LFS_ASSERT(size < 0x80000000);
+
+    // make sure to include the parity of the current crc
+    tag |= (lfs_popc(*cksum_) & 1) << 15;
+
+    // encode into a be16 and pair of leb128s
+    uint8_t tag_buf[LFSR_TAG_DSIZE];
+    tag_buf[0] = (uint8_t)(tag >> 8);
+    tag_buf[1] = (uint8_t)(tag >> 0);
+
+    lfs_size_t d = 2;
+    ssize_t d_ = lfs_toleb128(weight, &tag_buf[d], 5);
+    if (d_ < 0) {
+        return d_;
+    }
+    d += d_;
+
+    d_ = lfs_toleb128(size, &tag_buf[d], 5);
+    if (d_ < 0) {
+        return d_;
+    }
+    d += d_;
+
+    int err = lfsr_bd_prog(lfs, block, off, &tag_buf, d, cksum_);
+    if (err) {
+        return err;
+    }
+
+    return d;
+}
+
+
+
+
 // operations on attribute lists
 
 //struct lfs_mattr {
@@ -1833,7 +1838,8 @@ static int lfsr_rbyd_fetch(lfs_t *lfs, lfsr_rbyd_t *rbyd,
     rbyd->trunk = 0;
 
     // temporary state until we validate a cksum
-    lfs_off_t off = sizeof(uint32_t);
+    lfsr_data_t data = LFSR_DATA_DISK(block, sizeof(uint32_t),
+            lfs->cfg->block_size - sizeof(uint32_t));
     lfs_off_t trunk_ = 0;
     bool wastrunk = false;
     lfs_size_t weight = 0;
@@ -1845,31 +1851,32 @@ static int lfsr_rbyd_fetch(lfs_t *lfs, lfsr_rbyd_t *rbyd,
     bool maybeerased = false;
 
     // scan tags, checking valid bits, cksums, etc
-    while (off < lfs->cfg->block_size && (!trunk || rbyd->eoff <= trunk)) {
+    while (lfsr_data_size(&data) > 0 && (!trunk || rbyd->eoff <= trunk)) {
         lfsr_tag_t tag;
         lfs_size_t weight__;
         lfs_size_t size;
-        lfs_ssize_t d = lfsr_bd_readtag(lfs,
-                block, off, lfs->cfg->block_size,
+        lfsr_data_t data_ = data;
+        err = lfsr_data_readtag(lfs, &data_,
                 &tag, &weight__, &size, &cksum);
-        if (d < 0) {
-            if (d == LFS_ERR_INVAL || d == LFS_ERR_CORRUPT) {
-                maybeerased = maybeerased && d == LFS_ERR_INVAL;
+        if (err) {
+            if (err == LFS_ERR_INVAL || err == LFS_ERR_CORRUPT) {
+                maybeerased = maybeerased && err == LFS_ERR_INVAL;
                 break;
             }
-            return d;
+            return err;
         }
-        off += d;
 
         // tag goes out of range?
-        if (!lfsr_tag_isalt(tag) && off + size > lfs->cfg->block_size) {
+        if (!lfsr_tag_isalt(tag)
+                && data_.u.d.off + size > lfs->cfg->block_size) {
             break;
         }
 
         // not an end-of-commit cksum
         if (!lfsr_tag_isalt(tag) && lfsr_tag_suptype(tag) != LFSR_TAG_CKSUM) {
             // cksum the entry, hopefully leaving it in the cache
-            err = lfsr_bd_cksum(lfs, block, off, lfs->cfg->block_size, size,
+            err = lfsr_bd_cksum(lfs,
+                    block, data_.u.d.off, lfs->cfg->block_size, size,
                     &cksum);
             if (err) {
                 if (err == LFS_ERR_CORRUPT) {
@@ -1880,21 +1887,8 @@ static int lfsr_rbyd_fetch(lfs_t *lfs, lfsr_rbyd_t *rbyd,
 
             // found an ecksum? save for later
             if (tag == LFSR_TAG_ECKSUM) {
-                // TODO should we use LFSR_DATA_DISK here? hint issues?
-                uint8_t ecksum_buf[LFSR_ECKSUM_DSIZE];
-                err = lfsr_bd_read(lfs, block, off, lfs->cfg->block_size,
-                        ecksum_buf, lfs_min32(size, LFSR_ECKSUM_DSIZE));
-                if (err) {
-                    if (err == LFS_ERR_CORRUPT) {
-                        break;
-                    }
-                    return err;
-                }
-
-                err = lfsr_data_readecksum(lfs,
-                        &LFSR_DATA(ecksum_buf,
-                            lfs_min32(size, LFSR_ECKSUM_DSIZE)),
-                        &ecksum);
+                lfsr_data_t ecksum_data = data_;
+                err = lfsr_data_readecksum(lfs, &ecksum_data, &ecksum);
                 if (err && err != LFS_ERR_CORRUPT) {
                     return err;
                 }
@@ -1907,15 +1901,14 @@ static int lfsr_rbyd_fetch(lfs_t *lfs, lfsr_rbyd_t *rbyd,
         // is an end-of-commit cksum
         } else if (!lfsr_tag_isalt(tag)) {
             uint32_t cksum_ = 0;
-            err = lfsr_bd_read(lfs, block, off, lfs->cfg->block_size,
-                    &cksum_, sizeof(uint32_t));
+            lfsr_data_t cksum_data = data_;
+            err = lfsr_data_readle32(lfs, &cksum_data, &cksum_);
             if (err) {
                 if (err == LFS_ERR_CORRUPT) {
                     break;
                 }
                 return err;
             }
-            cksum_ = lfs_fromle32_(&cksum_);
 
             if (cksum != cksum_) {
                 // uh oh, cksums don't match
@@ -1933,7 +1926,7 @@ static int lfsr_rbyd_fetch(lfs_t *lfs, lfsr_rbyd_t *rbyd,
             hasecksum = false;
 
             // save what we've found so far
-            rbyd->eoff = off + size;
+            rbyd->eoff = data_.u.d.off + size;
             rbyd->cksum = cksum;
             rbyd->trunk = trunk_;
             rbyd->weight = weight;
@@ -1941,12 +1934,12 @@ static int lfsr_rbyd_fetch(lfs_t *lfs, lfsr_rbyd_t *rbyd,
 
         // found a trunk of a tree?
         if (lfsr_tag_istrunk(tag)
-                && (!trunk || trunk >= off-d || wastrunk)) {
+                && (!trunk || trunk >= data.u.d.off || wastrunk)) {
             // start of trunk?
             if (!wastrunk) {
                 wastrunk = true;
                 // save trunk entry point
-                trunk_ = off-d;
+                trunk_ = data.u.d.off;
                 // reset weight
                 weight_ = 0;
             }
@@ -1967,9 +1960,12 @@ static int lfsr_rbyd_fetch(lfs_t *lfs, lfsr_rbyd_t *rbyd,
             }
         }
 
+        // skip data
         if (!lfsr_tag_isalt(tag)) {
-            off += size;
+            lfsr_data_add(&data_, size);
         }
+
+        data = data_;
     }
 
     // no valid commits?
@@ -2029,11 +2025,12 @@ static int lfsr_rbyd_lookupnext(lfs_t *lfs, const lfsr_rbyd_t *rbyd,
         lfsr_tag_t alt;
         lfs_size_t weight;
         lfs_off_t jump;
-        lfs_ssize_t d = lfsr_bd_readtag(lfs,
-                rbyd->block, branch, 0,
+        lfsr_data_t data = LFSR_DATA_DISK(rbyd->block, branch,
+                lfs_min32(LFSR_TAG_DSIZE, lfs->cfg->block_size-branch));
+        int err = lfsr_data_readtag(lfs, &data,
                 &alt, &weight, &jump, NULL);
-        if (d < 0) {
-            return d;
+        if (err) {
+            return err;
         }
 
         // found an alt?
@@ -2044,7 +2041,7 @@ static int lfsr_rbyd_lookupnext(lfs_t *lfs, const lfsr_rbyd_t *rbyd,
                 branch = branch - jump;
             } else {
                 lfsr_tag_trim(alt, weight, &lower, &upper, NULL, NULL);
-                branch = branch + d;
+                branch = data.u.d.off;
             }
 
         // found end of tree?
@@ -2073,7 +2070,7 @@ static int lfsr_rbyd_lookupnext(lfs_t *lfs, const lfsr_rbyd_t *rbyd,
                 *weight_ = rid__ - lower;
             }
             if (data_) {
-                *data_ = LFSR_DATA_DISK(rbyd->block, branch + d, jump);
+                *data_ = LFSR_DATA_DISK(rbyd->block, data.u.d.off, jump);
             }
             return 0;
         }
@@ -2393,11 +2390,11 @@ static int lfsr_rbyd_append(lfs_t *lfs, lfsr_rbyd_t *rbyd,
         lfsr_tag_t alt;
         lfs_size_t weight;
         lfs_off_t jump;
-        lfs_ssize_t d = lfsr_bd_readtag(lfs,
-                rbyd->block, branch, 0,
+        lfsr_data_t data = LFSR_DATA_DISK(rbyd->block, branch,
+                lfs_min32(LFSR_TAG_DSIZE, lfs->cfg->block_size-branch));
+        err = lfsr_data_readtag(lfs, &data,
                 &alt, &weight, &jump, NULL);
-        if (d < 0) {
-            err = d;
+        if (err) {
             goto failed;
         }
 
@@ -2405,7 +2402,7 @@ static int lfsr_rbyd_append(lfs_t *lfs, lfsr_rbyd_t *rbyd,
         if (lfsr_tag_isalt(alt)) {
             // make jump absolute
             jump = branch - jump;
-            lfs_off_t branch_ = branch + d;
+            lfs_off_t branch_ = data.u.d.off;
 
             // do bounds want to take different paths? begin cutting
             if (!lfsr_tag_hasdiverged(tag_)
@@ -2907,7 +2904,7 @@ static int lfsr_rbyd_compact(lfs_t *lfs, lfsr_rbyd_t *rbyd,
     lfs_size_t layer_weight = 0;
 
     // first copy over raw tags, note this doesn't create a tree
-    lfs_off_t layer_start = rbyd->eoff;
+    lfs_off_t layer_off = rbyd->eoff;
     lfs_ssize_t rid = start_rid;
     lfsr_tag_t tag = 0;
     while (true) {
@@ -2951,7 +2948,6 @@ static int lfsr_rbyd_compact(lfs_t *lfs, lfsr_rbyd_t *rbyd,
         layer_trunks += 1;
         layer_weight += weight;
     }
-    lfs_off_t layer_end = rbyd->eoff;
 
     // connect every other trunk together, building layers of a perfectly
     // balanced binary tree upwards until we have a single trunk
@@ -2960,30 +2956,28 @@ static int lfsr_rbyd_compact(lfs_t *lfs, lfsr_rbyd_t *rbyd,
         layer_trunks = 0;
         layer_weight = 0;
 
-        lfs_off_t off = layer_start;
-        layer_start = rbyd->eoff;
-        while (off < layer_end) {
+        lfsr_data_t data = LFSR_DATA_DISK(rbyd->block,
+                layer_off, rbyd->eoff - layer_off);
+        layer_off = rbyd->eoff;
+        while (lfsr_data_size(&data) > 0) {
             // connect two trunks together with a new binary trunk
-            for (int i = 0; i < 2 && off < layer_end; i++) {
-                lfs_off_t trunk_off = off;
+            for (int i = 0; i < 2 && lfsr_data_size(&data) > 0; i++) {
+                lfs_off_t trunk_off = data.u.d.off;
                 lfsr_tag_t trunk_tag = 0;
                 lfs_size_t trunk_weight = 0;
                 while (true) {
                     lfsr_tag_t tag;
                     lfs_size_t weight;
                     lfs_size_t size;
-                    lfs_ssize_t d = lfsr_bd_readtag(lfs, rbyd->block, off,
-                            layer_end-off,
+                    err = lfsr_data_readtag(lfs, &data,
                             &tag, &weight, &size, NULL);
-                    if (d < 0) {
-                        err = d;
+                    if (err) {
                         goto failed;
                     }
-                    off += d;
 
                     // skip any data
                     if (!lfsr_tag_isalt(tag)) {
-                        off += size;
+                        lfsr_data_add(&data, size);
                     }
 
                     // keep track of trunk/layer weight, and the last non-null
@@ -3028,14 +3022,13 @@ static int lfsr_rbyd_compact(lfs_t *lfs, lfsr_rbyd_t *rbyd,
             // keep track of the number of trunks
             layer_trunks += 1;
         }
-        layer_end = rbyd->eoff;
     }
 
     // done! just need to update our trunk/weight, note we could have
     // no trunks after compaction. Leave this to upper layers to take
     // care of
     if (layer_trunks >= 1) {
-        rbyd->trunk = layer_start;
+        rbyd->trunk = layer_off;
     }
     rbyd->weight = layer_weight;