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-unsubscr...@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
