[patch 0/3] [RFC] kmsg macros and script, take x+1.

[patch 0/3] [RFC] kmsg macros and script, take x+1.

[Martin Schwidefsky]

Greetings,
the last kmsg patches on lkml got some useful feedback and Andrew advised
to extend the scope of the this discussion to lf_kernel_messages.  I've
added all individuals I could find who participated in the kernel messages
discussions last year.
Since then we continued to work on the kernel messages for s390 and the
three patches in this series is the gist of it.

The lastest changes:
 * kmsg.h has been moved from include/asm-s390 to include/linux
 * The s390 specific documentation is now in Documentation/kmsg/s390
 * The script checks for messages in the source file, Documentation/kmsg
   and Documentation/kmsg/$SRCARCH
 * I've reduced the number of patches to three, a single example how
   the kmsg interface is used should be enough.

The initial approach has been to keep the kmsg interface local to s390,
but Andrew thinks it has potential for common use. That is where all
the people on Cc: come into play: what is your opinion? Can we go ahead
with it?

-- 
blue skies,
   Martin.

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

[patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

[-- Attachment #1: 800-kmsg-macros.diff --]
[-- Type: text/plain, Size: 6281 bytes --]

From: Michael Holzheu <holzheu@de.ibm.com>
From: Martin Schwidefsky <schwidefsky@de.ibm.com>

Introduce a new family of printk macros which prefixes each kmsg message
with a component name and allows to tag the printk with a message id.

The kmsg component name is defined per source file with the KMSG_COMPONENT
macro. The first argument of each kmsg printk is the message id. The
message id "0" is special as it will suppress the message id prefix.

If the message id will be printed to the console / syslog at all depends
on CONFIG_MSG_IDS. If it is "n" then a kmsg_xxx call is just another
printk wrapper. These macros are intended to be used uniformly in the
s390 architecture and the s390 device drivers.

Signed-off-by: Michael Holzheu <holzheu@de.ibm.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---

 arch/s390/Kconfig    |    9 +++
 include/linux/kmsg.h |  124 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+)

Index: linux-2.6/arch/s390/Kconfig
===================================================================
--- linux-2.6.orig/arch/s390/Kconfig
+++ linux-2.6/arch/s390/Kconfig
@@ -568,6 +568,15 @@ bool "s390 guest support (EXPERIMENTAL)"
 	select VIRTIO_CONSOLE
 	help
 	  Select this option if you want to run the kernel under s390 linux
+
+config KMSG_IDS
+	bool "Kernel message numbers"
+	default y
+	help
+	  Select this option if you want to include a message number to the
+	  prefix for kernel messages issued by the s390 architecture and
+	  driver code. See "Documentation/s390/kmsg.txt" for more details.
+
 endmenu
 
 source "net/Kconfig"
Index: linux-2.6/include/linux/kmsg.h
===================================================================
--- /dev/null
+++ linux-2.6/include/linux/kmsg.h
@@ -0,0 +1,124 @@
+#ifndef _LINUX_KMSG_H
+#define _LINUX_KMSG_H
+
+#ifndef __KMSG_CHECKER
+#define __KMSG_CHECK(level, id) KERN_##level
+#endif
+
+#if defined(__KMSG_CHECKER) || !defined(CONFIG_KMSG_IDS)
+
+#define kmsg_dev_alert(id, dev, format, arg...) \
+	printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
+		": %s: " format, (dev)->bus_id , ## arg)
+
+#define kmsg_dev_err(id, dev, format, arg...) \
+	printk(__KMSG_CHECK(ERR, id) KMSG_COMPONENT \
+		": %s: " format, (dev)->bus_id , ## arg)
+
+#define kmsg_dev_warn(id, dev, format, arg...) \
+	printk(__KMSG_CHECK(WARNING, id) KMSG_COMPONENT \
+		": %s: " format, (dev)->bus_id , ## arg)
+
+#define kmsg_dev_info(id, dev, format, arg...) \
+	printk(__KMSG_CHECK(INFO, id) KMSG_COMPONENT \
+		": %s: " format, (dev)->bus_id , ## arg)
+
+#define kmsg_dev_notice(id, dev, format, arg...) \
+	printk(__KMSG_CHECK(NOTICE, id) KMSG_COMPONENT \
+		": %s: " format, (dev)->bus_id , ## arg)
+
+#define kmsg_alert(id, format, arg...) \
+	printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
+		": " format, ## arg)
+
+#define kmsg_err(id, format, arg...) \
+	printk(__KMSG_CHECK(ERR, id) KMSG_COMPONENT \
+		": " format, ## arg)
+
+#define kmsg_warn(id, format, arg...) \
+	printk(__KMSG_CHECK(WARNING, id) KMSG_COMPONENT \
+		": " format, ## arg)
+
+#define kmsg_info(id, format, arg...) \
+	printk(__KMSG_CHECK(INFO, id) KMSG_COMPONENT \
+		": " format, ## arg)
+
+#define kmsg_notice(id, format, arg...) \
+	printk(__KMSG_CHECK(NOTICE, id) KMSG_COMPONENT \
+		": " format, ## arg)
+
+#else  /* __KMSG_CHECKER || !CONFIG_KMSG_IDS */
+
+#define kmsg_dev_alert(id, dev, format, arg...) \
+	(__builtin_constant_p(id) && (id) > 0) ? \
+		printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
+			"." #id ": %s: " format, (dev)->bus_id , ## arg) : \
+		printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
+			": %s: " format, (dev)->bus_id , ## arg)
+
+#define kmsg_dev_err(id, dev, format, arg...) \
+	(__builtin_constant_p(id) && (id) > 0) ? \
+		printk(__KMSG_CHECK(ERR, id) KMSG_COMPONENT \
+			"." #id ": %s: " format, (dev)->bus_id , ## arg) : \
+		printk(__KMSG_CHECK(ERR, id) KMSG_COMPONENT \
+			": %s: " format, (dev)->bus_id , ## arg)
+
+#define kmsg_dev_warn(id, dev, format, arg...) \
+	(__builtin_constant_p(id) && (id) > 0) ? \
+		printk(__KMSG_CHECK(WARNING, id) KMSG_COMPONENT \
+			"." #id ": %s: " format, (dev)->bus_id , ## arg) : \
+		printk(__KMSG_CHECK(WARNING, id) KMSG_COMPONENT \
+			": %s: " format, (dev)->bus_id , ## arg)
+
+#define kmsg_dev_info(id, dev, format, arg...) \
+	(__builtin_constant_p(id) && (id) > 0) ? \
+		printk(__KMSG_CHECK(INFO, id) KMSG_COMPONENT \
+			"." #id ": %s: " format, (dev)->bus_id , ## arg) : \
+		printk(__KMSG_CHECK(INFO, id) KMSG_COMPONENT \
+			": %s: " format, (dev)->bus_id , ## arg)
+
+#define kmsg_dev_notice(id, dev, format, arg...) \
+	(__builtin_constant_p(id) && (id) > 0) ? \
+		printk(__KMSG_CHECK(NOTICE, id) KMSG_COMPONENT \
+			"." #id ": %s: " format, (dev)->bus_id , ## arg) : \
+		printk(__KMSG_CHECK(NOTICE, id) KMSG_COMPONENT \
+			": %s: " format, (dev)->bus_id , ## arg)
+
+#define kmsg_alert(id, format, arg...) \
+	(__builtin_constant_p(id) && (id) > 0) ? \
+		printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
+			"." #id ": " format, ## arg) : \
+		printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
+			": " format, ## arg)
+
+#define kmsg_err(id, format, arg...) \
+	(__builtin_constant_p(id) && (id) > 0) ? \
+		printk(__KMSG_CHECK(ERR, id) KMSG_COMPONENT \
+			"." #id ": " format, ## arg) : \
+		printk(__KMSG_CHECK(ERR, id) KMSG_COMPONENT \
+			": " format, ## arg)
+
+#define kmsg_warn(id, format, arg...) \
+	(__builtin_constant_p(id) && (id) > 0) ? \
+		printk(__KMSG_CHECK(WARNING, id) KMSG_COMPONENT \
+			"." #id ": " format, ## arg) : \
+		printk(__KMSG_CHECK(WARNING, id) KMSG_COMPONENT \
+			": " format, ## arg)
+
+#define kmsg_info(id, format, arg...) \
+	(__builtin_constant_p(id) && (id) > 0) ? \
+		printk(__KMSG_CHECK(INFO, id) KMSG_COMPONENT \
+			"." #id ": " format, ## arg) : \
+		printk(__KMSG_CHECK(INFO, id) KMSG_COMPONENT \
+			": " format, ## arg)
+
+#define kmsg_notice(id, format, arg...) \
+	(__builtin_constant_p(id) && (id) > 0) ? \
+		printk(__KMSG_CHECK(NOTICE, id) KMSG_COMPONENT \
+			"." #id ": " format, ## arg) : \
+		printk(__KMSG_CHECK(NOTICE, id) KMSG_COMPONENT \
+			": " format, ## arg)
+
+#endif /* __KMSG_CHECKER || !CONFIG_KMSG_IDS */
+
+#endif /* _LINUX_KMSG_H */

-- 
blue skies,
   Martin.

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

[patch 2/3] kmsg: Kernel message catalog script.

[Martin Schwidefsky]

[-- Attachment #1: 801-kmsg-script.diff --]
[-- Type: text/plain, Size: 13977 bytes --]

From: Michael Holzheu <holzheu@de.ibm.com>
From: Martin Schwidefsky <schwidefsky@de.ibm.com>

Add a script and the calls to the make process that allows to check the
kmsg printk messages and to format man pages from the message descriptions.

The kmsg message description is a comment with the following format:

/*?
 * Tag: <component>.<id>
 * Text: "<kmsg message text>"
 * Severity: <severity>
 * Parameter:
 *   @1: <description of the first message parameter>
 *   @2: <description of the second message parameter>
 *   ...
 * Description:
 * <What is the kmsg message all about>
 * User action:
 * <What can the user do to fix the problem>
 */

The script looks for a kmsg comment for a kmsg printk at three places,
the source file where the kmsg call is located, in the architecture
dependent file Documentation/kmsg/$ARCH/<component> and in the common
file Documentation/kmsg/<component>.

The kmsg check is invoked with "make D=1" and reads the source files for
all objects that are built by the current configuration and searches for
matching kmsg descriptions for the kmsg messages in the source which
have a messages id > 0. If a message description can not be found the
script prints a blueprint and causes a make error.

The kmsg man page creation is invoked with "make D=2" and reads the source
files for all built objects, looks up the message description and writes
a man page to $(objtree)/man.

Signed-off-by: Michael Holzheu <holzheu@de.ibm.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---

 Makefile               |   16 ++
 scripts/Makefile.build |   14 +
 scripts/kmsg-doc       |  366 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 396 insertions(+)

Index: linux-2.6/Makefile
===================================================================
--- linux-2.6.orig/Makefile
+++ linux-2.6/Makefile
@@ -63,6 +63,20 @@ ifndef KBUILD_CHECKSRC
   KBUILD_CHECKSRC = 0
 endif
 
+# Call message checker as part of the C compilation
+#
+# Use 'make D=1' to enable checking
+# Use 'make D=2' to create the message catalog
+
+ifdef D
+  ifeq ("$(origin D)", "command line")
+    KBUILD_KMSG_CHECK = $(D)
+  endif
+endif
+ifndef KBUILD_KMSG_CHECK
+  KBUILD_KMSG_CHECK = 0
+endif
+
 # Use make M=dir to specify directory of external module to build
 # Old syntax make ... SUBDIRS=$PWD is still supported
 # Setting the environment variable KBUILD_EXTMOD take precedence
@@ -321,6 +335,7 @@ PERL		= perl
 CHECK		= sparse
 
 CHECKFLAGS     := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ -Wbitwise $(CF)
+KMSG_CHECK	= $(srctree)/scripts/kmsg-doc
 MODFLAGS	= -DMODULE
 CFLAGS_MODULE   = $(MODFLAGS)
 AFLAGS_MODULE   = $(MODFLAGS)
@@ -355,6 +370,7 @@ export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODU
 export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS
 export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE
 export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE
+export KBUILD_KMSG_CHECK KMSG_CHECK
 
 # When compiling out-of-tree modules, put MODVERDIR in the module
 # tree rather than in the kernel tree. The kernel tree might
Index: linux-2.6/scripts/kmsg-doc
===================================================================
--- /dev/null
+++ linux-2.6/scripts/kmsg-doc
@@ -0,0 +1,366 @@
+#!/usr/bin/perl -w
+#
+# kmsg kernel messages check and print tool.
+#
+# To check the source code for missing messages the script is called
+# with check, the name compiler and the compile parameters
+#	kmsg-doc check $(CC) $(c_flags) $<
+# To create man pages for the messages the script is called with
+#	kmsg-doc print $(CC) $(c_flags) $<
+#
+# Copyright IBM Corp. 2008
+# Author(s):  Martin Schwidefsky <schwidefsky@de.ibm.com>
+#	      Michael Holzheu <holzheu@linux.vnet.ibm.com>
+#
+
+use Cwd;
+
+my $errors = 0;
+my $warnings = 0;
+my $srctree = "";
+my $objtree = "";
+my $kmsg_count = 0;
+
+sub add_kmsg_desc($$$$$$)
+{
+    my ($tag, $text, $sev, $argv, $desc, $user) = @_;
+
+    if ($kmsg_desc{$tag}) {
+	warn "Duplicate message with tag $tag\n";
+	$errors++;
+	return;
+    }
+    $text =~ s/\" \"//g; # remove ...
+    $kmsg_desc{$tag}->{'TEXT'} = $text;
+    $kmsg_desc{$tag}->{'SEV'} = $sev;
+    $kmsg_desc{$tag}->{'ARGV'} = $argv;
+    $kmsg_desc{$tag}->{'DESC'} = $desc;
+    $kmsg_desc{$tag}->{'USER'} = $user;
+}
+
+sub add_kmsg_print($$$$$)
+{
+    my ($component, $id, $text, $sev, $argv) = @_;
+    my ($tag, $count, $parm);
+
+    if ($id == 0) {
+	return;
+    }
+    $text =~ s/\\n//g; # remove trailing newline character
+    $text =~ s/\" \"//g; # remove ...
+    $text =~ s/$component:\s*//g;
+    $tag = $component . "." . $id;
+    # Pretty print severity
+    $sev =~ s/EMERG/Emerg/;
+    $sev =~ s/ALERT/Alert/;
+    $sev =~ s/CRIT/Critical/;
+    $sev =~ s/ERR/Error/;
+    $sev =~ s/WARNING/Warning/;
+    $sev =~ s/NOTICE/Notice/;
+    $sev =~ s/INFO/Informational/;
+    $sev =~ s/DEBUG/Debug/;
+    $kmsg_print{$kmsg_count}->{'TAG'} = $tag;
+    $kmsg_print{$kmsg_count}->{'TEXT'} = $text;
+    $kmsg_print{$kmsg_count}->{'SEV'} = $sev;
+    $kmsg_print{$kmsg_count}->{'ARGV'} = $argv;
+    $kmsg_count += 1;
+}
+
+sub process_source_file($)
+{
+    my $file = "@_";
+    my $state;
+    my $component = "";
+    my ($tag, $text, $sev, $argv, $desc, $user);
+
+    if (!open(FD, "$file")) {
+	return "";
+    }
+
+    $state = 0;
+    while (<FD>) {
+	chomp;
+	# kmsg message component: #define KMSG_COMPONENT "<id>"
+	if (/^#define\s+KMSG_COMPONENT\s+\"(.*)\"[^\"]*$/o) {
+	    $component = $1;
+	}
+	if ($state == 0) {
+	    # kmsg message start: '/*?'
+	    if (/^\s*\/\*\?\s*$/o) {
+		$state = 1;
+		($tag, $text, $sev, $argv, $desc, $user) = ( "", "", "", "", "", "" );
+	    }
+	} elsif ($state == 1) {
+	    # kmsg message tag: ' * Tag: <tag>'
+	    if (/^\s*\*\s*Tag:\s*(\S*)\s*$/o) {
+		$tag = $1;
+	    }
+	    # kmsg message text: ' * Text: "<message>"'
+	    elsif (/^\s*\*\s*Text:\s*\"(.*)\"\s*$/o) {
+		$text = $1;
+	    }
+	    # kmsg message severity: ' * Severity: <sev>'
+	    elsif (/^\s*\*\s*Severity:\s*(\S*)\s*$/o) {
+		$sev = $1;
+	    }
+	    # kmsg message parameter: ' * Parameter: <argv>'
+	    elsif (/^\s*\*\s*Parameter:\s*(\S*)\s*$/o) {
+		if (!defined($1)) {
+		    $argv = "";
+		} else {
+		    $argv = $1;
+		}
+		$state = 2;
+	    }
+	    # kmsg message description start: ' * Description:'
+	    elsif (/^\s*\*\s*Description:\s*(\S*)\s*$/o) {
+		if (!defined($1)) {
+		    $desc = "";
+		} else {
+		    $desc = $1;
+		}
+		$state = 3;
+	    }
+	    # kmsg has unrecognizable lines
+	    else {
+		warn "Warning(${file}:$.): Cannot understand $_";
+		$warnings++;
+		$state = 0;
+	    }
+	} elsif ($state == 2) {
+	    # kmsg message end: ' */'
+	    if (/^\s*\*\//o) {
+		warn "Warning(${file}:$.): Missing description, skipping message";
+		$warnings++;
+		$state = 0;
+	    }
+	    # kmsg message description start: ' * Description:'
+	    elsif (/^\s*\*\s*Description:\s*$/o) {
+		$desc = $1;
+		$state = 3;
+	    }
+	    # kmsg message parameter line: ' * <argv>'
+	    elsif (/^\s*\*(.*)$/o) {
+		$argv .= "\n" . $1;
+	    } else {
+		warn "Warning(${file}:$.): Cannot understand $_";
+		$warnings++;
+		$state = 0;
+	    }
+	} elsif ($state == 3) {
+	    # kmsg message end: ' */'
+	    if (/^\s*\*\/\s*/o) {
+		add_kmsg_desc($tag, $text, $sev, $argv, $desc, $user);
+		$state = 0;
+	    }
+	    # kmsg message description start: ' * User action:'
+	    elsif (/^\s*\*\s*User action:\s*$/o) {
+		$user = $1;
+		$state = 4;
+	    }
+	    # kmsg message description line: ' * <text>'
+	    elsif (/^\s*\*\s*(.*)$/o) {
+		$desc .= "\n" . $1;
+	    } else {
+		warn "Warning(${file}:$.): Cannot understand $_";
+		$warnings++;
+		$state = 0;
+	    }
+	} elsif ($state == 4) {
+	    # kmsg message end: ' */'
+	    if (/^\s*\*\/\s*/o) {
+		add_kmsg_desc($tag, $text, $sev, $argv, $desc, $user);
+		$state = 0;
+	    }
+	    # kmsg message user action line: ' * <text>'
+	    elsif (/^\s*\*\s*(.*)$/o) {
+		$user .= "\n" . $1;
+	    } else {
+		warn "Warning(${file}:$.): Cannot understand $_";
+		$warnings++;
+		$state = 0;
+	    }
+	}
+    }
+    return $component;
+}
+
+sub process_cpp_file($$$$)
+{
+    my ($cc, $options, $file, $component) = @_;
+
+    open(FD, "$cc $gcc_options|") or die ("Preprocessing failed.");
+
+    while (<FD>) {
+	chomp;
+	if (/.*printk\(\s*__KMSG_CHECK\s*\(([^,]*)\,\s*(\d+)\s*\)\s*\"(.*)\"\s*,\s*(.*)\s*\)/o) {
+	    if ($component ne "") {
+		add_kmsg_print($component, $2, $3, $1, $4)
+	    } else {
+		warn "Error(${file}:$.): kmsg without component\n";
+		$errors++;
+	    }
+	} elsif (/.*printk\(\s*__KMSG_CHECK\s*\(([^,]*)\,\s*(\d+)\s*\)\s*\"(.*)\"\s*(.*)\s*\)/o) {
+	    if ($component ne "") {
+		add_kmsg_print($component, $2, $3, $1, $4)
+	    } else {
+		warn "Error(${file}:$.): kmsg without component\n";
+		$errors++;
+	    }
+	}
+    }
+}
+
+sub check_messages($)
+{
+    my $component = "@_";
+    my $failed = 0;
+
+    for ($i = 0; $i < $kmsg_count; $i++) {
+	$tag = $kmsg_print{$i}->{'TAG'};
+	if (!defined($kmsg_desc{$tag})) {
+	    add_kmsg_desc($tag,
+			  $kmsg_print{$i}->{'TEXT'},
+			  $kmsg_print{$i}->{'SEV'},
+			  $kmsg_print{$i}->{'ARGV'},
+			  "Please insert description here",
+			  "What is the user supposed to do");
+	    $kmsg_desc{$tag}->{'CHECK'} = 1;
+	    $failed = 1;
+	    warn "$component: Missing description for: $tag\n";
+	    $errors++;
+	    next;
+	}
+	if ($kmsg_print{$i}->{'TEXT'} ne $kmsg_desc{$tag}->{'TEXT'}) {
+	    warn "$component: format string mismatch for: $tag\n";
+	    warn "  --- $kmsg_print{$i}->{'TEXT'}\n";
+	    warn "  +++ $kmsg_desc{$tag}->{'TEXT'}\n";
+	    $errors++;
+	}
+    }
+    return $failed;
+}
+
+sub print_templates()
+{
+    print "Templates for missing messages:\n";
+    @tags = keys %kmsg_desc;
+    @nums = ();
+    for (@tags) {
+	push @nums, /\.(\d+)/;
+    }
+    foreach $tag (@tags[ sort { $nums[$a] <=> $nums[$b] } 0..$#tags ]) {
+	if (!defined($kmsg_desc{$tag}->{'CHECK'})) {
+	    next;
+	}
+	print "/*?\n";
+	print " * Tag: $tag\n";
+	print " * Text: \"$kmsg_desc{$tag}->{'TEXT'}\"\n";
+	print " * Severity: $kmsg_desc{$tag}->{'SEV'}\n";
+	$argv = $kmsg_desc{$tag}->{'ARGV'};
+	if ($argv ne "") {
+	    print " * Parameter:\n";
+	    @parms = split(/\s*,\s*/,$kmsg_desc{$tag}->{'ARGV'});
+	    $count = 0;
+	    foreach $parm (@parms) {
+		$count += 1;
+		if (!($parm eq "")) {
+		    print " *   \@$count: $parm\n";
+		}
+	    }
+	}
+	print " * Description:\n";
+	print " * $kmsg_desc{$tag}->{'DESC'}\n";
+	print " * User action:\n";
+	print " * $kmsg_desc{$tag}->{'USER'}\n";
+	print " */\n\n";
+    }
+}
+
+sub write_man_pages()
+{
+    my $file;
+
+    foreach $tag (keys(%kmsg_desc)) {
+	if (defined($kmsg_desc{$tag}->{'CHECK'})) {
+	    next;
+	}
+	$file = $objtree . "man/" . $tag;
+	if (!open(WR, ">$file")) {
+	    warn "Error: Cannot open file $file\n";
+	    $errors++;
+	    return;
+	}
+	print WR ".TH \"$tag\" 9 \"Linux Messages\" LINUX\n";
+	print WR ".SH Message\n";
+	print WR $tag . ": " . $kmsg_desc{$tag}->{'TEXT'} . "\n";
+	print WR ".SH Severity\n";
+	print WR "$kmsg_desc{$tag}->{'SEV'}\n";
+	$argv = $kmsg_desc{$tag}->{'ARGV'};
+	if ($argv ne "") {
+	    print WR ".SH Parameters\n";
+	    @parms = split(/,/,$kmsg_desc{$tag}->{'ARGV'});
+	    map{s/\s*//} @parms;
+	    foreach $parm (@parms) {
+		$parm =~ s/^\s+//;
+		if (!($parm eq "")) {
+		    print WR "$parm\n";
+		}
+	    }
+	}
+	print WR ".SH Description";
+	print WR "$kmsg_desc{$tag}->{'DESC'}\n";
+	$user = $kmsg_desc{$tag}->{'USER'};
+	if ($user ne "") {
+	    print WR ".SH User action";
+	    print WR "$user\n";
+	}
+    }
+}
+
+if (defined($ENV{'srctree'})) {
+    $srctree = "$ENV{'srctree'}" . "/";
+} else {
+    $srctree = getcwd;
+}
+
+if (defined($ENV{'objtree'})) {
+    $objtree = "$ENV{'objtree'}" . "/";
+} else {
+    $objtree = getcwd;
+}
+
+if (defined($ENV{'SRCARCH'})) {
+    $srcarch = "$ENV{'SRCARCH'}" . "/";
+} else {
+    print "kmsg-doc called without a valid \$SRCARCH\n";
+    exit 1;
+}
+
+$option = shift;
+
+$cc = shift;
+$gcc_options = "-E -D __KMSG_CHECKER ";
+foreach $tmp (@ARGV) {
+    $tmp =~ s/\(/\\\(/;
+    $tmp =~ s/\)/\\\)/;
+    $gcc_options .= " $tmp";
+    $filename = $tmp;
+}
+
+$component = process_source_file($filename);
+if ($component ne "") {
+    process_source_file($srctree . "Documentation/kmsg/" . $srcarch . $component);
+    process_source_file($srctree . "Documentation/kmsg/" . $component);
+}
+
+if ($option eq "check") {
+    process_cpp_file($cc, $gcc_options, $filename, $component);
+    if (check_messages($component)) {
+	print_templates();
+    }
+} elsif ($option eq "print") {
+    write_man_pages();
+}
+
+exit($errors);
Index: linux-2.6/scripts/Makefile.build
===================================================================
--- linux-2.6.orig/scripts/Makefile.build
+++ linux-2.6/scripts/Makefile.build
@@ -211,12 +211,14 @@ endef
 # Built-in and composite module parts
 $(obj)/%.o: $(src)/%.c FORCE
 	$(call cmd,force_checksrc)
+	$(call cmd,force_check_kmsg)
 	$(call if_changed_rule,cc_o_c)
 
 # Single-part modules are special since we need to mark them in $(MODVERDIR)
 
 $(single-used-m): $(obj)/%.o: $(src)/%.c FORCE
 	$(call cmd,force_checksrc)
+	$(call cmd,force_check_kmsg)
 	$(call if_changed_rule,cc_o_c)
 	@{ echo $(@:.o=.ko); echo $@; } > $(MODVERDIR)/$(@F:.o=.mod)
 
@@ -339,6 +341,18 @@ $(multi-used-m) : %.o: $(multi-objs-m) F
 
 targets += $(multi-used-y) $(multi-used-m)
 
+# kmsg check tool
+ifneq ($(KBUILD_KMSG_CHECK),0)
+  ifeq ($(KBUILD_KMSG_CHECK),2)
+    kmsg_cmd := print
+    quiet_cmd_force_check_kmsg = KMSG_PRINT $<
+    $(shell [ -d $(objtree)/man ] || mkdir -p $(objtree)/man)
+  else
+    kmsg_cmd := check
+    quiet_cmd_force_check_kmsg = KMSG_CHECK $<
+  endif
+  cmd_force_check_kmsg = $(KMSG_CHECK) $(kmsg_cmd) $(CC) $(c_flags) $< ;
+endif
 
 # Descending
 # ---------------------------------------------------------------------------

-- 
blue skies,
   Martin.

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

[patch 3/3] kmsg: convert xpram messages to kmsg api.

[Martin Schwidefsky]

[-- Attachment #1: 806-kmsg-xpram.diff --]
[-- Type: text/plain, Size: 6251 bytes --]

From: Martin Schwidefsky <schwidefsky@de.ibm.com>

Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---

 Documentation/kmsg/s390/xpram |   54 ++++++++++++++++++++++++++++++++++++++++++
 drivers/s390/block/xpram.c    |   41 ++++++++++++++-----------------
 2 files changed, 73 insertions(+), 22 deletions(-)

Index: linux-2.6/Documentation/kmsg/s390/xpram
===================================================================
--- /dev/null
+++ linux-2.6/Documentation/kmsg/s390/xpram
@@ -0,0 +1,54 @@
+/*?
+ * Tag: xpram.1
+ * Text: "%d is not a valid number of XPRAM devices"
+ * Severity: Error
+ * Parameter:
+ *   @1: number of partitions
+ * Description:
+ * The number of XPRAM partitions specified for the 'devs' module parameter
+ * or with the 'xpram.parts' kernel parameter must be an integer in the
+ * range 1 to 32. The XPRAM device driver created a maximum of 32 partitions
+ * that are probably not configured as intended.
+ * User action:
+ * If the XPRAM device driver has been compiled as a separate module,
+ * unload the module and load it again with a correct value for the
+ * 'devs' module parameter. If the XPRAM device driver has been compiled
+ * into the kernel, correct the 'xpram.parts' parameter in the kernel
+ * parameter line and restart Linux.
+ */
+
+/*?
+ * Tag: xpram.2
+ * Text: "Not enough expanded memory available"
+ * Severity: Error
+ * Description:
+ * The amount of expanded memory required to set up your XPRAM partitions
+ * depends on the 'sizes' parameter specified for the xpram module or on
+ * the specifications for the 'xpram.parts' parameter if the XPRAM device
+ * driver has been compiled into the kernel. Your
+ * current specification exceed the amount of available expanded memory.
+ * Your XPRAM partitions are probably not configured as intended.
+ * User action:
+ * If the XPRAM device driver has been compiled as a separate module,
+ * unload the xpram module and load it again with an appropriate value
+ * for the 'sizes' module parameter. If the XPRAM device driver has been
+ * compiled into the kernel, adjust the 'xpram.parts' parameter in the
+ * kernel parameter line and restart Linux. If you need more than the
+ * available expanded memory, increase the expanded memory allocation for
+ * your virtual hardware or LPAR.
+ */
+
+/*?
+ * Tag: xpram.3
+ * Text: "No expanded memory available"
+ * Severity: Error
+ * Description:
+ * The XPRAM device driver has been loaded in a Linux instance that runs
+ * in an LPAR or virtual hardware without expanded memory.
+ * are created.
+ * User action:
+ * Allocate expanded memory for your LPAR or virtual hardware or do not
+ * load the xpram module. You can ignore this message, if you do not want
+ * to create XPRAM partitions.
+ */
+
Index: linux-2.6/drivers/s390/block/xpram.c
===================================================================
--- linux-2.6.orig/drivers/s390/block/xpram.c
+++ linux-2.6/drivers/s390/block/xpram.c
@@ -36,18 +36,15 @@
 #include <linux/hdreg.h>  /* HDIO_GETGEO */
 #include <linux/sysdev.h>
 #include <linux/bio.h>
+#include <linux/kmsg.h>
 #include <asm/uaccess.h>
 
+#define KMSG_COMPONENT "xpram"
+
 #define XPRAM_NAME	"xpram"
 #define XPRAM_DEVS	1	/* one partition */
 #define XPRAM_MAX_DEVS	32	/* maximal number of devices (partitions) */
 
-#define PRINT_DEBUG(x...)	printk(KERN_DEBUG XPRAM_NAME " debug:" x)
-#define PRINT_INFO(x...)	printk(KERN_INFO XPRAM_NAME " info:" x)
-#define PRINT_WARN(x...)	printk(KERN_WARNING XPRAM_NAME " warning:" x)
-#define PRINT_ERR(x...)		printk(KERN_ERR XPRAM_NAME " error:" x)
-
-
 typedef struct {
 	unsigned int	size;		/* size of xpram segment in pages */
 	unsigned int	offset;		/* start page of xpram segment */
@@ -263,7 +260,7 @@ static int __init xpram_setup_sizes(unsi
 
 	/* Check number of devices. */
 	if (devs <= 0 || devs > XPRAM_MAX_DEVS) {
-		PRINT_ERR("invalid number %d of devices\n",devs);
+		kmsg_err(1, "%d is not a valid number of XPRAM devices\n",devs);
 		return -EINVAL;
 	}
 	xpram_devs = devs;
@@ -294,22 +291,22 @@ static int __init xpram_setup_sizes(unsi
 			mem_auto_no++;
 	}
 	
-	PRINT_INFO("  number of devices (partitions): %d \n", xpram_devs);
+	kmsg_info(0, "  number of devices (partitions): %d \n", xpram_devs);
 	for (i = 0; i < xpram_devs; i++) {
 		if (xpram_sizes[i])
-			PRINT_INFO("  size of partition %d: %u kB\n",
-				   i, xpram_sizes[i]);
+			kmsg_info(0, "  size of partition %d: %u kB\n",
+				  i, xpram_sizes[i]);
 		else
-			PRINT_INFO("  size of partition %d to be set "
-				   "automatically\n",i);
+			kmsg_info(0, "  size of partition %d to be set "
+				  "automatically\n",i);
 	}
-	PRINT_DEBUG("  memory needed (for sized partitions): %lu kB\n",
-		    mem_needed);
-	PRINT_DEBUG("  partitions to be sized automatically: %d\n",
-		    mem_auto_no);
+	kmsg_info(0, "  memory needed (for sized partitions): %lu kB\n",
+		  mem_needed);
+	kmsg_info(0, "  partitions to be sized automatically: %d\n",
+		  mem_auto_no);
 
 	if (mem_needed > pages * 4) {
-		PRINT_ERR("Not enough expanded memory available\n");
+		kmsg_err(2, "Not enough expanded memory available\n");
 		return -EINVAL;
 	}
 
@@ -321,8 +318,8 @@ static int __init xpram_setup_sizes(unsi
 	 */
 	if (mem_auto_no) {
 		mem_auto = ((pages - mem_needed / 4) / mem_auto_no) * 4;
-		PRINT_INFO("  automatically determined "
-			   "partition size: %lu kB\n", mem_auto);
+		kmsg_info(0, "  automatically determined "
+			  "partition size: %lu kB\n", mem_auto);
 		for (i = 0; i < xpram_devs; i++)
 			if (xpram_sizes[i] == 0)
 				xpram_sizes[i] = mem_auto;
@@ -412,12 +409,12 @@ static int __init xpram_init(void)
 
 	/* Find out size of expanded memory. */
 	if (xpram_present() != 0) {
-		PRINT_WARN("No expanded memory available\n");
+		kmsg_err(3, "No expanded memory available\n");
 		return -ENODEV;
 	}
 	xpram_pages = xpram_highest_page_index() + 1;
-	PRINT_INFO("  %u pages expanded memory found (%lu KB).\n",
-		   xpram_pages, (unsigned long) xpram_pages*4);
+	kmsg_info(0, "  %u pages expanded memory found (%lu KB).\n",
+		  xpram_pages, (unsigned long) xpram_pages*4);
 	rc = xpram_setup_sizes(xpram_pages);
 	if (rc)
 		return rc;

-- 
blue skies,
   Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Greg KH]

On Wed, Jul 30, 2008 at 06:56:57PM +0200, Martin Schwidefsky wrote:
> From: Michael Holzheu <holzheu@de.ibm.com>
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> Introduce a new family of printk macros which prefixes each kmsg message
> with a component name and allows to tag the printk with a message id.
> 
> The kmsg component name is defined per source file with the KMSG_COMPONENT
> macro. The first argument of each kmsg printk is the message id. The
> message id "0" is special as it will suppress the message id prefix.
> 
> If the message id will be printed to the console / syslog at all depends
> on CONFIG_MSG_IDS. If it is "n" then a kmsg_xxx call is just another
> printk wrapper. These macros are intended to be used uniformly in the
> s390 architecture and the s390 device drivers.
> 
> Signed-off-by: Michael Holzheu <holzheu@de.ibm.com>
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
> 
>  arch/s390/Kconfig    |    9 +++
>  include/linux/kmsg.h |  124 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 133 insertions(+)
> 
> Index: linux-2.6/arch/s390/Kconfig
> ===================================================================
> --- linux-2.6.orig/arch/s390/Kconfig
> +++ linux-2.6/arch/s390/Kconfig
> @@ -568,6 +568,15 @@ bool "s390 guest support (EXPERIMENTAL)"
>  	select VIRTIO_CONSOLE
>  	help
>  	  Select this option if you want to run the kernel under s390 linux
> +
> +config KMSG_IDS
> +	bool "Kernel message numbers"
> +	default y
> +	help
> +	  Select this option if you want to include a message number to the
> +	  prefix for kernel messages issued by the s390 architecture and
> +	  driver code. See "Documentation/s390/kmsg.txt" for more details.
> +
>  endmenu
>  
>  source "net/Kconfig"
> Index: linux-2.6/include/linux/kmsg.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6/include/linux/kmsg.h
> @@ -0,0 +1,124 @@
> +#ifndef _LINUX_KMSG_H
> +#define _LINUX_KMSG_H
> +
> +#ifndef __KMSG_CHECKER
> +#define __KMSG_CHECK(level, id) KERN_##level
> +#endif

What if __KMSG_CHECKER is enabled?  What does __KMSG_CHECK resolve to
then?

And what sets __KMSG_CHECKER?

thanks,

greg k-h

Re: [patch 3/3] kmsg: convert xpram messages to kmsg api.

[Greg KH]

On Wed, Jul 30, 2008 at 06:56:59PM +0200, Martin Schwidefsky wrote:
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
> 
>  Documentation/kmsg/s390/xpram |   54 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/s390/block/xpram.c    |   41 ++++++++++++++-----------------
>  2 files changed, 73 insertions(+), 22 deletions(-)
> 
> Index: linux-2.6/Documentation/kmsg/s390/xpram
> ===================================================================
> --- /dev/null
> +++ linux-2.6/Documentation/kmsg/s390/xpram
> @@ -0,0 +1,54 @@
> +/*?
> + * Tag: xpram.1

Ick, so you are going to have to define a message number per file?
How is that going to work, it looks like you use ids 0-2 below in the .c
file, yet in this documentation file they are 1-3.  Off by one
somewhere?  :)

> + * Text: "%d is not a valid number of XPRAM devices"
> + * Severity: Error
> + * Parameter:
> + *   @1: number of partitions
> + * Description:
> + * The number of XPRAM partitions specified for the 'devs' module parameter
> + * or with the 'xpram.parts' kernel parameter must be an integer in the
> + * range 1 to 32. The XPRAM device driver created a maximum of 32 partitions
> + * that are probably not configured as intended.
> + * User action:
> + * If the XPRAM device driver has been compiled as a separate module,
> + * unload the module and load it again with a correct value for the
> + * 'devs' module parameter. If the XPRAM device driver has been compiled
> + * into the kernel, correct the 'xpram.parts' parameter in the kernel
> + * parameter line and restart Linux.
> + */

Any way to put this stuff in the .c file itself?  It's hard enough
getting people to update Documentation/ABI/, knowing to go modify
something else in the Documentation directory is going to be _very_
difficult.


>  
> +#define KMSG_COMPONENT "xpram"

Can't you just use KBUILD_MODULE_NAME instead?  That makes it one less
thing you have to define in the code (and forget about when moving files
around or cut-and-pasting).

>  	/* Check number of devices. */
>  	if (devs <= 0 || devs > XPRAM_MAX_DEVS) {
> -		PRINT_ERR("invalid number %d of devices\n",devs);
> +		kmsg_err(1, "%d is not a valid number of XPRAM devices\n",devs);
>  		return -EINVAL;

Magic number "1" here?  See comment above about this.

thanks,

greg k-h

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Kay Sievers]

On Wed, Jul 30, 2008 at 18:56, Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:

> +#define kmsg_dev_alert(id, dev, format, arg...) \
> +       printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
> +               ": %s: " format, (dev)->bus_id , ## arg)

Care to use dev_name(), bus_id will be removed soon.

Thanks,
Kay

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Greg KH]

On Thu, Jul 31, 2008 at 12:02:04AM +0200, Kay Sievers wrote:
> On Wed, Jul 30, 2008 at 18:56, Martin Schwidefsky
> <schwidefsky@de.ibm.com> wrote:
> 
> > +#define kmsg_dev_alert(id, dev, format, arg...) \
> > +       printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
> > +               ": %s: " format, (dev)->bus_id , ## arg)
> 
> Care to use dev_name(), bus_id will be removed soon.

Hm, just use dev_printk() instead, it handles all of these kinds of
changes automatically.

thanks,

greg k-h

Re: [patch 2/3] kmsg: Kernel message catalog script.

[KOSAKI Motohiro]

> From: Michael Holzheu <holzheu@de.ibm.com>
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> Add a script and the calls to the make process that allows to check the
> kmsg printk messages and to format man pages from the message descriptions.
> 
> The kmsg message description is a comment with the following format:
> 
> /*?
>  * Tag: <component>.<id>
>  * Text: "<kmsg message text>"
>  * Severity: <severity>
>  * Parameter:
>  *   @1: <description of the first message parameter>
>  *   @2: <description of the second message parameter>
>  *   ...
>  * Description:
>  * <What is the kmsg message all about>
>  * User action:
>  * <What can the user do to fix the problem>
>  */
> 
> The script looks for a kmsg comment for a kmsg printk at three places,
> the source file where the kmsg call is located, in the architecture
> dependent file Documentation/kmsg/$ARCH/<component> and in the common
> file Documentation/kmsg/<component>.
> 
> The kmsg check is invoked with "make D=1" and reads the source files for
> all objects that are built by the current configuration and searches for
> matching kmsg descriptions for the kmsg messages in the source which
> have a messages id > 0. If a message description can not be found the
> script prints a blueprint and causes a make error.
> 
> The kmsg man page creation is invoked with "make D=2" and reads the source
> files for all built objects, looks up the message description and writes
> a man page to $(objtree)/man.

I don't know message id numbering is good or bad.
but I like a check script/Makefile of printk format changing.
because many people(include me :) often forget printk/Documentation change.

Thanks

Re: [patch 3/3] kmsg: convert xpram messages to kmsg api.

[Martin Schwidefsky]

On Wed, 2008-07-30 at 12:43 -0700, Greg KH wrote:
> On Wed, Jul 30, 2008 at 06:56:59PM +0200, Martin Schwidefsky wrote:
> > Index: linux-2.6/Documentation/kmsg/s390/xpram
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6/Documentation/kmsg/s390/xpram
> > @@ -0,0 +1,54 @@
> > +/*?
> > + * Tag: xpram.1
> 
> Ick, so you are going to have to define a message number per file?
> How is that going to work, it looks like you use ids 0-2 below in the .c
> file, yet in this documentation file they are 1-3.  Off by one
> somewhere?  :)

The kmsg number 0 is special, the message tag will not include the
message number for id 0. And the script won't complain that the message
description is missing.
And yes, we WANT to define the message numbers per kmsg component. 

> > + * Text: "%d is not a valid number of XPRAM devices"
> > + * Severity: Error
> > + * Parameter:
> > + *   @1: number of partitions
> > + * Description:
> > + * The number of XPRAM partitions specified for the 'devs' module parameter
> > + * or with the 'xpram.parts' kernel parameter must be an integer in the
> > + * range 1 to 32. The XPRAM device driver created a maximum of 32 partitions
> > + * that are probably not configured as intended.
> > + * User action:
> > + * If the XPRAM device driver has been compiled as a separate module,
> > + * unload the module and load it again with a correct value for the
> > + * 'devs' module parameter. If the XPRAM device driver has been compiled
> > + * into the kernel, correct the 'xpram.parts' parameter in the kernel
> > + * parameter line and restart Linux.
> > + */
> 
> Any way to put this stuff in the .c file itself?  It's hard enough
> getting people to update Documentation/ABI/, knowing to go modify
> something else in the Documentation directory is going to be _very_
> difficult.

Yes, already implemented. Just put the comment anywhere in the .c file
where the message is printed. The Documentation/kmsg directory is just
an additional option.

> 
> >  
> > +#define KMSG_COMPONENT "xpram"
> 
> Can't you just use KBUILD_MODULE_NAME instead?  That makes it one less
> thing you have to define in the code (and forget about when moving files
> around or cut-and-pasting).

Two reason why we don't want to use KBUILD_MODULE_NAME:
1) the message tag (message component + message id) should never change,
if you change the code structure the module name might change as well.
2) we want to be able to use the same kmsg component in multiple .c
files. 

> >  	/* Check number of devices. */
> >  	if (devs <= 0 || devs > XPRAM_MAX_DEVS) {
> > -		PRINT_ERR("invalid number %d of devices\n",devs);
> > +		kmsg_err(1, "%d is not a valid number of XPRAM devices\n",devs);
> >  		return -EINVAL;
> 
> Magic number "1" here?  See comment above about this.

This is just the kmsg printk for xpram.1, see comment above about id 0.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Wed, 2008-07-30 at 12:39 -0700, Greg KH wrote:
> > Index: linux-2.6/include/linux/kmsg.h
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6/include/linux/kmsg.h
> > @@ -0,0 +1,124 @@
> > +#ifndef _LINUX_KMSG_H
> > +#define _LINUX_KMSG_H
> > +
> > +#ifndef __KMSG_CHECKER
> > +#define __KMSG_CHECK(level, id) KERN_##level
> > +#endif
> 
> What if __KMSG_CHECKER is enabled?  What does __KMSG_CHECK resolve to
> then?
> 
> And what sets __KMSG_CHECKER?

The kmsg-doc script sets __KMSG_CHECKER and nothing else should ever
define it. It is used to preprocess the .c files to get at the kernel
messages. See process_cpp_file in the kmsg-doc script.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Thu, 2008-07-31 at 00:02 +0200, Kay Sievers wrote:
> On Wed, Jul 30, 2008 at 18:56, Martin Schwidefsky
> <schwidefsky@de.ibm.com> wrote:
> 
> > +#define kmsg_dev_alert(id, dev, format, arg...) \
> > +       printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
> > +               ": %s: " format, (dev)->bus_id , ## arg)
> 
> Care to use dev_name(), bus_id will be removed soon.

Yes, will do. Thanks.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Wed, 2008-07-30 at 15:04 -0700, Greg KH wrote:
> On Thu, Jul 31, 2008 at 12:02:04AM +0200, Kay Sievers wrote:
> > On Wed, Jul 30, 2008 at 18:56, Martin Schwidefsky
> > <schwidefsky@de.ibm.com> wrote:
> > 
> > > +#define kmsg_dev_alert(id, dev, format, arg...) \
> > > +       printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
> > > +               ": %s: " format, (dev)->bus_id , ## arg)
> > 
> > Care to use dev_name(), bus_id will be removed soon.
> 
> Hm, just use dev_printk() instead, it handles all of these kinds of
> changes automatically.

Using dev_printk won't work because of the order of the elements of the
printk. The kmsg tag should not have a "random" position in the printk
but should be the first element. If we use dev_printk the kmsg tag will
be the third element, for other kmsg printks it will be the first. In
addition the kmsg message tag for the device drivers already includes
the driver name ..

-- 
blue skies,
  Martin.

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

RE: [patch 2/3] kmsg: Kernel message catalog script.

[Takashi Nishiie]

> 
> /*?
>  * Tag: <component>.<id>
>  * Text: "<kmsg message text>"
>  * Severity: <severity>
>  * Parameter:
>  *   @1: <description of the first message parameter>
>  *   @2: <description of the second message parameter>
>  *   ...
>  * Description:
>  * <What is the kmsg message all about>
>  * User action:
>  * <What can the user do to fix the problem>  */
> 
> The script looks for a kmsg comment for a kmsg printk at three places, 
> the source file where the kmsg call is located, in the architecture 
> dependent file Documentation/kmsg/$ARCH/<component> and in the common 
> file Documentation/kmsg/<component>.
> 
> The kmsg check is invoked with "make D=1" and reads the source files 
> for all objects that are built by the current configuration and 
> searches for matching kmsg descriptions for the kmsg messages in the 
> source which have a messages id > 0. If a message description can not 
> be found the script prints a blueprint and causes a make error.
> 
> The kmsg man page creation is invoked with "make D=2" and reads the 
> source files for all built objects, looks up the message description 
> and writes a man page to $(objtree)/man.

  I think it is good to number the message. It might be gospel for 
providing with the support service for the corporation.
  However, there is a problem of the scale in numbering of not only a
specific module but also all modules to the message. (There might be 
at least 34000 relating messages or more. )

  It introduces the following Web sites as a approach that arranges 
the message on a Japanese Web site though it apologizes. 

OSS Message Pedia
http://ossmpedia.org/
(Attention: This Web site is being written in Japanese. )

Thanks

--
NISHIIE Takashi

RE: [patch 2/3] kmsg: Kernel message catalog script.

[Martin Schwidefsky]

On Thu, 2008-07-31 at 19:23 +0900, Takashi Nishiie wrote:
>   However, there is a problem of the scale in numbering of not only a
> specific module but also all modules to the message. (There might be 
> at least 34000 relating messages or more. )

Could you explain this a bit more? I'm not sure I understood it
correctly. I'm unsure about the term "relating message". A single ksmg
message can be printed from several modules if it is the same message.
Do you want to create a list of modules that issue a particular message?

-- 
blue skies,
  Martin.

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

Re: [patch 3/3] kmsg: convert xpram messages to kmsg api.

[Pavel Machek]

Hi!
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> 
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
> 
>  Documentation/kmsg/s390/xpram |   54 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/s390/block/xpram.c    |   41 ++++++++++++++-----------------
>  2 files changed, 73 insertions(+), 22 deletions(-)
> 
> Index: linux-2.6/Documentation/kmsg/s390/xpram
> ===================================================================
> --- /dev/null
> +++ linux-2.6/Documentation/kmsg/s390/xpram
> @@ -0,0 +1,54 @@
> +/*?
> + * Tag: xpram.1
> + * Text: "%d is not a valid number of XPRAM devices"
> + * Severity: Error
> + * Parameter:
> + *   @1: number of partitions
> + * Description:
> + * The number of XPRAM partitions specified for the 'devs' module parameter
> + * or with the 'xpram.parts' kernel parameter must be an integer in the
> + * range 1 to 32. The XPRAM device driver created a maximum of 32 partitions
> + * that are probably not configured as intended.
> + * User action:
> + * If the XPRAM device driver has been compiled as a separate module,
> + * unload the module and load it again with a correct value for the
> + * 'devs' module parameter. If the XPRAM device driver has been compiled
> + * into the kernel, correct the 'xpram.parts' parameter in the kernel
> + * parameter line and restart Linux.

It is "Kernel command line". Plus, I do not think I find "User action"
section really helpful here.

...user that does not understand what "between 1 and 32 means" is
unlikely to know if the driver is built-in or module...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 3/3] kmsg: convert xpram messages to kmsg api.

[Martin Schwidefsky]

On Mon, 2008-08-04 at 08:48 +0200, Pavel Machek wrote:
> It is "Kernel command line". Plus, I do not think I find "User action"
> section really helpful here.

Ok, I'll fix that.

> ...user that does not understand what "between 1 and 32 means" is
> unlikely to know if the driver is built-in or module...

Yeah, it is a trivial example for a user action. Perhaps even too
trivial. On the other hand it doesn't hurt either. At least it is good
english thanks to our ID department.

-- 
blue skies,
  Martin.

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

Re: [patch 0/3] [RFC] kmsg macros and script, take x+1.

[Martin Schwidefsky]

On Mon, 2008-08-04 at 22:26 +0200, Sam Ravnborg wrote:
> > The lastest changes:
> >  * kmsg.h has been moved from include/asm-s390 to include/linux
> >  * The s390 specific documentation is now in Documentation/kmsg/s390
> >  * The script checks for messages in the source file, Documentation/kmsg
> >    and Documentation/kmsg/$SRCARCH
> 
> The tendency is that all ARCH specific files should be part of
> arch/$ARCH/*
> What is the correct approach here I dunno.

Hmm, arch/$ARCH/kmsg. But then Documentation/kmsg doesn't really fit,
does it? Don't no either.. 

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Greg KH]

On Thu, Jul 31, 2008 at 11:10:04AM +0200, Martin Schwidefsky wrote:
> On Wed, 2008-07-30 at 15:04 -0700, Greg KH wrote:
> > On Thu, Jul 31, 2008 at 12:02:04AM +0200, Kay Sievers wrote:
> > > On Wed, Jul 30, 2008 at 18:56, Martin Schwidefsky
> > > <schwidefsky@de.ibm.com> wrote:
> > > 
> > > > +#define kmsg_dev_alert(id, dev, format, arg...) \
> > > > +       printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
> > > > +               ": %s: " format, (dev)->bus_id , ## arg)
> > > 
> > > Care to use dev_name(), bus_id will be removed soon.
> > 
> > Hm, just use dev_printk() instead, it handles all of these kinds of
> > changes automatically.
> 
> Using dev_printk won't work because of the order of the elements of the
> printk. The kmsg tag should not have a "random" position in the printk
> but should be the first element. If we use dev_printk the kmsg tag will
> be the third element, for other kmsg printks it will be the first. In
> addition the kmsg message tag for the device drivers already includes
> the driver name ..

But the structure of dev_printk() is well definied and should be pretty
trival to parse even with missing fields.

And if not, fix up dev_printk() to properly delinate when we have
missing fields, why create a totally new thing here that we need to
convert subsystems into?

thanks,

greg k-h

Re: [patch 3/3] kmsg: convert xpram messages to kmsg api.

[Greg KH]

On Thu, Jul 31, 2008 at 10:33:42AM +0200, Martin Schwidefsky wrote:
> On Wed, 2008-07-30 at 12:43 -0700, Greg KH wrote:
> > On Wed, Jul 30, 2008 at 06:56:59PM +0200, Martin Schwidefsky wrote:
> > > Index: linux-2.6/Documentation/kmsg/s390/xpram
> > > ===================================================================
> > > --- /dev/null
> > > +++ linux-2.6/Documentation/kmsg/s390/xpram
> > > @@ -0,0 +1,54 @@
> > > +/*?
> > > + * Tag: xpram.1
> > 
> > Ick, so you are going to have to define a message number per file?
> > How is that going to work, it looks like you use ids 0-2 below in the .c
> > file, yet in this documentation file they are 1-3.  Off by one
> > somewhere?  :)
> 
> The kmsg number 0 is special, the message tag will not include the
> message number for id 0. And the script won't complain that the message
> description is missing.

Was "0 is special" defined anywhere that I missed?

> And yes, we WANT to define the message numbers per kmsg component. 

But again, you have to manually match up numbers by hand, that's prone
to get messy very quickly.

> > > + * Text: "%d is not a valid number of XPRAM devices"
> > > + * Severity: Error
> > > + * Parameter:
> > > + *   @1: number of partitions
> > > + * Description:
> > > + * The number of XPRAM partitions specified for the 'devs' module parameter
> > > + * or with the 'xpram.parts' kernel parameter must be an integer in the
> > > + * range 1 to 32. The XPRAM device driver created a maximum of 32 partitions
> > > + * that are probably not configured as intended.
> > > + * User action:
> > > + * If the XPRAM device driver has been compiled as a separate module,
> > > + * unload the module and load it again with a correct value for the
> > > + * 'devs' module parameter. If the XPRAM device driver has been compiled
> > > + * into the kernel, correct the 'xpram.parts' parameter in the kernel
> > > + * parameter line and restart Linux.
> > > + */
> > 
> > Any way to put this stuff in the .c file itself?  It's hard enough
> > getting people to update Documentation/ABI/, knowing to go modify
> > something else in the Documentation directory is going to be _very_
> > difficult.
> 
> Yes, already implemented. Just put the comment anywhere in the .c file
> where the message is printed. The Documentation/kmsg directory is just
> an additional option.

Why not have your example show this?

> > > +#define KMSG_COMPONENT "xpram"
> > 
> > Can't you just use KBUILD_MODULE_NAME instead?  That makes it one less
> > thing you have to define in the code (and forget about when moving files
> > around or cut-and-pasting).
> 
> Two reason why we don't want to use KBUILD_MODULE_NAME:
> 1) the message tag (message component + message id) should never change,
> if you change the code structure the module name might change as well.

Um, isn't that the point?  If the code structure changes, then perhaps
the message also should change?  If not, it's trival to adjust.

> 2) we want to be able to use the same kmsg component in multiple .c
> files. 

Why would this matter?  It's just a "tag", who cares about the actual
name?

thanks,

greg k-h

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Tue, 2008-08-05 at 15:31 -0700, Greg KH wrote:
> On Thu, Jul 31, 2008 at 11:10:04AM +0200, Martin Schwidefsky wrote:
> > On Wed, 2008-07-30 at 15:04 -0700, Greg KH wrote:
> > > On Thu, Jul 31, 2008 at 12:02:04AM +0200, Kay Sievers wrote:
> > > > On Wed, Jul 30, 2008 at 18:56, Martin Schwidefsky
> > > > <schwidefsky@de.ibm.com> wrote:
> > > > 
> > > > > +#define kmsg_dev_alert(id, dev, format, arg...) \
> > > > > +       printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
> > > > > +               ": %s: " format, (dev)->bus_id , ## arg)
> > > > 
> > > > Care to use dev_name(), bus_id will be removed soon.
> > > 
> > > Hm, just use dev_printk() instead, it handles all of these kinds of
> > > changes automatically.
> > 
> > Using dev_printk won't work because of the order of the elements of the
> > printk. The kmsg tag should not have a "random" position in the printk
> > but should be the first element. If we use dev_printk the kmsg tag will
> > be the third element, for other kmsg printks it will be the first. In
> > addition the kmsg message tag for the device drivers already includes
> > the driver name ..
> 
> But the structure of dev_printk() is well definied and should be pretty
> trival to parse even with missing fields.

The missing field is the message tag. Which is the key for the message
lookup. As far as the kmsg catalog is concerned this is a pretty
important field that may not be missing from the printk itself.

> And if not, fix up dev_printk() to properly delinate when we have
> missing fields, why create a totally new thing here that we need to
> convert subsystems into?

To "fix" dev_printk would mean to add the message tag. Which requires to
change all callers. The better approach is to add a new printk wrapper
and decide for each subsystem if it shouuld be converted. I'm not
advocating to change everything. The kmsg macros is a way to document
the message from a piece of code if you choose to use these macros. 

What could be done is to move the kmsg_dev_xxx macros to
include/linux/device.h if you want to keep all device printk related
definitions in one place. Would that be more acceptable ?

-- 
blue skies,
  Martin.

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

Re: [patch 3/3] kmsg: convert xpram messages to kmsg api.

[Martin Schwidefsky]

On Tue, 2008-08-05 at 15:34 -0700, Greg KH wrote:
> On Thu, Jul 31, 2008 at 10:33:42AM +0200, Martin Schwidefsky wrote:
> > On Wed, 2008-07-30 at 12:43 -0700, Greg KH wrote:
> > > On Wed, Jul 30, 2008 at 06:56:59PM +0200, Martin Schwidefsky wrote:
> > > > Index: linux-2.6/Documentation/kmsg/s390/xpram
> > > > ===================================================================
> > > > --- /dev/null
> > > > +++ linux-2.6/Documentation/kmsg/s390/xpram
> > > > @@ -0,0 +1,54 @@
> > > > +/*?
> > > > + * Tag: xpram.1
> > > 
> > > Ick, so you are going to have to define a message number per file?
> > > How is that going to work, it looks like you use ids 0-2 below in the .c
> > > file, yet in this documentation file they are 1-3.  Off by one
> > > somewhere?  :)
> > 
> > The kmsg number 0 is special, the message tag will not include the
> > message number for id 0. And the script won't complain that the message
> > description is missing.
> 
> Was "0 is special" defined anywhere that I missed?

>From the patch description of the kernel message catalog script:

The kmsg check is invoked with "make D=1" and reads the source files for
all objects that are built by the current configuration and searches for
matching kmsg descriptions for the kmsg messages in the source which
have a messages id > 0. If a message description can not be found the
script prints a blueprint and causes a make error.

> > And yes, we WANT to define the message numbers per kmsg component. 
> 
> But again, you have to manually match up numbers by hand, that's prone
> to get messy very quickly.

The kmsg catalog script if called with "make D=1" will check all the
messages in the current build. When a message is introduced you have to
pick a free number. The checking is done automatically and errors will
be caught without trouble.

> > > > + * Text: "%d is not a valid number of XPRAM devices"
> > > > + * Severity: Error
> > > > + * Parameter:
> > > > + *   @1: number of partitions
> > > > + * Description:
> > > > + * The number of XPRAM partitions specified for the 'devs' module parameter
> > > > + * or with the 'xpram.parts' kernel parameter must be an integer in the
> > > > + * range 1 to 32. The XPRAM device driver created a maximum of 32 partitions
> > > > + * that are probably not configured as intended.
> > > > + * User action:
> > > > + * If the XPRAM device driver has been compiled as a separate module,
> > > > + * unload the module and load it again with a correct value for the
> > > > + * 'devs' module parameter. If the XPRAM device driver has been compiled
> > > > + * into the kernel, correct the 'xpram.parts' parameter in the kernel
> > > > + * parameter line and restart Linux.
> > > > + */
> > > 
> > > Any way to put this stuff in the .c file itself?  It's hard enough
> > > getting people to update Documentation/ABI/, knowing to go modify
> > > something else in the Documentation directory is going to be _very_
> > > difficult.
> > 
> > Yes, already implemented. Just put the comment anywhere in the .c file
> > where the message is printed. The Documentation/kmsg directory is just
> > an additional option.
> 
> Why not have your example show this?

Ok, fair enough.

> > > > +#define KMSG_COMPONENT "xpram"
> > > 
> > > Can't you just use KBUILD_MODULE_NAME instead?  That makes it one less
> > > thing you have to define in the code (and forget about when moving files
> > > around or cut-and-pasting).
> > 
> > Two reason why we don't want to use KBUILD_MODULE_NAME:
> > 1) the message tag (message component + message id) should never change,
> > if you change the code structure the module name might change as well.
> 
> Um, isn't that the point?  If the code structure changes, then perhaps
> the message also should change?  If not, it's trival to adjust.

NO! The message nor the message tag should change if the message
semantically still reports the same thing. If the meaning of the message
changes then change the message AND the message tag.

> > 2) we want to be able to use the same kmsg component in multiple .c
> > files. 
> 
> Why would this matter?  It's just a "tag", who cares about the actual
> name?

The actual name is not really important, but if the name is choosen
wisely it does convey information. Guess what "dasd.17" tells you
something about the dasd driver, "zfcp.42" about the zfcp driver and so
on. The code structure should not dictate how the message tag is
created.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Greg KH]

On Wed, Aug 06, 2008 at 10:35:16AM +0200, Martin Schwidefsky wrote:
> On Tue, 2008-08-05 at 15:31 -0700, Greg KH wrote:
> > On Thu, Jul 31, 2008 at 11:10:04AM +0200, Martin Schwidefsky wrote:
> > > On Wed, 2008-07-30 at 15:04 -0700, Greg KH wrote:
> > > > On Thu, Jul 31, 2008 at 12:02:04AM +0200, Kay Sievers wrote:
> > > > > On Wed, Jul 30, 2008 at 18:56, Martin Schwidefsky
> > > > > <schwidefsky@de.ibm.com> wrote:
> > > > > 
> > > > > > +#define kmsg_dev_alert(id, dev, format, arg...) \
> > > > > > +       printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
> > > > > > +               ": %s: " format, (dev)->bus_id , ## arg)
> > > > > 
> > > > > Care to use dev_name(), bus_id will be removed soon.
> > > > 
> > > > Hm, just use dev_printk() instead, it handles all of these kinds of
> > > > changes automatically.
> > > 
> > > Using dev_printk won't work because of the order of the elements of the
> > > printk. The kmsg tag should not have a "random" position in the printk
> > > but should be the first element. If we use dev_printk the kmsg tag will
> > > be the third element, for other kmsg printks it will be the first. In
> > > addition the kmsg message tag for the device drivers already includes
> > > the driver name ..
> > 
> > But the structure of dev_printk() is well definied and should be pretty
> > trival to parse even with missing fields.
> 
> The missing field is the message tag. Which is the key for the message
> lookup. As far as the kmsg catalog is concerned this is a pretty
> important field that may not be missing from the printk itself.

No, I mean use dev_printk() as the base for your logging macro.  Add
your message tag as the first field after the dev_printk() information.

thanks,

greg k-h

Re: [patch 3/3] kmsg: convert xpram messages to kmsg api.

[Greg KH]

On Wed, Aug 06, 2008 at 10:46:13AM +0200, Martin Schwidefsky wrote:
> On Tue, 2008-08-05 at 15:34 -0700, Greg KH wrote:
> > On Thu, Jul 31, 2008 at 10:33:42AM +0200, Martin Schwidefsky wrote:
> > > On Wed, 2008-07-30 at 12:43 -0700, Greg KH wrote:
> > > > On Wed, Jul 30, 2008 at 06:56:59PM +0200, Martin Schwidefsky wrote:
> > > > > Index: linux-2.6/Documentation/kmsg/s390/xpram
> > > > > ===================================================================
> > > > > --- /dev/null
> > > > > +++ linux-2.6/Documentation/kmsg/s390/xpram
> > > > > @@ -0,0 +1,54 @@
> > > > > +/*?
> > > > > + * Tag: xpram.1
> > > > 
> > > > Ick, so you are going to have to define a message number per file?
> > > > How is that going to work, it looks like you use ids 0-2 below in the .c
> > > > file, yet in this documentation file they are 1-3.  Off by one
> > > > somewhere?  :)
> > > 
> > > The kmsg number 0 is special, the message tag will not include the
> > > message number for id 0. And the script won't complain that the message
> > > description is missing.
> > 
> > Was "0 is special" defined anywhere that I missed?
> 
> >From the patch description of the kernel message catalog script:
> 
> The kmsg check is invoked with "make D=1" and reads the source files for
> all objects that are built by the current configuration and searches for
> matching kmsg descriptions for the kmsg messages in the source which
> have a messages id > 0. If a message description can not be found the
> script prints a blueprint and causes a make error.

Ok, but you don't describe messages with "0" in them :)

> > > > > +#define KMSG_COMPONENT "xpram"
> > > > 
> > > > Can't you just use KBUILD_MODULE_NAME instead?  That makes it one less
> > > > thing you have to define in the code (and forget about when moving files
> > > > around or cut-and-pasting).
> > > 
> > > Two reason why we don't want to use KBUILD_MODULE_NAME:
> > > 1) the message tag (message component + message id) should never change,
> > > if you change the code structure the module name might change as well.
> > 
> > Um, isn't that the point?  If the code structure changes, then perhaps
> > the message also should change?  If not, it's trival to adjust.
> 
> NO! The message nor the message tag should change if the message
> semantically still reports the same thing. If the meaning of the message
> changes then change the message AND the message tag.

Why can't the message reporting change?  What's the reluctance for
change here for something that did happen to move to a different file?
It shouldn't matter _at all_ as you are only looking at the
tags/messages for a specific kernel version to ensure they match up.
Any future kernel version might have different ones.

It's not like once you write a message/tag it will stay that way fixed
for all time, that's just not going to fly with the way the Linux kernel
is developed.

> > > 2) we want to be able to use the same kmsg component in multiple .c
> > > files. 
> > 
> > Why would this matter?  It's just a "tag", who cares about the actual
> > name?
> 
> The actual name is not really important, but if the name is choosen
> wisely it does convey information. Guess what "dasd.17" tells you
> something about the dasd driver, "zfcp.42" about the zfcp driver and so
> on. The code structure should not dictate how the message tag is
> created.

The message tag should not dictate anything except how to look it up
somehow.  So it doesn't matter if the name changes, as long as the
ability to get the real information is still there.

So the kernel could change the tags every other release and there would
be no problem.

thanks,

greg k-h

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Wed, 2008-08-06 at 13:07 -0700, Greg KH wrote:
> > > > Using dev_printk won't work because of the order of the elements of the
> > > > printk. The kmsg tag should not have a "random" position in the printk
> > > > but should be the first element. If we use dev_printk the kmsg tag will
> > > > be the third element, for other kmsg printks it will be the first. In
> > > > addition the kmsg message tag for the device drivers already includes
> > > > the driver name ..
> > > 
> > > But the structure of dev_printk() is well definied and should be pretty
> > > trival to parse even with missing fields.
> > 
> > The missing field is the message tag. Which is the key for the message
> > lookup. As far as the kmsg catalog is concerned this is a pretty
> > important field that may not be missing from the printk itself.
> 
> No, I mean use dev_printk() as the base for your logging macro.  Add
> your message tag as the first field after the dev_printk() information.

Hmm, you are proposing to introduce a second format for the kmsg
messages to avoid the need for some more printk wrapper macros. To me it
seems that this has two problems:
1) The message tag is for the user of the system. If it does not have a
fixed position it gets confusing.
2) The message tag for a driver message usually already includes the
driver name, the dev_printk will print it again. This is ugly and
reduces the quality of the message.

-- 
blue skies,
  Martin.

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

Re: [patch 3/3] kmsg: convert xpram messages to kmsg api.

[Martin Schwidefsky]

On Wed, 2008-08-06 at 13:11 -0700, Greg KH wrote:
> > >From the patch description of the kernel message catalog script:
> > 
> > The kmsg check is invoked with "make D=1" and reads the source files for
> > all objects that are built by the current configuration and searches for
> > matching kmsg descriptions for the kmsg messages in the source which
> > have a messages id > 0. If a message description can not be found the
> > script prints a blueprint and causes a make error.
> 
> Ok, but you don't describe messages with "0" in them :)

I'll add the creation of a howto to my to-do list that describes how to
work with kmsg.

> > > > > > +#define KMSG_COMPONENT "xpram"
> > > > > 
> > > > > Can't you just use KBUILD_MODULE_NAME instead?  That makes it one less
> > > > > thing you have to define in the code (and forget about when moving files
> > > > > around or cut-and-pasting).
> > > > 
> > > > Two reason why we don't want to use KBUILD_MODULE_NAME:
> > > > 1) the message tag (message component + message id) should never change,
> > > > if you change the code structure the module name might change as well.
> > > 
> > > Um, isn't that the point?  If the code structure changes, then perhaps
> > > the message also should change?  If not, it's trival to adjust.
> > 
> > NO! The message nor the message tag should change if the message
> > semantically still reports the same thing. If the meaning of the message
> > changes then change the message AND the message tag.
> 
> Why can't the message reporting change?  What's the reluctance for
> change here for something that did happen to move to a different file?
> It shouldn't matter _at all_ as you are only looking at the
> tags/messages for a specific kernel version to ensure they match up.
> Any future kernel version might have different ones.
>
> It's not like once you write a message/tag it will stay that way fixed
> for all time, that's just not going to fly with the way the Linux kernel
> is developed.

The message tag should uniquely identify the message so that the
translation projects have something to work with. If we keep changing
the message tag with each kernel release that will create a huge effort
to keep track of the messages.

> > > > 2) we want to be able to use the same kmsg component in multiple .c
> > > > files. 
> > > 
> > > Why would this matter?  It's just a "tag", who cares about the actual
> > > name?
> > 
> > The actual name is not really important, but if the name is choosen
> > wisely it does convey information. Guess what "dasd.17" tells you
> > something about the dasd driver, "zfcp.42" about the zfcp driver and so
> > on. The code structure should not dictate how the message tag is
> > created.
> 
> The message tag should not dictate anything except how to look it up
> somehow.  So it doesn't matter if the name changes, as long as the
> ability to get the real information is still there.
> 
> So the kernel could change the tags every other release and there would
> be no problem.

I think the unique message ids are very important. If we change them all
the time this would severly limit the value of the kernel message
catalog.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Joe Perches]

On Thu, 2008-08-07 at 10:31 +0200, Martin Schwidefsky wrote:
> On Wed, 2008-08-06 at 13:07 -0700, Greg KH wrote:
> > No, I mean use dev_printk() as the base for your logging macro.  Add
> > your message tag as the first field after the dev_printk() information.
> Hmm, you are proposing to introduce a second format for the kmsg
> messages to avoid the need for some more printk wrapper macros. To me it
> seems that this has two problems:
> 1) The message tag is for the user of the system. If it does not have a
> fixed position it gets confusing.

I think the message tag today is not useful for a user.

It seems a barely useful maintainer value.

Perhaps the message tag allows for run-time language
translation and maybe a bit better selective logging.

> 2) The message tag for a driver message usually already includes the
> driver name, the dev_printk will print it again. This is ugly and
> reduces the quality of the message.

Ugly is a beholder problem.

Either printk or dev_printk could reorder the output.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Greg KH]

On Thu, Aug 07, 2008 at 10:31:41AM +0200, Martin Schwidefsky wrote:
> On Wed, 2008-08-06 at 13:07 -0700, Greg KH wrote:
> > > > > Using dev_printk won't work because of the order of the elements of the
> > > > > printk. The kmsg tag should not have a "random" position in the printk
> > > > > but should be the first element. If we use dev_printk the kmsg tag will
> > > > > be the third element, for other kmsg printks it will be the first. In
> > > > > addition the kmsg message tag for the device drivers already includes
> > > > > the driver name ..
> > > > 
> > > > But the structure of dev_printk() is well definied and should be pretty
> > > > trival to parse even with missing fields.
> > > 
> > > The missing field is the message tag. Which is the key for the message
> > > lookup. As far as the kmsg catalog is concerned this is a pretty
> > > important field that may not be missing from the printk itself.
> > 
> > No, I mean use dev_printk() as the base for your logging macro.  Add
> > your message tag as the first field after the dev_printk() information.
> 
> Hmm, you are proposing to introduce a second format for the kmsg
> messages to avoid the need for some more printk wrapper macros. To me it
> seems that this has two problems:

No, only 1 format, use dev_printk() instead of printk() in your macro.

> 1) The message tag is for the user of the system. If it does not have a
> fixed position it gets confusing.

How would it not be in a fixed position with dev_printk()?

> 2) The message tag for a driver message usually already includes the
> driver name, the dev_printk will print it again. This is ugly and
> reduces the quality of the message.

Then the message needs to change and remove that "driver name", as it is
redundant, saving a tiny ammount of space :)

thanks,

greg k-h

Re: [patch 3/3] kmsg: convert xpram messages to kmsg api.

[Greg KH]

On Thu, Aug 07, 2008 at 10:39:13AM +0200, Martin Schwidefsky wrote:
> On Wed, 2008-08-06 at 13:11 -0700, Greg KH wrote:
> > > > > > > +#define KMSG_COMPONENT "xpram"
> > > > > > 
> > > > > > Can't you just use KBUILD_MODULE_NAME instead?  That makes it one less
> > > > > > thing you have to define in the code (and forget about when moving files
> > > > > > around or cut-and-pasting).
> > > > > 
> > > > > Two reason why we don't want to use KBUILD_MODULE_NAME:
> > > > > 1) the message tag (message component + message id) should never change,
> > > > > if you change the code structure the module name might change as well.
> > > > 
> > > > Um, isn't that the point?  If the code structure changes, then perhaps
> > > > the message also should change?  If not, it's trival to adjust.
> > > 
> > > NO! The message nor the message tag should change if the message
> > > semantically still reports the same thing. If the meaning of the message
> > > changes then change the message AND the message tag.
> > 
> > Why can't the message reporting change?  What's the reluctance for
> > change here for something that did happen to move to a different file?
> > It shouldn't matter _at all_ as you are only looking at the
> > tags/messages for a specific kernel version to ensure they match up.
> > Any future kernel version might have different ones.
> >
> > It's not like once you write a message/tag it will stay that way fixed
> > for all time, that's just not going to fly with the way the Linux kernel
> > is developed.
> 
> The message tag should uniquely identify the message so that the
> translation projects have something to work with. If we keep changing
> the message tag with each kernel release that will create a huge effort
> to keep track of the messages.

But that's the point here.  The kernel does change a lot with each
release (you have seen the numbers, right?)  And if a change happens,
then the message also needs to change as something changed!  Why would
you think that these messages could be static till the end of time?
That's not how the kernel works.

> > > > > 2) we want to be able to use the same kmsg component in multiple .c
> > > > > files. 
> > > > 
> > > > Why would this matter?  It's just a "tag", who cares about the actual
> > > > name?
> > > 
> > > The actual name is not really important, but if the name is choosen
> > > wisely it does convey information. Guess what "dasd.17" tells you
> > > something about the dasd driver, "zfcp.42" about the zfcp driver and so
> > > on. The code structure should not dictate how the message tag is
> > > created.
> > 
> > The message tag should not dictate anything except how to look it up
> > somehow.  So it doesn't matter if the name changes, as long as the
> > ability to get the real information is still there.
> > 
> > So the kernel could change the tags every other release and there would
> > be no problem.
> 
> I think the unique message ids are very important. If we change them all
> the time this would severly limit the value of the kernel message
> catalog.

Again, they will be unique, but again, they will change as the kernel
changes, they will have to.

thanks,

greg k-h

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Thu, 2008-08-07 at 10:01 -0700, Greg KH wrote: 
> On Thu, Aug 07, 2008 at 10:31:41AM +0200, Martin Schwidefsky wrote:
> > On Wed, 2008-08-06 at 13:07 -0700, Greg KH wrote:
> > > > > > Using dev_printk won't work because of the order of the elements of the
> > > > > > printk. The kmsg tag should not have a "random" position in the printk
> > > > > > but should be the first element. If we use dev_printk the kmsg tag will
> > > > > > be the third element, for other kmsg printks it will be the first. In
> > > > > > addition the kmsg message tag for the device drivers already includes
> > > > > > the driver name ..
> > > > > 
> > > > > But the structure of dev_printk() is well definied and should be pretty
> > > > > trival to parse even with missing fields.
> > > > 
> > > > The missing field is the message tag. Which is the key for the message
> > > > lookup. As far as the kmsg catalog is concerned this is a pretty
> > > > important field that may not be missing from the printk itself.
> > > 
> > > No, I mean use dev_printk() as the base for your logging macro.  Add
> > > your message tag as the first field after the dev_printk() information.
> > 
> > Hmm, you are proposing to introduce a second format for the kmsg
> > messages to avoid the need for some more printk wrapper macros. To me it
> > seems that this has two problems:
> 
> No, only 1 format, use dev_printk() instead of printk() in your macro.

No, there are more printks in the system then just dev_printk. The kmsg interface
is supposed to cover all of them.

> > 1) The message tag is for the user of the system. If it does not have a
> > fixed position it gets confusing.
> 
> How would it not be in a fixed position with dev_printk()?

It is fixed in the macro for standard kmsg printks, there the message
tag is the first field. It is fixed in the macro for the dev_printk
variant of the kmsg message, there it is the third field. This mismatch
I refer to as not have a fixed position, for some message (the standard
ones) it is at the start of the final message, for others (the
dev_printk ones) it is in the middle of the message. Not good.

> > 2) The message tag for a driver message usually already includes the
> > driver name, the dev_printk will print it again. This is ugly and
> > reduces the quality of the message.
> 
> Then the message needs to change and remove that "driver name", as it is
> redundant, saving a tiny ammount of space :)

Then lets look at how this will look like. First a standard printk
message and its conversion to kmsg:
	printk(KERN_WARNING
               "cpcmd: could not allocate response buffer\n");
vs.
	kmsg_warn(1, "The cpcmd kernel function failed "
                     "to allocate a response buffer\n");

The message comes out as
	cpcmd: cound not allocate response buffer
vs.
	cpcmd.1: The cpcmd kernel function failed to allocate a response buffer

As an example for a dev_printk I use a message from the zfcp driver:
	dev_warn(&req->adapter->ccw_device->dev,
                 "The local link is down: no light detected.\n");
vs.
        kmsg_dev_warn(27, &req->adapter->ccw_device->dev,
                 "The local link is down: no light detected.\n");

The dev_printk versus the original kmsg macro comes out as
	zfcp: 0.0.1234: The local link is down: no light detected.
vs.
	zfcp.27: 0.0.1234: The local link is down: no light detected.

If I would just use dev_printk in kmsg_dev_warn as proposed by Greg:
	zfcp: 0.0.1234: zfcp.27: The  local link is down: no light detected.

If the message component is skipped from the message tag:
	zfcp: 0.0.1234: 27: The  local link is down: no light detected.

Is it just me who thinks that the later two message variants are crap? 

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Thu, 2008-08-07 at 08:59 -0700, Joe Perches wrote:
> On Thu, 2008-08-07 at 10:31 +0200, Martin Schwidefsky wrote:
> > On Wed, 2008-08-06 at 13:07 -0700, Greg KH wrote:
> > > No, I mean use dev_printk() as the base for your logging macro.  Add
> > > your message tag as the first field after the dev_printk() information.
> > Hmm, you are proposing to introduce a second format for the kmsg
> > messages to avoid the need for some more printk wrapper macros. To me it
> > seems that this has two problems:
> > 1) The message tag is for the user of the system. If it does not have a
> > fixed position it gets confusing.
> 
> I think the message tag today is not useful for a user.
> 
> It seems a barely useful maintainer value.
> 
> Perhaps the message tag allows for run-time language
> translation and maybe a bit better selective logging.

Aehhm, .. what ? If a user does not understand a message directly
how can he NOT be interested in the kmsg tag? As a random example
consider the situation when the user gets the message

	monwriter.1: Writing monitor data failed with rc=5

Usually a user will go "Huh?" except if he is a z/VM expert. Looking
at the source code won't help too much either:

static int monwrite_diag(struct monwrite_hdr *myhdr, char *buffer, int fcn)
{
        struct appldata_product_id id;
        int rc;

        strcpy(id.prod_nr, "LNXAPPL");
        id.prod_fn = myhdr->applid;
        id.record_nr = myhdr->record_num;
        id.version_nr = myhdr->version;
        id.release_nr = myhdr->release;
        id.mod_lvl = myhdr->mod_level;
        rc = appldata_asm(&id, fcn, (void *) buffer, myhdr->datalen);
        if (rc <= 0)
                return rc;
        if (rc == 5)
                return -EPERM;
        kmsg_err(1,"Writing monitor data failed with rc=%i\n", rc);
        return -EINVAL;
}

What the user should do is to cut-copy-paste the kmsg tag monwriter.1
and pass it to man:

# man monwriter.1

What the user will then get is the man page that has been generated
from the kmsg comment with "make D=2":

--- snip
monwriter.1(9)                                                  monwriter.1(9)

Message
       monwriter.1: Writing monitor data failed with rc=%i

Severity
       Error

Parameters
       @1: return code

Description
       The  monitor  stream  application  device driver used the z/VM diagnose
       call DIAG X'DC' to start writing monitor data. z/VM returned  an  error
       and  the  monitor data cannot be written. If the return code is 5, your
       z/VM guest virtual machine is not authorized to write monitor data.

User action
       If the return code is 5, ensure that your z/VM guest virtual  machine's
       entry in the z/VM directory includes the OPTION APPLMON statement.  For
       other return codes see the section about DIAGNOSE Code X'DC'  in  "z/VM
       CP Programming Services".

LINUX                           Linux Messages                  monwriter.1(9)
--- snip

Now the user actually gets help: the description tells him that rc=5
probably is because the virtual machine is not authorized. The key to
be able to get to the description associated to a message is the kmsg
tag.

By the way the kmsg comment for monwriter.1 is as follows just in case
you are wondering:

/*?
 * Tag: monwriter.1
 * Text: "Writing monitor data failed with rc=%i"
 * Severity: Error
 * Parameter:
 *   @1: return code
 * Description:
 * The monitor stream application device driver used the z/VM diagnose call
 * DIAG X'DC' to start writing monitor data. z/VM returned an error and the
 * monitor data cannot be written. If the return code is 5, your z/VM guest
 * virtual machine is not authorized to write monitor data.
 * User action:
 * If the return code is 5, ensure that your z/VM guest virtual machine's
 * entry in the z/VM directory includes the OPTION APPLMON statement.
 * For other return codes see the section about DIAGNOSE Code X'DC'
 * in "z/VM CP Programming Services".
 */

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Jan Kara]

On Sun 10-08-08 02:03:41, Martin Schwidefsky wrote:
...
> Then lets look at how this will look like. First a standard printk
> message and its conversion to kmsg:
> 	printk(KERN_WARNING
>                "cpcmd: could not allocate response buffer\n");
> vs.
> 	kmsg_warn(1, "The cpcmd kernel function failed "
>                      "to allocate a response buffer\n");
> 
> The message comes out as
> 	cpcmd: cound not allocate response buffer
> vs.
> 	cpcmd.1: The cpcmd kernel function failed to allocate a response buffer
> 
> As an example for a dev_printk I use a message from the zfcp driver:
> 	dev_warn(&req->adapter->ccw_device->dev,
>                  "The local link is down: no light detected.\n");
> vs.
>         kmsg_dev_warn(27, &req->adapter->ccw_device->dev,
>                  "The local link is down: no light detected.\n");
> 
> The dev_printk versus the original kmsg macro comes out as
> 	zfcp: 0.0.1234: The local link is down: no light detected.
> vs.
> 	zfcp.27: 0.0.1234: The local link is down: no light detected.
> 
> If I would just use dev_printk in kmsg_dev_warn as proposed by Greg:
> 	zfcp: 0.0.1234: zfcp.27: The  local link is down: no light detected.
> 
> If the message component is skipped from the message tag:
> 	zfcp: 0.0.1234: 27: The  local link is down: no light detected.
> 
> Is it just me who thinks that the later two message variants are crap? 
  If I may vote in this poll ;) I agree with you that the first two device
messages look better (more comprehensible) than the last two...

									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

On Wed, Jul 30, 2008 at 9:56 AM, Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:
> From: Michael Holzheu <holzheu@de.ibm.com>
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
>
> Introduce a new family of printk macros which prefixes each kmsg message
> with a component name and allows to tag the printk with a message id.

We've been talking about doing something like this, just this week.  AKPM
pointed me at this thread.  Good timing. :)

> +#define kmsg_dev_alert(id, dev, format, arg...) \
> +    printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
> +        ": %s: " format, (dev)->bus_id , ## arg)

Rather than the gcc-specific '## arg', shouldn't we use the standard C99
form: '##__VA_ARGS__'  ?

As a format, I would argue to leave out the spaces after colons, making
the inevitable string splitting even easier.

I don't care for the magic file-global KMSG_COMPONENT.  It just feels
wrong.  In most cases, I want the prefix to be hierarchical.  For example,
rather than "tg3" I want "net.tg3" or "netdev:tg3" or something.

Further, We want to make a clearer denotation between "just another
printk()" and these things, which are really "important events".  Maybe a
global prefix that makes it easy to seperate them out?  Or a parallel
"event" interface that logs to printk() and a netlink socket?


Tim

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Rusty Russell]

On Thursday 31 July 2008 02:56:57 Martin Schwidefsky wrote:
> From: Michael Holzheu <holzheu@de.ibm.com>
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
>
> Introduce a new family of printk macros which prefixes each kmsg message
> with a component name and allows to tag the printk with a message id.

Can you hash the format string to generate the id?  6 hex digits should be 
enough, and your tool can check for clashes.  As it's bad form to have 
identical strings for different semantics anyway, this seems to make sense.

Rusty.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

But there are a number of places that have different strings for the
same meaning: "Can't" vs "can't" vs "can not" vs "failed to", etc.

This is a big part of what hurts us - you end up with lots of loose
regexes when parsing logs.

Tim



On Tue, Aug 12, 2008 at 9:33 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> On Thursday 31 July 2008 02:56:57 Martin Schwidefsky wrote:
>> From: Michael Holzheu <holzheu@de.ibm.com>
>> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
>>
>> Introduce a new family of printk macros which prefixes each kmsg message
>> with a component name and allows to tag the printk with a message id.
>
> Can you hash the format string to generate the id?  6 hex digits should be
> enough, and your tool can check for clashes.  As it's bad form to have
> identical strings for different semantics anyway, this seems to make sense.
>
> 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/
>

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Pavel Machek]

Hi!

> But there are a number of places that have different strings for the
> same meaning: "Can't" vs "can't" vs "can not" vs "failed to", etc.

Do you have examples?

That wastes space in bzImage (and hints of duplicated code), so we may
want to fix it anyway.

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

On Wed, Aug 13, 2008 at 12:13 AM, Pavel Machek <pavel@suse.cz> wrote:
> Hi!
>
>> But there are a number of places that have different strings for the
>> same meaning: "Can't" vs "can't" vs "can not" vs "failed to", etc.
>
> Do you have examples?
>
> That wastes space in bzImage (and hints of duplicated code), so we may
> want to fix it anyway.

I can look for the specific examples.  I just saw the regexes we use
this week and was taken aback at how they have been loosened for these
sorts of cases.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Rusty Russell]

On Wednesday 13 August 2008 17:04:11 Tim Hockin wrote:
> But there are a number of places that have different strings for the
> same meaning: "Can't" vs "can't" vs "can not" vs "failed to", etc.
>
> This is a big part of what hurts us - you end up with lots of loose
> regexes when parsing logs.

Well, if they're to be the same messages under the manual-numbering scheme, 
someone has to verify that they are really the same anyway.  Not much more 
work to simply fix them in that case, is it?

If it improves normal kernel messages, then everyone wins.
Rusty.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

On Aug 13, 2008 9:15 PM, "Rusty Russell" <rusty@rustcorp.com.au> wrote:

> On Wednesday 13 August 2008 17:04:11 Tim Hockin wrote:
>> But there are a number of places that have ...Well, if they're to be the
>> same messages under the manual-numbering scheme,
>
> someone has to verify that they are really the same anyway.  Not much more
> work to simply fix them in that case, is it?
>
> If it improves normal kernel messages, then everyone wins.

Oh, I agree with that.  I personally don't care much for the msg ID part of
this patch (I mean I don't care either way).  It might be nice to have man
pages, but I just see it becoming out of sync, duplicate numbers, etc.

What I want to see is the elevation of these call-sites from "it's just a
printk()" to "it's a report-worthy event and part of our user<->kernel API".  I
want people to add them when it's appropriate and then not change the strings
later.  I don't care if, in the end, they are structured strings.  I do care if
I have to run dozens of regexes against thousands of log-lines on thousands of
systems.

:)

Tim

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Tue, 2008-08-12 at 17:35 -0700, Tim Hockin wrote:
> On Wed, Jul 30, 2008 at 9:56 AM, Martin Schwidefsky
> <schwidefsky@de.ibm.com> wrote:
> > From: Michael Holzheu <holzheu@de.ibm.com>
> > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> >
> > Introduce a new family of printk macros which prefixes each kmsg message
> > with a component name and allows to tag the printk with a message id.
> 
> We've been talking about doing something like this, just this week.  AKPM
> pointed me at this thread.  Good timing. :)

Good to hear that we are not the only one working on this.

> > +#define kmsg_dev_alert(id, dev, format, arg...) \
> > +    printk(__KMSG_CHECK(ALERT, id) KMSG_COMPONENT \
> > +        ": %s: " format, (dev)->bus_id , ## arg)
> 
> Rather than the gcc-specific '## arg', shouldn't we use the standard C99
> form: '##__VA_ARGS__'  ?

Ok, I changed the macros accordingly.

> As a format, I would argue to leave out the spaces after colons, making
> the inevitable string splitting even easier.

Makes sense and it does not make the message harder to read.

> I don't care for the magic file-global KMSG_COMPONENT.  It just feels
> wrong.  In most cases, I want the prefix to be hierarchical.  For example,
> rather than "tg3" I want "net.tg3" or "netdev:tg3" or something.

But what is the alternative to the #define to specify the base name of
the message tag? KBUILD_MODNAME has been proposed but I don't think this
is a good idea since I want the message tags to be independent of the
code structure.

> Further, We want to make a clearer denotation between "just another
> printk()" and these things, which are really "important events".  Maybe a
> global prefix that makes it easy to seperate them out?  Or a parallel
> "event" interface that logs to printk() and a netlink socket?

Well the kmsg macros distinguish between id==0 and id>0 messages. For a
id==0 message there is no documentation required and the kmsg prefix
will be just the KMSG_COMPONENT. You could consider that to be "just
another printk". For id>0 the kmsg macros will create the tuppel
KMSG_COMPONENT.ID so you already can separate "normal" and "important"
prinkts.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Wed, 2008-08-13 at 14:33 +1000, Rusty Russell wrote:
> On Thursday 31 July 2008 02:56:57 Martin Schwidefsky wrote:
> > From: Michael Holzheu <holzheu@de.ibm.com>
> > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> >
> > Introduce a new family of printk macros which prefixes each kmsg message
> > with a component name and allows to tag the printk with a message id.
> 
> Can you hash the format string to generate the id?  6 hex digits should be 
> enough, and your tool can check for clashes.  As it's bad form to have 
> identical strings for different semantics anyway, this seems to make sense.

First, how do you do that with the C preprocessor? Second the kmsg-doc
script will check that the message format strings match if a kmsg with a
specific id is used in multiple locations.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Thu, 2008-08-14 at 08:40 -0700, Tim Hockin wrote:
> On Aug 13, 2008 9:15 PM, "Rusty Russell" <rusty@rustcorp.com.au> wrote:
> 
> > On Wednesday 13 August 2008 17:04:11 Tim Hockin wrote:
> >> But there are a number of places that have ...Well, if they're to be the
> >> same messages under the manual-numbering scheme,
> >
> > someone has to verify that they are really the same anyway.  Not much more
> > work to simply fix them in that case, is it?
> >
> > If it improves normal kernel messages, then everyone wins.
> 
> Oh, I agree with that.  I personally don't care much for the msg ID part of
> this patch (I mean I don't care either way).  It might be nice to have man
> pages, but I just see it becoming out of sync, duplicate numbers, etc.

No, the kmsg-doc script will prevent that the kmsg message will become
of of sync and it checks for duplicate numbers. The idea here is that
the kmsg man pages are generated with the kernel compile (just try it
with "make D=2").

> What I want to see is the elevation of these call-sites from "it's just a
> printk()" to "it's a report-worthy event and part of our user<->kernel API".  I
> want people to add them when it's appropriate and then not change the strings
> later.  I don't care if, in the end, they are structured strings.  I do care if
> I have to run dozens of regexes against thousands of log-lines on thousands of
> systems.
> 
> :)

Yes, and this has already happened with the s390 device driver messages.
With the conversion to kmsg our developers removed a lot of crud from
the code, this alone makes it worthwhile.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

On Thu, Aug 14, 2008 at 10:04 AM, Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:

>> I don't care for the magic file-global KMSG_COMPONENT. It just feels
>> wrong. In most cases, I want the prefix to be hierarchical. For example,
>> rather than "tg3" I want "net.tg3" or "netdev:tg3" or something.
>
> But what is the alternative to the #define to specify the base name of
> the message tag? KBUILD_MODNAME has been proposed but I don't think this
> is a good idea since I want the message tags to be independent of the
> code structure.

We had talked internally about something like this:

  globally:

    // this is the actual implementation
    extern void send_kmsg(int level, const char *fmt, ...);

    // this is a convenience wrapper, maybe not needed as they get combinatoric
    #define kmsg_err(fmt, ...) \
    	send_kmsg(KERN_ERROR, fmt, ##__VA_ARGS__)

  inside a subsystem-specific header:

    #define mysubsys_kmsg_err(fmt, ...) \
    	kmsg_err("mysubsys:" fmt "\n", ##__VA_ARGS__)

  inside each subsystem-driver:

    #define mydriver_kmsg_err(fmt, ...) \
    	mysubsys_kmsg_err("mydriver:" fmt, ##__VA_ARGS__)

  then the driver just calls:

    mydriver_kmsg_err("something happened");

  and you get, at the bottom:

    printk(KERN_ERROR "mysubsy:mydriver:something happened\n");

I'm not sure how your ID number fits in there, propbably

    printk(KERN_ERROR "mysubsy:mydriver.%d something happened\n", id);

That way, the hierarchy is defined by the meaningful hierarchy of
subsystems and busses and drivers.  Subsystems which have device structs
can add them into the mix (netdev, ATA, etc).  The core infrastructure
stays small, while providing driver and subsys maintainers the flexibility
to define their own event hierarchies.

>> Further, We want to make a clearer denotation between "just another
>> printk()" and these things, which are really "important events". Maybe a
>> global prefix that makes it easy to seperate them out? Or a parallel
>> "event" interface that logs to printk() and a netlink socket?
>
> Well the kmsg macros distinguish between id==0 and id>0 messages. For a
> id==0 message there is no documentation required and the kmsg prefix
> will be just the KMSG_COMPONENT. You could consider that to be "just
> another printk". For id>0 the kmsg macros will create the tuppel
> KMSG_COMPONENT.ID so you already can separate "normal" and "important"
> prinkts.

That works, I suppose.  I'd still advocate for a netlink-style conduit
(maybe with buffering?) for "important event" kmsgs, separate from plain
old printk.

I'd also advocate that maybe we should go a bit further with the
structure, by providing a soft-structured set of key-value pairs.  I'm not
100% on the details, but something like:

    printk(KERN_ERROR
    	"mysubsy:mydriver.%d something happened -- pid=%d status=0x%04x\n",
    	id, pid, status);

Where everything after "--" (or some other delimiter) is a key=value.

>> What I want to see is the elevation of these call-sites from "it's just a
>> printk()" to "it's a report-worthy event and part of our user<->kernel
>> API".  I want people to add them when it's appropriate and then not change
>> the strings later.  I don't care if, in the end, they are structured
>> strings.  I do care if I have to run dozens of regexes against thousands
>> of log-lines on thousands of systems.

> Yes, and this has already happened with the s390 device driver messages.
> With the conversion to kmsg our developers removed a lot of crud from
> the code, this alone makes it worthwhile.

Do you see *every* printk() becoming a kmsg?  Just printk() in drivers?
Or just some hand-chosen printk()s ?

Tim

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Rusty Russell]

On Friday 15 August 2008 03:07:03 Martin Schwidefsky wrote:
> On Wed, 2008-08-13 at 14:33 +1000, Rusty Russell wrote:
> > On Thursday 31 July 2008 02:56:57 Martin Schwidefsky wrote:
> > > From: Michael Holzheu <holzheu@de.ibm.com>
> > > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > >
> > > Introduce a new family of printk macros which prefixes each kmsg
> > > message with a component name and allows to tag the printk with a
> > > message id.
> >
> > Can you hash the format string to generate the id?  6 hex digits should
> > be enough, and your tool can check for clashes.  As it's bad form to have
> > identical strings for different semantics anyway, this seems to make
> > sense.
>
> First, how do you do that with the C preprocessor? Second the kmsg-doc
> script will check that the message format strings match if a kmsg with a
> specific id is used in multiple locations.

Well, why would you do it with the preprocessor?  It's simplest to spit it out 
at runtime.

The (offline) catalogger will need to generate them, of course.
Rusty.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Joe Perches]

On Thu, 2008-08-14 at 11:50 -0700, Tim Hockin wrote:
>     // this is the actual implementation
>     extern void send_kmsg(int level, const char *fmt, ...);
>     // this is a convenience wrapper, maybe not needed as they get combinatoric
>     #define kmsg_err(fmt, ...) \
>     	send_kmsg(KERN_ERROR, fmt, ##__VA_ARGS__)
>   inside a subsystem-specific header:
>     #define mysubsys_kmsg_err(fmt, ...) \
>     	kmsg_err("mysubsys:" fmt "\n", ##__VA_ARGS__)
>   inside each subsystem-driver:
>     #define mydriver_kmsg_err(fmt, ...) \
>     	mysubsys_kmsg_err("mydriver:" fmt, ##__VA_ARGS__)
>   then the driver just calls:
>     mydriver_kmsg_err("something happened");

I think adding system/subsystem/driver prefixes
to function names is not good.

Though Martin Schwidefsky thinks KMSG_COMPONENT is necessary,
I am unconvinced.

Perhaps using something like Jason Baron's new dynamic
debug infrastructure, all the event logging functions
could optionally print KBUILD_MODNAME, __func__ and __line__
if desired.

> Do you see *every* printk() becoming a kmsg?
> Just printk() in drivers?
> Or just some hand-chosen printk()s ?

I'd prefer every printk(KERN_<level> become pr_<level>
and kernel.h be reorganized so that all logging functions
are declared in a separate include file, maybe logging.h

kmsg_<level> could eventually live in logging.h

Here's a reorganization of kernel.h:
----------------------

Create <linux/logging.h>
Move content of <linux/ratelimit.h> into <linux/logging.h>
Delete <linux/ratelimit.h>
Move logging content from <linux/kernel.h> into <linux/logging.h>

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/linux/kernel.h    |  109 +----------------------------------
 include/linux/logging.h   |  139 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/ratelimit.h |   27 ---------
 3 files changed, 141 insertions(+), 134 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2651f80..63e647b 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -15,13 +15,11 @@
 #include <linux/bitops.h>
 #include <linux/log2.h>
 #include <linux/typecheck.h>
-#include <linux/ratelimit.h>
+#include <linux/logging.h>
+
 #include <asm/byteorder.h>
 #include <asm/bug.h>
 
-extern const char linux_banner[];
-extern const char linux_proc_banner[];
-
 #define USHORT_MAX	((u16)(~0U))
 #define SHORT_MAX	((s16)(USHORT_MAX>>1))
 #define SHORT_MIN	(-SHORT_MAX - 1)
@@ -81,29 +79,6 @@ extern const char linux_proc_banner[];
  */
 #define lower_32_bits(n) ((u32)(n))
 
-#define	KERN_EMERG	"<0>"	/* system is unusable			*/
-#define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
-#define	KERN_CRIT	"<2>"	/* critical conditions			*/
-#define	KERN_ERR	"<3>"	/* error conditions			*/
-#define	KERN_WARNING	"<4>"	/* warning conditions			*/
-#define	KERN_NOTICE	"<5>"	/* normal but significant condition	*/
-#define	KERN_INFO	"<6>"	/* informational			*/
-#define	KERN_DEBUG	"<7>"	/* debug-level messages			*/
-
-/*
- * Annotation for a "continued" line of log printout (only done after a
- * line that had no enclosing \n). Only to be used by core/arch code
- * during early bootup (a continued line is not SMP-safe otherwise).
- */
-#define	KERN_CONT	""
-
-extern int console_printk[];
-
-#define console_loglevel (console_printk[0])
-#define default_message_loglevel (console_printk[1])
-#define minimum_console_loglevel (console_printk[2])
-#define default_console_loglevel (console_printk[3])
-
 struct completion;
 struct pt_regs;
 struct user;
@@ -190,48 +165,8 @@ extern int kernel_text_address(unsigned long addr);
 struct pid;
 extern struct pid *session_of_pgrp(struct pid *pgrp);
 
-#ifdef CONFIG_PRINTK
-asmlinkage int vprintk(const char *fmt, va_list args)
-	__attribute__ ((format (printf, 1, 0)));
-asmlinkage int printk(const char * fmt, ...)
-	__attribute__ ((format (printf, 1, 2))) __cold;
-
-extern struct ratelimit_state printk_ratelimit_state;
-extern int printk_ratelimit(void);
-extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
-				   unsigned int interval_msec);
-#else
-static inline int vprintk(const char *s, va_list args)
-	__attribute__ ((format (printf, 1, 0)));
-static inline int vprintk(const char *s, va_list args) { return 0; }
-static inline int printk(const char *s, ...)
-	__attribute__ ((format (printf, 1, 2)));
-static inline int __cold printk(const char *s, ...) { return 0; }
-static inline int printk_ratelimit(void) { return 0; }
-static inline bool printk_timed_ratelimit(unsigned long *caller_jiffies, \
-					  unsigned int interval_msec)	\
-		{ return false; }
-#endif
-
-extern void asmlinkage __attribute__((format(printf, 1, 2)))
-	early_printk(const char *fmt, ...);
-
 unsigned long int_sqrt(unsigned long);
 
-static inline void console_silent(void)
-{
-	console_loglevel = 0;
-}
-
-static inline void console_verbose(void)
-{
-	if (console_loglevel)
-		console_loglevel = 15;
-}
-
-extern void bust_spinlocks(int yes);
-extern void wake_up_klogd(void);
-extern int oops_in_progress;		/* If set, an oops, panic(), BUG() or die() is in progress */
 extern int panic_timeout;
 extern int panic_on_oops;
 extern int panic_on_unrecovered_nmi;
@@ -261,22 +196,6 @@ extern enum system_states {
 #define TAINT_OVERRIDDEN_ACPI_TABLE	(1<<8)
 #define TAINT_WARN			(1<<9)
 
-extern void dump_stack(void) __cold;
-
-enum {
-	DUMP_PREFIX_NONE,
-	DUMP_PREFIX_ADDRESS,
-	DUMP_PREFIX_OFFSET
-};
-extern void hex_dump_to_buffer(const void *buf, size_t len,
-				int rowsize, int groupsize,
-				char *linebuf, size_t linebuflen, bool ascii);
-extern void print_hex_dump(const char *level, const char *prefix_str,
-				int prefix_type, int rowsize, int groupsize,
-				const void *buf, size_t len, bool ascii);
-extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
-			const void *buf, size_t len);
-
 extern const char hex_asc[];
 #define hex_asc_lo(x)	hex_asc[((x) & 0x0f)]
 #define hex_asc_hi(x)	hex_asc[((x) & 0xf0) >> 4]
@@ -288,30 +207,6 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
 	return buf;
 }
 
-#define pr_emerg(fmt, arg...) \
-	printk(KERN_EMERG fmt, ##arg)
-#define pr_alert(fmt, arg...) \
-	printk(KERN_ALERT fmt, ##arg)
-#define pr_crit(fmt, arg...) \
-	printk(KERN_CRIT fmt, ##arg)
-#define pr_err(fmt, arg...) \
-	printk(KERN_ERR fmt, ##arg)
-#define pr_warning(fmt, arg...) \
-	printk(KERN_WARNING fmt, ##arg)
-#define pr_notice(fmt, arg...) \
-	printk(KERN_NOTICE fmt, ##arg)
-#define pr_info(fmt, arg...) \
-	printk(KERN_INFO fmt, ##arg)
-
-#ifdef DEBUG
-/* If you are writing a driver, please use dev_dbg instead */
-#define pr_debug(fmt, arg...) \
-	printk(KERN_DEBUG fmt, ##arg)
-#else
-#define pr_debug(fmt, arg...) \
-	({ if (0) printk(KERN_DEBUG fmt, ##arg); 0; })
-#endif
-
 /*
  *      Display an IP address in readable format.
  */
diff --git a/include/linux/logging.h b/include/linux/logging.h
new file mode 100644
index 0000000..63734cd
--- /dev/null
+++ b/include/linux/logging.h
@@ -0,0 +1,139 @@
+#ifndef _LINUX_LOGGING_H
+#define _LINUX_LOGGING_H
+
+#include <linux/param.h>
+
+extern const char linux_banner[];
+extern const char linux_proc_banner[];
+
+extern const char *print_tainted(void);
+
+extern int console_printk[];
+
+#define console_loglevel (console_printk[0])
+#define default_message_loglevel (console_printk[1])
+#define minimum_console_loglevel (console_printk[2])
+#define default_console_loglevel (console_printk[3])
+
+static inline void console_silent(void)
+{
+	console_loglevel = 0;
+}
+
+static inline void console_verbose(void)
+{
+	if (console_loglevel)
+		console_loglevel = 15;
+}
+
+
+#ifdef CONFIG_PRINTK
+asmlinkage int vprintk(const char *fmt, va_list args)
+	__attribute__ ((format (printf, 1, 0)));
+asmlinkage int printk(const char * fmt, ...)
+	__attribute__ ((format (printf, 1, 2))) __cold;
+
+extern struct ratelimit_state printk_ratelimit_state;
+extern int printk_ratelimit(void);
+extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
+				   unsigned int interval_msec);
+#else
+static inline int vprintk(const char *s, va_list args)
+	__attribute__ ((format (printf, 1, 0)));
+static inline int vprintk(const char *s, va_list args) { return 0; }
+static inline int printk(const char *s, ...)
+	__attribute__ ((format (printf, 1, 2)));
+static inline int __cold printk(const char *s, ...) { return 0; }
+static inline int printk_ratelimit(void) { return 0; }
+static inline bool printk_timed_ratelimit(unsigned long *caller_jiffies, \
+					  unsigned int interval_msec)	\
+		{ return false; }
+#endif
+
+extern void asmlinkage __attribute__((format(printf, 1, 2)))
+	early_printk(const char *fmt, ...);
+
+extern void dump_stack(void) __cold;
+
+enum {
+	DUMP_PREFIX_NONE,
+	DUMP_PREFIX_ADDRESS,
+	DUMP_PREFIX_OFFSET
+};
+extern void hex_dump_to_buffer(const void *buf, size_t len,
+				int rowsize, int groupsize,
+				char *linebuf, size_t linebuflen, bool ascii);
+extern void print_hex_dump(const char *level, const char *prefix_str,
+				int prefix_type, int rowsize, int groupsize,
+				const void *buf, size_t len, bool ascii);
+extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
+			const void *buf, size_t len);
+
+#define	KERN_EMERG	"<0>"	/* system is unusable			*/
+#define	KERN_ALERT	"<1>"	/* action must be taken immediately	*/
+#define	KERN_CRIT	"<2>"	/* critical conditions			*/
+#define	KERN_ERR	"<3>"	/* error conditions			*/
+#define	KERN_WARNING	"<4>"	/* warning conditions			*/
+#define	KERN_NOTICE	"<5>"	/* normal but significant condition	*/
+#define	KERN_INFO	"<6>"	/* informational			*/
+#define	KERN_DEBUG	"<7>"	/* debug-level messages			*/
+
+/*
+ * Annotation for a "continued" line of log printout (only done after a
+ * line that had no enclosing \n). Only to be used by core/arch code
+ * during early bootup (a continued line is not SMP-safe otherwise).
+ */
+#define	KERN_CONT	""
+
+#define pr_emerg(fmt, arg...) \
+	printk(KERN_EMERG fmt, ##arg)
+#define pr_alert(fmt, arg...) \
+	printk(KERN_ALERT fmt, ##arg)
+#define pr_crit(fmt, arg...) \
+	printk(KERN_CRIT fmt, ##arg)
+#define pr_err(fmt, arg...) \
+	printk(KERN_ERR fmt, ##arg)
+#define pr_warning(fmt, arg...) \
+	printk(KERN_WARNING fmt, ##arg)
+#define pr_notice(fmt, arg...) \
+	printk(KERN_NOTICE fmt, ##arg)
+#define pr_info(fmt, arg...) \
+	printk(KERN_INFO fmt, ##arg)
+
+#ifdef DEBUG
+/* If you are writing a driver, please use dev_dbg instead */
+#define pr_debug(fmt, arg...) \
+	printk(KERN_DEBUG fmt, ##arg)
+#else
+#define pr_debug(fmt, arg...) \
+	({ if (0) printk(KERN_DEBUG fmt, ##arg); 0; })
+#endif
+
+extern void bust_spinlocks(int yes);
+extern void wake_up_klogd(void);
+extern int oops_in_progress;		/* If set, an oops, panic(), BUG() or die() is in progress */
+
+#define DEFAULT_RATELIMIT_INTERVAL (5 * HZ)
+#define DEFAULT_RATELIMIT_BURST 10
+
+struct ratelimit_state {
+	int interval;
+	int burst;
+	int printed;
+	int missed;
+	unsigned long begin;
+};
+
+#define DEFINE_RATELIMIT_STATE(name, interval, burst)		\
+		struct ratelimit_state name = {interval, burst,}
+
+extern int __ratelimit(struct ratelimit_state *rs);
+
+static inline int ratelimit(void)
+{
+	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
+					DEFAULT_RATELIMIT_BURST);
+	return __ratelimit(&rs);
+}
+
+#endif
diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
deleted file mode 100644
index 18a5b9b..0000000
--- a/include/linux/ratelimit.h
+++ /dev/null
@@ -1,27 +0,0 @@
-#ifndef _LINUX_RATELIMIT_H
-#define _LINUX_RATELIMIT_H
-#include <linux/param.h>
-
-#define DEFAULT_RATELIMIT_INTERVAL (5 * HZ)
-#define DEFAULT_RATELIMIT_BURST 10
-
-struct ratelimit_state {
-	int interval;
-	int burst;
-	int printed;
-	int missed;
-	unsigned long begin;
-};
-
-#define DEFINE_RATELIMIT_STATE(name, interval, burst)		\
-		struct ratelimit_state name = {interval, burst,}
-
-extern int __ratelimit(struct ratelimit_state *rs);
-
-static inline int ratelimit(void)
-{
-	static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
-					DEFAULT_RATELIMIT_BURST);
-	return __ratelimit(&rs);
-}
-#endif

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Greg KH]

On Thu, Aug 14, 2008 at 08:08:59PM -0700, Joe Perches wrote:
> On Thu, 2008-08-14 at 11:50 -0700, Tim Hockin wrote:
> >     // this is the actual implementation
> >     extern void send_kmsg(int level, const char *fmt, ...);
> >     // this is a convenience wrapper, maybe not needed as they get combinatoric
> >     #define kmsg_err(fmt, ...) \
> >     	send_kmsg(KERN_ERROR, fmt, ##__VA_ARGS__)
> >   inside a subsystem-specific header:
> >     #define mysubsys_kmsg_err(fmt, ...) \
> >     	kmsg_err("mysubsys:" fmt "\n", ##__VA_ARGS__)
> >   inside each subsystem-driver:
> >     #define mydriver_kmsg_err(fmt, ...) \
> >     	mysubsys_kmsg_err("mydriver:" fmt, ##__VA_ARGS__)
> >   then the driver just calls:
> >     mydriver_kmsg_err("something happened");
> 
> I think adding system/subsystem/driver prefixes
> to function names is not good.

Eeek!

What is wrong with what we have already agreed to standardise on here
people?  dev_printk() for devices!  It uniquely shows the device, what
driver is bound to it (if any), the bus id, and everything else.

It's in a parseable, standard format, that everyone agreed a few years
was going to be the way to go!

And now you want to create your own new one?  Come on, please don't be
that foolish.  Please work with what we already have.  Extend it or
something to have these "magic tokens" that you all want to index off
of, that's fine, I really could care less (and I really think it's going
to be a pain in the end, does everyone forget how frickin fast this
body of software moves?)

But whatever you do, don't create yet-another-printk-macro that you are
going to want everyone to use in their drivers/modules.  We've already
done that, and we are still slowly converting people over to them (I did
just that for a large number USB drivers just today...)

Work off of dev_printk() and friends, don't do something new.

thanks,

greg k-h

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

On Thu, Aug 14, 2008 at 8:44 PM, Greg KH <gregkh@suse.de> wrote:
>
> What is wrong with what we have already agreed to standardise on here
> people?  dev_printk() for devices!  It uniquely shows the device, what
> driver is bound to it (if any), the bus id, and everything else.

Part of the problem, imho, is the "if any" part.  But I am more than happy to
build on existing solutions.  All the world is not a dev, though.  I'd like to
be able to report something like an OOM kill in (roughly) the same way as an
ATA error, and I want (though could be talked out of) a way to tell these
"events" (for lack of a better word) apart from plain-old-printk()s.

Tim

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Jan Blunck]

On Thu, Aug 14, Tim Hockin wrote:

> On Thu, Aug 14, 2008 at 8:44 PM, Greg KH <gregkh@suse.de> wrote:
> >
> > What is wrong with what we have already agreed to standardise on here
> > people?  dev_printk() for devices!  It uniquely shows the device, what
> > driver is bound to it (if any), the bus id, and everything else.
> 
> Part of the problem, imho, is the "if any" part.  But I am more than happy to
> build on existing solutions.  All the world is not a dev, though.  I'd like to
> be able to report something like an OOM kill in (roughly) the same way as an
> ATA error, and I want (though could be talked out of) a way to tell these
> "events" (for lack of a better word) apart from plain-old-printk()s.

If I interpret Martins patches correctly he wants to be able to put add
extended information to specific messages the kernel is printing. This is a
good way to explain the rational of certain situations to people unwilling to
read the sources ;)

I don't think that he wants to unify all the printk's in the system. I don't
think that reporting all errors "in the same way as an ATA error" makes any
sense. That would just lead to very stupid and unnatural messages for all
errors that are not like "ATA errors". Annotation of existing errors is a much
more flexible and feasible solution to that problem.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

On Fri, Aug 15, 2008 at 4:21 AM, Jan Blunck <jblunck@suse.de> wrote:
> On Thu, Aug 14, Tim Hockin wrote:
>
>> On Thu, Aug 14, 2008 at 8:44 PM, Greg KH <gregkh@suse.de> wrote:
>> >
>> > What is wrong with what we have already agreed to standardise on here
>> > people?  dev_printk() for devices!  It uniquely shows the device, what
>> > driver is bound to it (if any), the bus id, and everything else.
>>
>> Part of the problem, imho, is the "if any" part.  But I am more than happy to
>> build on existing solutions.  All the world is not a dev, though.  I'd like to
>> be able to report something like an OOM kill in (roughly) the same way as an
>> ATA error, and I want (though could be talked out of) a way to tell these
>> "events" (for lack of a better word) apart from plain-old-printk()s.
>
> I don't think that he wants to unify all the printk's in the system. I don't
> think that reporting all errors "in the same way as an ATA error" makes any
> sense. That would just lead to very stupid and unnatural messages for all
> errors that are not like "ATA errors". Annotation of existing errors is a much
> more flexible and feasible solution to that problem.

Please don't misinterpret.  I don't want to make other errors parse
like an ATA error, I want to make the plumbing be parallel.  I want
one umbrella mechanism for reporting things that are more important
than just-another-printk().

Because frankly, "parse dmesg" is a pretty crappy way to have to
monitor your system for failures, and I am tired of explaining to
people why we still do that.

Tim

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Greg KH]

On Thu, Aug 14, 2008 at 10:33:10PM -0700, Tim Hockin wrote:
> On Thu, Aug 14, 2008 at 8:44 PM, Greg KH <gregkh@suse.de> wrote:
> >
> > What is wrong with what we have already agreed to standardise on here
> > people?  dev_printk() for devices!  It uniquely shows the device, what
> > driver is bound to it (if any), the bus id, and everything else.
> 
> Part of the problem, imho, is the "if any" part.  But I am more than happy to
> build on existing solutions.  All the world is not a dev, though.

No, but way over half the kernel is.

> I'd like to be able to report something like an OOM kill in (roughly)
> the same way as an ATA error, and I want (though could be talked out
> of) a way to tell these "events" (for lack of a better word) apart
> from plain-old-printk()s.

That's great, then create something that can handle both!  Don't throw
away some wonderful information that way over half the kernel has access
to just because the minority doesn't.  That would mean that we would
loose information in those drivers overall.

thanks,

greg k-h

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

On Fri, Aug 15, 2008 at 9:03 AM, Greg KH <gregkh@suse.de> wrote:

>> I'd like to be able to report something like an OOM kill in (roughly)
>> the same way as an ATA error, and I want (though could be talked out
>> of) a way to tell these "events" (for lack of a better word) apart
>> from plain-old-printk()s.
>
> That's great, then create something that can handle both!  Don't throw
> away some wonderful information that way over half the kernel has access
> to just because the minority doesn't.  That would mean that we would
> loose information in those drivers overall.

Sorry, I never meant to imply that dev_printk() was to be ignored, my
examples was meant to be trivial to demonstrate :)  dev_printk() is
good.  We should keep it :)

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Rick Troth]

On Wed, 13 Aug 2008, Rusty Russell wrote:
>
> Can you hash the format string to generate the id?  6 hex digits should be
> enough, and your tool can check for clashes.  As it's bad form to have
> identical strings for different semantics anyway, this seems to make sense.

I second this suggestion (or propose something like it).
A short hash or even an index into some off-line table
lets the user hit the right page when looking for an explanation.

I've been using a similar (and perhaps ugly to some eyes)
numeric message catalog for years (with token substitution and all).
The catalogs can be translated and then replaced with local language
so the users don't have to learn English (or French, or German,
or Japanese, or Chinese).

-- R;   <><

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Fri, 2008-08-15 at 16:05 -0400, Rick Troth wrote:
> On Wed, 13 Aug 2008, Rusty Russell wrote:
> > Can you hash the format string to generate the id?  6 hex digits should be
> > enough, and your tool can check for clashes.  As it's bad form to have
> > identical strings for different semantics anyway, this seems to make sense.
> 
> I second this suggestion (or propose something like it).
> A short hash or even an index into some off-line table
> lets the user hit the right page when looking for an explanation.

The kmsg id is just that, an index into the table of message for a
particular component.

> I've been using a similar (and perhaps ugly to some eyes)
> numeric message catalog for years (with token substitution and all).
> The catalogs can be translated and then replaced with local language
> so the users don't have to learn English (or French, or German,
> or Japanese, or Chinese).

That is what we are trying to achieve as well.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Fri, 2008-08-15 at 09:22 +1000, Rusty Russell wrote:
> On Friday 15 August 2008 03:07:03 Martin Schwidefsky wrote:
> > On Wed, 2008-08-13 at 14:33 +1000, Rusty Russell wrote:
> > > On Thursday 31 July 2008 02:56:57 Martin Schwidefsky wrote:
> > > > From: Michael Holzheu <holzheu@de.ibm.com>
> > > > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > > >
> > > > Introduce a new family of printk macros which prefixes each kmsg
> > > > message with a component name and allows to tag the printk with a
> > > > message id.
> > >
> > > Can you hash the format string to generate the id?  6 hex digits should
> > > be enough, and your tool can check for clashes.  As it's bad form to have
> > > identical strings for different semantics anyway, this seems to make
> > > sense.
> >
> > First, how do you do that with the C preprocessor? Second the kmsg-doc
> > script will check that the message format strings match if a kmsg with a
> > specific id is used in multiple locations.
> 
> Well, why would you do it with the preprocessor?  It's simplest to spit it out 
> at runtime.

Ok, so  a specialized version of printk will do the work to calculate
the hash. Only, what will we do if there ever is a conflict? The message
tag has to be unique. The shorter the hash is, the more likely a
collision gets. Don't know if 6 hash digits is enough to just ignore the
problem. 

> The (offline) catalogger will need to generate them, of course.
> Rusty.

Sounds doable to me.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Fri, 2008-08-15 at 10:03 -0700, Tim Hockin wrote:
> On Fri, Aug 15, 2008 at 9:03 AM, Greg KH <gregkh@suse.de> wrote:
> >> I'd like to be able to report something like an OOM kill in (roughly)
> >> the same way as an ATA error, and I want (though could be talked out
> >> of) a way to tell these "events" (for lack of a better word) apart
> >> from plain-old-printk()s.
> >
> > That's great, then create something that can handle both!  Don't throw
> > away some wonderful information that way over half the kernel has access
> > to just because the minority doesn't.  That would mean that we would
> > loose information in those drivers overall.
> 
> Sorry, I never meant to imply that dev_printk() was to be ignored, my
> examples was meant to be trivial to demonstrate :)  dev_printk() is
> good.  We should keep it :)

The invention of dev_printk was a good thing. It adds structure to the
messages if it is about a device. Before we had a simple printk that did
or did not include information about the device driver and the device
name. dev_printk makes sure that this information is always added.
BUT, what dev_printk does not do is to make sure that the message can be
uniquely identified so that the description of the message can be found
in the message catalog. Our approach is to combine the driver name and a
simple index into a message tag. The original dev_printk remains almost
unchanged. A typical hunk in the code looks like this:

@@ -24,8 +27,8 @@ static int zfcp_ccw_probe(struct ccw_dev

        down(&zfcp_data.config_sema);
        if (zfcp_adapter_enqueue(ccw_device)) {
-               dev_err(&ccw_device->dev,
-                       "Setup of data structures failed.\n");
+               kmsg_dev_err(73, &ccw_device->dev,
+                            "Setup of data structures failed.\n");
                retval = -EINVAL;
        }
        up(&zfcp_data.config_sema);

The kmsg_dev_printk still has almost the same structure, only the
message id is added. There is one real difference though:
the driver name is specified with KMSG_COMPONENT and not via
dev_driver_string(dev). We do that so that the kmsg-doc script can
actually find the driver name. The dev_driver_string(dev) is dynamic and
a static tool has a hard time to find the actual driver string.  

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Joe Perches]

On Sun, 2008-08-10 at 02:08 +0200, Martin Schwidefsky wrote:
> On Thu, 2008-08-07 at 08:59 -0700, Joe Perches wrote:
> > On Thu, 2008-08-07 at 10:31 +0200, Martin Schwidefsky wrote:
> > > 1) The message tag is for the user of the system. If it does not have a
> > > fixed position it gets confusing.
> > I think the message tag today is not useful for a user.
> > It seems a barely useful maintainer value.
> > Perhaps the message tag allows for run-time language
> > translation and maybe a bit better selective logging.
> Aehhm, .. what ? If a user does not understand a message directly
> how can he NOT be interested in the kmsg tag? As a random example
> consider the situation when the user gets the message
> 
> 	monwriter.1: Writing monitor data failed with rc=5
> 
> Usually a user will go "Huh?" except if he is a z/VM expert. Looking
> at the source code won't help too much either:
> 
> static int monwrite_diag(struct monwrite_hdr *myhdr, char *buffer, int fcn)
> {
>         struct appldata_product_id id;
>         int rc;
> 
>         strcpy(id.prod_nr, "LNXAPPL");
>         id.prod_fn = myhdr->applid;
>         id.record_nr = myhdr->record_num;
>         id.version_nr = myhdr->version;
>         id.release_nr = myhdr->release;
>         id.mod_lvl = myhdr->mod_level;
>         rc = appldata_asm(&id, fcn, (void *) buffer, myhdr->datalen);
>         if (rc <= 0)
>                 return rc;
>         if (rc == 5)
>                 return -EPERM;
>         kmsg_err(1,"Writing monitor data failed with rc=%i\n", rc);
>         return -EINVAL;
> }
> 
> What the user should do is to cut-copy-paste the kmsg tag monwriter.1
> and pass it to man:
> 
> # man monwriter.1
> 
> What the user will then get is the man page that has been generated
> from the kmsg comment with "make D=2":
> 
> --- snip
> monwriter.1(9)                                                  monwriter.1(9)
> 
> Message
>        monwriter.1: Writing monitor data failed with rc=%i
> 
> Severity
>        Error
> 
> Parameters
>        @1: return code
> 
> Description
>        The  monitor  stream  application  device driver used the z/VM diagnose
>        call DIAG X'DC' to start writing monitor data. z/VM returned  an  error
>        and  the  monitor data cannot be written. If the return code is 5, your
>        z/VM guest virtual machine is not authorized to write monitor data.
> 
> User action
>        If the return code is 5, ensure that your z/VM guest virtual  machine's
>        entry in the z/VM directory includes the OPTION APPLMON statement.  For
>        other return codes see the section about DIAGNOSE Code X'DC'  in  "z/VM
>        CP Programming Services".

Doesn't this example better illustrate the barely useful
user value of such a message and documentation?

In your example, monwriter.1 with a return code of 5 isn't possible.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

On Sat, Aug 16, 2008 at 10:49 AM, Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:

>> > > Can you hash the format string to generate the id?  6 hex digits should
>> > > be enough, and your tool can check for clashes.  As it's bad form to have
>> > > identical strings for different semantics anyway, this seems to make
>> > > sense.

> Ok, so  a specialized version of printk will do the work to calculate
> the hash. Only, what will we do if there ever is a conflict? The message
> tag has to be unique. The shorter the hash is, the more likely a
> collision gets. Don't know if 6 hash digits is enough to just ignore the
> problem.

And if you ever need to change the text that is in the format string?
The hash changes.  That seems exactly counter to your goal...

Tim

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Rick Troth]

This is really an easy thing to do.
It can be achieved without a huge retro-fit.
It can happen even without yet another macro.
(Not saying that there should not be a new macro or new function,
just that we can enjoy unique message IDs without those mechanisms.)

Martin Schwidefsky said:
> > Ok, so  a specialized version of printk will do the work to calculate
> > the hash. Only, what will we do if there ever is a conflict? The message
> > tag has to be unique. The shorter the hash is, the more likely a
> > collision gets. Don't know if 6 hash digits is enough to just ignore the
> > problem.

Point to note: the message tag has to be unique.
The tag is what's relevant, what gets catalogued.

I don't really see the value of a hash per se.
What Martin has suggested (unless I have misunderstood) is
the driver name combined with some index number ==> unique msg id.
That's not exactly a "hash".  (One can generate a hash from it,
but why bother?  You've got a unique string already.)

Then Tim Hockin wrote:
> And if you ever need to change the text that is in the format string?
> The hash changes.  That seems exactly counter to your goal...

Whether hashed or not, what's needed is premeditated reporting,
intentional dividing between variable content (like device ID)
and the explanatory text arround it.  Prefix each line with a token
(or as most are calling it, a "tag") that can be looked up off-line.

I hope no one is thinking gettext in this thread.
Don't clutter the kernel with something like gettext.

And don't burden the kernel with run-time text crunching.
Message codes go in when the module is written.
They get looked up either in user space or off-line.
Easy!  Almost trivial!

-- R;   <><

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Rusty Russell]

On Sunday 17 August 2008 06:40:50 Tim Hockin wrote:
> On Sat, Aug 16, 2008 at 10:49 AM, Martin Schwidefsky
>
> <schwidefsky@de.ibm.com> wrote:
> >> > > Can you hash the format string to generate the id?  6 hex digits
> >> > > should be enough, and your tool can check for clashes.  As it's bad
> >> > > form to have identical strings for different semantics anyway, this
> >> > > seems to make sense.
> >
> > Ok, so  a specialized version of printk will do the work to calculate
> > the hash. Only, what will we do if there ever is a conflict? The message
> > tag has to be unique. The shorter the hash is, the more likely a
> > collision gets. Don't know if 6 hash digits is enough to just ignore the
> > problem.
>
> And if you ever need to change the text that is in the format string?
> The hash changes.  That seems exactly counter to your goal...

You need to catalogue them all anyway, so you can detect clashes at build 
time.  Yes, you have to change the new string in that case, but that's easy.

And the number changing as the message changes is arguably a feature.  You 
shouldn't (and we generally don't) idly change kernel messages.

Rusty.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Sat, 2008-08-16 at 12:36 -0700, Joe Perches wrote:
> Doesn't this example better illustrate the barely useful
> user value of such a message and documentation?
> 
> In your example, monwriter.1 with a return code of 5 isn't possible.

That is true, thanks for pointing out this bug. I doesn't change
anything about the need for a message documentation though, it only
proves that the documentation can have errors. At least the customers
running linux on System z want to have the documentation.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Sat, 2008-08-16 at 13:40 -0700, Tim Hockin wrote:
> >> > > Can you hash the format string to generate the id?  6 hex digits should
> >> > > be enough, and your tool can check for clashes.  As it's bad form to have
> >> > > identical strings for different semantics anyway, this seems to make
> >> > > sense.
> 
> > Ok, so  a specialized version of printk will do the work to calculate
> > the hash. Only, what will we do if there ever is a conflict? The message
> > tag has to be unique. The shorter the hash is, the more likely a
> > collision gets. Don't know if 6 hash digits is enough to just ignore the
> > problem.
> 
> And if you ever need to change the text that is in the format string?
> The hash changes.  That seems exactly counter to your goal...

The only problem here is trivial change like typos. Then the hash
changes although the message is semantically still the same. If the
message really changes, its id should change as well. 

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

On Sun, Aug 17, 2008 at 10:28 AM, Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:
> On Sat, 2008-08-16 at 13:40 -0700, Tim Hockin wrote:
>> >> > > Can you hash the format string to generate the id?  6 hex digits should
>> >> > > be enough, and your tool can check for clashes.  As it's bad form to have
>> >> > > identical strings for different semantics anyway, this seems to make
>> >> > > sense.
>>
>> > Ok, so  a specialized version of printk will do the work to calculate
>> > the hash. Only, what will we do if there ever is a conflict? The message
>> > tag has to be unique. The shorter the hash is, the more likely a
>> > collision gets. Don't know if 6 hash digits is enough to just ignore the
>> > problem.
>>
>> And if you ever need to change the text that is in the format string?
>> The hash changes.  That seems exactly counter to your goal...
>
> The only problem here is trivial change like typos. Then the hash
> changes although the message is semantically still the same. If the
> message really changes, its id should change as well.

That's what I meant.  Never underestimate the urge to fix the
printk(), because "hey, it's JUST A PRINTK".  That's one of my
motivations - to make it no longer just a printk.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Sun, 2008-08-17 at 15:11 +1000, Rusty Russell wrote:
> > > Ok, so  a specialized version of printk will do the work to calculate
> > > the hash. Only, what will we do if there ever is a conflict? The message
> > > tag has to be unique. The shorter the hash is, the more likely a
> > > collision gets. Don't know if 6 hash digits is enough to just ignore the
> > > problem.
> >
> > And if you ever need to change the text that is in the format string?
> > The hash changes.  That seems exactly counter to your goal...
> 
> You need to catalogue them all anyway, so you can detect clashes at build 
> time.  Yes, you have to change the new string in that case, but that's easy.

Is the suggestion that in case of a clash just change the message string
slightly to avoid the clash ? 

> And the number changing as the message changes is arguably a feature.  You 
> shouldn't (and we generally don't) idly change kernel messages.

This is very true. If a message is touched most likely the message
description has to be changed as well. If the message hash then changes
the tools can warn about it. The small thing now only is the matter of
actually implementing the hash and changing the script to work on them..

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Pavel Machek]

Hi!

> > I don't think that he wants to unify all the printk's in the system. I don't
> > think that reporting all errors "in the same way as an ATA error" makes any
> > sense. That would just lead to very stupid and unnatural messages for all
> > errors that are not like "ATA errors". Annotation of existing errors is a much
> > more flexible and feasible solution to that problem.
> 
> Please don't misinterpret.  I don't want to make other errors parse
> like an ATA error, I want to make the plumbing be parallel.  I want
> one umbrella mechanism for reporting things that are more important
> than just-another-printk().
> 
> Because frankly, "parse dmesg" is a pretty crappy way to have to
> monitor your system for failures, and I am tired of explaining to
> people why we still do that.

"parse dmesg" does not work for monitoring your system for failures;
dmesg buffer can overflow.

If something fails, you should get errno returned for userspace, and
that's where you should be doing the monitoring.

So... what parts don't return enough information to userspace so that
you need to parse dmesg? Lets fix them.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Jan Kara]

On Mon 18-08-08 11:23:20, Pavel Machek wrote:
> Hi!
> 
> > > I don't think that he wants to unify all the printk's in the system. I don't
> > > think that reporting all errors "in the same way as an ATA error" makes any
> > > sense. That would just lead to very stupid and unnatural messages for all
> > > errors that are not like "ATA errors". Annotation of existing errors is a much
> > > more flexible and feasible solution to that problem.
> > 
> > Please don't misinterpret.  I don't want to make other errors parse
> > like an ATA error, I want to make the plumbing be parallel.  I want
> > one umbrella mechanism for reporting things that are more important
> > than just-another-printk().
> > 
> > Because frankly, "parse dmesg" is a pretty crappy way to have to
> > monitor your system for failures, and I am tired of explaining to
> > people why we still do that.
> 
> "parse dmesg" does not work for monitoring your system for failures;
> dmesg buffer can overflow.
> 
> If something fails, you should get errno returned for userspace, and
> that's where you should be doing the monitoring.
  Yes, but there are far to few errno's to identify the problem. Imagine
how many different causes can result in EIO. Or have you ever tried to find
out why the crappy filesystem doesn't want to mount without looking into
kernel messages - the returned errno is always the same :). So I don't
think errno is a solution...

> So... what parts don't return enough information to userspace so that
> you need to parse dmesg? Lets fix them.
  
									Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Tim Hockin]

On Mon, Aug 18, 2008 at 2:23 AM, Pavel Machek <pavel@suse.cz> wrote:
> Hi!
>
>> > I don't think that he wants to unify all the printk's in the system. I don't
>> > think that reporting all errors "in the same way as an ATA error" makes any
>> > sense. That would just lead to very stupid and unnatural messages for all
>> > errors that are not like "ATA errors". Annotation of existing errors is a much
>> > more flexible and feasible solution to that problem.
>>
>> Please don't misinterpret.  I don't want to make other errors parse
>> like an ATA error, I want to make the plumbing be parallel.  I want
>> one umbrella mechanism for reporting things that are more important
>> than just-another-printk().
>>
>> Because frankly, "parse dmesg" is a pretty crappy way to have to
>> monitor your system for failures, and I am tired of explaining to
>> people why we still do that.
>
> "parse dmesg" does not work for monitoring your system for failures;
> dmesg buffer can overflow.
>
> If something fails, you should get errno returned for userspace, and
> that's where you should be doing the monitoring.
>
> So... what parts don't return enough information to userspace so that
> you need to parse dmesg? Lets fix them.

If I get a DMA timeout on my disk, I want to know about it.  If I get
an OOM kill, I want to know about it.  Etc.

I *don't* want every application to participate in system monitoring,
and that's what it seems you're suggesting.  I want a monitoring
daemon which is notified of important system events.  We like to
report these things in various ways, including squirting them out onto
the network.

I *don't* want to run regexes against dmesg or /var/log/messages or
/var/log/kernel every N seconds, that's just a gross hack.  I really
want first-class notifications of significant events.  I don't mind
having to do parsing of events - as I said before, they can even be
loosely structured strings.  They just need to be more important than
a plain old printk(), and preferably come through a different channel.

I understand that many users will not want this level of monitoring,
and that's why it should be flexible enough to devolve into printk().
But we have thousands of systems.  I need a better view of what is
happening.

Tim

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Wed, 2008-08-13 at 14:33 +1000, Rusty Russell wrote:
> On Thursday 31 July 2008 02:56:57 Martin Schwidefsky wrote:
> > From: Michael Holzheu <holzheu@de.ibm.com>
> > From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> >
> > Introduce a new family of printk macros which prefixes each kmsg message
> > with a component name and allows to tag the printk with a message id.
> 
> Can you hash the format string to generate the id?  6 hex digits should be 
> enough, and your tool can check for clashes.  As it's bad form to have 
> identical strings for different semantics anyway, this seems to make sense.

If we go with hashes there is one more thing: kmsg(0, <string>)
The variant where we manually assign the message ids knows about the
"special" id 0. There is no documentation required for id 0 and none is
wanted. If we replace the manual ids with hashes this will get lost. You
could argue that a kmsg with id 0 is a normal printk so why not just use
printk? What is lost is the information that this printk has been found
to be not important enough to be documented.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Rusty Russell]

On Tuesday 26 August 2008 01:56:30 Martin Schwidefsky wrote:
> On Wed, 2008-08-13 at 14:33 +1000, Rusty Russell wrote:
> > Can you hash the format string to generate the id?  6 hex digits should
> > be enough, and your tool can check for clashes.  As it's bad form to have
> > identical strings for different semantics anyway, this seems to make
> > sense.
>
> If we go with hashes there is one more thing: kmsg(0, <string>)
> The variant where we manually assign the message ids knows about the
> "special" id 0. There is no documentation required for id 0 and none is
> wanted. If we replace the manual ids with hashes this will get lost. You
> could argue that a kmsg with id 0 is a normal printk so why not just use
> printk? What is lost is the information that this printk has been found
> to be not important enough to be documented.

Hmm, #define KERN_IGNORE KERN_DEBUG?

Rusty.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Tue, 2008-08-26 at 11:38 +1000, Rusty Russell wrote:
> On Tuesday 26 August 2008 01:56:30 Martin Schwidefsky wrote:
> > On Wed, 2008-08-13 at 14:33 +1000, Rusty Russell wrote:
> > > Can you hash the format string to generate the id?  6 hex digits should
> > > be enough, and your tool can check for clashes.  As it's bad form to have
> > > identical strings for different semantics anyway, this seems to make
> > > sense.
> >
> > If we go with hashes there is one more thing: kmsg(0, <string>)
> > The variant where we manually assign the message ids knows about the
> > "special" id 0. There is no documentation required for id 0 and none is
> > wanted. If we replace the manual ids with hashes this will get lost. You
> > could argue that a kmsg with id 0 is a normal printk so why not just use
> > printk? What is lost is the information that this printk has been found
> > to be not important enough to be documented.
> 
> Hmm, #define KERN_IGNORE KERN_DEBUG?

Hmm, shouldn't KERN_xxx levels be independent of the special id 0? 

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Rusty Russell]

On Monday 01 September 2008 22:28:50 Martin Schwidefsky wrote:
> On Tue, 2008-08-26 at 11:38 +1000, Rusty Russell wrote:
> > Hmm, #define KERN_IGNORE KERN_DEBUG?
>
> Hmm, shouldn't KERN_xxx levels be independent of the special id 0?

I don't know.  Why should something be untranslatable?

Rusty.

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Tue, 2008-09-02 at 23:34 +1000, Rusty Russell wrote:
> On Monday 01 September 2008 22:28:50 Martin Schwidefsky wrote:
> > On Tue, 2008-08-26 at 11:38 +1000, Rusty Russell wrote:
> > > Hmm, #define KERN_IGNORE KERN_DEBUG?
> >
> > Hmm, shouldn't KERN_xxx levels be independent of the special id 0?
> 
> I don't know.  Why should something be untranslatable?

Do we really want to require that every message printed with the kmsg
macros has to be documented? I don't think this is a good idea. The
special id 0 is there to tell the script not to bother with a particular
message, e.g. the lovely "module <x> has been loaded" messages. The
message still can be translated.

-- 
blue skies,
  Martin.

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

Re: [patch 1/3] kmsg: Kernel message catalog macros.

[Martin Schwidefsky]

On Tue, 2008-08-26 at 17:28 +0200, Utz Bacher wrote:
> Martin Schwidefsky wrote on 25.08.2008 17:56:30:
> > If we go with hashes there is one more thing: kmsg(0, <string>)
> > The variant where we manually assign the message ids knows about the
> > "special" id 0. There is no documentation required for id 0 and none is
> > wanted. If we replace the manual ids with hashes this will get lost. You
> > could argue that a kmsg with id 0 is a normal printk so why not just use
> > printk? What is lost is the information that this printk has been found
> > to be not important enough to be documented. 
> 
> what about a kmsg(<string>) and a kmsg_not_for_doc(<string>) with only kmsg 
> being hashed and used for man pages. There might be better names for these 
> functions, though. 

That would double the number of macros we'd have to define. Not really
something I would like to do. Only if there is no other way ..

-- 
blue skies,
  Martin.

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