[PATCH v3] kexec-tools: Perform run-time linking of libxenctrl.so
Eric DeVolder
eric.devolder at oracle.com
Wed Jan 17 08:36:39 PST 2018
Responses are inlined below.
Eric
On 01/16/2018 03:39 PM, Daniel Kiper wrote:
> On Fri, Jan 12, 2018 at 03:21:13PM -0600, Eric DeVolder wrote:
>> When kexec is utilized in a Xen environment, it has an explicit
>> run-time dependency on libxenctrl.so. This dependency occurs
>> during the configure stage and when building kexec-tools.
>>
>> When kexec is utilized in a non-Xen environment (either bare
>> metal or KVM), the configure and build of kexec-tools omits
>> any reference to libxenctrl.so.
>
> [...]
>
> Wow! What a long story! Patch looks quite good. Just a few nitpicks.
> If you fix them then you can add Reviewed-by: Daniel Kiper <daniel.kiper at oracle.com > to the next version.
Thanks, I've incorporated all these and added your RB and posted V4.
>
>> Signed-off-by: Eric DeVolder <eric.devolder at oracle.com>
>> ---
>> v1: 29nov2017
>> - Daniel Kiper suggested Debian's libxen package of libraries,
>> but I did not find similar package on most other systems.
>>
>> v2: 14dec2017
>> - Reposted to kexec and xen-devel mailing lists
>>
>> v3: 12jan2018
>> - Incorporated feedback from Daniel Kiper.
>> Configure changes for --with-xen=dl, style problems corrected.
>> Extensive testing of the new mode.
>> ---
>> configure.ac | 27 ++++++++++----
>> kexec/Makefile | 1 +
>> kexec/arch/i386/crashdump-x86.c | 4 +--
>> kexec/arch/i386/kexec-x86-common.c | 4 +--
>> kexec/crashdump-xen.c | 4 +--
>> kexec/kexec-xen.c | 41 ++++++++++++++++++++-
>> kexec/kexec-xen.h | 73 ++++++++++++++++++++++++++++++++++++++
>> 7 files changed, 138 insertions(+), 16 deletions(-)
>> create mode 100644 kexec/kexec-xen.h
>>
>> diff --git a/configure.ac b/configure.ac
>> index 208dc0a..4dab57f 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -167,12 +167,27 @@ if test "$with_xen" = yes ; then
>> AC_CHECK_HEADER(xenctrl.h,
>> [AC_CHECK_LIB(xenctrl, xc_kexec_load, ,
>> AC_MSG_NOTICE([Xen support disabled]))])
>> - if test "$ac_cv_lib_xenctrl_xc_kexec_load" = yes ; then
>> - AC_CHECK_LIB(xenctrl, xc_kexec_status,
>> - AC_DEFINE(HAVE_KEXEC_CMD_STATUS, 1,
>> - [The kexec_status call is available]),
>> - AC_MSG_NOTICE([The kexec_status call is not available]))
>> - fi
>> +fi
>> +
>> +dnl Link libxenctrl.so at run-time rather than build-time
>> +if test "$with_xen" = dl ; then
>> + AC_CHECK_HEADER(dlfcn.h,
>> + [AC_CHECK_LIB(dl, dlopen, ,
>> + AC_MSG_ERROR([Dynamic library linking not available]))],
>> + AC_MSG_ERROR([Dynamic library linking not available]))
>
> I think that this error message should be like this:
> Dynamic library linking header not available
done
>
>> + AC_DEFINE(CONFIG_LIBXENCTRL_DL, 1, [Define to 1 to link libxenctrl.so at run-time rather than build-time])
>> + AC_CHECK_HEADER(xenctrl.h,
>> + [AC_CHECK_LIB(xenctrl, xc_kexec_load,
>> + AC_DEFINE(HAVE_LIBXENCTRL, 1, ), # required define, and prevent -lxenctrl
>> + AC_MSG_NOTICE([Xen support disabled]))])
>> +fi
>> +
>> +dnl Check for the Xen kexec_status hypercall - reachable from --with-xen=yes|dl
>> +if test "$ac_cv_lib_xenctrl_xc_kexec_load" = yes ; then
>> + AC_CHECK_LIB(xenctrl, xc_kexec_status,
>> + AC_DEFINE(HAVE_KEXEC_CMD_STATUS, 1,
>> + [The kexec_status call is available]),
>> + AC_MSG_NOTICE([The kexec_status call is not available]))
>> fi
>>
>> dnl ---Sanity checks
>> diff --git a/kexec/Makefile b/kexec/Makefile
>> index 2b4fb3d..8871731 100644
>> --- a/kexec/Makefile
>> +++ b/kexec/Makefile
>> @@ -36,6 +36,7 @@ dist += kexec/Makefile \
>> kexec/kexec-elf-boot.h \
>> kexec/kexec-elf.h kexec/kexec-sha256.h \
>> kexec/kexec-zlib.h kexec/kexec-lzma.h \
>> + kexec/kexec-xen.h \
>
> Could you fix backslash alignment? Maybe in separate patch. It can be
> part of this series.
Turns out I had tabstop=4 and it should have been tabstop=8. I've
corrected my tabstop and the backslash alignment and everything aligns
now; no additional patch needed.
>
>> kexec/kexec-syscall.h kexec/kexec.h kexec/kexec.8
>>
>> dist += kexec/proc_iomem.c
>> diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
>> index 69a063a..a948d9f 100644
>> --- a/kexec/arch/i386/crashdump-x86.c
>> +++ b/kexec/arch/i386/crashdump-x86.c
>> @@ -44,9 +44,7 @@
>> #include "kexec-x86.h"
>> #include "crashdump-x86.h"
>>
>
> Please remove this blank line...
done
>
>> -#ifdef HAVE_LIBXENCTRL
>> -#include <xenctrl.h>
>> -#endif /* HAVE_LIBXENCTRL */
>> +#include "../../kexec-xen.h"
>>
>
> ...and this... Same things below... This was done to separate
> conditional include from unconditional ones. So, blank lines
> after your patch are no longer needed.
done
>
>> #include "x86-linux-setup.h"
>>
>> diff --git a/kexec/arch/i386/kexec-x86-common.c b/kexec/arch/i386/kexec-x86-common.c
>> index be03618..b44c8b7 100644
>> --- a/kexec/arch/i386/kexec-x86-common.c
>> +++ b/kexec/arch/i386/kexec-x86-common.c
>> @@ -40,9 +40,7 @@
>> #include "../../crashdump.h"
>> #include "kexec-x86.h"
>>
>
> Ditto...
done
>
>> -#ifdef HAVE_LIBXENCTRL
>> -#include <xenctrl.h>
>> -#endif /* HAVE_LIBXENCTRL */
>> +#include "../../kexec-xen.h"
>>
>
> But leave this one...
ok
>
>> /* Used below but not present in (older?) xenctrl.h */
>> #ifndef E820_PMEM
>> diff --git a/kexec/crashdump-xen.c b/kexec/crashdump-xen.c
>> index 60594f6..2e4cbdc 100644
>> --- a/kexec/crashdump-xen.c
>> +++ b/kexec/crashdump-xen.c
>> @@ -18,9 +18,7 @@
>>
>> #include "config.h"
>>
>
> Ditto...
>
done
>> -#ifdef HAVE_LIBXENCTRL
>> -#include <xenctrl.h>
>> -#endif
>> +#include "kexec-xen.h"
>>
>
> But leave this one... And same rules below...
ok
>
>> struct crash_note_info {
>> unsigned long base;
>> diff --git a/kexec/kexec-xen.c b/kexec/kexec-xen.c
>> index 2b448d3..8ec3398 100644
>> --- a/kexec/kexec-xen.c
>> +++ b/kexec/kexec-xen.c
>> @@ -10,10 +10,49 @@
>> #include "config.h"
>>
>> #ifdef HAVE_LIBXENCTRL
>> -#include <xenctrl.h>
>> +#include "kexec-xen.h"
>>
>> #include "crashdump.h"
>>
>> +#ifdef CONFIG_LIBXENCTRL_DL
>> +void *xc_dlhandle;
>> +xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(HYPERCALL_BUFFER_NULL);
>> +xc_interface *__xc_interface_open(xentoollog_logger *logger,
>> + xentoollog_logger *dombuild_logger,
>> + unsigned open_flags)
>> +{
>> + xc_interface *xch = NULL;
>> +
>> + if (!xc_dlhandle)
>> + xc_dlhandle = dlopen("libxenctrl.so", RTLD_NOW | RTLD_NODELETE);
>> +
>> + if (xc_dlhandle) {
>> + typedef xc_interface *(*func_t)(xentoollog_logger *logger,
>> + xentoollog_logger *dombuild_logger,
>> + unsigned open_flags);
>> + func_t func = (func_t)dlsym(xc_dlhandle, "xc_interface_open");
>
> I know your opinion about that definitions but I am not happy with them here.
> However, if maintainer is OK with them I will not insist on changing that. Just
> please add one extra line between definitions above and function call below.
I've added the extra line; and otherwise left the code as is.
>
>> + xch = func(logger, dombuild_logger, open_flags);
>> + }
>> +
>> + return xch;
>> +}
>> +
>> +int __xc_interface_close(xc_interface *xch)
>> +{
>> + int rc = -1;
>> +
>> + if (xc_dlhandle) {
>> + typedef int (*func_t)(xc_interface *xch);
>> + func_t func = (func_t)dlsym(xc_dlhandle, "xc_interface_close");
>
> Same as above... Please add empty line here...
done
>
>> + rc = func(xch);
>> +
>
> ...and I think that you can drop this one here...
done
>
>> + xc_dlhandle = NULL;
>> + }
>> +
>> + return rc;
>> +}
>> +#endif /* CONFIG_LIBXENCTRL_DL */
>> +
>> int xen_kexec_load(struct kexec_info *info)
>> {
>> uint32_t nr_segments = info->nr_segments;
>> diff --git a/kexec/kexec-xen.h b/kexec/kexec-xen.h
>> new file mode 100644
>> index 0000000..50e2b3d
>> --- /dev/null
>> +++ b/kexec/kexec-xen.h
>> @@ -0,0 +1,73 @@
>> +#ifndef KEXEC_XEN_H
>> +#define KEXEC_XEN_H
>> +
>> +#ifdef HAVE_LIBXENCTRL
>
> I would add empty line here...
done
>
>> +#include <xenctrl.h>
>> +
>> +#ifdef CONFIG_LIBXENCTRL_DL
>
> ...and here...
done
>
>> +#include <dlfcn.h>
>> +
>> +/* The handle from dlopen(), needed by dlsym(), dlclose() */
>> +extern void *xc_dlhandle;
>> +
>> +/* Wrappers around xc_interface_open/close() to insert dlopen/dlclose() */
>> +xc_interface *__xc_interface_open(xentoollog_logger *logger,
>> + xentoollog_logger *dombuild_logger,
>> + unsigned open_flags);
>> +int __xc_interface_close(xc_interface *xch);
>> +
>> +/* GCC expression statements for evaluating dlsym() */
>> +#define __xc_call(dtype, name, args...) \
>> +( \
>> + { dtype value; \
>> + typedef dtype (*func_t)(xc_interface *, ...); \
>> + func_t func = dlsym(xc_dlhandle, #name); \
>> + value = func(args); \
>> + value; } \
>
> Please use one tab instead of spaces here.
done (all spaces converted to tabs)
>
>> +)
>> +#define __xc_data(dtype, name) \
>> +( \
>> + { dtype *value = (dtype *)dlsym(xc_dlhandle, #name); \
>> + value; } \
>
> It looks that this code can fit into one line. Could you change that?
> And one tab instead of spaces...
done
>
>> +)
>> +
>> +/* The wrappers around utilized xenctrl.h functions */
>> +#define xc_interface_open(a, b, c) \
>> + __xc_interface_open(a, b, c)
>
> I would put 2 tabs instead of spaces here and below...
done
>
>> +#define xc_interface_close(a) \
>> + __xc_interface_close(a)
>> +#define xc_version(args...) \
>> + __xc_call(int, xc_version, args)
>> +#define xc_get_max_cpus(args...) \
>> + __xc_call(int, xc_get_max_cpus, args)
>> +#define xc_get_machine_memory_map(args...) \
>> + __xc_call(int, xc_get_machine_memory_map, args)
>> +#define xc_kexec_get_range(args...) \
>> + __xc_call(int, xc_kexec_get_range, args)
>> +#define xc_kexec_load(args...) \
>> + __xc_call(int, xc_kexec_load, args)
>> +#define xc_kexec_unload(args...) \
>> + __xc_call(int, xc_kexec_unload, args)
>> +#define xc_kexec_status(args...) \
>> + __xc_call(int, xc_kexec_status, args)
>> +#define xc_kexec_exec(args...) \
>> + __xc_call(int, xc_kexec_exec, args)
>> +#define xc_hypercall_buffer_array_create(args...) \
>> + __xc_call(xc_hypercall_buffer_array_t *, xc_hypercall_buffer_array_create, args)
>> +#define xc__hypercall_buffer_alloc_pages(args...) \
>> + __xc_call(void *, xc__hypercall_buffer_alloc_pages, args)
>> +#define xc__hypercall_buffer_free_pages(args...) \
>> + __xc_call(void , xc__hypercall_buffer_free_pages, args)
>> +#define xc__hypercall_buffer_array_alloc(args...) \
>> + __xc_call(void *, xc__hypercall_buffer_array_alloc, args)
>> +#define xc__hypercall_buffer_array_get(args...) \
>> + __xc_call(void *, xc__hypercall_buffer_array_get, args)
>> +#define xc_hypercall_buffer_array_destroy(args...) \
>> + __xc_call(void *, xc_hypercall_buffer_array_destroy, args)
>> +
>> +#endif /* CONFIG_LIBXENCTRL_DL */
>> +
>
> You can drop this empty line...
done
>
>> +#endif /* HAVE_LIBXENCTRL */
>> +
>
> ...and leave this one...
ok
>
>> +#endif /* KEXEC_XEN_H */
>> +
>
> ...and drop this one...
done
>
> Thanks,
>
> Daniel
>
More information about the kexec
mailing list