Re: time(1): perror(3) -> err(3) and friends

[Scott Cheloha]

Few weeks bump.

Any feedback on this fabulous Scott Cheloha/ingo@ collaboration?

Slightly re-tweaked patch below; while here:

kill(getpid(), sig) -> raise(sig)

I didn't even know there was a raise(3) function until this weekend,
they really did think of everything.

--
Scott Cheloha

Index: time.c
===
RCS file: /cvs/src/usr.bin/time/time.c,v
retrieving revision 1.24
diff -u -p -r1.24 time.c
--- time.c  22 Jul 2017 17:01:09 -  1.24
+++ time.c  20 Aug 2017 22:12:39 -
@@ -67,7 +67,6 @@ main(int argc, char *argv[])
break;
default:
usage();
-   /* NOTREACHED */
}
}
 
@@ -80,14 +79,12 @@ main(int argc, char *argv[])
clock_gettime(CLOCK_MONOTONIC, );
switch(pid = vfork()) {
case -1:/* error */
-   perror("time");
-   exit(1);
-   /* NOTREACHED */
+   warn("fork");
+   return 1;
case 0: /* child */
execvp(*argv, argv);
-   perror(*argv);
+   warn("%s", *argv);
_exit((errno == ENOENT) ? 127 : 126);
-   /* NOTREACHED */
}
 
/* parent */
@@ -168,11 +165,11 @@ main(int argc, char *argv[])
 
if (exitonsig) {
if (signal(exitonsig, SIG_DFL) == SIG_ERR)
-   perror("signal");
+   return 128 + exitonsig;
else
-   kill(getpid(), exitonsig);
+   raise(exitonsig);
}
-   exit(WIFEXITED(status) ? WEXITSTATUS(status) : EXIT_FAILURE);
+   return WIFEXITED(status) ? WEXITSTATUS(status) : EXIT_FAILURE;
 }
 
 __dead void

Re: time(1): perror(3) -> err(3) and friends

[Scott Cheloha]

> On Jul 22, 2017, at 12:49 PM, Ingo Schwarze  wrote:
> 
> [...]
> 
> * In main(), prefer "return" over exit(3).

Any particular reason for this or is that just the style that people
have settled on?

And to be clear you're saying

warn("whatever");
return 1;

is preferred over

err(1, "whatever");

in main().

--
Scott Cheloha

Re: time(1): perror(3) -> err(3) and friends

[Ingo Schwarze]

Hi,

Scott Cheloha wrote on Thu, Jul 13, 2017 at 07:30:26PM -0500:

> We currently use a mix of perror(3) and err(3).
> 
> In one case you can merge perror + exit into err, which is nice.
> 
> The warns, though, are not equivalent (you get a "time: " prefix),
> so maybe this is too risky.
> 
> Putting it out here anyway.

We can do even better.

 * In main(), prefer "return" over exit(3).

 * This is ugly and confusing:

  $ sh -c 'ulimit -p 29; /usr/bin/time ls' 
 time: Resource temporarily unavailable

   This is easier to understand:

  $ sh -c 'ulimit -p 29; /usr/bin/time ls' 
 time: fork: Resource temporarily unavailable

 * In complex situations, this can be confusing:

  $ /usr/bin/time foo/bar
 foo/bar: No such file or directory

   This is easier to understand:

  $ /usr/bin/time foo/bar 
 time: foo/bar: No such file or directory

 * This is plain nonsense:

  $ ( /usr/bin/time sleep 100 ; echo $? ) & pkill -KILL sleep 
 Command terminated abnormally.
0.00 real 0.00 user 0.00 sys
 signal: Invalid argument
 1

   signal(3) can only fail for SIGKILL or SIGSTOP (or for an invalid
   argument, which cannot happen here).  Printing "Invalid argument"
   is completely pointless, and confusing.

   However, we do want an EXIT STATUS of 128 + 9 in this case:

 $ ( /usr/bin/time sleep 100 ; echo $? ) & pkill -KILL sleep 
Command terminated abnormally.
0.00 real 0.00 user 0.00 sys
137

To wit, look at the builtin:

 $ ( time sleep 100 ; echo $? ) & pkill -KILL sleep 
Killed 
0m00.00s real 0m00.00s user 0m00.00s system
137

OK?
  Ingo

   
Index: time.c
===
RCS file: /cvs/src/usr.bin/time/time.c,v
retrieving revision 1.24
diff -u -p -r1.24 time.c
--- time.c  22 Jul 2017 17:01:09 -  1.24
+++ time.c  22 Jul 2017 17:43:57 -
@@ -67,7 +67,6 @@ main(int argc, char *argv[])
break;
default:
usage();
-   /* NOTREACHED */
}
}
 
@@ -80,14 +79,12 @@ main(int argc, char *argv[])
clock_gettime(CLOCK_MONOTONIC, );
switch(pid = vfork()) {
case -1:/* error */
-   perror("time");
-   exit(1);
-   /* NOTREACHED */
+   warn("fork");
+   return 1;
case 0: /* child */
execvp(*argv, argv);
-   perror(*argv);
+   warn("%s", *argv);
_exit((errno == ENOENT) ? 127 : 126);
-   /* NOTREACHED */
}
 
/* parent */
@@ -168,11 +165,11 @@ main(int argc, char *argv[])
 
if (exitonsig) {
if (signal(exitonsig, SIG_DFL) == SIG_ERR)
-   perror("signal");
+   return 128 + exitonsig;
else
kill(getpid(), exitonsig);
}
-   exit(WIFEXITED(status) ? WEXITSTATUS(status) : EXIT_FAILURE);
+   return WIFEXITED(status) ? WEXITSTATUS(status) : EXIT_FAILURE;
 }
 
 __dead void

time(1): perror(3) -> err(3) and friends

[Scott Cheloha]

We currently use a mix of perror(3) and err(3).

In one case you can merge perror + exit into err, which is nice.

The warns, though, are not equivalent (you get a "time: " prefix), so
maybe this is too risky.

Putting it out here anyway.

--
Scott Cheloha

Index: usr.bin/time/time.c
===
RCS file: /cvs/src/usr.bin/time/time.c,v
retrieving revision 1.22
diff -u -p -r1.22 time.c
--- usr.bin/time/time.c 13 Jul 2017 06:39:54 -  1.22
+++ usr.bin/time/time.c 14 Jul 2017 00:24:59 -
@@ -82,12 +82,11 @@ main(int argc, char *argv[])
clock_gettime(CLOCK_MONOTONIC, );
switch(pid = vfork()) {
case -1:/* error */
-   perror("time");
-   exit(1);
+   err(1, NULL);
/* NOTREACHED */
case 0: /* child */
execvp(*argv, argv);
-   perror(*argv);
+   warn("%s", *argv);
_exit((errno == ENOENT) ? 127 : 126);
/* NOTREACHED */
}
@@ -170,7 +169,7 @@ main(int argc, char *argv[])
 
if (exitonsig) {
if (signal(exitonsig, SIG_DFL) == SIG_ERR)
-   perror("signal");
+   warn("signal");
else
kill(getpid(), exitonsig);
}