All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.