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
prev parent 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).