Re: [MODALTS v0.1 1/4] tools/depmod.c: use symbol alternatives for modules.dep

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, sorry for the huge delay on getting to this.

On Fri, May 10, 2024 at 04:21:25AM GMT, Valerii Chernous wrote:
Add depmod cmd line switch -D to extend symbol search dependencies algorithm
Currently, depmod use only one symbol provider into symbols hash table
So if some symbol exported by different modules only one symbol provider
will be presented into symbols hash table(the latest found during modules parsing)
All other alternatives will be deleted from hash table
As result depmod can point dependency to module different correspond to build time dependencies
and create invalid dependency
To avoid this issue "-D" flag extend items of symbols hash table to symbol list,
check build time depencies and choissing proper symbol alternative if found
corresponding module name into available modules list
In case if no corresponding found it use latest found symbol alternative as previous alorithm


I'm trying to understand what exactly you are trying to do.
Can you provide an example? Is this required for something new in the
kernel or external modules, or what?

There can only ever be one symbol provider for calculating module
dependencies because there can only ever be one such symbol in runtime.
depmod will use the priority from the locations as detailed in
depmod.d(5)

What I'm really missing is a cover letter to this patch series
so we can understand the use case before trying to dive into how it was
implemented.

Lucas De Marchi


Cc: xe-linux-external@xxxxxxxxx
Cc: Valerii Chernous <vchernou@xxxxxxxxx>
Signed-off-by: Valerii Chernous <vchernou@xxxxxxxxx>
---
libkmod/libkmod-internal.h |   5 ++
libkmod/libkmod-module.c   |   5 --
shared/util.c              |  20 ++++++
shared/util.h              |   1 +
tools/depmod.c             | 144 +++++++++++++++++++++++++++++++++----
5 files changed, 155 insertions(+), 20 deletions(-)

diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 3bc6e11..0a274e7 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -68,6 +68,11 @@ enum kmod_file_compression_type {
	KMOD_FILE_COMPRESSION_ZLIB,
};

+struct kmod_module_info {
+        char *key;
+        char value[];
+};
+
struct kmod_list *kmod_list_append(struct kmod_list *list, const void *data) _must_check_ __attribute__((nonnull(2)));
struct kmod_list *kmod_list_prepend(struct kmod_list *list, const void *data) _must_check_ __attribute__((nonnull(2)));
struct kmod_list *kmod_list_remove(struct kmod_list *list) _must_check_;
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index d309948..2cdec34 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -2261,11 +2261,6 @@ static struct kmod_elf *kmod_module_get_elf(const struct kmod_module *mod)
	return kmod_file_get_elf(mod->file);
}

-struct kmod_module_info {
-	char *key;
-	char value[];
-};
-
static struct kmod_module_info *kmod_module_info_new(const char *key, size_t keylen, const char *value, size_t valuelen)
{
	struct kmod_module_info *info;
diff --git a/shared/util.c b/shared/util.c
index e2bab83..ea4b9c0 100644
--- a/shared/util.c
+++ b/shared/util.c
@@ -546,3 +546,23 @@ unsigned long long stat_mstamp(const struct stat *st)
	return (unsigned long long) st->st_mtime;
#endif
}
+
+int str_in_coma_separated_str_list(const char *where, const char *what)
+{
+	char *p = strstr(where, what);
+	if (p) {
+		int is_head_ok, is_tail_ok;
+		size_t l = strlen(what);
+		if (p == where || (p > where && *(p-1) == ','))
+			is_head_ok = 1;
+		else
+			is_head_ok = 0;
+		if (*(p+l) == 0 || *(p+l) == ',')
+			is_tail_ok = 1;
+		else
+			is_tail_ok = 0;
+		if (is_head_ok == 1 && is_tail_ok == 1)
+			return p - where;
+	}
+	return -1;
+}
diff --git a/shared/util.h b/shared/util.h
index c4a3916..4337bd6 100644
--- a/shared/util.h
+++ b/shared/util.h
@@ -17,6 +17,7 @@
#define strstartswith(a, b) (strncmp(a, b, strlen(b)) == 0)
char *strchr_replace(char *s, char c, char r);
void *memdup(const void *p, size_t n) __attribute__((nonnull(1)));
+int str_in_coma_separated_str_list(const char *where, const char *what);

/* module-related functions                                                 */
/* ************************************************************************ */
diff --git a/tools/depmod.c b/tools/depmod.c
index 43fc354..24f79c4 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -59,7 +59,7 @@ static const char *const default_cfg_paths[] = {
	NULL
};

