[RFC PATCH 0/2] Unify symbol access API

[RFC PATCH 0/2] Unify symbol access API

[Yauheni Kaliuta]

Hi!

I'm not very happy with the naming, but how do you like the idea in general?


Yauheni Kaliuta (2):
  libkmod: module: introduce common symbol API.
  libkmod: module: make old symbol access API as wrapper to the new

 libkmod/libkmod-module.c | 387 ++++++++++++++++-----------------------
 libkmod/libkmod.h        |  12 ++
 2 files changed, 171 insertions(+), 228 deletions(-)

-- 
2.19.1

[RFC PATCH 1/2] libkmod: module: introduce common symbol API.

[Yauheni Kaliuta]

Introduce one family of functions to replace the duplicated:

   - kmod_module_version_get_symbol()
   - kmod_module_version_get_crc()
   - kmod_module_symbol_get_symbol()
   - kmod_module_symbol_get_crc()
   - kmod_module_dependency_symbol_get_symbol()
   - kmod_module_dependency_symbol_get_crc()
   - kmod_module_versions_free_list()
   - kmod_module_symbols_free_list()
   - kmod_module_dependency_symbols_free_list()

It reuses kmod_module_symbol structure but extends it with the
"bind" field, keeping the API still the same.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 libkmod/libkmod-module.c | 196 ++++++++++++++++++++++++++++++++++++++-
 libkmod/libkmod.h        |  12 +++
 2 files changed, 203 insertions(+), 5 deletions(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 889f26479a98..f595f40032e0 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -2419,6 +2419,197 @@ KMOD_EXPORT void kmod_module_info_free_list(struct kmod_list *list)
 	}
 }
 
+struct kmod_module_symbol {
+	uint64_t crc;
+	uint8_t bind;
+	char symbol[];
+};
+
+static struct kmod_module_symbol *kmod_module_typed_symbol_new(uint64_t crc, uint8_t bind, const char *symbol)
+{
+	struct kmod_module_symbol *ms;
+	size_t symbollen = strlen(symbol) + 1;
+
+	ms = malloc(sizeof(struct kmod_module_symbol) + symbollen);
+	if (ms == NULL)
+		return NULL;
+
+	ms->crc = crc;
+	ms->bind = bind;
+	memcpy(ms->symbol, symbol, symbollen);
+	return ms;
+}
+
+static void kmod_module_typed_symbol_free(struct kmod_module_symbol *ms)
+{
+	free(ms);
+}
+
+typedef int (*kmod_symbols_getter)(const struct kmod_elf *elf, struct kmod_modversion **array);
+
+static kmod_symbols_getter kmod_module_getter_lookup(const struct kmod_module *mod,
+						     enum kmod_symbol_type type)
+{
+	switch (type) {
+	case KMOD_SYMBOL_VERSIONS:
+		return kmod_elf_get_modversions;
+	case KMOD_SYMBOL_CRC:
+		return kmod_elf_get_symbols;
+	case KMOD_SYMBOL_DEPENDENCY:
+		return kmod_elf_get_dependency_symbols;
+	default:
+		ERR(mod->ctx, "Wrong symbol type requested: %d\n", type);
+	}
+
+	return NULL;
+}
+
+/**
+ * kmod_module_get_typed_symbols:
+ * @mod: kmod module
+ * @type: type of symbols to get:
+ *        KMOD_SYMBOL_VERSIONS
+ *        KMOD_SYMBOL_CRC
+ *        KMDO_SYMBOL_DEPENDENCY
+ * @list: where to return list of module symbols. Use
+ *        kmod_module_typed_symbol_get_symbol(),
+ *        kmod_module_typed_symbol_get_crc() and
+ *        kmod_module_typed_symbol_get_bind() to access the data.
+ *
+ *        After use, free the @list by calling
+ *        kmod_module_typed_symbols_free_list().
+ *
+ * Returns: 0 on success or < 0 otherwise.
+ */
+KMOD_EXPORT int kmod_module_get_typed_symbols(const struct kmod_module *mod, enum kmod_symbol_type type, struct kmod_list **list)
+{
+	struct kmod_elf *elf;
+	struct kmod_modversion *symbols;
+	int i, count, ret = 0;
+	kmod_symbols_getter getter;
+
+	if (mod == NULL || list == NULL)
+		return -ENOENT;
+
+	assert(*list == NULL);
+
+	elf = kmod_module_get_elf(mod);
+	if (elf == NULL)
+		return -errno;
+
+	getter = kmod_module_getter_lookup(mod, type);
+	if (getter == NULL)
+		return -ENOENT;
+
+	count = getter(elf, &symbols);
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		struct kmod_module_symbol *ms;
+		struct kmod_list *n;
+
+		ms = kmod_module_typed_symbol_new(symbols[i].crc,
+						  symbols[i].bind,
+						  symbols[i].symbol);
+		if (ms == NULL) {
+			ret = -errno;
+			kmod_module_typed_symbols_free_list(*list);
+			*list = NULL;
+			goto list_error;
+		}
+
+		n = kmod_list_append(*list, ms);
+		if (n != NULL)
+			*list = n;
+		else {
+			kmod_module_typed_symbol_free(ms);
+			kmod_module_typed_symbols_free_list(*list);
+			*list = NULL;
+			ret = -ENOMEM;
+			goto list_error;
+		}
+	}
+	ret = count;
+
+list_error:
+	free(symbols);
+	return ret;
+}
+
+/**
+ * kmod_module_typed_symbol_get_symbol:
+ * @entry: a list entry representing a kmod module typed symbol
+ *
+ * Get the symbol's name of the entry.
+ *
+ * Returns: the symbol's name of this kmod module symbols on success or NULL
+ * on failure. The string is owned by the list, do not free it.
+ */
+KMOD_EXPORT const char *kmod_module_typed_symbol_get_symbol(const struct kmod_list *entry)
+{
+	struct kmod_module_symbol *ms;
+
+	if (entry == NULL || entry->data == NULL)
+		return NULL;
+
+	ms = entry->data;
+	return ms->symbol;
+}
+
+/**
+ * kmod_module_typed_symbol_get_crc:
+ * @entry: a list entry representing a kmod module symbol
+ *
+ * Get the crc of the symbol.
+ *
+ * Returns: the crc of this kmod module symbol if available, otherwise default to 0.
+ */
+KMOD_EXPORT uint64_t kmod_module_typed_symbol_get_crc(const struct kmod_list *entry)
+{
+	struct kmod_module_symbol *ms;
+
+	if (entry == NULL || entry->data == NULL)
+		return 0;
+
+	ms = entry->data;
+	return ms->crc;
+}
+
+/**
+ * kmod_module_dependency_symbol_get_bind:
+ * @entry: a list entry representing a kmod module symbol
+ *
+ * Get the bind type of the symbol.
+ *
+ * Returns: the bind of this kmod module symbol on success
+ * or < 0 on failure.
+ */
+KMOD_EXPORT int kmod_module_typed_symbol_get_bind(const struct kmod_list *entry)
+{
+	struct kmod_module_symbol *ms;
+
+	if (entry == NULL || entry->data == NULL)
+		return 0;
+
+	ms = entry->data;
+	return ms->bind;
+}
+
+/**
+ * kmod_module_typed_symbols_free_list:
+ * @list: kmod module typed symbols list
+ *
+ * Release the resources taken by @list
+ */
+KMOD_EXPORT void kmod_module_typed_symbols_free_list(struct kmod_list *list)
+{
+	while (list) {
+		kmod_module_typed_symbol_free(list->data);
+		list = kmod_list_remove(list);
+	}
+}
+
 struct kmod_module_version {
 	uint64_t crc;
 	char symbol[];
@@ -2559,11 +2750,6 @@ KMOD_EXPORT void kmod_module_versions_free_list(struct kmod_list *list)
 	}
 }
 
-struct kmod_module_symbol {
-	uint64_t crc;
-	char symbol[];
-};
-
 static struct kmod_module_symbol *kmod_module_symbols_new(uint64_t crc, const char *symbol)
 {
 	struct kmod_module_symbol *mv;
diff --git a/libkmod/libkmod.h b/libkmod/libkmod.h
index 352627e5d018..9c10e8a54f54 100644
--- a/libkmod/libkmod.h
+++ b/libkmod/libkmod.h
@@ -258,6 +258,18 @@ int kmod_module_dependency_symbol_get_bind(const struct kmod_list *entry);
 uint64_t kmod_module_dependency_symbol_get_crc(const struct kmod_list *entry);
 void kmod_module_dependency_symbols_free_list(struct kmod_list *list);
 
+enum kmod_symbol_type {
+	KMOD_SYMBOL_VERSIONS = 1,
+	KMOD_SYMBOL_CRC,
+	KMOD_SYMBOL_DEPENDENCY,
+};
+
+int kmod_module_get_typed_symbols(const struct kmod_module *mod, enum kmod_symbol_type type, struct kmod_list **list);
+const char *kmod_module_typed_symbol_get_symbol(const struct kmod_list *entry);
+int kmod_module_typed_symbol_get_bind(const struct kmod_list *entry);
+uint64_t kmod_module_typed_symbol_get_crc(const struct kmod_list *entry);
+void kmod_module_typed_symbols_free_list(struct kmod_list *list);
+
 #ifdef __cplusplus
 } /* extern "C" */
 #endif
