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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#42760): https://edk2.groups.io/g/devel/message/42760
Mute This Topic: https://groups.io/mt/32122467/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
