kernelnewbies.kernelnewbies.org archive mirror
 help / color / mirror / Atom feed
From: greg@kroah.com (Greg KH)
To: kernelnewbies@lists.kernelnewbies.org
Subject: parameter of module_init() and module_exit() must not be a macro
Date: Fri, 16 Oct 2015 06:59:58 -0700	[thread overview]
Message-ID: <20151016135958.GA12791@kroah.com> (raw)
In-Reply-To: <6D83E89737156549AEA25EF9ED712C5D159B91@DEFTHW99EK1MSX.ww902.siemens.net>

On Fri, Oct 16, 2015 at 06:40:40AM +0000, Warlich, Christof wrote:
> > Ick, no, don't do that, you will make life much harder for everyone
> > involved in the end.  Just write out the code "for real", it's trivial
> > to do so, and you aren't really saving anyone anytime as nothing like
> > this would ever be acceptable to the upstream kernel developers.
> > 
> > Really, you aren't saving any time/energy here, the "template" code is
> > the easiest part to write, it's what is in those functions that really
> > matters.
> 
> Ok, agreed, the CONCAT() construct looks a bit strange at first sight, although
> things could further be simplified, e.g.:
> 
> #define _CONCAT(x, y) x##y
> #define CONCAT(x, y) _CONCAT(x, y)
> #define _STRINGIFY(x) #x
> #define STRINGIFY(x) _STRINGIFY(x)
> #define DRIVER_NAMESPACE(x) CONCAT(CONCAT(DRIVER_NAME, _), x)
> 
> ...
> 
> static void __exit DRIVER_NAMESPACE(exit)(void)
> {
> 	...
> }
> 
> module_init(DRIVER_NAMESPACE(init));
> module_exit(DRIVER_NAMESPACE(exit));
> 
> But anyhow, as you are most probably right that this would not be accepted
> upstream, there is not much point in arguing further whether my example is a
> good idea.
> 
> I'd still like to make a point if the current implementation of the module_init()
> and module_exit() macros is _correct_ though. With the following code snippet:
> 
> #define FOO BAR
> module_init(FOO);
> 
> the current implementation of the module_init() macro would expand to
> 
> 	static inline initcall_t __inittest(void)		\
> 	{ return BAR; }					\
> 	int init_module(void) __attribute__((alias("FOO")));
> 
> i.e. we see both FOO and BAR in the expanded code, which is rather unexpected.
> The patch that I've suggested would fix that, as the macro would then expand to:
> 
> 	static inline initcall_t __inittest(void)		\
> 	{ return BAR; }					\
> 	int init_module(void) __attribute__((alias("BAR")));
> 
> Thus, even when not being able to give an acceptable real world example, my
> proposed patch would still enforce the principle of "least surprise". Just consider
> this similar to coding standardization patches: It would just improve overall
> code quality.
> 
> So my initial question remains: How are the chances to get my (code quality
> improvement) patch upstream?

As it doesn't fix an in-kernel issues, I doubt it's worth the effort,
but I'm not the maintainer of this portion of the kernel, so I do not
know if it will be accepted or not, sorry.

good luck!

greg k-h

      reply	other threads:[~2015-10-16 13:59 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-15  9:48 parameter of module_init() and module_exit() must not be a macro Warlich, Christof
2015-10-15 13:55 ` Greg KH
2015-10-15 14:26   ` AW: " Warlich, Christof
2015-10-15 15:44     ` Greg KH
2015-10-16  6:40       ` AW: " Warlich, Christof
2015-10-16 13:59         ` Greg KH [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20151016135958.GA12791@kroah.com \
    --to=greg@kroah.com \
    --cc=kernelnewbies@lists.kernelnewbies.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).