From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 24 Jun 2019 15:08:23 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0A1553082B5A; Mon, 24 Jun 2019 22:08:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-226.ams2.redhat.com [10.36.116.226]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8B6265D9C5; Mon, 24 Jun 2019 22:08:10 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 1/2] LegacyBios: set NumberBbsEntries to the size of BbsTable To: David Woodhouse , devel@edk2.groups.io, jljusten@gmail.com, afish@apple.com, Paolo Bonzini References: <9c9d53f5da9c3dec56cff0ff07c399e3034444a1.camel@infradead.org> <584f8af7979d40fd960122fc077fad02ecce09b1.camel@infradead.org> <29111f7d-0f4c-bba6-697d-c061ce1b6488@redhat.com> <3def810039016f9b48deff0526720136a996a7b3.camel@infradead.org> <3f9de0c1-afc3-1984-fe1c-4ea4021974ba@redhat.com> From: "Laszlo Ersek" Cc: Ray Ni Message-ID: <2d61fce0-b8b7-9cad-13ec-31ebd9682d86@redhat.com> Date: Tue, 25 Jun 2019 00:08:09 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Mon, 24 Jun 2019 22:08:13 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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