On Tue, Jan 02 2018, Scott Cheloha <scottchel...@gmail.com> 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 via email to