On Sat, 27 Feb 2021 13:19:17 +0100 Daniel Kiper <dki...@net-space.pl> wrote:
> On Thu, Feb 18, 2021 at 08:47:01PM -0600, Glenn Washburn wrote: > > This patch series fixes all compile errors due to format string > > issues on grub_error. This was tested against nearly all supported > > platforms successfully. This is important because earlier versions > > of these changes compiled successfully on x86 platforms, but had > > issues on other ones not tested. > > Could you give examples what kind of errors you get when you build > non-x86 platforms? More format string errors showed up when building for non-x86 platforms because code which contained a format string issue, but is not compiled for x86 was then compiled. For example, the diff for grub-core/kern/arm64/dl.c in patch #10. That bug did not show up in my original patch series because I was only building for x86 targets. > > All patches except the last fix actual format string issues. The > > last patch turns format string issues into errors. This is a good > > idea because it will help to prevent introducing new format string > > issues into the code. Since, I presume, Daniel does not do not do a > > build test for all architectures before committing to master, this > > will not ensure that no format string issues get introduced into > > the code. However, it should flush out any format string > > I am confused by these sentence. Anyway, I do build tests of all > patches before committing for all architectures which I am aware of. > I think difference between our results come from difference in build > environments, flags and options to do tests. I hope this did not sound like a criticism or that you missed something due to the way you are build testing. You would have never gotten a build failure for any of these bugs even if you were building for all architectures (which it sounds like you were). I was merely unsure if you did a build test for all known architectures for every update of master. With patch #13 it can be more of an issue if you're not building testing all architectures because of the situation I outlined above in the first paragraph. In the worst case though, GRUB would fail to build for certain non-tested architectures where patches with format string bugs were introduced. Presumably, this would be discovered quickly by the people here who use those architectures. > > issues when the CI build is done. > > > > Many of these changes are fairly obvious. I tried to use the > > PRI*GRUB_*_T macros as much as I could and to not cast arguments. > > There are some notable exceptions. There is some code that uses the > > same grub_error code in both 32 and 64 bit architectures (riscv), > > so casting was needed to force the larger storage type. The second > > to last patch for zfs is still confounding to me as to why the > > macro PRIuGRUB_UINT64_T was not expanding to llu, and yet the > > compiler was saying the argument was a long long unsigned. > > For all the patches except #12 Reviewed-by: Daniel Kiper > <daniel.ki...@oracle.com> > > Though I will probably do not get #13 until #12 is updated and merged. Yes definitely, #13 requires the issue in #12 be addressed in someway. Glenn _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