-- 
2.19.1

[RFC PATCH 2/2] libkmod: module: make old symbol access API as wrapper to the new

[Yauheni Kaliuta]

Take in use the unified symbol access API but for compatibility keep
the old functions as wrappers to the new ones.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 libkmod/libkmod-module.c | 281 ++-------------------------------------
 1 file changed, 13 insertions(+), 268 deletions(-)

diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index f595f40032e0..e21a69bf2fd5 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -2610,30 +2610,6 @@ KMOD_EXPORT void kmod_module_typed_symbols_free_list(struct kmod_list *list)
 	}
 }
 
-struct kmod_module_version {
-	uint64_t crc;
-	char symbol[];
-};
-
-static struct kmod_module_version *kmod_module_versions_new(uint64_t crc, const char *symbol)
-{
-	struct kmod_module_version *mv;
-	size_t symbollen = strlen(symbol) + 1;
-
-	mv = malloc(sizeof(struct kmod_module_version) + symbollen);
-	if (mv == NULL)
-		return NULL;
-
-	mv->crc = crc;
-	memcpy(mv->symbol, symbol, symbollen);
-	return mv;
-}
-
-static void kmod_module_version_free(struct kmod_module_version *version)
-{
-	free(version);
-}
-
 /**
  * kmod_module_get_versions:
  * @mod: kmod module
@@ -2650,53 +2626,8 @@ static void kmod_module_version_free(struct kmod_module_version *version)
  */
 KMOD_EXPORT int kmod_module_get_versions(const struct kmod_module *mod, struct kmod_list **list)
 {
-	struct kmod_elf *elf;
-	struct kmod_modversion *versions;
-	int i, count, ret = 0;
-
-	if (mod == NULL || list == NULL)
-		return -ENOENT;
-
-	assert(*list == NULL);
-
-	elf = kmod_module_get_elf(mod);
-	if (elf == NULL)
-		return -errno;
-
-	count = kmod_elf_get_modversions(elf, &versions);
-	if (count < 0)
-		return count;
-
-	for (i = 0; i < count; i++) {
-		struct kmod_module_version *mv;
-		struct kmod_list *n;
-
-		mv = kmod_module_versions_new(versions[i].crc, versions[i].symbol);
-		if (mv == NULL) {
-			ret = -errno;
-			kmod_module_versions_free_list(*list);
-			*list = NULL;
-			goto list_error;
-		}
-
-		n = kmod_list_append(*list, mv);
-		if (n != NULL)
-			*list = n;
-		else {
-			kmod_module_version_free(mv);
-			kmod_module_versions_free_list(*list);
-			*list = NULL;
-			ret = -ENOMEM;
-			goto list_error;
-		}
-	}
-	ret = count;
-
-list_error:
-	free(versions);
-	return ret;
+	return kmod_module_get_typed_symbols(mod, KMOD_SYMBOL_VERSIONS, list);
 }
-
 /**
  * kmod_module_version_get_symbol:
  * @entry: a list entry representing a kmod module versions
@@ -2708,13 +2639,7 @@ list_error:
  */
 KMOD_EXPORT const char *kmod_module_version_get_symbol(const struct kmod_list *entry)
 {
-	struct kmod_module_version *version;
-
-	if (entry == NULL || entry->data == NULL)
-		return NULL;
-
-	version = entry->data;
-	return version->symbol;
+	return kmod_module_typed_symbol_get_symbol(entry);
 }
 
 /**
@@ -2727,13 +2652,7 @@ KMOD_EXPORT const char *kmod_module_version_get_symbol(const struct kmod_list *e
  */
 KMOD_EXPORT uint64_t kmod_module_version_get_crc(const struct kmod_list *entry)
 {
-	struct kmod_module_version *version;
-
-	if (entry == NULL || entry->data == NULL)
-		return 0;
-
-	version = entry->data;
-	return version->crc;
+	return kmod_module_typed_symbol_get_crc(entry);
 }
 
 /**
@@ -2744,29 +2663,7 @@ KMOD_EXPORT uint64_t kmod_module_version_get_crc(const struct kmod_list *entry)
  */
 KMOD_EXPORT void kmod_module_versions_free_list(struct kmod_list *list)
 {
-	while (list) {
-		kmod_module_version_free(list->data);
-		list = kmod_list_remove(list);
-	}
-}
-
-static struct kmod_module_symbol *kmod_module_symbols_new(uint64_t crc, const char *symbol)
-{
-	struct kmod_module_symbol *mv;
-	size_t symbollen = strlen(symbol) + 1;
-
-	mv = malloc(sizeof(struct kmod_module_symbol) + symbollen);
-	if (mv == NULL)
-		return NULL;
-
-	mv->crc = crc;
-	memcpy(mv->symbol, symbol, symbollen);
-	return mv;
-}
-
-static void kmod_module_symbol_free(struct kmod_module_symbol *symbol)
-{
-	free(symbol);
+	kmod_module_typed_symbols_free_list(list);
 }
 
 /**
@@ -2785,51 +2682,7 @@ static void kmod_module_symbol_free(struct kmod_module_symbol *symbol)
  */
 KMOD_EXPORT int kmod_module_get_symbols(const struct kmod_module *mod, struct kmod_list **list)
 {
-	struct kmod_elf *elf;
-	struct kmod_modversion *symbols;
-	int i, count, ret = 0;
-
-	if (mod == NULL || list == NULL)
-		return -ENOENT;
-
-	assert(*list == NULL);
-
-	elf = kmod_module_get_elf(mod);
-	if (elf == NULL)
-		return -errno;
-
-	count = kmod_elf_get_symbols(elf, &symbols);
-	if (count < 0)
-		return count;
-
-	for (i = 0; i < count; i++) {
-		struct kmod_module_symbol *mv;
-		struct kmod_list *n;
-
-		mv = kmod_module_symbols_new(symbols[i].crc, symbols[i].symbol);
-		if (mv == NULL) {
-			ret = -errno;
-			kmod_module_symbols_free_list(*list);
-			*list = NULL;
-			goto list_error;
-		}
-
-		n = kmod_list_append(*list, mv);
-		if (n != NULL)
-			*list = n;
-		else {
-			kmod_module_symbol_free(mv);
-			kmod_module_symbols_free_list(*list);
-			*list = NULL;
-			ret = -ENOMEM;
-			goto list_error;
-		}
-	}
-	ret = count;
-
-list_error:
-	free(symbols);
-	return ret;
+	return kmod_module_get_typed_symbols(mod, KMOD_SYMBOL_CRC, list);
 }
 
 /**
@@ -2843,13 +2696,7 @@ list_error:
  */
 KMOD_EXPORT const char *kmod_module_symbol_get_symbol(const struct kmod_list *entry)
 {
-	struct kmod_module_symbol *symbol;
-
-	if (entry == NULL || entry->data == NULL)
-		return NULL;
-
-	symbol = entry->data;
-	return symbol->symbol;
+	return kmod_module_typed_symbol_get_symbol(entry);
 }
 
 /**
@@ -2862,13 +2709,7 @@ KMOD_EXPORT const char *kmod_module_symbol_get_symbol(const struct kmod_list *en
  */
 KMOD_EXPORT uint64_t kmod_module_symbol_get_crc(const struct kmod_list *entry)
 {
-	struct kmod_module_symbol *symbol;
-
-	if (entry == NULL || entry->data == NULL)
-		return 0;
-
-	symbol = entry->data;
-	return symbol->crc;
+	return kmod_module_typed_symbol_get_crc(entry);
 }
 
 /**
@@ -2879,36 +2720,7 @@ KMOD_EXPORT uint64_t kmod_module_symbol_get_crc(const struct kmod_list *entry)
  */
 KMOD_EXPORT void kmod_module_symbols_free_list(struct kmod_list *list)
 {
-	while (list) {
-		kmod_module_symbol_free(list->data);
-		list = kmod_list_remove(list);
-	}
-}
-
-struct kmod_module_dependency_symbol {
-	uint64_t crc;
-	uint8_t bind;
-	char symbol[];
-};
-
-static struct kmod_module_dependency_symbol *kmod_module_dependency_symbols_new(uint64_t crc, uint8_t bind, const char *symbol)
-{
-	struct kmod_module_dependency_symbol *mv;
-	size_t symbollen = strlen(symbol) + 1;
-
-	mv = malloc(sizeof(struct kmod_module_dependency_symbol) + symbollen);
-	if (mv == NULL)
-		return NULL;
-
-	mv->crc = crc;
-	mv->bind = bind;
-	memcpy(mv->symbol, symbol, symbollen);
-	return mv;
-}
-
-static void kmod_module_dependency_symbol_free(struct kmod_module_dependency_symbol *dependency_symbol)
-{
-	free(dependency_symbol);
+	kmod_module_typed_symbols_free_list(list);
 }
 
 /**
@@ -2928,53 +2740,7 @@ static void kmod_module_dependency_symbol_free(struct kmod_module_dependency_sym
  */
 KMOD_EXPORT int kmod_module_get_dependency_symbols(const struct kmod_module *mod, struct kmod_list **list)
 {
-	struct kmod_elf *elf;
-	struct kmod_modversion *symbols;
-	int i, count, ret = 0;
-
-	if (mod == NULL || list == NULL)
-		return -ENOENT;
-
-	assert(*list == NULL);
-
-	elf = kmod_module_get_elf(mod);
-	if (elf == NULL)
-		return -errno;
-
-	count = kmod_elf_get_dependency_symbols(elf, &symbols);
-	if (count < 0)
-		return count;
-
-	for (i = 0; i < count; i++) {
-		struct kmod_module_dependency_symbol *mv;
-		struct kmod_list *n;
-
-		mv = kmod_module_dependency_symbols_new(symbols[i].crc,
-							symbols[i].bind,
-							symbols[i].symbol);
-		if (mv == NULL) {
-			ret = -errno;
-			kmod_module_dependency_symbols_free_list(*list);
-			*list = NULL;
-			goto list_error;
-		}
-
-		n = kmod_list_append(*list, mv);
-		if (n != NULL)
-			*list = n;
-		else {
-			kmod_module_dependency_symbol_free(mv);
-			kmod_module_dependency_symbols_free_list(*list);
-			*list = NULL;
-			ret = -ENOMEM;
-			goto list_error;
-		}
-	}
-	ret = count;
-
-list_error:
-	free(symbols);
-	return ret;
+	return kmod_module_get_typed_symbols(mod, KMOD_SYMBOL_DEPENDENCY, list);
 }
 
 /**
@@ -2988,13 +2754,7 @@ list_error:
  */
 KMOD_EXPORT const char *kmod_module_dependency_symbol_get_symbol(const struct kmod_list *entry)
 {
-	struct kmod_module_dependency_symbol *dependency_symbol;
-
-	if (entry == NULL || entry->data == NULL)
-		return NULL;
-
-	dependency_symbol = entry->data;
-	return dependency_symbol->symbol;
+	return kmod_module_typed_symbol_get_symbol(entry);
 }
 
 /**
@@ -3007,13 +2767,7 @@ KMOD_EXPORT const char *kmod_module_dependency_symbol_get_symbol(const struct km
  */
 KMOD_EXPORT uint64_t kmod_module_dependency_symbol_get_crc(const struct kmod_list *entry)
 {
-	struct kmod_module_dependency_symbol *dependency_symbol;
-
-	if (entry == NULL || entry->data == NULL)
-		return 0;
-
-	dependency_symbol = entry->data;
-	return dependency_symbol->crc;
+	return kmod_module_typed_symbol_get_crc(entry);
 }
 
 /**
@@ -3027,13 +2781,7 @@ KMOD_EXPORT uint64_t kmod_module_dependency_symbol_get_crc(const struct kmod_lis
  */
 KMOD_EXPORT int kmod_module_dependency_symbol_get_bind(const struct kmod_list *entry)
 {
-	struct kmod_module_dependency_symbol *dependency_symbol;
-
-	if (entry == NULL || entry->data == NULL)
-		return 0;
-
-	dependency_symbol = entry->data;
-	return dependency_symbol->bind;
+	return kmod_module_typed_symbol_get_bind(entry);
 }
 
 /**
@@ -3044,8 +2792,5 @@ KMOD_EXPORT int kmod_module_dependency_symbol_get_bind(const struct kmod_list *e
  */
 KMOD_EXPORT void kmod_module_dependency_symbols_free_list(struct kmod_list *list)
 {
-	while (list) {
-		kmod_module_dependency_symbol_free(list->data);
-		list = kmod_list_remove(list);
-	}
+	kmod_module_typed_symbols_free_list(list);
 }
-- 
2.19.1

Re: [RFC PATCH 0/2] Unify symbol access API

[Lucas De Marchi]

On Fri, Nov 23, 2018 at 11:53:20PM +0200, Yauheni Kaliuta wrote:
> Hi!
> 
> I'm not very happy with the naming, but how do you like the idea in general?

I don't like the naming, too. The idea is good and I like the
de-duplication of the implementation, thanks.

These are all symbols, but coming from different elf sections. I wonder
if we can add a kmod_module_get_symbols2() with an additional
argument.

then we would still have kmod_module_symbol_get_symbol(),
kmod_module_symbol_get_crc() and add kmod_module_symbol_get_bind_type()

And these will need to be added to the testsuite

thoughts?

Lucas De Marchi

> 
> 
> Yauheni Kaliuta (2):
>   libkmod: module: introduce common symbol API.
>   libkmod: module: make old symbol access API as wrapper to the new
> 
>  libkmod/libkmod-module.c | 387 ++++++++++++++++-----------------------
>  libkmod/libkmod.h        |  12 ++
>  2 files changed, 171 insertions(+), 228 deletions(-)
> 
> -- 
> 2.19.1
> 

Re: [RFC PATCH 1/2] libkmod: module: introduce common symbol API.

[Yauheni Kaliuta]

