Linux-Modules Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH 0/2] Unify symbol access API
@ 2018-11-23 21:53 Yauheni Kaliuta
  2018-11-23 21:53 ` [RFC PATCH 1/2] libkmod: module: introduce common symbol API Yauheni Kaliuta
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Yauheni Kaliuta @ 2018-11-23 21:53 UTC (permalink / raw)
  To: linux-modules; +Cc: ykaliuta, Lucas De Marchi

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


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

* [RFC PATCH 1/2] libkmod: module: introduce common symbol API.
  2018-11-23 21:53 [RFC PATCH 0/2] Unify symbol access API Yauheni Kaliuta
@ 2018-11-23 21:53 ` Yauheni Kaliuta
       [not found]   ` <20181219184930.GD6410@ldmartin-desk.jf.intel.com>
  2018-11-23 21:53 ` [RFC PATCH 2/2] libkmod: module: make old symbol access API as wrapper to the new Yauheni Kaliuta
  2018-12-19 19:24 ` [RFC PATCH 0/2] Unify symbol access API Lucas De Marchi
  2 siblings, 1 reply; 5+ messages in thread
From: Yauheni Kaliuta @ 2018-11-23 21:53 UTC (permalink / raw)
  To: linux-modules; +Cc: ykaliuta, Lucas De Marchi

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


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

* [RFC PATCH 2/2] libkmod: module: make old symbol access API as wrapper to the new
  2018-11-23 21:53 [RFC PATCH 0/2] Unify symbol access API Yauheni Kaliuta
  2018-11-23 21:53 ` [RFC PATCH 1/2] libkmod: module: introduce common symbol API Yauheni Kaliuta
@ 2018-11-23 21:53 ` Yauheni Kaliuta
  2018-12-19 19:24 ` [RFC PATCH 0/2] Unify symbol access API Lucas De Marchi
  2 siblings, 0 replies; 5+ messages in thread
From: Yauheni Kaliuta @ 2018-11-23 21:53 UTC (permalink / raw)
  To: linux-modules; +Cc: ykaliuta, Lucas De Marchi

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


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

* Re: [RFC PATCH 0/2] Unify symbol access API
  2018-11-23 21:53 [RFC PATCH 0/2] Unify symbol access API Yauheni Kaliuta
  2018-11-23 21:53 ` [RFC PATCH 1/2] libkmod: module: introduce common symbol API Yauheni Kaliuta
  2018-11-23 21:53 ` [RFC PATCH 2/2] libkmod: module: make old symbol access API as wrapper to the new Yauheni Kaliuta
@ 2018-12-19 19:24 ` Lucas De Marchi
  2 siblings, 0 replies; 5+ messages in thread
From: Lucas De Marchi @ 2018-12-19 19:24 UTC (permalink / raw)
  To: Yauheni Kaliuta; +Cc: linux-modules, ykaliuta

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
> 

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

* Re: [RFC PATCH 1/2] libkmod: module: introduce common symbol API.
       [not found]   ` <20181219184930.GD6410@ldmartin-desk.jf.intel.com>
@ 2018-12-20 11:08     ` Yauheni Kaliuta
  0 siblings, 0 replies; 5+ messages in thread
From: Yauheni Kaliuta @ 2018-12-20 11:08 UTC (permalink / raw)
  To: Lucas De Marchi; +Cc: linux-modules, ykaliuta

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

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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-23 21:53 [RFC PATCH 0/2] Unify symbol access API Yauheni Kaliuta
2018-11-23 21:53 ` [RFC PATCH 1/2] libkmod: module: introduce common symbol API Yauheni Kaliuta
     [not found]   ` <20181219184930.GD6410@ldmartin-desk.jf.intel.com>
2018-12-20 11:08     ` Yauheni Kaliuta
2018-11-23 21:53 ` [RFC PATCH 2/2] libkmod: module: make old symbol access API as wrapper to the new Yauheni Kaliuta
2018-12-19 19:24 ` [RFC PATCH 0/2] Unify symbol access API Lucas De Marchi

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


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