sys/log, fs/fcb: Add support for trailers per entry in logs for fcb by vrahane · Pull Request #3400 · apache/mynewt-core

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

    }
+
+    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

 #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


+    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

 */
-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


+    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.

sjanc · 2025-06-11T10:32:53Z

@sjanc Can you please take a look at the unit tests and style check, no sure whats going on there. Thank you.

I think there are some bugs related to uninitialized memory being used (and thus behavior is random and depends on toolchain etc), from very quick look eg log_register() is not initializing newly added fields, leaving those pointer dangling

vrahane · 2025-06-17T23:40:45Z

@sjanc Ok, I think that fixed the tests. As for the updated style check, I think that needs to account for 79 characters in the prototype. It wants me to unwrap lines which is wrong.

andrzej-kaczmarek · 2025-06-24T13:44:23Z

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

+     * read function to read custom data from log entries
+     * at init
+     */
+    log_walk_func_t l_init_cb;


make this also a syscfg and disabled by default

andrzej-kaczmarek · 2025-06-24T13:44:47Z

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

+#if MYNEWT_VAL(LOG_FLAGS_TRAILER)
+    /* Log trailer support */
+    /* Void argument for callbacks registeration */
+    void *l_tr_arg;


this argument shall be passed to all callbacks, otherwise it doesn't make any sense

So, it is part of the log structure. So, all callbacks already have access to it. I can add it if you want but its just two copies of the same pointer.

the whole point of user data for callbacks is that it's passed as an argument to each callback. application code (e.g. callbacks) should not access members of log structure directly.

andrzej-kaczmarek · 2025-06-24T13:45:34Z

sys/log/full/src/log_fcb.c

- */
 static int
-log_fcb_hdr_body_bytes(uint8_t align, uint8_t hdr_len)
+log_fcb_hdr_trailer_bytes(uint16_t align, uint16_t len)


should not be renamed since it apparently is still used when trailer support is disabled

…at boot up

vrahane · 2025-06-25T22:20:03Z

@andrzej-kaczmarek I have made the changes. Please take a look.

- 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.

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

vrahane requested review from andrzej-kaczmarek, kasjer and sjanc 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 force-pushed the vipul/logging_num_entries_callbacks branch 2 times, most recently from d4ab8d2 to e24ac42 Compare June 17, 2025 21:40

github-actions bot added size/m and removed size/l labels Jun 17, 2025

vrahane force-pushed the vipul/logging_num_entries_callbacks branch 4 times, most recently from 8d895da to 5dc2c60 Compare June 17, 2025 22:05

github-actions bot added size/l and removed size/m labels Jun 17, 2025

vrahane force-pushed the vipul/logging_num_entries_callbacks branch 4 times, most recently from 1932305 to cf6ea41 Compare June 17, 2025 23:03

vrahane force-pushed the vipul/logging_num_entries_callbacks branch from cf6ea41 to 5e4ebee Compare June 18, 2025 00:31

andrzej-kaczmarek requested changes Jun 24, 2025

View reviewed changes

vrahane force-pushed the vipul/logging_num_entries_callbacks branch 2 times, most recently from 24a803a to 346b663 Compare June 24, 2025 21:02

vrahane requested a review from andrzej-kaczmarek June 24, 2025 21:35

vrahane force-pushed the vipul/logging_num_entries_callbacks branch from 346b663 to fec8584 Compare June 25, 2025 18:42

sys/log: Add init_cb to be called when most recent log entry is read …

762b99e

…at boot up

vrahane force-pushed the vipul/logging_num_entries_callbacks branch from fec8584 to 307914d Compare June 25, 2025 22:08

vrahane force-pushed the vipul/logging_num_entries_callbacks branch from 307914d to b8d6178 Compare June 26, 2025 06:23

andrzej-kaczmarek approved these changes Jun 26, 2025

View reviewed changes

vrahane merged commit f565f06 into apache:master Jun 26, 2025
20 of 21 checks passed

vrahane deleted the vipul/logging_num_entries_callbacks branch June 26, 2025 16:46

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!

Uh oh!

sjanc commented Jun 11, 2025

Uh oh!

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

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

vrahane commented Jun 17, 2025 •

edited

Loading