From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Tue, 2 Jun 2020 14:47:12 +0200 Subject: [PATCH] env: Add option to only ever append environment In-Reply-To: <20200602124450.GA21630@bill-the-cat> References: <20200529175404.627741-1-marex@denx.de> <7f9cd3ee-6661-4780-90cf-d2e81591a7ce@prevas.dk> <02e0326f-c93c-da9e-e0ec-dd3bc2dc9e4a@prevas.dk> <20200602124450.GA21630@bill-the-cat> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 6/2/20 2:44 PM, Tom Rini wrote: > On Tue, Jun 02, 2020 at 02:05:39PM +0200, Rasmus Villemoes wrote: >> On 02/06/2020 13.04, Marek Vasut wrote: >>> On 6/2/20 8:42 AM, Rasmus Villemoes wrote: >>>> On 29/05/2020 19.54, Marek Vasut wrote: >>>>> +config ENV_APPEND >>>>> + bool "Always append the environment with new data" >>>>> + default n >>>>> + help >>>>> + If defined, the environment hash table is only ever appended with new >>>>> + data, but the existing hash table can never be dropped and reloaded >>>>> + with newly imported data. This may be used in combination with static >>>>> + flags to e.g. to protect variables which must not be modified. >>>>> + >>>>> config ENV_ACCESS_IGNORE_FORCE >>>>> bool "Block forced environment operations" >>>>> default n >>>>> diff --git a/env/env.c b/env/env.c >>>>> index 024d36fdbe..967a9d36d7 100644 >>>>> --- a/env/env.c >>>>> +++ b/env/env.c >>>>> @@ -204,7 +204,9 @@ int env_load(void) >>>>> ret = drv->load(); >>>>> if (!ret) { >>>>> printf("OK\n"); >>>>> +#if !CONFIG_IS_ENABLED(ENV_APPEND) >>>>> return 0; >>>>> +#endif >>>> >>>> Don't use CONFIG_IS_ENABLED() unless you actually introduce both >>>> CONFIG_FOO and CONFIG_SPL_FOO. Otherwise the above >>>> CONFIG_IS_ENABLED(ENV_APPEND) is guaranteed to evaluate to false in SPL. >>>> Of course that only matters if environment support is enabled in SPL, >>>> but some actually use that. >>> >>> We actually want to use CONFIG_IS_ENABLED() as much as possible to make >>> these options future-proof, so that others won't have to chase down all >>> kinds of #ifdef CONFIG stuff and fix it later on for SPL/TPL/etc. >>> >> >> That makes no sense. You're introducing something whose help text >> doesn't spell out that the option only applies to U-Boot proper, and is >> completely ignored in SPL (since CONFIG_SPL_ENV_APPEND never exists). >> The reason it's ignored in SPL is that you use the SPL-or-not-SPL-aware >> CONFIG_IS_ENABLED() helper, and you say that's so that somebody in the >> future can implement CONFIG_SPL_ENV_APPEND? >> >> If you intend for ENV_APPEND to be something that's either set or not >> set for a given board, then the code needs to use the SPL-agnostic >> IS_ENABLED(CONFIG_ENV_APPEND). If you intend it to be something that can >> be set independently for the env support in SPL vs U-Boot proper, you >> need to add both config options and, as you do, use CONFIG_IS_ENABLED. > > How will this code behave if there is a mismatch between SPL and full > U-Boot (disabled SPL, enabled full, as the patch stands today) ? One will append the environment, the other will override it (if you have multiple envs enabled).