[1/2] net/netfilter/ipvs: Move #define KMSG_COMPONENT to Makefile

This centralizes the definition and removes the
replicated #defines from all files

Signed-off-by: Joe Perches <joe@perches.com>
---
 net/netfilter/ipvs/Makefile             |    3 +++
 net/netfilter/ipvs/ip_vs_app.c          |    3 ---
 net/netfilter/ipvs/ip_vs_conn.c         |    3 ---
 net/netfilter/ipvs/ip_vs_core.c         |    3 ---
 net/netfilter/ipvs/ip_vs_ctl.c          |    3 ---
 net/netfilter/ipvs/ip_vs_dh.c           |    3 ---
 net/netfilter/ipvs/ip_vs_est.c          |    3 ---
 net/netfilter/ipvs/ip_vs_ftp.c          |    3 ---
 net/netfilter/ipvs/ip_vs_lblc.c         |    3 ---
 net/netfilter/ipvs/ip_vs_lblcr.c        |    3 ---
 net/netfilter/ipvs/ip_vs_lc.c           |    3 ---
 net/netfilter/ipvs/ip_vs_nq.c           |    3 ---
 net/netfilter/ipvs/ip_vs_proto.c        |    3 ---
 net/netfilter/ipvs/ip_vs_proto_ah_esp.c |    3 ---
 net/netfilter/ipvs/ip_vs_proto_tcp.c    |    3 ---
 net/netfilter/ipvs/ip_vs_proto_udp.c    |    3 ---
 net/netfilter/ipvs/ip_vs_rr.c           |    3 ---
 net/netfilter/ipvs/ip_vs_sched.c        |    3 ---
 net/netfilter/ipvs/ip_vs_sed.c          |    3 ---
 net/netfilter/ipvs/ip_vs_sh.c           |    3 ---
 net/netfilter/ipvs/ip_vs_sync.c         |    3 ---
 net/netfilter/ipvs/ip_vs_wlc.c          |    3 ---
 net/netfilter/ipvs/ip_vs_wrr.c          |    3 ---
 net/netfilter/ipvs/ip_vs_xmit.c         |    3 ---
 24 files changed, 3 insertions(+), 69 deletions(-)

On Thursday 2009-10-01 00:37, Joe Perches wrote:

>This centralizes the definition and removes the
>replicated #defines from all files

And increases the length of the command line. Not that Linux does not
support long command lines (in fact, configure often determines huge
possible values on the max length test), but sometimes, developers
have to inspect the command lines anyway for bugs, or something. It
is already pretty long due to all the compiler flags. Oh what were
the days of DOS programs that allowed to pass in arguments in a
so-called "response file", wish we had that in gcc.

>+ccflags-y += -D "KMSG_COMPONENT=\"IPVS\""
>+ccflags-y += -D "pr_fmt(fmt)=KMSG_COMPONENT \": \" fmt"
>+
> # IPVS transport protocol load balancing support
> ip_vs_proto-objs-y :=
> ip_vs_proto-objs-$(CONFIG_IP_VS_PROTO_TCP) += ip_vs_proto_tcp.o
>diff --git a/net/netfilter/ipvs/ip_vs_app.c b/net/netfilter/ipvs/ip_vs_app.c
>index 3c7e427..a5283d8 100644
>--- a/net/netfilter/ipvs/ip_vs_app.c
>+++ b/net/netfilter/ipvs/ip_vs_app.c
>@@ -18,9 +18,6 @@
>  *
>  */
> 
>-#define KMSG_COMPONENT "IPVS"
>-#define pr_fmt(fmt) KMSG_COMPONENT ": " fmt

How about an #include file for the ipvs private things?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2009-10-01 at 00:46 +0200, Jan Engelhardt wrote:
> On Thursday 2009-10-01 00:37, Joe Perches wrote:
> >This centralizes the definition and removes the
> >replicated #defines from all files
> And increases the length of the command line. Not that Linux does not
> support long command lines (in fact, configure often determines huge
> possible values on the max length test), but sometimes, developers
> have to inspect the command lines anyway for bugs, or something. It
> is already pretty long due to all the compiler flags.

Hi Jan.

I think this increased command line length hardly matters.

I think a reasonable complaint might be that it separates
the definition of a macro from the code.  I think it's
similar to the already used KBUILD_MODNAME macro though.

> How about an #include file for the ipvs private things?

It's not just IPVS, this style could be used treewide
without requiring extra #includes.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday 2009-10-01 01:09, Joe Perches wrote:
>On Thu, 2009-10-01 at 00:46 +0200, Jan Engelhardt wrote:
>> On Thursday 2009-10-01 00:37, Joe Perches wrote:
>> >This centralizes the definition and removes the
>> >replicated #defines from all files
>
>I think this increased command line length hardly matters.
>
>I think a reasonable complaint might be that it separates
>the definition of a macro from the code.  I think it's
>similar to the already used KBUILD_MODNAME macro though.

KBUILD_MODNAME is special in that it is derived from the actual
source filename. Of course you could put #define KBUILD_MODNAME "foo"
in your source file, but that is like putting changelogs there
when they belong into the git log.

