kasjer commented on code in PR #3365:
URL: https://github.com/apache/mynewt-core/pull/3365#discussion_r1952536300
##########
fs/fcb/include/fcb/fcb.h:
##########
@@ -168,6 +168,28 @@ typedef int (*fcb_walk_cb)(struct fcb_entry *loc, void
*arg);
int fcb_walk(struct fcb *, struct flash_area *, fcb_walk_cb cb, void *cb_arg);
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 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,
+ struct fcb_entry *loc);
+
+/**
+ * Get next area pointer from the FCB pointer to by fcb pointer
Review Comment:
probably should be `pointed to` instead of `pointer to`
##########
sys/log/full/syscfg.yml:
##########
@@ -179,6 +179,18 @@ syscfg.defs:
the global index to be sequentially increasing for persisted logs.
value: 0
+ LOG_FCB_NUM_ABS_BMARKS:
Review Comment:
Inconsistent new syscfg values one uses `BOOKMARKS` another `BMARKS`
##########
sys/log/full/include/log/log_fcb.h:
##########
@@ -39,6 +39,8 @@ 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;
Review Comment:
This is never use
##########
sys/log/full/include/log/log_fcb.h:
##########
@@ -49,11 +51,17 @@ struct log_fcb_bset {
/** The maximum number of bookmarks. */
int lfs_cap;
+ /** The number of currently used non-sector bookmarks. */
+ int lfs_non_s_size;
+
/** The number of currently usable bookmarks. */
int lfs_size;
/** The index where the next bookmark will get written. */
- int lfs_next;
+ uint32_t lfs_next;
Review Comment:
`lfs_next` was used before but is not used anymore as an indicator of the
place to write
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -70,65 +118,246 @@ 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;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
+}
+
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
+{
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ 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);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, fcb2_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ if (pos == bset->lfs_cap - 1) {
+ goto add;
}
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+ memcpy(&bset->lfs_bmarks[pos + 1], &bset->lfs_bmarks[pos],
Review Comment:
memmove should be used
##########
sys/log/full/src/log_shell.c:
##########
@@ -39,7 +39,7 @@
#include "tinycbor/compilersupport_p.h"
#include "log_cbor_reader/log_cbor_reader.h"
-void log_console_print_hdr(const struct log_entry_hdr *hdr);
+void log_console_print_hdr(struct log_entry_hdr *hdr);
Review Comment:
What is the reason for this change?
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -70,65 +121,247 @@ 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;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ 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);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, fcb2_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ if (pos == bset->lfs_cap - 1) {
+ goto add;
}
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+ 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++;
- if (bset->lfs_next >= bset->lfs_cap) {
- bset->lfs_next = 0;
+ 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)
+#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)
+#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) {
+ for (i = fcb_log->fl_fcb.f_sector_cnt;
+ i < (bset->lfs_non_s_size + fcb_log->fl_fcb.f_sector_cnt);
+ i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return;
+ }
+ }
+ } else
+#endif
+ if (!en_sect_bmark) {
+ for (i = 0; i < bset->lfs_non_s_size; i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return;
+ }
+ }
}
Review Comment:
When sector bookmarks are enabled, this code looks for non-sector bookmarks
in wrong place that will result in duplicated non-sector bookmarks
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -22,17 +22,68 @@
#include "os/mynewt.h"
#if MYNEWT_VAL(LOG_FCB_BOOKMARKS)
-
+#include "log/log.h"
#include "log/log_fcb.h"
-void
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+static int
+log_fcb_init_sector_bmarks(struct fcb_log *fcb_log)
+{
+ int rc = 0;
+ int i = 0;
+ struct fcb_entry loc = {0};
+ struct log_entry_hdr ueh = {0};
+ struct flash_area *fa = NULL;
+
+ rc = fcb_getnext(&fcb_log->fl_fcb, &loc);
+ if (rc) {
+ return -1;
+ }
+
+ for (i = 0; i < fcb_log->fl_fcb.f_sector_cnt; i++) {
Review Comment:
Usage of `f_sector_cnt` suggest that there is some constraint on
`bmark_count` in call `log_fcb_init_bmarks()`.
It's not clear how user should compute array size needed for
`log_fcb_init_bmarks()`
##########
sys/log/full/include/log/log_fcb.h:
##########
@@ -39,21 +39,31 @@ 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;
+ bool lfs_en_sect_bmarks;
Review Comment:
field name is not clear. Does `_sect_` here mean what `_s_` mean few line
below?
##########
fs/fcb/include/fcb/fcb.h:
##########
@@ -168,6 +168,28 @@ typedef int (*fcb_walk_cb)(struct fcb_entry *loc, void
*arg);
int fcb_walk(struct fcb *, struct flash_area *, fcb_walk_cb cb, void *cb_arg);
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 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,
+ struct fcb_entry *loc);
+
+/**
+ * Get next area pointer from the FCB pointer to by fcb pointer
+ *
+ * @param fcb Pointer to the FCB
+ * @param fap Pointer to the flash_area
+ *
+ * @return Pointer to the flash_area that comes next
+ */
+struct flash_area *fcb_getnext_area(struct fcb *fcb, struct flash_area *fap);
Review Comment:
Slightly inconsistent argument name in `fcb_getnext_in_area()` and
`fcb_getnext_area()` -> `fa` and `fap`
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -70,65 +118,246 @@ 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;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
+}
+
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
+{
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ 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);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, fcb2_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ if (pos == bset->lfs_cap - 1) {
+ goto add;
}
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+ memcpy(&bset->lfs_bmarks[pos + 1], &bset->lfs_bmarks[pos],
+ sizeof(bset->lfs_bmarks[0]) * bset->lfs_size - pos - 1);
Review Comment:
size calculation is incorrect
##########
sys/log/full/src/log_shell.c:
##########
@@ -172,11 +174,18 @@ shell_log_dump_cmd(int argc, char **argv)
bool clear_log;
bool reverse = false;
bool dump_logs = true;
+ bool dump_bmarks = false;
+ uint32_t bmarks_size = 0;
+ struct log_fcb_bmark *bmarks = NULL;
struct walk_arg arg = {};
int i;
int rc;
clear_log = false;
+ (void)dump_bmarks;
+ (void)bmarks;
+ (void)bmarks_size;
Review Comment:
There is no reason to have those line
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -70,65 +121,247 @@ 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;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ 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);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, fcb2_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ if (pos == bset->lfs_cap - 1) {
+ goto add;
Review Comment:
using `goto` for skipping one statement recalls Fortran from the seventies :)
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -70,65 +121,247 @@ 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;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ 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);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
Review Comment:
Original code had `const fcb_entry *entry` is there a reason for dropping
`const` here?
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -70,65 +121,247 @@ 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;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ 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);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, fcb2_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ if (pos == bset->lfs_cap - 1) {
+ goto add;
}
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+ 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++;
- if (bset->lfs_next >= bset->lfs_cap) {
- bset->lfs_next = 0;
+ 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)
+#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)
+#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) {
+ for (i = fcb_log->fl_fcb.f_sector_cnt;
+ i < (bset->lfs_non_s_size + fcb_log->fl_fcb.f_sector_cnt);
+ i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return;
+ }
+ }
+ } else
+#endif
+ if (!en_sect_bmark) {
+ for (i = 0; i < bset->lfs_non_s_size; i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return;
+ }
+ }
}
+
+ bset->lfs_bmarks[pos] = (struct log_fcb_bmark) {
+ .lfb_entry = *entry,
+ .lfb_index = index,
+ .lfb_sect_bmark = sect_bmark
+ };
+}
+
+#if MYNEWT_VAL(LOG_FCB)
+void
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark)
+#elif MYNEWT_VAL(LOG_FCB2)
+void
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index, bool sect_bmark)
+#endif
+{
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ int i = 0;
+ int pos = 0;
+ bool en_sect_bmark = sect_bmark & bset->lfs_en_sect_bmarks;
+
+ (void)i;
+
+ if (bset->lfs_cap == 0) {
+ return;
+ }
+
+ (void)en_sect_bmark;
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (en_sect_bmark) {
+ for (i = 0; i < bset->lfs_size; i++) {
+ if (index > bset->lfs_bmarks[i].lfb_index) {
+ pos = i;
+ break;
+ } else {
+ pos = i + 1;
+ }
+ }
Review Comment:
`pos` may be set to `i` in line 297 and then loop will end.
If that never happens `pos` will be set to `bset->lfs_size` so there is no
point in updating it every time. `pos` can be set to this value before loop
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -70,65 +121,247 @@ 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;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#endif
Review Comment:
Those lines result in not finding exact (non-sector bookmark) that was
stored in previous walk.
Only after whole fcb is filled non-sector bookmarks can be found and used.
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -70,65 +121,247 @@ 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;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ 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);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, fcb2_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ if (pos == bset->lfs_cap - 1) {
+ goto add;
}
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+ 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++;
- if (bset->lfs_next >= bset->lfs_cap) {
- bset->lfs_next = 0;
+ 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)
+#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)
+#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) {
+ for (i = fcb_log->fl_fcb.f_sector_cnt;
+ i < (bset->lfs_non_s_size + fcb_log->fl_fcb.f_sector_cnt);
+ i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return;
+ }
+ }
+ } else
+#endif
+ if (!en_sect_bmark) {
+ for (i = 0; i < bset->lfs_non_s_size; i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return;
+ }
+ }
}
+
+ bset->lfs_bmarks[pos] = (struct log_fcb_bmark) {
+ .lfb_entry = *entry,
+ .lfb_index = index,
+ .lfb_sect_bmark = sect_bmark
+ };
+}
+
+#if MYNEWT_VAL(LOG_FCB)
+void
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark)
+#elif MYNEWT_VAL(LOG_FCB2)
+void
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index, bool sect_bmark)
+#endif
+{
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ int i = 0;
+ int pos = 0;
+ bool en_sect_bmark = sect_bmark & bset->lfs_en_sect_bmarks;
+
+ (void)i;
+
+ if (bset->lfs_cap == 0) {
+ return;
+ }
+
+ (void)en_sect_bmark;
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (en_sect_bmark) {
+ for (i = 0; i < bset->lfs_size; i++) {
+ if (index > bset->lfs_bmarks[i].lfb_index) {
+ pos = i;
+ break;
+ } else {
+ pos = i + 1;
+ }
+ }
+ log_fcb_insert_bmark(fcb_log, entry, index, sect_bmark, pos);
+ if (pos >= fcb_log->fl_fcb.f_sector_cnt - 1) {
+ /* Make sure inserts do not trickle into the non-sector
+ * bookmarks
+ */
+ memset(&bset->lfs_bmarks[pos], 0, sizeof(bset->lfs_bmarks[0]));
+ } else {
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "insert bmark index: %u, pos: %u,
\n",
+ (unsigned int)index,
+ (unsigned int)pos);
+ }
+ } 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);
Review Comment:
missing `(` `)` for correct calculation
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -70,65 +121,247 @@ 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;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ 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);
+ /* We found the exact match, no need to keep searching for a
+ * better match
+ */
+ if (*min_diff == 0) {
+ break;
+ }
}
}
}
return closest;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
#if MYNEWT_VAL(LOG_FCB)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#elif MYNEWT_VAL(LOG_FCB2)
-void
-log_fcb_add_bmark(struct fcb_log *fcb_log, const struct fcb2_entry *entry,
- uint32_t index)
+static void
+log_fcb_insert_bmark(struct fcb_log *fcb_log, fcb2_entry *entry,
+ uint32_t index, bool sect_bmark, int pos)
#endif
{
struct log_fcb_bset *bset;
bset = &fcb_log->fl_bset;
- if (bset->lfs_cap == 0) {
- return;
+ if (pos == bset->lfs_cap - 1) {
+ goto add;
}
- bset->lfs_bmarks[bset->lfs_next] = (struct log_fcb_bmark) {
+ 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++;
- if (bset->lfs_next >= bset->lfs_cap) {
- bset->lfs_next = 0;
+ 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)
+#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)
+#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) {
+ for (i = fcb_log->fl_fcb.f_sector_cnt;
+ i < (bset->lfs_non_s_size + fcb_log->fl_fcb.f_sector_cnt);
+ i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return;
+ }
+ }
+ } else
+#endif
+ if (!en_sect_bmark) {
+ for (i = 0; i < bset->lfs_non_s_size; i++) {
+ if (index == bset->lfs_bmarks[i].lfb_index) {
+ /* If index matches, no need to replace */
+ return;
+ }
+ }
}
+
+ bset->lfs_bmarks[pos] = (struct log_fcb_bmark) {
+ .lfb_entry = *entry,
+ .lfb_index = index,
+ .lfb_sect_bmark = sect_bmark
+ };
+}
+
+#if MYNEWT_VAL(LOG_FCB)
+void
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, bool sect_bmark)
+#elif MYNEWT_VAL(LOG_FCB2)
+void
+log_fcb_add_bmark(struct fcb_log *fcb_log, struct fcb2_entry *entry,
+ uint32_t index, bool sect_bmark)
+#endif
+{
+ struct log_fcb_bset *bset = &fcb_log->fl_bset;
+ int i = 0;
+ int pos = 0;
+ bool en_sect_bmark = sect_bmark & bset->lfs_en_sect_bmarks;
+
+ (void)i;
+
+ if (bset->lfs_cap == 0) {
+ return;
+ }
+
+ (void)en_sect_bmark;
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ if (en_sect_bmark) {
+ for (i = 0; i < bset->lfs_size; i++) {
+ if (index > bset->lfs_bmarks[i].lfb_index) {
+ pos = i;
+ break;
+ } else {
+ pos = i + 1;
+ }
+ }
+ log_fcb_insert_bmark(fcb_log, entry, index, sect_bmark, pos);
+ if (pos >= fcb_log->fl_fcb.f_sector_cnt - 1) {
+ /* Make sure inserts do not trickle into the non-sector
+ * bookmarks
+ */
+ memset(&bset->lfs_bmarks[pos], 0, sizeof(bset->lfs_bmarks[0]));
+ } else {
+ MODLOG_DEBUG(LOG_MODULE_DEFAULT, "insert bmark index: %u, pos: %u,
\n",
+ (unsigned int)index,
+ (unsigned int)pos);
+ }
+ } 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);
+ 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);
+ } else {
+ if (!bset->lfs_non_s_size) {
+ /* First non-sector bmark position */
+ bset->lfs_next_non_s = pos;
+ }
Review Comment:
this should be replaced by
```c
bset->lfs_next_non_s++;
```
current implementation overwrites same bookmark LOG_FCB_NUM_ABS_BMARKS times
or both ifs could be reduced to:
```c
if (bset->lfs_non_s_size < MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS)) {
bset->lfs_non_s_size++;
bset->lfs_size++;
}
bset->lfs_next_non_s = (bset->lfs_next_non_s + 1) %
MYNEWT_VAL(LOG_FCB_NUM_ABS_BMARKS);
```
##########
sys/log/full/src/log_fcb2.c:
##########
@@ -179,6 +180,14 @@ log_fcb2_start_append(struct log *log, int len, struct
fcb2_entry *loc)
goto err;
}
+#if MYNEWT_VAL(LOG_FCB_SECTOR_BOOKMARKS)
+ /* The FCB needs to be rotated, reinit previously allocated
+ * bookmarks
+ */
+ log_fcb_init_bmarks(fcb_log, fcb_log->fl_bset.lfs_bmarks,
Review Comment:
missing 4th argument
##########
sys/log/full/src/log_fcb2.c:
##########
@@ -476,12 +485,13 @@ log_fcb2_walk(struct log *log, log_walk_func_t walk_func,
struct fcb_log *fcb_log;
struct fcb2_entry loc;
int rc;
+ int min_diff = -1;
Review Comment:
name `min_diff` made sense in original code where it was used for comparison
with `diff`
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -70,65 +121,247 @@ 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;
+ memset(fcb_log->fl_bset.lfs_bmarks, 0,
+ sizeof(struct log_fcb_bmark) *
+ fcb_log->fl_bset.lfs_cap);
}
-const struct log_fcb_bmark *
-log_fcb_closest_bmark(const struct fcb_log *fcb_log, uint32_t index)
+struct log_fcb_bmark *
+log_fcb_get_bmarks(struct log *log, uint32_t *bmarks_size)
{
- const struct log_fcb_bmark *closest;
- const struct log_fcb_bmark *bmark;
- uint32_t min_diff;
+ struct fcb_log *fcb_log = (struct fcb_log *)log->l_arg;
+
+ *bmarks_size = fcb_log->fl_bset.lfs_cap;
+
+ return fcb_log->fl_bset.lfs_bmarks;
+}
+
+struct log_fcb_bmark *
+log_fcb_closest_bmark(struct fcb_log *fcb_log, uint32_t index,
+ int *min_diff)
+{
+ struct log_fcb_bmark *closest;
+ struct log_fcb_bmark *bmark;
uint32_t diff;
int i;
- min_diff = UINT32_MAX;
+ *min_diff = -1;
closest = NULL;
+ /* This works for both sector as well as non-sector bmarks
+ * because we calculate the min diff and iterate to the end
+ * of the bmarks array keeping track of min diff
+ */
for (i = 0; i < fcb_log->fl_bset.lfs_size; i++) {
bmark = &fcb_log->fl_bset.lfs_bmarks[i];
+#if MYNEWT_VAL(LOG_FCB)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_area) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#elif MYNEWT_VAL(LOG_FCB2)
+ if (!fcb_log->fl_bset.lfs_bmarks[i].lfb_entry.fe_range) {
+ /* Previous closest bookmark is the closest one */
+ break;
+ }
+#endif
if (bmark->lfb_index <= index) {
diff = index - bmark->lfb_index;
- if (diff < min_diff) {
- min_diff = diff;
+ if (diff < *min_diff) {
+ *min_diff = diff;
closest = bmark;
+ MODLOG_INFO(LOG_MODULE_DEFAULT, "index: %u, closest bmark idx:
%u, \n",
Review Comment:
`MODLOG_INFO` for this seems excessive
##########
sys/log/full/src/log_fcb.c:
##########
@@ -108,6 +110,7 @@ fcb_walk_back_find_start(struct fcb *fcb, struct log *log,
struct log_offset *lo
* ignored; the "index" field is used instead.
*
* XXX: We should rename "timestamp" or make it an actual timestamp.
+ * If bmark is found, fill up min_diff.
Review Comment:
It's not clear without studding code what min_diff means
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
