* [PATCH 0/2] Using function type to cleanup the function parament
@ 2015-07-14 6:59 Minfei Huang
2015-07-14 6:59 ` [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code Minfei Huang
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Minfei Huang @ 2015-07-14 6:59 UTC (permalink / raw)
To: akpm, rob.jones, amhyung, rusty; +Cc: linux-kernel, Minfei Huang
From: Minfei Huang <mnfhuang@gmail.com>
This patchset do the cleanup. For now, we can use function type
as the parament to simplify the code.
Previously, we will declare the function as following:
bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
struct module *owner,
void *data), void *data);
Once we define the function as a type, we can just declare the function
as following.
bool each_symbol_section(find_symbol_in_section_t fn, void *data);
Minfei Huang (2):
Define find_symbol_in_section_t as function type to simplify the code
Define kallsyms_cmp_symbol_t as function type to simplify the code
include/linux/kallsyms.h | 10 +++-------
include/linux/module.h | 19 +++++++++----------
kernel/kallsyms.c | 4 +---
kernel/module.c | 13 +++----------
4 files changed, 16 insertions(+), 30 deletions(-)
--
2.2.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code
2015-07-14 6:59 [PATCH 0/2] Using function type to cleanup the function parament Minfei Huang
@ 2015-07-14 6:59 ` Minfei Huang
2015-07-14 7:04 ` Minfei Huang
2015-07-14 21:52 ` Rusty Russell
2015-07-14 6:59 ` [PATCH 2/2] Define kallsyms_cmp_symbol_t " Minfei Huang
2015-07-14 7:06 ` [PATCH 0/2] Using function type to cleanup the function parament Minfei Huang
2 siblings, 2 replies; 10+ messages in thread
From: Minfei Huang @ 2015-07-14 6:59 UTC (permalink / raw)
To: akpm, rob.jones, amhyung, rusty; +Cc: linux-kernel, Minfei Huang
From: Minfei Huang <mnfhuang@gmail.com>
It is not elegance, if we use function directly as the argument, like
following:
bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
struct module *owner,
void *data), void *data);
Here introduce a type defined function find_symbol_in_section_t. Now
we can use these type defined function directly, if we want to pass
the function as the argument.
bool each_symbol_section(find_symbol_in_section_t fn, void *data);
Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
---
include/linux/module.h | 6 +++---
kernel/module.c | 9 ++-------
2 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index d67b193..1e125b1 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -462,14 +462,14 @@ const struct kernel_symbol *find_symbol(const char *name,
bool gplok,
bool warn);
+typedef bool (*find_symbol_in_section_t)(const struct symsearch *arr,
+ struct module *owner, void *data);
/*
* Walk the exported symbol table
*
* Must be called with module_mutex held or preemption disabled.
*/
-bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
- struct module *owner,
- void *data), void *data);
+bool each_symbol_section(find_symbol_in_section_t fn, void *data);
/* Returns 0 and fills in value, defined and namebuf, or -ERANGE if
symnum out of range. */
diff --git a/kernel/module.c b/kernel/module.c
index 4d2b82e..1400c0b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -426,9 +426,7 @@ extern const unsigned long __start___kcrctab_unused_gpl[];
static bool each_symbol_in_section(const struct symsearch *arr,
unsigned int arrsize,
struct module *owner,
- bool (*fn)(const struct symsearch *syms,
- struct module *owner,
- void *data),
+ find_symbol_in_section_t fn,
void *data)
{
unsigned int j;
@@ -442,10 +440,7 @@ static bool each_symbol_in_section(const struct symsearch *arr,
}
/* Returns true as soon as fn returns true, otherwise false. */
-bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
- struct module *owner,
- void *data),
- void *data)
+bool each_symbol_section(find_symbol_in_section_t fn, void *data)
{
struct module *mod;
static const struct symsearch arr[] = {
--
2.2.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] Define kallsyms_cmp_symbol_t as function type to simplify the code
2015-07-14 6:59 [PATCH 0/2] Using function type to cleanup the function parament Minfei Huang
2015-07-14 6:59 ` [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code Minfei Huang
@ 2015-07-14 6:59 ` Minfei Huang
2015-07-14 7:05 ` Minfei Huang
2015-07-14 7:06 ` [PATCH 0/2] Using function type to cleanup the function parament Minfei Huang
2 siblings, 1 reply; 10+ messages in thread
From: Minfei Huang @ 2015-07-14 6:59 UTC (permalink / raw)
To: akpm, rob.jones, amhyung, rusty; +Cc: linux-kernel, Minfei Huang
From: Minfei Huang <mnfhuang@gmail.com>
It is not elegance, if we use function directly as the argument, like
following:
int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
struct module *, unsigned long),
void *data);
Here introduce a type defined function kallsyms_cmp_symbol_t. Now
we can use these type defined function directly, if we want to pass
the function as the argument.
int module_kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn,
void *data);
Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
---
include/linux/kallsyms.h | 10 +++-------
include/linux/module.h | 13 ++++++-------
kernel/kallsyms.c | 4 +---
kernel/module.c | 4 +---
4 files changed, 11 insertions(+), 20 deletions(-)
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 6883e19..e8ed37d 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -8,6 +8,7 @@
#include <linux/errno.h>
#include <linux/kernel.h>
#include <linux/stddef.h>
+#include <linux/module.h>
#define KSYM_NAME_LEN 128
#define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s]") + (KSYM_NAME_LEN - 1) + \
@@ -20,9 +21,7 @@ struct module;
unsigned long kallsyms_lookup_name(const char *name);
/* Call a function on each kallsyms symbol in the core kernel */
-int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
- unsigned long),
- void *data);
+int kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn, void *data);
extern int kallsyms_lookup_size_offset(unsigned long addr,
unsigned long *symbolsize,
@@ -52,10 +51,7 @@ static inline unsigned long kallsyms_lookup_name(const char *name)
return 0;
}
-static inline int kallsyms_on_each_symbol(int (*fn)(void *, const char *,
- struct module *,
- unsigned long),
- void *data)
+static inline int kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn, void *data)
{
return 0;
}
diff --git a/include/linux/module.h b/include/linux/module.h
index 1e125b1..6a05a24 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -479,9 +479,10 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
/* Look for this name: can be of form module:name. */
unsigned long module_kallsyms_lookup_name(const char *name);
-int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
- struct module *, unsigned long),
- void *data);
+typedef int (*kallsyms_cmp_symbol_t)(void *, const char *,
+ struct module *, unsigned long);
+
+int module_kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn, void *data);
extern void __module_put_and_exit(struct module *mod, long code)
__attribute__((noreturn));
@@ -637,10 +638,8 @@ static inline unsigned long module_kallsyms_lookup_name(const char *name)
return 0;
}
-static inline int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
- struct module *,
- unsigned long),
- void *data)
+static inline int module_kallsyms_on_each_symbol(
+ kallsyms_cmp_symbol_t fn, void *data)
{
return 0;
}
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 5c5987f..be5786b 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -193,9 +193,7 @@ unsigned long kallsyms_lookup_name(const char *name)
}
EXPORT_SYMBOL_GPL(kallsyms_lookup_name);
-int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
- unsigned long),
- void *data)
+int kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn, void *data)
{
char namebuf[KSYM_NAME_LEN];
unsigned long i;
diff --git a/kernel/module.c b/kernel/module.c
index 1400c0b..67ed39b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3811,9 +3811,7 @@ unsigned long module_kallsyms_lookup_name(const char *name)
return ret;
}
-int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
- struct module *, unsigned long),
- void *data)
+int module_kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn, void *data)
{
struct module *mod;
unsigned int i;
--
2.2.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code
2015-07-14 6:59 ` [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code Minfei Huang
@ 2015-07-14 7:04 ` Minfei Huang
2015-07-14 21:52 ` Rusty Russell
1 sibling, 0 replies; 10+ messages in thread
From: Minfei Huang @ 2015-07-14 7:04 UTC (permalink / raw)
To: Minfei Huang; +Cc: akpm, rob.jones, namhyung, rusty, linux-kernel
Lost the character 'n' in the Namhyung Kim <namhyung@kernel.org>.
Resend it.
On 07/14/15 at 02:59P, Minfei Huang wrote:
> From: Minfei Huang <mnfhuang@gmail.com>
>
> It is not elegance, if we use function directly as the argument, like
> following:
>
> bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> struct module *owner,
> void *data), void *data);
>
> Here introduce a type defined function find_symbol_in_section_t. Now
> we can use these type defined function directly, if we want to pass
> the function as the argument.
>
> bool each_symbol_section(find_symbol_in_section_t fn, void *data);
>
> Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> ---
> include/linux/module.h | 6 +++---
> kernel/module.c | 9 ++-------
> 2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index d67b193..1e125b1 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -462,14 +462,14 @@ const struct kernel_symbol *find_symbol(const char *name,
> bool gplok,
> bool warn);
>
> +typedef bool (*find_symbol_in_section_t)(const struct symsearch *arr,
> + struct module *owner, void *data);
> /*
> * Walk the exported symbol table
> *
> * Must be called with module_mutex held or preemption disabled.
> */
> -bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> - struct module *owner,
> - void *data), void *data);
> +bool each_symbol_section(find_symbol_in_section_t fn, void *data);
>
> /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if
> symnum out of range. */
> diff --git a/kernel/module.c b/kernel/module.c
> index 4d2b82e..1400c0b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -426,9 +426,7 @@ extern const unsigned long __start___kcrctab_unused_gpl[];
> static bool each_symbol_in_section(const struct symsearch *arr,
> unsigned int arrsize,
> struct module *owner,
> - bool (*fn)(const struct symsearch *syms,
> - struct module *owner,
> - void *data),
> + find_symbol_in_section_t fn,
> void *data)
> {
> unsigned int j;
> @@ -442,10 +440,7 @@ static bool each_symbol_in_section(const struct symsearch *arr,
> }
>
> /* Returns true as soon as fn returns true, otherwise false. */
> -bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> - struct module *owner,
> - void *data),
> - void *data)
> +bool each_symbol_section(find_symbol_in_section_t fn, void *data)
> {
> struct module *mod;
> static const struct symsearch arr[] = {
> --
> 2.2.2
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] Define kallsyms_cmp_symbol_t as function type to simplify the code
2015-07-14 6:59 ` [PATCH 2/2] Define kallsyms_cmp_symbol_t " Minfei Huang
@ 2015-07-14 7:05 ` Minfei Huang
0 siblings, 0 replies; 10+ messages in thread
From: Minfei Huang @ 2015-07-14 7:05 UTC (permalink / raw)
To: Minfei Huang; +Cc: akpm, rob.jones, namhyung, rusty, linux-kernel
Lost the character 'n' in the Namhyung Kim <namhyung@kernel.org>.
Resent it.
On 07/14/15 at 02:59P, Minfei Huang wrote:
> From: Minfei Huang <mnfhuang@gmail.com>
>
> It is not elegance, if we use function directly as the argument, like
> following:
>
> int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> struct module *, unsigned long),
> void *data);
>
> Here introduce a type defined function kallsyms_cmp_symbol_t. Now
> we can use these type defined function directly, if we want to pass
> the function as the argument.
>
> int module_kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn,
> void *data);
>
> Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> ---
> include/linux/kallsyms.h | 10 +++-------
> include/linux/module.h | 13 ++++++-------
> kernel/kallsyms.c | 4 +---
> kernel/module.c | 4 +---
> 4 files changed, 11 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
> index 6883e19..e8ed37d 100644
> --- a/include/linux/kallsyms.h
> +++ b/include/linux/kallsyms.h
> @@ -8,6 +8,7 @@
> #include <linux/errno.h>
> #include <linux/kernel.h>
> #include <linux/stddef.h>
> +#include <linux/module.h>
>
> #define KSYM_NAME_LEN 128
> #define KSYM_SYMBOL_LEN (sizeof("%s+%#lx/%#lx [%s]") + (KSYM_NAME_LEN - 1) + \
> @@ -20,9 +21,7 @@ struct module;
> unsigned long kallsyms_lookup_name(const char *name);
>
> /* Call a function on each kallsyms symbol in the core kernel */
> -int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
> - unsigned long),
> - void *data);
> +int kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn, void *data);
>
> extern int kallsyms_lookup_size_offset(unsigned long addr,
> unsigned long *symbolsize,
> @@ -52,10 +51,7 @@ static inline unsigned long kallsyms_lookup_name(const char *name)
> return 0;
> }
>
> -static inline int kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> - struct module *,
> - unsigned long),
> - void *data)
> +static inline int kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn, void *data)
> {
> return 0;
> }
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 1e125b1..6a05a24 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -479,9 +479,10 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> /* Look for this name: can be of form module:name. */
> unsigned long module_kallsyms_lookup_name(const char *name);
>
> -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> - struct module *, unsigned long),
> - void *data);
> +typedef int (*kallsyms_cmp_symbol_t)(void *, const char *,
> + struct module *, unsigned long);
> +
> +int module_kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn, void *data);
>
> extern void __module_put_and_exit(struct module *mod, long code)
> __attribute__((noreturn));
> @@ -637,10 +638,8 @@ static inline unsigned long module_kallsyms_lookup_name(const char *name)
> return 0;
> }
>
> -static inline int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> - struct module *,
> - unsigned long),
> - void *data)
> +static inline int module_kallsyms_on_each_symbol(
> + kallsyms_cmp_symbol_t fn, void *data)
> {
> return 0;
> }
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 5c5987f..be5786b 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -193,9 +193,7 @@ unsigned long kallsyms_lookup_name(const char *name)
> }
> EXPORT_SYMBOL_GPL(kallsyms_lookup_name);
>
> -int kallsyms_on_each_symbol(int (*fn)(void *, const char *, struct module *,
> - unsigned long),
> - void *data)
> +int kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn, void *data)
> {
> char namebuf[KSYM_NAME_LEN];
> unsigned long i;
> diff --git a/kernel/module.c b/kernel/module.c
> index 1400c0b..67ed39b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3811,9 +3811,7 @@ unsigned long module_kallsyms_lookup_name(const char *name)
> return ret;
> }
>
> -int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> - struct module *, unsigned long),
> - void *data)
> +int module_kallsyms_on_each_symbol(kallsyms_cmp_symbol_t fn, void *data)
> {
> struct module *mod;
> unsigned int i;
> --
> 2.2.2
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Using function type to cleanup the function parament
2015-07-14 6:59 [PATCH 0/2] Using function type to cleanup the function parament Minfei Huang
2015-07-14 6:59 ` [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code Minfei Huang
2015-07-14 6:59 ` [PATCH 2/2] Define kallsyms_cmp_symbol_t " Minfei Huang
@ 2015-07-14 7:06 ` Minfei Huang
2 siblings, 0 replies; 10+ messages in thread
From: Minfei Huang @ 2015-07-14 7:06 UTC (permalink / raw)
To: Minfei Huang; +Cc: akpm, rob.jones, amhyung, rusty, linux-kernel
Lost the character 'n' in the Namhyung Kim <namhyung@kernel.org>.
Resent it.
On 07/14/15 at 02:59P, Minfei Huang wrote:
> From: Minfei Huang <mnfhuang@gmail.com>
>
> This patchset do the cleanup. For now, we can use function type
> as the parament to simplify the code.
>
> Previously, we will declare the function as following:
>
> bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> struct module *owner,
> void *data), void *data);
>
> Once we define the function as a type, we can just declare the function
> as following.
>
> bool each_symbol_section(find_symbol_in_section_t fn, void *data);
>
> Minfei Huang (2):
> Define find_symbol_in_section_t as function type to simplify the code
> Define kallsyms_cmp_symbol_t as function type to simplify the code
>
> include/linux/kallsyms.h | 10 +++-------
> include/linux/module.h | 19 +++++++++----------
> kernel/kallsyms.c | 4 +---
> kernel/module.c | 13 +++----------
> 4 files changed, 16 insertions(+), 30 deletions(-)
>
> --
> 2.2.2
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code
2015-07-14 6:59 ` [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code Minfei Huang
2015-07-14 7:04 ` Minfei Huang
@ 2015-07-14 21:52 ` Rusty Russell
2015-07-15 2:11 ` Minfei Huang
2015-07-15 20:31 ` Andrew Morton
1 sibling, 2 replies; 10+ messages in thread
From: Rusty Russell @ 2015-07-14 21:52 UTC (permalink / raw)
To: Minfei Huang, akpm, rob.jones, amhyung; +Cc: linux-kernel, Minfei Huang
Minfei Huang <mhuang@redhat.com> writes:
> From: Minfei Huang <mnfhuang@gmail.com>
>
> It is not elegance, if we use function directly as the argument, like
> following:
>
> bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> struct module *owner,
> void *data), void *data);
>
> Here introduce a type defined function find_symbol_in_section_t. Now
> we can use these type defined function directly, if we want to pass
> the function as the argument.
>
> bool each_symbol_section(find_symbol_in_section_t fn, void *data);
I disagree.
It's shorter, but it's less clear. typedefs on functions are not very
useful:
1) They require readers to look in two places to see how to use the
function (ie each_symbol_section).
2) They can't use the typedef to declare their function, since that
doesn't work in C.
If the function were being used many times, it makes sense. But
it's only used twice, once static inside module.c.
So I won't be applying these.
Cheers,
Rusty.
> Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> ---
> include/linux/module.h | 6 +++---
> kernel/module.c | 9 ++-------
> 2 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index d67b193..1e125b1 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -462,14 +462,14 @@ const struct kernel_symbol *find_symbol(const char *name,
> bool gplok,
> bool warn);
>
> +typedef bool (*find_symbol_in_section_t)(const struct symsearch *arr,
> + struct module *owner, void *data);
> /*
> * Walk the exported symbol table
> *
> * Must be called with module_mutex held or preemption disabled.
> */
> -bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> - struct module *owner,
> - void *data), void *data);
> +bool each_symbol_section(find_symbol_in_section_t fn, void *data);
>
> /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if
> symnum out of range. */
> diff --git a/kernel/module.c b/kernel/module.c
> index 4d2b82e..1400c0b 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -426,9 +426,7 @@ extern const unsigned long __start___kcrctab_unused_gpl[];
> static bool each_symbol_in_section(const struct symsearch *arr,
> unsigned int arrsize,
> struct module *owner,
> - bool (*fn)(const struct symsearch *syms,
> - struct module *owner,
> - void *data),
> + find_symbol_in_section_t fn,
> void *data)
> {
> unsigned int j;
> @@ -442,10 +440,7 @@ static bool each_symbol_in_section(const struct symsearch *arr,
> }
>
> /* Returns true as soon as fn returns true, otherwise false. */
> -bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> - struct module *owner,
> - void *data),
> - void *data)
> +bool each_symbol_section(find_symbol_in_section_t fn, void *data)
> {
> struct module *mod;
> static const struct symsearch arr[] = {
> --
> 2.2.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code
2015-07-14 21:52 ` Rusty Russell
@ 2015-07-15 2:11 ` Minfei Huang
2015-07-15 20:31 ` Andrew Morton
1 sibling, 0 replies; 10+ messages in thread
From: Minfei Huang @ 2015-07-15 2:11 UTC (permalink / raw)
To: Rusty Russell; +Cc: akpm, rob.jones, namhyung, linux-kernel, Minfei Huang
On 07/15/15 at 07:22am, Rusty Russell wrote:
> Minfei Huang <mhuang@redhat.com> writes:
> > From: Minfei Huang <mnfhuang@gmail.com>
> >
> > It is not elegance, if we use function directly as the argument, like
> > following:
> >
> > bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> > struct module *owner,
> > void *data), void *data);
> >
> > Here introduce a type defined function find_symbol_in_section_t. Now
> > we can use these type defined function directly, if we want to pass
> > the function as the argument.
> >
> > bool each_symbol_section(find_symbol_in_section_t fn, void *data);
>
> I disagree.
>
> It's shorter, but it's less clear. typedefs on functions are not very
> useful:
> 1) They require readers to look in two places to see how to use the
> function (ie each_symbol_section).
> 2) They can't use the typedef to declare their function, since that
> doesn't work in C.
>
> If the function were being used many times, it makes sense. But
> it's only used twice, once static inside module.c.
>
> So I won't be applying these.
>
> Cheers,
> Rusty.
>
Hi, Rusty.
The main reason why I posted these patch is the function has too much
parameters which may be confused with readers at the first glance.
So it is better define a typedef function, although the readers shall
dig out what find_symbol_in_section_t is.
Thanks
Minfei
> > Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> > ---
> > include/linux/module.h | 6 +++---
> > kernel/module.c | 9 ++-------
> > 2 files changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index d67b193..1e125b1 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -462,14 +462,14 @@ const struct kernel_symbol *find_symbol(const char *name,
> > bool gplok,
> > bool warn);
> >
> > +typedef bool (*find_symbol_in_section_t)(const struct symsearch *arr,
> > + struct module *owner, void *data);
> > /*
> > * Walk the exported symbol table
> > *
> > * Must be called with module_mutex held or preemption disabled.
> > */
> > -bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> > - struct module *owner,
> > - void *data), void *data);
> > +bool each_symbol_section(find_symbol_in_section_t fn, void *data);
> >
> > /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if
> > symnum out of range. */
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 4d2b82e..1400c0b 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -426,9 +426,7 @@ extern const unsigned long __start___kcrctab_unused_gpl[];
> > static bool each_symbol_in_section(const struct symsearch *arr,
> > unsigned int arrsize,
> > struct module *owner,
> > - bool (*fn)(const struct symsearch *syms,
> > - struct module *owner,
> > - void *data),
> > + find_symbol_in_section_t fn,
> > void *data)
> > {
> > unsigned int j;
> > @@ -442,10 +440,7 @@ static bool each_symbol_in_section(const struct symsearch *arr,
> > }
> >
> > /* Returns true as soon as fn returns true, otherwise false. */
> > -bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> > - struct module *owner,
> > - void *data),
> > - void *data)
> > +bool each_symbol_section(find_symbol_in_section_t fn, void *data)
> > {
> > struct module *mod;
> > static const struct symsearch arr[] = {
> > --
> > 2.2.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code
2015-07-14 21:52 ` Rusty Russell
2015-07-15 2:11 ` Minfei Huang
@ 2015-07-15 20:31 ` Andrew Morton
2015-07-16 11:29 ` Rusty Russell
1 sibling, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2015-07-15 20:31 UTC (permalink / raw)
To: Rusty Russell
Cc: Minfei Huang, rob.jones, amhyung, linux-kernel, Minfei Huang
On Wed, 15 Jul 2015 07:22:32 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
> Minfei Huang <mhuang@redhat.com> writes:
> > From: Minfei Huang <mnfhuang@gmail.com>
> >
> > It is not elegance, if we use function directly as the argument, like
> > following:
> >
> > bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
> > struct module *owner,
> > void *data), void *data);
> >
> > Here introduce a type defined function find_symbol_in_section_t. Now
> > we can use these type defined function directly, if we want to pass
> > the function as the argument.
> >
> > bool each_symbol_section(find_symbol_in_section_t fn, void *data);
>
> I disagree.
>
> It's shorter, but it's less clear. typedefs on functions are not very
> useful:
> 1) They require readers to look in two places to see how to use the
> function (ie each_symbol_section).
> 2) They can't use the typedef to declare their function, since that
> doesn't work in C.
>
> If the function were being used many times, it makes sense. But
> it's only used twice, once static inside module.c.
>
Using a foo_t typedef for a function callback is a common pattern.
It's (almost) the only approved use of typedefs. The usage is
widespread enough that when one sees a foo_t type, one says "ahah,
that's a function pointer".
Sorry, but I don't think "Rusty doesn't like it" is a good reason for
the module code to be different. All of us dislike some aspects of
kernel coding practices, but we go along because consistency is more
important.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code
2015-07-15 20:31 ` Andrew Morton
@ 2015-07-16 11:29 ` Rusty Russell
0 siblings, 0 replies; 10+ messages in thread
From: Rusty Russell @ 2015-07-16 11:29 UTC (permalink / raw)
To: Andrew Morton
Cc: Minfei Huang, rob.jones, amhyung, linux-kernel, Minfei Huang
Andrew Morton <akpm@linux-foundation.org> writes:
> On Wed, 15 Jul 2015 07:22:32 +0930 Rusty Russell <rusty@rustcorp.com.au> wrote:
>> It's shorter, but it's less clear. typedefs on functions are not very
>> useful:
>> 1) They require readers to look in two places to see how to use the
>> function (ie each_symbol_section).
>> 2) They can't use the typedef to declare their function, since that
>> doesn't work in C.
>>
>> If the function were being used many times, it makes sense. But
>> it's only used twice, once static inside module.c.
>>
>
> Using a foo_t typedef for a function callback is a common pattern.
> It's (almost) the only approved use of typedefs. The usage is
> widespread enough that when one sees a foo_t type, one says "ahah,
> that's a function pointer".
I always thought of a type which can map to varying types under
different arch/configs as the typical typedef.
> Sorry, but I don't think "Rusty doesn't like it" is a good reason for
> the module code to be different.
But "Rusty has to maintain it" is a pretty strong counter argument,
IMHO.
> All of us dislike some aspects of
> kernel coding practices, but we go along because consistency is more
> important.
Consistency is important when it makes things more readable, sure.
I don't think any kernel devs are going to get confused seeing a
function pointer, and I think this patch makes the code slightly
less readable.
Enough not to apply the patch, but not enough waste more time on it.
Cheers,
Rusty.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-07-16 11:35 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-14 6:59 [PATCH 0/2] Using function type to cleanup the function parament Minfei Huang
2015-07-14 6:59 ` [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code Minfei Huang
2015-07-14 7:04 ` Minfei Huang
2015-07-14 21:52 ` Rusty Russell
2015-07-15 2:11 ` Minfei Huang
2015-07-15 20:31 ` Andrew Morton
2015-07-16 11:29 ` Rusty Russell
2015-07-14 6:59 ` [PATCH 2/2] Define kallsyms_cmp_symbol_t " Minfei Huang
2015-07-14 7:05 ` Minfei Huang
2015-07-14 7:06 ` [PATCH 0/2] Using function type to cleanup the function parament Minfei Huang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.