Re: wrong exit code for sv status in lsb mode

Re: wrong exit code for sv status in lsb mode

[David Mountney via supervision]

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

This looks like it's still a problem introduced in runit 2.1.2

If you have defined a log handler, only the status of the log handler is
returned by sv status.

Its not clear to me from the changelog why this change was made.

I guess, ideally if the service status was successful, you could then also
check the log status, but as it is now, if the service is down, but the log
is up, sv status reports all is well for its exit codes.

I've seen other references to this bug, where projects have reverted to the
previous runit version: https://github.com/chef/omnibus-software/pull/793

It would be ideal if we could just get this fixed.


> From: Fabian Ruff
> Date: Tue, 18 Aug 2015 23:33:50 +0200
>  Tue, 18 Aug 2015 23:33:50 +0200
> Hi,
> I just noticed that the exit code of sv status in "lsb" mode is not 3 in
> all cases when
> the service is "down".
> The problem arises when a service defines a log handler. In that case the
> return code
> of svstatus print for the log process overwrites the return code of
> svstatus print of
> the actual supervised process (line 170 of sv.c).
> Is this intentional? Looks like a bug to me.
> Kind regards,

Re: wrong exit code for sv status in lsb mode

[Gerrit Pape]

Hi,

donations for my free software increased recently, not sure why, but
thanks.

runit upstream sources with history still are available through

 $ git clone http://smarden.org/git/runit.git

Below is the diff for sv.c from 2.1.1 to 2.1.2.  Possibly anyone can
spot the error and create a patch if that issue is annoying.

Regards, Gerrit.


$ git diff v2.1.1..v2.1.2 src/sv.c
diff --git a/src/sv.c b/src/sv.c
index d126cee..9003142 100644
--- a/src/sv.c
+++ b/src/sv.c
@@ -41,6 +41,7 @@ unsigned int lsb;
 unsigned int verbose =0;
 unsigned long wait =7;
 unsigned int kll =0;
+unsigned int islog =0;
 struct taia tstart, tnow, tdiff;
 struct tai tstatus;
 
@@ -67,6 +68,7 @@ void fatal2(char *m1, char *m2) {
 void out(char *p, char *m1) {
   buffer_puts(buffer_1, p);
   buffer_puts(buffer_1, *service);
+  if (islog) buffer_puts(buffer_1, "/log");
   buffer_puts(buffer_1, ": ");
   buffer_puts(buffer_1, m1);
   if (errno) {
@@ -153,20 +155,22 @@ int status(char *unused) {
   int rc;
 
   rc =svstatus_get();
-  switch(r) { case -1: if (lsb) done(4); case 0: return(0); }
+  switch(rc) { case -1: if (lsb) done(4); case 0: return(0); }
   rc =svstatus_print(*service);
+  islog =1;
   if (chdir("log") == -1) {
     if (errno != error_noent) {
-      outs("; log: "); outs(WARN);
-      outs("unable to change to log service directory: ");
-      outs(error_str(errno));
+      outs("; ");
+      warn("unable to change directory");
     }
+    else outs("\n");
   }
-  else
-    if (svstatus_get()) {
-      outs("; "); svstatus_print("log");
-    }
-  flush("\n");
+  else {
+    outs("; ");
+    if (svstatus_get()) { rc =svstatus_print("log"); outs("\n"); }
+  }
+  islog =0;
+  flush("");
   if (lsb) switch(rc) { case 1: done(0); case 2: done(3); case 0: done(4); }
   return(rc);
 }
@@ -305,9 +309,11 @@ int main(int argc, char **argv) {
     acts ="d"; kll =1; cbk =✓ break;
   case 'T':
     acts ="tc"; kll =1; cbk =✓ break;
+  case 't':
+    if (!str_diff(action, "try-restart")) { acts ="tc"; cbk =✓ break; }
   case 'c':
     if (!str_diff(action, "check")) { act =0; acts ="C"; cbk =✓ break; }
-  case 'u': case 'd': case 'o': case 't': case 'p': case 'h':
+  case 'u': case 'd': case 'o': case 'p': case 'h':
   case 'a': case 'i': case 'k': case 'q': case '1': case '2':
     action[1] =0; acts =action; break;
   case 's':
@@ -318,6 +324,7 @@ int main(int argc, char **argv) {
     act =&status; cbk =0; break;
   case 'r':
     if (!str_diff(action, "restart")) { acts ="tcu"; cbk =✓ break; }
+    if (!str_diff(action, "reload")) { acts ="h"; cbk =✓ break; }
     usage();
   case 'f':
     if (!str_diff(action, "force-reload"))
$ 


On Fri, Jan 04, 2019 at 05:10:13PM -0800, David Mountney via supervision wrote:
> This looks like it's still a problem introduced in runit 2.1.2
> 
> If you have defined a log handler, only the status of the log handler is
> returned by sv status.
> 
> Its not clear to me from the changelog why this change was made.
> 
> I guess, ideally if the service status was successful, you could then also
> check the log status, but as it is now, if the service is down, but the log
> is up, sv status reports all is well for its exit codes.
> 
> I've seen other references to this bug, where projects have reverted to the
> previous runit version: https://github.com/chef/omnibus-software/pull/793
> 
> It would be ideal if we could just get this fixed.
> 
> 
> > From: Fabian Ruff
> > Date: Tue, 18 Aug 2015 23:33:50 +0200
> >  Tue, 18 Aug 2015 23:33:50 +0200
> > Hi,
> > I just noticed that the exit code of sv status in "lsb" mode is not 3 in
> > all cases when
> > the service is "down".
> > The problem arises when a service defines a log handler. In that case the
> > return code
> > of svstatus print for the log process overwrites the return code of
> > svstatus print of
> > the actual supervised process (line 170 of sv.c).
> > Is this intentional? Looks like a bug to me.
> > Kind regards,

Re: wrong exit code for sv status in lsb mode

[David Mountney via supervision]

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

The error was introduced in commit 5fe1bc773c2d979093fe4b1f3ecbbed5e6acdaf0
"sv.c: properly format status command's output on failure cases."

Where the log status is being saved to the same variable as the service
status. The quick fix would be to not record the log status for now:

$ git diff src/sv.c
diff --git a/src/sv.c b/src/sv.c
index 9003142..1676227 100644
--- a/src/sv.c
+++ b/src/sv.c
@@ -167,7 +167,7 @@ int status(char *unused) {
   }
   else {
     outs("; ");
-    if (svstatus_get()) { rc =svstatus_print("log"); outs("\n"); }
+    if (svstatus_get()) { svstatus_print("log"); outs("\n"); }
   }
   islog =0;
   flush("");
$

On Tue, Jan 8, 2019 at 12:18 AM Gerrit Pape <pape@smarden.org> wrote:

> Hi,
>
> donations for my free software increased recently, not sure why, but
> thanks.
>
> runit upstream sources with history still are available through
>
>  $ git clone http://smarden.org/git/runit.git
>
> Below is the diff for sv.c from 2.1.1 to 2.1.2.  Possibly anyone can
> spot the error and create a patch if that issue is annoying.
>
> Regards, Gerrit.
>
>
> $ git diff v2.1.1..v2.1.2 src/sv.c
> diff --git a/src/sv.c b/src/sv.c
> index d126cee..9003142 100644
> --- a/src/sv.c
> +++ b/src/sv.c
> @@ -41,6 +41,7 @@ unsigned int lsb;
>  unsigned int verbose =0;
>  unsigned long wait =7;
>  unsigned int kll =0;
> +unsigned int islog =0;
>  struct taia tstart, tnow, tdiff;
>  struct tai tstatus;
>
> @@ -67,6 +68,7 @@ void fatal2(char *m1, char *m2) {
>  void out(char *p, char *m1) {
>    buffer_puts(buffer_1, p);
>    buffer_puts(buffer_1, *service);
> +  if (islog) buffer_puts(buffer_1, "/log");
>    buffer_puts(buffer_1, ": ");
>    buffer_puts(buffer_1, m1);
>    if (errno) {
> @@ -153,20 +155,22 @@ int status(char *unused) {
>    int rc;
>
>    rc =svstatus_get();
> -  switch(r) { case -1: if (lsb) done(4); case 0: return(0); }
> +  switch(rc) { case -1: if (lsb) done(4); case 0: return(0); }
>    rc =svstatus_print(*service);
> +  islog =1;
>    if (chdir("log") == -1) {
>      if (errno != error_noent) {
> -      outs("; log: "); outs(WARN);
> -      outs("unable to change to log service directory: ");
> -      outs(error_str(errno));
> +      outs("; ");
> +      warn("unable to change directory");
>      }
> +    else outs("\n");
>    }
> -  else
> -    if (svstatus_get()) {
> -      outs("; "); svstatus_print("log");
> -    }
> -  flush("\n");
> +  else {
> +    outs("; ");
> +    if (svstatus_get()) { rc =svstatus_print("log"); outs("\n"); }
> +  }
> +  islog =0;
> +  flush("");
>    if (lsb) switch(rc) { case 1: done(0); case 2: done(3); case 0:
> done(4); }
>    return(rc);
>  }
> @@ -305,9 +309,11 @@ int main(int argc, char **argv) {
>      acts ="d"; kll =1; cbk =&check; break;
>    case 'T':
>      acts ="tc"; kll =1; cbk =&check; break;
> +  case 't':
> +    if (!str_diff(action, "try-restart")) { acts ="tc"; cbk =&check;
> break; }
>    case 'c':
>      if (!str_diff(action, "check")) { act =0; acts ="C"; cbk =&check;
> break; }
> -  case 'u': case 'd': case 'o': case 't': case 'p': case 'h':
> +  case 'u': case 'd': case 'o': case 'p': case 'h':
>    case 'a': case 'i': case 'k': case 'q': case '1': case '2':
>      action[1] =0; acts =action; break;
>    case 's':
> @@ -318,6 +324,7 @@ int main(int argc, char **argv) {
>      act =&status; cbk =0; break;
>    case 'r':
>      if (!str_diff(action, "restart")) { acts ="tcu"; cbk =&check; break; }
> +    if (!str_diff(action, "reload")) { acts ="h"; cbk =&check; break; }
>      usage();
>    case 'f':
>      if (!str_diff(action, "force-reload"))
> $
>
>
> On Fri, Jan 04, 2019 at 05:10:13PM -0800, David Mountney via supervision
> wrote:
> > This looks like it's still a problem introduced in runit 2.1.2
> >
> > If you have defined a log handler, only the status of the log handler is
> > returned by sv status.
> >
> > Its not clear to me from the changelog why this change was made.
> >
> > I guess, ideally if the service status was successful, you could then
> also
> > check the log status, but as it is now, if the service is down, but the
> log
> > is up, sv status reports all is well for its exit codes.
> >
> > I've seen other references to this bug, where projects have reverted to
> the
> > previous runit version:
> https://github.com/chef/omnibus-software/pull/793
> >
> > It would be ideal if we could just get this fixed.
> >
> >
> > > From: Fabian Ruff
> > > Date: Tue, 18 Aug 2015 23:33:50 +0200
> > >  Tue, 18 Aug 2015 23:33:50 +0200
> > > Hi,
> > > I just noticed that the exit code of sv status in "lsb" mode is not 3
> in
> > > all cases when
> > > the service is "down".
> > > The problem arises when a service defines a log handler. In that case
> the
> > > return code
> > > of svstatus print for the log process overwrites the return code of
> > > svstatus print of
> > > the actual supervised process (line 170 of sv.c).
> > > Is this intentional? Looks like a bug to me.
> > > Kind regards,
>

Re: wrong exit code for sv status in lsb mode

[Gerrit Pape]

On Tue, Jan 08, 2019 at 08:19:30AM -0800, David Mountney via supervision wrote:
> The error was introduced in commit 5fe1bc773c2d979093fe4b1f3ecbbed5e6acdaf0
> "sv.c: properly format status command's output on failure cases."
> 
> Where the log status is being saved to the same variable as the service
> status. The quick fix would be to not record the log status for now:
> 
> $ git diff src/sv.c
> diff --git a/src/sv.c b/src/sv.c
> index 9003142..1676227 100644
> --- a/src/sv.c
> +++ b/src/sv.c
> @@ -167,7 +167,7 @@ int status(char *unused) {
>    }
>    else {
>      outs("; ");
> -    if (svstatus_get()) { rc =svstatus_print("log"); outs("\n"); }
> +    if (svstatus_get()) { svstatus_print("log"); outs("\n"); }
>    }
>    islog =0;
>    flush("");
> $

Thanks for the patch, David.

I'm looking forward to do a maintenance release of runit eventually and
am collecting patches.  I'm about to apply this one, has anyone applied
it already and can provide feedback?

Regards, Gerrit.

Re: wrong exit code for sv status in lsb mode

[David Mountney via supervision]

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

We went ahead and applied the patch in GitLab back in January:
https://gitlab.com/gitlab-org/omnibus-gitlab/commit/cdecb4ecc52f8f9aa845df5e73473f2b3af291b0
And it's worked fine for our use of it there.

On Wed, Mar 13, 2019 at 1:11 AM Gerrit Pape <pape@smarden.org> wrote:

> On Tue, Jan 08, 2019 at 08:19:30AM -0800, David Mountney via supervision
> wrote:
> > The error was introduced in commit
> 5fe1bc773c2d979093fe4b1f3ecbbed5e6acdaf0
> > "sv.c: properly format status command's output on failure cases."
> >
> > Where the log status is being saved to the same variable as the service
> > status. The quick fix would be to not record the log status for now:
> >
> > $ git diff src/sv.c
> > diff --git a/src/sv.c b/src/sv.c
> > index 9003142..1676227 100644
> > --- a/src/sv.c
> > +++ b/src/sv.c
> > @@ -167,7 +167,7 @@ int status(char *unused) {
> >    }
> >    else {
> >      outs("; ");
> > -    if (svstatus_get()) { rc =svstatus_print("log"); outs("\n"); }
> > +    if (svstatus_get()) { svstatus_print("log"); outs("\n"); }
> >    }
> >    islog =0;
> >    flush("");
> > $
>
> Thanks for the patch, David.
>
> I'm looking forward to do a maintenance release of runit eventually and
> am collecting patches.  I'm about to apply this one, has anyone applied
> it already and can provide feedback?
>
> Regards, Gerrit.
>

Re: wrong exit code for sv status in lsb mode

[James Byrne]

Hi Gerrit,

On 13/03/2019 08:04, Gerrit Pape wrote:
> I'm looking forward to do a maintenance release of runit eventually and
> am collecting patches.

I'm glad to hear that you're collecting patches for a maintenance
release. In case you don't already have these, please can I ask you to
consider the following two, which I submitted to the supervision list
back in 2015:

http://www.mail-archive.com/supervision@list.skarnet.org/msg00500.html

http://www.mail-archive.com/supervision@list.skarnet.org/msg00501.html

Thanks!

James
This email is from Origami Energy Limited. The contents of this email and any attachment are confidential to the intended recipient(s). If you are not an intended recipient: (i) do not use, disclose, distribute, copy or publish this email or its contents; (ii) please contact Origami Energy Limited immediately; and then (iii) delete this email. For more information, our privacy policy is available here: https://origamienergy.com/privacy-policy/. Origami Energy Limited (company number 8619644) is a company registered in England with its registered office at Ashcombe Court, Woolsack Way, Godalming, GU7 1LQ.