sys/log, fs/fcb: Address some review comments

vrahane · vrahane · commit 3bcb976e5189 · 2025-02-14T18:53:24.000-08:00
diff --git a/fs/fcb/include/fcb/fcb.h b/fs/fcb/include/fcb/fcb.h
@@ -172,16 +172,16 @@ int fcb_getnext(struct fcb *, struct fcb_entry *loc);
  * Get first entry in the provided flash area
  *
  * @param fcb Pointer to FCB
- * @param fa Optional pointer to flash area
+ * @param fap Optional pointer to flash area
  * @param loc Pointer to first FCB entry in the provided flash area
  *
  * @return 0 on success, non-zero on failure
  */
-int fcb_getnext_in_area(struct fcb *fcb, struct flash_area *fa,
+int fcb_getnext_in_area(struct fcb *fcb, struct flash_area *fap,
                         struct fcb_entry *loc);
 
 /**
- * Get next area pointer from the FCB pointer to by fcb pointer
+ * Get next area pointer from the FCB pointer
  *
  * @param fcb Pointer to the FCB
  * @param fap Pointer to the flash_area
diff --git a/fs/fcb/src/fcb_getnext.c b/fs/fcb/src/fcb_getnext.c
@@ -63,14 +63,14 @@ fcb_start_offset(struct fcb *fcb)
 }
 
 int
-fcb_getnext_in_area(struct fcb *fcb, struct flash_area *fa,
+fcb_getnext_in_area(struct fcb *fcb, struct flash_area *fap,
                     struct fcb_entry *loc)
 {
     int rc;
 
     /* If a flash area is specified, find first entry in that area */
-    if (fa) {
-        loc->fe_area = fa;
+    if (fap) {
+        loc->fe_area = fap;
         loc->fe_elem_off = fcb_len_in_flash(fcb, sizeof(struct fcb_disk_area));
         loc->fe_elem_ix = 0;
         loc->fe_data_len = 0;
diff --git a/sys/log/full/include/log/log_fcb.h b/sys/log/full/include/log/log_fcb.h
@@ -39,31 +39,27 @@ struct log_fcb_bmark {
 #endif
     /* The index of the log entry that the FCB entry contains. */
     uint32_t lfb_index;
-    /* Is this a sector boundary bookmark */
-    bool lfb_sect_bmark;
 };
 
 /** A set of fcb log bookmarks. */
 struct log_fcb_bset {
     /** Array of bookmarks. */
     struct log_fcb_bmark *lfs_bmarks;
 
+    /** Enable sector bookmarks */
     bool lfs_en_sect_bmarks;
 
     /** The maximum number of bookmarks. */
     int lfs_cap;
 
     /** The number of currently used non-sector bookmarks. */
-    int lfs_non_s_size;
+    int lfs_non_sect_size;
 
     /** The number of currently usable bookmarks. */
     int lfs_size;
 
-    /** The index where the next bookmark will get written. */
-    uint32_t lfs_next;
-
     /** The index where the next non-sector bmark will get written */
-    uint32_t lfs_next_non_s;
+    uint32_t lfs_next_non_sect;
 };
 
 /**
diff --git a/sys/log/full/src/log_fcb.c b/sys/log/full/src/log_fcb.c
@@ -113,6 +113,8 @@ fcb_walk_back_find_start(struct fcb *fcb, struct log *log,
  * If bmark is found, fill up min_diff.
  *
  * The "index" field corresponds to a log entry index.
+ * For min_diff, if bookmark was found, fill it up with the difference
+ * else it will return -1.
  *
  * If bookmarks are enabled, this function uses them in the search.
  *
diff --git a/sys/log/full/src/log_fcb2.c b/sys/log/full/src/log_fcb2.c
@@ -47,6 +47,8 @@ static int log_fcb2_rtr_erase(struct log *log);
  * The "index" field corresponds to a log entry index.
  *
  * If bookmarks are enabled, this function uses them in the search.
+ * For min_diff, if bookmark was found, fill it up with the difference
+ * else it will return -1.
  *
  * @return                      0 if an entry was found
  *                              SYS_ENOENT if there are no suitable entries.
@@ -185,7 +187,7 @@ log_fcb2_start_append(struct log *log, int len, struct fcb2_entry *loc)
          * bookmarks
          */
         log_fcb_init_bmarks(fcb_log, fcb_log->fl_bset.lfs_bmarks,
-                            fcb_log->fl_bset.lfs_cap);
+                            fcb_log->fl_bset.lfs_cap, true);
 #endif
 
 #if MYNEWT_VAL(LOG_STORAGE_WATERMARK)
diff --git a/sys/log/full/src/log_fcb_bmark.c b/sys/log/full/src/log_fcb_bmark.c
@@ -121,8 +121,8 @@ log_fcb_clear_bmarks(struct fcb_log *fcb_log)
 {
     fcb_log->fl_bset.lfs_size = 0;
     fcb_log->fl_bset.lfs_next = 0;
-    fcb_log->fl_bset.lfs_next_non_s = 0;
-    fcb_log->fl_bset.lfs_non_s_size = 0;
+    fcb_log->fl_bset.lfs_next_non_sect = 0;
+    fcb_log->fl_bset.lfs_non_sect_size = 0;
     memset(fcb_log->fl_bset.lfs_bmarks, 0,
            sizeof(struct log_fcb_bmark) *
            fcb_log->fl_bset.lfs_cap);
@@ -172,9 +172,9 @@ log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
             if (diff < *min_diff) {
                 *min_diff = diff;
                 closest = bmark;
-                MODLOG_INFO(LOG_MODULE_DEFAULT, "index: %u, closest bmark idx: %u, \n",
-                            (unsigned int)index,
-                            (unsigned int)bmark->lfb_index);
+                MODLOG_DEBUG(LOG_MODULE_DEFAULT, "index: %u, closest bmark idx: %u, \n",
+                             (unsigned int)index,
+                             (unsigned int)bmark->lfb_index);
                 /* We found the exact match, no need to keep searching for a
                  * better match
                  */
@@ -191,57 +191,49 @@ log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
 #if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
 #if MYNEWT_VAL(LOG_FCB)
 static void
-log_fcb_insert_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
-                     uint32_t index, bool sect_bmark, int pos)
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+                          uint32_t index, int pos)
 #elif MYNEWT_VAL(LOG_FCB2)
 static void
-log_fcb_insert_bmark(struct fcb_log *fcb_log, fcb2_entry *entry,
-                     uint32_t index, bool sect_bmark, int pos)
+log_fcb_insert_sect_bmark(struct fcb_log *fcb_log, fcb2_entry *entry,
+                          uint32_t index, int pos)
 #endif
 {
     struct log_fcb_bset *bset;
 
     bset = &fcb_log->fl_bset;
 
-    if (pos == bset->lfs_cap - 1) {
-        goto add;
+    if (pos != bset->lfs_cap - 1) {
+        memmove(&bset->lfs_bmarks[pos + 1], &bset->lfs_bmarks[pos],
+                sizeof(bset->lfs_bmarks[0]) * (bset->lfs_size - pos - 1));
     }
 
-    memcpy(&bset->lfs_bmarks[pos + 1], &bset->lfs_bmarks[pos],
-           sizeof(bset->lfs_bmarks[0]) * bset->lfs_size - pos - 1);
-
-add:
     bset->lfs_bmarks[pos] = (struct log_fcb_bmark) {
         .lfb_entry = *entry,
         .lfb_index = index,
-        .lfb_sect_bmark = sect_bmark
     };
 
     if (bset->lfs_size < bset->lfs_cap) {
         bset->lfs_size++;
     }
-
-    bset->lfs_next++;
-    bset->lfs_next %= bset->lfs_cap;
 }
 #endif
 
 #if MYNEWT_VAL(LOG_FCB)
 static void
-log_fcb_replace_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
-                      uint32_t index, bool sect_bmark, int pos)
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+                               uint32_t index, int pos)
 #elif MYNEWT_VAL(LOG_FCB2)
 static void
-log_fcb_replace_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
-                      uint32_t index, bool sect_bmark, int pos)
+log_fcb_replace_non_sect_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+                               uint32_t index, int pos)
 #endif
 {
     int i = 0;
     struct log_fcb_bset *bset = &fcb_log->fl_bset;
-    bool en_sect_bmark = sect_bmark & bset->lfs_en_sect_bmarks;
 
 #if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
-    if (en_sect_bmark) {
+    if (bset->lfs_en_sect_bmarks) {
         for (i = fcb_log->fl_fcb.f_sector_cnt;
              i < (bset->lfs_non_s_size + fcb_log->fl_fcb.f_sector_cnt);
              i++) {
@@ -252,8 +244,8 @@ log_fcb_replace_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
         }
     } else
 #endif
-    if (!en_sect_bmark) {
-        for (i = 0; i < bset->lfs_non_s_size; i++) {
+    {
+        for (i = 0; i < bset->lfs_non_sect_size; i++) {
             if (index == bset->lfs_bmarks[i].lfb_index) {
                 /* If index matches, no need to replace */
                 return;
@@ -264,7 +256,6 @@ log_fcb_replace_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
     bset->lfs_bmarks[pos] = (struct log_fcb_bmark) {
         .lfb_entry = *entry,
         .lfb_index = index,
-        .lfb_sect_bmark = sect_bmark
     };
 }
 
@@ -300,7 +291,7 @@ log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
                 pos = i + 1;
             }
         }
-        log_fcb_insert_bmark(fcb_log, entry, index, sect_bmark, pos);
+        log_fcb_insert_sect_bmark(fcb_log, entry, index, pos);
         if (pos >= fcb_log->fl_fcb.f_sector_cnt - 1) {
             /* Make sure inserts do not trickle into the non-sector
              * bookmarks
@@ -313,48 +304,49 @@ log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
         }
     } else {
         /* Replace oldest non-sector bmark */
-        log_fcb_replace_bmark(fcb_log, entry, index, sect_bmark,
-                              bset->lfs_next_non_s +
-                              bset->lfs_en_sect_bmarks ? fcb_log->fl_fcb.f_sector_cnt
-                              : 0);
+        log_fcb_replace_non_sect_bmark(fcb_log, entry, index,
+                                       bset->lfs_next_non_sect +
+                                       (bset->lfs_en_sect_bmarks ?
+                                        fcb_log->fl_fcb.f_sector_cnt : 0));
         MODLOG_DEBUG(LOG_MODULE_DEFAULT, "replace bmark index: %u, pos: %u, \n",
                      (unsigned int)index,
-                     (unsigned int)bset->lfs_next_non_s + bset->lfs_en_sect_bmarks ?
-                     fcb_log->fl_fcb.f_sector_cnt : 0);
-        if (bset->lfs_non_s_size >= MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS)) {
-            bset->lfs_next_non_s = (bset->lfs_next_non_s + 1) %
-                                   MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS);
+                     (unsigned int)bset->lfs_next_non_sect +
+                     (bset->lfs_en_sect_bmarks ?
+                      fcb_log->fl_fcb.f_sector_cnt : 0));
+        if (bset->lfs_non_sect_size >= MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS)) {
+            bset->lfs_next_non_sect = (bset->lfs_next_non_sect + 1) %
+                                      MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS);
         } else {
-            if (!bset->lfs_non_s_size) {
+            if (!bset->lfs_non_sect_size) {
                 /* First non-sector bmark position */
-                bset->lfs_next_non_s = pos;
+                bset->lfs_next_non_sect = pos;
             }
-            bset->lfs_non_s_size++;
+            bset->lfs_non_sect_size++;
             bset->lfs_size++;
         }
 
-        assert(bset->lfs_non_s_size <= MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS));
+        assert(bset->lfs_non_sect_size <= MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS));
     }
 #else
     if (!sect_bmark) {
         if (bset->lfs_size >= MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS)) {
             /* Replace oldest non-sector bmark */
-            log_fcb_replace_bmark(fcb_log, entry, index, sect_bmark,
-                                  bset->lfs_next_non_s);
+            log_fcb_replace_non_sect_bmark(fcb_log, entry, index,
+                                           bset->lfs_next_non_sect);
             MODLOG_DEBUG(LOG_MODULE_DEFAULT, "replace bmark index: %u, pos: %u, \n",
                          (unsigned int)index,
-                         (unsigned int)bset->lfs_next_non_s);
-            bset->lfs_next_non_s = (bset->lfs_next_non_s + 1) %
-                                   MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS);
+                         (unsigned int)bset->lfs_next_non_sect);
+            bset->lfs_next_non_sect = (bset->lfs_next_non_sect + 1) %
+                                      MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS);
         } else {
-            log_fcb_replace_bmark(fcb_log, entry, index, sect_bmark,
-                                  bset->lfs_size);
+            log_fcb_replace_non_sect_bmark(fcb_log, entry, index,
+                                           bset->lfs_size);
             MODLOG_DEBUG(LOG_MODULE_DEFAULT, "replace bmark index: %u, pos: %u, \n",
                          (unsigned int)index,
                          bset->lfs_size);
             if (!bset->lfs_size) {
                 /* First non-sector bmark position */
-                bset->lfs_next_non_s = pos;
+                bset->lfs_next_non_sect = pos;
             }
             bset->lfs_size++;
         }

