[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 4/4] livepatch: differentiate between old and new build systems
On Thu, Mar 10, 2022 at 06:01:48PM +0000, Andrew Cooper wrote: > On 08/03/2022 14:52, Roger Pau Monne wrote: > > On Tue, Mar 08, 2022 at 02:38:47PM +0000, Andrew Cooper wrote: > >> On 02/03/2022 14:27, Roger Pau Monne wrote: > >>> diff --git a/livepatch-build b/livepatch-build > >>> index 38a92be..656cdac 100755 > >>> --- a/livepatch-build > >>> +++ b/livepatch-build > >>> @@ -98,14 +98,20 @@ function build_special() > >>> > >>> # Build with special GCC flags > >>> cd "${SRCDIR}/xen" || die > >>> - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc > >>> -ffunction-sections -fdata-sections/' Rules.mk > >>> - cp -p arch/x86/Makefile arch/x86/Makefile.bak > >>> - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' > >>> arch/x86/Makefile > >>> - # Restore timestamps to prevent spurious rebuilding > >>> - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > >>> - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" > >>> || die > >>> - sed -i 's/CFLAGS += -nostdinc -ffunction-sections > >>> -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > >>> - mv -f arch/x86/Makefile.bak arch/x86/Makefile > >>> + if grep -q 'nostdinc' Rules.mk; then > >>> + # Support for old build system, attempt to set > >>> -f{function,data}-sections and rebuild > >>> + sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc > >>> -ffunction-sections -fdata-sections/' Rules.mk > >>> + cp -p arch/x86/Makefile arch/x86/Makefile.bak > >>> + sed -i > >>> 's/--section-alignment=0x200000/--section-alignment=0x1000/' > >>> arch/x86/Makefile > >>> + # Restore timestamps to prevent spurious rebuilding > >>> + touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > >>> + make "-j$CPUS" $XEN_DEBUG &> > >>> "${OUTPUT}/build_${name}_compile.log" || die > >>> + sed -i 's/CFLAGS += -nostdinc -ffunction-sections > >>> -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > >>> + mv -f arch/x86/Makefile.bak arch/x86/Makefile > >>> + else > >>> + # -f{function,data}-sections set by CONFIG_LIVEPATCH > >>> + make "-j$CPUS" $XEN_DEBUG &> > >>> "${OUTPUT}/build_${name}_compile.log" || die > >>> + fi > >> This really ought to be the other way around, by spotting the thing we > >> know is good, and then falling back to the heuristics. In light of the > >> updates to the Xen side, something like: > > I'm not sure I agree. I do prefer to spot the 'bad' one, and just > > fallback to expecting Xen to correctly set -f{function,data}-sections > > otherwise. > > > >> if grep -q CC_SPLIT_SECTIONS Kconfig; then > > Because this logic ties us to not moving CC_SPLIT_SECTIONS from being > > defined in xen/Kconfig (or even changing it's name), and gain ties the > > livepatch tools to internal details about the Xen build system. > > It doesn't particularly matter which way around the if/else is. It does > matter that we're choosing based on something relevant. > > nostdinc in Rules.mk has exactly the same amount of "magic string in > magic file" as CC_SPLIT_SECTIONS in Kconfig, but has absolutely nothing > to do with the property we actually care about. > > Really what you actually want is > > if grep -q CC_SPLIT_SECTIONS Kconfig; then > # Xen behaves sensibly > elif grep -q 'nostdinc' Rules.mk; then > # Legacy mess with Rules.mk > else > die "Help with build system divination" > fi > > The "behaves sensibly" case is unlikely to change name and unlikely to > move locations, but each are easy to cope with via `grep -e FOO -e BAR > file1 file2`, and this approach avoids the problem of blindly (and > falsely) assuming that anything which is 4.14 and later splits sections > correctly, and that this will remain true even when someone adds "# use > to have -nostdinc here" to Rules.mk. TBH, I don't find the proposed solution is much better to what's in this patch, and as said I really dislike tying the behavior of the livepatch build tools to heuristics against Xen internal build files - be it a Kconfig or a Makefile. Specially because your proposed approach adds heuristics to detect the 'good' case which should be the default one going forward. A better option might be to just make the 'build adjustments' a command line option that the user can pass to the tools, ie: --build-adjust and let the user decide whether it needs the adjustments or not. If I was a livepatch user myself I would seriously consider picking the linker script changes and backport that to my production version. Thanks, Roger.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |