Dear Takahiro, In message <20190719081556.gr21...@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(); > > > + else > > > + ret = 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(); > > > + else > > > + ret = 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 -- 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: w...@denx.de It's hard to think of you as the end result of millions of years of evolution. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot