linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
@ 2012-11-27 11:48 Vitalii Demianets
  2012-11-27 12:24 ` Vitalii Demianets
  2012-11-27 22:43 ` Hans J. Koch
  0 siblings, 2 replies; 24+ messages in thread
From: Vitalii Demianets @ 2012-11-27 11:48 UTC (permalink / raw)
  To: linux-kernel; +Cc: Hans J. Koch, Greg Kroah-Hartman

Fix warning: 'ret' might be used uninitialized

Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
---
 drivers/uio/uio.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..c33fd18 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -280,12 +280,16 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 			map_found = 1;
 			idev->map_dir = kobject_create_and_add("maps",
 							&idev->dev->kobj);
-			if (!idev->map_dir)
+			if (!idev->map_dir) {
+				ret = -ENOMEM;
 				goto err_map;
+			}
 		}
 		map = kzalloc(sizeof(*map), GFP_KERNEL);
-		if (!map)
+		if (!map) {
+			ret = -ENOMEM;
 			goto err_map;
+		}
 		kobject_init(&map->kobj, &map_attr_type);
 		map->mem = mem;
 		mem->map = map;
@@ -305,12 +309,16 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 			portio_found = 1;
 			idev->portio_dir = kobject_create_and_add("portio",
 							&idev->dev->kobj);
-			if (!idev->portio_dir)
+			if (!idev->portio_dir) {
+				ret = -ENOMEM;
 				goto err_portio;
+			}
 		}
 		portio = kzalloc(sizeof(*portio), GFP_KERNEL);
-		if (!portio)
+		if (!portio) {
+			ret = -ENOMEM;
 			goto err_portio;
+		}
 		kobject_init(&portio->kobj, &portio_attr_type);
 		portio->port = port;
 		port->portio = portio;
-- 
1.7.8.6

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-27 11:48 [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized Vitalii Demianets
@ 2012-11-27 12:24 ` Vitalii Demianets
  2012-11-27 22:43 ` Hans J. Koch
  1 sibling, 0 replies; 24+ messages in thread
From: Vitalii Demianets @ 2012-11-27 12:24 UTC (permalink / raw)
  To: linux-kernel; +Cc: Hans J. Koch, Greg Kroah-Hartman

By the way, I've found that warning while working on the older kernel with 
older gcc (ver. 3.4.4). The modern gcc (ver. 4.5.4) does not emit that 
warning no matter how hard I try. For example, it does not warn even with the 
following line in the drivers/uio/Makefile:

  ccflags-$(CONFIG_UIO) += -O2 -Wall -Wextra -Wuninitialized

In that case the actual command line looks like (from "make V=1" output):

gcc -Wp,-MD,drivers/uio/.uio.o.d  -nostdinc -isystem /usr/lib/gcc/x86_64-pc-linux-gnu/4.5.4/include -I/home/vitas/Progs/net-next/net-next/arch/x86/include -Iarch/x86/include/generated  -Iinclude -I/home/vitas/Progs/net-next/net-next/arch/x86/include/uapi -Iarch/x86/include/generated/uapi -I/home/vitas/Progs/net-next/net-next/include/uapi -Iinclude/generated/uapi -include /home/vitas/Progs/net-next/net-next/include/linux/kconfig.h -D__KERNEL__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -Wno-format-security -fno-delete-null-pointer-checks -O2 -m64 -mtune=generic -mno-red-zone -mcmodel=kernel -funit-at-a-time -maccumulate-outgoing-args -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -DCONFIG_AS_CFI_SECTIONS=1 -DCONFIG_AS_FXSAVEQ=1 -DCONFIG_AS_AVX=1 -pipe -Wno-sign-compare -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -mno-avx -Wframe-larger-than=2048 -fno-stack-protector -fno-omit-frame-pointer -fno-optimize-sibling-calls -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow -fconserve-stack -DCC_HAVE_ASM_GOTO -O2 -Wall -Wextra -Wuninitialized    -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(uio)"  -D"KBUILD_MODNAME=KBUILD_STR(uio)" -c -o 
drivers/uio/uio.o drivers/uio/uio.c

I wander why doesn't modern gcc warn about obvious use of uninitialized 
variable. Is it some known regression in gcc?

-- 
With Best Regards,
Vitalii Demianets

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-27 11:48 [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized Vitalii Demianets
  2012-11-27 12:24 ` Vitalii Demianets
@ 2012-11-27 22:43 ` Hans J. Koch
  2012-11-28  8:58   ` Vitalii Demianets
  1 sibling, 1 reply; 24+ messages in thread
From: Hans J. Koch @ 2012-11-27 22:43 UTC (permalink / raw)
  To: Vitalii Demianets; +Cc: linux-kernel, Hans J. Koch, Greg Kroah-Hartman

On Tue, Nov 27, 2012 at 01:48:14PM +0200, Vitalii Demianets wrote:
> Fix warning: 'ret' might be used uninitialized
> 
> Signed-off-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
> ---
>  drivers/uio/uio.c |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 5110f36..c33fd18 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -280,12 +280,16 @@ static int uio_dev_add_attributes(struct uio_device *idev)
>  			map_found = 1;
>  			idev->map_dir = kobject_create_and_add("maps",
>  							&idev->dev->kobj);
> -			if (!idev->map_dir)
> +			if (!idev->map_dir) {
> +				ret = -ENOMEM;
>  				goto err_map;
> +			}
>  		}
>  		map = kzalloc(sizeof(*map), GFP_KERNEL);
> -		if (!map)
> +		if (!map) {
> +			ret = -ENOMEM;
>  			goto err_map;
> +		}
>  		kobject_init(&map->kobj, &map_attr_type);
>  		map->mem = mem;
>  		mem->map = map;
> @@ -305,12 +309,16 @@ static int uio_dev_add_attributes(struct uio_device *idev)
>  			portio_found = 1;
>  			idev->portio_dir = kobject_create_and_add("portio",
>  							&idev->dev->kobj);
> -			if (!idev->portio_dir)
> +			if (!idev->portio_dir) {
> +				ret = -ENOMEM;
>  				goto err_portio;
> +			}
>  		}
>  		portio = kzalloc(sizeof(*portio), GFP_KERNEL);
> -		if (!portio)
> +		if (!portio) {
> +			ret = -ENOMEM;
>  			goto err_portio;
> +		}
>  		kobject_init(&portio->kobj, &portio_attr_type);
>  		portio->port = port;
>  		port->portio = portio;
> -- 
> 1.7.8.6
> 

Thanks, good catch, but why don't you simply do this:


>From 228445996bb75a44d16b6237eca6a0916d9b2d7e Mon Sep 17 00:00:00 2001
From: "Hans J. Koch" <hjk@hansjkoch.de>
Date: Tue, 27 Nov 2012 23:38:00 +0100
Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized

In two cases, the return value variable "ret" can be undefined.

Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
---
 drivers/uio/uio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..fc60e35 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -263,7 +263,7 @@ static struct class uio_class = {
  */
 static int uio_dev_add_attributes(struct uio_device *idev)
 {
-	int ret;
+	int ret = -ENOMEM;
 	int mi, pi;
 	int map_found = 0;
 	int portio_found = 0;
-- 
1.7.9


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-27 22:43 ` Hans J. Koch
@ 2012-11-28  8:58   ` Vitalii Demianets
  2012-11-28 21:05     ` Hans J. Koch
  0 siblings, 1 reply; 24+ messages in thread
From: Vitalii Demianets @ 2012-11-28  8:58 UTC (permalink / raw)
  To: Hans J. Koch; +Cc: linux-kernel, Greg Kroah-Hartman

On Wednesday 28 November 2012 00:43:41 Hans J. Koch wrote:
> 
> Thanks, good catch, but why don't you simply do this:
> 

Just a matter of personal preference. As a maintainer you can apply either 
patch you want. I guess you would prefer your approach and I have no 
objections to that :)

> 
> >From 228445996bb75a44d16b6237eca6a0916d9b2d7e Mon Sep 17 00:00:00 2001
> From: "Hans J. Koch" <hjk@hansjkoch.de>
> Date: Tue, 27 Nov 2012 23:38:00 +0100
> Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
> 
> In two cases, the return value variable "ret" can be undefined.
> 
> Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
> ---
>  drivers/uio/uio.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 5110f36..fc60e35 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -263,7 +263,7 @@ static struct class uio_class = {
>   */
>  static int uio_dev_add_attributes(struct uio_device *idev)
>  {
> -	int ret;
> +	int ret = -ENOMEM;
>  	int mi, pi;
>  	int map_found = 0;
>  	int portio_found = 0;



-- 
With Best Regards,
Vitalii Demianets


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-28  8:58   ` Vitalii Demianets
@ 2012-11-28 21:05     ` Hans J. Koch
  2012-11-29 16:05       ` Tux9
  0 siblings, 1 reply; 24+ messages in thread
From: Hans J. Koch @ 2012-11-28 21:05 UTC (permalink / raw)
  To: Vitalii Demianets; +Cc: Hans J. Koch, linux-kernel, Greg Kroah-Hartman

On Wed, Nov 28, 2012 at 10:58:32AM +0200, Vitalii Demianets wrote:
> On Wednesday 28 November 2012 00:43:41 Hans J. Koch wrote:
> > 
> > Thanks, good catch, but why don't you simply do this:
> > 
> 
> Just a matter of personal preference.

Your patch: 1 files changed, 12 insertions(+), 4 deletions(-)
My patch: 1 files changed, 1 insertions(+), 1 deletions(-)

Both achieve exactly the same. That's not a matter of personal
preference, that's the difference between a working solution and
a good solution. In the kernel, we want the latter.

> As a maintainer you can apply either 
> patch you want. I guess you would prefer your approach and I have no 
> objections to that :)

That's not the right kind of comment. Don't make it a habit.

Thanks,
Hans

> 
> > 
> > >From 228445996bb75a44d16b6237eca6a0916d9b2d7e Mon Sep 17 00:00:00 2001
> > From: "Hans J. Koch" <hjk@hansjkoch.de>
> > Date: Tue, 27 Nov 2012 23:38:00 +0100
> > Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
> > 
> > In two cases, the return value variable "ret" can be undefined.
> > 
> > Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
> > ---
> >  drivers/uio/uio.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> > index 5110f36..fc60e35 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -263,7 +263,7 @@ static struct class uio_class = {
> >   */
> >  static int uio_dev_add_attributes(struct uio_device *idev)
> >  {
> > -	int ret;
> > +	int ret = -ENOMEM;
> >  	int mi, pi;
> >  	int map_found = 0;
> >  	int portio_found = 0;
> 
> 
> 
> -- 
> With Best Regards,
> Vitalii Demianets
> 
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-28 21:05     ` Hans J. Koch
@ 2012-11-29 16:05       ` Tux9
  2012-11-29 16:22         ` Vitalii Demianets
  0 siblings, 1 reply; 24+ messages in thread
From: Tux9 @ 2012-11-29 16:05 UTC (permalink / raw)
  To: Hans J. Koch; +Cc: Vitalii Demianets, linux-kernel, Greg Kroah-Hartman

Hans, I think there are something wrong in your patch, while Vitalii's
is right. The variable "ret" is reused in line 292 and line 295, so
the value of "ret" would be overridden (if it goto err_map in line 284
when mi>=1).

Best,
Cong

On Wed, Nov 28, 2012 at 10:05 PM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> On Wed, Nov 28, 2012 at 10:58:32AM +0200, Vitalii Demianets wrote:
>> On Wednesday 28 November 2012 00:43:41 Hans J. Koch wrote:
>> >
>> > Thanks, good catch, but why don't you simply do this:
>> >
>>
>> Just a matter of personal preference.
>
> Your patch: 1 files changed, 12 insertions(+), 4 deletions(-)
> My patch: 1 files changed, 1 insertions(+), 1 deletions(-)
>
> Both achieve exactly the same. That's not a matter of personal
> preference, that's the difference between a working solution and
> a good solution. In the kernel, we want the latter.
>
>> As a maintainer you can apply either
>> patch you want. I guess you would prefer your approach and I have no
>> objections to that :)
>
> That's not the right kind of comment. Don't make it a habit.
>
> Thanks,
> Hans
>
>>
>> >
>> > >From 228445996bb75a44d16b6237eca6a0916d9b2d7e Mon Sep 17 00:00:00 2001
>> > From: "Hans J. Koch" <hjk@hansjkoch.de>
>> > Date: Tue, 27 Nov 2012 23:38:00 +0100
>> > Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
>> >
>> > In two cases, the return value variable "ret" can be undefined.
>> >
>> > Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
>> > ---
>> >  drivers/uio/uio.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> > index 5110f36..fc60e35 100644
>> > --- a/drivers/uio/uio.c
>> > +++ b/drivers/uio/uio.c
>> > @@ -263,7 +263,7 @@ static struct class uio_class = {
>> >   */
>> >  static int uio_dev_add_attributes(struct uio_device *idev)
>> >  {
>> > -   int ret;
>> > +   int ret = -ENOMEM;
>> >     int mi, pi;
>> >     int map_found = 0;
>> >     int portio_found = 0;
>>
>>
>>
>> --
>> With Best Regards,
>> Vitalii Demianets
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-29 16:05       ` Tux9
@ 2012-11-29 16:22         ` Vitalii Demianets
  2012-11-29 16:33           ` Cong Ding
  2012-11-29 16:36           ` Vitalii Demianets
  0 siblings, 2 replies; 24+ messages in thread
From: Vitalii Demianets @ 2012-11-29 16:22 UTC (permalink / raw)
  To: Tux9; +Cc: Hans J. Koch, linux-kernel, Greg Kroah-Hartman

On Thursday 29 November 2012 18:05:27 Tux9 wrote:
> Hans, I think there are something wrong in your patch, while Vitalii's
> is right. The variable "ret" is reused in line 292 and line 295, so
> the value of "ret" would be overridden (if it goto err_map in line 284
> when mi>=1).
> 

Actually, both patches do exactly the same thing. Hans's patch establishes 
default value for the ret for all those "other" cases when ret is not 
explicitly overridden. My patch explicitly enumerates all those "other" cases 
in more wordily manner.

> 
> On Wed, Nov 28, 2012 at 10:05 PM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> > On Wed, Nov 28, 2012 at 10:58:32AM +0200, Vitalii Demianets wrote:
> >> On Wednesday 28 November 2012 00:43:41 Hans J. Koch wrote:
> >> >
> >> > Thanks, good catch, but why don't you simply do this:
> >> >
> >>
> >> Just a matter of personal preference.
> >
> > Your patch: 1 files changed, 12 insertions(+), 4 deletions(-)
> > My patch: 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > Both achieve exactly the same. That's not a matter of personal
> > preference, that's the difference between a working solution and
> > a good solution. In the kernel, we want the latter.
> >
> >> As a maintainer you can apply either
> >> patch you want. I guess you would prefer your approach and I have no
> >> objections to that :)
> >
> > That's not the right kind of comment. Don't make it a habit.
> >
> > Thanks,
> > Hans
> >
> >>
> >> >
> >> > >From 228445996bb75a44d16b6237eca6a0916d9b2d7e Mon Sep 17 00:00:00 2001
> >> > From: "Hans J. Koch" <hjk@hansjkoch.de>
> >> > Date: Tue, 27 Nov 2012 23:38:00 +0100
> >> > Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
> >> >
> >> > In two cases, the return value variable "ret" can be undefined.
> >> >
> >> > Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
> >> > ---
> >> >  drivers/uio/uio.c |    2 +-
> >> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> >> > index 5110f36..fc60e35 100644
> >> > --- a/drivers/uio/uio.c
> >> > +++ b/drivers/uio/uio.c
> >> > @@ -263,7 +263,7 @@ static struct class uio_class = {
> >> >   */
> >> >  static int uio_dev_add_attributes(struct uio_device *idev)
> >> >  {
> >> > -   int ret;
> >> > +   int ret = -ENOMEM;
> >> >     int mi, pi;
> >> >     int map_found = 0;
> >> >     int portio_found = 0;

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-29 16:22         ` Vitalii Demianets
@ 2012-11-29 16:33           ` Cong Ding
  2012-11-29 16:36           ` Vitalii Demianets
  1 sibling, 0 replies; 24+ messages in thread
From: Cong Ding @ 2012-11-29 16:33 UTC (permalink / raw)
  To: Vitalii Demianets; +Cc: Hans J. Koch, linux-kernel, Greg Kroah-Hartman

No, there are some exceptions. Imagine this case, when mi=0,
everything works correct in the loop, and then mi=1, if the kzalloc in
line 286 fails, you patch will goto err_map with ret=-ENOMEM, while
Hans's patch will goto err_map with ret=0 (the ret=0 is from line 295
when mi=0).

On Thu, Nov 29, 2012 at 5:22 PM, Vitalii Demianets
<vitas@nppfactor.kiev.ua> wrote:
> Actually, both patches do exactly the same thing. Hans's patch establishes
> default value for the ret for all those "other" cases when ret is not
> explicitly overridden. My patch explicitly enumerates all those "other" cases
> in more wordily manner.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-29 16:22         ` Vitalii Demianets
  2012-11-29 16:33           ` Cong Ding
@ 2012-11-29 16:36           ` Vitalii Demianets
  2012-11-29 23:58             ` Hans J. Koch
  1 sibling, 1 reply; 24+ messages in thread
From: Vitalii Demianets @ 2012-11-29 16:36 UTC (permalink / raw)
  To: Tux9; +Cc: Hans J. Koch, linux-kernel, Greg Kroah-Hartman

> On Thursday 29 November 2012 18:05:27 Tux9 wrote:
> > Hans, I think there are something wrong in your patch, while Vitalii's
> > is right. The variable "ret" is reused in line 292 and line 295, so
> > the value of "ret" would be overridden (if it goto err_map in line 284
> > when mi>=1).
>
> Actually, both patches do exactly the same thing. Hans's patch establishes
> default value for the ret for all those "other" cases when ret is not
> explicitly overridden. My patch explicitly enumerates all those "other"
> cases in more wordily manner.
>

Oops, disregard this. After looking at it more thoroughly I got your point.
You are right, ret is overridden at first iteration (mi == 0), so Hans's 
approach does not work.
I must do more thinking before replying in a hurry.

> > On Wed, Nov 28, 2012 at 10:05 PM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> > > On Wed, Nov 28, 2012 at 10:58:32AM +0200, Vitalii Demianets wrote:
> > >> On Wednesday 28 November 2012 00:43:41 Hans J. Koch wrote:
> > >> > Thanks, good catch, but why don't you simply do this:
> > >>
> > >> Just a matter of personal preference.
> > >
> > > Your patch: 1 files changed, 12 insertions(+), 4 deletions(-)
> > > My patch: 1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > Both achieve exactly the same. That's not a matter of personal
> > > preference, that's the difference between a working solution and
> > > a good solution. In the kernel, we want the latter.
> > >
> > >> As a maintainer you can apply either
> > >> patch you want. I guess you would prefer your approach and I have no
> > >> objections to that :)
> > >
> > > That's not the right kind of comment. Don't make it a habit.
> > >
> > > Thanks,
> > > Hans
> > >
> > >> > >From 228445996bb75a44d16b6237eca6a0916d9b2d7e Mon Sep 17 00:00:00
> > >> > > 2001
> > >> >
> > >> > From: "Hans J. Koch" <hjk@hansjkoch.de>
> > >> > Date: Tue, 27 Nov 2012 23:38:00 +0100
> > >> > Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
> > >> >
> > >> > In two cases, the return value variable "ret" can be undefined.
> > >> >
> > >> > Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
> > >> > ---
> > >> >  drivers/uio/uio.c |    2 +-
> > >> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > >> >
> > >> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> > >> > index 5110f36..fc60e35 100644
> > >> > --- a/drivers/uio/uio.c
> > >> > +++ b/drivers/uio/uio.c
> > >> > @@ -263,7 +263,7 @@ static struct class uio_class = {
> > >> >   */
> > >> >  static int uio_dev_add_attributes(struct uio_device *idev)
> > >> >  {
> > >> > -   int ret;
> > >> > +   int ret = -ENOMEM;
> > >> >     int mi, pi;
> > >> >     int map_found = 0;
> > >> >     int portio_found = 0;

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-29 16:36           ` Vitalii Demianets
@ 2012-11-29 23:58             ` Hans J. Koch
  2012-11-30 11:12               ` Tux9
  2012-11-30 11:16               ` Vitalii Demianets
  0 siblings, 2 replies; 24+ messages in thread
From: Hans J. Koch @ 2012-11-29 23:58 UTC (permalink / raw)
  To: Vitalii Demianets; +Cc: Tux9, Hans J. Koch, linux-kernel, Greg Kroah-Hartman

On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
> > On Thursday 29 November 2012 18:05:27 Tux9 wrote:
> > > Hans, I think there are something wrong in your patch, while Vitalii's
> > > is right. The variable "ret" is reused in line 292 and line 295, so
> > > the value of "ret" would be overridden (if it goto err_map in line 284
> > > when mi>=1).
> >
> > Actually, both patches do exactly the same thing. Hans's patch establishes
> > default value for the ret for all those "other" cases when ret is not
> > explicitly overridden. My patch explicitly enumerates all those "other"
> > cases in more wordily manner.
> >
> 
> Oops, disregard this. After looking at it more thoroughly I got your point.
> You are right, ret is overridden at first iteration (mi == 0), so Hans's 
> approach does not work.
> I must do more thinking before replying in a hurry.

You're right. Initialization of "ret" has to take place at the beginning of
the loop.

I think this version is right:


>From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
From: "Hans J. Koch" <hjk@hansjkoch.de>
Date: Fri, 30 Nov 2012 00:51:50 +0100
Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized

In two cases, the return value variable "ret" can be undefined.

Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
---
 drivers/uio/uio.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..0c80df2 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 	struct uio_portio *portio;
 
 	for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
+		ret = -ENOMEM;
 		mem = &idev->info->mem[mi];
 		if (mem->size == 0)
 			break;
@@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 	}
 
 	for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
+		ret = -ENOMEM;
 		port = &idev->info->port[pi];
 		if (port->size == 0)
 			break;
-- 
1.7.9


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-29 23:58             ` Hans J. Koch
@ 2012-11-30 11:12               ` Tux9
  2012-11-30 21:33                 ` Hans J. Koch
  2012-11-30 11:16               ` Vitalii Demianets
  1 sibling, 1 reply; 24+ messages in thread
From: Tux9 @ 2012-11-30 11:12 UTC (permalink / raw)
  To: Hans J. Koch; +Cc: Vitalii Demianets, linux-kernel, Greg Kroah-Hartman

I like Vitalii's solution more. Hans's solution assign the value
-ENOMEM to ret in every round of the loop, which is a kind of wasting
CPU cycles.

On Fri, Nov 30, 2012 at 12:58 AM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
>> > On Thursday 29 November 2012 18:05:27 Tux9 wrote:
>> > > Hans, I think there are something wrong in your patch, while Vitalii's
>> > > is right. The variable "ret" is reused in line 292 and line 295, so
>> > > the value of "ret" would be overridden (if it goto err_map in line 284
>> > > when mi>=1).
>> >
>> > Actually, both patches do exactly the same thing. Hans's patch establishes
>> > default value for the ret for all those "other" cases when ret is not
>> > explicitly overridden. My patch explicitly enumerates all those "other"
>> > cases in more wordily manner.
>> >
>>
>> Oops, disregard this. After looking at it more thoroughly I got your point.
>> You are right, ret is overridden at first iteration (mi == 0), so Hans's
>> approach does not work.
>> I must do more thinking before replying in a hurry.
>
> You're right. Initialization of "ret" has to take place at the beginning of
> the loop.
>
> I think this version is right:
>
>
> From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
> From: "Hans J. Koch" <hjk@hansjkoch.de>
> Date: Fri, 30 Nov 2012 00:51:50 +0100
> Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
>
> In two cases, the return value variable "ret" can be undefined.
>
> Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
> ---
>  drivers/uio/uio.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 5110f36..0c80df2 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
>         struct uio_portio *portio;
>
>         for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
> +               ret = -ENOMEM;
>                 mem = &idev->info->mem[mi];
>                 if (mem->size == 0)
>                         break;
> @@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
>         }
>
>         for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
> +               ret = -ENOMEM;
>                 port = &idev->info->port[pi];
>                 if (port->size == 0)
>                         break;
> --
> 1.7.9
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-29 23:58             ` Hans J. Koch
  2012-11-30 11:12               ` Tux9
@ 2012-11-30 11:16               ` Vitalii Demianets
  2012-11-30 21:39                 ` Hans J. Koch
  1 sibling, 1 reply; 24+ messages in thread
From: Vitalii Demianets @ 2012-11-30 11:16 UTC (permalink / raw)
  To: Hans J. Koch; +Cc: Tux9, linux-kernel, Greg Kroah-Hartman

On Friday 30 November 2012 01:58:22 Hans J. Koch wrote:
> On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
> > > On Thursday 29 November 2012 18:05:27 Tux9 wrote:
> > > > Hans, I think there are something wrong in your patch, while
> > > > Vitalii's is right. The variable "ret" is reused in line 292 and line
> > > > 295, so the value of "ret" would be overridden (if it goto err_map in
> > > > line 284 when mi>=1).
> > >
> > > Actually, both patches do exactly the same thing. Hans's patch
> > > establishes default value for the ret for all those "other" cases when
> > > ret is not explicitly overridden. My patch explicitly enumerates all
> > > those "other" cases in more wordily manner.
> >
> > Oops, disregard this. After looking at it more thoroughly I got your
> > point. You are right, ret is overridden at first iteration (mi == 0), so
> > Hans's approach does not work.
> > I must do more thinking before replying in a hurry.
>
> You're right. Initialization of "ret" has to take place at the beginning of
> the loop.
>
> I think this version is right:

Yes, this looks right for me.

> >From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
>
> From: "Hans J. Koch" <hjk@hansjkoch.de>
> Date: Fri, 30 Nov 2012 00:51:50 +0100
> Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
>
> In two cases, the return value variable "ret" can be undefined.
>
> Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
> ---
>  drivers/uio/uio.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 5110f36..0c80df2 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device
> *idev) struct uio_portio *portio;
>
>  	for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
> +		ret = -ENOMEM;
>  		mem = &idev->info->mem[mi];
>  		if (mem->size == 0)
>  			break;
> @@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device
> *idev) }
>
>  	for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
> +		ret = -ENOMEM;
>  		port = &idev->info->port[pi];
>  		if (port->size == 0)
>  			break;

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-30 11:12               ` Tux9
@ 2012-11-30 21:33                 ` Hans J. Koch
  2012-12-01  1:22                   ` Cong Ding
  0 siblings, 1 reply; 24+ messages in thread
From: Hans J. Koch @ 2012-11-30 21:33 UTC (permalink / raw)
  To: Tux9; +Cc: Hans J. Koch, Vitalii Demianets, linux-kernel, Greg Kroah-Hartman

On Fri, Nov 30, 2012 at 12:12:46PM +0100, Tux9 wrote:
> I like Vitalii's solution more. Hans's solution assign the value
> -ENOMEM to ret in every round of the loop, which is a kind of wasting
> CPU cycles.

The difference between
1 files changed, 12 insertions(+), 4 deletions(-) and
1 files changed, 2 insertions(+), 0 deletions(-)

is more important. Note that this code is not in a fast path but only
called once at device creation.

Thanks,
Hans

> 
> On Fri, Nov 30, 2012 at 12:58 AM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> > On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
> >> > On Thursday 29 November 2012 18:05:27 Tux9 wrote:
> >> > > Hans, I think there are something wrong in your patch, while Vitalii's
> >> > > is right. The variable "ret" is reused in line 292 and line 295, so
> >> > > the value of "ret" would be overridden (if it goto err_map in line 284
> >> > > when mi>=1).
> >> >
> >> > Actually, both patches do exactly the same thing. Hans's patch establishes
> >> > default value for the ret for all those "other" cases when ret is not
> >> > explicitly overridden. My patch explicitly enumerates all those "other"
> >> > cases in more wordily manner.
> >> >
> >>
> >> Oops, disregard this. After looking at it more thoroughly I got your point.
> >> You are right, ret is overridden at first iteration (mi == 0), so Hans's
> >> approach does not work.
> >> I must do more thinking before replying in a hurry.
> >
> > You're right. Initialization of "ret" has to take place at the beginning of
> > the loop.
> >
> > I think this version is right:
> >
> >
> > From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
> > From: "Hans J. Koch" <hjk@hansjkoch.de>
> > Date: Fri, 30 Nov 2012 00:51:50 +0100
> > Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
> >
> > In two cases, the return value variable "ret" can be undefined.
> >
> > Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
> > ---
> >  drivers/uio/uio.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> > index 5110f36..0c80df2 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
> >         struct uio_portio *portio;
> >
> >         for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
> > +               ret = -ENOMEM;
> >                 mem = &idev->info->mem[mi];
> >                 if (mem->size == 0)
> >                         break;
> > @@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
> >         }
> >
> >         for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
> > +               ret = -ENOMEM;
> >                 port = &idev->info->port[pi];
> >                 if (port->size == 0)
> >                         break;
> > --
> > 1.7.9
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-30 11:16               ` Vitalii Demianets
@ 2012-11-30 21:39                 ` Hans J. Koch
  2012-11-30 23:43                   ` Greg Kroah-Hartman
  2012-12-03  8:35                   ` [PATCH] uio.c: " Vitalii Demianets
  0 siblings, 2 replies; 24+ messages in thread
From: Hans J. Koch @ 2012-11-30 21:39 UTC (permalink / raw)
  To: Vitalii Demianets; +Cc: Hans J. Koch, Tux9, linux-kernel, Greg Kroah-Hartman

On Fri, Nov 30, 2012 at 01:16:19PM +0200, Vitalii Demianets wrote:
> On Friday 30 November 2012 01:58:22 Hans J. Koch wrote:
> > On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
> > > > On Thursday 29 November 2012 18:05:27 Tux9 wrote:
> > > > > Hans, I think there are something wrong in your patch, while
> > > > > Vitalii's is right. The variable "ret" is reused in line 292 and line
> > > > > 295, so the value of "ret" would be overridden (if it goto err_map in
> > > > > line 284 when mi>=1).
> > > >
> > > > Actually, both patches do exactly the same thing. Hans's patch
> > > > establishes default value for the ret for all those "other" cases when
> > > > ret is not explicitly overridden. My patch explicitly enumerates all
> > > > those "other" cases in more wordily manner.
> > >
> > > Oops, disregard this. After looking at it more thoroughly I got your
> > > point. You are right, ret is overridden at first iteration (mi == 0), so
> > > Hans's approach does not work.
> > > I must do more thinking before replying in a hurry.
> >
> > You're right. Initialization of "ret" has to take place at the beginning of
> > the loop.
> >
> > I think this version is right:
> 
> Yes, this looks right for me.

OK, I'll send that patch offically, then. This might also be material for
the stable updates. Greg?

Thanks a lot for reporting and discussing that problem. I'll add a

Reported-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>

if you have no objections.

Thanks,
Hans

> 
> > >From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
> >
> > From: "Hans J. Koch" <hjk@hansjkoch.de>
> > Date: Fri, 30 Nov 2012 00:51:50 +0100
> > Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
> >
> > In two cases, the return value variable "ret" can be undefined.
> >
> > Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
> > ---
> >  drivers/uio/uio.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> > index 5110f36..0c80df2 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device
> > *idev) struct uio_portio *portio;
> >
> >  	for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
> > +		ret = -ENOMEM;
> >  		mem = &idev->info->mem[mi];
> >  		if (mem->size == 0)
> >  			break;
> > @@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device
> > *idev) }
> >
> >  	for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
> > +		ret = -ENOMEM;
> >  		port = &idev->info->port[pi];
> >  		if (port->size == 0)
> >  			break;
> 

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-30 21:39                 ` Hans J. Koch
@ 2012-11-30 23:43                   ` Greg Kroah-Hartman
  2012-12-03  7:41                     ` [PATCH] stable: uio: " Hans J. Koch
  2012-12-03  8:35                   ` [PATCH] uio.c: " Vitalii Demianets
  1 sibling, 1 reply; 24+ messages in thread
From: Greg Kroah-Hartman @ 2012-11-30 23:43 UTC (permalink / raw)
  To: Hans J. Koch; +Cc: Vitalii Demianets, Tux9, linux-kernel

On Fri, Nov 30, 2012 at 10:39:06PM +0100, Hans J. Koch wrote:
> On Fri, Nov 30, 2012 at 01:16:19PM +0200, Vitalii Demianets wrote:
> > On Friday 30 November 2012 01:58:22 Hans J. Koch wrote:
> > > On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
> > > > > On Thursday 29 November 2012 18:05:27 Tux9 wrote:
> > > > > > Hans, I think there are something wrong in your patch, while
> > > > > > Vitalii's is right. The variable "ret" is reused in line 292 and line
> > > > > > 295, so the value of "ret" would be overridden (if it goto err_map in
> > > > > > line 284 when mi>=1).
> > > > >
> > > > > Actually, both patches do exactly the same thing. Hans's patch
> > > > > establishes default value for the ret for all those "other" cases when
> > > > > ret is not explicitly overridden. My patch explicitly enumerates all
> > > > > those "other" cases in more wordily manner.
> > > >
> > > > Oops, disregard this. After looking at it more thoroughly I got your
> > > > point. You are right, ret is overridden at first iteration (mi == 0), so
> > > > Hans's approach does not work.
> > > > I must do more thinking before replying in a hurry.
> > >
> > > You're right. Initialization of "ret" has to take place at the beginning of
> > > the loop.
> > >
> > > I think this version is right:
> > 
> > Yes, this looks right for me.
> 
> OK, I'll send that patch offically, then. This might also be material for
> the stable updates. Greg?

Yes, that sounds good.

greg k-h

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-30 21:33                 ` Hans J. Koch
@ 2012-12-01  1:22                   ` Cong Ding
  2012-12-01  3:56                     ` Hans J. Koch
  0 siblings, 1 reply; 24+ messages in thread
From: Cong Ding @ 2012-12-01  1:22 UTC (permalink / raw)
  To: Hans J. Koch; +Cc: Vitalii Demianets, linux-kernel, Greg Kroah-Hartman

On Fri, Nov 30, 2012 at 10:33 PM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> On Fri, Nov 30, 2012 at 12:12:46PM +0100, Tux9 wrote:
>> I like Vitalii's solution more. Hans's solution assign the value
>> -ENOMEM to ret in every round of the loop, which is a kind of wasting
>> CPU cycles.
>
> The difference between
> 1 files changed, 12 insertions(+), 4 deletions(-) and
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> is more important. Note that this code is not in a fast path but only
> called once at device creation.
Why do you think the size of the patch is so important? I think the
most important thing is the coding style and efficiency. I agree
efficiency is not important in this case, but what about coding style?
Your code is _not_ very easy to understand. It's a very natural thing
to set the return value and then goto the error-handling codes, which
is exactly same as what Vitalii did, rather than setting an initial
value in the beginning of each round of the loop as you did. There are
also a bunch of codes in kernel in the same style with Vitalii, but I
cannot find anything same as your codes.

Cong
>
>>
>> On Fri, Nov 30, 2012 at 12:58 AM, Hans J. Koch <hjk@hansjkoch.de> wrote:
>> > On Thu, Nov 29, 2012 at 06:36:59PM +0200, Vitalii Demianets wrote:
>> >> > On Thursday 29 November 2012 18:05:27 Tux9 wrote:
>> >> > > Hans, I think there are something wrong in your patch, while Vitalii's
>> >> > > is right. The variable "ret" is reused in line 292 and line 295, so
>> >> > > the value of "ret" would be overridden (if it goto err_map in line 284
>> >> > > when mi>=1).
>> >> >
>> >> > Actually, both patches do exactly the same thing. Hans's patch establishes
>> >> > default value for the ret for all those "other" cases when ret is not
>> >> > explicitly overridden. My patch explicitly enumerates all those "other"
>> >> > cases in more wordily manner.
>> >> >
>> >>
>> >> Oops, disregard this. After looking at it more thoroughly I got your point.
>> >> You are right, ret is overridden at first iteration (mi == 0), so Hans's
>> >> approach does not work.
>> >> I must do more thinking before replying in a hurry.
>> >
>> > You're right. Initialization of "ret" has to take place at the beginning of
>> > the loop.
>> >
>> > I think this version is right:
>> >
>> >
>> > From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
>> > From: "Hans J. Koch" <hjk@hansjkoch.de>
>> > Date: Fri, 30 Nov 2012 00:51:50 +0100
>> > Subject: [PATCH] uio: Fix warning: 'ret' might be used uninitialized
>> >
>> > In two cases, the return value variable "ret" can be undefined.
>> >
>> > Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
>> > ---
>> >  drivers/uio/uio.c |    2 ++
>> >  1 files changed, 2 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>> > index 5110f36..0c80df2 100644
>> > --- a/drivers/uio/uio.c
>> > +++ b/drivers/uio/uio.c
>> > @@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
>> >         struct uio_portio *portio;
>> >
>> >         for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
>> > +               ret = -ENOMEM;
>> >                 mem = &idev->info->mem[mi];
>> >                 if (mem->size == 0)
>> >                         break;
>> > @@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
>> >         }
>> >
>> >         for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
>> > +               ret = -ENOMEM;
>> >                 port = &idev->info->port[pi];
>> >                 if (port->size == 0)
>> >                         break;
>> > --
>> > 1.7.9
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-12-01  1:22                   ` Cong Ding
@ 2012-12-01  3:56                     ` Hans J. Koch
  2012-12-01  9:58                       ` Cong Ding
  0 siblings, 1 reply; 24+ messages in thread
From: Hans J. Koch @ 2012-12-01  3:56 UTC (permalink / raw)
  To: Cong Ding
  Cc: Hans J. Koch, Vitalii Demianets, linux-kernel, Greg Kroah-Hartman

On Sat, Dec 01, 2012 at 02:22:44AM +0100, Cong Ding wrote:
> On Fri, Nov 30, 2012 at 10:33 PM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> > On Fri, Nov 30, 2012 at 12:12:46PM +0100, Tux9 wrote:
> >> I like Vitalii's solution more. Hans's solution assign the value
> >> -ENOMEM to ret in every round of the loop, which is a kind of wasting
> >> CPU cycles.
> >
> > The difference between
> > 1 files changed, 12 insertions(+), 4 deletions(-) and
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > is more important. Note that this code is not in a fast path but only
> > called once at device creation.
> Why do you think the size of the patch is so important? I think the
> most important thing is the coding style and efficiency. I agree
> efficiency is not important in this case, but what about coding style?

Most important. Short is beautiful.

> Your code is _not_ very easy to understand.

You think so?

> It's a very natural thing
> to set the return value and then goto the error-handling codes, which
> is exactly same as what Vitalii did, rather than setting an initial
> value in the beginning of each round of the loop as you did.

Setting a default value at the beginning of a block is the most natural
thing. I don't want to repeat the same code in three places.

> There are
> also a bunch of codes in kernel in the same style with Vitalii, but I
> cannot find anything same as your codes.

If you follow LKML closely, you'll notice that simplifying code by
replacing unnecessary repetitions with shorter versions is always welcome.
If we didn't go for that, the kernel source would be a few million lines
bigger by now.

Thanks,
Hans

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-12-01  3:56                     ` Hans J. Koch
@ 2012-12-01  9:58                       ` Cong Ding
  2012-12-03  7:51                         ` Hans J. Koch
  0 siblings, 1 reply; 24+ messages in thread
From: Cong Ding @ 2012-12-01  9:58 UTC (permalink / raw)
  To: Hans J. Koch; +Cc: Vitalii Demianets, linux-kernel, Greg Kroah-Hartman

On Sat, Dec 01, 2012 at 04:56:25AM +0100, Hans J. Koch wrote:
> On Sat, Dec 01, 2012 at 02:22:44AM +0100, Cong Ding wrote:
> > On Fri, Nov 30, 2012 at 10:33 PM, Hans J. Koch <hjk@hansjkoch.de> wrote:
> > > On Fri, Nov 30, 2012 at 12:12:46PM +0100, Tux9 wrote:
> > >> I like Vitalii's solution more. Hans's solution assign the value
> > >> -ENOMEM to ret in every round of the loop, which is a kind of wasting
> > >> CPU cycles.
> > >
> > > The difference between
> > > 1 files changed, 12 insertions(+), 4 deletions(-) and
> > > 1 files changed, 2 insertions(+), 0 deletions(-)
> > >
> > > is more important. Note that this code is not in a fast path but only
> > > called once at device creation.
> > Why do you think the size of the patch is so important? I think the
> > most important thing is the coding style and efficiency. I agree
> > efficiency is not important in this case, but what about coding style?
> 
> Most important. Short is beautiful.
> 
> > Your code is _not_ very easy to understand.
> 
> You think so?
> 
> > It's a very natural thing
> > to set the return value and then goto the error-handling codes, which
> > is exactly same as what Vitalii did, rather than setting an initial
> > value in the beginning of each round of the loop as you did.
> 
> Setting a default value at the beginning of a block is the most natural
> thing. I don't want to repeat the same code in three places.
> 
> > There are
> > also a bunch of codes in kernel in the same style with Vitalii, but I
> > cannot find anything same as your codes.
> 
> If you follow LKML closely, you'll notice that simplifying code by
> replacing unnecessary repetitions with shorter versions is always welcome.
> If we didn't go for that, the kernel source would be a few million lines
> bigger by now.
> 
> Thanks,
> Hans
If it is really necessary to save the 4 lines of codes, I would suggest to do 
in the following style. But you are more senior than me, so I may be wrong in 
this aspect. 

 drivers/uio/uio.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..cb20168 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -263,7 +263,7 @@ static struct class uio_class = {
  */
 static int uio_dev_add_attributes(struct uio_device *idev)
 {
-	int ret;
+	int ret = 0;
 	int mi, pi;
 	int map_found = 0;
 	int portio_found = 0;
@@ -339,6 +339,8 @@ err_map:
 		kobject_put(&map->kobj);
 	}
 	kobject_put(idev->map_dir);
+	if (!ret)
+		ret = -ENOMEM;
 	dev_err(idev->dev, "error creating sysfs files (%d)\n", ret);
 	return ret;
 }


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH] stable: uio: Fix warning: 'ret' might be used uninitialized
  2012-11-30 23:43                   ` Greg Kroah-Hartman
@ 2012-12-03  7:41                     ` Hans J. Koch
  2012-12-03 13:38                       ` Ben Hutchings
  0 siblings, 1 reply; 24+ messages in thread
From: Hans J. Koch @ 2012-12-03  7:41 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Hans J. Koch, Vitalii Demianets, Tux9, linux-kernel, stable

In two cases, the return value variable "ret" can be undefined.

Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
Reported-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>

---
 drivers/uio/uio.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index 5110f36..0c80df2 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -273,6 +273,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 	struct uio_portio *portio;
 
 	for (mi = 0; mi < MAX_UIO_MAPS; mi++) {
+		ret = -ENOMEM;
 		mem = &idev->info->mem[mi];
 		if (mem->size == 0)
 			break;
@@ -298,6 +299,7 @@ static int uio_dev_add_attributes(struct uio_device *idev)
 	}
 
 	for (pi = 0; pi < MAX_UIO_PORT_REGIONS; pi++) {
+		ret = -ENOMEM;
 		port = &idev->info->port[pi];
 		if (port->size == 0)
 			break;
-- 
1.7.9


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-12-01  9:58                       ` Cong Ding
@ 2012-12-03  7:51                         ` Hans J. Koch
  0 siblings, 0 replies; 24+ messages in thread
From: Hans J. Koch @ 2012-12-03  7:51 UTC (permalink / raw)
  To: Hans J. Koch, Vitalii Demianets, linux-kernel, Greg Kroah-Hartman

On Sat, Dec 01, 2012 at 09:58:32AM +0000, Cong Ding wrote:
> If it is really necessary to save the 4 lines of codes, I would suggest to do 
> in the following style. But you are more senior than me, so I may be wrong in 
> this aspect. 

"Seniority" (whatever you mean by that) has got nothing to do with it.
I'll accept any child's patch if it is good. Discussing code on LKML is
a purely technically thing, only if two patches are technically equivalent,
the maintainer's personal taste will decide. And if it makes you feel better:
My patches (and those of any other developer) are also sometimes rejected
because they are not to some other maintainer's taste.

Thanks,
Hans

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-11-30 21:39                 ` Hans J. Koch
  2012-11-30 23:43                   ` Greg Kroah-Hartman
@ 2012-12-03  8:35                   ` Vitalii Demianets
  2012-12-03  8:53                     ` Vitalii Demianets
  1 sibling, 1 reply; 24+ messages in thread
From: Vitalii Demianets @ 2012-12-03  8:35 UTC (permalink / raw)
  To: Hans J. Koch; +Cc: Tux9, linux-kernel, Greg Kroah-Hartman

On Friday 30 November 2012 23:39:06 Hans J. Koch wrote:
>
> Thanks a lot for reporting and discussing that problem. I'll add a
>
> Reported-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
>
> if you have no objections.
>

No objections. Thanks, Hans.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-12-03  8:35                   ` [PATCH] uio.c: " Vitalii Demianets
@ 2012-12-03  8:53                     ` Vitalii Demianets
  2012-12-03 17:27                       ` Hans J. Koch
  0 siblings, 1 reply; 24+ messages in thread
From: Vitalii Demianets @ 2012-12-03  8:53 UTC (permalink / raw)
  To: Hans J. Koch; +Cc: Tux9, linux-kernel, Greg Kroah-Hartman

> On Friday 30 November 2012 23:39:06 Hans J. Koch wrote:
> > Thanks a lot for reporting and discussing that problem. I'll add a
> >
> > Reported-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
> >
> > if you have no objections.
>
> No objections. Thanks, Hans.

By the way, what do you think about my revised version of uio_pdrv_genirq.c 
patch from November 29th?

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] stable: uio: Fix warning: 'ret' might be used uninitialized
  2012-12-03  7:41                     ` [PATCH] stable: uio: " Hans J. Koch
@ 2012-12-03 13:38                       ` Ben Hutchings
  0 siblings, 0 replies; 24+ messages in thread
From: Ben Hutchings @ 2012-12-03 13:38 UTC (permalink / raw)
  To: Hans J. Koch
  Cc: Greg Kroah-Hartman, Vitalii Demianets, Tux9, linux-kernel, stable

[-- Attachment #1: Type: text/plain, Size: 458 bytes --]

On Mon, 2012-12-03 at 08:41 +0100, Hans J. Koch wrote:
> In two cases, the return value variable "ret" can be undefined.
> 
> Signed-off-by: Hans J. Koch <hjk@hansjkoch.de>
> Reported-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
[...]

This is not the correct way to submit patches to stable.  See
Documentation/stable_kernel_rules.txt

Ben.

-- 
Ben Hutchings
Always try to do things in chronological order;
it's less confusing that way.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized
  2012-12-03  8:53                     ` Vitalii Demianets
@ 2012-12-03 17:27                       ` Hans J. Koch
  0 siblings, 0 replies; 24+ messages in thread
From: Hans J. Koch @ 2012-12-03 17:27 UTC (permalink / raw)
  To: Vitalii Demianets; +Cc: Hans J. Koch, Tux9, linux-kernel, Greg Kroah-Hartman

On Mon, Dec 03, 2012 at 10:53:45AM +0200, Vitalii Demianets wrote:
> > On Friday 30 November 2012 23:39:06 Hans J. Koch wrote:
> > > Thanks a lot for reporting and discussing that problem. I'll add a
> > >
> > > Reported-by: Vitalii Demianets <vitas@nppfactor.kiev.ua>
> > >
> > > if you have no objections.
> >
> > No objections. Thanks, Hans.
> 
> By the way, what do you think about my revised version of uio_pdrv_genirq.c 
> patch from November 29th?

I hope I find the time to look at that later tonight. I'm a bit busy ATM.

Hans

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2012-12-03 17:28 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-27 11:48 [PATCH] uio.c: Fix warning: 'ret' might be used uninitialized Vitalii Demianets
2012-11-27 12:24 ` Vitalii Demianets
2012-11-27 22:43 ` Hans J. Koch
2012-11-28  8:58   ` Vitalii Demianets
2012-11-28 21:05     ` Hans J. Koch
2012-11-29 16:05       ` Tux9
2012-11-29 16:22         ` Vitalii Demianets
2012-11-29 16:33           ` Cong Ding
2012-11-29 16:36           ` Vitalii Demianets
2012-11-29 23:58             ` Hans J. Koch
2012-11-30 11:12               ` Tux9
2012-11-30 21:33                 ` Hans J. Koch
2012-12-01  1:22                   ` Cong Ding
2012-12-01  3:56                     ` Hans J. Koch
2012-12-01  9:58                       ` Cong Ding
2012-12-03  7:51                         ` Hans J. Koch
2012-11-30 11:16               ` Vitalii Demianets
2012-11-30 21:39                 ` Hans J. Koch
2012-11-30 23:43                   ` Greg Kroah-Hartman
2012-12-03  7:41                     ` [PATCH] stable: uio: " Hans J. Koch
2012-12-03 13:38                       ` Ben Hutchings
2012-12-03  8:35                   ` [PATCH] uio.c: " Vitalii Demianets
2012-12-03  8:53                     ` Vitalii Demianets
2012-12-03 17:27                       ` Hans J. Koch

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).