Re: [PATCH 3/5] Add the "verbose" module option.

[Jean Delvare]

On Thu, 07 Aug 2014 18:29:23 +0200, Goffredo Baroncelli wrote:
> On 08/07/2014 10:52 AM, Jean Delvare wrote:
> > Le Wednesday 06 August 2014 à 21:05 +0000, Goffredo Baroncelli a écrit :
> >> +	 */
> >> +	tempchanged = x.temp != temp || x.casetemp != casetemp;
> >> +	if ((verbose > 1 && tempchanged) ||
> >> +	    (verbose > 0 && level >= 0)) {
> >> +		printk(KERN_INFO);
> >> +		print_temp("CPU-temp: ", temp);
> > 
> > This can be written more efficiently as a single statement:
> > 
> > 		print_temp(KERN_INFO "CPU-temp: ", temp);
> 
> I suppose that KERN_* has to be in the beginning of the line. 

Correct.

> Because a single line is composed by several prink,

In this case, it is, but FYI, this is generally discouraged. The reason
is that another piece of the kernel may be calling printk at the same
time, and then that other message may split your own message into
pieces. If you run checkpatch.pl on this file, you'll see it complains
about this.

> KERN_INFO has 
> to be only in the first printk. To me it seems more polite to have
> one printk for the level, and the others (there are more than one) 
> for the message parts.

The fewer printks is better. Ideally there would be only one to avoid
the risk of line splitting altogether. I understand this isn't easy to
achieve in this case, but I still believe that you shouldn't have more
calls to printk than necessary, to reduce the risk.

-- 
Jean Delvare
SUSE L3 Support
--
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/