Change Reboot log from text to CBOR encoded.#2011
Conversation
vrahane
left a comment
vrahane
left a comment
There was a problem hiding this comment.
Generally, it looks fine but I think you should also add a dependency for tinycbor in pkg.yml for the reboot package
ACK |
sys/reboot/src/log_reboot.c
Outdated
| * buffer, then trim the filename from left. */ | ||
| if (strlen(info->file) > (sizeof(buf) / 3)) | ||
| { | ||
| off = strlen(info->file) - (sizeof(buf)/3); |
sys/reboot/src/log_reboot.c
Outdated
| cbor_encode_text_stringz(&map, "img"); | ||
| snprintf(buf, sizeof(buf), "%u.%u.%u.%u", | ||
| ver.iv_major, ver.iv_minor, ver.iv_revision, | ||
| (unsigned int) ver.iv_build_num); |
There was a problem hiding this comment.
original snprintf did not have space after cast, which is more common in mynewt
sys/reboot/src/log_reboot.c
Outdated
| if (state_flags & IMGMGR_STATE_F_PENDING) { | ||
| off += snprintf(buf + off, sizeof buf - off, "%s ", "pending"); | ||
| } | ||
| buf[off - 1] = '\0'; |
There was a problem hiding this comment.
in case of no flags (I don't know if it can happen) buf[off - 1] = 0; would write outside buf.
That will be caught by coverity tool.
There was a problem hiding this comment.
I thought of adding a protection earlier, but didn't thought it's absolutely required as technically there will be at-least one flag. Anyway, for the sanity I guarded this unlikely case.
sys/reboot/src/log_reboot.c
Outdated
| state_flags = imgmgr_state_flags(boot_current_slot); | ||
| cbor_encode_text_stringz(&map, "flags"); | ||
| off = 0; | ||
| memset(buf, 0, sizeof buf); |
There was a problem hiding this comment.
memset seems like overkill where buf[0] = '\0'; would do
kasjer
left a comment
kasjer
left a comment
There was a problem hiding this comment.
Quality of the code is very good. I'm not sure why the switch from text to cbor was done, but I certainly don't have any problem with that.
I don't knew much details of cbor stuff in mynewt so I was not hard on buf size vs cbor_enc_buf size which may have room for future stack size optimization.
|
@manish-sudo Thanks, one thing I forgot to mention is for future PRs please mention the modules in the commit message. I should have mentioned this before approving but yeah, it's nice to have to go back and look at the history and know which module the commit is addressed for. |
4 participants
No description provided.