Builtin sleep and the tmxdate() function

[krader1961]

Outside of the AST time subsystem there are relatively few uses of that code by ksh. One such use is the tmxdate() function. It is used in one place in src/cmd/ksh93/bltins/print.c and three places in src/cmd/ksh93/bltins/sleep.c. I'd like to remove those uses because tmxdate.c has quite a few problems as identified by oclint and Coverity Scan and I'd rather remove the dependency than fix the problems. This issue focuses on the uses by the builtin sleep command.

The first thing to note is that the man page and sleep --help don't define the valid durations. The former says "seconds" and the latter "duration" but without defining either term. As of right now, in the US/Pacific timezone, the following invocations result in the indicated sleep duration in seconds:

• sleep 7d => 604800
• sleep '7 day' => 608400
• sleep '1 week' => 608400

Whoa! Shouldn't those all have the same sleep duration? The first one is obviously correct since 7 * 24 * 60 * 60 == 604800. The other two have an extra hour. Why? Because those forms cause tmxdate() to be called which calculates the delta between two wall clock times. Between now and 7 days in the future my timezone will switch from daylight savings time (DST) to standard time and wall clock time falls back one hour thus requiring adding an extra hour to achieve the same wall clock time. I'll bet 99.999% of people will be surprised at the difference between those formulations. Most people would expect them to produce the same sleep duration.

Note 1: A bare number optionally followed by the suffix d, h, m or s is handled directly by the b_sleep() function. This is equivalent to the behavior of the GNU sleep command. Anything else is handed off to tmxdate(). Apparently the people who implemented this thought it would be cool if you could type sleep '1 week 2 day 3 hour 4 second'. Quick, tell me what sleep 'Nov 2' does if it is currently any time in Nov 1? Answer: it calculates the duration to reach the current wall clock time on that date taking into account DST shifts. This is plain nuts.

Note 2: Neither bash nor zsh implement sleep as a builtin and thus you're at the mercy of whatever the platform external command implements.

P.S., I find it ironic how people have argued for the builtin echo behavior depending on whether the ATT or UCB universe was in effect or the contents of PATH. Yet here we have an example of a command that deliberately does not attempt to conform to the implementation of the command provided by the platform.

[krader1961]

For the record here are the problems identified by oclint. There are lots of problems with the other AST time subsystem code.

src/lib/libast/tm/tmxdate.c:690:17: Value stored to 'n' is never read
src/lib/libast/tm/tmxdate.c:695:21: Value stored to 'n' is never read
src/lib/libast/tm/tmxdate.c:784:37: Value stored to 'i' is never read
src/lib/libast/tm/tmxdate.c:799:37: Value stored to 'i' is never read
src/lib/libast/tm/tmxdate.c:1080:29: Value stored to 'n' is never read
src/lib/libast/tm/tmxdate.c:1119:33: Value stored to 'f' is never read
src/lib/libast/tm/tmxdate.c:1121:33: Value stored to 'f' is never read
src/lib/libast/tm/tmxdate.c:1123:33: Value stored to 'f' is never read
src/lib/libast/tm/tmxdate.c:1125:33: Value stored to 'f' is never read

src/lib/libast/tm/tmxdate.c:736:21: empty for statement [empty|P2]
src/lib/libast/tm/tmxdate.c:861:25: empty for statement [empty|P2]
src/lib/libast/tm/tmxdate.c:872:29: empty for statement [empty|P2]
src/lib/libast/tm/tmxdate.c:1131:33: empty for statement [empty|P2]
src/lib/libast/tm/tmxdate.c:1484:17: empty for statement [empty|P2]
src/lib/libast/tm/tmxdate.c:216:14: deep nested block [size|P3] Block depth of 8 exceeds limit of 5
src/lib/libast/tm/tmxdate.c:482:30: deep nested block [size|P3] Block depth of 6 exceeds limit of 5
src/lib/libast/tm/tmxdate.c:638:26: deep nested block [size|P3] Block depth of 7 exceeds limit of 5
src/lib/libast/tm/tmxdate.c:734:20: deep nested block [size|P3] Block depth of 6 exceeds limit of 5
src/lib/libast/tm/tmxdate.c:960:26: deep nested block [size|P3] Block depth of 7 exceeds limit of 5
src/lib/libast/tm/tmxdate.c:1062:59: deep nested block [size|P3] Block depth of 6 exceeds limit of 5
src/lib/libast/tm/tmxdate.c:468:17: empty while statement [empty|P2]
src/lib/libast/tm/tmxdate.c:505:25: empty while statement [empty|P2]
src/lib/libast/tm/tmxdate.c:512:21: empty while statement [empty|P2]
src/lib/libast/tm/tmxdate.c:867:29: empty while statement [empty|P2]
src/lib/libast/tm/tmxdate.c:878:33: empty while statement [empty|P2]
src/lib/libast/tm/tmxdate.c:588:17: prefer early exits and continue [convention|P3] Use early exit/continue to simplify code and reduce indentation
src/lib/libast/tm/tmxdate.c:911:25: prefer early exits and continue [convention|P3] Use early exit/continue to simplify code and reduce indentation
src/lib/libast/tm/tmxdate.c:461:17: avoid branching statement as last in loop [convention|P2]
src/lib/libast/tm/tmxdate.c:152:1: long method [size|P3] Method with 1358 lines exceeds limit of 60
src/lib/libast/tm/tmxdate.c:298:25: missing default in switch statements [convention|P3]
src/lib/libast/tm/tmxdate.c:387:25: missing default in switch statements [convention|P3]
src/lib/libast/tm/tmxdate.c:402:25: missing default in switch statements [convention|P3]
src/lib/libast/tm/tmxdate.c:901:25: missing default in switch statements [convention|P3]
src/lib/libast/tm/tmxdate.c:982:17: missing default in switch statements [convention|P3]
src/lib/libast/tm/tmxdate.c:1065:21: missing default in switch statements [convention|P3]
src/lib/libast/tm/tmxdate.c:1205:29: missing default in switch statements [convention|P3]
src/lib/libast/tm/tmxdate.c:451:21: non case label in switch statement [convention|P3]

[krader1961]

If we eliminate the use of tmxdate() by ksh we can also eliminate the src/lib/libast/tm/tmxscan.c module since the only use of the code in that module is by tmxdate(). And that module has its own problems:

src/lib/libast/tm/tmxscan.c:349:26: 3rd function call argument is an uninitialized value
src/lib/libast/tm/tmxscan.c:360:21: Value stored to 'x' is never read

src/lib/libast/tm/tmxscan.c:433:21: empty for statement [empty|P2]
src/lib/libast/tm/tmxscan.c:413:93: deep nested block [size|P3] Block depth of 7 exceeds limit of 5
src/lib/libast/tm/tmxscan.c:424:30: deep nested block [size|P3] Block depth of 6 exceeds limit of 5
src/lib/libast/tm/tmxscan.c:424:5: prefer early exits and continue [convention|P3] Use early exit/continue to simplify code and reduce indentation
src/lib/libast/tm/tmxscan.c:81:11: long method [size|P3] Method with 61 lines exceeds limit of 60
src/lib/libast/tm/tmxscan.c:147:11: long method [size|P3] Method with 258 lines exceeds limit of 60
src/lib/libast/tm/tmxscan.c:193:17: non case label in switch statement [convention|P3]
src/lib/libast/tm/tmxscan.c:207:17: non case label in switch statement [convention|P3]

[krader1961]

I asked earlier:

Quick, tell me what sleep 'Nov 2' does if it is currently any time in Nov 1? Answer: it calculates the duration to reach the current wall clock time on that date taking into account DST shifts. This is plain nuts.

