* [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
@ 2006-02-15 2:21 Matthew Martin
2006-02-15 3:04 ` Greg KH
` (16 more replies)
0 siblings, 17 replies; 18+ messages in thread
From: Matthew Martin @ 2006-02-15 2:21 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 702 bytes --]
Hello,
Gcc 4.0.2 had the warning:
drivers/usb/media/ov511.c: In function 'show_exposure':
drivers/usb/media/ov511.c:5642: warning: 'exp' may be used uninitialized
in this function
Here is the patch to fix that warning.
Signed-off-by: Matthew Martin <lihnucks@gmail.com>
---
--- orig-linux-2.6.15/drivers/usb/media/ov511.c 2006-02-14 15:15:10.000000000 -0600
+++ linux-2.6.15/drivers/usb/media/ov511.c 2006-02-14 15:12:32.000000000 -0600
@@ -5639,7 +5639,7 @@ static CLASS_DEVICE_ATTR(hue, S_IRUGO, s
static ssize_t show_exposure(struct class_device *cd, char *buf)
{
struct usb_ov511 *ov = cd_to_ov(cd);
- unsigned char exp;
+ unsigned char exp = '\0';
if (!ov->dev)
return -ENODEV;
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
@ 2006-02-15 3:04 ` Greg KH
2006-02-15 16:30 ` Nico Golde
` (15 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2006-02-15 3:04 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 824 bytes --]
On Tue, Feb 14, 2006 at 08:21:12PM -0600, Matthew Martin wrote:
>
> Hello,
> Gcc 4.0.2 had the warning:
>
> drivers/usb/media/ov511.c: In function 'show_exposure':
> drivers/usb/media/ov511.c:5642: warning: 'exp' may be used uninitialized
> in this function
>
> Here is the patch to fix that warning.
>
> Signed-off-by: Matthew Martin <lihnucks@gmail.com>
>
> ---
>
> --- orig-linux-2.6.15/drivers/usb/media/ov511.c 2006-02-14 15:15:10.000000000 -0600
> +++ linux-2.6.15/drivers/usb/media/ov511.c 2006-02-14 15:12:32.000000000 -0600
> @@ -5639,7 +5639,7 @@ static CLASS_DEVICE_ATTR(hue, S_IRUGO, s
> static ssize_t show_exposure(struct class_device *cd, char *buf)
> {
> struct usb_ov511 *ov = cd_to_ov(cd);
> - unsigned char exp;
> + unsigned char exp = '\0';
What's wrong with just "0" here?
thanks,
greg k-h
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
2006-02-15 3:04 ` Greg KH
@ 2006-02-15 16:30 ` Nico Golde
2006-02-15 17:05 ` Greg KH
` (14 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Nico Golde @ 2006-02-15 16:30 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1.1: Type: text/plain, Size: 534 bytes --]
Hallo Greg,
* Greg KH <greg@kroah.com> [2006-02-15 11:14]:
> On Tue, Feb 14, 2006 at 08:21:12PM -0600, Matthew Martin wrote:
[...]
> > struct usb_ov511 *ov = cd_to_ov(cd);
> > - unsigned char exp;
> > + unsigned char exp = '\0';
>
> What's wrong with just "0" here?
Nothing, doesnt matter.
Regards Nico
--
Nico Golde - JAB: nion@jabber.ccc.de | GPG: 0x73647CFF
http://www.ngolde.de | http://www.muttng.org | http://grml.org
Forget about that mouse with 3/4/5 buttons -
gimme a keyboard with 103/104/105 keys!
[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
2006-02-15 3:04 ` Greg KH
2006-02-15 16:30 ` Nico Golde
@ 2006-02-15 17:05 ` Greg KH
2006-02-15 21:41 ` Matthew Martin
` (13 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2006-02-15 17:05 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 443 bytes --]
On Wed, Feb 15, 2006 at 05:30:21PM +0100, Nico Golde wrote:
> Hallo Greg,
>
> * Greg KH <greg@kroah.com> [2006-02-15 11:14]:
> > On Tue, Feb 14, 2006 at 08:21:12PM -0600, Matthew Martin wrote:
> [...]
> > > struct usb_ov511 *ov = cd_to_ov(cd);
> > > - unsigned char exp;
> > > + unsigned char exp = '\0';
> >
> > What's wrong with just "0" here?
>
> Nothing, doesnt matter.
Great, care to redo the patch then with 0?
thanks,
greg k-h
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (2 preceding siblings ...)
2006-02-15 17:05 ` Greg KH
@ 2006-02-15 21:41 ` Matthew Martin
2006-02-15 23:01 ` Nishanth Aravamudan
` (12 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Matthew Martin @ 2006-02-15 21:41 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 703 bytes --]
Hello,
Gcc 4.0.2 had the warning:
drivers/usb/media/ov511.c: In function 'show_exposure':
drivers/usb/media/ov511.c:5642: warning: 'exp' may be used uninitialized
in this function
Here is the patch to fix that warning.
Signed-off-by: Matthew Martin <lihnucks at gmail.com>
---
--- orig-linux-2.6.15/drivers/usb/media/ov511.c 2003-12-03 10:03:52.000000000 -0500
+++ linux-2.6.15/drivers/usb/media/ov511.c 2003-12-03 02:40:51.000000000 -0500
@@ -5639,7 +5639,7 @@ static CLASS_DEVICE_ATTR(hue, S_IRUGO, s
static ssize_t show_exposure(struct class_device *cd, char *buf)
{
struct usb_ov511 *ov = cd_to_ov(cd);
- unsigned char exp;
+ unsigned char exp = 0;
if (!ov->dev)
return -ENODEV;
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (3 preceding siblings ...)
2006-02-15 21:41 ` Matthew Martin
@ 2006-02-15 23:01 ` Nishanth Aravamudan
2006-02-15 23:16 ` Håkon Løvdal
` (11 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Nishanth Aravamudan @ 2006-02-15 23:01 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 396 bytes --]
On 15.02.2006 [15:41:25 -0600], Matthew Martin wrote:
>
> Hello,
> Gcc 4.0.2 had the warning:
>
> drivers/usb/media/ov511.c: In function 'show_exposure':
> drivers/usb/media/ov511.c:5642: warning: 'exp' may be used uninitialized
> in this function
>
> Here is the patch to fix that warning.
>
> Signed-off-by: Matthew Martin <lihnucks at gmail.com>
Real e-mail address please.
Thanks,
Nish
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (4 preceding siblings ...)
2006-02-15 23:01 ` Nishanth Aravamudan
@ 2006-02-15 23:16 ` Håkon Løvdal
2006-02-15 23:27 ` Greg KH
` (10 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Håkon Løvdal @ 2006-02-15 23:16 UTC (permalink / raw)
To: kernel-janitors
On 2/15/06, Greg KH <greg@kroah.com> wrote:
> On Tue, Feb 14, 2006 at 08:21:12PM -0600, Matthew Martin wrote:
> > - unsigned char exp;
> > + unsigned char exp = '\0';
>
> What's wrong with just "0" here?
In my opinion '\0' is the correct representation here.
'\0' ASCII nul character
0 arithmetic value zero
NULL null pointer
All of those _could_ be represented with just "0", however using 0 as
such a multipurpose character/value/pointer constant is not a good idea
because it makes the code less clear to read.
Linus has been very clear on that NULL and only NULL should be used for
pointers, and there was a large set of patches converting from 0 to NULL
in 2004.
How is '\0' vs 0 different?
BR Håkon Løvdal
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (5 preceding siblings ...)
2006-02-15 23:16 ` Håkon Løvdal
@ 2006-02-15 23:27 ` Greg KH
2006-02-15 23:31 ` Randy.Dunlap
` (9 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2006-02-15 23:27 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 597 bytes --]
On Thu, Feb 16, 2006 at 12:16:35AM +0100, H?kon L?vdal wrote:
> On 2/15/06, Greg KH <greg@kroah.com> wrote:
> > On Tue, Feb 14, 2006 at 08:21:12PM -0600, Matthew Martin wrote:
> > > - unsigned char exp;
> > > + unsigned char exp = '\0';
> >
> > What's wrong with just "0" here?
>
> In my opinion '\0' is the correct representation here.
>
> '\0' ASCII nul character
> 0 arithmetic value zero
> NULL null pointer
If you look at how this variable is used, it's not used as an ASCII
value, but rather as a arithmetic value.
That is why I said it should be 0, not '\0'.
thanks,
greg k-h
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (6 preceding siblings ...)
2006-02-15 23:27 ` Greg KH
@ 2006-02-15 23:31 ` Randy.Dunlap
2006-02-15 23:37 ` Håkon Løvdal
` (8 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Randy.Dunlap @ 2006-02-15 23:31 UTC (permalink / raw)
To: kernel-janitors
On Thu, 16 Feb 2006, Håkon Løvdal wrote:
> On 2/15/06, Greg KH <greg@kroah.com> wrote:
> > On Tue, Feb 14, 2006 at 08:21:12PM -0600, Matthew Martin wrote:
> > > - unsigned char exp;
> > > + unsigned char exp = '\0';
> >
> > What's wrong with just "0" here?
>
> In my opinion '\0' is the correct representation here.
>
> '\0' ASCII nul character
> 0 arithmetic value zero
> NULL null pointer
>
> All of those _could_ be represented with just "0", however using 0 as
> such a multipurpose character/value/pointer constant is not a good idea
> because it makes the code less clear to read.
>
> Linus has been very clear on that NULL and only NULL should be used for
> pointers, and there was a large set of patches converting from 0 to NULL
> in 2004.
>
> How is '\0' vs 0 different?
I didn't see any problem with using '\0' either, as long as it's
meant to be a character instead of an 8-bit integer.
Looking at ov511.c, it appears to be an 8-bit integer...
so 0 makes more sense to me in this case.
I find its usage more interesting. Did the patch also address
this part? What does this do?
sensor_get_exposure(ov, &exp);
return sprintf(buf, "%d\n", exp >> 8);
Does that always print 0? (for exp >> 8)
--
~Randy
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (7 preceding siblings ...)
2006-02-15 23:31 ` Randy.Dunlap
@ 2006-02-15 23:37 ` Håkon Løvdal
2006-02-16 0:38 ` Greg KH
` (7 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Håkon Løvdal @ 2006-02-15 23:37 UTC (permalink / raw)
To: kernel-janitors
On 2/16/06, Greg KH <greg@kroah.com> wrote:
> If you look at how this variable is used, it's not used as an ASCII
> value, but rather as a arithmetic value.
>
> That is why I said it should be 0, not '\0'.
Ah, I see. I must confess that I only looked at the patch itself and
when '\0' was used I assumed that this was because the variable was
used as a character later on. When this is not the case I have not done
a good enough review. Good that someone else is :)
BR Håkon Løvdal
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (8 preceding siblings ...)
2006-02-15 23:37 ` Håkon Løvdal
@ 2006-02-16 0:38 ` Greg KH
2006-02-16 1:49 ` Matthew Martin
` (6 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2006-02-16 0:38 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]
On Wed, Feb 15, 2006 at 03:31:27PM -0800, Randy.Dunlap wrote:
> On Thu, 16 Feb 2006, H?kon L?vdal wrote:
>
> > On 2/15/06, Greg KH <greg@kroah.com> wrote:
> > > On Tue, Feb 14, 2006 at 08:21:12PM -0600, Matthew Martin wrote:
> > > > - unsigned char exp;
> > > > + unsigned char exp = '\0';
> > >
> > > What's wrong with just "0" here?
> >
> > In my opinion '\0' is the correct representation here.
> >
> > '\0' ASCII nul character
> > 0 arithmetic value zero
> > NULL null pointer
> >
> > All of those _could_ be represented with just "0", however using 0 as
> > such a multipurpose character/value/pointer constant is not a good idea
> > because it makes the code less clear to read.
> >
> > Linus has been very clear on that NULL and only NULL should be used for
> > pointers, and there was a large set of patches converting from 0 to NULL
> > in 2004.
> >
> > How is '\0' vs 0 different?
>
> I didn't see any problem with using '\0' either, as long as it's
> meant to be a character instead of an 8-bit integer.
>
> Looking at ov511.c, it appears to be an 8-bit integer...
> so 0 makes more sense to me in this case.
>
> I find its usage more interesting. Did the patch also address
> this part? What does this do?
>
> sensor_get_exposure(ov, &exp);
> return sprintf(buf, "%d\n", exp >> 8);
>
> Does that always print 0? (for exp >> 8)
Heh, it should :)
Anyone have this device to test this with?
thanks,
greg k-h
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (9 preceding siblings ...)
2006-02-16 0:38 ` Greg KH
@ 2006-02-16 1:49 ` Matthew Martin
2006-02-16 14:51 ` Matthew Martin
` (5 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Matthew Martin @ 2006-02-16 1:49 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 266 bytes --]
> I find its usage more interesting. Did the patch also address
> this part? What does this do?
>
> sensor_get_exposure(ov, &exp);
> return sprintf(buf, "%d\n", exp >> 8);
>
> Does that always print 0? (for exp >> 8)
>
No, the patch did not address that.
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (10 preceding siblings ...)
2006-02-16 1:49 ` Matthew Martin
@ 2006-02-16 14:51 ` Matthew Martin
2006-02-16 15:43 ` Nishanth Aravamudan
` (4 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Matthew Martin @ 2006-02-16 14:51 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 147 bytes --]
On Wed, 2006-02-15 at 15:01 -0800, Nishanth Aravamudan wrote:
> Real e-mail address please.
>
> Thanks,
> Nish
That is my real e-mail address.
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (11 preceding siblings ...)
2006-02-16 14:51 ` Matthew Martin
@ 2006-02-16 15:43 ` Nishanth Aravamudan
2006-02-16 17:20 ` Randy.Dunlap
` (3 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Nishanth Aravamudan @ 2006-02-16 15:43 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 445 bytes --]
On 16.02.2006 [08:51:05 -0600], Matthew Martin wrote:
> On Wed, 2006-02-15 at 15:01 -0800, Nishanth Aravamudan wrote:
>
> > Real e-mail address please.
> >
> > Thanks,
> > Nish
>
> That is my real e-mail address.
You snipped the part that matters, but:
> > Signed-off-by: Matthew Martin <lihnucks at gmail.com>
s/\ at\ /\@/
You did it fine with th resent patch, I think. Just wanted to make sure
this one was unintentional.
Thanks,
Nish
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (12 preceding siblings ...)
2006-02-16 15:43 ` Nishanth Aravamudan
@ 2006-02-16 17:20 ` Randy.Dunlap
2006-02-17 21:53 ` Alexey Dobriyan
` (2 subsequent siblings)
16 siblings, 0 replies; 18+ messages in thread
From: Randy.Dunlap @ 2006-02-16 17:20 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: TEXT/PLAIN, Size: 367 bytes --]
On Thu, 16 Feb 2006, Matthew Martin wrote:
> On Wed, 2006-02-15 at 15:01 -0800, Nishanth Aravamudan wrote:
>
> > Real e-mail address please.
> >
> > Thanks,
> > Nish
>
> That is my real e-mail address.
email addresses are of the form addy@domain.tld
while yours was listed as: addy at domain.tld
Just a small difference, that's all. No obfuscation.
--
~Randy
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (13 preceding siblings ...)
2006-02-16 17:20 ` Randy.Dunlap
@ 2006-02-17 21:53 ` Alexey Dobriyan
2006-02-18 11:48 ` walter harms
2006-02-18 16:28 ` Greg KH
16 siblings, 0 replies; 18+ messages in thread
From: Alexey Dobriyan @ 2006-02-17 21:53 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2671 bytes --]
[jumps from train]
Gentlemen, instead of broke-arounding non-issues, I suggest to
actually read sensor_get_exposure(). You'll notice quite a few strange
things for 1.5-screenful function, I promise:
drivers/usb/media/ov511.c:
1856 static int
1857 sensor_get_exposure(struct usb_ov511 *ov, unsigned char *val)
1858 {
1859 int rc;
1860
1861 switch (ov->sensor) {
1862 case SEN_OV7610:
1863 case SEN_OV6620:
1864 case SEN_OV6630:
1865 case SEN_OV7620:
1866 case SEN_OV76BE:
1867 case SEN_OV8600:
1868 rc = i2c_r(ov, 0x10);
1869 if (rc < 0)
1870 return rc;
Oops, it can return error value. So, instead checking return value of
sensor_get_exposure(), you tell gcc to STFU. Notice that *val is not
touched on this path.
1871 else
1872 *val = rc;
It's touched here, but the function will return 0.
1873 break;
1874 case SEN_KS0127:
1875 case SEN_KS0127B:
1876 case SEN_SAA7111A:
1877 val = NULL;
Now, who can explain this line? Change of "val" will be forgotten right
after "return".
1878 PDEBUG(3, "Unsupported with this sensor");
1879 return -EPERM;
This looks slightly bizarre. -EPERM occurs usually when Joe User tries
to do something not under root.
1880 default:
1881 err("Sensor not supported for get_exposure");
1882 return -EINVAL;
"Invalid argument". So there should be valid argument, right? Wrong.
"Sensor not supported".
1883 }
1884
1885 PDEBUG(3, "%d", *val);
1886 ov->exposure = *val;
1887
1888 return 0;
1889 }
As was noted, "unsigned char >> 8" somewhere near line 5396 is pretty
funny too.
Resume:
Patch is rejected because it's very wrong, fixes problem in the
wrong place and only makes whole situation worse.
Future "may be used uninitialized" patches must contain
description of code path that can lead to said uninitialized using.
Fed my pet project to 3.4.4:
-Wall -- no warnings
-Wall -O -- out little friends and I can prove they are bogus.
-Wall -O[123s] -- ditto
Alexey "warnings by commitee" Dobriyan
On Wed, Feb 15, 2006 at 03:41:25PM -0600, Matthew Martin wrote:
> Gcc 4.0.2 had the warning:
> drivers/usb/media/ov511.c: In function 'show_exposure':
> drivers/usb/media/ov511.c:5642: warning: 'exp' may be used uninitialized
> in this function
> --- orig-linux-2.6.15/drivers/usb/media/ov511.c
> +++ linux-2.6.15/drivers/usb/media/ov511.c
> @@ -5639,7 +5639,7 @@ static CLASS_DEVICE_ATTR(hue, S_IRUGO, s
> static ssize_t show_exposure(struct class_device *cd, char *buf)
> {
> struct usb_ov511 *ov = cd_to_ov(cd);
> - unsigned char exp;
> + unsigned char exp = 0;
>
> if (!ov->dev)
> return -ENODEV;
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (14 preceding siblings ...)
2006-02-17 21:53 ` Alexey Dobriyan
@ 2006-02-18 11:48 ` walter harms
2006-02-18 16:28 ` Greg KH
16 siblings, 0 replies; 18+ messages in thread
From: walter harms @ 2006-02-18 11:48 UTC (permalink / raw)
To: kernel-janitors
hi alexey,
several usb drivers return -EPERM. perhaps
gkh will accept a cleanup patch for this issue.
re,
walter
Alexey Dobriyan wrote:
> [jumps from train]
>
> Gentlemen, instead of broke-arounding non-issues, I suggest to
> actually read sensor_get_exposure(). You'll notice quite a few strange
> things for 1.5-screenful function, I promise:
>
> drivers/usb/media/ov511.c:
> 1856 static int
> 1857 sensor_get_exposure(struct usb_ov511 *ov, unsigned char *val)
> 1858 {
> 1859 int rc;
> 1860
> 1861 switch (ov->sensor) {
> 1862 case SEN_OV7610:
> 1863 case SEN_OV6620:
> 1864 case SEN_OV6630:
> 1865 case SEN_OV7620:
> 1866 case SEN_OV76BE:
> 1867 case SEN_OV8600:
> 1868 rc = i2c_r(ov, 0x10);
> 1869 if (rc < 0)
> 1870 return rc;
>
> Oops, it can return error value. So, instead checking return value of
> sensor_get_exposure(), you tell gcc to STFU. Notice that *val is not
> touched on this path.
>
> 1871 else
> 1872 *val = rc;
>
> It's touched here, but the function will return 0.
>
> 1873 break;
> 1874 case SEN_KS0127:
> 1875 case SEN_KS0127B:
> 1876 case SEN_SAA7111A:
> 1877 val = NULL;
>
> Now, who can explain this line? Change of "val" will be forgotten right
> after "return".
>
> 1878 PDEBUG(3, "Unsupported with this sensor");
> 1879 return -EPERM;
>
> This looks slightly bizarre. -EPERM occurs usually when Joe User tries
> to do something not under root.
>
> 1880 default:
> 1881 err("Sensor not supported for get_exposure");
> 1882 return -EINVAL;
>
> "Invalid argument". So there should be valid argument, right? Wrong.
> "Sensor not supported".
>
> 1883 }
> 1884
> 1885 PDEBUG(3, "%d", *val);
> 1886 ov->exposure = *val;
> 1887
> 1888 return 0;
> 1889 }
>
> As was noted, "unsigned char >> 8" somewhere near line 5396 is pretty
> funny too.
>
> Resume:
> Patch is rejected because it's very wrong, fixes problem in the
> wrong place and only makes whole situation worse.
>
> Future "may be used uninitialized" patches must contain
> description of code path that can lead to said uninitialized using.
>
> Fed my pet project to 3.4.4:
> -Wall -- no warnings
> -Wall -O -- out little friends and I can prove they are bogus.
> -Wall -O[123s] -- ditto
>
> Alexey "warnings by commitee" Dobriyan
>
> On Wed, Feb 15, 2006 at 03:41:25PM -0600, Matthew Martin wrote:
>> Gcc 4.0.2 had the warning:
>
>> drivers/usb/media/ov511.c: In function 'show_exposure':
>> drivers/usb/media/ov511.c:5642: warning: 'exp' may be used uninitialized
>> in this function
>
>> --- orig-linux-2.6.15/drivers/usb/media/ov511.c
>> +++ linux-2.6.15/drivers/usb/media/ov511.c
>> @@ -5639,7 +5639,7 @@ static CLASS_DEVICE_ATTR(hue, S_IRUGO, s
>> static ssize_t show_exposure(struct class_device *cd, char *buf)
>> {
>> struct usb_ov511 *ov = cd_to_ov(cd);
>> - unsigned char exp;
>> + unsigned char exp = 0;
>>
>> if (!ov->dev)
>> return -ENODEV;
>
>
> ------------------------------------------------------------------------
>
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
` (15 preceding siblings ...)
2006-02-18 11:48 ` walter harms
@ 2006-02-18 16:28 ` Greg KH
16 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2006-02-18 16:28 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 211 bytes --]
On Sat, Feb 18, 2006 at 12:48:57PM +0100, walter harms wrote:
> hi alexey,
> several usb drivers return -EPERM. perhaps
> gkh will accept a cleanup patch for this issue.
I will gladly do so.
thanks,
greg k-h
[-- Attachment #2: Type: text/plain, Size: 168 bytes --]
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2006-02-18 16:28 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-02-15 2:21 [KJ] [PATCH] Fix warning in drivers/usb/media/ov511.c Matthew Martin
2006-02-15 3:04 ` Greg KH
2006-02-15 16:30 ` Nico Golde
2006-02-15 17:05 ` Greg KH
2006-02-15 21:41 ` Matthew Martin
2006-02-15 23:01 ` Nishanth Aravamudan
2006-02-15 23:16 ` Håkon Løvdal
2006-02-15 23:27 ` Greg KH
2006-02-15 23:31 ` Randy.Dunlap
2006-02-15 23:37 ` Håkon Løvdal
2006-02-16 0:38 ` Greg KH
2006-02-16 1:49 ` Matthew Martin
2006-02-16 14:51 ` Matthew Martin
2006-02-16 15:43 ` Nishanth Aravamudan
2006-02-16 17:20 ` Randy.Dunlap
2006-02-17 21:53 ` Alexey Dobriyan
2006-02-18 11:48 ` walter harms
2006-02-18 16:28 ` Greg KH
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.