mg: extract child status with WEXITSTATUS

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

mg: extract child status with WEXITSTATUS

Scott Cheloha
Hey,

In the mg(1) *compile* buffer, currently you get incorrect
output like:

        Command exited abnormally with code 256 at [...]

Using the W* macros in <sys/wait.h> corrects this:

        Command exited abnormally with code 1 at [...]

ok?

--
Scott Cheloha

Index: usr.bin/mg/grep.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/grep.c,v
retrieving revision 1.45
diff -u -p -r1.45 grep.c
--- usr.bin/mg/grep.c 12 Oct 2017 14:12:00 -0000 1.45
+++ usr.bin/mg/grep.c 2 Jan 2018 01:42:11 -0000
@@ -4,6 +4,8 @@
 
 #include <sys/queue.h>
 #include <sys/types.h>
+#include <sys/wait.h>
+
 #include <ctype.h>
 #include <libgen.h>
 #include <limits.h>
@@ -180,7 +182,7 @@ compile_mode(const char *name, const cha
  char *buf;
  size_t sz;
  ssize_t len;
- int ret, n;
+ int ret, n, status;
  char cwd[NFILEN], qcmd[NFILEN];
  char timestr[NTIME];
  time_t t;
@@ -223,12 +225,13 @@ compile_mode(const char *name, const cha
  if (ferror(fpipe))
  ewprintf("Problem reading pipe");
  ret = pclose(fpipe);
+ status = WIFEXITED(ret) ? WEXITSTATUS(ret) : 128 + WTERMSIG(ret);
  t = time(NULL);
  strftime(timestr, sizeof(timestr), "%a %b %e %T %Y", localtime(&t));
  addline(bp, "");
- if (ret != 0)
+ if (status != 0)
  addlinef(bp, "Command exited abnormally with code %d"
-    " at %s", ret, timestr);
+    " at %s", status, timestr);
  else
  addlinef(bp, "Command finished at %s", timestr);
 

Reply | Threaded
Open this post in threaded view
|

Re: mg: extract child status with WEXITSTATUS

Todd C. Miller-2
On Mon, 01 Jan 2018 19:54:07 -0600, Scott Cheloha wrote:

> Hey,
>
> In the mg(1) *compile* buffer, currently you get incorrect
> output like:
>
> Command exited abnormally with code 256 at [...]
>
> Using the W* macros in <sys/wait.h> corrects this:
>
> Command exited abnormally with code 1 at [...]

Is it worth using an explicit message if the command was terminated
by a signal?

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: mg: extract child status with WEXITSTATUS

Scott Cheloha
On Mon, Jan 01, 2018 at 09:07:25PM -0700, Todd C. Miller wrote:

> On Mon, 01 Jan 2018 19:54:07 -0600, Scott Cheloha wrote:
>
> > Hey,
> >
> > In the mg(1) *compile* buffer, currently you get incorrect
> > output like:
> >
> > Command exited abnormally with code 256 at [...]
> >
> > Using the W* macros in <sys/wait.h> corrects this:
> >
> > Command exited abnormally with code 1 at [...]
>
> Is it worth using an explicit message if the command was terminated
> by a signal?

Like in lieu of 128+WTERMSIG?  I don't personally see my jobs in mg
get killed all that often, but if I did I think I'd prefer something
with the signal name, sure.

While we're at it, I'd like to move the timestamp left so it's separate
from the other output.  I'd also like to always print the exit status,
as "abnormally" is inapplicable for programs like diff and grep.

Thoughts?

--
Scott Cheloha

Index: usr.bin/mg/grep.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/grep.c,v
retrieving revision 1.45
diff -u -p -r1.45 grep.c
--- usr.bin/mg/grep.c 12 Oct 2017 14:12:00 -0000 1.45
+++ usr.bin/mg/grep.c 3 Jan 2018 01:24:09 -0000
@@ -4,6 +4,8 @@
 
 #include <sys/queue.h>
 #include <sys/types.h>
+#include <sys/wait.h>
+
 #include <ctype.h>
 #include <libgen.h>
 #include <limits.h>
@@ -180,7 +182,7 @@ compile_mode(const char *name, const cha
  char *buf;
  size_t sz;
  ssize_t len;
- int ret, n;
+ int ret, n, signo;
  char cwd[NFILEN], qcmd[NFILEN];
  char timestr[NTIME];
  time_t t;
@@ -226,17 +228,19 @@ compile_mode(const char *name, const cha
  t = time(NULL);
  strftime(timestr, sizeof(timestr), "%a %b %e %T %Y", localtime(&t));
  addline(bp, "");
- if (ret != 0)
- addlinef(bp, "Command exited abnormally with code %d"
-    " at %s", ret, timestr);
- else
- addlinef(bp, "Command finished at %s", timestr);
+ if (WIFEXITED(ret)) {
+ addlinef(bp, "[%s] Command exited with status %d",
+    timestr, WEXITSTATUS(ret));
+ } else {
+ signo = WTERMSIG(ret);
+ addlinef(bp, "[%s] Command killed by %s: %s",
+    timestr, sys_signame[signo], strsignal(signo));
+ }
 
  bp->b_dotp = bfirstlp(bp);
  bp->b_modes[0] = name_mode("fundamental");
  bp->b_modes[1] = name_mode("compile");
  bp->b_nmodes = 1;
-
  compile_buffer = bp;
 
  if (chdir(cwd) == -1) {

Reply | Threaded
Open this post in threaded view
|

Re: mg: extract child status with WEXITSTATUS

Jeremie Courreges-Anglas-2
On Tue, Jan 02 2018, Scott Cheloha <[hidden email]> wrote:

> On Mon, Jan 01, 2018 at 09:07:25PM -0700, Todd C. Miller wrote:
>> On Mon, 01 Jan 2018 19:54:07 -0600, Scott Cheloha wrote:
>>
>> > Hey,
>> >
>> > In the mg(1) *compile* buffer, currently you get incorrect
>> > output like:
>> >
>> > Command exited abnormally with code 256 at [...]
>> >
>> > Using the W* macros in <sys/wait.h> corrects this:
>> >
>> > Command exited abnormally with code 1 at [...]
>>
>> Is it worth using an explicit message if the command was terminated
>> by a signal?
>
> Like in lieu of 128+WTERMSIG?  I don't personally see my jobs in mg
> get killed all that often, but if I did I think I'd prefer something
> with the signal name, sure.
>
> While we're at it, I'd like to move the timestamp left so it's separate
> from the other output.  I'd also like to always print the exit status,
> as "abnormally" is inapplicable for programs like diff and grep.

"abnormally" doesn't seem very useful if the status is printed indeed;
printing the status if zero doesn't look very useful though.

> Thoughts?

Disclaimer: I'm not an mg(1) user, but please see below.

> --
> Scott Cheloha
>
> Index: usr.bin/mg/grep.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mg/grep.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 grep.c
> --- usr.bin/mg/grep.c 12 Oct 2017 14:12:00 -0000 1.45
> +++ usr.bin/mg/grep.c 3 Jan 2018 01:24:09 -0000
> @@ -4,6 +4,8 @@
>  
>  #include <sys/queue.h>
>  #include <sys/types.h>
> +#include <sys/wait.h>
> +
>  #include <ctype.h>
>  #include <libgen.h>
>  #include <limits.h>
> @@ -180,7 +182,7 @@ compile_mode(const char *name, const cha
>   char *buf;
>   size_t sz;
>   ssize_t len;
> - int ret, n;
> + int ret, n, signo;
>   char cwd[NFILEN], qcmd[NFILEN];
>   char timestr[NTIME];
>   time_t t;
> @@ -226,17 +228,19 @@ compile_mode(const char *name, const cha
>   t = time(NULL);
>   strftime(timestr, sizeof(timestr), "%a %b %e %T %Y", localtime(&t));
>   addline(bp, "");
> - if (ret != 0)
> - addlinef(bp, "Command exited abnormally with code %d"
> -    " at %s", ret, timestr);
> - else
> - addlinef(bp, "Command finished at %s", timestr);
> + if (WIFEXITED(ret)) {
> + addlinef(bp, "[%s] Command exited with status %d",
> +    timestr, WEXITSTATUS(ret));
> + } else {

This won't catch cases where the shell exits with 128 + the signal that
killed its child process.

> + signo = WTERMSIG(ret);
> + addlinef(bp, "[%s] Command killed by %s: %s",
> +    timestr, sys_signame[signo], strsignal(signo));

I'm not thrilled by sys_signame, it's not portable, you need to do make
sure that the signal number is valid, and when adding errno values the size
of sys_signame changes -> libc major crank.  It's a shame there are no
sane standard accessors.

  (http://austingroupbugs.net/view.php?id=1138&nbn=8)

Sorry for the bikeshed but wouldn't just printing the signal
number be enough?  Also, why change the way the timestamp is printed?

I would probably do something like the diff below.


Index: grep.c
===================================================================
RCS file: /d/cvs/src/usr.bin/mg/grep.c,v
retrieving revision 1.45
diff -u -p -p -u -r1.45 grep.c
--- grep.c 12 Oct 2017 14:12:00 -0000 1.45
+++ grep.c 5 Jan 2018 06:36:53 -0000
@@ -4,6 +4,8 @@
 
 #include <sys/queue.h>
 #include <sys/types.h>
+#include <sys/wait.h>
+
 #include <ctype.h>
 #include <libgen.h>
 #include <limits.h>
@@ -226,10 +228,14 @@ compile_mode(const char *name, const cha
  t = time(NULL);
  strftime(timestr, sizeof(timestr), "%a %b %e %T %Y", localtime(&t));
  addline(bp, "");
- if (ret != 0)
- addlinef(bp, "Command exited abnormally with code %d"
-    " at %s", ret, timestr);
- else
+ if (WIFSIGNALED(ret) || WEXITSTATUS(ret) > 128) {
+ addlinef(bp, "Command killed by signal %d at %s",
+    WIFSIGNALED(ret) ? WTERMSIG(ret) : WEXITSTATUS(ret) - 128,
+    timestr);
+ } else if (WEXITSTATUS(ret)) {
+ addlinef(bp, "Command exited with status %d at %s",
+    WEXITSTATUS(ret), timestr);
+ } else
  addlinef(bp, "Command finished at %s", timestr);
 
  bp->b_dotp = bfirstlp(bp);

--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Reply | Threaded
Open this post in threaded view
|

Re: mg: extract child status with WEXITSTATUS

Scott Cheloha
After discussing it with jca@ and trying a few variations I've settled
on the attached diff.

We indicate if sh(1) was signalled because we have that information at
hand.  Otherwise we report the exit status as we always have.

ok?

Index: grep.c
===================================================================
RCS file: /cvs/src/usr.bin/mg/grep.c,v
retrieving revision 1.45
diff -u -p -r1.45 grep.c
--- grep.c 12 Oct 2017 14:12:00 -0000 1.45
+++ grep.c 9 Jan 2018 15:23:35 -0000
@@ -4,6 +4,8 @@
 
 #include <sys/queue.h>
 #include <sys/types.h>
+#include <sys/wait.h>
+
 #include <ctype.h>
 #include <libgen.h>
 #include <limits.h>
@@ -180,7 +182,7 @@ compile_mode(const char *name, const cha
  char *buf;
  size_t sz;
  ssize_t len;
- int ret, n;
+ int ret, n, status;
  char cwd[NFILEN], qcmd[NFILEN];
  char timestr[NTIME];
  time_t t;
@@ -226,11 +228,16 @@ compile_mode(const char *name, const cha
  t = time(NULL);
  strftime(timestr, sizeof(timestr), "%a %b %e %T %Y", localtime(&t));
  addline(bp, "");
- if (ret != 0)
- addlinef(bp, "Command exited abnormally with code %d"
-    " at %s", ret, timestr);
- else
- addlinef(bp, "Command finished at %s", timestr);
+ if (WIFEXITED(ret)) {
+ status = WEXITSTATUS(ret);
+ if (status == 0)
+ addlinef(bp, "Command finished at %s", timestr);
+ else
+ addlinef(bp, "Command exited abnormally with code %d "
+    "at %s", status, timestr);
+ } else
+ addlinef(bp, "Subshell killed by signal %d at %s",
+    WTERMSIG(ret), timestr);
 
  bp->b_dotp = bfirstlp(bp);
  bp->b_modes[0] = name_mode("fundamental");

Reply | Threaded
Open this post in threaded view
|

Re: mg: extract child status with WEXITSTATUS

Scott Cheloha
On Tue, Jan 09, 2018 at 10:05:22AM -0600, Scott Cheloha wrote:
> After discussing it with jca@ and trying a few variations I've settled
> on the attached diff.
>
> We indicate if sh(1) was signalled because we have that information at
> hand.  Otherwise we report the exit status as we always have.

... reasoning being that there isn't a nice, portable way to determine
if a given return value >128 is a valid signal, it effectively never
happens anyway, and using the exec builtin for the command run in
compile mode in order to reliably retrieve the signal number would break
conditional execution chains, which are useful and supported by devel/emacs.

> ok?
>
> Index: grep.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/mg/grep.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 grep.c
> --- grep.c 12 Oct 2017 14:12:00 -0000 1.45
> +++ grep.c 9 Jan 2018 15:23:35 -0000
> @@ -4,6 +4,8 @@
>  
>  #include <sys/queue.h>
>  #include <sys/types.h>
> +#include <sys/wait.h>
> +
>  #include <ctype.h>
>  #include <libgen.h>
>  #include <limits.h>
> @@ -180,7 +182,7 @@ compile_mode(const char *name, const cha
>   char *buf;
>   size_t sz;
>   ssize_t len;
> - int ret, n;
> + int ret, n, status;
>   char cwd[NFILEN], qcmd[NFILEN];
>   char timestr[NTIME];
>   time_t t;
> @@ -226,11 +228,16 @@ compile_mode(const char *name, const cha
>   t = time(NULL);
>   strftime(timestr, sizeof(timestr), "%a %b %e %T %Y", localtime(&t));
>   addline(bp, "");
> - if (ret != 0)
> - addlinef(bp, "Command exited abnormally with code %d"
> -    " at %s", ret, timestr);
> - else
> - addlinef(bp, "Command finished at %s", timestr);
> + if (WIFEXITED(ret)) {
> + status = WEXITSTATUS(ret);
> + if (status == 0)
> + addlinef(bp, "Command finished at %s", timestr);
> + else
> + addlinef(bp, "Command exited abnormally with code %d "
> +    "at %s", status, timestr);
> + } else
> + addlinef(bp, "Subshell killed by signal %d at %s",
> +    WTERMSIG(ret), timestr);
>  
>   bp->b_dotp = bfirstlp(bp);
>   bp->b_modes[0] = name_mode("fundamental");

Reply | Threaded
Open this post in threaded view
|

Re: mg: extract child status with WEXITSTATUS

Todd C. Miller-2
In reply to this post by Scott Cheloha
On Tue, 09 Jan 2018 10:05:22 -0600, Scott Cheloha wrote:

> After discussing it with jca@ and trying a few variations I've settled
> on the attached diff.
>
> We indicate if sh(1) was signalled because we have that information at
> hand.  Otherwise we report the exit status as we always have.

Looks good to me.  OK millert@

 - todd