[U-Boot] [RFC, PATCH v4 02/16] env: extend interfaces to import/export U-Boot environment per context

[Wolfgang Denk]

Dear Takahiro,

In message <20190719081556.GR21948 at 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: wd at denx.de
It's hard to think of you as the end result of millions of  years  of
evolution.