Linux-Modules Archive on lore.kernel.org
 help / color / Atom feed
* Improvements in search of kernel modules directory
@ 2016-08-16  0:50 Nikolay Amiantov
  2016-08-16  0:50 ` [PATCH 1/4] libkmod: add MODULE_DIR to override " Nikolay Amiantov
                   ` (5 more replies)
  0 siblings, 6 replies; 9+ messages in thread
From: Nikolay Amiantov @ 2016-08-16  0:50 UTC (permalink / raw)
  To: linux-modules; +Cc: Shea Levy

These series of patches add more control over how kernel modules directory is
found:

* Add an environment variable which allows to override kernel modules
  directory;
* Allow to hardcode several paths which are searched in order for `uname -r`
  subdirectory;
* Add a configure option to set those paths, instead of hardcoding
  /lib/modules.

We have used the first patch in NixOS[1] for a long time to point kmod to
kernel modules. While an environment variable is handy and has been solving our
problems, it doesn't cover all our cases. We have two directories:

* /run/current-system/kernel-modules/lib/modules
* /run/booted-system/kernel-modules/lib/modules

, which are symlinked to modules for current system configuration (i.e. after
an update) and the one which the system was booted with. It allows us to both
give users ability to install new modules and have their old kernel modules
accessible until a reboot (which is useful in case of kernel upgrade).

Before those patches the necessary logic (see if kernel modules for current
kernel version are available in current-system, if not then fall back to
booted-system) was implemented as a shell wrapper around kmod, which was
unwieldy and didn't work for applications that use kmod directly. It was
considered better to move this logic to kmod itself. Also, NixOS uses nixpkgs,
a set of distribution-agnostic packages (which run on e.g. Ubuntu and even Mac
OS X where applicable), so it was necessary to preserve /lib/modules as a
fallback directory in kmod for it to work on FHS distributions.

As a result a set of generic patches was written that implement necessary
behaviour while being potentially useful for upstream. Environment variable is
still used in several places (e.g. in automatic generation and running of
virtual machines) and is useful for us even with the rest of those patches.

A home of those patches can be found on GitHub[2] along with some
discussion, as can be related NixOS patches and discussion[3].

Big thanks to Shea Levy for the original patch, extensive code review and
useful advices.

Additionally, while working on those it was discovered that kmod makes use of
PATH_MAX. This constant is considered harmful[4] because it doesn't correspond
to real possible length of filesystem paths. This can be considered a bug, but
in those patches it was decided to follow upstream design decisions wherever
possible and so we also use it here.

[1]: http://nixos.org/
[2]: https://github.com/abbradar/kmod/
[3]: https://github.com/NixOS/nixpkgs/pull/17738/
[4]: http://insanecoding.blogspot.ru/2007/11/pathmax-simply-isnt.html


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/4] libkmod: add MODULE_DIR to override kernel modules directory
  2016-08-16  0:50 Improvements in search of kernel modules directory Nikolay Amiantov
@ 2016-08-16  0:50 ` " Nikolay Amiantov
  2016-08-16  0:50 ` [PATCH 2/4] libkmod: allow hardcoding array of dirname prefixes Nikolay Amiantov
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Nikolay Amiantov @ 2016-08-16  0:50 UTC (permalink / raw)
  To: linux-modules; +Cc: Shea Levy

From: Shea Levy <shea@shealevy.com>

It is used as a path if this environment variable is set, and `uname -r` is
appended to the end. Otherwise, default /lib/modules/`uname -r` is used as
usual.
---
 libkmod/libkmod.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index 69fe431..7b0247f 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c
@@ -211,7 +211,7 @@ static const char *dirname_default_prefix = "/lib/modules";
 static char *get_kernel_release(const char *dirname)
 {
 	struct utsname u;
-	char *p;
+	char *p, *dirname_prefix;
 
 	if (dirname != NULL)
 		return path_make_absolute_cwd(dirname);
@@ -219,7 +219,10 @@ static char *get_kernel_release(const char *dirname)
 	if (uname(&u) < 0)
 		return NULL;
 
-	if (asprintf(&p, "%s/%s", dirname_default_prefix, u.release) < 0)
+	if ((dirname_prefix = getenv("MODULE_DIR")) == NULL)
+		dirname_prefix = dirname_default_prefix;
+
+	if (asprintf(&p, "%s/%s", dirname_prefix, u.release) < 0)
 		return NULL;
 
 	return p;
-- 
2.9.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/4] libkmod: allow hardcoding array of dirname prefixes
  2016-08-16  0:50 Improvements in search of kernel modules directory Nikolay Amiantov
  2016-08-16  0:50 ` [PATCH 1/4] libkmod: add MODULE_DIR to override " Nikolay Amiantov
@ 2016-08-16  0:50 ` Nikolay Amiantov
  2016-08-16  0:50 ` [PATCH 3/4] static-nodes: use kmod to get modules directory Nikolay Amiantov
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Nikolay Amiantov @ 2016-08-16  0:50 UTC (permalink / raw)
  To: linux-modules; +Cc: Shea Levy, Nikolay Amiantov

Directories in the array are searched until the first directory with `uname -r`
subdirectory is found. As a fallback last item in the array is used
unconditionally.
---
 libkmod/libkmod.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index 7b0247f..be9358d 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c
@@ -206,7 +206,10 @@ static int log_priority(const char *priority)
 	return 0;
 }
 
-static const char *dirname_default_prefix = "/lib/modules";
+static const char *dirname_default_prefixes[] = {
+	"/lib/modules",
+	NULL
+};
 
 static char *get_kernel_release(const char *dirname)
 {
@@ -219,11 +222,43 @@ static char *get_kernel_release(const char *dirname)
 	if (uname(&u) < 0)
 		return NULL;
 
-	if ((dirname_prefix = getenv("MODULE_DIR")) == NULL)
-		dirname_prefix = dirname_default_prefix;
+	if ((dirname_prefix = getenv("MODULE_DIR")) != NULL) {
+		if(asprintf(&p, "%s/%s", dirname_prefix, u.release) < 0)
+			return NULL;
+	} else {
+		size_t i;
+		char buf[PATH_MAX];
+
+		for (i = 0; dirname_default_prefixes[i] != NULL; i++) {
+			int plen;
+
+			plen = snprintf(buf, sizeof(buf), "%s/%s", dirname_default_prefixes[i], u.release);
+			if (plen < 0)
+				return NULL;
+			else if (plen >= PATH_MAX)
+				continue;
+
+			if (dirname_default_prefixes[i + 1] != NULL) {
+				struct stat dirstat;
+
+				if (stat(buf, &dirstat) < 0) {
+					if (errno == ENOENT)
+						continue;
+					else
+						return NULL;
+				}
+
+				if (!S_ISDIR(dirstat.st_mode))
+					continue;
+			}
 
-	if (asprintf(&p, "%s/%s", dirname_prefix, u.release) < 0)
-		return NULL;
+			p = malloc(plen + 1);
+			if (p == NULL)
+				return NULL;
+			memcpy(p, buf, plen + 1);
+			break;
+		}
+	}
 
 	return p;
 }
-- 
2.9.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/4] static-nodes: use kmod to get modules directory
  2016-08-16  0:50 Improvements in search of kernel modules directory Nikolay Amiantov
  2016-08-16  0:50 ` [PATCH 1/4] libkmod: add MODULE_DIR to override " Nikolay Amiantov
  2016-08-16  0:50 ` [PATCH 2/4] libkmod: allow hardcoding array of dirname prefixes Nikolay Amiantov
@ 2016-08-16  0:50 ` Nikolay Amiantov
  2016-08-16  0:50 ` [PATCH 4/4] libkmod: add --with-modulesdirs configure option Nikolay Amiantov
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 9+ messages in thread
From: Nikolay Amiantov @ 2016-08-16  0:50 UTC (permalink / raw)
  To: linux-modules; +Cc: Shea Levy, Nikolay Amiantov

static-nodes has just used /lib/modules/`uname -r` before with no way to
specify another directory. Instead, make it get the path via kmod, which has a
more sophisticated algorithm for searching the modules directory.

As a side effect, cleanup error messages printing a little.
---
 tools/static-nodes.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/tools/static-nodes.c b/tools/static-nodes.c
index 8d2356d..2ed306d 100644
--- a/tools/static-nodes.c
+++ b/tools/static-nodes.c
@@ -29,10 +29,11 @@
 #include <unistd.h>
 #include <sys/stat.h>
 #include <sys/types.h>
-#include <sys/utsname.h>
 
 #include <shared/util.h>
 
+#include <libkmod/libkmod.h>
+
 #include "kmod.h"
 
 struct static_nodes_format {
@@ -154,8 +155,8 @@ static void help(void)
 
 static int do_static_nodes(int argc, char *argv[])
 {
-	struct utsname kernel;
 	char modules[PATH_MAX], buf[4096];
+	struct kmod_ctx *ctx;
 	const char *output = "/dev/stdout";
 	FILE *in = NULL, *out = NULL;
 	const struct static_nodes_format *format = &static_nodes_format_human;
@@ -206,22 +207,25 @@ static int do_static_nodes(int argc, char *argv[])
 		}
 	}
 
-	if (uname(&kernel) < 0) {
-		fputs("Error: uname failed!\n", stderr);
+	ctx = kmod_new(NULL, NULL);
+	if (ctx == NULL) {
+		fprintf(stderr, "Error: failed to create kmod context\n");
 		ret = EXIT_FAILURE;
 		goto finish;
 	}
-
-	snprintf(modules, sizeof(modules), "/lib/modules/%s/modules.devname", kernel.release);
+	if (snprintf(modules, sizeof(modules), "%s/modules.devname", kmod_get_dirname(ctx)) < 0) {
+		fprintf(stderr, "Error: path to modules.devname is too long\n");
+		ret = EXIT_FAILURE;
+		goto finish;
+	}
+	kmod_unref(ctx);
 	in = fopen(modules, "re");
 	if (in == NULL) {
 		if (errno == ENOENT) {
-			fprintf(stderr, "Warning: /lib/modules/%s/modules.devname not found - ignoring\n",
-				kernel.release);
+			fprintf(stderr, "Warning: %s not found - ignoring\n", modules);
 			ret = EXIT_SUCCESS;
 		} else {
-			fprintf(stderr, "Error: could not open /lib/modules/%s/modules.devname - %m\n",
-				kernel.release);
+			fprintf(stderr, "Error: could not open %s - %m\n", modules);
 			ret = EXIT_FAILURE;
 		}
 		goto finish;
-- 
2.9.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 4/4] libkmod: add --with-modulesdirs configure option
  2016-08-16  0:50 Improvements in search of kernel modules directory Nikolay Amiantov
                   ` (2 preceding siblings ...)
  2016-08-16  0:50 ` [PATCH 3/4] static-nodes: use kmod to get modules directory Nikolay Amiantov
@ 2016-08-16  0:50 ` Nikolay Amiantov
  2016-11-11  2:13 ` Improvements in search of kernel modules directory Lucas De Marchi
  2016-12-05  3:24 ` Lucas De Marchi
  5 siblings, 0 replies; 9+ messages in thread
From: Nikolay Amiantov @ 2016-08-16  0:50 UTC (permalink / raw)
  To: linux-modules; +Cc: Shea Levy, Nikolay Amiantov

Let the user override default /lib/modules path. One can also
define several directories to be looked in order by specifying them
separated with a colon, like this:

./configure --with-modulesdirs=/lib/modules:/usr/local/lib/modules
---
 Makefile.am       | 1 +
 configure.ac      | 6 ++++++
 libkmod/libkmod.c | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Makefile.am b/Makefile.am
index d4eeb7e..5c9f603 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -19,6 +19,7 @@ AM_CPPFLAGS = \
 	-include $(top_builddir)/config.h \
 	-I$(top_srcdir) \
 	-DSYSCONFDIR=\""$(sysconfdir)"\" \
+	-DMODULESDIRS=\""$(shell echo $(modulesdirs) | $(SED) 's|:|\\",\\"|g')"\" \
 	${zlib_CFLAGS}
 
 AM_CFLAGS = $(OUR_CFLAGS)
diff --git a/configure.ac b/configure.ac
index 23510c8..66490cf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -202,6 +202,12 @@ GTK_DOC_CHECK([1.14],[--flavour no-tmpl-flat])
 ], [
 AM_CONDITIONAL([ENABLE_GTK_DOC], false)])
 
