You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@mynewt.apache.org by "vrahane (via GitHub)" <gi...@apache.org> on 2025/03/27 20:38:19 UTC
[PR] sys/log, fs/fcb: Add support for trailers per entry in logs, fcb and fcb2 [mynewt-core]
vrahane opened a new pull request, #3400:
URL: https://github.com/apache/mynewt-core/pull/3400
- Add syscfgs for controlling the functionality along with callbacks
- This is an optional feature enabled only if LOG_FLAGS_TRAILER_SUPPORT: 1
Note: This is a replacement for the number of entries PR, https://github.com/apache/mynewt-core/pull/3168 and most of the comments should be pre-handled in this code.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076437891
##########
sys/log/full/src/log_cbmem.c:
##########
@@ -40,21 +51,61 @@ log_cbmem_append_body(struct log *log, const struct log_entry_hdr *hdr,
.flat_buf = body,
.flat_len = body_len,
},
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ {
+ .flat_buf = trailer,
+ .flat_len = 0
+ },
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
},
- .count = 3,
+ .count = 5,
+#else
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
+ },
+ .count = 4,
+#endif
};
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
+ sg->entries[1].flat_len = LOG_IMG_HASHLEN;
+ }
+
+ if (hdr->ue_flags & LOG_FLAGS_TRAILER_SUPPORT) {
+ rc = log_trailer_append(log, trailer, &trailer_len, NULL, NULL);
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2075277611
##########
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)
Review Comment:
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.
##########
sys/log/full/syscfg.yml:
##########
@@ -196,6 +202,10 @@ syscfg.defs:
restrictions:
- LOG_FCB_BOOKMARKS
- LOG_FCB
+ LOG_MAX_TRAILER_LEN:
+ description: >
+ Maximum length of trailer that can be appended to a log entry
+ value: "LF_MAX_ALIGN * 3"
Review Comment:
I'd prefer that this is just a number, not something that is actually an injected code.
##########
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;
Review Comment:
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?
##########
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) {
Review Comment:
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?
##########
sys/log/full/include/log/log.h:
##########
@@ -531,11 +660,10 @@ int log_read_hdr(struct log *log, const void *dptr, struct log_entry_hdr *hdr);
* @brief Reads the header length
*
* @param hdr Ptr to the header
- *
+ *
* @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);
Review Comment:
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.
##########
sys/log/full/src/log_cbmem.c:
##########
@@ -40,21 +51,61 @@ log_cbmem_append_body(struct log *log, const struct log_entry_hdr *hdr,
.flat_buf = body,
.flat_len = body_len,
},
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ {
+ .flat_buf = trailer,
+ .flat_len = 0
+ },
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
},
- .count = 3,
+ .count = 5,
+#else
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
+ },
+ .count = 4,
+#endif
};
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
+ sg->entries[1].flat_len = LOG_IMG_HASHLEN;
+ }
+
+ if (hdr->ue_flags & LOG_FLAGS_TRAILER_SUPPORT) {
+ rc = log_trailer_append(log, trailer, &trailer_len, NULL, NULL);
Review Comment:
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.
##########
sys/log/full/include/log/log.h:
##########
@@ -204,6 +309,16 @@ STATS_SECT_END
#define LOG_STATS_INCN(log, name, cnt)
#endif
+/* Trailer support callbacks */
+struct log_trailer_handler {
+ log_trailer_len_func_t *log_trailer_len;
+ log_trailer_data_len_func_t *log_trailer_data_len;
+ log_trailer_append_func_t *log_trailer_append;
+ 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;
+};
Review Comment:
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...
##########
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
Review Comment:
Description is not correct.
##########
sys/log/full/include/log/log.h:
##########
@@ -131,25 +213,48 @@ struct log_handler {
/* Image hash length to be looged */
#define LOG_IMG_HASHLEN 4
-/* Flags used to indicate type of data in reserved payload*/
-#define LOG_FLAGS_IMG_HASH (1 << 0)
+/* Flags used to indicate type of data in reserved payload */
+#define LOG_FLAGS_IMG_HASH (1 << 0)
+#define LOG_FLAGS_TRAILER_SUPPORT (1 << 1)
#if MYNEWT_VAL(LOG_VERSION) == 3
struct log_entry_hdr {
int64_t ue_ts;
uint32_t ue_index;
uint8_t ue_module;
uint8_t ue_level;
- uint8_t ue_etype:4;
- uint8_t ue_flags:4;
+ uint8_t ue_etype : 4;
+ uint8_t ue_flags : 4;
uint8_t ue_imghash[4];
-}__attribute__((__packed__));
+} __attribute__((__packed__));
#else
#error "Unsupported log version"
#endif
#define LOG_BASE_ENTRY_HDR_SIZE (15)
+/* Assume the flash alignment requirement is no stricter than 32. */
+#define LOG_FCB_MAX_ALIGN 32
+#define LOG_FCB2_MAX_ALIGN 32
+
+
+#if MYNEWT_VAL(LOG_FCB2)
+#define LF_MAX_ALIGN LOG_FCB2_MAX_ALIGN
+#else
+#define LF_MAX_ALIGN LOG_FCB_MAX_ALIGN
+#endif
+
+#define LOG_FCB_EXT_HDR_SIZE LOG_BASE_ENTRY_HDR_SIZE + LOG_IMG_HASHLEN + \
+ LF_MAX_ALIGN
+
+#ifndef LOG_FCB_FLAT_BUF_SIZE
+/* 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
Review Comment:
I don't really understand the logic behind above calculations and magic values.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2082380287
##########
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)
Review Comment:
done
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076102170
##########
sys/log/full/src/log_cbmem.c:
##########
@@ -40,21 +51,61 @@ log_cbmem_append_body(struct log *log, const struct log_entry_hdr *hdr,
.flat_buf = body,
.flat_len = body_len,
},
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ {
+ .flat_buf = trailer,
+ .flat_len = 0
+ },
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
},
- .count = 3,
+ .count = 5,
+#else
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
+ },
+ .count = 4,
+#endif
};
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
+ sg->entries[1].flat_len = LOG_IMG_HASHLEN;
+ }
+
+ if (hdr->ue_flags & LOG_FLAGS_TRAILER_SUPPORT) {
+ rc = log_trailer_append(log, trailer, &trailer_len, NULL, NULL);
Review Comment:
That's a good catch. Thank you. I will check the existence of `log->l_th` to set the flag.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2164897042
##########
sys/log/full/src/log_fcb.c:
##########
@@ -324,20 +331,15 @@ log_fcb_start_append(struct log *log, int len, struct fcb_entry *loc)
return (rc);
}
-/**
- * Calculates the number of message body bytes that should be included after
- * the entry header in the first write. Inclusion of body bytes is necessary
- * to satisfy the flash hardware's write alignment restrictions.
- */
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)
Review Comment:
done
##########
sys/log/full/include/log/log.h:
##########
@@ -218,6 +272,21 @@ struct log {
#endif
#if MYNEWT_VAL(LOG_STATS)
STATS_SECT_DECL(logs) l_stats;
+#endif
+ /* Custom log init callback to be called by the last hdr
+ * read function to read custom data from log entries
+ * at init
+ */
+ log_walk_func_t l_init_cb;
Review Comment:
done
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#issuecomment-2867688199
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."
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2164911931
##########
sys/log/full/include/log/log.h:
##########
@@ -218,6 +272,21 @@ struct log {
#endif
#if MYNEWT_VAL(LOG_STATS)
STATS_SECT_DECL(logs) l_stats;
+#endif
+ /* Custom log init callback to be called by the last hdr
+ * read function to read custom data from log entries
+ * at init
+ */
+ log_walk_func_t l_init_cb;
+
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER)
+ /* Log trailer support */
+ /* Void argument for callbacks registeration */
+ void *l_tr_arg;
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076092180
##########
sys/log/full/include/log/log.h:
##########
@@ -204,6 +309,16 @@ STATS_SECT_END
#define LOG_STATS_INCN(log, name, cnt)
#endif
+/* Trailer support callbacks */
+struct log_trailer_handler {
+ log_trailer_len_func_t *log_trailer_len;
+ log_trailer_data_len_func_t *log_trailer_data_len;
+ log_trailer_append_func_t *log_trailer_append;
+ 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;
+};
Review Comment:
- 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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076162700
##########
sys/log/full/syscfg.yml:
##########
@@ -196,6 +202,10 @@ syscfg.defs:
restrictions:
- LOG_FCB_BOOKMARKS
- LOG_FCB
+ LOG_MAX_TRAILER_LEN:
+ description: >
+ Maximum length of trailer that can be appended to a log entry
+ value: "LF_MAX_ALIGN * 3"
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2165161308
##########
sys/log/full/include/log/log.h:
##########
@@ -218,6 +272,21 @@ struct log {
#endif
#if MYNEWT_VAL(LOG_STATS)
STATS_SECT_DECL(logs) l_stats;
+#endif
+ /* Custom log init callback to be called by the last hdr
+ * read function to read custom data from log entries
+ * at init
+ */
+ log_walk_func_t l_init_cb;
+
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER)
+ /* Log trailer support */
+ /* Void argument for callbacks registeration */
+ void *l_tr_arg;
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs, fcb and fcb2 [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#issuecomment-2770947079
Note: I fixed the coding styling issues but the compliance check is showing stale information.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#issuecomment-2845912693
@andrzej-kaczmarek I have updated the PR. Please take a look.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076102170
##########
sys/log/full/src/log_cbmem.c:
##########
@@ -40,21 +51,61 @@ log_cbmem_append_body(struct log *log, const struct log_entry_hdr *hdr,
.flat_buf = body,
.flat_len = body_len,
},
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ {
+ .flat_buf = trailer,
+ .flat_len = 0
+ },
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
},
- .count = 3,
+ .count = 5,
+#else
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
+ },
+ .count = 4,
+#endif
};
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
+ sg->entries[1].flat_len = LOG_IMG_HASHLEN;
+ }
+
+ if (hdr->ue_flags & LOG_FLAGS_TRAILER_SUPPORT) {
+ rc = log_trailer_append(log, trailer, &trailer_len, NULL, NULL);
Review Comment:
That's a good catch. Thank you. I will check the existence of `log->l_th` to set the flag.
##########
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;
Review Comment:
That's a good catch. Thank you. I will check the existence of `log->l_th` to set the flag.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2078219252
##########
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;
Review Comment:
Fixed it.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#issuecomment-3006405259
@andrzej-kaczmarek I have made the changes. Please take a look.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb [mynewt-core]
Posted by "sjanc (via GitHub)" <gi...@apache.org>.
sjanc commented on PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#issuecomment-2962144319
> @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
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#issuecomment-2960412258
@sjanc Can you please take a look at the unit tests and style check, no sure whats going on there. Thank you.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2079567253
##########
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)
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2079565405
##########
sys/log/full/include/log/log.h:
##########
@@ -204,6 +309,16 @@ STATS_SECT_END
#define LOG_STATS_INCN(log, name, cnt)
#endif
+/* Trailer support callbacks */
+struct log_trailer_handler {
+ log_trailer_len_func_t *log_trailer_len;
+ log_trailer_data_len_func_t *log_trailer_data_len;
+ log_trailer_append_func_t *log_trailer_append;
+ 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;
+};
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2079595472
##########
sys/log/full/include/log/log.h:
##########
@@ -311,11 +301,23 @@ STATS_SECT_END
/* Trailer support callbacks */
struct log_trailer_handler {
+ /* Trailer length callback used to get the length of the trailer from provided
+ * log entry
+ */
log_trailer_len_func_t *log_trailer_len;
- log_trailer_data_len_func_t *log_trailer_data_len;
+ /* Trailer append callback used to append trailer to the log entry */
log_trailer_append_func_t *log_trailer_append;
+ /* Trailer process callback used to process trailer from the log entry
+ * which gets called at various places where the log entry gets read
+ */
log_process_trailer_func_t *log_process_trailer;
+ /* Trailer mbuf append callback used to append trailer to the log entry
+ * using the mbufs
+ */
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
+ */
Review Comment:
As I mentioned in some other comment, there's no "user data" in the callback registration API.
##########
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;
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2082353827
##########
sys/log/full/include/log/log.h:
##########
@@ -204,6 +309,16 @@ STATS_SECT_END
#define LOG_STATS_INCN(log, name, cnt)
#endif
+/* Trailer support callbacks */
+struct log_trailer_handler {
+ log_trailer_len_func_t *log_trailer_len;
+ log_trailer_data_len_func_t *log_trailer_data_len;
+ log_trailer_append_func_t *log_trailer_append;
+ 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;
+};
Review Comment:
Well, we do need to flush the data. So, do you have an alternate approach in mind ?
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2164050298
##########
sys/log/full/include/log/log.h:
##########
@@ -218,6 +272,21 @@ struct log {
#endif
#if MYNEWT_VAL(LOG_STATS)
STATS_SECT_DECL(logs) l_stats;
+#endif
+ /* Custom log init callback to be called by the last hdr
+ * read function to read custom data from log entries
+ * at init
+ */
+ log_walk_func_t l_init_cb;
+
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER)
+ /* Log trailer support */
+ /* Void argument for callbacks registeration */
+ void *l_tr_arg;
Review Comment:
this argument shall be passed to all callbacks, otherwise it doesn't make any sense
##########
sys/log/full/src/log_fcb.c:
##########
@@ -324,20 +331,15 @@ log_fcb_start_append(struct log *log, int len, struct fcb_entry *loc)
return (rc);
}
-/**
- * Calculates the number of message body bytes that should be included after
- * the entry header in the first write. Inclusion of body bytes is necessary
- * to satisfy the flash hardware's write alignment restrictions.
- */
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)
Review Comment:
should not be renamed since it apparently is still used when trailer support is disabled
##########
sys/log/full/include/log/log.h:
##########
@@ -218,6 +272,21 @@ struct log {
#endif
#if MYNEWT_VAL(LOG_STATS)
STATS_SECT_DECL(logs) l_stats;
+#endif
+ /* Custom log init callback to be called by the last hdr
+ * read function to read custom data from log entries
+ * at init
+ */
+ log_walk_func_t l_init_cb;
Review Comment:
make this also a syscfg and disabled by default
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2082106468
##########
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)
Review Comment:
ok, I'll check it in the #ifdef.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2079582407
##########
sys/log/full/include/log/log.h:
##########
@@ -131,25 +213,48 @@ struct log_handler {
/* Image hash length to be looged */
#define LOG_IMG_HASHLEN 4
-/* Flags used to indicate type of data in reserved payload*/
-#define LOG_FLAGS_IMG_HASH (1 << 0)
+/* Flags used to indicate type of data in reserved payload */
+#define LOG_FLAGS_IMG_HASH (1 << 0)
+#define LOG_FLAGS_TRAILER_SUPPORT (1 << 1)
#if MYNEWT_VAL(LOG_VERSION) == 3
struct log_entry_hdr {
int64_t ue_ts;
uint32_t ue_index;
uint8_t ue_module;
uint8_t ue_level;
- uint8_t ue_etype:4;
- uint8_t ue_flags:4;
+ uint8_t ue_etype : 4;
+ uint8_t ue_flags : 4;
uint8_t ue_imghash[4];
-}__attribute__((__packed__));
+} __attribute__((__packed__));
#else
#error "Unsupported log version"
#endif
#define LOG_BASE_ENTRY_HDR_SIZE (15)
+/* Assume the flash alignment requirement is no stricter than 32. */
+#define LOG_FCB_MAX_ALIGN 32
+#define LOG_FCB2_MAX_ALIGN 32
+
+
+#if MYNEWT_VAL(LOG_FCB2)
+#define LF_MAX_ALIGN LOG_FCB2_MAX_ALIGN
+#else
+#define LF_MAX_ALIGN LOG_FCB_MAX_ALIGN
+#endif
+
+#define LOG_FCB_EXT_HDR_SIZE LOG_BASE_ENTRY_HDR_SIZE + LOG_IMG_HASHLEN + \
+ LF_MAX_ALIGN
+
+#ifndef LOG_FCB_FLAT_BUF_SIZE
+/* 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
Review Comment:
I guess `LF_MAX_ALIGN * 3` is the same as *default* value for syscfg so if that one is changes, the whole calculation breaks.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076108267
##########
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) {
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076098173
##########
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)
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076192178
##########
sys/log/full/include/log/log.h:
##########
@@ -531,11 +660,10 @@ int log_read_hdr(struct log *log, const void *dptr, struct log_entry_hdr *hdr);
* @brief Reads the header length
*
* @param hdr Ptr to the header
- *
+ *
* @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);
Review Comment:
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:
https://github.com/apache/mynewt-core/pull/3168#discussion_r1642737620
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#issuecomment-2982124382
Ok, I think that fixed the tests. As for the updated style check, I think that needs to account for 79 characters in the header. It wants me to wrap s line which is not necessary.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076108267
##########
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) {
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#issuecomment-2856426646
@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 address some of the comments you mentioned. Please take a look. I have explained my reasoning for others.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2082358169
##########
sys/log/full/src/log_cbmem.c:
##########
@@ -40,21 +51,61 @@ log_cbmem_append_body(struct log *log, const struct log_entry_hdr *hdr,
.flat_buf = body,
.flat_len = body_len,
},
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ {
+ .flat_buf = trailer,
+ .flat_len = 0
+ },
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
},
- .count = 3,
+ .count = 5,
+#else
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
+ },
+ .count = 4,
+#endif
};
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
+ sg->entries[1].flat_len = LOG_IMG_HASHLEN;
+ }
+
+ if (hdr->ue_flags & LOG_FLAGS_TRAILER_SUPPORT) {
+ rc = log_trailer_append(log, trailer, &trailer_len, NULL, NULL);
Review Comment:
The `log_cbmem` implementation is what I am talking about. I tried to keep it consistent with that. That's how its implemented.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2078220077
##########
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
Review Comment:
Fixed it.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2079585469
##########
sys/log/full/syscfg.yml:
##########
@@ -196,6 +202,10 @@ syscfg.defs:
restrictions:
- LOG_FCB_BOOKMARKS
- LOG_FCB
+ LOG_MAX_TRAILER_LEN:
+ description: >
+ Maximum length of trailer that can be appended to a log entry
+ value: "LF_MAX_ALIGN * 3"
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#issuecomment-2946825532
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
```
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs, fcb and fcb2 [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#issuecomment-2842043227
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2079569218
##########
sys/log/full/src/log_cbmem.c:
##########
@@ -40,21 +51,61 @@ log_cbmem_append_body(struct log *log, const struct log_entry_hdr *hdr,
.flat_buf = body,
.flat_len = body_len,
},
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ {
+ .flat_buf = trailer,
+ .flat_len = 0
+ },
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
},
- .count = 3,
+ .count = 5,
+#else
+ {
+ .flat_buf = NULL,
+ .flat_len = 0,
+ },
+ },
+ .count = 4,
+#endif
};
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER_SUPPORT)
+ if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
+ sg->entries[1].flat_len = LOG_IMG_HASHLEN;
+ }
+
+ if (hdr->ue_flags & LOG_FLAGS_TRAILER_SUPPORT) {
+ rc = log_trailer_append(log, trailer, &trailer_len, NULL, NULL);
Review Comment:
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"?
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2167384644
##########
sys/log/full/include/log/log.h:
##########
@@ -218,6 +272,21 @@ struct log {
#endif
#if MYNEWT_VAL(LOG_STATS)
STATS_SECT_DECL(logs) l_stats;
+#endif
+ /* Custom log init callback to be called by the last hdr
+ * read function to read custom data from log entries
+ * at init
+ */
+ log_walk_func_t l_init_cb;
+
+#if MYNEWT_VAL(LOG_FLAGS_TRAILER)
+ /* Log trailer support */
+ /* Void argument for callbacks registeration */
+ void *l_tr_arg;
Review Comment:
done
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076108267
##########
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) {
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2079591435
##########
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) {
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2076153644
##########
sys/log/full/include/log/log.h:
##########
@@ -131,25 +213,48 @@ struct log_handler {
/* Image hash length to be looged */
#define LOG_IMG_HASHLEN 4
-/* Flags used to indicate type of data in reserved payload*/
-#define LOG_FLAGS_IMG_HASH (1 << 0)
+/* Flags used to indicate type of data in reserved payload */
+#define LOG_FLAGS_IMG_HASH (1 << 0)
+#define LOG_FLAGS_TRAILER_SUPPORT (1 << 1)
#if MYNEWT_VAL(LOG_VERSION) == 3
struct log_entry_hdr {
int64_t ue_ts;
uint32_t ue_index;
uint8_t ue_module;
uint8_t ue_level;
- uint8_t ue_etype:4;
- uint8_t ue_flags:4;
+ uint8_t ue_etype : 4;
+ uint8_t ue_flags : 4;
uint8_t ue_imghash[4];
-}__attribute__((__packed__));
+} __attribute__((__packed__));
#else
#error "Unsupported log version"
#endif
#define LOG_BASE_ENTRY_HDR_SIZE (15)
+/* Assume the flash alignment requirement is no stricter than 32. */
+#define LOG_FCB_MAX_ALIGN 32
+#define LOG_FCB2_MAX_ALIGN 32
+
+
+#if MYNEWT_VAL(LOG_FCB2)
+#define LF_MAX_ALIGN LOG_FCB2_MAX_ALIGN
+#else
+#define LF_MAX_ALIGN LOG_FCB_MAX_ALIGN
+#endif
+
+#define LOG_FCB_EXT_HDR_SIZE LOG_BASE_ENTRY_HDR_SIZE + LOG_IMG_HASHLEN + \
+ LF_MAX_ALIGN
+
+#ifndef LOG_FCB_FLAT_BUF_SIZE
+/* 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
Review Comment:
`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 )`.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log, fs/fcb: Add support for trailers per entry in logs for fcb, fcb2 and cbmem [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3400:
URL: https://github.com/apache/mynewt-core/pull/3400#discussion_r2082133081
##########
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;
Review Comment:
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.
--
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: notifications-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org