diff mbox series

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

Message ID 20181123215322.5438-2-yauheni.kaliuta@redhat.com (mailing list archive)
State New, archived
Headers show
Series Unify symbol access API | expand

Commit Message

Yauheni Kaliuta Nov. 23, 2018, 9:53 p.m. UTC 
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(-)

Comments

Yauheni Kaliuta Dec. 20, 2018, 11:08 a.m. UTC | #1 
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
 >>
diff mbox series

Patch

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