Hi, Lucas!

>>>>> On Wed, 19 Dec 2018 10:49:30 -0800, Lucas De Marchi  wrote:

 > On Fri, Nov 23, 2018 at 11:53:21PM +0200, Yauheni Kaliuta wrote:
 >> Introduce one family of functions to replace the duplicated:
 >> 
 >> - kmod_module_version_get_symbol()
 >> - kmod_module_version_get_crc()
 >> - kmod_module_symbol_get_symbol()
 >> - kmod_module_symbol_get_crc()
 >> - kmod_module_dependency_symbol_get_symbol()
 >> - kmod_module_dependency_symbol_get_crc()
 >> - kmod_module_versions_free_list()
 >> - kmod_module_symbols_free_list()
 >> - kmod_module_dependency_symbols_free_list()
 >> 
 >> It reuses kmod_module_symbol structure but extends it with the
 >> "bind" field, keeping the API still the same.
 >> 
 >> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
 >> ---
 >> libkmod/libkmod-module.c | 196 ++++++++++++++++++++++++++++++++++++++-
 >> libkmod/libkmod.h        |  12 +++
 >> 2 files changed, 203 insertions(+), 5 deletions(-)
 >> 
 >> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
 >> index 889f26479a98..f595f40032e0 100644
 >> --- a/libkmod/libkmod-module.c
 >> +++ b/libkmod/libkmod-module.c
 >> @@ -2419,6 +2419,197 @@ KMOD_EXPORT void kmod_module_info_free_list(struct kmod_list *list)
 >> }
 >> }
 >> 
 >> +struct kmod_module_symbol {
 >> +	uint64_t crc;
 >> +	uint8_t bind;

 > I think this can be enum kmod_symbol_bind.  Or just use "unsigned
 > int"... we would have a whole here nonetheless.

Ok. As with the parts below, it's a copy of the existing code.

 >> +	char symbol[];
 >> +};
 >> +
 >> +static struct kmod_module_symbol
 >> *kmod_module_typed_symbol_new(uint64_t crc, uint8_t bind, const char
 >> *symbol)
 >> +{
 >> +	struct kmod_module_symbol *ms;
 >> +	size_t symbollen = strlen(symbol) + 1;

 > I know this comes from the other functions, but the naming is not ideal:
 > use len for the string len, use sz when including the '\0'. So

 > 	size_t symbolsz = strlen(symbol) + 1;

 >> +
 >> +	ms = malloc(sizeof(struct kmod_module_symbol) + symbollen);

 > 	ms = malloc(sizeof(*ms) + symbolsz);

Sure, I prefer it myself.

 >> +	if (ms == NULL)
 >> +		return NULL;
 >> +
 >> +	ms->crc = crc;
 >> +	ms->bind = bind;
 >> +	memcpy(ms->symbol, symbol, symbollen);

 > blank line

 >> +	return ms;
 >> +}
 >> +
 >> +static void kmod_module_typed_symbol_free(struct kmod_module_symbol *ms)
 >> +{
 >> +	free(ms);
 >> +}
 >> +
 >> +typedef int (*kmod_symbols_getter)(const struct kmod_elf *elf,
 >> struct kmod_modversion **array);
 >> +
 >> +static kmod_symbols_getter kmod_module_getter_lookup(const struct
 >> kmod_module *mod,
 >> +						     enum kmod_symbol_type type)
 >> +{
 >> +	switch (type) {
 >> +	case KMOD_SYMBOL_VERSIONS:
 >> +		return kmod_elf_get_modversions;
 >> +	case KMOD_SYMBOL_CRC:
 >> +		return kmod_elf_get_symbols;
 >> +	case KMOD_SYMBOL_DEPENDENCY:
 >> +		return kmod_elf_get_dependency_symbols;
 >> +	default:
 >> +		ERR(mod->ctx, "Wrong symbol type requested: %d\n", type);
 >> +	}

 > I think we can get away without this indirection and just embed the
 > switch inside kmod_module_get_typed_symbols().

If you insist.

 >> +
 >> +	return NULL;
 >> +}
 >> +
 >> +/**
 >> + * kmod_module_get_typed_symbols:
 >> + * @mod: kmod module
 >> + * @type: type of symbols to get:
 >> + *        KMOD_SYMBOL_VERSIONS
 >> + *        KMOD_SYMBOL_CRC
 >> + *        KMDO_SYMBOL_DEPENDENCY
 >> + * @list: where to return list of module symbols. Use
 >> + *        kmod_module_typed_symbol_get_symbol(),
 >> + *        kmod_module_typed_symbol_get_crc() and
 >> + *        kmod_module_typed_symbol_get_bind() to access the data.

 > humn... bind() only makes sense for KMDO_SYMBOL_DEPENDENCY, but I think
 > it's fine since we won't return garbage.

 >> + *
 >> + *        After use, free the @list by calling
 >> + *        kmod_module_typed_symbols_free_list().
 >> + *
 >> + * Returns: 0 on success or < 0 otherwise.
 >> + */
 >> +KMOD_EXPORT int kmod_module_get_typed_symbols(const struct
 >> kmod_module *mod, enum kmod_symbol_type type, struct kmod_list
 >> **list)
 >> +{
 >> +	struct kmod_elf *elf;
 >> +	struct kmod_modversion *symbols;
 >> +	int i, count, ret = 0;
 >> +	kmod_symbols_getter getter;
 >> +
 >> +	if (mod == NULL || list == NULL)
 >> +		return -ENOENT;
 >> +
 >> +	assert(*list == NULL);
 >> +
 >> +	elf = kmod_module_get_elf(mod);
 >> +	if (elf == NULL)
 >> +		return -errno;
 >> +
 >> +	getter = kmod_module_getter_lookup(mod, type);
 >> +	if (getter == NULL)
 >> +		return -ENOENT;
 >> +
 >> +	count = getter(elf, &symbols);
 >> +	if (count < 0)
 >> +		return count;
 >> +
 >> +	for (i = 0; i < count; i++) {
 >> +		struct kmod_module_symbol *ms;
 >> +		struct kmod_list *n;
 >> +
 >> +		ms = kmod_module_typed_symbol_new(symbols[i].crc,
 >> +						  symbols[i].bind,
 >> +						  symbols[i].symbol);
 >> +		if (ms == NULL) {
 >> +			ret = -errno;
 >> +			kmod_module_typed_symbols_free_list(*list);
 >> +			*list = NULL;
 >> +			goto list_error;
 >> +		}
 >> +
 >> +		n = kmod_list_append(*list, ms);
 >> +		if (n != NULL)
 >> +			*list = n;
 >> +		else {
 >> +			kmod_module_typed_symbol_free(ms);
 >> +			kmod_module_typed_symbols_free_list(*list);
 >> +			*list = NULL;
 >> +			ret = -ENOMEM;
 >> +			goto list_error;
 >> +		}
 >> +	}
 >> +	ret = count;
 >> +
 >> +list_error:
 >> +	free(symbols);
 >> +	return ret;
 >> +}
 >> +
 >> +/**
 >> + * kmod_module_typed_symbol_get_symbol:
 >> + * @entry: a list entry representing a kmod module typed symbol
 >> + *
 >> + * Get the symbol's name of the entry.
 >> + *
 >> + * Returns: the symbol's name of this kmod module symbols on success or NULL
 >> + * on failure. The string is owned by the list, do not free it.
 >> + */
 >> +KMOD_EXPORT const char *kmod_module_typed_symbol_get_symbol(const
 >> struct kmod_list *entry)
 >> +{
 >> +	struct kmod_module_symbol *ms;
 >> +
 >> +	if (entry == NULL || entry->data == NULL)
 >> +		return NULL;
 >> +
 >> +	ms = entry->data;
 >> +	return ms->symbol;

 > we can make this smaller with

 > return ((struct kmod_module_symbol *)entry->data)->symbol

I ususally declare temporary variables (optimizer gets rid of
them anyway) to make the types obvious for readers, but here with
the casting it's fine. Ok.

 >> +}
 >> +
 >> +/**
 >> + * kmod_module_typed_symbol_get_crc:
 >> + * @entry: a list entry representing a kmod module symbol
 >> + *
 >> + * Get the crc of the symbol.
 >> + *
 >> + * Returns: the crc of this kmod module symbol if available,
 >> otherwise default to 0.
 >> + */
 >> +KMOD_EXPORT uint64_t kmod_module_typed_symbol_get_crc(const struct
 >> kmod_list *entry)
 >> +{
 >> +	struct kmod_module_symbol *ms;
 >> +
 >> +	if (entry == NULL || entry->data == NULL)
 >> +		return 0;
 >> +
 >> +	ms = entry->data;
 >> +	return ms->crc;

 > ditto

 >> +}
 >> +
 >> +/**
 >> + * kmod_module_dependency_symbol_get_bind:
 >> + * @entry: a list entry representing a kmod module symbol
 >> + *
 >> + * Get the bind type of the symbol.
 >> + *
 >> + * Returns: the bind of this kmod module symbol on success
 >> + * or < 0 on failure.
 >> + */
 >> +KMOD_EXPORT int kmod_module_typed_symbol_get_bind(const struct kmod_list *entry)
 >> +{
 >> +	struct kmod_module_symbol *ms;
 >> +
 >> +	if (entry == NULL || entry->data == NULL)
 >> +		return 0;
 >> +
 >> +	ms = entry->data;
 >> +	return ms->bind;
 >> +}
 >> +
 >> +/**
 >> + * kmod_module_typed_symbols_free_list:
 >> + * @list: kmod module typed symbols list
 >> + *
 >> + * Release the resources taken by @list
 >> + */
 >> +KMOD_EXPORT void kmod_module_typed_symbols_free_list(struct kmod_list *list)
 >> +{
 >> +	while (list) {
 >> +		kmod_module_typed_symbol_free(list->data);
 >> +		list = kmod_list_remove(list);
 >> +	}
 >> +}
 >> +
 >> struct kmod_module_version {
 >> uint64_t crc;
 >> char symbol[];
 >> @@ -2559,11 +2750,6 @@ KMOD_EXPORT void kmod_module_versions_free_list(struct kmod_list *list)
 >> }
 >> }
 >> 
 >> -struct kmod_module_symbol {
 >> -	uint64_t crc;
 >> -	char symbol[];
 >> -};
 >> -
 >> static struct kmod_module_symbol *kmod_module_symbols_new(uint64_t crc, const char *symbol)
 >> {
 >> struct kmod_module_symbol *mv;
 >> diff --git a/libkmod/libkmod.h b/libkmod/libkmod.h
 >> index 352627e5d018..9c10e8a54f54 100644
 >> --- a/libkmod/libkmod.h
 >> +++ b/libkmod/libkmod.h
 >> @@ -258,6 +258,18 @@ int kmod_module_dependency_symbol_get_bind(const struct kmod_list *entry);
 >> uint64_t kmod_module_dependency_symbol_get_crc(const struct kmod_list *entry);
 >> void kmod_module_dependency_symbols_free_list(struct kmod_list *list);
 >> 
 >> +enum kmod_symbol_type {
 >> +	KMOD_SYMBOL_VERSIONS = 1,

 > why 1? we usually start counting from 0.

Well, 0 is too special in my mind and easy to mix with default
initialized value. Not a big deal, will change.

 > thanks
 > Lucas De Marchi

 >> +	KMOD_SYMBOL_CRC,
 >> +	KMOD_SYMBOL_DEPENDENCY,
 >> +};
 >> +
 >> +int kmod_module_get_typed_symbols(const struct kmod_module *mod,
 >> enum kmod_symbol_type type, struct kmod_list **list);
 >> +const char *kmod_module_typed_symbol_get_symbol(const struct kmod_list *entry);
 >> +int kmod_module_typed_symbol_get_bind(const struct kmod_list *entry);
 >> +uint64_t kmod_module_typed_symbol_get_crc(const struct kmod_list *entry);
 >> +void kmod_module_typed_symbols_free_list(struct kmod_list *list);
 >> +
 >> #ifdef __cplusplus
 >> } /* extern "C" */
 >> #endif
 >> -- 
 >> 2.19.1
 >> 

-- 
WBR,
Yauheni Kaliuta