KVStore: Add init bit flags for read/write and creation mode

[pea-pod]

Summary of changes

Add read, write, and creation mode flags to the initialization method for each KVStore instance using a default parameter. The addition should not cause existing code to fail to compile. These flags use an enum class new to C++11.

Add documentation to each class implementation regarding new failure values returned in each case if the R/W mode does not match the flag used when init() was called.

Add a platform level macro in platform/mbed_enum_flags.h named MBED_SCOPED_ENUM_FLAGS which provides bitwise operator overloads (including assignment) to scoped enum types.

Add tests to check if the flag is set, the KVStore instances do not create if a valid instance does not already exist.

Move KVStore concrete protected method _is_valid_flags to KVStore.cpp and added helper functions for flags.

Note: some of the creation modes (truncate, etc.) are not yet implemented. This will be followed by another PR to add these methods, assuming this addition is satisfactory. For now, the unimplemented modes do not fail silently, but return an error.

Impact of changes

New creation methods are allowed if desired. The device can avoid overwriting existing data if the data is valid, but not a KVStore instance. See the InitModeFlags.h file for more details.

Migration actions required

None.

Documentation

Added documentation to each class which now implements the new feature.

Added documentation to the new macro and the new InitModeFlags enum class.

---

Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[X] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[X] Tests / results supplied as part of this PR

---

Reviewers

---

[ciarmcom]

@pea-pod, thank you for your changes.
@ARMmbed/mbed-os-storage @ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

[VeijoPesonen]

I read the description and I would like to get an additional information appended to it which would explain why all these changes are done/ what is the problem you're trying to solve.

[SeppoTakalo]

Thank you for the contribution.

This is API extension, without any suggested use cases.
I'm not in favour of accepting it in, as it seems to add complexity and code size, without any benefit for most common users.

[kjbracey]

Thanks for the enum/bitset stuff. Gave me something to think about. I like the idea of having the platform helper, but there's a lot to bikeshed about there.

