[patch] powerpc: sysfs fix compiler warning

[patch] powerpc: sysfs fix compiler warning

[Christian Krafft]

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

From: Christian Krafft <krafft@de.ibm.com>

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 <krafft@de.ibm.com>

Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
===================================================================
--- linux-2.6.22-rc5.orig/arch/powerpc/kernel/sysfs.c
+++ 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);
 


-- 
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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch] powerpc: sysfs fix compiler warning

[Josh Boyer]

On Fri, 2007-06-22 at 19:48 +0200, Christian Krafft wrote:
> From: Christian Krafft <krafft@de.ibm.com>
> 
> 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 <krafft@de.ibm.com>
> 
> Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
> ===================================================================
> --- linux-2.6.22-rc5.orig/arch/powerpc/kernel/sysfs.c
> +++ 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);

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

josh

Re: [patch] powerpc: sysfs fix compiler warning

[Sergei Shtylyov]

Hello.

Josh Boyer wrote:

>>From: Christian Krafft <krafft@de.ibm.com>

>>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 <krafft@de.ibm.com>

>>Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
>>===================================================================
>>--- linux-2.6.22-rc5.orig/arch/powerpc/kernel/sysfs.c
>>+++ 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);
> 
> 
> 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. :-)

> josh

WBR, Sergei

Re: [patch] powerpc: sysfs fix compiler warning

[Josh Boyer]

On Fri, 2007-06-22 at 22:57 +0400, Sergei Shtylyov wrote:
> Hello.
> 
> Josh Boyer wrote:
> 
> >>From: Christian Krafft <krafft@de.ibm.com>
> 
> >>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 <krafft@de.ibm.com>
> 
> >>Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
> >>===================================================================
> >>--- linux-2.6.22-rc5.orig/arch/powerpc/kernel/sysfs.c
> >>+++ 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);
> > 
> > 
> > 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. :-)

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

Re: [patch] powerpc: sysfs fix compiler warning

[Sergei Shtylyov]

Josh Boyer wrote:
> On Fri, 2007-06-22 at 22:57 +0400, Sergei Shtylyov wrote:
> 
>>Hello.
>>
>>Josh Boyer wrote:
>>
>>
>>>>From: Christian Krafft <krafft@de.ibm.com>
>>
>>>>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 <krafft@de.ibm.com>
>>
>>>>Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
>>>>===================================================================
>>>>--- linux-2.6.22-rc5.orig/arch/powerpc/kernel/sysfs.c
>>>>+++ 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);

>>>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. :-)

> 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.

> josh

WBR, Sergei

Re: [patch] powerpc: sysfs fix compiler warning

[Josh Boyer]

On Fri, 2007-06-22 at 23:09 +0400, Sergei Shtylyov wrote:
> Josh Boyer wrote:
> > On Fri, 2007-06-22 at 22:57 +0400, Sergei Shtylyov wrote:
> > 
> >>Hello.
> >>
> >>Josh Boyer wrote:
> >>
> >>
> >>>>From: Christian Krafft <krafft@de.ibm.com>
> >>
> >>>>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 <krafft@de.ibm.com>
> >>
> >>>>Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
> >>>>===================================================================
> >>>>--- linux-2.6.22-rc5.orig/arch/powerpc/kernel/sysfs.c
> >>>>+++ 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);
> 
> >>>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. :-)
> 
> > 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

Re: [patch] powerpc: sysfs fix compiler warning

[Michael Buesch]

On Friday 22 June 2007 19:48:53 Christian Krafft wrote:
> From: Christian Krafft <krafft@de.ibm.com>
> 
> 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 <krafft@de.ibm.com>
> 
> Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
> ===================================================================
> --- linux-2.6.22-rc5.orig/arch/powerpc/kernel/sysfs.c
> +++ 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);

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

