Fix warning: 'ret' might be used uninitialized
Signed-off-by: Vitalii Demianets <[email protected]>
---
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
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
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 <[email protected]>
> ---
> 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" <[email protected]>
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 <[email protected]>
---
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
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" <[email protected]>
> 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 <[email protected]>
> ---
> 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
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" <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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
>
>
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 <[email protected]> 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" <[email protected]>
>> > 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 <[email protected]>
>> > ---
>> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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 <[email protected]> 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" <[email protected]>
> >> > 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 <[email protected]>
> >> > ---
> >> > 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;
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
<[email protected]> 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.
> 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 <[email protected]> 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" <[email protected]>
> > >> > 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 <[email protected]>
> > >> > ---
> > >> > 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;
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" <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]> 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" <[email protected]>
> 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 <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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" <[email protected]>
> 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 <[email protected]>
> ---
> 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;
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 <[email protected]> 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" <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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 [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
>
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 <[email protected]>
if you have no objections.
Thanks,
Hans
>
> > >From 00c3c734c0dde67873a628bcb18cee403c95c301 Mon Sep 17 00:00:00 2001
> >
> > From: "Hans J. Koch" <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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;
>
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
On Fri, Nov 30, 2012 at 10:33 PM, Hans J. Koch <[email protected]> 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 <[email protected]> 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" <[email protected]>
>> > 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 <[email protected]>
>> > ---
>> > 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 [email protected]
>> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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 <[email protected]> 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