sys/log, fs/fcb: Add support for trailers per entry in logs for fcb #3400

vrahane · 2025-03-27T20:38:17Z

Trailer callbacks can be enabled using
LOG_FLAGS_TRAILER_SUPPORT.
This is needed since we cannot update the log header
without breaking backwards compatibility
Adding a trailer keeps the log header in place and the entry
intact. On an upgrade or downgrade, log entries are still
readable.
Trailer callbacks can be registered using log_trailer_cbs_register().
Trailer data is managed by the callbacks
Add trailer buffer append/free callback
Add reset_data callback for trailer data
Allocate trailer buffer in log_append_prepare() and
free the trailer buffer with log_trailer_free() in
log.c. Both of them being callbacks to be implemented
by the user.
Add 2 byte trailer length field after trailer at
the end of the log entry.
Note: This is a replacement for the number of entries PR, sys/log: Add number of entries support in log #3168 and most of the comments should be pre-handled in this code. the trailer can be implemented by the user of this feature as they want privately only keeping the trailer handling code upstream.

vrahane · 2025-04-02T00:08:26Z

Note: I fixed the coding stye issues but the compliance check is showing stale information.

andrzej-kaczmarek · 2025-04-30T13:44:44Z

Please cleanup this PR so there are no commits that fix code added in other commits - it makes it more difficult to review.

Please also add some more verbose commit message that explains this feature since it's not a trivial fix.

vrahane · 2025-05-01T22:26:54Z

@andrzej-kaczmarek I have updated the PR. Please take a look.

andrzej-kaczmarek

It's quite hard to understand how this code should work and how it should be used since there's not much description for the new APIs and the usage of various callbacks is inconsistent. Also there are a lot of unrelated changes which makes it even harder to figure out which parts are relevant.

My general understanding is that it was supposed to work smth like:
For each new entry a callback is called so application can append an arbitrary data to a log entry. This data is effectively appended as a part of log body so there's a flag indicating presence of a trailer so the data can be later restored.

But it doesn't seem to do that, e.g. the flag is set for each entry regardless of the presence of the trailer, there's no way to read the trailer data from the log except for one single case handled in the code and so on. See inline comments for more details.

andrzej-kaczmarek · 2025-05-06T11:35:20Z

sys/log/full/include/log/log.h

+    log_process_trailer_func_t *log_process_trailer;
+    log_trailer_mbuf_append_func_t *log_trailer_mbuf_append;
+    log_trailer_reset_data_func_t *log_trailer_reset_data;
+};


I don't think anyone can figure out how to use these callbacks.

What is the difference between trailer_len and trailer_data_len? The latter doesn't seem to be used anywhere.
What is the purpose of trailer_reset_data? What is trailer data and how it's managed?
How the application should handle append callbacks?

and so on...

So, earlier I had l_trailer_arg which you wanted me to make private so, I moved that out. Now, there needs to be a way to reset that trailer data which is what trailer_reset_data allows. If you have any other way to perform a reset, please let me know. This is called from log_flush.

trailer_len helps user of the library to get the length of the trailer as the name suggest.

trailer_data_len helps user of the library get the length of the trailer data. This was used earlier before I got rid of l_trailer_arg but now since the callbacks take care of the trailer data. I can remove it.

I think what you have in mind in this context is the "user data" that was supposed to be provided when registering callbacks. In that context, some kind of flush callback probably would make sense, but there's no "user data" in this PR so not sure what you want to reset.

Well, we do need to flush the data. So, do you have an alternate approach in mind ?

andrzej-kaczmarek · 2025-05-06T11:36:52Z

sys/log/full/src/log_cbmem.c

@@ -88,9 +140,21 @@ log_cbmem_append_mbuf_body(struct log *log, const struct log_entry_hdr *hdr,
    if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
        sg.entries[1].flat_len = LOG_IMG_HASHLEN;
    }
+
+    if (hdr->ue_flags & LOG_FLAGS_TRAILER_SUPPORT) {
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)


Not sure why you check flags and have #ifdef since your code will always set that flag for each new entry if the feature is enabled.

The reason is I don't want to impose the addition of extra memory thats needed for the implementation of the function to someone who is not using the trailer support.

My point is, the LOG_FLAGS_TRAILER_SUPPORT only makes sens if trailer support is enabled so checking it outside #ifdef doesn't make any sense.

ok, I'll check it in the #ifdef.

andrzej-kaczmarek · 2025-05-06T12:01:38Z

sys/log/full/src/log.c

