kasjer commented on code in PR #3365:
URL: https://github.com/apache/mynewt-core/pull/3365#discussion_r1958218868
##########
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:
Original code was had this line:
```c
bool en_sect_bmark = sect_bmark & bset->lfs_en_sect_bmarks;
```
which triggered error I mentioned if sector bookmarks were enabled and
non-sector bookmark was added. en_sec_bmark was not just variable that showed
if sector bookmarks are enabled for this log but there was `&` that made code
look in wrong place
New version does not have this feature
##########
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:
I did not marked code as incorrect. I just mentioned that there is no point
in updating pos variable hundred times when it can be set only once before loop.
If loop is finished by:
```c
pos = 1;
break;
```
`pos` is set to some value in range `<0, bset->lfs_size)`.
If it is NOT loop will end when `i` reaches `bset->lfs_size` and `pos` is
updated last time when `i == bset->lfs_size - 1` hence `pos` will be set to
`bset->lfs_size - 1 + 1`
##########
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:
For clarity, memcpy can't be used in such cases. It will not work for RiscV
and PIC32 and also for ARM when newlibc is selected as libc. It works due to
baselibc ARM implementation that choses to copy data from the end.
##########
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:
Origin code have just diff and min_diff was used while searching for correct
bookmark. Just diff seems enough.
##########
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:
I don't see how faking prototype for existing function would make code run
faster.
##########
sys/log/full/src/log_fcb_bmark.c:
##########
@@ -61,74 +112,246 @@ log_fcb_rotate_bmarks(struct fcb_log *fcb_log)
i--;
}
bset->lfs_size--;
- bset->lfs_next = bset->lfs_size;
}
}
void
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_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);
}
-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_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
+ */
+ 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_sect_bmark(struct fcb_log *fcb_log, struct fcb_entry *entry,
+ uint32_t index, 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_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 (bset->lfs_cap == 0) {
- return;
+ 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));
}
Review Comment:
Adding vert first bookmark (lfs_size == 0) ends up crashing.
##########
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:
As mentioned before when logs are not in all sectors yet because log was
never full. Some entries of bookmark table will not be filled. There will be
number of empty entries between sector bookmarks and non-sector bookmarks.
While lfs_size will have correct number of bookmarks (sum of both) loop will
never check non-sector bookmarks if there is gap
##########
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:
It would be nice to know how to chose bookmark array size and that is not
documented anywhere. Previously user code would just pass array and number of
elements. Now it seems that if values are not as expected (which are not clear
how it must be calculated) it will break something.
##########
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:
Let me explain what your code does with non sector bookmarks (variable name
already reflect name changes):
1. initially `lfs_non_sect_size = 0`, `lfs_next_non_sect = 0`
2. code calls `log_fcb_add_bmark(..., false);`
3. `pos = 0`
4. call to `log_fcb_replace_non_sect_bmark()` with `lfs_next_non_sect == 0`
5. non-sector bookmark is set at lfs_next_non_sect position which is 0
(starting from fcb_log->fl_fcb.f_sector_cnt)
6. for first non-sector bookmark code executes `bset->lfs_next_non_sect =
pos;` which stays at 0
7. `lfs_non_sect_size` is incremented to 1
8. `lfs_size` is also incremented (it keeps sector and non-sector number)
9. code calls `log_fcb_add_bmark(..., false);` with different index
10. call to `log_fcb_replace_non_sect_bmark()` with `lfs_next_non_sect == 0`
replaces previous bookmark even though there was space for new one
11. this time only those increments happen: `lfs_non_sect_size++; lfs_size++`
12. now depending on what is configured for `LOG_FCB_NUM_ABS_BMARKS` several
times same bookmark is overwritten.
13. Only after `bset->lfs_non_sect_size` reaches `LOG_FCB_NUM_ABS_BMARKS`
lfs_next_non_sect starts to be modified and additional bookmarks are actually
written.
##########
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:
I don't understand confusion. Two new syscfg valuas are added one
`LOG_FCB_NUM_ABS_BMARKS` and `LOG_FCB_SECTOR_BOOKMARKS`. why not use _BOOKMARKS
in all places?
##########
sys/log/full/src/log_fcb2.c:
##########
@@ -47,14 +47,16 @@ 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.
Review Comment:
This does not explain for the difference is. Difference between what?
It says that if bookmark is not there it will be -1. But if it's there what
is the meaning of this variable?
--
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]
