Clean up ks_set_attributes() a bit.

Fixed handling of deletion actions: code was still using the
zero-length attribute convention instead of HAL_PKEY_ATTRIBUTE_NIL.

Track existing attributes more closely while copying data from old
chunks to new ones in the slow path: the old algorithm had a few
dangerous corner cases which could have resulted in the wrong values
being written to the new chunks.

Single-block-update fast path now under compile-time conditional; in
the long run, we probably want this enabled, but it's disabled for now
in order to force use and testing of the slow path.

This function probably needs to be broken up into a collection of
smaller inline functions for readability.

1 files changed, 141 insertions, 32 deletions

diff --git a/ks_flash.c b/ks_flash.c
index e983c29..f784539 100644
--- a/ks_flash.c
+++ b/ks_flash.c
@@ -1223,21 +1223,39 @@ static hal_error_t ks_match(hal_ks_t *ks,
   return HAL_OK;
 }
 
+/*
+ * This controls whether we include a separate code path for the
+ * common case where we can handle attribute setting via a single
+ * block update.  It's a lot simpler when it works, but it's yet
+ * another code path, and enabling it leaves the slower full-blown
+ * algorithm less tested.
+ */
+
+#ifndef KS_SET_ATTRIBUTES_SINGLE_BLOCK_UPDATE_FAST_PATH
+#define KS_SET_ATTRIBUTES_SINGLE_BLOCK_UPDATE_FAST_PATH 0
+#endif
+
+/*
+ * ks_set_attributes() is much too long.  Probably needs to be broken
+ * up into a collection of inline functions, even if most of them end
+ * up being called exactly once.
+ */
+
 static  hal_error_t ks_set_attributes(hal_ks_t *ks,
                                       hal_pkey_slot_t *slot,
                                       const hal_pkey_attribute_t *attributes,
                                       const unsigned attributes_len)
 {
-#warning This function is much too long
-  // Probably needs to be broken up into multiple sub-functions, or at least
-  // into discrete blocks to scope most of the variables, but let's finish
-  // writing the icky version before polishing the chrome.
-
   if (ks != &db.ks || slot == NULL || attributes == NULL || attributes_len == 0)
     return HAL_ERROR_BAD_ARGUMENTS;
 
   /*
-   * Try to add new attribute as a single-block update.
+   * Perform initial scan of the object to figure out the total
+   * attribute count and a few other parameters.
+   *
+   * If enabled, try to do everything as as a single-block update.  If
+   * single block update fails, we MUST clear the modified block from
+   * the cache before doing anything else.
    */
 
   unsigned updated_attributes_len = attributes_len;
@@ -1270,13 +1288,15 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
 
     updated_attributes_len += *attrs_len;
 
+#if KS_SET_ATTRIBUTES_SINGLE_BLOCK_UPDATE_FAST_PATH
+
     hal_pkey_attribute_t attrs[*attrs_len + attributes_len];
     size_t total;
 
     if ((err = hal_ks_attribute_scan(bytes, bytes_len, attrs, *attrs_len, &total)) != HAL_OK)
       return err;
 
-    for (int i = 0; err == HAL_OK && i < attributes_len; i++) {
+    for (int i = 0; err == HAL_OK && i < attributes_len; i++)
       if (attributes[i].length == HAL_PKEY_ATTRIBUTE_NIL)
         err = hal_ks_attribute_delete(bytes, bytes_len, attrs, attrs_len, &total,
                                       attributes[i].type);
@@ -1285,9 +1305,9 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
                                       attributes[i].type,
                                       attributes[i].value,
                                       attributes[i].length);
-      if (err != HAL_OK)
-        cache_release(block);
-    }
+
+    if (err != HAL_OK)
+      cache_release(block);
 
     if (err == HAL_ERROR_RESULT_TOO_LONG)
       continue;
@@ -1297,14 +1317,17 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
 
     return block_update(b, block, &slot->name, chunk, &hint);
 
+#endif /* KS_SET_ATTRIBUTES_SINGLE_BLOCK_UPDATE_FAST_PATH */
+
   } while (++chunk < block->header.total_chunks);
 
   /*
-   * If we get here, we have to add a new block, which requires
-   * rewriting all the others to bump the total_blocks count.  We need
-   * to keep track of all the old chunks so we can zero them at the
-   * end, and because we can't zero them until we've written out the
-   * new chunks, we need enough free blocks for the entire new object.
+   * If we get here, we're on the slow path, which requires rewriting
+   * all the chunks in this object but which can also add or remove
+   * chunks from this object.  We need to keep track of all the old
+   * chunks so we can zero them at the end, and because we can't zero
+   * them until we've written out the new chunks, we need enough free
+   * blocks to hold all the new chunks.
    *
    * Calculating all of this is extremely tedious, but flash writes
    * are so much more expensive than anything else we do here that
@@ -1330,6 +1353,12 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
 
   updated_attributes_len = 0;
 
+  /*
+   * Phase 0.1: Walk the old chunks to populate updated_attributes[].
+   * This also initializes bytes_available, since we can only get that
+   * by reading old chunk zero.
+   */
+
   for (chunk = 0; chunk < total_chunks_old; chunk++) {
     int hint = slot->hint + chunk;
 
@@ -1359,39 +1388,56 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
       bytes_available = bytes_len;
 
     for (int i = 0; i < *attrs_len; i++) {
+
       if (updated_attributes_len >= sizeof(updated_attributes)/sizeof(*updated_attributes))
         return HAL_ERROR_IMPOSSIBLE;
-      updated_attributes[updated_attributes_len].type = attrs[i].type;
+
+      updated_attributes[updated_attributes_len].type   = attrs[i].type;
       updated_attributes[updated_attributes_len].length = attrs[i].length;
-      updated_attributes[updated_attributes_len].value = NULL;
+      updated_attributes[updated_attributes_len].value  = NULL;
       updated_attributes_len++;
     }
   }
 
+  /*
+   * Phase 0.2: Merge new attributes into updated_attributes[].
+   */
+
   for (int i = 0; i < attributes_len; i++) {
 
     for (int j = 0; j < updated_attributes_len; j++)
       if (updated_attributes[j].type == attributes[i].type)
-        updated_attributes[j].length = 0;
+        updated_attributes[j].length = HAL_PKEY_ATTRIBUTE_NIL;
 
     if (updated_attributes_len >= sizeof(updated_attributes)/sizeof(*updated_attributes))
       return HAL_ERROR_IMPOSSIBLE;
+
     updated_attributes[updated_attributes_len].type   = attributes[i].type;
     updated_attributes[updated_attributes_len].length = attributes[i].length;
     updated_attributes[updated_attributes_len].value  = attributes[i].value;
     updated_attributes_len++;
   }
 
+  /*
+   * Phase 0.3: Prune trailing deletion actions: we don't need them to
+   * maintain synchronization with existing attributes, and doing so
+   * simplifies logic for updating the final new chunk.
+   */
+
+  while (updated_attributes_len > 0 &&
+         updated_attributes[updated_attributes_len - 1].length == HAL_PKEY_ATTRIBUTE_NIL)
+    --updated_attributes_len;
+
+  /*
+   * Phase 0.4: Figure out how many chunks all this will occupy.
+   */
+
   chunk = 0;
 
   for (int i = 0; i < updated_attributes_len; i++) {
 
-    if (updated_attributes[i].length == 0) {
-      memmove(&updated_attributes[i], &updated_attributes[i + 1], updated_attributes_len - i - 1);
-      updated_attributes_len--;
-      i--;
+    if (updated_attributes[i].length == HAL_PKEY_ATTRIBUTE_NIL)
       continue;
-    }
 
     const size_t needed = hal_ks_attribute_header_size + updated_attributes[i].length;
 
@@ -1408,6 +1454,10 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
 
   const unsigned total_chunks_new = chunk + 1;
 
+  /*
+   * If there aren't enough free blocks, give up now, before changing anything.
+   */
+
   if (db.ksi.used + total_chunks_new > db.ksi.size)
     return HAL_ERROR_NO_KEY_INDEX_SLOTS;
 
@@ -1426,8 +1476,8 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
   }
 
   /*
-   * Phase 2: Write new chunks, copying attributes from old chunks and attributes[]
-   * as needed.  This is tedious.
+   * Phase 2: Write new chunks, copying attributes from old chunks or
+   * from attributes[], as needed.
    */
 
   {
@@ -1450,12 +1500,22 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
       if (old_chunk >= total_chunks_old || new_chunk >= total_chunks_new)
         return HAL_ERROR_IMPOSSIBLE;
 
-      if (updated_attributes[updated_attributes_i].value != NULL) {
+      /*
+       * If we've gotten as far as new data that comes from
+       * attributes[], we have it in hand and can just copy it.
+       */
+
+      if (updated_attributes_len - updated_attributes_i <= attributes_len) {
         new_attr_type   = updated_attributes[updated_attributes_i].type;
         new_attr_length = updated_attributes[updated_attributes_i].length;
         new_attr_value  = updated_attributes[updated_attributes_i].value;
       }
 
+      /*
+       * Otherwise, we have to read it from an old block, which may in
+       * turn require reading in the next old block.
+       */
+
       else {
 
         if (old_block == NULL) {
@@ -1475,10 +1535,6 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
           old_attrs_i = 0;
         }
 
-        while (old_attrs_i < *old_attrs_len &&
-               old_attrs[old_attrs_i].type != updated_attributes[updated_attributes_i].type)
-          old_attrs_i++;
-
         if (old_attrs_i >= *old_attrs_len) {
           old_chunk++;
           old_block = NULL;
@@ -1488,8 +1544,26 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
         new_attr_type   = old_attrs[old_attrs_i].type;
         new_attr_length = old_attrs[old_attrs_i].length;
         new_attr_value  = old_attrs[old_attrs_i].value;
+
+        if (new_attr_type != updated_attributes[updated_attributes_i].type)
+          return HAL_ERROR_IMPOSSIBLE;
+
+        old_attrs_i++;
       }
 
+      /*
+       * Unless this is a deletion, we should have something to write.
+       */
+
+      if (new_attr_length != HAL_PKEY_ATTRIBUTE_NIL && new_attr_value == NULL)
+        return HAL_ERROR_IMPOSSIBLE;
+
+      /*
+       * Initialize the new block if necessary.  If it's the new chunk
+       * zero, we need to copy all the non-attribute data from the old
+       * chunk zero; otherwise, it's a new empty attribute block.
+       */
+
       if (new_block == NULL) {
 
         new_block = cache_pick_lru();
@@ -1527,8 +1601,18 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
         new_total = 0;
       }
 
-      err = hal_ks_attribute_insert(new_bytes, new_bytes_len, new_attrs, new_attrs_len, &new_total,
-                                    new_attr_type, new_attr_value, new_attr_length);
+      /*
+       * After all that setup, we finally get to write the frelling attribute.
+       */
+
+      if (new_attr_length != HAL_PKEY_ATTRIBUTE_NIL)
+        err = hal_ks_attribute_insert(new_bytes, new_bytes_len, new_attrs, new_attrs_len, &new_total,
+                                      new_attr_type, new_attr_value, new_attr_length);
+
+      /*
+       * Figure out what to do next: immediately loop for next
+       * attribute, write current block, or bail out.
+       */
 
       switch (err) {
       case HAL_OK:
@@ -1543,6 +1627,11 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
         return err;
       }
 
+      /*
+       * If we get here, either the current new block is full or we
+       * finished the last block, so we need to write it out.
+       */
+
       int hint = slot->hint + new_chunk;
 
       if (new_chunk < total_chunks_old)
@@ -1558,6 +1647,20 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
       new_block = NULL;
       new_chunk++;
     }
+
+    /*
+     * If number of blocks shrank, we need to clear trailing entries from the index.
+     */
+
+    for (old_chunk = total_chunks_new; old_chunk < total_chunks_old; old_chunk++) {
+      int hint = slot->hint + old_chunk;
+
+      err = hal_ks_index_delete(&db.ksi, &slot->name, old_chunk, NULL, &hint);
+
+      if (err != HAL_OK)
+        return err;
+    }
+
   }
 
   /*
@@ -1569,6 +1672,12 @@ static  hal_error_t ks_set_attributes(hal_ks_t *ks,
       return err;
 
   return HAL_OK;
+
+#warning What happens if something goes wrong partway through this awful mess?
+  // We're left in a state with all the old blocks deprecated and
+  // (maybe) some of the new blocks current, need to clean that up.
+  // Would be nice if we could just reuse the keystore initialization
+  // code, but don't quite see how (yet?).
 }
 
 static  hal_error_t ks_get_attributes(hal_ks_t *ks,