@@ -571,6 +691,10 @@ log_append_prepare(struct log *log, uint8_t module, uint8_t level,
    }
 #endif

+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+    ue->ue_flags |= LOG_FLAGS_TRAILER_SUPPORT;


Not sure what is the purpose of this flag. I assume this should be an indication whether trailer is appended to log entry or not, but it's set for each entry even in case application decides not to append a trailer. How can application figure out if an entry has a trailer when log is read?

That's a good catch. Thank you. I will check the existence of log->l_th to set the flag.

andrzej-kaczmarek · 2025-05-06T12:14:26Z

sys/log/full/src/log_cbmem.c

+    }
+
+    if (hdr->ue_flags & LOG_FLAGS_TRAILER_SUPPORT) {
+        rc = log_trailer_append(log, trailer, &trailer_len, NULL, NULL);


Usage of log_trailer_append is inconsistent in logs implementation. Here it seems that the underlying callback should write to trailer buffer since it's later written via sg struct. In FCB implementation the same parameter is not used so I assume callback should write data directly to FCB.

I see what you mean but the way cbmem is used we actually use the scatter gather implementation to add optional data. For example: the image hash is also added as one entry to the scatter gather array. So, that's what I did for the trailer as well. There is no flat buffer implementation for logging to cbmem, so, we do not really need to implement it that way. Unless you feel otherwise, I would like to keep it this way for the sake of simplicity and consistency.

The same append callback expects the application to either write the data to the log or return it in provided buffer depending on the log type - how is that "for the sake of simplicity and consistency"?

The log_cbmem implementation is what I am talking about. I tried to keep it consistent with that. That's how its implemented.

andrzej-kaczmarek · 2025-05-06T12:15:32Z

sys/log/full/syscfg.yml

+    LOG_MAX_TRAILER_LEN:
+        description: >
+            Maximum length of trailer that can be appended to a log entry
+        value: "LF_MAX_ALIGN * 3"


I'd prefer that this is just a number, not something that is actually an injected code.

If the max alignment changes, the max trailer length is going to get affected because alignment plays a big role in fitting the trailer, the implementation of a trailer can be a TLV, a reverse TLV or static data but in order for it to be read successfully from the end, alignment matters. I would prefer to keep it this way if you don't feel strongly.

The alignment should be handle by the log framework and I should not really need to care about alignment when setting this number, especially that different log storage can have different alignment requirements.

andrzej-kaczmarek · 2025-05-06T12:16:29Z

sys/log/full/syscfg.yml

@@ -63,6 +63,12 @@ syscfg.defs:
            1 - enable.
        value: 0

+    LOG_FLAGS_TRAILER_SUPPORT:
+        description: >
+            Enable logging TLV with custom data types in every log entry


Description is not correct.

andrzej-kaczmarek · 2025-05-06T12:16:58Z

sys/log/full/include/log/log.h

 * @return Length of the header
 */
-uint16_t
-log_hdr_len(const struct log_entry_hdr *hdr);
+uint16_t log_hdr_len(const struct log_entry_hdr *hdr);


This PR has a lot of unrelated code changes (e.g. code style changes) which should be removed. You can create separate PR for those changes.

The other changes namely adding the name of the variable for the prototypes:
Example: https://github.com/apache/mynewt-core/pull/3400/files#diff-3fc9ab99b4a74c4fe2a0c49448e087a70997d395c5c16f99ce6fb1d923b72e4cR164
was actually requested by @kasjer here:
#3168 (comment)
I can remove the changes. As mentioned in the comment even I agree, keeping code consistent is better. Clean up should be done as a separate task.

andrzej-kaczmarek · 2025-05-06T12:24:13Z

sys/log/full/include/log/log.h

+/* Assuming the trailer fits in this, an arbitrary value */
+#define LOG_FCB_FLAT_BUF_SIZE (LOG_FCB_EXT_HDR_SIZE > LF_MAX_ALIGN * 3) ? \
+    LOG_FCB_EXT_HDR_SIZE : LF_MAX_ALIGN * 3
+#endif


I don't really understand the logic behind above calculations and magic values.

LOG_FCB_FLAT_BUF_SIZE is used as a constant to indicate how big a buffer needs to be to read the 15 byte log base header + the image hash length. The above calculation allows us to use the same buffer to read the trailer.
So, if extended log header > max trailer length, the length of the buffer can be set to that of the extended log header or it needs to be set to the max trailer length which is ( LF_MAX_ALIGN * 3 ).

I guess LF_MAX_ALIGN * 3 is the same as default value for syscfg so if that one is changes, the whole calculation breaks.

andrzej-kaczmarek · 2025-05-06T12:36:09Z

sys/log/full/src/log.c

@@ -320,21 +324,41 @@ log_read_hdr_walk(struct log *log, struct log_offset *log_offset, const void *dp
        }
    }

