powerpc: sysfs fix compiler warning

PATCH In review Christian Krafft 4 participants 8 messages Jun 22, 2007 – Jun 25, 2007 PowerPC

lore

Share & tools

lore

Christian Krafft

author

2007-06-22 17:49 UTC #0 lore raw

PATCH 1/1 powerpc: sysfs fix compiler warning

From: Christian Krafft <[email protected]>

This patch fixes the following compiler warning:
arch/powerpc/kernel/sysfs.c:385: warning: ignoring return value of `sysfs_create_group',

Signed-off-by Christian Krafft <[email protected]>

arch/powerpc/kernel/sysfs.c +9 -2

Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
===================================================================
@@ -380,16 +380,23 @@ int cpu_add_sysdev_attr_group(struct att
 {
 	int cpu;
 	struct sys_device *sysdev;
+	int error;
 
 	mutex_lock(&cpu_mutex);
 
 	for_each_possible_cpu(cpu) {
 		sysdev = get_cpu_sysdev(cpu);
-		sysfs_create_group(&sysdev->kobj, attrs);
+		error |= sysfs_create_group(&sysdev->kobj, attrs);
 	}
 
+	if(error)
+		for_each_possible_cpu(cpu) {
+			sysdev = get_cpu_sysdev(cpu);
+			sysfs_remove_group(&sysdev->kobj, attrs);
+		}
+
 	mutex_unlock(&cpu_mutex);
-	return 0;
+	return error;
 }
 EXPORT_SYMBOL_GPL(cpu_add_sysdev_attr_group);

Signature · 13 lines

Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294

Josh Boyer

2007-06-22 18:38 UTC #1 lore raw

Quoted 14 lines · Christian Krafft

From: Christian Krafft <[email protected]>
This patch fixes the following compiler warning:
arch/powerpc/kernel/sysfs.c:385: warning: ignoring return value of `sysfs_create_group',
Signed-off-by Christian Krafft <[email protected]>
Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
===================================================================
arch/powerpc/kernel/sysfs.c +0 -0
@@ -380,16 +380,23 @@ int cpu_add_sysdev_attr_group(struct att
 {

arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 389

Quoted 10 lines

arch/powerpc/kernel/sysfs.c +2 -1

 	int cpu;
 	struct sys_device *sysdev;
+	int error;
 
 	mutex_lock(&cpu_mutex);
 
 	for_each_possible_cpu(cpu) {
 		sysdev = get_cpu_sysdev(cpu);
-		sysfs_create_group(&sysdev->kobj, attrs);
+		error |= sysfs_create_group(&sysdev->kobj, attrs);

Why are you or'ing the return value with whatever is in the uninitialized variable?

josh

Sergei Shtylyov

2007-06-22 18:56 UTC #2 lore raw

Hello.

Quoted 20 lines · Josh Boyer

Quoted 20 lines
From: Christian Krafft <[email protected]>
This patch fixes the following compiler warning:
arch/powerpc/kernel/sysfs.c:385: warning: ignoring return value of `sysfs_create_group',
Signed-off-by Christian Krafft <[email protected]>
Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
===================================================================
arch/powerpc/kernel/sysfs.c +1 -0
@@ -380,16 +380,23 @@ int cpu_add_sysdev_attr_group(struct att
 {
 	int cpu;
 	struct sys_device *sysdev;
+	int error;
 
 	mutex_lock(&cpu_mutex);
 

Quoted 8 lines

arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 389
Quoted 4 lines
arch/powerpc/kernel/sysfs.c +1 -1
 	for_each_possible_cpu(cpu) {
 		sysdev = get_cpu_sysdev(cpu);
-		sysfs_create_group(&sysdev->kobj, attrs);
+		error |= sysfs_create_group(&sysdev->kobj, attrs);
Why are you or'ing the return value with whatever is in the uninitialized variable?

To make it always fail, no doubt. ;-)
ISO accumulation error, it makes more sense to start cleanup created
groups right after the first creation failure -- although with the callers opf this function ignoring the result anyway, who cares. :-)

Quoted 1 line

josh

WBR, Sergei

Josh Boyer

2007-06-22 19:05 UTC #3 lore raw

Quoted 33 lines · Sergei Shtylyov

Hello.
Quoted 20 lines · Josh Boyer
Quoted 20 lines
From: Christian Krafft <[email protected]>
This patch fixes the following compiler warning:
arch/powerpc/kernel/sysfs.c:385: warning: ignoring return value of `sysfs_create_group',
Signed-off-by Christian Krafft <[email protected]>
Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
===================================================================
arch/powerpc/kernel/sysfs.c +1 -0
@@ -380,16 +380,23 @@ int cpu_add_sysdev_attr_group(struct att
 {
 	int cpu;
 	struct sys_device *sysdev;
+	int error;
 
 	mutex_lock(&cpu_mutex);
 
Quoted 8 lines
arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 389
Quoted 4 lines
arch/powerpc/kernel/sysfs.c +1 -1
 	for_each_possible_cpu(cpu) {
 		sysdev = get_cpu_sysdev(cpu);
-		sysfs_create_group(&sysdev->kobj, attrs);
+		error |= sysfs_create_group(&sysdev->kobj, attrs);
Why are you or'ing the return value with whatever is in the uninitialized variable?

Quoted 4 lines

To make it always fail, no doubt. ;-)
ISO accumulation error, it makes more sense to start cleanup created
groups right after the first creation failure -- although with the callers opf this function ignoring the result anyway, who cares. :-)

I care because he does an if (error) below that. Since error isn't initialized, it could be some random value and or'ing a 0 return code from sysfs_create_group would still trigger the error path.

Plus, it's bogus anyway and should be fixed.

josh

Sergei Shtylyov

2007-06-22 19:07 UTC #4 lore raw

Quoted 39 lines · Josh Boyer

Quoted 33 lines · Sergei Shtylyov
Hello.
Quoted 20 lines · Josh Boyer
Quoted 20 lines
From: Christian Krafft <[email protected]>
This patch fixes the following compiler warning:
arch/powerpc/kernel/sysfs.c:385: warning: ignoring return value of `sysfs_create_group',
Signed-off-by Christian Krafft <[email protected]>
Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
===================================================================
arch/powerpc/kernel/sysfs.c +1 -0
@@ -380,16 +380,23 @@ int cpu_add_sysdev_attr_group(struct att
 {
 	int cpu;
 	struct sys_device *sysdev;
+	int error;
 
 	mutex_lock(&cpu_mutex);
 
Quoted 7 lines
arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 389
Quoted 4 lines
arch/powerpc/kernel/sysfs.c +1 -1
 	for_each_possible_cpu(cpu) {
 		sysdev = get_cpu_sysdev(cpu);
-		sysfs_create_group(&sysdev->kobj, attrs);
+		error |= sysfs_create_group(&sysdev->kobj, attrs);
Why are you or'ing the return value with whatever is in the uninitialized variable?
Quoted 3 lines
To make it always fail, no doubt. ;-)
ISO accumulation error, it makes more sense to start cleanup created
groups right after the first creation failure -- although with the callers opf this function ignoring the result anyway, who cares. :-)

Quoted 5 lines

I care because he does an if (error) below that. Since error isn't initialized, it could be some random value and or'ing a 0 return code from sysfs_create_group would still trigger the error path.
Plus, it's bogus anyway and should be fixed.

You misunderstood: I didn't at all object to fixing the bogosity you've
pointed out, just mused about how/if the error cleanup should be done.

Quoted 1 line

josh

WBR, Sergei

Josh Boyer

2007-06-22 19:12 UTC #5 lore raw

Quoted 41 lines · Sergei Shtylyov

Quoted 39 lines · Josh Boyer
Quoted 37 lines · Sergei Shtylyov
Hello.
Quoted 20 lines · Josh Boyer
Quoted 20 lines
From: Christian Krafft <[email protected]>
This patch fixes the following compiler warning:
arch/powerpc/kernel/sysfs.c:385: warning: ignoring return value of `sysfs_create_group',
Signed-off-by Christian Krafft <[email protected]>
Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
===================================================================
arch/powerpc/kernel/sysfs.c +1 -0
@@ -380,16 +380,23 @@ int cpu_add_sysdev_attr_group(struct att
 {
 	int cpu;
 	struct sys_device *sysdev;
+	int error;
 
 	mutex_lock(&cpu_mutex);
 
Quoted 7 lines
arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 389
Quoted 4 lines
arch/powerpc/kernel/sysfs.c +1 -1
 	for_each_possible_cpu(cpu) {
 		sysdev = get_cpu_sysdev(cpu);
-		sysfs_create_group(&sysdev->kobj, attrs);
+		error |= sysfs_create_group(&sysdev->kobj, attrs);
Why are you or'ing the return value with whatever is in the uninitialized variable?
To make it always fail, no doubt. ;-)
ISO accumulation error, it makes more sense to start cleanup created
groups right after the first creation failure -- although with the callers opf this function ignoring the result anyway, who cares. :-)

Quoted 8 lines

Quoted 5 lines
I care because he does an if (error) below that. Since error isn't initialized, it could be some random value and or'ing a 0 return code from sysfs_create_group would still trigger the error path.
Plus, it's bogus anyway and should be fixed.
You misunderstood: I didn't at all object to fixing the bogosity you've
pointed out, just mused about how/if the error cleanup should be done.

Ah, gotcha. Sorry, it's Friday and I shouldn't be here ;)

josh

Michael Buesch

2007-06-23 09:47 UTC #6 lore raw

In reply to Christian Krafft

Quoted 14 lines · On Friday 22 June 2007 19:48:53 Christian Krafft wrote:

From: Christian Krafft <[email protected]>
This patch fixes the following compiler warning:
arch/powerpc/kernel/sysfs.c:385: warning: ignoring return value of `sysfs_create_group',
Signed-off-by Christian Krafft <[email protected]>
Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
===================================================================
arch/powerpc/kernel/sysfs.c +0 -0
@@ -380,16 +380,23 @@ int cpu_add_sysdev_attr_group(struct att
 {

arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 389

Quoted 10 lines

arch/powerpc/kernel/sysfs.c +2 -1

 	int cpu;
 	struct sys_device *sysdev;
+	int error;
 
 	mutex_lock(&cpu_mutex);
 
 	for_each_possible_cpu(cpu) {
 		sysdev = get_cpu_sysdev(cpu);
-		sysfs_create_group(&sysdev->kobj, attrs);
+		error |= sysfs_create_group(&sysdev->kobj, attrs);

That is probably dangerous, if multiple sysfs_create_group fail with different error codes. So it will wiggle the error codes together.

arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 392

Quoted 3 lines

arch/powerpc/kernel/sysfs.c +1 -0
 	}
 
+	if(error)

{

arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 396

Quoted 4 lines

arch/powerpc/kernel/sysfs.c +4 -0

+		for_each_possible_cpu(cpu) {
+			sysdev = get_cpu_sysdev(cpu);
+			sysfs_remove_group(&sysdev->kobj, attrs);
+		}

probably do something like

		error = -ENOENT;

}
or some other error code here to fix it.

Quoted 9 lines

arch/powerpc/kernel/sysfs.c +2 -1

+
 	mutex_unlock(&cpu_mutex);
-	return 0;
+	return error;
 }
 EXPORT_SYMBOL_GPL(cpu_add_sysdev_attr_group);

Signature · 1 line

Greetings Michael.

Christian Krafft

author

2007-06-25 17:07 UTC #7 lore raw

Quoted 9 lines · On Sat, 23 Jun 2007 11:46:54 +0200 Michael Buesch <[email protected]> wrote:

Quoted 8 lines · On Friday 22 June 2007 19:48:53 Christian Krafft wrote:
From: Christian Krafft <[email protected]>
This patch fixes the following compiler warning:
arch/powerpc/kernel/sysfs.c:385: warning: ignoring return value of `sysfs_create_group',
Signed-off-by Christian Krafft <[email protected]>

Quoted 9 lines

arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 383
Quoted 9 lines
Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
===================================================================
arch/powerpc/kernel/sysfs.c +1 -0
@@ -380,16 +380,23 @@ int cpu_add_sysdev_attr_group(struct att
 {
 	int cpu;
 	struct sys_device *sysdev;
+	int error;

Looks like I introduced a new warning while fixing another. Have checked the patch with sparse just after sending it ;-)

Quoted 11 lines

arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 389
Quoted 7 lines
arch/powerpc/kernel/sysfs.c +1 -1
 	mutex_lock(&cpu_mutex);
 
 	for_each_possible_cpu(cpu) {
 		sysdev = get_cpu_sysdev(cpu);
-		sysfs_create_group(&sysdev->kobj, attrs);
+		error |= sysfs_create_group(&sysdev->kobj, attrs);
That is probably dangerous, if multiple sysfs_create_group fail with different error codes. So it will wiggle the error codes together.

Yeah, that's right, will fix it to do a break in case of error.

Quoted 25 lines

arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 392
Quoted 4 lines
arch/powerpc/kernel/sysfs.c +1 -0
 	}
 
+	if(error)
{
arch/powerpc/kernel/sysfs.c int cpu_add_sysdev_attr_group(struct att · line 396
Quoted 4 lines
arch/powerpc/kernel/sysfs.c +4 -0
+		for_each_possible_cpu(cpu) {
+			sysdev = get_cpu_sysdev(cpu);
+			sysfs_remove_group(&sysdev->kobj, attrs);
+		}
probably do something like
		error = -ENOENT;
}
or some other error code here to fix it.
Quoted 9 lines
arch/powerpc/kernel/sysfs.c +2 -1
+
 	mutex_unlock(&cpu_mutex);
-	return 0;
+	return error;
 }
 EXPORT_SYMBOL_GPL(cpu_add_sysdev_attr_group);
 
 

Signature · 13 lines

Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294