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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/