Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate' comand to shell

["Carsey, Jaben" <jaben.carsey@intel.com>]

> -----Original Message-----
> From: Marcin Wojtas [mailto:mw@semihalf.com]
> Sent: Wednesday, November 16, 2016 3:58 PM
> To: Carsey, Jaben <jaben.carsey@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; linaro-uefi <linaro-uefi@lists.linaro.org>; Neta Zur
> Hershkovits <neta@marvell.com>; Yehuda Yitschak <yehuday@marvell.com>;
> Haim Boot <hayim@marvell.com>; Jan Dabros <jsd@semihalf.com>; Bartosz
> Szczepanek <bsz@semihalf.com>; edk2-devel-01 <edk2-devel@lists.01.org>; Ni,
> Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [PATCH v6 21/23] Applications/FirmwareUpdate: Add 'fupdate'
> comand to shell
> Importance: High
> 
> Hi Jaben,
> 
> I haven't found RunRegisteredCommand in newest edk2. Can you please point
> exactly what you meant?
> 
I meant add a new function like that.  It does not exist at all now.

> As an alternative I checked ShellCommandRunCommandHandler - this is in fact a
> library helper function and it's nested deep down in RunShellCommand, which is
> for now the only working option.
> ShellCommandRunCommandHandler requires a lot of additional processing of
> the command line. All is done in Application/Shell code.

What's the difference that you need?  I see this:  looks like you would need a command line (which I assume you have), the return value (which I assume you want), and control over whether you want LastError changed based on this (I would suggest not, but...)

RETURN_STATUS
EFIAPI
ShellCommandRunCommandHandler (
  IN CONST CHAR16               *CommandString,
  IN OUT SHELL_STATUS           *RetVal,
  IN OUT BOOLEAN                *CanAffectLE OPTIONAL
  )

The main difference between that and the RunShellCommand is things like file redirection, trimming space, alias replacement, and things that feel very "command-line" processing.  I would think that your TFTP command line would be less needy of fixing up.

> 
> Is there any chance to expose RunShellCommand (or equivalent), so that it can
> be used in a nice way, not with including multi "../../"
> relative path header?
> 
> I expected, that the commands, which are in fact some wrappers or they mix
> multiple others is pretty much of a standard, I'm pretty surprised there are so
> huge difficulties in EDK2. I'm wondering of if there are any other options to be
> used here (unless we accept, what we have for
> now:) ).

I think we have less wrapping and less mixing that it appears at first.  Those that are related are (I believe) in the same lib and as such, just call each other.

> 
> Best regards,
> Marcin
> 
> 2016-11-16 22:48 GMT+01:00 Marcin Wojtas <mw@semihalf.com>:
> > Hi Jaben,
> >
> > Thank you for your input.
> >
> >>
> >> I agree on EBL, but I have very little experience with EBL so I don’t want to
> discuss in detail as I am not the right person without more research.  Specifically,
> my gut reaction is that needing a platform specific boot loader indicates that
> something has already gone wrong on that platform.
> >>
> >> However, this does not seem like a boot loader or an application at all.  this is
> an internal shell command. The goal here seems to be to create a NULL library
> to add a new internal command to the UEFI Shell.  This library gets
> compiled/linked into the shell itself.
> >
> > Indeed, it's nothing similar to the bootloader whatsoever. This
> > command simply enables updating firmware in SPI flash directly from
> > local path or from tftp.
> >
> >>
> >> I feel that we have found a "new" use case that I encountered, but worked
> around in the past because all previous cases involved commands in the same
> library (there are interactions between Reconnect and Disconnect/Connect).
> >
> > Right, however Reconnect is easy, as it simply calls gBS callbacks,
> > whose definition are also in the same location.
> >
> >>
> >> I would say that a new API in the ShellCommandLib that links the UEFI Shell
> Application to the NULL libraries that make up the internal commands would be
> my first choice for implementation.  I would lean to something like the function
> that Marcin already called.  Maybe this?
> >>
> >> EFIAPI
> >> RunRegisteredCommand(
> >>   CHAR16* CommandLine,
> >>   EFI_STATUS *CommandReturnValue
> >> )
> >
> > I'll try this one and let know.
> >
> > Thanks,
> > Marcin