[PATCH v1] kexec-tools: Tweak run-time handling of libxenctrl.so

[PATCH v1] kexec-tools: Tweak run-time handling of libxenctrl.so

[Eric DeVolder]

This patch is a follow-on to commit 43d3932e "kexec-tools: Perform
run-time linking of libxenctrl.so". This patch addresses feedback
from Daniel Kiper concerning the lack of a dlclose() and the desire
to make xc_dlhandle static, rather than extern.

In the original patch, the call to dlclose() was omitted, in contrast
to the description in the commit message. This patch inserts the call.
Note that this dynamic linking feature is dependent upon the proper
operation of the RTLD_NODELETE flag to dlopen(), which does work as
advertised on Linux (otherwise the call to dlclose() should be omitted).

This patch also implements Daniel's suggestion to make the
xc_dlhandle variable static.

This patch has been re-tested per scenarios outlined in the original
patch commit message.

Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
---
v1: 23jan2018
 - Implemented feedback from Daniel Kiper
---
 kexec/kexec-xen.c | 13 ++++++++++++-
 kexec/kexec-xen.h |  9 ++++-----
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/kexec/kexec-xen.c b/kexec/kexec-xen.c
index d42a45a..03ea4a8 100644
--- a/kexec/kexec-xen.c
+++ b/kexec/kexec-xen.c
@@ -15,8 +15,18 @@
 #include "crashdump.h"
 
 #ifdef CONFIG_LIBXENCTRL_DL
-void *xc_dlhandle;
+/* The handle from dlopen(), needed by dlsym(), dlclose() */
+static void *xc_dlhandle;
 xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(HYPERCALL_BUFFER_NULL);
+
+void *__xc_dlsym(const char *symbol)
+{
+	if (xc_dlhandle)
+		return dlsym(xc_dlhandle, symbol);
+	else
+		return NULL;
+}
+
 xc_interface *__xc_interface_open(xentoollog_logger *logger,
 				  xentoollog_logger *dombuild_logger,
 				  unsigned open_flags)
@@ -47,6 +57,7 @@ int __xc_interface_close(xc_interface *xch)
 
 		func_t func = (func_t)dlsym(xc_dlhandle, "xc_interface_close");
 		rc = func(xch);
+		dlclose(xc_dlhandle);
 		xc_dlhandle = NULL;
 	}
 
diff --git a/kexec/kexec-xen.h b/kexec/kexec-xen.h
index ffb8743..8955334 100644
--- a/kexec/kexec-xen.h
+++ b/kexec/kexec-xen.h
@@ -6,9 +6,8 @@
 
 #ifdef CONFIG_LIBXENCTRL_DL
 #include <dlfcn.h>
