Re: [edk2-announce] Re: Soft Feature Freeze starts now for edk2-stable202008

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

On 08/19/20 15:19, Laszlo Ersek wrote:
> On 08/19/20 13:48, Leif Lindholm wrote:
>> (Slightly trimmed recipient list due to different patch being discussed.)
>>
>> So, I can't make this call, because I'm the one who messed up.
>>
>> This patch does exactly what I had requested Abner to do some time
>> back (off-list, unfortunately), and I was *convinced* I gave it an R-b
>> as soon as it hit my inbox - until Abner nudged me about it yesterday.
>>
>> The patch in question is
>> https://edk2.groups.io/g/devel/topic/76021725
> 
> My understanding is:
> 
> (1) there is an external project that consumes the FDT library in
> EmbeddedPkg, meaning the lib class header "EmbeddedPkg/Include/libfdt.h"
> and the lib instance "EmbeddedPkg/Library/FdtLib/FdtLib.inf",
> 
> (2) the lib class header pulls in "fdt.h" and "libfdt_env.h",
> 
> (3) the external project is not edk2-platforms,
> 
> (4) the external project wants -- for some strange reason -- edk2's
> "libfdt_env.h" to provide an strncmp() function (or function-like
> macro), with that particular stncmp() implementation not being needed in
> either edk2-platforms or edk2 itself,
> 
> (5) the patch for adding said strncmp() was posted on Aug 6th (at least
> when viewed from my time zone), i.e., before the SFF,
> 
> (6) it was reviewed 12 days later (within the SFF)
> 
> If my understanding is correct, then I don't see how this patch could be
> considered a bugfix -- even as a feature addition, it seems hardly
> justified to me --, and there would have been ~8 days before the SFF to
> review it.
> 
> I think we should postpone the patch until after the stable tag.

Regarding OpenSBI, as of commit 9d56961b2314, the sbi_strncmp() function
has not been removed, and it seems that sbi_strncmp() is still used /
called in at least some build configurations (where the

lib/utils/libfdt/libfdt_env.h:#define strncmp           sbi_strncmp

definition is supposed to be in effect).

This means that the codebase can not rid itself of sbi_strncmp().

To me this indicates that OpenSBI commit 2cfd2fc90488 ("lib: utils: Use
strncmp in fdt_parse_hart_id()", 2020-07-29) was wrong. It should have
replaced sbi_strcmp() with sbi_strncmp(), not strncmp().

After all, the size limit seems to be the motivation for the entire
change -- put a size limit on the string comparison in
fdt_parse_hart_id(). Commit 2cfd2fc90488 did two things at the same
time: replace the size-unbounded comparison with the size-bounded one,
*and* switch to the standard C function name from the self-contained
function. The former goal looks justifiable, the latter I don't understand.

Now: I realize that, going back to edk2 commit fa4a70633868
("EmbeddedPkg: Added support to libfdt", 2012-09-27), we already have a
bunch of wrappers in "EmbeddedPkg/Include/libfdt_env.h". So I guess
adding "one more" is not inconsistent with those -- even though the lib
instance within edk2 doesn't need the new function.

But it's still a micro-feature whose review should have completed before
the SFF.

Thanks
Laszlo