- Download Diff

Summary:

11858 crontab could support /step

Review Request #2407 — Created Oct. 23, 2019 and submitted Oct. 28, 2019, 1:43 p.m.

Information
Owner:	citrus
Repository:	illumos-gate
Branch:	master
Bugs:	11858
Depends On:
Commit:	4295056...
Reviewers
Groups:	general
People:

Description

Testing Done

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

Issues

0
0
4
1
All Issues: 5

Description	From	Last Updated
Why are you defining this macro instead of using strtoul()? The latter would avoid having to preceed this with a ...	mscheler	Oct. 23, 2019, 8:28 p.m.
This macro is dangerous. If it is used like this ... if (condition) ADDELEMENT(...); ... it will cause a bug. ...	mscheler	Oct. 23, 2019, 12:23 p.m.
Should we check for overflow?	jbk	Oct. 26, 2019, 1:42 p.m.
Can any of these be turned into unsigned ints?	jbk	Oct. 26, 2019, 1:42 p.m.
Can these be unsigned as well?	jbk	Oct. 26, 2019, 1:42 p.m.

citrus Oct. 23, 2019, 12:43 p.m.

Thanks for taking the time to review this.

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) {

citrus Oct. 23, 2019, 12:43 p.m.

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).

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

Show all issues

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.

citrus Oct. 23, 2019, 12:43 p.m.

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.

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

Show all issues

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.

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

This check is repeated multiple times. Please consider turning it into an inline fuction.

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.

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

I would again prefer "cursor++" to be on separate line.

Commit:

-	a89609ee4bc05ab0a9a5ac8173c48eaa58976a94
+	4de901f158f59a90ed5315234b7b024a80f22d89

Diff:

Revision 2 (+412 -186)

Show changes

	usr/src/cmd/cron/Makefile
	usr/src/cmd/cron/cron.h
	usr/src/cmd/cron/cron.c
	usr/src/cmd/cron/crontab.c
	usr/src/cmd/cron/parse.c
	usr/src/man/man1/crontab.1

Change Summary:

Simplify man page example.

Commit:

-	4de901f158f59a90ed5315234b7b024a80f22d89
+	f1ca85b745c77b7b84a711c20025a0e13741f260

Diff:

Revision 3 (+412 -186)

Show changes

	usr/src/cmd/cron/Makefile
	usr/src/cmd/cron/cron.h
	usr/src/cmd/cron/cron.c
	usr/src/cmd/cron/crontab.c
	usr/src/cmd/cron/parse.c
	usr/src/man/man1/crontab.1

Ship it!

```
Ship It!
```

usr/src/cmd/cron/parse.c (Diff revision 3)
Show all issues
```
Should we check for overflow?
```
usr/src/cmd/cron/parse.c (Diff revision 3)
Show all issues
```
Can any of these be turned into unsigned ints?
```
usr/src/cmd/cron/parse.c (Diff revision 3)
Show all issues
```
Can these be unsigned as well?
```

Change Summary:

Updates following jbk review.

Testing Done:

		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

Commit:

-	f1ca85b745c77b7b84a711c20025a0e13741f260
+	18255cc8d5fbcfac1d0eedb025a1af06d1f419fd

Diff:

Revision 4 (+416 -186)

Show changes

	usr/src/cmd/cron/Makefile
	usr/src/cmd/cron/cron.h
	usr/src/cmd/cron/cron.c
	usr/src/cmd/cron/crontab.c
	usr/src/cmd/cron/parse.c
	usr/src/man/man1/crontab.1

Change Summary:

Simplify step loops.

Commit:

-	18255cc8d5fbcfac1d0eedb025a1af06d1f419fd
+	3c83ff505b60666354cefa63252f38041dbbac41

Diff:

Revision 5 (+412 -186)

Show changes

	usr/src/cmd/cron/Makefile
	usr/src/cmd/cron/cron.h
	usr/src/cmd/cron/cron.c
	usr/src/cmd/cron/crontab.c
	usr/src/cmd/cron/parse.c
	usr/src/man/man1/crontab.1

Commit:

-	3c83ff505b60666354cefa63252f38041dbbac41
+	a902da3e7c142a59a174ae41d892b08d84bd5ee4

Diff:

Revision 6 (+412 -186)

Show changes

	usr/src/cmd/cron/Makefile
	usr/src/cmd/cron/cron.h
	usr/src/cmd/cron/cron.c
	usr/src/cmd/cron/crontab.c
	usr/src/cmd/cron/parse.c
	usr/src/man/man1/crontab.1

Ship it!

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);

Change Summary:

Simplify wraparound logic (thanks to Matthias Scheler)

Commit:

-	a902da3e7c142a59a174ae41d892b08d84bd5ee4
+	42950568bac0c2b21180f5285c8630187c13f4bf

Diff:

Revision 7 (+407 -186)

Show changes

	usr/src/cmd/cron/Makefile
	usr/src/cmd/cron/cron.h
	usr/src/cmd/cron/cron.c
	usr/src/cmd/cron/crontab.c
	usr/src/cmd/cron/parse.c
	usr/src/man/man1/crontab.1

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

11858 crontab could support /step

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Testing Done:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)