-static const char cmdopts_s[] = "aAb:o:C:E:F:euqrvnP:wmVh";
+static const char cmdopts_s[] = "aAb:o:C:E:F:DeuqrvnP:wmVh";
static const struct option cmdopts[] = {
	{ "all", no_argument, 0, 'a' },
	{ "quick", no_argument, 0, 'A' },
@@ -68,6 +68,7 @@ static const struct option cmdopts[] = {
	{ "config", required_argument, 0, 'C' },
	{ "symvers", required_argument, 0, 'E' },
	{ "filesyms", required_argument, 0, 'F' },
+	{ "deps-alternatives", no_argument, 0, 'D' },
	{ "errsyms", no_argument, 0, 'e' },
	{ "unresolved-error", no_argument, 0, 'u' }, /* deprecated */
	{ "quiet", no_argument, 0, 'q' }, /* deprecated */
@@ -95,6 +96,7 @@ static void help(void)
		"Options:\n"
		"\t-a, --all            Probe all modules\n"
		"\t-A, --quick          Only does the work if there's a new module\n"
+		"\t-D, --deps-alternatives	using symbols alternatives for generating modules deps\n"
		"\t-e, --errsyms        Report not supplied symbols\n"
		"\t-n, --show           Write the dependency file on stdout only\n"
		"\t-P, --symbol-prefix  Architecture symbol prefix\n"
@@ -476,6 +478,7 @@ struct cfg {
	uint8_t check_symvers;
	uint8_t print_unknown;
	uint8_t warn_dups;
+	uint8_t use_deps_alternatives;
	struct cfg_override *overrides;
	struct cfg_search *searches;
	struct cfg_external *externals;
@@ -924,10 +927,12 @@ struct mod {
	uint16_t users; /* how many modules depend on this one */
	bool visited; /* helper field to report cycles */
	struct vertex *vertex; /* helper field to report cycles */
+	struct kmod_module_info *deps_from_mod_info;
	char modname[];
};

struct symbol {
+	struct symbol *next;
	struct mod *owner;
	uint64_t crc;
	char name[];
@@ -975,13 +980,40 @@ static int mod_add_dependency(struct mod *mod, struct symbol *sym)
	return 0;
}

-static void symbol_free(struct symbol *sym)
+static void symbol_free_sub(struct symbol *sym)
{
	DBG("free %p sym=%s, owner=%p %s\n", sym, sym->name, sym->owner,
	    sym->owner != NULL ? sym->owner->path : "");
	free(sym);
}

+static void symbol_free(struct symbol *sym)
+{
+	struct symbol *sym_i, *sym_i_tmp;
+	sym_i = sym;
+	while (sym_i != NULL) {
+		sym_i_tmp = sym_i;
+		sym_i = sym_i->next;
+		symbol_free_sub(sym_i_tmp);
+	}
+}
+
+static struct kmod_module_info *depmod_get_mod_info(struct mod *mod, const char *key)
+{
+	struct kmod_list *l;
+	struct kmod_module_info *rval = NULL;
+
+	kmod_list_foreach(l, mod->info_list) {
+		struct kmod_module_info *info;
+		info = (struct kmod_module_info *)l->data;
+		if (strcmp(info->key, key) == 0 ) {
+			rval = info;
+			break;
+		}
+	}
+	return rval;
+}
+
static int depmod_init(struct depmod *depmod, struct cfg *cfg,
							struct kmod_ctx *ctx)
{
@@ -1537,8 +1569,9 @@ static int depmod_symbol_add(struct depmod *depmod, const char *name,
					const struct mod *owner)
{
	size_t namelen;
-	int err;
	struct symbol *sym;
+	int err = 0;
+	struct symbol *sym_l = NULL, *sym_li = NULL;

	if (!prefix_skipped && (name[0] == depmod->cfg->sym_prefix))
		name++;
@@ -1548,20 +1581,49 @@ static int depmod_symbol_add(struct depmod *depmod, const char *name,
	if (sym == NULL)
		return -ENOMEM;

+	sym->next = NULL;
	sym->owner = (struct mod *)owner;
	sym->crc = crc;
	memcpy(sym->name, name, namelen);

-	err = hash_add(depmod->symbols, sym->name, sym);
-	if (err < 0) {
-		free(sym);
-		return err;
+	if (depmod->cfg->use_deps_alternatives == 0) {
+	    err = hash_add(depmod->symbols, sym->name, sym);
+	    if (err < 0)
+		goto err_ext;
+	} else {
+	    sym_l = hash_find(depmod->symbols, sym->name);
+	    for (sym_li = sym_l; sym_li != NULL; sym_li = sym_li->next)
+		if (sym_li->crc == sym->crc && sym_li->owner == sym->owner)
+		    break;
+	    if (sym_li != NULL)
+		// symbol already in the list
+		goto clr_ext;
+	    else {
+		if (sym_l != NULL) {
+		    // insert new sym at second pos to left start list pointer from
+		    // hash without changes
+		    sym->next = sym_l->next;
+		    sym_l->next = sym;
+		} else {
+		    // new symbol
+		    err = hash_add(depmod->symbols, sym->name, sym);
+		    if (err < 0)
+			goto err_ext;
+		}
+	    }
	}

-	DBG("add %p sym=%s, owner=%p %s\n", sym, sym->name, owner,
+	DBG("add %p sym=%s,crc(%#"PRIx64"), owner=%p %s\n", sym, name, crc, owner,
	    owner != NULL ? owner->path : "");

	return 0;
+
+err_ext:
+	ERR("Failed to add %p sym=%s,crc(%#"PRIx64"), owner=%p %s\n", sym, name, crc, owner,
+	        owner != NULL ? owner->path : "");
+clr_ext:
+	symbol_free(sym);
+	return err;
}

static struct symbol *depmod_symbol_find(const struct depmod *depmod,
@@ -1571,7 +1633,7 @@ static struct symbol *depmod_symbol_find(const struct depmod *depmod,
		name++;
	if (name[0] == depmod->cfg->sym_prefix)
		name++;
-	return hash_find(depmod->symbols, name);
+	return (struct symbol *)hash_find(depmod->symbols, name);
}

static int depmod_load_modules(struct depmod *depmod)
@@ -1615,6 +1677,40 @@ load_info:
	return 0;
}

+static struct symbol *depmod_symbol_get_primary(const struct depmod *depmod,
+							const char *name,
+							struct mod *mod)
+{
+	struct symbol *sym, *sym_l;
+	sym_l = depmod_symbol_find(depmod, name);
+	if (sym_l == NULL)
+		sym = NULL;
+	else if (sym_l->next == NULL)
+		sym = sym_l;
+	else {
+		struct symbol *sym_li;
+		if (mod->deps_from_mod_info == NULL && mod->info_list != NULL)
+			mod->deps_from_mod_info = depmod_get_mod_info(mod, "depends");
+		if (mod->deps_from_mod_info == NULL)
+			sym = sym_l;
+		else {
+			sym = NULL;
+			for (sym_li = sym_l; sym_li != NULL; sym_li = sym_li->next) {
+				if (sym_li->owner->modname != NULL &&
+				    str_in_coma_separated_str_list(mod->deps_from_mod_info->value, sym_li->owner->modname) >= 0) {
+					sym = sym_li;
+					break;
+				}
+			}
+			if (sym == NULL) {
+				DBG("Can't find proper owner for symbol: %s use first from list\n", name);
+				sym = sym_l;
+			}
+		}
+	}
+	return sym;
+}
+
static int depmod_load_module_dependencies(struct depmod *depmod, struct mod *mod)
{
	const struct cfg *cfg = depmod->cfg;
@@ -1625,7 +1721,7 @@ static int depmod_load_module_dependencies(struct depmod *depmod, struct mod *mo
		const char *name = kmod_module_dependency_symbol_get_symbol(l);
		uint64_t crc = kmod_module_dependency_symbol_get_crc(l);
		int bindtype = kmod_module_dependency_symbol_get_bind(l);
-		struct symbol *sym = depmod_symbol_find(depmod, name);
+		struct symbol *sym = depmod_symbol_get_primary(depmod, name, mod);
		uint8_t is_weak = bindtype == KMOD_SYMBOL_WEAK;

		if (sym == NULL) {
@@ -2306,12 +2402,27 @@ static int output_symbols(struct depmod *depmod, FILE *out)
	hash_iter_init(depmod->symbols, &iter);

	while (hash_iter_next(&iter, NULL, &v)) {
-		const struct symbol *sym = v;
-		if (sym->owner == NULL)
-			continue;
-
-		fprintf(out, "alias symbol:%s %s\n",
+		if (depmod->cfg->use_deps_alternatives == 0) {
+			const struct symbol *sym = v;
+			if (sym->owner == NULL)
+				continue;
+			fprintf(out, "alias symbol:%s %s\n",
					sym->name, sym->owner->modname);
+		} else {
+			struct symbol *sym_li;
+			int is_printed = 0;
+			for ( sym_li = (struct symbol*)v; sym_li != NULL; sym_li = sym_li->next) {
+				if (sym_li->owner == NULL)
+					continue;
+				if ( is_printed == 0 ) {
+					fprintf(out, "alias symbol:%s", sym_li->name);
+					is_printed = 1;
+				}
+				fprintf(out, " %s", sym_li->owner->modname);
+			}
+			if ( is_printed == 1 )
+				fprintf(out,"\n");
+		}
	}

	return 0;
@@ -2956,6 +3067,9 @@ static int do_depmod(int argc, char *argv[])
			config_paths[n_config_paths] = NULL;
			break;
		}
+		case 'D':
+			cfg.use_deps_alternatives = 1;
+			break;
		case 'E':
			module_symvers = optarg;
			cfg.check_symvers = 1;
--
2.35.6





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux