linux-kernel - Re: [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4E2616E1.8000808@gmail.com>
Date:	Wed, 20 Jul 2011 09:44:33 +1000
From:	Ryan Mallon <rmallon@...il.com>
To:	Ravishankar <ravishankarkm32@...il.com>
CC:	gregkh@...e.de, wfp5p@...ginia.edu, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org,
	Ravishankar <ravi.shankar@...enturtles.in>
Subject: Re: [PATCH 1/2 v3] Staging: comedi: fix printk() issue in adv_pci1710.c

On 19/07/11 16:07, Ryan Mallon wrote:
> On 19/07/11 15:50, Ravishankar wrote:
>> From: Ravishankar<ravi.shankar@...enturtles.in>
>>
>> This is a patch to the adv_pci1710.c file that fixes up a printk() 
>> warning found by the checkpatch.pl tool
>>
>> Signed-off-by: Ravishankar<ravishankarkm32@...il.com>
>> ---
>> KERN_CONT issue is fixed
>>
>>   drivers/staging/comedi/drivers/adv_pci1710.c |   20 
>> ++++++++++----------
>>   1 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/adv_pci1710.c 
>> b/drivers/staging/comedi/drivers/adv_pci1710.c
>> index fd71cc6..093b9e6 100644
>> --- a/drivers/staging/comedi/drivers/adv_pci1710.c
>> +++ b/drivers/staging/comedi/drivers/adv_pci1710.c
>> @@ -1396,14 +1396,14 @@ static int pci1710_attach(struct 
>> comedi_device *dev,
>>       int i;
>>       int board_index;
>>
>> -    printk("comedi%d: adv_pci1710: ", dev->minor);
>> +    printk(KERN_INFO "comedi%d: adv_pci1710: ", dev->minor);
>>
>>       opt_bus = it->options[0];
>>       opt_slot = it->options[1];
>>
>>       ret = alloc_private(dev, sizeof(struct pci1710_private));
>>       if (ret<  0) {
>> -        printk(" - Allocation failed!\n");
>> +        printk(KERN_CONT "\n");
>> +        printk(KERN_ERR "Comedi%d: adv_pci1710: Allocation failed\n",
>> +        dev->minor);
>
> This still isn't correct. The initial printk (KERN_INFO above) has no 
> trailing newline. Here you are almost doing it correctly, except that 
> you are adding an extra newline. This error message will look like this:
>
> comedi0: adv_pci1710:
> comedi0: adv_pci1710: Allocation failed
>
> Which is just silly. The printk's below are broken in that you are 
> using KERN_ERR where you should be using KERN_CONT.
>
> As it stands the code is a little tricky to follow since the trailing 
> newline gets added at the end unless there is an error when it gets 
> added in place. It might be better just to print the full message, 
> including the "comedi%d: adv_pci1710:" bit, at each printk call site 
> and get rid of the whole KERN_CONT nonsense altogether. Alternatively 
> you could create a comedi_printk function (or use pr_fmt) which wraps 
> this up.

In fact, looking at the code again, the printks should probably just be 
replaced with the dev_printk functions which should print the necessary 
device name information.

~Ryan

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