Re: [Xen-devel] [livepatch-build-tools 4/4] livepatch-build: Handle newly created object files

["Wieczorkiewicz, Pawel" <wipawel@xxxxxxxxx>]

> On 29. Apr 2019, at 15:53, Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> wrote:
> 
> On 4/8/19 9:32 AM, Pawel Wieczorkiewicz wrote:
>> Up to now the livepatch-build ignores newly created object files.
>> When patch applies new .c file and augments its Makefile to build it
>> the resulting object file is not taken into account for final linking
>> step.
>> Such newly created object files can be detected by comparing patched/
>> and original/ directories and copied over to the output directory for
>> the final linking step.
>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@xxxxxxxxx>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@xxxxxxxxxx>
>> Reviewed-by: Bjoern Doebel <doebel@xxxxxxxxx>
>> Reviewed-by: Norbert Manthey <nmanthey@xxxxxxxxx>
>> ---
>>  livepatch-build | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> diff --git a/livepatch-build b/livepatch-build
>> index 796838c..b43730c 100755
>> --- a/livepatch-build
>> +++ b/livepatch-build
>> @@ -146,6 +146,12 @@ function create_patch()
>>          fi
>>      done
>>  +    NEW_FILES=$(comm -23 <(find patched -type f -name '*.o' | cut -f2- 
>> -d'/' | sort -u) <(find original -type f -name '*.o' | cut -f2- -d'/' | sort 
>> -u))
> 
> The paths should be `patched/xen` and `original/xen` so that only hypervisor 
> changes are processed. It is done this way earlier (see FILES="$(find xen 
> ...)").

ACK. Will fix.

> 
> Since process substitution creates a subshell, it might be simpler to do <(cd 
> patched/xen && find . ...) and then drop the `cut`. Also, the `-u` argument 
> to sort seems unnecessary.

ACK. Will fix.
The -u for sort is just my paranoia.

> 
>> +    for i in $NEW_FILES; do
>> +        cp "patched/$i" "output/$i"
>> +        CHANGED=1
>> +    done
> 
> If the live patch for whatever reason has no "patched" object files and only 
> "new" object files, then it is not going to do anything useful since nothing 
> will use the new functions. This should fail to build with an error. E.g. set 
> NEW=1 above and then later check for CHANGED=0 && NEW=1.
> 

I disagree here. Inline assembly patching can use new functions even without 
having any "patched" objects.
Also hotpatch modules with hooks (functionality I will send upstream soon) can 
use the "new" objects provided
functionality for various reasons (re-registrating a callback for instance, or 
some testing related activities).

> Previously all object files that were linked into the livepatch were from the 
> output of create-diff-object. This change just includes the newly built 
> object files directly. I wonder if there are any issues or subtle differences 
> when doing this? A couple of differences off the top of my head:
> 
> 1) The object file will include _everything_ (e.g. potentially unused 
> functions) while create-diff-object generally only includes the minimum that 
> is needed.

Yes, that’s right. When a patch defines new files (and thereby new object 
files) it should do it wisely.

> 
> 2) Hooks and ignored functions/sections in the new object file would not be 
> processed at all.

Not sure I follow the point here, but hooks and functions properly defined in 
new object files will be
processed and used during link time.

I have a test hotpatch that does that.

> 
> The was previously a patch on xen-devel which took a different approach 
> (https://lists.xenproject.org/archives/html/xen-devel/2017-06/msg03532.html). 
> Perhaps you could look at it and see which approach would be better?

Taking a look. Thanks.

> 
> Thanks,
> -- 
> Ross Lagerwall
> 
> 

Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel