[U-Boot] [RFC, PATCH v4 05/16] env: fat: add U-Boot environment context support

[AKASHI Takahiro]

On Fri, Jul 19, 2019 at 10:21:12AM +0200, Wolfgang Denk wrote:
> Dear AKASHI Takahiro,
> 
> In message <20190717082525.891-6-takahiro.akashi at linaro.org> you wrote:
> > Please note that the aim of this patch is to illustrate how we can
> > extend the existing backing storage drivers for env interfaces to
> > support U-Boot environment context.
> >
> > We will be able to support more devices as well as more contexts
> > in a similar way. Existing drivers still work exactly in the same
> > way as before while they are not extended yet.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> >  env/fat.c | 103 ++++++++++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 84 insertions(+), 19 deletions(-)
> >
> > diff --git a/env/fat.c b/env/fat.c
> > index 7f74c64dfe7e..e4a672d2730a 100644
> > --- a/env/fat.c
> > +++ b/env/fat.c
> > @@ -30,23 +30,44 @@
> >  # endif
> >  #endif
> >  
> > +static struct evn_fat_context {
> > +	char *interface;
> > +	char *dev_and_part;
> > +	char *file;
> > +} fat_context[ENVCTX_COUNT] = {
> > +#if defined(CONFIG_ENV_FAT_INTERFACE) && \
> > +	defined(CONFIG_ENV_FAT_DEVICE_AND_PART) && defined(CONFIG_ENV_FAT_FILE)
> > +	[ENVCTX_UBOOT] = {
> > +		CONFIG_ENV_FAT_INTERFACE,
> > +		CONFIG_ENV_FAT_DEVICE_AND_PART,
> > +		CONFIG_ENV_FAT_FILE,
> > +	},
> > +#endif
> > +};
> 
> Does it make sense to define this in a FAT specific way? I guess we
> could come up with a common structure that covers all supported
> storage devices and use this here.

But a different driver has different sets of configurations required.
How can we *generalize* them?

> Also, the "#if defined" looks really dangerous to me, as missing
> #defines will go unnoticed, and in the end you are accessing
> uninitialized data... 

Yes, I have made this mistake before.
But I think that all the drivers must be verified in any case
before any changes are applied to the upstream. No?

> Did you review the patchset for memory leaks? 

No.

-Takahiro Akashi

> 
> 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
> If you can't explain it to a six year old, you  don't  understand  it
> yourself.                                           - Albert Einstein