headers
Alan Jenkins | 4 Dec 2011 12:30

Re: index license in module-init-tools

[CC'd to linux-modules@... as plain text]

On 12/04/11 11:27, Alan Jenkins wrote:
> On 12/03/11 22:58, Lucas De Marchi wrote:
>> We are almost complete now: parsing the config, looking up modules, inserting and deleting modules. We
still need to handle dependencies when inserting and deleting but our priority now is to optimize the
lookup stuff, caching the config/indexes when we start, etc. I welcome you to contribute to this lib if you
are interested. Regards, Lucas De Marchi
>
> Thanks for working on this. I may be able to provide some feedback.
>
> One nitpick:
>
> I'm not sure caching the /indexes/, as a whole, would be worth it.
>
> 1. Hopefully we benefit from the page cache, and the now-ubiquitous boot prefetch implementations.
[Although using stdio with the default buffer size probably means we copy several times more data than
necessary. See setvbuf()].
>
> 2. Caching the root index node sounds nice - don't know whether it'd be measurably useful.
>
> 3. mmap() might work to avoid copying completely. I avoided it for modprobe; my understanding was that for
a one-shot command it would probably be slower. Also the current indexes aren't stored in host byte order,
and the code relies on copying to keep things simple.
>
> I would avoid switching to mmap() without good evidence. In general, my approach would be to leave the core
of index.c alone, because it scares me :). The other two points would be reasonably small tweaks.
>
> Alan
(Continue reading)

Permalink | Reply | 0 comments |
headers
James Hunt | 6 Dec 2011 21:55
Favicon

Problem relating to modprobe not applying a modules kernel parameters at load time

Hi,

I've been investigating an issue in Ubuntu [1] which nominally relates to sysctl. After analysing
the problem [2], it appears that the real problem is that although modprobe allows a modules _module
parameters_ to be set at load time, it does not provide a way to set a modules sysfs _kernel
parameters_ at load time.

So, modules that specify kernel parameters but *not* module parameters, such as the 'bridge' module,
do not get the correct values set when modprobe loads.

The reason is that in Ubuntu we run 'sysctl -ep' as early as possible (for options like
kernel.printk). Any settings in /etc/sysctl.* relating to modules that haven't yet been loaded
cannot of course be applied. Later, the bridge module is loaded via modprobe. However, the bridges
fail to start since their settings are incorrect (as the required settings are in /etc/sysctl.*, but
sysctl has now finished).

One possible solution to the problem would be to mandate that all modules which have configurable
kernel parameters also provide equivalent module parameters to allow those values to be set at load
time.

Another possible solution might be for modprobe to extract the relevant bits from sysctl and apply
them somehow(?)

The approach adopted for Ubuntu currently is to simply call sysctl *twice* - once as early as
possible, and again after all network interfaces are up. This seems like the best we can do
currently, but it isn't perfect since it only fixes the problem for network devices.

I'd be interested in your thoughts on a holistic solution to this issue.

Kind regards,
(Continue reading)

Permalink | Reply | 2 comments |
headers
Alan Jenkins | 6 Dec 2011 23:19

Re: Problem relating to modprobe not applying a modules kernel parameters at load time

On 06/12/11 20:55, James Hunt wrote:
> One possible solution to the problem would be to mandate that all modules which have configurable
> kernel parameters also provide equivalent module parameters to allow those values to be set at load
> time.

Slightly confusing, because those initial values will show up under 
/sys/modules, and in fact some modules allow you to write to those 
parameter files after loading.

It sounds like an easy solution, until you realize that means almost all 
of /proc/sys/net/ (everything apart from /proc/sys/net/core/).

> Another possible solution might be for modprobe to extract the relevant bits from sysctl and apply
> them somehow(?)

There is no way to automatically tell which sysctls belong to which 
modules :(.

> The approach adopted for Ubuntu currently is to simply call sysctl *twice* - once as early as
> possible, and again after all network interfaces are up. This seems like the best we can do
> currently, but it isn't perfect since it only fixes the problem for network devices.
>
> I'd be interested in your thoughts on a holistic solution to this issue.

You can always run it again later, if that's what you need :).

In the bridge case, maybe the networking scripts need to run sysctl?  
You could require the bridge sysctls to live in a specific file, so you 
don't end up doing more than is necessary.  Similarly with sunrpc for 
portmap.
(Continue reading)

Permalink | Reply | 1 comment |
headers
Kay Sievers | 6 Dec 2011 23:44

Re: Problem relating to modprobe not applying a modules kernel parameters at load time

On Tue, Dec 6, 2011 at 23:19, Alan Jenkins
<alan.christopher.jenkins@...> wrote:
> On 06/12/11 20:55, James Hunt wrote:
>>
>> One possible solution to the problem would be to mandate that all modules
>> which have configurable
>> kernel parameters also provide equivalent module parameters to allow those
>> values to be set at load
>> time.
>
>
> Slightly confusing, because those initial values will show up under
> /sys/modules, and in fact some modules allow you to write to those parameter
> files after loading.
>
> It sounds like an easy solution, until you realize that means almost all of
> /proc/sys/net/ (everything apart from /proc/sys/net/core/).
>
>
>> Another possible solution might be for modprobe to extract the relevant
>> bits from sysctl and apply
>> them somehow(?)
>
>
> There is no way to automatically tell which sysctls belong to which modules
> :(.
>
>
>> The approach adopted for Ubuntu currently is to simply call sysctl *twice*
>> - once as early as
(Continue reading)

Permalink | Reply | 0 comments |
headers
Lucas De Marchi | 16 Dec 2011 07:15

Re: [PATCH] Library must use O_CLOEXEC whenever it opens file descriptors

Hi Cristian,

On Fri, Dec 16, 2011 at 3:49 AM, Cristian Rodríguez
<crrodriguez@...> wrote:
>
> Signed-off-by: Cristian Rodríguez <crrodriguez@...>
> ---
>  libkmod/libkmod-config.c |    2 +-
>  libkmod/libkmod-index.c  |    4 ++--
>  libkmod/libkmod-module.c |   10 +++++-----
>  3 files changed, 8 insertions(+), 8 deletions(-)

Patch has been applied, thanks. Small nit: we don't use
"Signed-off-by", so for future patches please do not include it. I
amended the commit so the commit log looks like the others.

Lucas De Marchi
Permalink | Reply | 0 comments |
headers
Lucas De Marchi | 16 Dec 2011 07:18

Re: [PATCH] All C files MUST include config.h before any other file

Hi Cristian,

On Fri, Dec 16, 2011 at 3:56 AM, Cristian Rodríguez
<crrodriguez@...> wrote:
>
> Signed-off-by: Cristian Rodríguez <crrodriguez@...>
> ---
>  libkmod/libkmod-config.c     |    4 ++++
>  libkmod/libkmod-hash.c       |    4 ++++
>  libkmod/libkmod-index.c      |    4 ++++
>  libkmod/libkmod-list.c       |    4 ++++
>  libkmod/libkmod-module.c     |    4 ++++
>  libkmod/libkmod-util.c       |    4 ++++
>  libkmod/libkmod.c            |    4 ++++
>  test/test-blacklist.c        |    4 ++++
>  test/test-get-dependencies.c |    4 ++++
>  test/test-init.c             |    4 ++++
>  test/test-insmod.c           |    4 ++++
>  test/test-loaded.c           |    4 ++++
>  test/test-lookup.c           |    4 ++++
>  test/test-mod-double-ref.c   |    4 ++++
>  test/test-path-from-name.c   |    4 ++++
>  test/test-rmmod.c            |    4 ++++
>  test/test-rmmod2.c           |    4 ++++
>  tools/kmod-insmod.c          |    4 ++++
>  tools/kmod-lsmod.c           |    4 ++++
>  tools/kmod-modprobe.c        |    4 ++++
>  tools/kmod-rmmod.c           |    4 ++++
>  21 files changed, 84 insertions(+), 0 deletions(-)
(Continue reading)

Permalink | Reply | 1 comment |
headers
Cristian Rodríguez | 16 Dec 2011 07:22
Favicon
Gravatar

Re: [PATCH] All C files MUST include config.h before any other file

On 16/12/11 03:18, Lucas De Marchi wrote:

> Are you really having any problem? This is automatically done by
> autotools. Try compiling with 'make V=1', you'll see things like:

Huh, I missed that -include ...
Permalink | Reply | 0 comments |
headers
Cristian Rodríguez | 16 Dec 2011 18:46
Favicon
Gravatar

[PATCH] Open more file descriptors with O_CLOEXEC

---
 libkmod/libkmod-config.c |    4 ++--
 libkmod/libkmod-module.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libkmod/libkmod-config.c b/libkmod/libkmod-config.c
index 1ba14cc..b9ef0cd 100644
--- a/libkmod/libkmod-config.c
+++ b/libkmod/libkmod-config.c
 <at>  <at>  -548,7 +548,7  <at>  <at>  int kmod_config_new(struct kmod_ctx *ctx, struct kmod_config **p_config,
 		}

 		if (S_ISREG(st.st_mode)) {
-			int fd = open(path, O_RDONLY);
+			int fd = open(path, O_RDONLY|O_CLOEXEC);
 			DBG(ctx, "parsing file '%s': %d\n", path, fd);
 			if (fd >= 0)
 				kmod_config_parse(config, fd, path);
 <at>  <at>  -562,7 +562,7  <at>  <at>  int kmod_config_new(struct kmod_ctx *ctx, struct kmod_config **p_config,
 		d = conf_files_list(ctx, &list, path);

 		for (; list != NULL; list = kmod_list_remove(list)) {
-			int fd = openat(dirfd(d), list->data, O_RDONLY);
+			int fd = openat(dirfd(d), list->data, O_RDONLY|O_CLOEXEC);
 			DBG(ctx, "parsing file '%s/%s': %d\n", path,
 				(const char *) list->data, fd);
 			if (fd >= 0)
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 8d70314..80da684 100644
--- a/libkmod/libkmod-module.c
(Continue reading)

Permalink | Reply | 1 comment |
headers
Cristian Rodríguez | 16 Dec 2011 19:27
Favicon
Gravatar

[PATCH] Fix static analyzers warnings

- Reduce the scope of i, child_count
- Fd leak on failure
- Initialize st
---
 libkmod/libkmod-index.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/libkmod/libkmod-index.c b/libkmod/libkmod-index.c
index 7f0c092..c376176 100644
--- a/libkmod/libkmod-index.c
+++ b/libkmod/libkmod-index.c
 <at>  <at>  -258,7 +258,6  <at>  <at>  static struct index_node_f *index_read(FILE *in, uint32_t offset)
 {
 	struct index_node_f *node;
 	char *prefix;
-	int i, child_count = 0;

 	if ((offset & INDEX_NODE_MASK) == 0)
 		return NULL;
 <at>  <at>  -276,6 +275,7  <at>  <at>  static struct index_node_f *index_read(FILE *in, uint32_t offset)
 	if (offset & INDEX_NODE_CHILDS) {
 		char first = read_char(in);
 		char last = read_char(in);
+		int i, child_count = 0;
 		child_count = last - first + 1;

 		node = NOFAIL(malloc(sizeof(struct index_node_f) +
 <at>  <at>  -343,6 +343,7  <at>  <at>  struct index_file *index_file_open(const char *filename)

 	magic = read_long(file);
(Continue reading)

Permalink | Reply | 3 comments |
headers
Mike Frysinger | 16 Dec 2011 19:49
Picon
Favicon
Gravatar

[PATCH] depmod: fix tabs in help output

Don't mix spaces and tabs in the help output so things always line up.

Signed-off-by: Mike Frysinger <vapier@...>
---
 depmod.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/depmod.c b/depmod.c
index a1d2f8c..90c24e4 100644
--- a/depmod.c
+++ b/depmod.c
 <at>  <at>  -380,7 +380,7  <at>  <at>  static void print_usage(const char *name)
 	"\t-P, --symbol-prefix  Architecture symbol prefix\n"
 	"\t-V, --version        Print the release version\n"
 	"\t-v, --verbose        Enable verbose mode\n"
-	"\t-w, --warn		Warn on duplicates\n"
+	"\t-w, --warn           Warn on duplicates\n"
 	"\t-h, --help           Print this usage message\n"
 	"\n"
 	"The following options are useful for people managing distributions:\n"
--

-- 
1.7.6.1
Permalink | Reply | 0 comments |

Gmane