11858 crontab could support /step

Review Request #2407 — Created Oct. 23, 2019 and submitted

citrus
illumos-gate
master
11858
4295056...
general

11858 crontab could support /step

Also available for review at: https://code.illumos.org/c/illumos-gate/+/125

Used the following test data to confirm that the new parser is producing expected output:

In the output below, the first column is the range of the field as provided to the parser, the second column is the input, and the last column shows the generated sequence or error.

 5-10                         6 OK 6
 5-10                         4 OK OutOfBounds
 5-10                        11 OK OutOfBounds
 5-10                        -1 OK UnexpectedChar
 0-10                         * OK *
 0-10                       *,1 OK UnexpectedChar
 0-19                       */2 OK 0,2,4,6,8,10,12,14,16,18
 0-10                     */2,5 OK 0,2,4,6,8,10,5
 0-10                       1-5 OK 1,2,3,4,5
 5-10                       6-6 OK 6
 0-10                     1,3,5 OK 1,3,5
 0-10                     1,x,5 OK UnexpectedChar
 0-10                      1,,5 OK UnexpectedChar
 0-10                       x-5 OK UnexpectedChar
 0-10                       1-x OK UnexpectedChar
 0-10                     1-5/x OK UnexpectedChar
 0-19                    1-10/2 OK 1,3,5,7,9
 0-59      0,1-6/2,7-20/5,20-23 OK 0,1,3,5,7,12,17,20,21,22,23
 0-23         20-0,1-6/2,7-20/5 OK 20,21,22,23,0,1,3,5,7,12,17
 0-59                     0-5/7 OK 0
 0-59                     0-5/0 OK OutOfBounds
 0-59                 0-59,0-59 OK Overflow
 0-59                     1-100 OK OutOfBounds
 0-10                       8-2 OK 8,9,10,0,1,2
 0-19                      15-8 OK 15,16,17,18,19,0,1,2,3,4,5,6,7,8
 0-59                   49-10/4 OK 49,53,57,1,5,9
 0-59                   50-10/4 OK 50,54,58,2,6,10
 0-59                   51-10/4 OK 51,55,59,3,7
 0-59                   52-10/4 OK 52,56,0,4,8
 1-58                   50-10/4 OK 50,54,58,4,8
 2-57                   50-10/4 OK 50,54,2,6,10
 3-56                   50-10/4 OK 50,54,4,8
 4-55                   50-10/4 OK 50,54,6,10
40-55                   50-45/4 OK 50,54,42
  • 0
  • 0
  • 4
  • 1
  • 5
Description From Last Updated
mscheler
  1. 
      
    1. Thanks for taking the time to review this.

  2. usr/src/cmd/cron/cron.c (Diff revision 1)
     
     
    As you are changing this line anyhow you could compare against an integer value as this is not a boolean expression:
    
    if ((rfork = fork()) != 0) {
    1. I was adding the brackets as part of cleaning up some compiler warnings.
      You're right, since I'm changing it anyway, I might as well fix the condition
      (and the other two).

  3. usr/src/cmd/cron/parse.c (Diff revision 1)
     
     
    Why are you defining this macro instead of using strtoul()? The latter would avoid having to preceed this with a check of the first digit and also provide you a pointer to the first character after the number.
    1. It isn't necessarily obvious since I moved the code to a new file, but this is mostly carried over from the original next_field() implementation (it's a shame that reviewboard can't spot that). The logic was duplicated in cron.c and crontab.c which is why I centralised it like this. I added the macro to make the function logic a bit easier to read through.

      strtoul() is actually a bit too clever as it will skip leading whitespace and accept a leading +/-.
      I'd also have to use the end pointer to increment cursor. There is a function in usr/src/cmd/cron/funcs.c called num() which I considered using, but that too uses string pointers which would mean a much bigger restructure here.

  4. usr/src/cmd/cron/parse.c (Diff revision 1)
     
     
    This macro is dangerous. If it is used like this ...
    
    if (condition)
             ADDELEMENT(...);
    
    ... it will cause a bug.
    
    Putting "do ... while (0)" around it would avoid that.
  5. usr/src/cmd/cron/parse.c (Diff revision 1)
     
     
    This check is repeated multiple times. Please consider turning it into an inline fuction.
  6. usr/src/cmd/cron/parse.c (Diff revision 1)
     
     
    I would prefer that you put "cursor++" on a separate line. That would make it much obvious that the code always skips over the dash.
  7. usr/src/cmd/cron/parse.c (Diff revision 1)
     
     
    I would again prefer "cursor++" to be on separate line.
  8. 
      
citrus
citrus
mscheler
  1. Ship It!
  2. 
      
jbk
  1. 
      
  2. usr/src/cmd/cron/parse.c (Diff revision 3)
     
     

    Should we check for overflow?

  3. usr/src/cmd/cron/parse.c (Diff revision 3)
     
     

    Can any of these be turned into unsigned ints?

  4. usr/src/cmd/cron/parse.c (Diff revision 3)
     
     

    Can these be unsigned as well?

  5. 
      
citrus
citrus
citrus
mscheler
  1. 
      
  2. usr/src/cmd/cron/parse.c (Diff revisions 3 - 6)
     
     
    I think this expression comes up with the same value without a need for an if statement:
    
    start = i - (upper - lower + 1);
  3. 
      
citrus
mscheler
  1. Ship It!
  2. 
      
citrus
Review request changed

Status: Closed (submitted)

Loading...