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
