Re: mg: extract child status with WEXITSTATUS

[Todd C. Miller]

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

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.c12 Oct 2017 14:12:00 -  1.45
> +++ grep.c9 Jan 2018 15:23:35 -
> @@ -4,6 +4,8 @@
>  
>  #include 
>  #include 
> +#include 
> +
>  #include 
>  #include 
>  #include 
> @@ -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());
>   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");

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 -  1.45
+++ grep.c  9 Jan 2018 15:23:35 -
@@ -4,6 +4,8 @@
 
 #include 
 #include 
+#include 
+
 #include 
 #include 
 #include 
@@ -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());
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");

Re: mg: extract child status with WEXITSTATUS

[Jeremie Courreges-Anglas]

On Tue, Jan 02 2018, Scott Cheloha  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  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 -  1.45
> +++ usr.bin/mg/grep.c 3 Jan 2018 01:24:09 -
> @@ -4,6 +4,8 @@
>  
>  #include 
>  #include 
> +#include 
> +
>  #include 
>  #include 
>  #include 
> @@ -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());
>   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=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 -  1.45
+++ grep.c  5 Jan 2018 06:36:53 -
@@ -4,6 +4,8 @@
 
 #include 
 #include 
+#include 
+
 #include 
 #include 
 #include 
@@ -226,10 +228,14 @@ compile_mode(const char *name, const cha
t = time(NULL);
strftime(timestr, sizeof(timestr), "%a %b %e %T %Y", localtime());
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

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  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 -  1.45
+++ usr.bin/mg/grep.c   3 Jan 2018 01:24:09 -
@@ -4,6 +4,8 @@
 
 #include 
 #include 
+#include 
+
 #include 
 #include 
 #include 
@@ -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());
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) {

Re: mg: extract child status with WEXITSTATUS

[Todd C. Miller]

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  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