On Fri, Jul 19, 2019 at 08:50:18AM +0200, Wolfgang Denk wrote:
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.
But this rule *does* conform with other flags' naming rule. Aren't other name, like env_flags_varaccess_changedefault in env_flags_varaccess, or ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR long? What is the upper limit that you think?
And we don't need 3 new flags, as ENV_FLAGS_VARSTORAGE_NON_VOLATILE is the default anyway.
"default" may be different on different contexts. It cannot be the reason.
Please just define
ENV_FLAGS_VOLATILE and ENV_FLAGS_AUTOSAVE
Disagree.
- 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.
You seem to misunderstand my patch. Please read my code first. I have not renamed the existing interfaces, which still remain for compatibility, and added new functions with "extended" 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:
What is your recommendation? I didn't find what are the definition of "illegal" characters.
=> 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.
How? I don't get your point. Did you read patch (#4 and) #5 and #14? Context-specific code are quite isolated and yet we need to modify drivers a bit.
Known issues/restriction/TODO:
- Existing interfaces may be marked obsolute or deleted in the future.
Which "existing interfaces" are you talking about?
h*_r(), env_import/export(), env_save/load() and etc. Once all the occurrences in the repository have been udpated using new interfaces, we will be able to delete them, won't you?
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...
For example, if we want to use FAT for UEFI variables, but don't want to enable U-Boot envrionment for some reason (don't ask me why now), we cannot simply enable ENV_IS_NOWHERE, then U-Boot envrionment won't work as expected in some places.
- 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.
NAK. Do you remember our discussion in the past? Allowing such a feature may end up with ugly and complicated implementation. I believe that we agreed on this point before.
-> In this sense, NON_VOLATILE[or _AUTOSAVE] should be attributed to a context itself.No. This is NOT what we need.
Why?
- 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.
NAK. This is a natural result of supporting multiple contexts in U-Boot environment (plus UEFI variables in one hash table.)
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.
I can't understand why we need to save stdin/stdout/stderr as non-volatile variables. It should be initialized at every boot.
- 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.
How? You denied, in the past discussions in ML, that key or value be 'structured' as an opaque object.
- 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?
Yes, so?
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.
'-nv' option is already merged in the upstream, and it is only valid for UEFI variables. And again, there is no *default* attribute for UEFI variables.
Thanks you for your comments, -Takahiro Akashi
More comments with the patches...
Best regards,
Wolfgang Denk
-- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de It usually takes more than three weeks to prepare a good impromptu speech. - Mark Twain