Re: [PATCH] libkmod-module: Remove directory existence check for KMOD_MODULE_BUILTIN
From: Harish Jenny Kandiga Nagaraj
Date: Thu Feb 19 2015 - 09:03:17 EST
On Thursday 19 February 2015 06:13 PM, Lucas De Marchi wrote:
> On Thu, Feb 19, 2015 at 10:32 AM, Harish Jenny Kandiga Nagaraj
> <harish_kandiga@xxxxxxxxxx> wrote:
>> On Thursday 19 February 2015 04:00 PM, Lucas De Marchi wrote:
>>> On Thu, Feb 19, 2015 at 3:49 AM, Harish Jenny Kandiga Nagaraj
>>> <harish_kandiga@xxxxxxxxxx> wrote:
>>>>> Harrish, in your patch if you just change the "return
>>>>> KMOD_MODULE_BUILTIN;" to "return KMOD_MODULE_COMING;" does it work?
>>>> Yes. Returning KMOD_MODULE_COMING instead of KMOD_MODULE_BUILTIN works. The built-in modules are handled by looking at the modules.builtin index file. Is there any chance of returning KMOD_MODULE_COMING for builti-in modules? If it does not have any impact, then the fix should be fine.
>>> well... you're not returning KMOD_MODULE_COMING for a builtin module.
>>> Having the directory /sys/module/<name> and not the initstate could be
>>> either that the module is builtin or that there's a race while loading
>>> the module and it's in the coming state. However since we use the
>>> index to decide if this module is builtin in the beginning of this
>>> function, here it can only be the second case.
>>>
>>> However... mod->builtin in the beginning of this function is only set
>>> if the module is created by a lookup rather than from name or from
>>> path.... maybe here we need to actually fallback to the index rather
>>> than the cached value, otherwise this test would fail (considering
>>> "vt" is builtin):
>>>
>>> kmod_module_new_from_name(ctx, "vt", &mod);
>>> kmod_module_get_initstate(mod, &state);
>>>
>>
>>
>> something like this ?
>>
>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>> index 19bb2ed..d424f3e 100644
>> --- a/libkmod/libkmod-module.c
>> +++ b/libkmod/libkmod-module.c
>> @@ -99,6 +99,8 @@ struct kmod_module {
>> * "module", except knowing it's builtin.
>> */
>> bool builtin : 1;
>> +
>> + bool lookup : 1;
>> };
>>
>> static inline const char *path_join(const char *path, size_t prefixlen,
>> @@ -215,6 +217,11 @@ void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
>> mod->builtin = builtin;
>> }
>>
>> +void kmod_module_set_lookup(struct kmod_module *mod, bool lookup)
>> +{
>> + mod->lookup = lookup;
>> +}
>> +
>> void kmod_module_set_required(struct kmod_module *mod, bool required)
>> {
>> mod->required = required;
>> @@ -1729,7 +1736,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
>> struct stat st;
>> path[pathlen - (sizeof("/initstate") - 1)] = '\0';
>> if (stat(path, &st) == 0 && S_ISDIR(st.st_mode))
>> - return KMOD_MODULE_COMING;
>> + return mod->lookup ? KMOD_MODULE_COMING : KMOD_MODULE_BUILTIN;
> no. I guess this doesn't pass the proposed test:
>
> 1)
> kmod_module_new_from_name(ctx, "vt", &mod);
> kmod_module_get_initstate(mod, &state);
>
> this must return builtin
>
> 2)
> kmod_module_new_from_lookup(ctx, "vt", &list);
> ... (get mod from list)
> kmod_module_get_initstate(mod, &state);
>
> this must return builtin as well.
>
> I suggest you add a kmod_module_is_builtin() which does the lookup
> (but doesn't increase the module refcount, i.e. doesn't call
> new_module_xxxx()) iff it's not already done. For this you will need
> to change mod->builtin to an enum: enum { XXXX_UNKNOWN, XXXX_NO,
> XXXX_YES } then you do:
>
> bool kmod_module_is_builtin(mod) (don't export this function)
> {
> if (mod->XXXX_UNKNOWN) {
> ... lookup in builtin index
> }
> return mod->builtin == XXXX_YES;
> }
>
> then you change the users of mod->builtin.
>
something like this ?
diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 417f232..f6ffd3e 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -46,6 +46,13 @@ static _always_inline_ _printf_format_(2, 3) void
# endif
#endif
+/* Flags to kmod_builtin_status() */
+enum kmod_builtin_status {
+ KMOD_BUILTIN_UNKNOWN = 0,
+ KMOD_BUILTIN_NO = 1,
+ KMOD_BUILTIN_YES = 2
+};
+
void kmod_log(const struct kmod_ctx *ctx,
int priority, const char *file, int line, const char *fn,
const char *format, ...) __attribute__((format(printf, 6, 7))) __attribute__((nonnull(1, 3, 5)));
@@ -92,6 +99,7 @@ int kmod_lookup_alias_from_symbols_file(struct kmod_ctx *ctx, const char *name,
int kmod_lookup_alias_from_aliases_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
int kmod_lookup_alias_from_moddep_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
+int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name)__attribute__((nonnull(1, 2)));
int kmod_lookup_alias_from_commands(struct kmod_ctx *ctx, const char *name, struct kmod_list **list) __attribute__((nonnull(1, 2, 3)));
void kmod_set_modules_visited(struct kmod_ctx *ctx, bool visited) __attribute__((nonnull((1))));
void kmod_set_modules_required(struct kmod_ctx *ctx, bool required) __attribute__((nonnull((1))));
@@ -143,9 +151,9 @@ int kmod_module_parse_depline(struct kmod_module *mod, char *line) __attribute__
void kmod_module_set_install_commands(struct kmod_module *mod, const char *cmd) __attribute__((nonnull(1)));
void kmod_module_set_remove_commands(struct kmod_module *mod, const char *cmd) __attribute__((nonnull(1)));
void kmod_module_set_visited(struct kmod_module *mod, bool visited) __attribute__((nonnull(1)));
-void kmod_module_set_builtin(struct kmod_module *mod, bool builtin) __attribute__((nonnull((1))));
+void kmod_module_set_builtin(struct kmod_module *mod, enum kmod_builtin_status builtin) __attribute__((nonnull((1))));
void kmod_module_set_required(struct kmod_module *mod, bool required) __attribute__((nonnull(1)));
-
+bool kmod_module_is_builtin(const struct kmod_module *mod);
/* libkmod-file.c */
struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx, const char *filename) _must_check_ __attribute__((nonnull(1,2)));
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 19bb2ed..6b2f2f1 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -98,7 +98,7 @@ struct kmod_module {
* is set. There's nothing much useful one can do with such a
* "module", except knowing it's builtin.
*/
- bool builtin : 1;
+ enum kmod_builtin_status builtin;
};
static inline const char *path_join(const char *path, size_t prefixlen,
@@ -210,7 +210,7 @@ void kmod_module_set_visited(struct kmod_module *mod, bool visited)
mod->visited = visited;
}
-void kmod_module_set_builtin(struct kmod_module *mod, bool builtin)
+void kmod_module_set_builtin(struct kmod_module *mod, enum kmod_builtin_status builtin)
{
mod->builtin = builtin;
}
@@ -220,6 +220,15 @@ void kmod_module_set_required(struct kmod_module *mod, bool required)
mod->required = required;
}
+bool kmod_module_is_builtin(const struct kmod_module *mod)
+{
+ int builtin = mod->builtin;
+
+ if (builtin == KMOD_BUILTIN_UNKNOWN)
+ builtin = kmod_just_lookup_alias_from_builtin_file(mod->ctx, mod->name);
+
+ return mod->builtin == KMOD_BUILTIN_YES;
+}
/*
* Memory layout with alias:
*
@@ -924,7 +933,7 @@ KMOD_EXPORT int kmod_module_apply_filter(const struct kmod_ctx *ctx,
module_is_blacklisted(mod))
continue;
- if ((filter_type & KMOD_FILTER_BUILTIN) && mod->builtin)
+ if ((filter_type & KMOD_FILTER_BUILTIN) && kmod_module_is_builtin(mod))
continue;
node = kmod_list_append(*output, mod);
@@ -1713,7 +1722,7 @@ KMOD_EXPORT int kmod_module_get_initstate(const struct kmod_module *mod)
if (mod == NULL)
return -ENOENT;
- if (mod->builtin)
+ if (kmod_module_is_builtin(mod))
return KMOD_MODULE_BUILTIN;
pathlen = snprintf(path, sizeof(path),
diff --git a/libkmod/libkmod.c b/libkmod/libkmod.c
index 1a5a66b..d79bb12 100644
--- a/libkmod/libkmod.c
+++ b/libkmod/libkmod.c
@@ -525,7 +525,7 @@ int kmod_lookup_alias_from_builtin_file(struct kmod_ctx *ctx, const char *name,
goto finish;
}
- kmod_module_set_builtin(mod, true);
+ kmod_module_set_builtin(mod, KMOD_BUILTIN_YES);
*list = kmod_list_append(*list, mod);
if (*list == NULL)
err = -ENOMEM;
@@ -536,6 +536,39 @@ finish:
return err;
}
+int kmod_just_lookup_alias_from_builtin_file(struct kmod_ctx *ctx,
+ const char *name) {
+ char *line = NULL;
+
+ if (ctx->indexes[KMOD_INDEX_MODULES_BUILTIN]) {
+ DBG(ctx, "use mmaped index '%s' modname=%s\n",
+ index_files[KMOD_INDEX_MODULES_BUILTIN].fn, name);
+ line = index_mm_search(ctx->indexes[KMOD_INDEX_MODULES_BUILTIN], name);
+ } else {
+ struct index_file *idx;
+ char fn[PATH_MAX];
+
+ snprintf(fn, sizeof(fn), "%s/%s.bin", ctx->dirname,
+ index_files[KMOD_INDEX_MODULES_BUILTIN].fn);
+ DBG(ctx, "file=%s modname=%s\n", fn, name);
+
+ idx = index_file_open(fn);
+ if (idx == NULL) {
+ DBG(ctx, "could not open builtin file '%s'\n", fn);
+ goto finish;
+ }
+
+ line = index_search(idx, name);
+ index_file_close(idx);
+ }
+
+ if (line != NULL)
+ return KMOD_BUILTIN_YES;
+
+finish:
+ return KMOD_BUILTIN_NO;
+}
+
char *kmod_search_moddep(struct kmod_ctx *ctx, const char *name)
{
struct index_file *idx;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/