Re: [PATCH v2 3/5] livepatch/klp-build: switch to GNU patch and recountdiff

[Joe Lawrence <joe.lawrence@redhat.com>]

On Wed, Feb 04, 2026 at 10:35:07AM -0800, Josh Poimboeuf wrote:
> On Tue, Feb 03, 2026 at 09:51:38PM -0500, Joe Lawrence wrote:
> > I think this does simplify things, but:
> > 
> >   - introduces a dependency on patchutil's recountdiff
> 
> I was wondering if we could instead just update fix-patch-lines to fix
> the line numbers/counts, but I get the feeling that would require
> rewriting the whole script and may not be worth the complexity.  That
> script is nice and simple and robust at the moment.
> 

Ok, I'll take a look at fix-patch-lines and see how complicated it might
turn out to incorporate a recount.

> >   - requires goofy epoch timestamp filtering as `diff -N` doesn't use
> >     the `git diff` /dev/null, but a localized beginining of time epoch
> >     that may be 1969 or 1970 depending on the local timezone
> >   - can be *really* chatty, for example:
> > 
> >   Validating patch(es)
> >   patching file fs/proc/cmdline.c
> >   Hunk #1 succeeded at 7 (offset 1 line).
> >   Fixing patch(es)
> >   patching file fs/proc/cmdline.c
> >   Hunk #1 succeeded at 7 (offset 1 line).
> >   patching file fs/proc/cmdline.c
> >   patching file fs/proc/cmdline.c
> >   Building patched kernel
> >   Copying patched object files
> >   Diffing objects
> >   vmlinux.o: changed function: override_release
> >   vmlinux.o: changed function: cmdline_proc_show
> >   Building patch module: livepatch-cmdline-string.ko
> >   SUCCESS
> > 
> >   My initial thought was that I'd only be interested in knowing about
> >   patch offset/fuzz during the validation phase.  And in the interest of
> >   clarifying some of the output messages, it would be nice to know the
> >   patch it was referring to, so how about a follow up patch
> >   pretty-formatting with some indentation like:
> > 
> >   Validating patch(es)
> >     cmdline-string.patch
> >       patching file fs/proc/cmdline.c
> >       Hunk #1 succeeded at 7 (offset 1 line).
> >   Fixing patch(es)
> >   Building patched kernel
> >   Copying patched object files
> >   Diffing objects
> >   vmlinux.o: changed function: override_release
> >   vmlinux.o: changed function: cmdline_proc_show
> >   Building patch module: livepatch-cmdline-string.ko
> >   SUCCESS
> > 
> >   That said, Song suggested using --silent across the board, so maybe
> >   tie that into the existing --verbose option?
> 
> Hm.  Currently we go to considerable effort to make klp-build's output
> as concise as possible, which is good.  On the other hand, it might be
> important to know the patch has fuzz.
> 

To keep it succinct, the script could check for offset/fuzz and only
report it, including the "patching file ..." part, if there is any.

> I'm thinking I would agree that maybe it should be verbose when
> validating patches and silent elsewhere.  And the pretty formatting is a
> nice upgrade to that.
> 

In the past I've used a little function like:

  indent() {
      local num="${1:-0}"
      sed "s/^/$(printf '%*s' "$num" '')/"
  }

so I could just pipe in echo or command output like: `./cmd | indent 2`.
Good enough or maybe you have one?

> We might also consider indenting the outputs of the other steps.  For
> example:
> 
>   Building patched kernel
>     vmlinux.o: some warning
>   Copying patched object files
>   Diffing objects
>     vmlinux.o: changed function: override_release
>     vmlinux.o: changed function: cmdline_proc_show
> 
> Or alternatively, print the step names in ASCII bold or something.
> 

While I do kinda like the recent color coded output from the compilers,
I don't know if I'm ready for a full-color livepatch build experience :D

I wouldn't be against it, but my vote leans towards the indentation
since it leaves prettier log files, even if the color codes are filtered
out.  Then again, the color scheme bikeshedding we could look forward
to!

> >  apply_patch() {
> >  	local patch="$1"
> > -	shift
> > -	local extra_args=("$@")
> >  
> >  	[[ ! -f "$patch" ]] && die "$patch doesn't exist"
> >  
> >  	(
> >  		cd "$SRC"
> > -
> > -		# The sed strips the version signature from 'git format-patch',
> > -		# otherwise 'git apply --recount' warns.
> > -		sed -n '/^-- /q;p' "$patch" |
> > -			git apply "${extra_args[@]}"
> > +		# The sed strips the version signature from 'git format-patch'.
> > +		sed -n '/^-- /q;p' "$patch" | \
> > +			patch -p1 --no-backup-if-mismatch -r /dev/null
> 
> Is this still needed now that we don't use git apply --recount?
> 

I'll double check.  I recall having difficulties with recountdiff when
taking these out, but can't reproduce that by hand at the moment.

> > @@ -490,7 +468,7 @@ fix_patches() {
> >  
> >  		cp -f "$old_patch" "$tmp_patch"
> >  		refresh_patch "$tmp_patch"
> > -		"$FIX_PATCH_LINES" "$tmp_patch" > "$new_patch"
> > +		"$FIX_PATCH_LINES" "$tmp_patch" | recountdiff > "$new_patch"
> >  		refresh_patch "$new_patch"
> 
> Do we still need to refresh after the recountdiff?
> 

Yeah I think it's redundant as long as recountdiff doesn't emit
something weird, but if it did, it's probably already screwed up the
patch.

--
Joe