This is a good example where the principal of orthogonality should have been applied. Even if sleeping till the same wall clock time at some future date is useful it should have been implemented via a composition pattern. For example sleep $(current_wall_clock_time_on 'Nov 2'). I am not seriously proposing introducing a current_wall_clock_time_on command but you get the idea. There is zero justification for the sleep command to be aware of DST or the concept of the same wall clock time on some other date. That belongs in some other command which returns a simple interval to be used by sleep. And the output of that hypothetical command is potentially useful in other ways but since it isn't a separate command those uses can't be realized.

[krader1961]

Grumble! The builtin printf sort of documents the %T format specifier which causes the associated argument to be evaluated by tmxdate(). Which implies we have to maintain that code for backward compatibility. Note that the documentation for printf does not document the related %Q format specifier:

KSH PROMPT:1: printf '%(%s)Q\n' '1M1day5s'
2505605
KSH PROMPT:2: printf '%(%s)T\n' '1 month 2 day 5 second'
1543824005

The first one produces the number of seconds represented by the argument as evaluated by strelapsed(). Albeit with a funny idea for what 1M (one month) means since it treats it as equivalent to 28 days rather than 30.5 days which is a better approximation of "one month".

The second invocation calculates seconds since the epoch represented by the date string as interpreted by tmxdate(): 1543824005 is 2018-12-03 00:00:05 local time when I did this experiment. Note how the "1 month" became December 1 rather than December 5. Another counterintuitive result.

[krader1961]

The ksh93v- work in progress code had a builtin date command which also utilized a lot of the AST time subsystem. Including the tmxdate() function. @siteshwar removed that code because it wasn't present as a builtin in the ksh93u+ release. I think we need to decide if we want to depend on and maintain the AST time subsystem or not. If we do then the builtin date code should be reinstated. If not then the use of tmxdate() by the printf and sleep commands should be eliminated and the tmxdate() function removed.

[krader1961]

While converting the code to use getopt_long() rather than the AST optget() function I ran sleep --man from a ksh93u+ shell. It emitted this:

DESCRIPTION
  sleep suspends execution for at least the time specified by duration or until a SIGALRM
  signal is received. duration may be one of the following:
    integer
          The number of seconds to sleep.
    floating point
          The number of seconds to sleep. The actual granularity depends on the
          underlying system, normally around 1 millisecond.
    PnYnMnDTnHnMnS
          An ISO 8601 duration where at least one of the duration parts must be
          specified.
    PnW   An ISO 8601 duration specifying n weeks.
    pnYnMnDTnHnmnS
          A case insensitive ISO 8601 duration except that M specifies months, m before s
          or S specifies minutes and after specifies milliseconds, u or U specifies
          microseconds, and n specifies nanoseconds.
    date/time
          Sleep until the date(1) compatible date/time.

Yet the ksh(1) man page says the argument is a simple floating point literal representing the number of seconds to sleep. So this discrepancy between the official man page and the doc string shown by sleep --man has existed for at least seven years. I'm going to omit mention of the arguments that are not absolute seconds to sleep in the translation of the doc string to RST format and add a "TODO" to the source that references this issue.

[krader1961]

The question is whether we can change the sleep implementation to match the ksh(1) man page rather than the internal documentation. Note that POSIX says that sleep takes a "non-negative decimal integer" and no other values. I have no idea how long sleep has supported non-literal floating point values. Do we care about breaking users who ran sleep --man and noticed it documented intervals different from those documented by the ksh(1) man page?

[krader1961]

Gah! I don't know how I missed the fact the ksh(1) man page says:

       sleep seconds
              Suspends execution for the number of decimal seconds or fractions of a  sec-
              ond  given by seconds.  seconds A suffix of one of smhd can be used to indi-
              cate seconds, minutes, hours, and days respectively.  Seconds  can  also  be
              specified using a date/time format.

However, it is still worth noting that description differs significantly from the output of sleep --man. TBD is how to reconcile the differences in documentation.