From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Roese <sr@denx.de>
Date: Fri, 13 Mar 2015 08:15:12 +0100
Subject: [U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting
 via SHA256 encrypted password
In-Reply-To: <CAPnjgZ3xP=O+Ad=OoODzX=TBHL-5WoP2C48fMu=Wdo6RRk3K3g@mail.gmail.com>
References: <1426063900-7267-1-git-send-email-sr@denx.de>	<20150311143646.GL32541@bill-the-cat>
 <CAPnjgZ3xP=O+Ad=OoODzX=TBHL-5WoP2C48fMu=Wdo6RRk3K3g@mail.gmail.com>
Message-ID: <55028E80.1050304@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Simon,

On 13.03.2015 03:48, Simon Glass wrote:
>>> This patch adds the feature to only stop the autobooting, and therefor
>>> boot into the U-Boot prompt, when the input string / password matches
>>> a values that is encypted via a SHA256 hash and saved in the environment.
>>>
>>> This feature is enabled by defined these config options:
>>>       CONFIG_AUTOBOOT_KEYED
>>>       CONFIG_AUTOBOOT_STOP_STR_SHA256
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>
>> This is certainly interesting but I think brings us back to a point
>> Simon made a long while back about needing to factor out this code
>> better.  Especially since this adds big long #if-#else-#endif blocks.
>> Can we re-do this so at least have some functions be called out instead?
>> Thanks!
>>
>
> Also if these CONFIG options are in Kconfig (as they should be) then we can use
>
> if (IS_ENABLED(CONFIG_AUTOBOOT_STOP_STR_SHA256))
>
> instead of #ifdef which may improve the code.

Right. I also thought about this. But the resulting code has all the 
functionality extracted into 2 functions:

#if defined(CONFIG_AUTOBOOT_STOP_STR_SHA256)
static int passwd_abort(uint64_t etime)
{
	const char *sha_env_str = getenv("bootstopkeysha256");
	...
}
#else
static int passwd_abort(uint64_t etime)
{
	int abort = 0;
	...
}
#endif

And this function is now called unconditionally:

	...
	abort = passwd_abort(etime);

So there is nothing here that could be simplified by using IS_ENABLED().

I could of course just add this new config option to Kconfig. But with 
all the other related options not in Kconfig (CONFIG_AUTOBOOT_KEYED, 
CONFIG_AUTOBOOT_DELAY_STR...), this doesn't make much sense. So at some 
point all those config options should be moved to Kconfig. Unfortunately 
I don't have the time for this right now. But I'll add it to my list to 
do this at a later time.

Thanks,
Stefan