[U-Boot] [RFC, PATCH v4 06/16] env: add variable storage attribute support

[Wolfgang Denk]

Dear Takahiro,

In message <20190717082525.891-7-takahiro.akashi at linaro.org> you wrote:
> If U-Boot environment variable is set to ENV_FLAGS_VARSTORAGE_NON_VOLATILE,
> it will be saved at env_save[_ext]().

NAK.  This is the status quo for all environment variables, and must
remain the default.

> If U-Boot environment variable is set to
> ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE, it will be saved at
> env_set_ext() as well as env_save[_ext]().
> Otherwise, it is handled as a temporary variable.

The logic is wrong.  It should be:

ENV_FLAGS_AUTOSAVE is saved at env_set().

ENV_FLAGS_VOLATILE is not saved at env_save().

That's all.

[And it should be checked that assigning ENV_FLAGS_VOLATILE and
ENV_FLAGS_AUTOSAVE flags to the same variable is handled as an
error.]

>  static const char env_flags_vartype_rep[] = "sdxb" ENV_FLAGS_NET_VARTYPE_REPS;
>  static const char env_flags_varaccess_rep[] = "aroc";
> +static const char env_flags_varstorage_rep[] = "vna";

Please fix.

> +static const char * const env_flags_varstorage_names[] = {
> +	"volatile",
> +	"non-volatile",
> +	"non-volatile-autosave",
> +};

And here.

> +/*
> + * Print the whole list of available storage flags.
> + */
> +void env_flags_print_varstorage(void)
> +{
> +	enum env_flags_varstorage curstorage = (enum env_flags_varstorage)0;
> +
> +	while (curstorage != env_flags_varstorage_end) {
> +		printf("\t%c   -\t%s\n", env_flags_varstorage_rep[curstorage],
> +		       env_flags_varstorage_names[curstorage]);
> +		curstorage++;
> +	}
> +}

This makes little sense to me.  It has nothing to do with "storage".
Also, "non-volatile" is no special case and does not need a separate
flag or state. 

Your handling suggests that these are states on the same level,
which is not the case - a variable may be volatile or not, but
non-volatile variables acan be auto-save, while volatile ones
cannot.

> +
> +/*
> + * Return the name of the storage.
> + */
> +const char *env_flags_get_varstorage_name(enum env_flags_varstorage storage)
> +{
> +	return env_flags_varstorage_names[storage];
> +}
>  #endif /* CONFIG_CMD_ENV_FLAGS */

This does not work, as one variiable can be both persistent _and_
auto-save at the same time.

> +/*
> + * Parse the flags string from a .flags attribute list into the varstorage enum.
> + */

Using an enum is inherently wrong here.

Please keep in ind that we may want toadd additional poperties which
are completely orthogonal to the flags you're adding here.  Your
enum type handling oes not allow any such additions.  Please drop
it.

> +/*
> + * Parse the binary flags from a hash table entry into the varstorage enum.
> + */
> +enum env_flags_varstorage env_flags_parse_varstorage_from_binflags(int binflags)
> +{
> +	int bits;
> +
> +	bits = binflags & ENV_FLAGS_VARSTORAGE_BIN_MASK;
> +	if (bits == ENV_FLAGS_VARSTORAGE_VOLATILE)
> +		return env_flags_varstorage_volatile;
> +	else if (bits == ENV_FLAGS_VARSTORAGE_NON_VOLATILE)
> +		return env_flags_varstorage_non_volatile;
> +	else if (bits == ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE)
> +		return env_flags_varstorage_non_volatile_autosave;
> +
> +	printf("Warning: Non-standard storage flags. (0x%x)\n",
> +	       binflags & ENV_FLAGS_VARSTORAGE_BIN_MASK);
> +
> +	return env_flags_varstorage_non_volatile;
> +}

Ditto here.

> +/*
> + * Look up the storage of a variable directly from the .flags var.
> + */
> +enum env_flags_varstorage env_flags_get_storage(const char *name)
> +{
> +	const char *flags_list = env_get(ENV_FLAGS_VAR);
> +	char flags[ENV_FLAGS_ATTR_MAX_LEN + 1];
> +
> +	if (env_flags_lookup(flags_list, name, flags))
> +		return env_flags_varstorage_non_volatile;
> +
> +	if (strlen(flags) <= ENV_FLAGS_VARSTORAGE_LOC)
> +		return env_flags_varstorage_non_volatile;
> +
> +	return env_flags_parse_varstorage(flags);
> +}

And here.

>  	binflags = env_flags_parse_vartype(flags) & ENV_FLAGS_VARTYPE_BIN_MASK;
>  	binflags |= env_flags_varaccess_mask[env_flags_parse_varaccess(flags)];
> +	switch (env_flags_parse_varstorage(flags)) {
> +	case env_flags_varstorage_volatile:
> +		/* actually no effect */
> +		binflags |= ENV_FLAGS_VARSTORAGE_VOLATILE;
> +		break;
> +	case env_flags_varstorage_non_volatile:
> +		binflags |= ENV_FLAGS_VARSTORAGE_NON_VOLATILE;
> +		break;
> +	case env_flags_varstorage_non_volatile_autosave:
> +		binflags |= ENV_FLAGS_VARSTORAGE_NON_VOLATILE_AUTOSAVE;
> +		break;
> +	case env_flags_varstorage_end:
> +		/* makes no sense */
> +		break;
> +	}

and here.

>  static int first_call = 1;
>  static const char *flags_list;
>  
> +static int env_flags_default(enum env_context ctx)
> +{
> +	if (ctx == ENVCTX_UBOOT)
> +		return ENV_FLAGS_VARSTORAGE_NON_VOLATILE;
> +#ifdef CONFIG_EFI_VARIABLE_USE_ENV
> +	else if (ctx == ENVCTX_UEFI)
> +		/* explicitly set by caller */
> +		return 0;
> +#endif

As mentioned several times before, ENV_FLAGS_VARSTORAGE_NON_VOLATILE
should not be needed at all, this is the default. Which means, the
whole funktion is not needed.  Drop it.

>   * Look for possible flags for a newly added variable
>   * This is called specifically when the variable did not exist in the hash
> @@ -434,6 +553,9 @@ void env_flags_init(ENTRY *var_entry)
>  	/* if any flags were found, set the binary form to the entry */
>  	if (!ret && strlen(flags))
>  		var_entry->flags = env_parse_flags_to_bin(flags);
> +	/* TODO: check early? */

What does that mean??

> +	/*
> +	 * TODO: context specific check necessary?
> +	 * For example, STORAGE_NON_VOLATILE and STORAGE_NON_VOLATILE_AUTOSAVE
> +	 * must not be mixed in one context.
> +	 */

They must never be mixed.  Yes, this must be checked and handled.

> +enum env_flags_varstorage {
> +	env_flags_varstorage_volatile,			/* 'v' */
> +	env_flags_varstorage_non_volatile,		/* 'n' */
> +	env_flags_varstorage_non_volatile_autosave,	/* 'a' */
> +	env_flags_varstorage_end
> +};

NAK, see above.  We don;t need a whole new set of flags, just add
"VOLATILE" and "AUTOSAVE" to the existing ones.

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
No journaling file system can recover your data if the disk dies.
                                 - Steve Rago in <D4Cw1p.L9E at plc.com>