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