* [Cluster-devel] bug in cman/init.d/cman.in?
@ 2011-08-29 8:44 Dietmar Maurer
2011-08-29 12:06 ` Fabio M. Di Nitto
0 siblings, 1 reply; 9+ messages in thread
From: Dietmar Maurer @ 2011-08-29 8:44 UTC (permalink / raw)
To: cluster-devel.redhat.com
* 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
^ permalink raw reply [flat|nested] 9+ messages in thread* [Cluster-devel] bug in cman/init.d/cman.in?
2011-08-29 8:44 [Cluster-devel] bug in cman/init.d/cman.in? Dietmar Maurer
@ 2011-08-29 12:06 ` Fabio M. Di Nitto
2011-08-29 13:14 ` Dietmar Maurer
0 siblings, 1 reply; 9+ messages in thread
From: Fabio M. Di Nitto @ 2011-08-29 12:06 UTC (permalink / raw)
To: cluster-devel.redhat.com
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [Cluster-devel] bug in cman/init.d/cman.in?
2011-08-29 12:06 ` Fabio M. Di Nitto
@ 2011-08-29 13:14 ` Dietmar Maurer
2011-08-29 13:20 ` Fabio M. Di Nitto
0 siblings, 1 reply; 9+ messages in thread
From: Dietmar Maurer @ 2011-08-29 13:14 UTC (permalink / raw)
To: cluster-devel.redhat.com
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* [Cluster-devel] bug in cman/init.d/cman.in?
2011-08-29 13:14 ` Dietmar Maurer
@ 2011-08-29 13:20 ` Fabio M. Di Nitto
2011-08-29 13:26 ` Dietmar Maurer
0 siblings, 1 reply; 9+ messages in thread
From: Fabio M. Di Nitto @ 2011-08-29 13:20 UTC (permalink / raw)
To: cluster-devel.redhat.com
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
>>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread* [Cluster-devel] bug in cman/init.d/cman.in?
2011-08-29 13:20 ` Fabio M. Di Nitto
@ 2011-08-29 13:26 ` Dietmar Maurer
2011-08-30 9:46 ` Fabio M. Di Nitto
0 siblings, 1 reply; 9+ messages in thread
From: Dietmar Maurer @ 2011-08-29 13:26 UTC (permalink / raw)
To: cluster-devel.redhat.com
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Cluster-devel] bug in cman/init.d/cman.in?
2011-08-29 13:26 ` Dietmar Maurer
@ 2011-08-30 9:46 ` Fabio M. Di Nitto
2011-08-31 9:07 ` Dietmar Maurer
0 siblings, 1 reply; 9+ messages in thread
From: Fabio M. Di Nitto @ 2011-08-30 9:46 UTC (permalink / raw)
To: cluster-devel.redhat.com
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Cluster-devel] bug in cman/init.d/cman.in?
2011-08-30 9:46 ` Fabio M. Di Nitto
@ 2011-08-31 9:07 ` Dietmar Maurer
2011-08-31 9:12 ` Fabio M. Di Nitto
0 siblings, 1 reply; 9+ messages in thread
From: Dietmar Maurer @ 2011-08-31 9:07 UTC (permalink / raw)
To: cluster-devel.redhat.com
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Cluster-devel] bug in cman/init.d/cman.in?
2011-08-31 9:07 ` Dietmar Maurer
@ 2011-08-31 9:12 ` Fabio M. Di Nitto
2011-08-31 10:33 ` Dietmar Maurer
0 siblings, 1 reply; 9+ messages in thread
From: Fabio M. Di Nitto @ 2011-08-31 9:12 UTC (permalink / raw)
To: cluster-devel.redhat.com
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2011-08-31 10:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-29 8:44 [Cluster-devel] bug in cman/init.d/cman.in? Dietmar Maurer
2011-08-29 12:06 ` Fabio M. Di Nitto
2011-08-29 13:14 ` Dietmar Maurer
2011-08-29 13:20 ` Fabio M. Di Nitto
2011-08-29 13:26 ` Dietmar Maurer
2011-08-30 9:46 ` Fabio M. Di Nitto
2011-08-31 9:07 ` Dietmar Maurer
2011-08-31 9:12 ` Fabio M. Di Nitto
2011-08-31 10:33 ` Dietmar Maurer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.