[PATCH] Don't look for kernel version if not running on linux

[PATCH] Don't look for kernel version if not running on linux

[Ulf Hermann]

We don't want to use it, even if it exists.

Signed-off-by: Ulf Hermann <ulf.hermann@qt.io>
---
 libdwfl/ChangeLog              | 5 +++++
 libdwfl/linux-kernel-modules.c | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index de73d79..80346d5 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,5 +1,10 @@
 2017-04-20  Ulf Hermann  <ulf.hermann@qt.io>
 
+	* linux-kernel-modules.c: Always return NULL from kernel_release() on
+	non-linux systems.
+
+2017-04-20  Ulf Hermann  <ulf.hermann@qt.io>
+
 	* dwfl_module_getdwarf.c: Check shnum for 0 before subtracting from
 	it.
 
diff --git a/libdwfl/linux-kernel-modules.c b/libdwfl/linux-kernel-modules.c
index 757eace..381711a 100644
--- a/libdwfl/linux-kernel-modules.c
+++ b/libdwfl/linux-kernel-modules.c
@@ -156,11 +156,15 @@ try_kernel_name (Dwfl *dwfl, char **fname, bool try_debug)
 static inline const char *
 kernel_release (void)
 {
+#ifdef __linux__
   /* Cache the `uname -r` string we'll use.  */
   static struct utsname utsname;
   if (utsname.release[0] == '\0' && uname (&utsname) != 0)
     return NULL;
   return utsname.release;
+#else
+  return NULL;
+#endif
 }
 
 static int
-- 
2.1.4

Re: [PATCH] Don't look for kernel version if not running on linux

[Mark Wielaard]

On Thu, Apr 20, 2017 at 04:08:48PM +0200, Ulf Hermann wrote:
> We don't want to use it, even if it exists.

I am not sure this is really the right place to patch.
The value is retrieved through uname () which is POSIX and so should
be available even on non-GNU/Linux systems. When kernel_release ()
returns NULL the callers expect errno to be set so they can use it
to return a failure code.

Cheers,

Mark

Re: [PATCH] Don't look for kernel version if not running on linux

[Ulf Hermann]

On 04/27/2017 08:02 PM, Mark Wielaard wrote:
> On Thu, Apr 20, 2017 at 04:08:48PM +0200, Ulf Hermann wrote:
>> We don't want to use it, even if it exists.
> 
> I am not sure this is really the right place to patch.
> The value is retrieved through uname () which is POSIX and so should
> be available even on non-GNU/Linux systems. When kernel_release ()
> returns NULL the callers expect errno to be set so they can use it
> to return a failure code.

You can get uname() on Windows through gnulib, but at the cost at linking to additional windows DLLs. It calls gethostname and for that we need to link against ws2_32.dll. That's an unreasonable dependency for getting something we cannot use anyway. I suggest we just set errno to ENOTSUP then.

Ulf

Re: [PATCH] Don't look for kernel version if not running on linux

[Ulf Hermann]

> You can get uname() on Windows through gnulib, but at the cost at
> linking to additional windows DLLs. It calls gethostname and for that
> we need to link against ws2_32.dll. That's an unreasonable dependency
> for getting something we cannot use anyway. I suggest we just set
> errno to ENOTSUP then.

I should clarify this a bit. We only use uname() to obtain the kernel 
version and we only use the kernel version to search for directories 
containing kernel modules. The only operating system where this makes 
sense is linux as we cannot handle anything but linux kernel modules 
there. Therefore there is no point in retrieving the kernel version on 
any other OS.

We might still be able to load linux kernel modules from some directory 
on a different OS, but the automatic detection of that directory can be 
skipped.

Ulf

Re: [PATCH] Don't look for kernel version if not running on linux

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

On Tue, May 02, 2017 at 12:17:24PM +0200, Ulf Hermann wrote:
> > You can get uname() on Windows through gnulib, but at the cost at
> > linking to additional windows DLLs. It calls gethostname and for that
> > we need to link against ws2_32.dll. That's an unreasonable dependency
> > for getting something we cannot use anyway. I suggest we just set
> > errno to ENOTSUP then.
> 
> I should clarify this a bit. We only use uname() to obtain the kernel
> version and we only use the kernel version to search for directories
> containing kernel modules. The only operating system where this makes sense
> is linux as we cannot handle anything but linux kernel modules there.
> Therefore there is no point in retrieving the kernel version on any other
> OS.

Agreed. So committed as attached.

Thanks,

Mark

[-- Attachment #2: 0001-Don-t-look-for-kernel-version-if-not-running-on-linu.patch --]
[-- Type: text/plain, Size: 1669 bytes --]

From e88787f9cd2af5be00aa6f53320cf85f7c08f1f2 Mon Sep 17 00:00:00 2001
From: Ulf Hermann <ulf.hermann@qt.io>
Date: Thu, 20 Apr 2017 16:08:48 +0200
Subject: [PATCH] Don't look for kernel version if not running on linux

We don't want to use it, even if it exists.

Signed-off-by: Ulf Hermann <ulf.hermann@qt.io>
Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 libdwfl/ChangeLog              | 6 ++++++
 libdwfl/linux-kernel-modules.c | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index 1fc9da6..859b2ff 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,4 +1,10 @@
 2017-04-20  Ulf Hermann  <ulf.hermann@qt.io>
+	    Mark Wielaard  <mark@klomp.org>
+
+	* linux-kernel-modules.c: Always return NULL from kernel_release() on
+	non-linux systems and set errno to ENOTSUP.
+
+2017-04-20  Ulf Hermann  <ulf.hermann@qt.io>
 
 	* libdwflP.h: Don't include config.h.
 	* argp-std.c: Include config.h.
diff --git a/libdwfl/linux-kernel-modules.c b/libdwfl/linux-kernel-modules.c
index 893110a..9d0fef2 100644
--- a/libdwfl/linux-kernel-modules.c
+++ b/libdwfl/linux-kernel-modules.c
@@ -156,11 +156,18 @@ try_kernel_name (Dwfl *dwfl, char **fname, bool try_debug)
 static inline const char *
 kernel_release (void)
 {
+#ifdef __linux__
   /* Cache the `uname -r` string we'll use.  */
   static struct utsname utsname;
   if (utsname.release[0] == '\0' && uname (&utsname) != 0)
     return NULL;
   return utsname.release;
+#else
+  /* Used for finding the running linux kernel, which isn't supported
+     on non-linux kernel systems.  */
+  errno = ENOTSUP;
+  return NULL;
+#endif
 }
 
 static int
-- 
2.9.3