Original file line number	Diff line number	Diff line change
`@@ -172,16 +172,16 @@ int fcb_getnext(struct fcb , struct fcb_entry loc);`
`172`	`172`	`* Get first entry in the provided flash area`
`173`	`173`	`*`
`174`	`174`	`* @param fcb Pointer to FCB`
`175`		`- * @param fa Optional pointer to flash area`
	`175`	`+ * @param fap Optional pointer to flash area`
`176`	`176`	`* @param loc Pointer to first FCB entry in the provided flash area`
`177`	`177`	`*`
`178`	`178`	`* @return 0 on success, non-zero on failure`
`179`	`179`	`*/`
`180`		`-int fcb_getnext_in_area(struct fcb fcb, struct flash_area fa,`
	`180`	`+int fcb_getnext_in_area(struct fcb fcb, struct flash_area fap,`
`181`	`181`	`struct fcb_entry *loc);`
`182`	`182`
`183`	`183`	`/**`
`184`		`- * Get next area pointer from the FCB pointer to by fcb pointer`
	`184`	`+ * Get next area pointer from the FCB pointer`
`185`	`185`	`*`
`186`	`186`	`* @param fcb Pointer to the FCB`
`187`	`187`	`* @param fap Pointer to the flash_area`
Original file line number	Diff line number	Diff line change
`@@ -113,6 +113,8 @@ fcb_walk_back_find_start(struct fcb fcb, struct log log,`
`113`	`113`	`* If bmark is found, fill up min_diff.`
`114`	`114`	`*`
`115`	`115`	`* The "index" field corresponds to a log entry index.`
	`116`	`+ * For min_diff, if bookmark was found, fill it up with the difference`
	`117`	`+ * else it will return -1.`
`116`	`118`	`*`
`117`	`119`	`* If bookmarks are enabled, this function uses them in the search.`
`118`	`120`	`*`