+    if (arg->hdr->ue_flags & LOG_FLAGS_TRAILER_SUPPORT && log->l_th) {
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+        if (log->l_th->log_process_trailer) {


This is the only place where process_trailer callback is called. How can application actually access the trailer data when reading the log in a generic way?

That is exactly why l_trailer_arg was there. If an application needs to process the data, they will have to implement the process callback in which the data will get read during the correct function calls in the logging code. If you want to read data, a separate callback will have to be implemented which is not really necessary for the implementation right now as the data is handled by the implemented callbacks, so, an application registering those callbacks already has access to the data.

I don't really understand what you are trying to do here.

If you call log_walk to read the log there should be a way for the application to get the trailer data for each entry. This can be either by a callback which log_walk should call for each entry or (I think preferably) there should be an API that application can use to read that trailer data.

vrahane · 2025-05-06T23:06:06Z

@andrzej-kaczmarek I understand the concerns, while I can address some of them, I have added additional comments to make the log_trailer_handler{} little more easier to understand. I have also addressed some of the comments you mentioned. Please take a look. I have explained my reasoning for others.

andrzej-kaczmarek

Please remove unrelated code from this PR to make it easier to review.
The fix commits should be squashed with their corresponding commits before pushing new version of PR - it's possible to diff PR revisions using GitHub easily if needed.

I think the major issue with this change is the convoluted flow for the callbacks. It seems like the application has to take care of lots of details it should not really care about, e.g. underlying log storage used (#3400 (comment)), log storage alignment (#3400 (comment) #3400 (comment)), restoring trailer data (#3400 (comment)), but actually there's no way for anyone to figure this out without going deep dive into the code.

The idea seems quite simple though, smth like:

the append callback (or equivalent) is called for each log entry so application can provide the trailer data; the log framework takes care of writing this to actual log storage
some callback or API is used during log_walk to provide trailer data to the application; the log framework takes care of reading the trailer data from the log entry if available

That should be really sufficient to do the same in a much easier way, unless there's something I'm missing here.

andrzej-kaczmarek · 2025-05-08T12:10:05Z

sys/log/full/src/log.c

@@ -692,7 +679,9 @@ log_append_prepare(struct log *log, uint8_t module, uint8_t level,
 #endif

 #if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
-    ue->ue_flags |= LOG_FLAGS_TRAILER_SUPPORT;
+    if (log->l_th) {
+        ue->ue_flags |= LOG_FLAGS_TRAILER_SUPPORT;


This has the same issue as previous code. If application registers callbacks, the flag will be set for each new log entry, even if the application doesn't append any trailer data via the callback.

If the application has registered trailer callbacks for that particular log, that means the application knows how to handle the trailer data. I really don't get your point. Please provide an alternate approach.

andrzej-kaczmarek · 2025-05-08T12:10:59Z

sys/log/full/include/log/log.h

    log_trailer_mbuf_append_func_t *log_trailer_mbuf_append;
+    /* Trailer reset data callback used to reset the trailer data
+     * per log which is defined by the application registering these callbacks
+     */


As I mentioned in some other comment, there's no "user data" in the callback registration API.

vrahane · 2025-05-09T19:29:08Z

I addressed review comment based on:

"Please remove unrelated code from this PR to make it easier to review.
The fix commits should be squashed with their corresponding commits before pushing new version of PR - it's possible to diff PR revisions using GitHub easily if needed."

- Trailer callbacks can be enabled using LOG_FLAGS_TRAILER_SUPPORT. - This is needed since we cannot update the log header without breaking backwards compatibility - Adding a trailer keeps the log header in place and the entry intact. On an upgrade or downgrade, log entries are still readable. - Trailer callbacks can be registered using log_trailer_cbs_register(). - Trailer data is managed by the callbacks - Add trailer buffer append/free callback - Add reset_data callback for trailer data - Allocate trailer buffer in log_append_prepare() and free the trailer buffer with log_trailer_free() in log.c. Both of them being callbacks to be implemented by the user. - Add 2 byte trailer length field after trailer at the end of the log entry.

vrahane · 2025-06-05T23:12:40Z

I cannot tell why the tests are failing on CI, they are succeeding locally:

Passed tests: [sys/log/full/selftest/align4 sys/log/full/selftest/fcb2_bookmarks sys/log/full/selftest/perlogidx sys/log/full/selftest/align2 sys/log/full/selftest/align1_imghash sys/log/full/selftest/fcb2_align1 sys/log/full/selftest/align8 sys/log/full/selftest/fcb_bookmarks_mixed sys/log/full/selftest/fcb2_watermarks sys/log/full/selftest/fcb_bookmarks sys/log/modlog/selftest sys/log/full/selftest/align1]
All tests passed

github-actions bot added the size/l label Mar 27, 2025

vrahane requested review from sjanc, andrzej-kaczmarek and kasjer March 27, 2025 20:38

vrahane force-pushed the vipul/logging_num_entries_callbacks branch 2 times, most recently from 6e0dae2 to 64aa51e Compare March 27, 2025 23:34

vrahane marked this pull request as draft March 27, 2025 23:35

vrahane changed the title ~~sys/log, fs/fcb: Add support for trailers per entry in logs, fcb and fcb2~~ [DNM - WIP] sys/log, fs/fcb: Add support for trailers per entry in logs, fcb and fcb2 Mar 27, 2025

vrahane force-pushed the vipul/logging_num_entries_callbacks branch 2 times, most recently from e0edfb5 to ee78f5f Compare April 1, 2025 23:08

github-actions bot added size/xl and removed size/l labels Apr 1, 2025

vrahane force-pushed the vipul/logging_num_entries_callbacks branch from ee78f5f to d510c97 Compare April 1, 2025 23:37

vrahane changed the title ~~[DNM - WIP] sys/log, fs/fcb: Add support for trailers per entry in logs, fcb and fcb2~~ sys/log, fs/fcb: Add support for trailers per entry in logs, fcb and fcb2 Apr 1, 2025

vrahane marked this pull request as ready for review April 1, 2025 23:52

vrahane force-pushed the vipul/logging_num_entries_callbacks branch 2 times, most recently from aa363f2 to de038c4 Compare April 7, 2025 17:29

github-actions bot added size/l and removed size/xl labels Apr 25, 2025

vrahane force-pushed the vipul/logging_num_entries_callbacks branch from f90941d to 080097a Compare April 29, 2025 21:37

vrahane force-pushed the vipul/logging_num_entries_callbacks branch 2 times, most recently from 6f99118 to b594e7f Compare April 30, 2025 19:08

vrahane changed the title ~~sys/log, fs/fcb: Add support for trailers per entry in logs, fcb and fcb2~~ sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem Apr 30, 2025

vrahane added feature New feature request LOGS labels Apr 30, 2025

andrzej-kaczmarek requested changes May 6, 2025

View reviewed changes

vrahane requested a review from andrzej-kaczmarek May 7, 2025 18:10

andrzej-kaczmarek requested changes May 8, 2025

View reviewed changes

vrahane force-pushed the vipul/logging_num_entries_callbacks branch from bda292d to c0e4fa9 Compare May 9, 2025 19:15

vrahane force-pushed the vipul/logging_num_entries_callbacks branch from c0e4fa9 to 303ace1 Compare May 29, 2025 21:03

vrahane changed the title ~~sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem~~ sys/log, fs/fcb: Add support for trailers per entry in logs for fcb May 29, 2025

vrahane force-pushed the vipul/logging_num_entries_callbacks branch 7 times, most recently from 8c4fd39 to eec93eb Compare June 5, 2025 22:28

vrahane requested a review from andrzej-kaczmarek June 5, 2025 22:39

vrahane force-pushed the vipul/logging_num_entries_callbacks branch from eec93eb to 8254b21 Compare June 5, 2025 22:50

sys/log, fs/fcb: Add support for trailers per entry in logs for fcb #3400

Are you sure you want to change the base?

sys/log, fs/fcb: Add support for trailers per entry in logs for fcb #3400

Uh oh!

Conversation

vrahane commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrahane commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrzej-kaczmarek commented Apr 30, 2025

Uh oh!

vrahane commented May 1, 2025

Uh oh!

andrzej-kaczmarek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrahane May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrahane commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

vrahane commented Mar 27, 2025 •

edited

Loading

vrahane commented Apr 2, 2025 •

edited

Loading

vrahane May 6, 2025 •

edited

Loading

vrahane commented May 6, 2025 •

edited

Loading

andrzej-kaczmarek left a comment •

edited

Loading