Dear Takahiro,
In message 20190717082525.891-1-takahiro.akashi@linaro.org you wrote:
# In version 4 of this patch set, the implementation is changed to meet # Wolfgang's requirements.
Thanks.
Each variable may have a third attribute, "variable storage." There are three new flags defined: ('flags' are a bit-wise representation of attributes.)
ENV_FLAGS_VARSTORAGE_VOLATILE: A variable only exists during runtime. ENV_FLAGS_VARSTORAGE_NON_VOLATILE: A variable is persistent, but explicit "load" and "save" command is required. ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE: A variable is persistent, any change will be written back to storage immediately.
Urgh... ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE is way too long, and unnecessary complicated. And we don't need 3 new flags, as ENV_FLAGS_VARSTORAGE_NON_VOLATILE is the default anyway.
Please just define
ENV_FLAGS_VOLATILE and ENV_FLAGS_AUTOSAVE
- Following those extensions, new interfaces are introduced: for hashtable, hsearch_r -> hsearch_ext hmatch_r -> hmatch_ext hdelete_r -> hdelete_ext himport_r -> himport_ext hexport_r -> hexport_ext for env, env_save -> env_save_ext env_load -> env_load_ext env_import -> env_import_ext env_export -> env_export_ext
Why do we need all these _ext names? Please don't. Just give the newly added functions new names.
- There is one thing visible to users; *Exported* variables now have attribute information, which is appended to variable's name in a format of ":xxx" For example, => printenv arch:san=arm baudrate:san=115200 board:san=qemu-arm ... This information is necessary to preserve attributes across reboot (save/load).
NAK. ':' is a legal character in a variable name, so you cannot use it here for new purposes. For example:
=> setenv baudrate:san foo => printenv baudrate:san baudrate:san=foo
- Each environment storage driver must be modified so as to be aware of contexts. (See flash and fat in my patch set.)
I dislike this. Can we not do all the different handling in a central place, and basically leave the storage handlers unchanged? They do not need to know about contexts; they just do the I/O.
Known issues/restriction/TODO:
- Existing interfaces may be marked obsolute or deleted in the future.
Which "existing interfaces" are you talking about?
ENV_IS_NOWHERE doesn't work if we want to disable U-Boot environment, but still want to enable UEFI (hence UEFI variables).
-> One solution would be to add *intermediate* configurations to set ENV_IS_NOWHERE properly in such a case.
Please explain what you have in ind here. What I'm guessing from this short description sounds not so good to me, but you may have better ideas...
- Any context won't have VARSTORAGE_NON_VOLATILE and VARSTORAGE_NON_VOLATILE_AUTOSAVE entries at the same time. This is because it will make "save" operation ugly and unnecessarily complicated as we discussed in ML.
NAK. Both the VOLATILE and the AUTOSAVE properties are interesting features, and should be available to normale U-Boot environment variables. So the U-Boot context must support all variantes of "normal" (persistent, non-autosave), volatile and autosave variables at the same time. If you don't need this for UEFI, ok. But we want to have this for U-Boot.
-> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed to a context itself.
No. This is NOT what we need.
- env_load() may lose some of compatibility; It now always imports variables *without* clearing existing entries (i.e. H_NOCLEAR specified) in order to support multiple contexts simultaneously.
NAK. This must ne fixed.
Unfortunately, some of existing U-Boot variables seem to be "volatile" in nature. For example, we will see errors at boot time: ... Loading Environment from Flash... OK ## Error inserting "fdtcontroladdr" variable, errno=22 In: pl011@9000000 Out: pl011@9000000 Err: pl011@9000000 ## Error inserting "stdin" variable, errno=22 ## Error inserting "stdout" variable, errno=22 ## Error inserting "stderr" variable, errno=22
-> We will have to changes their attributes one-by-one. (Those can also be set through ENV_FLAGS_LIST_STATIC in env_flags.h).
I think your approach to fix this is wrong. The problem should not happen in the first place. And no, stdin/stdout/stderr are _not_ volatile.
- As described above, attribute information is visible to users. I'm afraid that it might lead to any incompatibility somewhere. (I don't notice any issues though.)
Correct, the current code is incompatible. This must be solved differently. Colon is a legal character in a variable name.
- The whole area of storage will be saved at *every* update of one UEFI variable. It should be optimized if possible.
This is only true for UEFI storage, right?
You can also define a non-volatile variable from command interface: => setenv -e -nv FOO baa
This makes no sense. Being non-volatile is the default. This must remain as is. Only "volatile" and "autosave" are new attributes.
More comments with the patches...
Best regards,
Wolfgang Denk