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

[Yauheni Kaliuta <yauheni.kaliuta@redhat.com>]

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