Dear Takahiro,
In message 20190719081556.GR21948@linaro.org you wrote:
Okay, but this is not specific to this function. I'm going to add detailed function descriptions if you agree with these new interfaces. Do you?
I agee with the interface, but not with the new names. We should use the existing names, and not change them. Only add new functions where needed.
- if (himport_ext(&env_htab, ctx, (char *)ep->data, ep->data_size,
/** FIXME:* H_NOCLEAR is necessary here to handle* multiple contexts simultaneously.* This, however, breaks backward compatibility.*/'\0', H_NOCLEAR, 0, 0, NULL)) {This is the result of a deign bug. You can never have "multiple contexts simultaneously", because the code as you designed it always has only one context per data block - and I can;t see how this could be changed, as the external storage format ('name=value\0') does not provide a way to include variable-specific context information.
Please read path#14 where a fat driver is modified to maintain UEFI variables in U-Boot environment hash tables.
If UEFI variables need not be maintained in a single U-Boot environment, my whole patch set goes in vain. (This is my "Plan B" implementation.)
I was talking about external storage - there you can have only one context in a data block. Internally (in the hash table), the context should probably be art of the key, so there cannot be any such conflicts either.
- set_default_env("import failed", 0);
- if (ctx == ENVCTX_UBOOT)
set_default_env("import failed", 0);Should we not also have default settings for other contexts as well?
Handling for default (initial) variables may vary context to context. It should be implemented depending on their requirements.
It should be handled in a single, generic way, and we allow that a context defines his own implementation (say, through weak default handlers which can be overwritten by context-specific code).
+int env_import_redund_ext(const char *buf1, int buf1_read_fail,
const char *buf2, int buf2_read_fail,enum env_context ctx)+{
- /* TODO */
- return 0;
!!
Do you plan to implement redundant storage for UEFI data as well? It would make sense, wouldn't it?
Yes and no. My current focus is to determine if my approach is acceptable or not.
OK - but this shows clealy the disadvantages of defining new names. Please get rid of all this _ext stuff...
It seems you have a memory leak here. I cannot find a free(envp) anywhere.
envp will be returned to a caller via env_out.
Yes, but where does it get freed?
Speaking about memory... what is the overall code / data size impact of this patch set?
No statistics yet.
Can you please provide some?
- if (ctx == ENVCTX_UBOOT)
loc = env_get_location(op, prio);- else
loc = env_get_location_ext(ctx, op, prio);This makes no sense. ENVCTX_UBOOT is just one of the available contexts; no "if" should be needed here.
NAK. env_get_location is currently defined as a weak function. So this hack is necessary, in particular, on a system where UEFI is not enabled and env_get_location is yet overwritten.
Well, this mechanism need to be adapted for contexts, then It may indeed makle sense to provide the same overwrite possibiltiy for each context.
if (!drv->load)
if ((ctx == ENVCTX_UBOOT && !drv->load) ||(ctx != ENVCTX_UBOOT && !drv->load_ext)) continue;Ditto here.
ret = drv->load();
if (ctx == ENVCTX_UBOOT)ret = drv->load();elseret = drv->load_ext(ctx);...and here.
- env_get_location(ENVOP_LOAD, best_prio);
- if (ctx == ENVCTX_UBOOT)
env_get_location(ENVOP_LOAD, best_prio);- else
env_get_location_ext(ctx, ENVOP_LOAD, best_prio);...and here.
if (!drv->save)
if ((ctx == ENVCTX_UBOOT && !drv->save) ||(ctx != ENVCTX_UBOOT && !drv->save_ext)) return -ENODEV;...and here.
if (!env_has_inited(drv->location))
if (!env_has_inited(ctx, drv->location)) return -ENODEV;...and here.
ret = drv->save();
if (ctx == ENVCTX_UBOOT)ret = drv->save();elseret = drv->save_ext(ctx);...and here.
All this code _begs_ for cleanup.
int env_init(void) { struct env_driver *drv; int ret = -ENOENT;
- int prio;
- int ctx, prio;
Error. Context is an enum.
- /* other than ENVCTX_UBOOT */
- for (ctx = 1; ctx < ENVCTX_COUNT; ctx++) {
Ugly code. "ctx" is an enum, so "1" is a magic number here that should not be used.
There are already bunch of code where enum is interchangeably used as an integer.
There is no good excuse for following bad examples. There is not even any documentation that mentions that ENVCTX_UBOOT has to be the first entry in the enum.
- for (prio = 0; (drv = env_driver_lookup(ENVOP_INIT, prio)); prio++) {
- /* ENVCTX_UBOOT */
- ret = -ENOENT;
- for (prio = 0; (drv = env_driver_lookup(ENVCTX_UBOOT, ENVOP_INIT,
prio));prio++) {See problem above. U-Boot context should not need separate code, it is just one context among others...
It seems to me that this comment is conflicting with your assertion below.
No. You can still iterate over the whole enum, and in the look dosomething like "if (CTX == ENVCTX_UBOOT) continue;" or such, without relying on a specific numeric value. This is much more reliable.
And you can also extend this with proper testing for running in SPL ir not, etc.
NAK. Please keep this information out of GD. This is a tight resource, we must not extend it for such purposes.
But in this way, we will have to handle contexts differently depending on whether it is ENVCTX_UBOOT or not.
Yes, indeed, U-Boot environment may be handled differently, but we can still share the same code.
--- a/include/env_default.h +++ b/include/env_default.h @@ -15,6 +15,7 @@ env_t environment __UBOOT_ENV_SECTION__(environment) = { #ifdef CONFIG_SYS_REDUNDAND_ENVIRONMENT 1, /* Flags: valid */ #endif
- ENV_SIZE,
Full NAK!!!
You *MUST NOT* change the data structure of the environment on external storage, as this breaks compatibility with all existing systems. This is a strict no-go.
Let me think, but I don't have a good idea yet.
You can only _pre_pend the size field in a bigger structure, which has size first, followed by the existing struct env.
NAK. We should ot need always pairs of functions foo() and foo_ext(). There should always be only foo(), which supports contexts.
This is a transition state as I mentioned in "known issues/TODOs" in my cover letter. If you agree, I can remove all the existing interfaces but only when all the storage drivers are converted.
Adding the additional context parameter seems not so much a problem. This can be a single big commit including all the drivers, which get passed a '0' argument which they may ignore. The introdduce contexts as a scond step.
Best regards,
Wolfgang Denk