-
-/* The handle from dlopen(), needed by dlsym(), dlclose() */
-extern void *xc_dlhandle;
+/* Lookup symbols in libxenctrl.so */
+void *__xc_dlsym(const char *symbol);
 
 /* Wrappers around xc_interface_open/close() to insert dlopen/dlclose() */
 xc_interface *__xc_interface_open(xentoollog_logger *logger,
@@ -21,13 +20,13 @@ int __xc_interface_close(xc_interface *xch);
 ( \
 	{ dtype value; \
 	typedef dtype (*func_t)(xc_interface *, ...); \
-	func_t func = dlsym(xc_dlhandle, #name); \
+	func_t func = __xc_dlsym(#name); \
 	value = func(args); \
 	value; } \
 )
 #define __xc_data(dtype, name) \
 ( \
-	{ dtype *value = (dtype *)dlsym(xc_dlhandle, #name); value; } \
+	{ dtype *value = (dtype *)__xc_dlsym(#name); value; } \
 )
 
 /* The wrappers around utilized xenctrl.h functions */
-- 
2.7.4


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v1] kexec-tools: Tweak run-time handling of libxenctrl.so

[Daniel Kiper]

Hi Eric,

On Tue, Jan 23, 2018 at 11:39:29AM -0600, Eric DeVolder wrote:
> This patch is a follow-on to commit 43d3932e "kexec-tools: Perform
> run-time linking of libxenctrl.so". This patch addresses feedback
> from Daniel Kiper concerning the lack of a dlclose() and the desire
> to make xc_dlhandle static, rather than extern.
>
> In the original patch, the call to dlclose() was omitted, in contrast
> to the description in the commit message. This patch inserts the call.
> Note that this dynamic linking feature is dependent upon the proper
> operation of the RTLD_NODELETE flag to dlopen(), which does work as
> advertised on Linux (otherwise the call to dlclose() should be omitted).
>
> This patch also implements Daniel's suggestion to make the
> xc_dlhandle variable static.
>
> This patch has been re-tested per scenarios outlined in the original
> patch commit message.

Please split this patch into two separate patches. One for dlclose() change
and one for static xc_dlhandle change. This will help people who will be
doing git bisect in the future. In general it is much appreciated to
provide one logical change per patch.

> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
> ---
> v1: 23jan2018
>  - Implemented feedback from Daniel Kiper
> ---
>  kexec/kexec-xen.c | 13 ++++++++++++-
>  kexec/kexec-xen.h |  9 ++++-----
>  2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/kexec/kexec-xen.c b/kexec/kexec-xen.c
> index d42a45a..03ea4a8 100644
> --- a/kexec/kexec-xen.c
> +++ b/kexec/kexec-xen.c
> @@ -15,8 +15,18 @@
>  #include "crashdump.h"
>
>  #ifdef CONFIG_LIBXENCTRL_DL
> -void *xc_dlhandle;
> +/* The handle from dlopen(), needed by dlsym(), dlclose() */
> +static void *xc_dlhandle;
>  xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(HYPERCALL_BUFFER_NULL);
> +
> +void *__xc_dlsym(const char *symbol)
> +{
> +	if (xc_dlhandle)
> +		return dlsym(xc_dlhandle, symbol);
> +	else
> +		return NULL;

I am not sure why you need this "if (xc_dlhandle)...". I think that
"return dlsym(xc_dlhandle, symbol);" alone is sufficient here.

Daniel

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH v1] kexec-tools: Tweak run-time handling of libxenctrl.so

[Eric DeVolder]

Thanks Daniel, inline responses below.
Eric

On 01/23/2018 12:15 PM, Daniel Kiper wrote:
> Hi Eric,
> 
> On Tue, Jan 23, 2018 at 11:39:29AM -0600, Eric DeVolder wrote:
>> This patch is a follow-on to commit 43d3932e "kexec-tools: Perform
>> run-time linking of libxenctrl.so". This patch addresses feedback
>> from Daniel Kiper concerning the lack of a dlclose() and the desire
>> to make xc_dlhandle static, rather than extern.
>>
>> In the original patch, the call to dlclose() was omitted, in contrast
>> to the description in the commit message. This patch inserts the call.
>> Note that this dynamic linking feature is dependent upon the proper
>> operation of the RTLD_NODELETE flag to dlopen(), which does work as
>> advertised on Linux (otherwise the call to dlclose() should be omitted).
>>
>> This patch also implements Daniel's suggestion to make the
>> xc_dlhandle variable static.
>>
>> This patch has been re-tested per scenarios outlined in the original
>> patch commit message.
> 
> Please split this patch into two separate patches. One for dlclose() change
> and one for static xc_dlhandle change. This will help people who will be
> doing git bisect in the future. In general it is much appreciated to
> provide one logical change per patch.

Done. I've posted a v2 with these as separate patches.

> 
>> Signed-off-by: Eric DeVolder <eric.devolder@oracle.com>
>> ---
>> v1: 23jan2018
>>   - Implemented feedback from Daniel Kiper
>> ---
>>   kexec/kexec-xen.c | 13 ++++++++++++-
>>   kexec/kexec-xen.h |  9 ++++-----
>>   2 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/kexec/kexec-xen.c b/kexec/kexec-xen.c
>> index d42a45a..03ea4a8 100644
>> --- a/kexec/kexec-xen.c
>> +++ b/kexec/kexec-xen.c
>> @@ -15,8 +15,18 @@
>>   #include "crashdump.h"
>>
>>   #ifdef CONFIG_LIBXENCTRL_DL
>> -void *xc_dlhandle;
>> +/* The handle from dlopen(), needed by dlsym(), dlclose() */
>> +static void *xc_dlhandle;
>>   xc_hypercall_buffer_t XC__HYPERCALL_BUFFER_NAME(HYPERCALL_BUFFER_NULL);
>> +
>> +void *__xc_dlsym(const char *symbol)
>> +{
>> +	if (xc_dlhandle)
>> +		return dlsym(xc_dlhandle, symbol);
>> +	else
>> +		return NULL;
> 
> I am not sure why you need this "if (xc_dlhandle)...". I think that
> "return dlsym(xc_dlhandle, symbol);" alone is sufficient here.

I tested and dlsym() can gracefully accept NULL as the handle, and 
return NULL. The dlclose() segfaults with a NULL handle. I've made
the change.

> 
> Daniel
> 

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec