[Cluster-devel] bug in cman/init.d/cman.in?

[Cluster-devel] bug in cman/init.d/cman.in?

[Dietmar Maurer]

* do not overwrite global return status 'rtrn' - use local keyword

Index: new/cman/init.d/cman.in
===================================================================
--- new.orig/cman/init.d/cman.in	2010-12-02 07:19:35.000000000 +0100
+++ new/cman/init.d/cman.in	2010-12-23 11:32:12.000000000 +0100
@@ -46,7 +48,7 @@
 status()
 {
 	pid=$(pidof $1 2>/dev/null)
-	rtrn=$?
+	local rtrn=$?
 	if [ $rtrn -ne 0 ]; then
 		echo "$1 is stopped"
 	else

[Cluster-devel] bug in cman/init.d/cman.in?

[Fabio M. Di Nitto]

On 08/29/2011 10:44 AM, Dietmar Maurer wrote:
> * do not overwrite global return status 'rtrn' - use local keyword
> 
> Index: new/cman/init.d/cman.in
> ===================================================================
> --- new.orig/cman/init.d/cman.in	2010-12-02 07:19:35.000000000 +0100
> +++ new/cman/init.d/cman.in	2010-12-23 11:32:12.000000000 +0100
> @@ -46,7 +48,7 @@
>  status()
>  {
>  	pid=$(pidof $1 2>/dev/null)
> -	rtrn=$?
> +	local rtrn=$?
>  	if [ $rtrn -ne 0 ]; then
>  		echo "$1 is stopped"
>  	else
> 
> 

Is this purely semantic or are you actually fixing a bug?

Reading the code, it doesn't look like it makes any difference... but I
might not see it right.

Fabio

[Cluster-devel] bug in cman/init.d/cman.in?

[Dietmar Maurer]

This is a bug fix. I got wrong results.

Besides it does not lock sane to overwrite that global variable there (why?).

- Dietmar

> -----Original Message-----
> From: cluster-devel-bounces at redhat.com [mailto:cluster-devel-
> bounces at redhat.com] On Behalf Of Fabio M. Di Nitto
> Sent: Montag, 29. August 2011 14:06
> To: cluster-devel at redhat.com
> Subject: Re: [Cluster-devel] bug in cman/init.d/cman.in?
> 
> On 08/29/2011 10:44 AM, Dietmar Maurer wrote:
> > * do not overwrite global return status 'rtrn' - use local keyword
> >
> > Index: new/cman/init.d/cman.in
> >
> =================================================================
> ==
> > --- new.orig/cman/init.d/cman.in	2010-12-02 07:19:35.000000000 +0100
> > +++ new/cman/init.d/cman.in	2010-12-23 11:32:12.000000000 +0100
> > @@ -46,7 +48,7 @@
> >  status()
> >  {
> >  	pid=$(pidof $1 2>/dev/null)
> > -	rtrn=$?
> > +	local rtrn=$?
> >  	if [ $rtrn -ne 0 ]; then
> >  		echo "$1 is stopped"
> >  	else
> >
> >
> 
> Is this purely semantic or are you actually fixing a bug?
> 
> Reading the code, it doesn't look like it makes any difference... but I might not
> see it right.
> 
> Fabio
> 

[Cluster-devel] bug in cman/init.d/cman.in?

[Fabio M. Di Nitto]

On 08/29/2011 03:14 PM, Dietmar Maurer wrote:
> This is a bug fix. I got wrong results.

Ok, unless you have strong objections I prefer to rename the variable
completely in status() to avoid confusion and fix the issue.

> 
> Besides it does not lock sane to overwrite that global variable there (why?).

Simple overlook. I agree a fix is required, I simply wasn't sure if you
were hitting a bug or not since I couldn't spot the error in the code.

Fabio

> 
> - Dietmar
> 
>> -----Original Message-----
>> From: cluster-devel-bounces at redhat.com [mailto:cluster-devel-
>> bounces at redhat.com] On Behalf Of Fabio M. Di Nitto
>> Sent: Montag, 29. August 2011 14:06
>> To: cluster-devel at redhat.com
>> Subject: Re: [Cluster-devel] bug in cman/init.d/cman.in?
>>
>> On 08/29/2011 10:44 AM, Dietmar Maurer wrote:
>>> * do not overwrite global return status 'rtrn' - use local keyword
>>>
>>> Index: new/cman/init.d/cman.in
>>>
>> =================================================================
>> ==
>>> --- new.orig/cman/init.d/cman.in	2010-12-02 07:19:35.000000000 +0100
>>> +++ new/cman/init.d/cman.in	2010-12-23 11:32:12.000000000 +0100
>>> @@ -46,7 +48,7 @@
>>>  status()
>>>  {
>>>  	pid=$(pidof $1 2>/dev/null)
>>> -	rtrn=$?
>>> +	local rtrn=$?
>>>  	if [ $rtrn -ne 0 ]; then
>>>  		echo "$1 is stopped"
>>>  	else
>>>
>>>
>>
>> Is this purely semantic or are you actually fixing a bug?
>>
>> Reading the code, it doesn't look like it makes any difference... but I might not
>> see it right.
>>
>> Fabio
>>
> 
> 

[Cluster-devel] bug in cman/init.d/cman.in?

[Dietmar Maurer]

> Ok, unless you have strong objections I prefer to rename the variable completely
> in status() to avoid confusion and fix the issue.

Well, I have to say that I can't see the 'beauty' of shell scripts in general ;-) 
So please feel free to fix it as you like - it will then test the fix.

- Dietmar

[Cluster-devel] bug in cman/init.d/cman.in?

[Fabio M. Di Nitto]

On 8/29/2011 3:26 PM, Dietmar Maurer wrote:
>> Ok, unless you have strong objections I prefer to rename the variable completely
>> in status() to avoid confusion and fix the issue.
> 
> Well, I have to say that I can't see the 'beauty' of shell scripts in general ;-) 
> So please feel free to fix it as you like - it will then test the fix.

Fix is pushed in git, can you please let me know if it works for you? A
reproducer to trigger the error would be nice to have, so I can verify
it here too.

Fabio

[Cluster-devel] bug in cman/init.d/cman.in?

[Dietmar Maurer]

> Fix is pushed in git, can you please let me know if it works for you?

Thanks, seems to work.

> A reproducer
> to trigger the error would be nice to have, so I can verify it here too.

# /etc/init.d/cman start
Starting cluster: 
   Checking if cluster has been disabled at boot... [  OK  ]
   Checking Network Manager... [  OK  ]
...

# /etc/init.d/cman stop;echo $?
Stopping cluster: 
   Leaving fence domain... [  OK  ]
   Stopping dlm_controld... [  OK  ]
   Stopping fenced... [  OK  ]
   Stopping cman... [  OK  ]
   Waiting for corosync to shutdown:[  OK  ]
   Unloading kernel modules... [  OK  ]
   Unmounting configfs... [  OK  ]
1

So the stop fails (return 1 instead of 0).

BTW, I can see the same bug in rgmanager init script

- Dietmar

[Cluster-devel] bug in cman/init.d/cman.in?

[Fabio M. Di Nitto]

On 8/31/2011 11:07 AM, Dietmar Maurer wrote:
>> Fix is pushed in git, can you please let me know if it works for you?
> 
> Thanks, seems to work.
> 
>> A reproducer
>> to trigger the error would be nice to have, so I can verify it here too.
> 
> # /etc/init.d/cman start
> Starting cluster: 
>    Checking if cluster has been disabled at boot... [  OK  ]
>    Checking Network Manager... [  OK  ]
> ...
> 
> # /etc/init.d/cman stop;echo $?
> Stopping cluster: 
>    Leaving fence domain... [  OK  ]
>    Stopping dlm_controld... [  OK  ]
>    Stopping fenced... [  OK  ]
>    Stopping cman... [  OK  ]
>    Waiting for corosync to shutdown:[  OK  ]
>    Unloading kernel modules... [  OK  ]
>    Unmounting configfs... [  OK  ]
> 1
> 
> So the stop fails (return 1 instead of 0).

Ok thanks.

> 
> BTW, I can see the same bug in rgmanager init script

Nope... not any more ;)

Fabio

[Cluster-devel] bug in cman/init.d/cman.in?

[Dietmar Maurer]

> > BTW, I can see the same bug in rgmanager init script
> 
> Nope... not any more ;)

Thanks for the fix,

- Dietmar