Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sys/log: Add number of entries support in log #3168

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

vrahane
Copy link
Contributor

@vrahane vrahane commented Mar 20, 2024

Please check email on the dev list 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
  • Backwards compatible with earlier log header
  • Downgrading to older log header will cause the old entries to not be read, upgrade case works fine.
  • CLI command support is also added to help read the number of entries since a particular index on the CLI
    CLI command:
compat> log -ne reboot_log -i 50

043837 entries: 5

Example log entries:

7143966 [ih=0x92ea3333][ne=0] [1577940571820322]  [ix=16] {_ "rsn": "OTHER: 0x0", "cnt": 238, "img": "24.3.19.131805", "hash": "92ea3333ffe0c6eaa01b842e2a0f8692f6b4a748de10378423b7abc8887993d2", "flags": "active bootable confirmed"}
7143973 [ih=0x92ea3333][ne=1] [1577949942585905]  [ix=33] {_ "rsn": "SOFT", "cnt": 239, "img": "24.3.19.131805", "hash": "92ea3333ffe0c6eaa01b842e2a0f8692f6b4a748de10378423b7abc8887993d2", "flags": "active bootable confirmed"}
7143978 [ih=0x92ea3333][ne=2] [1577951411585993]  [ix=50] {_ "rsn": "SOFT", "cnt": 240, "img": "24.3.19.131805", "hash": "92ea3333ffe0c6eaa01b842e2a0f8692f6b4a748de10378423b7abc8887993d2", "flags": "active bootable confirmed"}
7143984 [ih=0x92ea3333][ne=3] [1577953577609410]  [ix=67] {_ "rsn": "SOFT", "cnt": 241, "img": "24.3.19.131805", "hash": "92ea3333ffe0c6eaa01b842e2a0f8692f6b4a748de10378423b7abc8887993d2", "flags": "active bootable confirmed"}
7143989 [ih=0x92ea3333][ne=4] [1577956085609419]  [ix=84] {_ "rsn": "WDOG", "cnt": 242, "img": "24.3.19.131805", "hash": "92ea3333ffe0c6eaa01b842e2a0f8692f6b4a748de10378423b7abc8887993d2", "flags": "active bootable confirmed"}

@vrahane vrahane changed the title Add number of entries support in log and add log version 4 sys/log: Add number of entries support in log and add log version 4 Mar 20, 2024
@andrzej-kaczmarek
Copy link
Contributor

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.

@vrahane
Copy link
Contributor Author

vrahane commented Mar 26, 2024

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.

@vrahane
Copy link
Contributor Author

vrahane commented Mar 26, 2024

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.

@andrzej-kaczmarek
Copy link
Contributor

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.

@vrahane
Copy link
Contributor Author

vrahane commented Mar 27, 2024

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.

@vrahane vrahane closed this Apr 2, 2024
@vrahane vrahane reopened this Apr 2, 2024
@vrahane
Copy link
Contributor Author

vrahane commented Apr 3, 2024

@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 ?

@vrahane vrahane changed the title sys/log: Add number of entries support in log and add log version 4 sys/log: Add number of entries support in log Apr 3, 2024
@kasjer kasjer added the LOGS label May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants