You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by "vrahane (via GitHub)" <gi...@apache.org> on 2024/03/20 18:39:30 UTC
[PR] Add number of entries support in log and add log version 4 [mynewt-core]
vrahane opened a new pull request, #3168:
URL: https://github.com/apache/mynewt-core/pull/3168
Please check email on the devlist for more information:
- This adds support for logging number of entries since a given index of the log, it is optional, so only used when enabled
- The mynewt VAL for the same is `LOG_FLAGS_NUM_ENTRIES`
- mcumgr support will be in a follow on PR along with newtmgr support on the tool side
- unittests will be implemented in a follow on PR
- CLI command support is also added to help read the number of entries since a particular index on the CLI
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1669117154
##########
sys/log/full/include/log/log.h:
##########
@@ -129,25 +131,40 @@ 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_TLV_SUPPORT (1 << 1)
+
+#define LOG_TLV_NUM_ENTRIES (1 << 0)
#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__));
+ uint32_t ue_num_entries;
Review Comment:
Ok, I'll add a comment for the field.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#issuecomment-2163512939
@sjanc @andrzej-kaczmarek @kasjer Can you please start reviewing this ?
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1671188463
##########
sys/log/full/src/log_fcb.c:
##########
@@ -346,23 +350,80 @@ log_fcb_append_body(struct log *log, const struct log_entry_hdr *hdr,
if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
memcpy(buf + LOG_BASE_ENTRY_HDR_SIZE, hdr->ue_imghash, LOG_IMG_HASHLEN);
}
+
memcpy(buf + hdr_len, u8p, hdr_alignment);
rc = flash_area_write(loc.fe_area, loc.fe_data_off, buf, chunk_sz);
if (rc != 0) {
return rc;
}
+ loc.fe_data_off += chunk_sz;
+
/* Append the remainder of the message body. */
u8p += hdr_alignment;
body_len -= hdr_alignment;
- if (body_len > 0) {
- rc = flash_area_write(loc.fe_area, loc.fe_data_off + chunk_sz, u8p,
- body_len);
- if (rc != 0) {
- return rc;
+ if (hdr->ue_flags & LOG_FLAGS_TLV_SUPPORT) {
+ memset(buf, 0, sizeof(buf));
+ /* Calculate trailer alignment */
+ trailer_alignment = log_fcb_hdr_trailer_bytes(fcb->f_align, chunk_sz + body_len);
+ if (trailer_alignment > trailer_len) {
+ chunk_sz = trailer_len;
+ } else {
+ chunk_sz = trailer_alignment;
+ }
Review Comment:
This is not needed as trailer comes at the end after the body and the padding takes care of alignment issues, so the trailer can be read on its own.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1669256798
##########
sys/log/full/src/log.c:
##########
@@ -413,13 +538,21 @@ log_register(const char *name, struct log *log, const struct log_handler *lh,
}
}
- /* If this is a persisted log, read the index from its most recent entry.
- * We need to ensure the index of all subseqently written entries is
- * monotonically increasing.
- */
if (log->l_log->log_type == LOG_TYPE_STORAGE) {
rc = log_read_last_hdr(log, &hdr);
if (rc == 0) {
+ /* If the number of entries are not set in the last header,
+ * it was probably not supported when the entry was logged.
+ * Count number of entries in this specific case
+ */
+ hdr.ue_num_entries = 0;
Review Comment:
Yeah, it should not be set to 0 this way. I probably messed it up while squashing changes.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1669289583
##########
sys/log/full/include/log/log.h:
##########
@@ -97,6 +97,7 @@ typedef int (*lh_append_mbuf_body_func_t)(struct log *log,
typedef int (*lh_walk_func_t)(struct log *,
log_walk_func_t walk_func, struct log_offset *log_offset);
typedef int (*lh_flush_func_t)(struct log *);
+typedef uint16_t (*lh_read_entry_len_func_t)(struct log *, const void *dptr);
Review Comment:
Anyways, let's focus on this comment. You can re-review once I am done making changes.
##########
sys/log/full/include/log/log.h:
##########
@@ -97,6 +97,7 @@ typedef int (*lh_append_mbuf_body_func_t)(struct log *log,
typedef int (*lh_walk_func_t)(struct log *,
log_walk_func_t walk_func, struct log_offset *log_offset);
typedef int (*lh_flush_func_t)(struct log *);
+typedef uint16_t (*lh_read_entry_len_func_t)(struct log *, const void *dptr);
Review Comment:
Are you sure compatibility is not preserved ? Maybe it does not work on hardware you are using. Is it possible for you to try on the same hardware as mine ?
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1669286527
##########
sys/log/full/include/log/log.h:
##########
@@ -97,6 +97,7 @@ typedef int (*lh_append_mbuf_body_func_t)(struct log *log,
typedef int (*lh_walk_func_t)(struct log *,
log_walk_func_t walk_func, struct log_offset *log_offset);
typedef int (*lh_flush_func_t)(struct log *);
+typedef uint16_t (*lh_read_entry_len_func_t)(struct log *, const void *dptr);
Review Comment:
Are you sure compatibility is not preserved ? Maybe it does not work on hardware you are using. Is it possible for it to try on the same hardware as mine ?
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1669172300
##########
sys/log/full/src/log_fcb.c:
##########
@@ -346,23 +350,80 @@ log_fcb_append_body(struct log *log, const struct log_entry_hdr *hdr,
if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
memcpy(buf + LOG_BASE_ENTRY_HDR_SIZE, hdr->ue_imghash, LOG_IMG_HASHLEN);
}
+
memcpy(buf + hdr_len, u8p, hdr_alignment);
rc = flash_area_write(loc.fe_area, loc.fe_data_off, buf, chunk_sz);
if (rc != 0) {
return rc;
}
+ loc.fe_data_off += chunk_sz;
+
/* Append the remainder of the message body. */
u8p += hdr_alignment;
body_len -= hdr_alignment;
- if (body_len > 0) {
- rc = flash_area_write(loc.fe_area, loc.fe_data_off + chunk_sz, u8p,
- body_len);
- if (rc != 0) {
- return rc;
+ if (hdr->ue_flags & LOG_FLAGS_TLV_SUPPORT) {
+ memset(buf, 0, sizeof(buf));
+ /* Calculate trailer alignment */
+ trailer_alignment = log_fcb_hdr_trailer_bytes(fcb->f_align, chunk_sz + body_len);
+ if (trailer_alignment > trailer_len) {
+ chunk_sz = trailer_len;
+ } else {
+ chunk_sz = trailer_alignment;
+ }
+
+#if MYNEWT_VAL(LOG_FLAGS_TLV_SUPPORT) && MYNEWT_VAL(LOG_TLV_NUM_ENTRIES)
+ struct log_tlv tlv;
+ uint16_t offset = 0;
+ uint16_t padding = 0;
+
+ if (body_len > 0) {
+ padding = trailer_alignment ? fcb->f_align - trailer_alignment : 0;
+ rc = flash_area_write(loc.fe_area, loc.fe_data_off, u8p,
+ body_len - padding);
+ if (rc != 0) {
+ return rc;
+ }
+
+ loc.fe_data_off += body_len - padding;
+
+ u8p = u8p + body_len - padding;
+ memcpy(buf, u8p, padding);
+ offset = padding;
+ u8p = (void *)&tlv;
Review Comment:
No specific reason, fixing 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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1671225134
##########
sys/log/full/src/log_fcb2.c:
##########
@@ -221,18 +221,21 @@ static int
log_fcb2_append_body(struct log *log, const struct log_entry_hdr *hdr,
const void *body, int body_len)
{
- uint8_t buf[LOG_BASE_ENTRY_HDR_SIZE + LOG_IMG_HASHLEN +
- LOG_FCB2_MAX_ALIGN - 1];
+ uint8_t buf[LOG_BASE_ENTRY_HDR_SIZE + LOG_IMG_HASHLEN + sizeof(struct log_tlv)
+ + LOG_NUM_ENTRIES_SIZE + LOG_FCB2_MAX_ALIGN * 2 - 1];
Review Comment:
Fixing 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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log and add log version 4 [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#issuecomment-2023068161
Yes, that’s what I have been thinking as well. I think the log version 3 was added at the time to add support for flags. So, I should be able to just add a field to the same header. Haven’t gotten to this yet again. Will let you know after I try it again and will update this PR accordingly.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log and add log version 4 [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#issuecomment-2033312706
@andrzej-kaczmarek I did try this out, keeping the same log version works fine with upgrades but downgrades do not work as the header now has an extra field which the old firmware does not know about which is fine in a way. I am thinking of adding the num_entries field after the log entry body, that way downgrades will work fine as well. Another option which we spoke about internally was to read the logs at bootup and populate number of entries and keep incrementing in RAM which is doable but then there are corner cases to worry about. What are your thoughts on 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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log and add log version 4 [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#issuecomment-2019207200
If we want to make this change such that it can log various entries of different types, flags could be utilized but this header in itself won't be enough to make the log format generic enough, perhaps we could agree upon a log header/trailer format and define its functional aspect. I am open to making some changes to the way it works but do keep in mind we want to keep this change backwards compatible with log version 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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1643104211
##########
sys/log/full/include/log/log.h:
##########
@@ -129,25 +131,40 @@ 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_TLV_SUPPORT (1 << 1)
+
+#define LOG_TLV_NUM_ENTRIES (1 << 0)
#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__));
+ uint32_t ue_num_entries;
Review Comment:
Do you mean the prefix `ue` or just in general num_entries ? I do not know why `ue` was used historically. Number of entries gets passed around in the functions which is what it is used for mainly. The header itself in flash only has the base header. Image hash is sort of an extension.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#issuecomment-2173994167
> TLV implementation looks suspicious to me.
>
> Lets consider how log entry was stored in FCB
>
> [2 byte len] managed by fcb code
> [8 byte] time stamp
> [4 byte index] that would be what num_entreis is not due to global log indexing
> [1 byte] module
> [1 byte] level
> [1 byte] type/flags
> [n bytes] of log body
> [1 byte] crc managed by fcb
> Now it would look like this
>
> [2 byte len] managed by fcb code
> [8 byte] time stamp
> [4 byte index] that would be what num_entreis is not due to global log indexing
> [1 byte] module
> [1 byte] level
> [1 byte] type/flags with TLV bit set
> [n bytes] of log body
> [6 bytes] for TLV with num_entries
> [1 byte] crc managed by fcb
> Code assumes that TLV start 6 bytes from the end of the body which is true if this is the only TLV with known size.
> It there was more entries in TLV style possibly with dynamic size as is allowed by TLV there would be no way of finding place where all TLVs are.
That is a good point. There is a limitation in our current log implementation since the header itself does not contain the length and log entries are of variable length. The length of the log entry can only be known either from the fcb header or the cbmem header after it is written. For backwards/forwards compatibility this is important.
So, the TLVs have to start from the end of the actual body of the log entry.
>
> Maybe if TLVs were place before normal log body it would be more future proof
The way the code is structured and the log entry in flash is accessed if the log entry has to be backwards/forwards compatible the TLV cannot be stored before the entry. That is exactly what I was doing earlier but it breaks compatibility with old format which we do not want in case of repeated upgrades/downgrades.
Well, one way to mitigate this concern is to do the following:
- Have reverse TLVS which means this:
> [2 byte len] managed by fcb code
> [8 byte] time stamp
> [4 byte index] that would be what num_entries is not due to global log indexing
> [1 byte] module
> [1 byte] level
> [1 byte] type/flags
> [n bytes] of log body
> [4 bytes] for num_entries
> [2 bytes] for tag and length in TLV
> [1 byte] crc managed by fcb
```
So, what will happen in this case is while reading we would read at the end of the fcb entry, read the TLV and interpret the value and we can keep reading TLVs in a repeated fashion in that way so the dynamic nature of TLVs is preserved.
Let me know if that seems like a good 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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1643104211
##########
sys/log/full/include/log/log.h:
##########
@@ -129,25 +131,40 @@ 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_TLV_SUPPORT (1 << 1)
+
+#define LOG_TLV_NUM_ENTRIES (1 << 0)
#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__));
+ uint32_t ue_num_entries;
Review Comment:
Do you mean the prefix `ue` or just in general num_entries ? I do not know why `ue` was used historically. Number of entries gets passed around in the functions which is what it is used for mainly. The header itself in flash only has the base header. Image hash is sort of an extension and so is number of entries since the TLV is what holds it in flash.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "kasjer (via GitHub)" <gi...@apache.org>.
kasjer commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1643188030
##########
sys/log/full/include/log/log.h:
##########
@@ -129,25 +131,40 @@ 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_TLV_SUPPORT (1 << 1)
+
+#define LOG_TLV_NUM_ENTRIES (1 << 0)
#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__));
+ uint32_t ue_num_entries;
Review Comment:
I was not concern with eu_ prefix. If you just look at the structure this there is little chance that you can figure out what num_entries mean
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "kasjer (via GitHub)" <gi...@apache.org>.
kasjer commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1669155560
##########
sys/log/full/include/log/log.h:
##########
@@ -97,6 +97,7 @@ typedef int (*lh_append_mbuf_body_func_t)(struct log *log,
typedef int (*lh_walk_func_t)(struct log *,
log_walk_func_t walk_func, struct log_offset *log_offset);
typedef int (*lh_flush_func_t)(struct log *);
+typedef uint16_t (*lh_read_entry_len_func_t)(struct log *, const void *dptr);
Review Comment:
I leave it up to you. If you do, I will click `Resolve conversation` button so others can focus on other things. I looked at the code and tested this change and I don't see any improvement. Combability is not preserved and with the way TLV is done it will not be better in the future if more changes are needed. I can't approve this. Even if you fix all small things I pointed out I can't see any reason to add this new field let alone the way it is added. If you find someone else willing to approve so be 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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1643099478
##########
sys/log/full/include/log/log.h:
##########
@@ -97,6 +97,7 @@ typedef int (*lh_append_mbuf_body_func_t)(struct log *log,
typedef int (*lh_walk_func_t)(struct log *,
log_walk_func_t walk_func, struct log_offset *log_offset);
typedef int (*lh_flush_func_t)(struct log *);
+typedef uint16_t (*lh_read_entry_len_func_t)(struct log *, const void *dptr);
Review Comment:
It's consistent with the other prototypes. That's the reason I made it that way. For function pointers it does not matter what the name of the variable is anyways.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log and add log version 4 [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane closed pull request #3168: sys/log: Add number of entries support in log and add log version 4
URL: https://github.com/apache/mynewt-core/pull/3168
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log and add log version 4 [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#issuecomment-2019204070
The purpose of logging number of entries in the entry itself is that we do not have to walk all the entries to count the number of entries. For a large number of log entries, walking logs serially to find the end counting number of entries is little slow. So, this gives us a way to persist number of entries and retrieve number of entries whenever needed without having to walk the log.
This change is supposed to be backwards compatible since the base header is the same 15 bytes but somehow entries with log version 3 are not getting read correctly. I am still figuring out why that is happening. We wanted to open a PR with changes sooner than later and hence I thought the backwards compatibility fix could be added later on in a follow on PR. It does not require any format/log header changes as such.
The ifdefs can actually be removed to be similar to image hash. The only reason I kept them is to save us some stack space and code space.
TLV would be a good idea but wouldn't that require too many changes to the logging code ? I wanted to keep the change backwards compatible.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "kasjer (via GitHub)" <gi...@apache.org>.
kasjer commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1643186097
##########
sys/log/full/include/log/log.h:
##########
@@ -97,6 +97,7 @@ typedef int (*lh_append_mbuf_body_func_t)(struct log *log,
typedef int (*lh_walk_func_t)(struct log *,
log_walk_func_t walk_func, struct log_offset *log_offset);
typedef int (*lh_flush_func_t)(struct log *);
+typedef uint16_t (*lh_read_entry_len_func_t)(struct log *, const void *dptr);
Review Comment:
So you are saying that is consistent with inconsistency?
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "kasjer (via GitHub)" <gi...@apache.org>.
kasjer commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1669231053
##########
sys/log/full/src/log_fcb.c:
##########
@@ -446,11 +507,35 @@ log_fcb_append_mbuf_body(struct log *log, const struct log_entry_hdr *hdr,
}
loc.fe_data_off += LOG_IMG_HASHLEN;
}
+
rc = log_fcb_write_mbuf(&loc, om);
if (rc != 0) {
return rc;
}
+ if (hdr->ue_flags & LOG_FLAGS_TLV_SUPPORT) {
+#if MYNEWT_VAL(LOG_FLAGS_TLV_SUPPORT) && MYNEWT_VAL(LOG_TLV_NUM_ENTRIES)
+ struct log_tlv tlv;
+ tlv.tag = LOG_TLV_NUM_ENTRIES;
+ tlv.len = LOG_NUM_ENTRIES_SIZE;
+ /* Write TLV */
+ rc = flash_area_write(loc.fe_area, loc.fe_data_off, &tlv,
+ sizeof(struct log_tlv));
+ if (rc != 0) {
+ return rc;
+ }
+ loc.fe_data_off += sizeof(struct log_tlv);;
+ /* Write LOG_NUM_ENTRIES_SIZE bytes */
+ rc = flash_area_write(loc.fe_area, loc.fe_data_off,
Review Comment:
if flash alignment is > 4 it will not work at all in this form
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1671193716
##########
sys/log/full/src/log_fcb.c:
##########
@@ -274,19 +274,19 @@ log_fcb_start_append(struct log *log, int len, struct fcb_entry *loc)
}
/**
- * 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.
+ * Calculates the number of bytes that should be included after
+ * the entry header or trailer in the first write. Inclusion of bytes
+ * is necessary to satisfy the flash hardware's write alignment restrictions.
Review Comment:
Fixing 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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log and add log version 4 [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane closed pull request #3168: sys/log: Add number of entries support in log and add log version 4
URL: https://github.com/apache/mynewt-core/pull/3168
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "kasjer (via GitHub)" <gi...@apache.org>.
kasjer commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1642737620
##########
sys/log/full/include/log/log.h:
##########
@@ -97,6 +97,7 @@ typedef int (*lh_append_mbuf_body_func_t)(struct log *log,
typedef int (*lh_walk_func_t)(struct log *,
log_walk_func_t walk_func, struct log_offset *log_offset);
typedef int (*lh_flush_func_t)(struct log *);
+typedef uint16_t (*lh_read_entry_len_func_t)(struct log *, const void *dptr);
Review Comment:
Inconsistent argument list, first argument does not have variable name while second has
##########
sys/log/full/include/log/log.h:
##########
@@ -129,25 +131,40 @@ 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_TLV_SUPPORT (1 << 1)
+
+#define LOG_TLV_NUM_ENTRIES (1 << 0)
#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__));
+ uint32_t ue_num_entries;
Review Comment:
What is the meaning of ue_num_entries?
##########
sys/log/full/include/log/log.h:
##########
@@ -511,6 +529,14 @@ void log_printf(struct log *log, uint8_t module, uint8_t level,
int log_read(struct log *log, const void *dptr, void *buf, uint16_t off,
uint16_t len);
+/**
+ * Reads entry length from the specified log.
+ *
+ * @return The number of bytes of entry length; 0 on failure.
Review Comment:
Inconsistent doxygen comment, only return value specified it's hard to guess what is the meaning of second argument.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log and add log version 4 [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#issuecomment-2011935111
Would be good to explain what's the purpose of logging number of entries in a log itself.
Since you introduce new log version which is not backwards compatible, why no just revamp this so we don't need to add new log version and bunch of ifdefs every time a new field is added, e.g. by using tlv.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log and add log version 4 [mynewt-core]
Posted by "andrzej-kaczmarek (via GitHub)" <gi...@apache.org>.
andrzej-kaczmarek commented on PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#issuecomment-2022908422
If the base header is the same, you don't need the log version 4 in the first place. The new struct for v4 is redundant and also invalid since counter doesn't need to follow img hash - this depends on flags.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "kasjer (via GitHub)" <gi...@apache.org>.
kasjer commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1690920606
##########
sys/log/full/src/log.c:
##########
@@ -964,16 +1143,16 @@ log_read(struct log *log, const void *dptr, void *buf, uint16_t off,
int
log_read_hdr(struct log *log, const void *dptr, struct log_entry_hdr *hdr)
{
- int bytes_read;
+ int bytes_read = 0;
bytes_read = log_read(log, dptr, hdr, 0, LOG_BASE_ENTRY_HDR_SIZE);
if (bytes_read != LOG_BASE_ENTRY_HDR_SIZE) {
return SYS_EIO;
}
if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
- bytes_read = log_read(log, dptr, hdr->ue_imghash,
- LOG_BASE_ENTRY_HDR_SIZE, LOG_IMG_HASHLEN);
+ bytes_read = log_read(log, dptr, hdr->ue_imghash, LOG_BASE_ENTRY_HDR_SIZE,
+ LOG_IMG_HASHLEN);
Review Comment:
unnecessary change still present `bytes_read = 0;` added and variable is changed in next line
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1669114077
##########
sys/log/full/include/log/log.h:
##########
@@ -97,6 +97,7 @@ typedef int (*lh_append_mbuf_body_func_t)(struct log *log,
typedef int (*lh_walk_func_t)(struct log *,
log_walk_func_t walk_func, struct log_offset *log_offset);
typedef int (*lh_flush_func_t)(struct log *);
+typedef uint16_t (*lh_read_entry_len_func_t)(struct log *, const void *dptr);
Review Comment:
Yes, it is, do you want me to fix it ? Just say so. This is a PR to add support for a targeted feature. In the past we would open separate PRs for refactoring Vs changes made for a feature. I can change it in a separate PR later. cc: @sjanc
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1669260014
##########
sys/log/full/src/log.c:
##########
@@ -964,16 +1143,16 @@ log_read(struct log *log, const void *dptr, void *buf, uint16_t off,
int
log_read_hdr(struct log *log, const void *dptr, struct log_entry_hdr *hdr)
{
- int bytes_read;
+ int bytes_read = 0;
bytes_read = log_read(log, dptr, hdr, 0, LOG_BASE_ENTRY_HDR_SIZE);
if (bytes_read != LOG_BASE_ENTRY_HDR_SIZE) {
return SYS_EIO;
}
if (hdr->ue_flags & LOG_FLAGS_IMG_HASH) {
- bytes_read = log_read(log, dptr, hdr->ue_imghash,
- LOG_BASE_ENTRY_HDR_SIZE, LOG_IMG_HASHLEN);
+ bytes_read = log_read(log, dptr, hdr->ue_imghash, LOG_BASE_ENTRY_HDR_SIZE,
+ LOG_IMG_HASHLEN);
Review Comment:
Left over from intermittent changes. Will fix 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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] sys/log: Add number of entries support in log [mynewt-core]
Posted by "vrahane (via GitHub)" <gi...@apache.org>.
vrahane commented on code in PR #3168:
URL: https://github.com/apache/mynewt-core/pull/3168#discussion_r1669171215
##########
sys/log/full/src/log_fcb.c:
##########
@@ -446,11 +507,35 @@ log_fcb_append_mbuf_body(struct log *log, const struct log_entry_hdr *hdr,
}
loc.fe_data_off += LOG_IMG_HASHLEN;
}
+
rc = log_fcb_write_mbuf(&loc, om);
if (rc != 0) {
return rc;
}
+ if (hdr->ue_flags & LOG_FLAGS_TLV_SUPPORT) {
+#if MYNEWT_VAL(LOG_FLAGS_TLV_SUPPORT) && MYNEWT_VAL(LOG_TLV_NUM_ENTRIES)
+ struct log_tlv tlv;
+ tlv.tag = LOG_TLV_NUM_ENTRIES;
+ tlv.len = LOG_NUM_ENTRIES_SIZE;
+ /* Write TLV */
+ rc = flash_area_write(loc.fe_area, loc.fe_data_off, &tlv,
+ sizeof(struct log_tlv));
+ if (rc != 0) {
+ return rc;
+ }
+ loc.fe_data_off += sizeof(struct log_tlv);;
+ /* Write LOG_NUM_ENTRIES_SIZE bytes */
+ rc = flash_area_write(loc.fe_area, loc.fe_data_off,
Review Comment:
Saves code size but increases stack space by 4 bytes.
--
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: commits-unsubscribe@mynewt.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org