From API point of view, we could probably pin down what the KVStore::InitModeFlags looks like to the user - let's say it's a C++ "bitmask type" (https://en.cppreference.com/w/cpp/named_req/BitmaskType). Maybe first pass doesn't use the helper yet, if that takes time to figure out. Exactly what it looks like doesn't matter much to the user - they're just using | and passing it to functions. It mainly matters in the implementation - how much hassle it is to test the bits.

[kjbracey]

Could this be a member of KVStore? There's precedent for that in create_flags.

If it is placed inside KVStore, you might be able to get away with being unscoped, eg values being KVStore::Read rather than KVStore::InitModeFlags::Read.

You could probably drop the Flags at least to get to KVStore::InitMode::Read. Comparing with https://en.cppreference.com/w/cpp/named_req/BitmaskType, "flags" is usually not explicit.

[pea-pod]

I would prefer to keep the enum external to the class. It is possible that this enum class could be useful in other contexts.

[kjbracey]

You can just say Read | Write in this context I think.

[pea-pod]

Done.

[kjbracey]

Convoluted? Why not

(_flags & flags) != InitModeFlags::NoFlags

or, as per the comment above below

(_flags & flags) != InitModeFlags()

No need to use a named constant for "no flags".

Do you even need the special helper function, as this is no more complex than what you have to do for a single bit? If you do have it, maybe the single bit test should use it - _has_flags_any(InitModeFlags::Write)...

[kjbracey]

This sort of verbosity grates, particularly the repetition. I feel it should be fine to say if (flags & X). But it isn't :(

Digging into it, I see there is some confusion about "bitmask types". See issue 3092 at https://cplusplus.github.io/LWG/lwg-unresolved.html

And previously there have been notes about stuff assuming what I was thinking, eg: https://cplusplus.github.io/LWG/issue2659

In the latter they settled on if ((flags & options::X) != options::none), which is mildly better IMO, if only because it's not repetitive. As the 3092 issue points out though, that could be shortened to if ((flags & options::X) != options()), which I quite like, visualising the empty bitset.

Still, would have been nice to get the contextual conversion to bool - I see one maniac on the web trying to go fully type safe, distinguishing the "bitmask" type from the "bit value" type, so you get contextual conversion to bool on the result of & and aren't allowed to accidentally do == instead of &: https://dalzhim.github.io/2017/08/11/Improving-the-enum-class-bitmask/

I find that actually quite tempting. BSD license...

[pea-pod]

@VeijoPesonen: I can provide more detail on use cases in the PR body for record keeping. I do have non-brief use cases below, but I would not put them in as is in the PR itself.

@SeppoTakalo: I am so sorry for the lack of brevity in the following, but here it is.

First, I have existing code in a project I was working on (in a different context). This project was based on os version 2. The code simply saved off a struct to NV memory. Fine, except for the wear-leveling, the lack of upgrading, etc. The newer firmware (that I am writing) would also need to handle both fresh hardware, and hardware with data already written to the NVM.

As I looked into the TDBStore source, I realized this was both a duplication of efforts, both my check and the TDBStore checks did roughly the same thing. However, when the TDBStore found used memory (but not a TDBStore), it tried to wipe it and go from there. I thought about copying the code out and doing that portion of the check myself but then I came upon these issues:

1. I would be violating the DRY principle (and I hate RY-ing anyway) as most of the code would be identical, or at least very similar.
2. I would be adding a lot of code that the linker may or may not see as duplicate. Would the linker be able to deduplicate it? If not, I would be adding more than just some if statements and return values to accomplish this.
3. If I really wanted the code module to stop creating its kv pairs, magic number, etc., in memory, how could I do that? There was no way to accomplish what I can easily do with the similar fopen semantics. Granted, They use cstrings, but surely, a bitfield would be better?
4. I could not really write better code than what the TDBStore already had. This is not an attempt at false humility. I want to build the application, not create NV storage from scratch if I could help it.

In any case, that's where I started. It was initially a bool and had this signature: init(no_overwrite = false). I realized I was basically copying fopen, but more in a more dumby-er way. Furthermore, since modifying the API once established is A Lot Harder with a bool than an enum, I went with the enum. Once I had the ExclusiveCreation (i.e. open, but do not create new) approached figured, I realized I was knee deep, and I might as well implement the parts I thought were simple enough.

Finally, for the use cases, besides the one I just defined (which I will have to use some form of, whether or not this improvement is accepted), here are each of the flags.

Access mode

Read only
Allows you to have a firmware load which has writing out disabled.

1. In higher application logic, your code only opens the KVStore in readonly when the device is (or can be) connected to the network.
2. Normal boot starts with readonly, but in debug mode, engineering mode, etc., this class "restarts" in R/W.
3. Read only is always enabled and a separate firmware load allows parameters to be set. An almost write-once memory, so to speak.
4. On multi-core systems, the secondary processor may open in this mode to make sure that it can only read. It is more of a guarantee that the second core doesn't try to write (not to insure against race conditions).

Write only

1. Allows for data to be pushed one way from a producer core which has write only to a consumer core which has read only. Obviously, a write would not be a frequent occurrence, but I could see a "highest temp" on a core dedicated to monitoring voltages, temperatures, and the number of cats sensed from the Cat Sensor 5000.
2. On complete device reset, insures that only overwrites are occurring, but it cannot (without modification of something) be read out.
3. It's almost "free". If you want readonly as well as read/write, this comes with the system as it is currently implemented, and is not "extra", beyond what is there to implement it.

Write only but with read keys
This was the only one I was not terribly sure on. I thought it would be useful, in some instances, to be able to iterate over the keys, but without the ability to read their data. I could actually see this being most useful for the SecureStore class, where knowledge of the key name is not valuable, but the old key data would be. However, for storing new data, you would not necessarily want to give that over.

Comment: on either of the "onlys", if LTO is enabled, and if the code is done properly (meaning my changes are done properly), the linker could even remove the unused code (i.e. if _flags is guaranteed to never have the write flag) which may not be super secure, but more helpful than not, I believe.

Creation mode

I basically took these from Python, but with bitflags to make sure there weren't magic strings lurking about to pounce. Plus, vscode knows how to read doc strings. The modes are meant to be virtually identical to their use in Python's open function. It is also orthogonal to the r/w modes. I consulted both the Linux man pages as well as Python's documentation.

Append
Opens the KVStore instance to append, and create new if necessary. Use case: default usage.

Truncate - Not yet implemented
Opens the KVStore and immediately erases the whole thing. This would be more cycle-efficient, if done in the class code to erase everything first, knowing then that you would start afresh. It would be functionally identical to init() followed by reset(), although with the right organization, it would only be slightly larger in code (i.e. moving whatever is common to a _reset() method).

CreateNewOnly Not yet implemented
Perhaps the presence of a KVStore instance means something else is wrong with the system (intrusion, or whatever -- I'm not exactly sure). Or maybe this is one instance where when you put in an SD card, the software attempts to use this method first. If it succeeds, then you do not have to worry about overwriting data. If it does not succeed, you might need to ask nicely before you blow out what is already there.

ExclusiveCreation
This was the was the starting point of my initial efforts. Of course, there are 1,000 ways to handle this, but without this built into the class, I have to reach outside of the class implementing this change in order to make sure I do not overwrite presumably useful data.

Regarding the complexity, there are three spots to look at, in my opinion. Some of this may be obviated by events if the scoped enum goes in separately, but here are my thoughts in any case:

enum class
The enum is (hopefully) a uint32_t and only adds 1 word to the class. All of the functions that it uses are (again, hopefully) small numbers of additional instructions for bitwise ANDs and XORs, etc.

Making this an enum class forces it to a single word and allows it to have namespace access in ways that are all but impossible for a normal enum or its ugly cousin, #define. Making it a struct would seem far more likely that it would bloat up in ROM.

Code complexity
A few ways to look at this one:

• I need this functionality, so where should it go, if I were King of Texas? The least addition to ROM and therefore the most efficient place (for flash, if not execution speed) is right inside the class method which is already checking for errors. (Plus, if I needed it, someone else probably did too.)
• By making this an enum, I almost guarantee that I can give it useful words to use and doc strings to go with it instead of a magic string. This may not be what you meant by complexity, but it's here anyway.
• I actually believe that this implementation is relatively straightforward. By that, I mean that the most code I changed was in TDBStore.cpp and in that, the init() method. I do not consider the 15 to 30 lines I changed in that too complex. This is obviously my opinion, but I believed I was very concise in those changes, and not terribly full of malarkey. (Only slightly malarkeyed.)
• The using of flags (of a sort) for this type of operation is common in all of the fopen/open/File.Open that I have ever used. The only difference between how I used init() here and open in Python is that I defaulted to r/w, which was done to preserve the API as currently used.
• By making it a flag and not a magic string (even if it is just "w"), I have made the order of flag declaring irrelevant ("bw" or "wb"?), and I have hopefully aided vscode users like myself.

One last argument here: this is the only class of I/O functionality that I am aware of where when it fails to open/read/etc. it doesn't allow the application developer options on how to deal with it. For networking, you get numerous error codes, state machines, and the like. For file I/O, you get not only failure, but why it failed. If it succeeds, it doesn't do so at the expense of your current data, unless you want it to. In the case of KVStore, only some of the failures are addressed by the class itself. Beyond that, you have to deal with the underlying device, or add functionality externally. And in case this is an issue: I do believe it an apt comparison to fopen, if for no other reason than both obviate the need to deal with the underlying block device. I quite like having to only deal with one level if possible, and I think this would be an appropriate place to do it in.

@kjbracey-arm: I will try to work through the suggestions soon. However, if it is not too much to ask, I would like to have more clarity on whether this PR has a chance to make it in.

And to all: I'm sorry I let loose the Word Cannon.

[mergify]

This PR cannot be merged due to conflicts. Please rebase to resolve them.

[0xc0170]

Thanks @pea-pod for providing more details for this addition.

@ARMmbed/mbed-os-storage would you be able to review this new feature?

[pea-pod]

ahem

Would someone review this?

[mergify]

This PR cannot be merged due to conflicts. Please rebase to resolve them.

[adbridge]

Hi @pea-pod most of our Finnish colleagues are currently on vacation (they tend to take all of July off). We will try and get someone to review again once they are back.

[pea-pod]

Thank you, @adbridge. I was hoping this would make it in before the Great Restructuring of 2020, but it seems like that was not in the cards.

I understand (at least a little). I used to work for a certain company with an office in Oulu, so I'm familiar with the summer vacations habits of the average Suomalainen.

I am still interested in a review. I am holding off on rebasing until the review, at least for the features themselves. If that is not quite good enough, then I will work on it. I would rather not work too much at this if the work itself will not be useful.

[0xc0170]

@ARMmbed/mbed-os-core I assume you will take over this one to help reviewing?

[pea-pod]

If I say that this is an update for the cmake feature branch, would someone review it, even though it is not? Asking for a friend.

[evedon]

Thanks for the PR and the effort you put into it.

Your original use case was for exclusive creation. I'd rather that you simplify the code and only provide support for that use case. This would remove the other modes which are not implemented (Truncate, CreateNewOnly) and the R/W access modes which I am not convinced add benefits for the common use. You can keep the flags concept (as opposed to a boolean) as it will give more flexibilty to add the other flags if and when needed.
Also please add test case for ExclusiceCreation in case of success.

[0xc0170]

@pea-pod I'll close this as it has not been updated for more than a month. Please reopen once updated (changes requested above).

[pea-pod]

Fair enough.

Every week I say to myself, "I'll get to it this week." Every week, it gets to Friday and Saturday and I haven't gotten to it.

I will try to move on it soon, but I'll update it before I reopen it.

Thanks!