Bug for sv status in lsb mode

Bug for sv status in lsb mode

[Mathieu Poumeyrol]

Hello there,

I have tracked down a bug in the lsb-compatible code for sv status:   
sv return 0 even if the service is down.

The variable r in the "status" function in the sv.c file is global,  
and altered in the curse of the function, making the last test  
useless. From what I have seen, saving the status in a local variable  
instead of using the global r solve the issue, but I don't know if  
this fix is without side-effect.

I hope somebody can fix this in a proper way and push the changes.

Thanks !

Re: Bug for sv status in lsb mode

[Charlie Brady]

On Mon, 13 Jul 2009, Mathieu Poumeyrol wrote:

> I have tracked down a bug in the lsb-compatible code for sv status:  sv 
> return 0 even if the service is down.

What makes you think that sv status should return non-zero if the service 
is down? I see nothing to suggest that here:

http://smarden.sunsite.dk/runit/sv.8.html

Re: Bug for sv status in lsb mode

[Mathieu Poumeyrol]

Le 13 juil. 09 à 15:42, Charlie Brady a écrit :
>
> On Mon, 13 Jul 2009, Mathieu Poumeyrol wrote:
>
>> I have tracked down a bug in the lsb-compatible code for sv  
>> status:  sv return 0 even if the service is down.
>
> What makes you think that sv status should return non-zero if the  
> service is down? I see nothing to suggest that here:
>
> http://smarden.sunsite.dk/runit/sv.8.html


It's not explicit here, but required for LSB compliance.

see http://refspecs.freestandards.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html

As a matter of fact, sv code looks like it is meant to achieve this,  
but fails because of the side effect on the "r" global.

Re: Bug for sv status in lsb mode

[Mathieu Poumeyrol]

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


Le 13 juil. 09 à 16:13, Mathieu Poumeyrol a écrit :
>
> Le 13 juil. 09 à 15:42, Charlie Brady a écrit :
>>
>> On Mon, 13 Jul 2009, Mathieu Poumeyrol wrote:
>>
>>> I have tracked down a bug in the lsb-compatible code for sv  
>>> status:  sv return 0 even if the service is down.
>>
>> What makes you think that sv status should return non-zero if the  
>> service is down? I see nothing to suggest that here:
>>
>> http://smarden.sunsite.dk/runit/sv.8.html
>
>
> It's not explicit here, but required for LSB compliance.
>
> see http://refspecs.freestandards.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html
>
> As a matter of fact, sv code looks like it is meant to achieve this,  
> but fails because of the side effect on the "r" global.

Sorry, it is actually explicit in the "Exit codes" last paragraph...

If the command is status, it exits 3 if the service is down, and 4 if  
the status is unknown. It exits 2 on wrong usage, and 151 on error.

Re: Bug for sv status in lsb mode

[Charlie Brady]

On Mon, 13 Jul 2009, Mathieu Poumeyrol wrote:

>> > What makes you think that sv status should return non-zero if the service 
>> > is down? I see nothing to suggest that here:
>> > 
>> > http://smarden.sunsite.dk/runit/sv.8.html
>> 
>> It's not explicit here, but required for LSB compliance.
>> 
>> see 
>> http://refspecs.freestandards.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html
>> 
>> As a matter of fact, sv code looks like it is meant to achieve this, but 
>> fails because of the side effect on the "r" global.
>
> Sorry, it is actually explicit in the "Exit codes" last paragraph...
>
> If the command is status, it exits 3 if the service is down, and 4 if the 
> status is unknown. It exits 2 on wrong usage, and 151 on error.

Indeed. Sorry for not reading that more carefully myself.

Re: Bug for sv status in lsb mode

[Gerrit Pape]

Mathieu Poumeyrol <kali <at> zoy.org> writes:
> Le 13 juil. 09 à 16:13, Mathieu Poumeyrol a écrit :
> > Le 13 juil. 09 à 15:42, Charlie Brady a écrit :
> > It's not explicit here, but required for LSB compliance.
> >
> > see
http://refspecs.freestandards.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html
> >
> > As a matter of fact, sv code looks like it is meant to achieve this,  
> > but fails because of the side effect on the "r" global.
> 
> Sorry, it is actually explicit in the "Exit codes" last paragraph...
> 
> If the command is status, it exits 3 if the service is down, and 4 if  
> the status is unknown. It exits 2 on wrong usage, and 151 on error.

Hi, this is a bug in the documentation then, the complete paragraph is

 If  sv  is called with a base name other than sv, it exits 1 on timeout
 or trouble sending the command.  If the command is status, it  exits  3
 if  the service is down, and 4 if the status is unknown.  It exits 2 on
 wrong usage, and 151 on error.

These exit codes only apply if sv is called with a different basename.

 # sv stop getty-5
 ok: down: getty-5: 0s, normally up
 # ln -s $(which sv) /tmp/getty-5
 # /tmp/getty-5 status
 down: getty-5: 4s, normally up
 # echo $?
 3
 #

Any suggestions on how to improve the man page?

Thanks, Gerrit.

Re: Bug for sv status in lsb mode

[Mathieu Poumeyrol]

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


Le 17 juil. 09 à 11:08, Gerrit Pape a écrit :

> Mathieu Poumeyrol <kali <at> zoy.org> writes:
>> Le 13 juil. 09 à 16:13, Mathieu Poumeyrol a écrit :
>>> Le 13 juil. 09 à 15:42, Charlie Brady a écrit :
>>> It's not explicit here, but required for LSB compliance.
>>>
>>> see
> http://refspecs.freestandards.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html
>>>
>>> As a matter of fact, sv code looks like it is meant to achieve this,
>>> but fails because of the side effect on the "r" global.
>>
>> Sorry, it is actually explicit in the "Exit codes" last paragraph...
>>
>> If the command is status, it exits 3 if the service is down, and 4 if
>> the status is unknown. It exits 2 on wrong usage, and 151 on error.
>
> Hi, this is a bug in the documentation then, the complete paragraph is
>
> If  sv  is called with a base name other than sv, it exits 1 on  
> timeout
> or trouble sending the command.  If the command is status, it   
> exits  3
> if  the service is down, and 4 if the status is unknown.  It exits 2  
> on
> wrong usage, and 151 on error.
>
> These exit codes only apply if sv is called with a different basename.
>
> # sv stop getty-5
> ok: down: getty-5: 0s, normally up
> # ln -s $(which sv) /tmp/getty-5
> # /tmp/getty-5 status
> down: getty-5: 4s, normally up
> # echo $?
> 3
> #
>
> Any suggestions on how to improve the man page?

Hello,

the man page is fine. And I aggree the behaviour you demounstrate is  
correct.

The problem I exhibited and tracked is only visible for services that  
use a logging script, as the function that will change the value of  
the "r" global is only called if the service has a logging service...

# tree /etc/service/that-one-does-not-work
/etc/service/that-one-does-not-work
|-- log
|   |-- main
|   |   |-- current
|   |   `-- lock
|   |-- run
|   `-- supervise
|       |-- control
|       |-- lock
|       |-- ok
|       |-- pid
|       |-- stat
|       `-- status
|-- run
`-- supervise
     |-- control
     |-- lock
     |-- ok
     |-- pid
     |-- stat
     `-- status

# ./that-one-does-not-work status ; echo $?
down: that-one-does-not: 8s, normally up; run: log: (pid 11444) 37s
0

Re: Bug for sv status in lsb mode

[Gerrit Pape]

Mathieu Poumeyrol <kali <at> zoy.org> writes:
> the man page is fine. And I aggree the behaviour you demounstrate is  
> correct.
> 
> The problem I exhibited and tracked is only visible for services that  
> use a logging script, as the function that will change the value of  
> the "r" global is only called if the service has a logging service...

I see.  Thanks a lot for the bug report.

Regards, Gerrit.

sv sometimes won't issue down request

[Earl Chew]

This defect was submitted against the busybox/runit implementation:

https://bugs.busybox.net/show_bug.cgi?id=461

> touch /tmp/testservice/down
> runsv /tmp/testservice &
> sv once /tmp/testservice
> sv down /tmp/testservice
> 
> The last line attempts to bring down the service, but does nothing.

Denys Vlasenko wrote:

> I propose just removing this test:
> 
>         if (svstatus.want == *a)
>                 return 0;
> 
> 
> it seems to be an optimization (and buggy one at that).


There is similar code in runit 2.0.0:

> int control(char *a) {
>   if (svstatus_get() <= 0) return(-1);
>   if (svstatus[17] == *a) return(0);
>   if ((fd =open_write("supervise/control")) == -1) {

Earl

svwaitup races with sv

[Earl Chew]

The script below generates the following output:

> down: ./svc: 0s
> svwaitup: warning: ./svc: is down.
> .... WaitUp 1
> Try again
> run: ./svc: (pid 8496) 2s, normally down, want down

It illustrates a race between runsv/sv and svwaitup.

Commenting out the "touch svc/down" leads to other strange
results like:

> run: ./svc: (pid 8651) 0s
> svwaitup: warning: ./svc: unable to change directory: file does not exist
> .... WaitUp 1
> Try again
> run: ./svc: (pid 8651) 4s, want down


The key is that sv(8) writes its command and simply exits. There is
no way to determine if the command has actually been acted upon.

For example, the following appears to synchronise ok but is a bit
clunky:

    rm svc/supervise/pid
    ../command/sv once ./svc
    while [ -e svc/supervise/pid ] ; do
      sleep 1
    done

It would be better if sv(8) itself could synchronise.

Earl

-----------------------------------------------------------------------

#!/bin/sh

rm -rf svc
mkdir svc

touch svc/down

cat > svc/run <<\EOF
#!/bin/sh

sleep 30
EOF
chmod +x svc/run

trap 'kill $SVC_' EXIT
../command/runsv ./svc &
SVC_=$!

../command/sv once ./svc
../command/sv status ./svc || echo .... Status $?
../command/svwaitup ./svc  || echo .... WaitUp $?

sleep 2
echo "Try again"

../command/sv status ./svc || echo .... Status $?
../command/svwaitup ./svc  || echo .... WaitUp $?

Re: sv sometimes won't issue down request (specifically, after 'sv once xxx')

[Charlie Brady]

On Sun, 19 Jul 2009, Earl Chew wrote:

> This defect was submitted against the busybox/runit implementation:
>
> https://bugs.busybox.net/show_bug.cgi?id=461
>
>>  touch /tmp/testservice/down
>>  runsv /tmp/testservice &
>>  sv once /tmp/testservice
>>  sv down /tmp/testservice
>>
>>  The last line attempts to bring down the service, but does nothing.
>
> Denys Vlasenko wrote:
>
>>  I propose just removing this test:
>>
>>          if (svstatus.want == *a)
>>                  return 0;
>>
>>  it seems to be an optimization (and buggy one at that).

Yes it does seem buggy optimisation.

Note that if you simply removed that check, you would change not only the 
'once' behaviour of sv/runsv, but also the behaviour when multiple 'sv d 
xxx' calls are made on a running service. If the optimisation is not 
re-implemented in runsv, then there will be multiple signals and/or custom 
control scripts run where previously there was only one.

Re: svwaitup races with sv

[Charlie Brady]

On Sun, 19 Jul 2009, Earl Chew wrote:

> The script below generates the following output:
>
>>  down: ./svc: 0s
>>  svwaitup: warning: ./svc: is down.
>>  .... WaitUp 1
>>  Try again
>>  run: ./svc: (pid 8496) 2s, normally down, want down
>
> It illustrates a race between runsv/sv and svwaitup.
...
> It would be better if sv(8) itself could synchronise.

I think that would be possible by making the fifo unbuffered, and having 
sv inject a nop command during 'sv status' (so sv would be blocked until 
runsv had processed previous commands). But that would change the timing 
of lots of current programs which call 'sv'.

Re: svwaitup races with sv

[Earl Chew]

Charlie Brady wrote:
> I think that would be possible by making the fifo unbuffered, and having 
> sv inject a nop command during 'sv status' (so sv would be blocked until 
> runsv had processed previous commands). But that would change the timing 
> of lots of current programs which call 'sv'.

Hmm ... I just noticted "sv start" which appears to combine "sv up"
and "svwaitup".


In situations where the initiator is willing to block and wait, perhaps
"sv start" is reasonable.

In other situations, the initiator does not want to block, but later on
some dependent wants to ensure that the service is up.

Is "sv start" also applicable in this situation ?

Earl

Re: sv sometimes won't issue down request (specifically, after 'sv once xxx')

[Earl Chew]

Charlie Brady wrote:
> Yes it does seem buggy optimisation.
> 
> Note that if you simply removed that check, you would change not only 
> the 'once' behaviour of sv/runsv, but also the behaviour when multiple 
> 'sv d xxx' calls are made on a running service. If the optimisation is 
> not re-implemented in runsv, then there will be multiple signals and/or 
> custom control scripts run where previously there was only one.

Are you saying it is prudent to move the optimisation in runsv itself?

Earl

Re: sv sometimes won't issue down request (specifically, after 'sv once xxx')

[Charlie Brady]

On Fri, 24 Jul 2009, Earl Chew wrote:

> Charlie Brady wrote:
>>  Yes it does seem buggy optimisation.
>>
>>  Note that if you simply removed that check, you would change not only the
>>  'once' behaviour of sv/runsv, but also the behaviour when multiple 'sv d
>>  xxx' calls are made on a running service. If the optimisation is not
>>  re-implemented in runsv, then there will be multiple signals and/or custom
>>  control scripts run where previously there was only one.
>
> Are you saying it is prudent to move the optimisation in runsv itself?

No, I am saying that it would be imprudent to remove the optimisation in 
sv without considering this issue.

I'd defer to Gerrit on a good solution to the problem you've found.

How often do you want to run something once (via 'sv once xxx') and then 
want to terminate it before it has completed?

Re: sv sometimes won't issue down request (specifically, after 'sv once xxx')

[Joan Picanyol i Puig]

* Charlie Brady <charlieb-supervision@budge.apana.org.au> [20090724 23:02]:
> How often do you want to run something once (via 'sv once xxx') and then 
> want to terminate it before it has completed?
> 
Pretty much any long manual service (i.e.: occasional mirrer updates) is
a use case for this.

qvb
-- 
pica

Re: sv sometimes won't issue down request

[Gerrit Pape]

Earl Chew <earl_chew <at> agilent.com> writes:
> This defect was submitted against the busybox/runit implementation:
> 
> https://bugs.busybox.net/show_bug.cgi?id=461
> 
> > touch /tmp/testservice/down
> > runsv /tmp/testservice &
> > sv once /tmp/testservice
> > sv down /tmp/testservice
> > 
> > The last line attempts to bring down the service, but does nothing.

Issuing 'sv term' would work though.

> Denys Vlasenko wrote:
> 
> > I propose just removing this test:
> > 
> >         if (svstatus.want == *a)
> >                 return 0;
> > 
> > 
> > it seems to be an optimization (and buggy one at that).
> 
> There is similar code in runit 2.0.0:
> 
> > int control(char *a) {
> >   if (svstatus_get() <= 0) return(-1);
> >   if (svstatus[17] == *a) return(0);
> >   if ((fd =open_write("supervise/control")) == -1) {

This patch should handle that special case ('once' followed by 'down').
If sv sees that the service is already scheduled to be down ('want
down'), but no TERM signal has been sent yet (no 'got TERM'), it sends
the 'd' command to runsv.  Does this work for you?

Regards, Gerrit.


diff --git a/src/sv.c b/src/sv.c
index 8269ca7..1a9b4b9 100644
--- a/src/sv.c
+++ b/src/sv.c
@@ -244,7 +244,8 @@ int check(char *a) {
 }
 int control(char *a) {
   if (svstatus_get() <= 0) return(-1);
-  if (svstatus[17] == *a) return(0);
+  if (svstatus[17] == *a)
+    if (*a != 'd' || svstatus[18] == 1) return(0);
   if ((fd =open_write("supervise/control")) == -1) {
     if (errno != error_nodevice)
       warn("unable to open supervise/control");

Re: sv sometimes won't issue down request

[Earl Chew]

Gerrit Pape wrote:
>> Denys Vlasenko wrote:
>>
>>> I propose just removing this test:
>>>
>>>         if (svstatus.want == *a)
>>>                 return 0;

Gerrit,

My original proposed patch (for the BusyBox implementation) was:

+	if (svstatus.want == *a) {
+		if (*a != 'd' || ! svstatus.pid_le32)
+			return 0;
+	}

which is very similar to your proposed patch:

>    if (svstatus_get() <= 0) return(-1);
> -  if (svstatus[17] == *a) return(0);
> +  if (svstatus[17] == *a)
> +    if (*a != 'd' || svstatus[18] == 1) return(0);


Is it preferred to test for svstatus[18] rather than checking for an
active PID ?


Are you saying that we're better of checking for this "special case"
rather than removing the optimisation in its entirety?


Earl

Re: Bug for sv status in lsb mode

[Erik Wright]

Gerrit Pape <pape <at> smarden.org> writes:

> 
> Mathieu Poumeyrol <kali <at> zoy.org> writes:
> > Le 13 juil. 09 à 16:13, Mathieu Poumeyrol a écrit :
> > > Le 13 juil. 09 à 15:42, Charlie Brady a écrit :
> > > It's not explicit here, but required for LSB compliance.
> > >
> > > see
>
http://refspecs.freestandards.org/LSB_3.1.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html
> > >
> > > As a matter of fact, sv code looks like it is meant to achieve this,  
> > > but fails because of the side effect on the "r" global.
> > 
> > Sorry, it is actually explicit in the "Exit codes" last paragraph...
> > 
> > If the command is status, it exits 3 if the service is down, and 4 if  
> > the status is unknown. It exits 2 on wrong usage, and 151 on error.
> 
> Hi, this is a bug in the documentation then, the complete paragraph is
> 
>  If  sv  is called with a base name other than sv, it exits 1 on timeout
>  or trouble sending the command.  If the command is status, it  exits  3
>  if  the service is down, and 4 if the status is unknown.  It exits 2 on
>  wrong usage, and 151 on error.
> 
> These exit codes only apply if sv is called with a different basename.
> 
>  # sv stop getty-5
>  ok: down: getty-5: 0s, normally up
>  # ln -s $(which sv) /tmp/getty-5
>  # /tmp/getty-5 status
>  down: getty-5: 4s, normally up
>  # echo $?
>  3
>  #
> 
> Any suggestions on how to improve the man page?
> 
> Thanks, Gerrit.
> 
> 


Hi,

This is a follow-up to a thread from last summer. Mathieu Poumeyrol reported
that 'sv status' returned 0 even when the service was down. Gerrit replied that
this was expected behaviour when sv is called with a basename of 'sv'.

Unfortunately, I'm seeing this behaviour even when calling sv with another
basename. To illustrate, here is a shell session snippet on Ubuntu:

--------
root@domU-12-31-39-0C-1D-B1:~# ls -l /etc/init.d/celeryd 
lrwxrwxrwx 1 root root 11 2010-05-14 18:42 /etc/init.d/celeryd -> /usr/bin/sv
root@domU-12-31-39-0C-1D-B1:~# /etc/init.d/celeryd status; echo $?
down: celeryd: 1576s, normally up; run: log: (pid 711) 1043375s
0
root@domU-12-31-39-0C-1D-B1:~# dpkg -s runit
Package: runit
...
Version: 2.0.0-1ubuntu3.1
root@domU-12-31-39-0C-1D-B1:~# cat /etc/issue
Ubuntu 9.10 \n \l
--------

Any ideas what might be causing this?

Thanks,

Erik

Re: Bug for sv status in lsb mode

[Gerrit Pape]

Erik Wright <erik <at> akoha.com> writes:
> This is a follow-up to a thread from last summer. Mathieu Poumeyrol reported
> that 'sv status' returned 0 even when the service was down. Gerrit replied that
> this was expected behaviour when sv is called with a basename of 'sv'.
> 
> Unfortunately, I'm seeing this behaviour even when calling sv with another
> basename. To illustrate, here is a shell session snippet on Ubuntu:
> 
> --------
> root <at> domU-12-31-39-0C-1D-B1:~# ls -l /etc/init.d/celeryd 
> lrwxrwxrwx 1 root root 11 2010-05-14 18:42 /etc/init.d/celeryd -> /usr/bin/sv
> root <at> domU-12-31-39-0C-1D-B1:~# /etc/init.d/celeryd status; echo $?
> down: celeryd: 1576s, normally up; run: log: (pid 711) 1043375s
> 0
> root <at> domU-12-31-39-0C-1D-B1:~# dpkg -s runit
> Package: runit
> ...
> Version: 2.0.0-1ubuntu3.1
> root <at> domU-12-31-39-0C-1D-B1:~# cat /etc/issue
> Ubuntu 9.10 \n \l
> --------
> 
> Any ideas what might be causing this?

Hi, there was a bug discovered in this very thread, which has been fixed
in runit version 2.1.0:

  * sv.c: fix exit code of status command in lsb mode (thx Mathieu
    Poumeyrol)

If you upgrade to the latest version, it should work fine.

Regards, Gerrit.

Re: Bug for sv status in lsb mode

[Erik Wright]

On 3-Jun-10, at 5:04 AM, Gerrit Pape wrote:

> Erik Wright <erik <at> akoha.com> writes:
>> This is a follow-up to a thread from last summer. Mathieu Poumeyrol  
>> reported
>> that 'sv status' returned 0 even when the service was down. Gerrit  
>> replied that
>> this was expected behaviour when sv is called with a basename of  
>> 'sv'.
>>
>> Unfortunately, I'm seeing this behaviour even when calling sv with  
>> another
>> basename. To illustrate, here is a shell session snippet on Ubuntu:
>>
>> --------
>> root <at> domU-12-31-39-0C-1D-B1:~# ls -l /etc/init.d/celeryd
>> lrwxrwxrwx 1 root root 11 2010-05-14 18:42 /etc/init.d/celeryd -> / 
>> usr/bin/sv
>> root <at> domU-12-31-39-0C-1D-B1:~# /etc/init.d/celeryd status;  
>> echo $?
>> down: celeryd: 1576s, normally up; run: log: (pid 711) 1043375s
>> 0
>> root <at> domU-12-31-39-0C-1D-B1:~# dpkg -s runit
>> Package: runit
>> ...
>> Version: 2.0.0-1ubuntu3.1
>> root <at> domU-12-31-39-0C-1D-B1:~# cat /etc/issue
>> Ubuntu 9.10 \n \l
>> --------
>>
>> Any ideas what might be causing this?
>
> Hi, there was a bug discovered in this very thread, which has been  
> fixed
> in runit version 2.1.0:
>
>  * sv.c: fix exit code of status command in lsb mode (thx Mathieu
>    Poumeyrol)
>
> If you upgrade to the latest version, it should work fine.
>
> Regards, Gerrit.
>

Great news, Gerrit. Thanks!

Is there a public source repository or change-log where these commit  
messages / release notes are available?

-Erik