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; Tue, 25 Jun 2019 12:16:40 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 113C130BB54B; Tue, 25 Jun 2019 19:16:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-100.ams2.redhat.com [10.36.116.100]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1EBB960BE5; Tue, 25 Jun 2019 19:16:24 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 5/7] OvmfPkg/LegacyBiosDxe: Use EfiBootManagerGetBootDescription() To: David Woodhouse , devel@edk2.groups.io Cc: Ray Ni References: <20190625114859.795331-1-dwmw2@infradead.org> <20190625114859.795331-5-dwmw2@infradead.org> <7f917141-3522-c8f0-2f06-b9d5e52a5b58@redhat.com> <76174dc3b41dbc62d9d953522c2474707afea2df.camel@infradead.org> From: "Laszlo Ersek" Message-ID: <3ad47704-b32a-c92f-8779-6384ae75cf31@redhat.com> Date: Tue, 25 Jun 2019 21:16:23 +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: <76174dc3b41dbc62d9d953522c2474707afea2df.camel@infradead.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Tue, 25 Jun 2019 19:16:32 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/25/19 16:10, David Woodhouse wrote: > On Tue, 2019-06-25 at 15:29 +0200, Laszlo Ersek wrote: >> (2) You removed the NULL-initialization altogether, > > Well yeah, you told me the EDK-II coding standards forbid the common > defensive coding technique used in the language called "C", of > initialising the variable where it's declared. I wasn't up for arguing > about it, so I just stopped doing it. > > Now you tell me to put it back, just in a more cumbersome way... OK, > I'll do that too. For the record, I wrote: The edk2 coding style forbids initialization of locals. Please use a separate assignment if necessary. I guess I didn't foresee that you'd decide it wasn't necessary after all :) If I had thought of that, I could have dropped "if necessary". > I'm not sure the answer is going to make me any happier... but *why* > are we not allowed to just initialise variables where they're declared? I don't remember. I had simply accepted this guideline years ago and haven't gone back to question it since. There's no time to fight the same old battles again and again. :) >>> + >>> + // >>> + // Truncate Description and convert to ASCII. >>> + // >>> + if (StrLen (Description) >= sizeof (AsciiDescription)) { >>> + Description[sizeof (AsciiDescription) - 1] = L'0'; >> >> (3) Sneaky typo. You mean (and I requested) L'\0'. L'0' is different. :) > > Oops :) > > What was it I said about testing of corner cases when > updating/rebasing? I had explicitly tested this by cutting the size > down to 16 and watching it actually truncate. Obviously I didn't do > *that* again this time round (I have now!) > > Well spotted; thanks! > > I've pushed all the fixes you've just pointed out (thanks again) to my > 'csm' branch. Won't send another series by email today; I'll let the > dust settle and some of the other discussions continue. I agree, let's give Ray and others time to react. My comments have been mostly cosmetic in this round anyway. > When I do repost, I may send the required functional patches 1-3,7 on > their own as I think they're ready to apply as-is, and leave the > cosmetic patches 4-6 in a separate series for further bikeshedding. 1-3,7 only touch OvmfPkg, so I could push those quickly then. Thanks, Laszlo