Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable

["Laszlo Ersek" <lersek@redhat.com>]

On 06/21/19 12:59, David Woodhouse wrote:
> On Thu, 2019-06-20 at 22:35 +0200, Laszlo Ersek wrote:
>>>> I think your note on patch#2 is valuable too and should be captured in
>>>> the commit message body. Please consider formulating it with a bit more
>>>> neutral tone :)
>>>
>>> I would prefer to express that particular concern in 'diff -up' form
>>> when actually fixing it :)
>>>
>>
>> I'm a big fan of detailed commit messages. It's OK (and welcome) to
>> describe current shortcomings even if you intend to fix them later.
> 
> It isn't strictly a shortcoming of that patch itself, it's more of a
> pre-existing shortcoming which is next on my list.
> 
> But keen-eyed reviewers or testers might potentially have said "It's
> all very well exposing all these new drives but they're all just called
> Harddisk". Mentioning it in the note was partly an attempt to avoid
> that, but mostly an attempt to solicit discussion on how to *fix* it.
> 
> Since it appears to have completely failed to achieve the latter, I'll
> try again :)
> 
> As noted, UefiBootManagerLib already has this BmGetBootDescription()
> function, which is internal. Can I just rename it to
> EfiBootManagerGetBootDescription() and export it without having to
> change any specifications first?

I would say "yes", and we certainly have precedent for that. Please see
commit 4ed2440d4415 ("MdeModulePkg/UefiBootManagerLib: Expose
*GetLoadOptionBuffer() API", 2016-05-04).

> Adding a generic way for block devices to report a human-readable
> description in order to kill off all the device-type-specific functions
> in BmBootDescription.c presumably *would* involve actually coordinating
> with UEFI Specifications first?
> 
> But we could consider that a second step. If I make the LegacyBm code
> just call the existing (but renamed) EfiBootManagerGetBootDescription()
> then all the horrid special cases and the specification work that's
> required to fix them are purely an implementation detail in
> EfiBootManagerLib? 

I think exposing EfiBootManagerGetBootDescription() as a public
function, as-is, is a no-brainer, if platforms need it.

*Changing* EfiBootManagerGetBootDescription() is hairier.
UefiBootManagerLib strives for strict spec compliance (and minimalism),
if I remember correctly. However, I'm not a big fan of that approach
myself, and recently, "extend first, standardize second" has seemed more
accepted/tolerated than before. (I'm an active proponent of this latter
approach.)

A new hook into PlatformBootManagerLib might help, either way. Please
see TianoCore#982, and commit range cef7ecf6cdb4..1010873becc5.

So, please ask Ray (CC'd) :)

Thanks
Laszlo