diff mbox series

[livepatch-build-tools,4/4] livepatch-build: Handle newly created object files

Message ID 20190408083224.104802-4-wipawel@amazon.de (mailing list archive)
State New, archived
Headers show
Series [livepatch-build-tools,1/4] livepatch-gcc: Allow toolchain command with versions | expand

Commit Message

Wieczorkiewicz, Pawel April 8, 2019, 8:32 a.m. UTC 
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@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
---
 livepatch-build | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Andrew Cooper April 29, 2019, 12:53 p.m. UTC | #1 
On 08/04/2019 09:32, 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@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>

This looks to resolve:

https://xenproject.atlassian.net/browse/XEN-90

(I think?)

~Andrew
Ross Lagerwall April 29, 2019, 1:53 p.m. UTC | #2 
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@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> ---
>   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 ...)").

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.

> +    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.

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.

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

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?

Thanks,
Ross Lagerwall April 29, 2019, 1:53 p.m. UTC | #3 
On 4/29/19 1:53 PM, Andrew Cooper wrote:
> On 08/04/2019 09:32, 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@amazon.de>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> 
> This looks to resolve:
> 
> https://xenproject.atlassian.net/browse/XEN-90
> 
> (I think?)
> 

Yes, I expect it would.
Wieczorkiewicz, Pawel April 29, 2019, 3:43 p.m. UTC | #4 
> On 29. Apr 2019, at 15:53, Ross Lagerwall <ross.lagerwall@citrix.com> 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@amazon.de>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
>> ---
>>  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
diff mbox series

Patch

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))
+    for i in $NEW_FILES; do
+        cp "patched/$i" "output/$i"
+        CHANGED=1
+    done
+
     if [[ $ERROR -ne 0 ]]; then
         die "$ERROR error(s) encountered"
     fi