>  	}
>  
> +	if(error)
 {
> +		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.
> +
>  	mutex_unlock(&cpu_mutex);
> -	return 0;
> +	return error;
>  }
>  EXPORT_SYMBOL_GPL(cpu_add_sysdev_attr_group);
>  
> 
> 



-- 
Greetings Michael.

Re: [patch] powerpc: sysfs fix compiler warning

[Christian Krafft]

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

On Sat, 23 Jun 2007 11:46:54 +0200
Michael Buesch <mb@bu3sch.de> wrote:

> On Friday 22 June 2007 19:48:53 Christian Krafft wrote:
> > From: Christian Krafft <krafft@de.ibm.com>
> > 
> > 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 <krafft@de.ibm.com>
> > 
> > Index: linux-2.6.22-rc5/arch/powerpc/kernel/sysfs.c
> > ===================================================================
> > --- linux-2.6.22-rc5.orig/arch/powerpc/kernel/sysfs.c
> > +++ 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;

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

> >  
> >  	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.

> 
> >  	}
> >  
> > +	if(error)
>  {
> > +		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.
> > +
> >  	mutex_unlock(&cpu_mutex);
> > -	return 0;
> > +	return error;
> >  }
> >  EXPORT_SYMBOL_GPL(cpu_add_sysdev_attr_group);
> >  
> > 
> > 
> 
> 
> 


-- 
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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[patch] powerpc: sysfs fix compiler warning

[Christian Krafft]

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

From: Christian Krafft <krafft@de.ibm.com>

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 <krafft@de.ibm.com>

--- linux-2.6.orig/arch/powerpc/kernel/sysfs.c
+++ linux-2.6/arch/powerpc/kernel/sysfs.c
@@ -380,16 +380,25 @@ int cpu_add_sysdev_attr_group(struct att
 {
 	int cpu;
 	struct sys_device *sysdev;
+	int error = 0;
 
 	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);
+			}
+			break;
+		}
 	}
 
 	mutex_unlock(&cpu_mutex);
-	return 0;
+	return error;
 }
 EXPORT_SYMBOL_GPL(cpu_add_sysdev_attr_group);
 


-- 
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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch] powerpc: sysfs fix compiler warning

[Michael Buesch]

On Friday 29 June 2007 16:50:10 Christian Krafft wrote:
> From: Christian Krafft <krafft@de.ibm.com>
> 
> 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 <krafft@de.ibm.com>
> 
> --- linux-2.6.orig/arch/powerpc/kernel/sysfs.c
> +++ linux-2.6/arch/powerpc/kernel/sysfs.c
> @@ -380,16 +380,25 @@ int cpu_add_sysdev_attr_group(struct att
>  {
>  	int cpu;
>  	struct sys_device *sysdev;
> +	int error = 0;
>  
>  	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);

Is sysfs_remove_group() safe to call on kobjs for which
we did not call sysfs_create_group()?

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



-- 
Greetings Michael.

Re: [patch] powerpc: sysfs fix compiler warning

[Christian Krafft]

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

On Fri, 29 Jun 2007 16:57:20 +0200
Michael Buesch <mb@bu3sch.de> wrote:

> On Friday 29 June 2007 16:50:10 Christian Krafft wrote:
> > From: Christian Krafft <krafft@de.ibm.com>
> > 
> > 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 <krafft@de.ibm.com>
> > 
> > --- linux-2.6.orig/arch/powerpc/kernel/sysfs.c
> > +++ linux-2.6/arch/powerpc/kernel/sysfs.c
> > @@ -380,16 +380,25 @@ int cpu_add_sysdev_attr_group(struct att
> >  {
> >  	int cpu;
> >  	struct sys_device *sysdev;
> > +	int error = 0;
> >  
> >  	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);
> 
> Is sysfs_remove_group() safe to call on kobjs for which
> we did not call sysfs_create_group()?

I only looked into it a little and haven't seen a problem.
From the interface point of view I would say it would be a bug, if it's not safe.
But I will do a short test on this.


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


-- 
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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]