Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1184167 > unrolled thread

Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code Rusty Russell <rusty@rustcorp.com.au> - 2015-07-15 02:50 +0200
    Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to  simplify the code Minfei Huang <mhuang@redhat.com> - 2015-07-15 04:10 +0200
    Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to  simplify the code Andrew Morton <akpm@linux-foundation.org> - 2015-07-15 22:40 +0200
      Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code Rusty Russell <rusty@rustcorp.com.au> - 2015-07-16 13:40 +0200

#1184167 — Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1184195 — Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1185031 — Re: [PATCH 1/2] Define find_symbol_in_section_t as function type to simplify the code

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1185776

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web