On Tue, 3 Nov 2009 08:36:18 pm Alan Jenkins wrote:
> find_symbol() will use bsearch() instead of each_symbol(). each_symbol()
> is still desired by Ksplice (although it is not in-tree yet). Let's try
> to minimize the code which will be duplicated between these two
> functions, by changing how the symbol tables are declared.
Alan, this is a particularly neat cleanup. Thanks!
> +typedef bool each_symbol_fn_t (enum export_type type,
> + const struct kernel_symbol *sym,
> + const unsigned long *crc,
> + struct module *owner,
> + void *data);
> +
> /* Walk the exported symbol table */
> -bool each_symbol(bool (*fn)(const struct symsearch *arr, struct module *owner,
> - unsigned int symnum, void *data), void *data);
> +bool each_symbol(each_symbol_fn_t *fn, void *data);
I really hate throwaway typedefs like this. But it's used in two other
places, so I'll suppress my distaste :)
> +static struct ksymtab ksymtab[EXPORT_TYPE_MAX];
> +
> +static int __init init_ksymtab(void)
> +{
> + struct ksymtab tmp[EXPORT_TYPE_MAX] = {
> + [EXPORT_TYPE_PLAIN] = {
> + __start___ksymtab, __start___kcrctab,
> + __stop___ksymtab - __start___ksymtab,
> + },
> + [EXPORT_TYPE_GPL] = {
> + __start___ksymtab_gpl, __start___kcrctab_gpl,
> + __stop___ksymtab_gpl - __start___ksymtab_gpl,
> + },
> +#ifdef CONFIG_UNUSED_EXPORTS
> + [EXPORT_TYPE_UNUSED] = {
> + __start___ksymtab_unused, __start___kcrctab_unused,
> + __stop___ksymtab_unused - __start___ksymtab_unused,
> + },
> + [EXPORT_TYPE_UNUSED_GPL] = {
> + __start___ksymtab_unused_gpl,
> + __start___kcrctab_unused_gpl,
> + __stop___ksymtab_unused_gpl -
> + __start___ksymtab_unused_gpl,
> + },
> +#endif
> + [EXPORT_TYPE_GPL_FUTURE] = {
> + __start___ksymtab_gpl_future,
> + __start___kcrctab_gpl_future,
> + __stop___ksymtab_gpl_future -
> + __start___ksymtab_gpl_future,
> + },
> + };
> +
> + memcpy(ksymtab, tmp, sizeof(ksymtab));
This works, but I'd prefer you to open-code the assignments. Simpler and
marginally more efficient.
> @@ -322,9 +322,9 @@ static bool find_symbol_in_section(const struct symsearch *syms,
> }
> #endif
>
> + fsa->sym = sym;
> + fsa->crc = crc;
> fsa->owner = owner;
> - fsa->crc = symversion(syms->crcs, symnum);
> - fsa->sym = &syms->start[symnum];
> return true;
Strange gratuitous reorder here?
> +static const char *export_section_names[EXPORT_TYPE_MAX] = {
> + [EXPORT_TYPE_PLAIN] = "__ksymtab",
> + [EXPORT_TYPE_GPL] = "__ksymtab_gpl",
> +#ifdef CONFIG_UNUSED_SYMBOLS
> + [EXPORT_TYPE_UNUSED] = "__ksymtab_unused",
> + [EXPORT_TYPE_UNUSED_GPL] = "__ksymtab_unused_gpl",
> +#endif
> + [EXPORT_TYPE_GPL_FUTURE] = "__ksymtab_gpl_future",
> +};
> +
> +static const char *crc_section_names[EXPORT_TYPE_MAX] = {
> + [EXPORT_TYPE_PLAIN] = "__kcrctab",
> + [EXPORT_TYPE_GPL] = "__kcrctab_gpl",
> +#ifdef CONFIG_UNUSED_SYMBOLS
> + [EXPORT_TYPE_UNUSED] = "__kcrctab_unused",
> + [EXPORT_TYPE_UNUSED_GPL] = "__kcrctab_unused_gpl",
> +#endif
> + [EXPORT_TYPE_GPL_FUTURE] = "__kcrctab_gpl_future",
> +};
You can use [] here for size instead of explicit EXPORT_TYPE_MAX. We should
have named these sections better :(
> + for (export_type = 0; export_type < EXPORT_TYPE_MAX; export_type++) {
Then use ARRAY_SIZE(export_section_names) here.
> + for (export_type = 0; export_type < EXPORT_TYPE_MAX; export_type++) {
> + if (mod->syms[export_type].syms &&
Similar ARRAY_SIZE(mod->syms). It's less indirect, IMHO.
But all minor nitpicks; code is excellent!
Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