+AC_ARG_WITH([modulesdirs],
+	AS_HELP_STRING([--with-modulesdirs=DIRS], [Kernel modules directories, separated by :]),
+	[],
+	[with_modulesdirs=/lib/modules])
+AC_SUBST([modulesdirs], [$with_modulesdirs])
+
 
 #####################################################################
 # Default CFLAGS and LDFLAGS
diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index be9358d..291c08d 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c
@@ -207,7 +207,7 @@ static int log_priority(const char *priority)
 }
 
 static const char *dirname_default_prefixes[] = {
-	"/lib/modules",
+	MODULESDIRS,
 	NULL
 };
 
-- 
2.9.2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Improvements in search of kernel modules directory
  2016-08-16  0:50 Improvements in search of kernel modules directory Nikolay Amiantov
                   ` (3 preceding siblings ...)
  2016-08-16  0:50 ` [PATCH 4/4] libkmod: add --with-modulesdirs configure option Nikolay Amiantov
@ 2016-11-11  2:13 ` Lucas De Marchi
  2016-12-05  3:24 ` Lucas De Marchi
  5 siblings, 0 replies; 9+ messages in thread
From: Lucas De Marchi @ 2016-11-11  2:13 UTC (permalink / raw)
  To: Nikolay Amiantov; +Cc: linux-modules, Shea Levy

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

Hi Nikolay,

I'm very sorry to miss these patches and not give an answer soon...
I totally forgot about them. I'll take a look this week.

Lucas De Marchi

On Mon, Aug 15, 2016 at 9:50 PM, Nikolay Amiantov <ab@fmap.me> wrote:

> These series of patches add more control over how kernel modules directory
> is
> found:
>
> * Add an environment variable which allows to override kernel modules
>   directory;
> * Allow to hardcode several paths which are searched in order for `uname
> -r`
>   subdirectory;
> * Add a configure option to set those paths, instead of hardcoding
>   /lib/modules.
>
> We have used the first patch in NixOS[1] for a long time to point kmod to
> kernel modules. While an environment variable is handy and has been
> solving our
> problems, it doesn't cover all our cases. We have two directories:
>
> * /run/current-system/kernel-modules/lib/modules
> * /run/booted-system/kernel-modules/lib/modules
>
> , which are symlinked to modules for current system configuration (i.e.
> after
> an update) and the one which the system was booted with. It allows us to
> both
> give users ability to install new modules and have their old kernel modules
> accessible until a reboot (which is useful in case of kernel upgrade).
>
> Before those patches the necessary logic (see if kernel modules for current
> kernel version are available in current-system, if not then fall back to
> booted-system) was implemented as a shell wrapper around kmod, which was
> unwieldy and didn't work for applications that use kmod directly. It was
> considered better to move this logic to kmod itself. Also, NixOS uses
> nixpkgs,
> a set of distribution-agnostic packages (which run on e.g. Ubuntu and even
> Mac
> OS X where applicable), so it was necessary to preserve /lib/modules as a
> fallback directory in kmod for it to work on FHS distributions.
>
> As a result a set of generic patches was written that implement necessary
> behaviour while being potentially useful for upstream. Environment
> variable is
> still used in several places (e.g. in automatic generation and running of
> virtual machines) and is useful for us even with the rest of those patches.
>
> A home of those patches can be found on GitHub[2] along with some
> discussion, as can be related NixOS patches and discussion[3].
>
> Big thanks to Shea Levy for the original patch, extensive code review and
> useful advices.
>
> Additionally, while working on those it was discovered that kmod makes use
> of
> PATH_MAX. This constant is considered harmful[4] because it doesn't
> correspond
> to real possible length of filesystem paths. This can be considered a bug,
> but
> in those patches it was decided to follow upstream design decisions
> wherever
> possible and so we also use it here.
>
> [1]: http://nixos.org/
> [2]: https://github.com/abbradar/kmod/
> [3]: https://github.com/NixOS/nixpkgs/pull/17738/
> [4]: http://insanecoding.blogspot.ru/2007/11/pathmax-simply-isnt.html
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-modules" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Lucas De Marchi