>> How about an #include file for the ipvs private things?
>
>It's not just IPVS, this style could be used treewide
>without requiring extra #includes.

Well I personally prefer the #include instead of hiding such in 
Makefiles. You know, when newcomers could start doing `grep 
KMSG_COMPONENT *.[ch]`. Perhaps GCC's -include flag in a Makefile
to avoid #includes in .c files?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2009-10-01 at 02:31 +0200, Jan Engelhardt wrote:
> KBUILD_MODNAME is special in that it is derived from the actual
> source filename.

Kind of.  It's derived from the module name, not the filename.

> Of course you could put #define KBUILD_MODNAME "foo"
> in your source file, but that is like putting changelogs there
> when they belong into the git log.

I agree with that.

> Well I personally prefer the #include instead of hiding such in 
> Makefiles. You know, when newcomers could start doing `grep 
> KMSG_COMPONENT *.[ch]`. Perhaps GCC's -include flag in a Makefile
> to avoid #includes in .c files?

I imagine an eventual goal of standardizing the default
pr_fmt define in kernel.h to

	#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

so that all pr_<level> calls get this unless otherwise
specified.

Or perhaps better, to get rid of pr_fmt(fmt) altogether and
have printk emit the filename/modulename, function and/or
code offset by using something like %pS after the level.

I see the Makefile use, which I don't really like too much
because of the information hiding, as an intermediate step
until that's possible.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thursday 2009-10-01 02:50, Joe Perches wrote:
>On Thu, 2009-10-01 at 02:31 +0200, Jan Engelhardt wrote:
>> Well I personally prefer the #include instead of hiding such in 
>> Makefiles. You know, when newcomers could start doing `grep 
>> KMSG_COMPONENT *.[ch]`. Perhaps GCC's -include flag in a Makefile
>> to avoid #includes in .c files?
>
>I imagine an eventual goal of standardizing the default
>pr_fmt define in kernel.h to
>
>	#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
>so that all pr_<level> calls get this unless otherwise
>specified.

I like that approach. Saves me adding that line to .c
files repeatedly.

>Or perhaps better, to get rid of pr_fmt(fmt) altogether and
>have printk emit the filename/modulename, function and/or
>code offset by using something like %pS after the level.

I object to that. You would be spamming the dmesg ring buffer
with all that info, plus

filename: you would have to keep filename strings in the kernel.
Surely I do not find that thrilling when there are ~18000
non-arch .[ch] files whose pathnames amount to 542K.
Same goes similar for functions.

modulename: obj-y files would only get "<built-in>" or something
for KBUILD_MODNAME. Printing that to dmesg is not too useful.

I would rather keep plain printk as-is.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2009-10-01 at 10:27 +0200, Jan Engelhardt wrote:
> On Thursday 2009-10-01 02:50, Joe Perches wrote:
> >I imagine an eventual goal of standardizing the default
> >pr_fmt define in kernel.h to
> >	#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >so that all pr_<level> calls get this unless otherwise
> >specified.
> 
> I like that approach. Saves me adding that line to .c
> files repeatedly.

There aren't too many existing pr_<level> calls so
that this couldn't be considered.

Files with pr_<level> without pr_fmt:

$ grep -rPl --include=*.[ch] \
	"\bpr_(info|warning|err|alert|notice|crit)\b" * |
  xargs grep -Lw "pr_fmt" | wc -l
569

Uses of pr_<level> without pr_fmt:

$ grep -rPl --include=*.[ch] \
	"\bpr_(info|warning|err|alert|notice|crit)\b" * |
  xargs grep -Lw "pr_fmt" |
  xargs grep -P "\bpr_(info|warning|err|alert|notice|crit)\b" |
  wc -l
2885

If you look at the pr_<levels>, it's nearly
a mechanical thing to strip the ones with
some sort of prefix and add a #define pr_fmt
to replace them.  Most all of them without
prefixes might benefit by using a standardized
#define pr_fmt(etc...) in kernel.h, so the
actual count of changes isn't that high.

> >Or perhaps better, to get rid of pr_fmt(fmt) altogether and
> >have printk emit the filename/modulename, function and/or
> >code offset by using something like %pS after the level.
> I object to that. You would be spamming the dmesg ring buffer
> with all that info

Of course printks could not change, there are way too
many of those to consider doing that globally.

But the printks emitted by pr_<level> might change.
Maybe by setting a bit in the string "<level>" or by
some other mechanism.

> filename: you would have to keep filename strings in the kernel.
> Surely I do not find that thrilling when there are ~18000
> non-arch .[ch] files whose pathnames amount to 542K.
> Same goes similar for functions.
> 
> modulename: obj-y files would only get "<built-in>" or something
> for KBUILD_MODNAME. Printing that to dmesg is not too useful.

The removal of KBUILD_MODNAME could only be done
for builds with CONFIG_KALLSYMS or
CONFIG_DYNAMIC_DEBUG.

It might also be possible to use something like
CONFIG_DYNAMIC_DEBUG to control which modules get
MODNAME, __func__, __LINE__ or offset emitted
by the pr_<level> via some boot/module/sysconf
or FTRACE like parameters.

cheers, Joe

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html