hush: initial trap support

[Denys Vlasenko]

On Sunday 29 March 2009 12:16, Mike Frysinger wrote:
> thinking about things pathologically, this version should address race 
> conditions in updating of existing handlers

Nitpicking...

+       for (i = 0; i < ARRAY_SIZE(traps); ++i)
+               if (traps[i].sig == sig) {
+                       argv[1] = traps[i].cmd;
+                       break;
+               }

if you have if() {} inside for(), add {} to for() too.
Otherwise, in longer loops it gets easy to get it wrong.


+       if (!argv[1] || !*argv[1])
+               return;

*argv[1] is more confusing than argv[1][0].


+                                               if (!old_cmd)
+                                                       sigaction(sig, NULL, &traps[i].oact);
+                                               sigaction_set(sig, &act);

Combine it into one: sigaction(sig, &act, old_cmd ? NULL : &traps[i].oact)


+       if (!strcmp(argv[1], ""))

!strcmp(argv[1], "abc") reads as 'argv[1] is not "abc"' -> bad.
strcmp(argv[1], "abc") == 0 is less confusing, it suggests equality.
For "", even better is argv[1][0] == '\0'.


More substantial notes:

handle_trap is a signal handler and therefore must preserve errno.


+struct {
+       int sig;
+       const char *name;
+       char *cmd;
+       struct sigaction oact;
+} traps[] = {
+       { 0,       "EXIT", },
+       { SIGABRT, "ABRT", },

(a) I prefer all data to go to struct globals, never to .data.
(b) It would be nice to allocate it on first use, it's big
    (so that scripts which never use traps don't waste memory for it).
(c) int sig; const char *name; is wasting 8 bytes,
    uint8_t sig; char name[7]; is more compact.
(d) libbb has get_signum(str) which already does signal name or number ->
    signal number conversion. get_signame(num) is an opposite convertor.
    Use them, this allows you to not duplicate signal names.


Add a comment somewhere that we improperly execute traps at once
(even within, say, malloc() calls... which is rather bad)
while we need to wait for latest command completion.
--
vda