[-- Attachment #2: Type: text/html, Size: 4354 bytes --]

<div dir="ltr">Hi Nikolay,<div><br></div><div>I&#39;m very sorry to miss these patches and not give an answer soon... </div><div>I totally forgot about them. I&#39;ll take a look this week.</div><div><br></div><div>Lucas De Marchi</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 15, 2016 at 9:50 PM, Nikolay Amiantov <span dir="ltr">&lt;<a href="mailto:ab@fmap.me" target="_blank">ab@fmap.me</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">These series of patches add more control over how kernel modules directory is<br>
found:<br>
<br>
* Add an environment variable which allows to override kernel modules<br>
  directory;<br>
* Allow to hardcode several paths which are searched in order for `uname -r`<br>
  subdirectory;<br>
* Add a configure option to set those paths, instead of hardcoding<br>
  /lib/modules.<br>
<br>
We have used the first patch in NixOS[1] for a long time to point kmod to<br>
kernel modules. While an environment variable is handy and has been solving our<br>
problems, it doesn&#39;t cover all our cases. We have two directories:<br>
<br>
* /run/current-system/kernel-<wbr>modules/lib/modules<br>
* /run/booted-system/kernel-<wbr>modules/lib/modules<br>
<br>
, which are symlinked to modules for current system configuration (i.e. after<br>
an update) and the one which the system was booted with. It allows us to both<br>
give users ability to install new modules and have their old kernel modules<br>
accessible until a reboot (which is useful in case of kernel upgrade).<br>
<br>
Before those patches the necessary logic (see if kernel modules for current<br>
kernel version are available in current-system, if not then fall back to<br>
booted-system) was implemented as a shell wrapper around kmod, which was<br>
unwieldy and didn&#39;t work for applications that use kmod directly. It was<br>
considered better to move this logic to kmod itself. Also, NixOS uses nixpkgs,<br>
a set of distribution-agnostic packages (which run on e.g. Ubuntu and even Mac<br>
OS X where applicable), so it was necessary to preserve /lib/modules as a<br>
fallback directory in kmod for it to work on FHS distributions.<br>
<br>
As a result a set of generic patches was written that implement necessary<br>
behaviour while being potentially useful for upstream. Environment variable is<br>
still used in several places (e.g. in automatic generation and running of<br>
virtual machines) and is useful for us even with the rest of those patches.<br>
<br>
A home of those patches can be found on GitHub[2] along with some<br>
discussion, as can be related NixOS patches and discussion[3].<br>
<br>
Big thanks to Shea Levy for the original patch, extensive code review and<br>
useful advices.<br>
<br>
Additionally, while working on those it was discovered that kmod makes use of<br>
PATH_MAX. This constant is considered harmful[4] because it doesn&#39;t correspond<br>
to real possible length of filesystem paths. This can be considered a bug, but<br>
in those patches it was decided to follow upstream design decisions wherever<br>
possible and so we also use it here.<br>
<br>
[1]: <a href="http://nixos.org/" rel="noreferrer" target="_blank">http://nixos.org/</a><br>
[2]: <a href="https://github.com/abbradar/kmod/" rel="noreferrer" target="_blank">https://github.com/abbradar/<wbr>kmod/</a><br>
[3]: <a href="https://github.com/NixOS/nixpkgs/pull/17738/" rel="noreferrer" target="_blank">https://github.com/NixOS/<wbr>nixpkgs/pull/17738/</a><br>
[4]: <a href="http://insanecoding.blogspot.ru/2007/11/pathmax-simply-isnt.html" rel="noreferrer" target="_blank">http://insanecoding.blogspot.<wbr>ru/2007/11/pathmax-simply-<wbr>isnt.html</a><br>
<br>
--<br>
To unsubscribe from this list: send the line &quot;unsubscribe linux-modules&quot; in<br>
the body of a message to <a href="mailto:majordomo@vger.kernel.org">majordomo@vger.kernel.org</a><br>
More majordomo info at  <a href="http://vger.kernel.org/majordomo-info.html" rel="noreferrer" target="_blank">http://vger.kernel.org/<wbr>majordomo-info.html</a><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature">Lucas De Marchi</div>
</div></div>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Improvements in search of kernel modules directory
  2016-08-16  0:50 Improvements in search of kernel modules directory Nikolay Amiantov
                   ` (4 preceding siblings ...)
  2016-11-11  2:13 ` Improvements in search of kernel modules directory Lucas De Marchi
@ 2016-12-05  3:24 ` Lucas De Marchi
  2016-12-05 12:35   ` Shea Levy
  5 siblings, 1 reply; 9+ messages in thread
From: Lucas De Marchi @ 2016-12-05  3:24 UTC (permalink / raw)
  To: Nikolay Amiantov; +Cc: linux-modules, Shea Levy

Hi,

On Mon, Aug 15, 2016 at 5:50 PM, Nikolay Amiantov <ab@fmap.me> wrote:
> These series of patches add more control over how kernel modules directory is
> found:
>
> * Add an environment variable which allows to override kernel modules
>   directory;
> * Allow to hardcode several paths which are searched in order for `uname -r`
>   subdirectory;
> * Add a configure option to set those paths, instead of hardcoding
>   /lib/modules.
>
> We have used the first patch in NixOS[1] for a long time to point kmod to
> kernel modules. While an environment variable is handy and has been solving our
> problems, it doesn't cover all our cases. We have two directories:

An environment variable for the library IMO is not a good option. If
you take a look on how we separate the responsibility of each
component you will see that we parse environment vars on the tools
(e.g. modprobe) not on the library.

> Additionally, while working on those it was discovered that kmod makes use of
> PATH_MAX. This constant is considered harmful[4] because it doesn't correspond
> to real possible length of filesystem paths. This can be considered a bug, but
> in those patches it was decided to follow upstream design decisions wherever
> possible and so we also use it here.

PATH_MAX is just used as a large enough buffer to hold some filenames,
module paths, etc.  Recently we introduced a new helper APIs
(scratchbuffer) to cover the cases in which we don't know the maximum
size of the buffer... Some places were converted to this new API. Few
free to submit patches using it in places it makes sense (i.e. it's
buggy to use PATH_MAX).

Lucas De Marchi

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Improvements in search of kernel modules directory
  2016-12-05  3:24 ` Lucas De Marchi
@ 2016-12-05 12:35   ` Shea Levy
  2016-12-07  7:06     ` Yauheni Kaliuta
  0 siblings, 1 reply; 9+ messages in thread
From: Shea Levy @ 2016-12-05 12:35 UTC (permalink / raw)
  To: Lucas De Marchi, Nikolay Amiantov; +Cc: linux-modules

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

Hi Lucas,

Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
>
> An environment variable for the library IMO is not a good option. If
> you take a look on how we separate the responsibility of each
> component you will see that we parse environment vars on the tools
> (e.g. modprobe) not on the library.
>

Can you explain a bit why an environment variable is not OK here? The
code path using get_kernel_release is shared by many tools, and as
things currently stand there is already a global setting fixed in this
file. If we were to move this out to the tools, we would have to
duplicate this code in several places and any external tools linked
against kmod (e.g. systemd) will need to be updated as well. By default
users of this interface will not know that they need to check the env
var, because as things stand the code will just work, so it's likely
that new uses, in and out of kmod, will start out not respecting
MODULE_DIR and only after someone notices things aren't working for a
case relying on it will they be changed.

Thanks,
Shea

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Improvements in search of kernel modules directory
  2016-12-05 12:35   ` Shea Levy
@ 2016-12-07  7:06     ` Yauheni Kaliuta
  0 siblings, 0 replies; 9+ messages in thread
From: Yauheni Kaliuta @ 2016-12-07  7:06 UTC (permalink / raw)
  To: Shea Levy; +Cc: Lucas De Marchi, Nikolay Amiantov, linux-modules

Hi, Shea!

>>>>> On Mon, 05 Dec 2016 07:35:51 -0500, Shea Levy  wrote:
 > Lucas De Marchi <lucas.de.marchi@gmail.com> writes:
 >> 
 >> An environment variable for the library IMO is not a good option. If
 >> you take a look on how we separate the responsibility of each
 >> component you will see that we parse environment vars on the tools
 >> (e.g. modprobe) not on the library.
 >> 

 > Can you explain a bit why an environment variable is not OK here? The
 > code path using get_kernel_release is shared by many tools, and as
 > things currently stand there is already a global setting fixed in this
 > file. If we were to move this out to the tools, we would have to
 > duplicate this code in several places and any external tools linked
 > against kmod (e.g. systemd) will need to be updated as well. By default
 > users of this interface will not know that they need to check the env
 > var, because as things stand the code will just work, so it's likely
 > that new uses, in and out of kmod, will start out not respecting
 > MODULE_DIR and only after someone notices things aren't working for a
 > case relying on it will they be changed.

What worries me with the approach is increasing ambiguity and number of
points of syncronization.

I see kernel modules directory location as a part of shared state of
several parties, at least "make modules_install" of kbuild, module builders
like dkms, binary module packages, the kmod tools themself (this is
significant different to variables like LS_COLORS, which affect only the
current program run). Since the possible modules directories are mutually
exclusive (only one is taken in use), they must be somehow in sync, or I
expect some surprises for users, a-la:

- you install a module package, it has it's own idea about the directory
  (most probably hardcoded), but depmod from postinstall script doesn't
  generate working configuration (but doesn't fail as well most probably,
  since it's just working on a different directory);
- in general, result of an operation like "dpkg -i module.deb" depends of
  which user (with what environment) performs it;
...

Something similar about several directories. If we install some package
which installs modules (and creates the dir) into a directory which did not
exist and is checked earlier, then the old modules stop working without
very obvious (as for me) connection with the installed one.

The modules installation tools should be aware somehow about directory,
which is going to be used.

Since, as I mentioned, they are exclusive, there is no analogy with
variables like PATH, where all of the dirs are part of the configuration.

I'm not sure if the concerns are important in the real life.

-- 
WBR,
Yauheni Kaliuta

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-16  0:50 Improvements in search of kernel modules directory Nikolay Amiantov
2016-08-16  0:50 ` [PATCH 1/4] libkmod: add MODULE_DIR to override " Nikolay Amiantov
2016-08-16  0:50 ` [PATCH 2/4] libkmod: allow hardcoding array of dirname prefixes Nikolay Amiantov
2016-08-16  0:50 ` [PATCH 3/4] static-nodes: use kmod to get modules directory Nikolay Amiantov
2016-08-16  0:50 ` [PATCH 4/4] libkmod: add --with-modulesdirs configure option Nikolay Amiantov
2016-11-11  2:13 ` Improvements in search of kernel modules directory Lucas De Marchi
2016-12-05  3:24 ` Lucas De Marchi
2016-12-05 12:35   ` Shea Levy
2016-12-07  7:06     ` Yauheni Kaliuta

Linux-Modules Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-modules/0 linux-modules/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-modules linux-modules/ https://lore.kernel.org/linux-modules \
		linux-modules@vger.kernel.org linux-modules@archiver.kernel.org
	public-inbox-index linux-modules

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-modules


AGPL code for this site: git clone https://public-inbox.org/ public-inbox