[U-Boot] [PATCH] autoboot.c: Add feature to stop autobooting via SHA256 encrypted password

[Simon Glass]

Hi Stefan,

On 13 March 2015 at 01:15, Stefan Roese <sr at denx.de> wrote:
> 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 at 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.

Well rather than adding more options, perhaps we should wait until we
get this moved to Kconfig? It's not going to get any easier :-)

Regards,
Simon