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