[PATCH] efidebug: Introduce bootmgr command

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Akashi, 

On Wed, Feb 10, 2021 at 09:50:33PM +0900, AKASHI Takahiro wrote:
> On Wed, Feb 10, 2021 at 01:53:55PM +0200, Ilias Apalodimas wrote:
> > Hi Wolfgang,
> > 
> > Thanks for having a look, 
> > 
> > On Wed, Feb 10, 2021 at 12:23:47PM +0100, Wolfgang Denk wrote:
> > > Dear Ilias,
> > > 
> > > In message <20210210105425.356131-1-ilias.apalodimas@linaro.org> you wrote:
> > > > Up to now we've been adding all the efi related configuration to
> > > > 'efidebug' command.  The command name feels a bit weird to configure boot
> > > > manager related commands.  Since the bootmanager is growing and we intend
> 
> I developed the command as a poorman's "efishell" as, at that time,
> EDK2's shell didn't work well on U-Boot UEFI subsystem.
> 
> As far as I remember, Alex (ex-maintainer) didn't like to take this
> command, including "efidebug boot" subcommand, as a standard U-Boot command.
> 

UEFI has grown substantially since then though and we can now use it to
install full distros. Having a command named 'efidebug' is the last place some
would look to configure boot options. Does it only feel weird to me?

> > > > to extend it with features like defining the initrd we want to expose to
> > > > the kernel, it would make sense to split it on a command of it's own.
> > > >
> > > > So let's introduce a new command called bootmgr and move all of the
> > > > existing Boot manager functionality there.
> > > 
> > > As this is EFI specific, I would appreciate to have "efi" in the
> > > command name, too.
> > > 
> > > Maybe all EFi related commands should be collected as "efi <subcommand>"
> > > like we did it with the "env" commands long ago.
> > 
> > We could, I'll discuss this with Heinrich and see what he thinks.
> > 
> > > 
> > > For backward compatibility e. g. 'efidebug' could be kept, but the
> > > new name would be 'efi debug'; likewise, your new command would be
> > > 'efi bootmgr' [or just 'efi boot' ?]
> 
> As a matter of fact, "efi" is even now recognized as "efidebug" thanks to
> command name completion as there is no other "efi*" command.
> 
> Then,
>   efidebug boot ..., and
>   [efi]bootmgr boot ...
> can be invoked literally as
>   efi boot ...
> 
> So I don't see much advantage to Ilias' proposal.
> 

Breaking the code in smaller, readable files, looked like a better option.
I'd much prefer it over a single huge file implementing a variety of
commands completely unrelated to each other, just because 'everything belongs
to the efi spec'.
It would also make our life and readability a lot easier in case we want to
include specific commands on a built. The alternative you propose implies a 
file full of ifdefs, while we could just control them with Kconfig options 
and makefiles.

> My suggestions are:
> * alias "efi" to "efidebug" (if preferred),
> * add new configuration options for efidebug's subcommands,
> * only enable "boot"-related options by default (if needed)
> 
> Personally, I don't like to move the portion of code from one file
> to another since it will break git history.
> 

How? The commit message clearly says "move the bootmgr functions to a diffrent file" 
and the git log on efidebug will clear that up. It will even show the file that was
moved to.

> In addition, I have proposed to make "bootefi bootmgr" a standalone
> command/application, but Heinrich rejected it.

That's a different discussion, but if you go down that road you'd have to move
the 'bootefi selftest' as well?

> Given Ilias' concern(?), I still believe that the change is logical
> and makes more sense.

My concern here is to help people wanting to use the bootmgr. I don't have a
strong opinion on how that eventually can be implemented and that's why I am
open to suggestions.  For the record I do prefer seperate commands, maybe even
prefixed by efi to make the distinction clear between u-boot native and EFI
related commands.

Cheers
/Ilias
> 
> -Takahiro Akashi
> 
> > 
> > The efidebug for boot options wasn't introduced that long ago and I don't
> > think anyone uses it in production.  If someone would want to have it backwards 
> > compatible, please shout and we'll see what we can do, but I'd strongly prefer 
> > replacing it overall. If we truly want backwards compatibility though we must keep
> > efidebug, changing the name to something like 'efi debug' just for the name
> > similarity wouldn't help much as it would break things regardless.
> > 
> > Heinrich feel free to ignore the followup patch fixing the documentation of
> > efidebug.  I'll change the name to something we all agree and fold in the doc
> > changes in v2.
> > 
> > Thanks
> > /Ilias
> > > 
> > > Thanks!
> > > 
> > > 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
> > > In my experience the best way to get something done  is to give it to
> > > someone who is busy.               - Terry Pratchett, _Going_Postal_