[RFC] move #define pr_fmt KBUILD_MODNAME and KMSG_COMPONENT to Makefiles?

[RFC] move #define pr_fmt KBUILD_MODNAME and KMSG_COMPONENT to Makefiles?

[Joe Perches]

There are starting to be more uses of

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

and

#define KMSG_COMPONENT "foo"
#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt

in kernel sources.

Perhaps it would be better to put these
defines in the appropriate Makefile and not
sprinkle the sources with them multiple times?

Re: [RFC] move #define pr_fmt KBUILD_MODNAME and KMSG_COMPONENT to Makefiles?

[Sam Ravnborg]

Hi Joe.

On Tue, Sep 29, 2009 at 11:15:04AM -0700, Joe Perches wrote:
> There are starting to be more uses of
> 
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> and
> 
> #define KMSG_COMPONENT "foo"
> #define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
> 
> in kernel sources.
> 
> Perhaps it would be better to put these
> defines in the appropriate Makefile and not
> sprinkle the sources with them multiple times?

Maybe I'm a bit dense but I do not quiete follow you
here. What is it you want moved to kbuild here?

To me it looks more like a logical extension to the
pr_* macros based on info we already have available.

	Sam

Re: [RFC] move #define pr_fmt KBUILD_MODNAME and KMSG_COMPONENT to Makefiles?

[Joe Perches]

On Wed, 2009-09-30 at 08:55 +0200, Sam Ravnborg wrote:
> On Tue, Sep 29, 2009 at 11:15:04AM -0700, Joe Perches wrote:
> > There are starting to be more uses of
> > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > and
> > #define KMSG_COMPONENT "foo"
> > #define pr_fmt(fmt) KMSG_COMPONENT ": " fmt
> > in kernel sources.
> > 
> > Perhaps it would be better to put these
> > defines in the appropriate Makefile and not
> > sprinkle the sources with them multiple times?
> 
> Maybe I'm a bit dense but I do not quite follow you
> here. What is it you want moved to kbuild here?
> 
> To me it looks more like a logical extension to the
> pr_* macros based on info we already have available.

Hi Sam.

I agree it's a logical extension to what exists
already, unfortunately there are already many
uses of pr_<level> calls, some that use pr_fmt(),
some that use an embedded constant string,
and some with nothing.  Using a predeclared
standardized pr_fmt would be a problem for
those calls that use embedded constant strings.

So today, every file in a module that uses
pr_fmt #defines pr_fmt before the #includes.

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

There are starting to be a lot of these and
likely there will be a lot more unless the
defines are standardized somehow.

Per Makefile ccflags would allow a gradual
conversion of the many pr_<level> calls that
are already prefixed with nothing, or constant
strings, or already use pr_fmt with either
KBUILD_MODNAME, KMSG_COMPONENT, or some
other fixed string.

If something like were added to a module Makefile:

	ccflags-y += -D "pr_fmt(fmt)=KBUILD_MODNAME \": \" fmt"

or if necessary a per-file entry in the Makefile:

	CFLAGS_foo.o += -D "pr_fmt(fmt)=KBUILD_MODNAME \": \" fmt"

the number of these #defines pr_fmt in source could be
reduced.

These new Makefiles -D entries could later be stripped
and the pr_fmt define standardized perhaps in the top
level Makefile after the existing pr_<level> calls with
embedded prefix strings in source are modified as necessary.

Something similar could be done in the Makefiles for
KMSG_COMPONENT uses as well.

cheers, Joe

Re: [RFC] move #define pr_fmt KBUILD_MODNAME and KMSG_COMPONENT to Makefiles?

[Martin Schwidefsky]

On Wed, 30 Sep 2009 00:54:59 -0700
Joe Perches <joe@perches.com> wrote:

> If something like were added to a module Makefile:
> 
> 	ccflags-y += -D "pr_fmt(fmt)=KBUILD_MODNAME \": \" fmt"

That would define a pr_fmt macro for ALL source files. That would add
the prefix to every single pr_xxx printk. That a) changes the text of
these printk and b) makes printks that already have a different prefix
look very funny. The point is that you need to adapt you source file in
order to use the pr_fmt prefix mechanis, no automatic conversion is
possible.

> or if necessary a per-file entry in the Makefile:
> 
> 	CFLAGS_foo.o += -D "pr_fmt(fmt)=KBUILD_MODNAME \": \" fmt"

Where is the difference between the definition of the macro in the
source file? It's still one additional line, no? And if you are
dreaming of converting all source files to the pr_fmt mechanism, this
is a big effort ..

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [RFC] move #define pr_fmt KBUILD_MODNAME and KMSG_COMPONENT to Makefiles?

[Joe Perches]

On Wed, 2009-09-30 at 11:20 +0200, Martin Schwidefsky wrote:
> On Wed, 30 Sep 2009 00:54:59 -0700
> Joe Perches <joe@perches.com> wrote:
> > If something like were added to a module Makefile:
> > 	ccflags-y += -D "pr_fmt(fmt)=KBUILD_MODNAME \": \" fmt"
> 
> Where is the difference between the definition of the macro in the 
> source file? It's still one additional line, no?

Look at net/netfilter/ipvs for instance.

It would have been possible to add:

	ccflags-y += -D "KMSG_COMPONENT=IPVS"
	ccflags-y += -D "pr_fmt(fmt)=KMSG_COMPONENT \": \" fmt"

to net/netfilter/ipvs/Makefile instead of adding it
to 23 files.

Same sort of thing for drivers/s390/char/Makefile,
though it's less beneficial there.

Multiple
	CFLAGS_foo.o += -D "KMSG_COMPONENT=foo"
and a single
	ccflags-y += -D "pr_fmt(fmt)=KMSG_COMPONENT \": \" fmt"

> And if you are dreaming of converting all source files
> to the pr_fmt mechanism, this is a big effort ..

I think it could be a useful mechanism to define pr_fmt
for individual module Makefiles and there's no rush...

cheers, Joe

Re: [RFC] move #define pr_fmt KBUILD_MODNAME and KMSG_COMPONENT to Makefiles?

[Martin Schwidefsky]

On Wed, 30 Sep 2009 07:52:58 -0700
Joe Perches <joe@perches.com> wrote:

> On Wed, 2009-09-30 at 11:20 +0200, Martin Schwidefsky wrote:
> > On Wed, 30 Sep 2009 00:54:59 -0700
> > Joe Perches <joe@perches.com> wrote:
> > > If something like were added to a module Makefile:
> > > 	ccflags-y += -D "pr_fmt(fmt)=KBUILD_MODNAME \": \" fmt"
> > 
> > Where is the difference between the definition of the macro in the 
> > source file? It's still one additional line, no?
> 
> Look at net/netfilter/ipvs for instance.
> 
> It would have been possible to add:
> 
> 	ccflags-y += -D "KMSG_COMPONENT=IPVS"
> 	ccflags-y += -D "pr_fmt(fmt)=KMSG_COMPONENT \": \" fmt"
> 
> to net/netfilter/ipvs/Makefile instead of adding it
> to 23 files.

That can make sense if you have lots of files that use the same
KMSG_COMPONENT. 

> Same sort of thing for drivers/s390/char/Makefile,
> though it's less beneficial there.
> 
> Multiple
> 	CFLAGS_foo.o += -D "KMSG_COMPONENT=foo"
> and a single
> 	ccflags-y += -D "pr_fmt(fmt)=KMSG_COMPONENT \": \" fmt"
> 
> > And if you are dreaming of converting all source files
> > to the pr_fmt mechanism, this is a big effort ..
> 
> I think it could be a useful mechanism to define pr_fmt
> for individual module Makefiles and there's no rush...

If you need multiple
	CFLAGS_foo.o += -D "KMSG_COMPONENT=foo"
lines the benefit of the Makfile method is limited. In general I find
the code to be less clear if I have to search for the message prefix in
the Makefile instead of the start of the C file. Personal preference I
guess.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.