- Download Diff

Summary:

11876 Want a native sleep

Review Request #2417 — Created Oct. 25, 2019 and submitted Jan. 11, 2020, 3:40 a.m.

Information
Owner:	rm
Repository:	illumos-gate
Branch:	master
Bugs:	11876
Depends On:
Commit:	cf0ee79...
Reviewers
Groups:	general
People:

Description

11876 Want a native sleep

Testing Done

Issues

0
0
12
1
All Issues: 13

Description	From	Last Updated
lint is no more	ptribble	Oct. 29, 2019, 1:05 a.m.
Shouldn't this read "sleep time[suffix]" instead?	marcel	Oct. 29, 2019, 12:57 a.m.
I suggest to add \|\| eptr == argv[1] here to cover two cases: 1) sleep s will properly report "failed ...	marcel	Oct. 29, 2019, 1:03 a.m.
We should fail for sec < 0 (man page says: non-negative). We should also fail for cases like sleep -0.2.	marcel	Oct. 29, 2019, 12:57 a.m.
This doesn't match the usage in the program itself.	yuripv	Oct. 29, 2019, 1:04 a.m.
spelling: least	hans	Oct. 29, 2019, 12:59 a.m.
hours, not minutes	marcel	Oct. 29, 2019, 12:59 a.m.
terminate	yuripv	Oct. 29, 2019, 12:59 a.m.
BSIGALARM?	marcel	Oct. 29, 2019, 12:58 a.m.
I don't think LC_MESSAGES and NLSPATH affect anything as you don't seem to use gettext()?	yuripv	Oct. 29, 2019, 1:08 a.m.
Don't need this target?	yuripv	Oct. 29, 2019, 1:05 a.m.
infinity, not infinite	ptribble	Oct. 29, 2019, 1:05 a.m.
Please test what does: sleep " " sleep " s" sleep " -s" sleep " +s" sleep "1 " I ...	marcel	Oct. 31, 2019, 4:19 p.m.

usr/src/man/man1/sleep.1 (Diff revision 1)

Show all issues

This doesn't match the usage in the program itself.

usr/src/man/man1/sleep.1 (Diff revision 1)
Show all issues
```
terminate
```

usr/src/man/man1/sleep.1 (Diff revision 1)

I'd strip the roff codes here, they don't help anything.

usr/src/man/man1/sleep.1 (Diff revision 1)

Show all issues

I don't think LC_MESSAGES and NLSPATH affect anything as you don't seem to use gettext()?

rm Oct. 29, 2019, 1:11 a.m.

OK. I guess that's true. Should I go back and use gettext()? Or wait until someone has translations?

gdamore Oct. 29, 2019, 5:43 p.m.

Use gettext() now. Then someone can add the translations without touching the code. i18n should always be done first, l10n can occur later, or even never.

rm Oct. 29, 2019, 6:47 p.m.
```
OK, the most recent revision has this.
```

usr/src/man/man1/sleep.1 (Diff revision 1)

Why? I don't think linking to library functions is helpful here.

rm Oct. 29, 2019, 1:11 a.m.

It's something we've done in other manual pages. I think knowing what library functions can be used to do the same thing is useful and helps one understand it.

gdamore Oct. 29, 2019, 5:43 p.m.
```
I agree with Robert on this one.
```

usr/src/test/util-tests/tests/sleep/Makefile (Diff revision 1)
Show all issues
```
Don't need this target?
```

usr/src/cmd/sleep/sleep.c (Diff revision 1)

nit: Since you're really using strtold() further down, perhaps use that in the comment too instead of strtod()?

usr/src/man/man1/sleep.1 (Diff revision 1)
Show all issues
```
spelling: least
```

Ship it!

```
Ship It!
```

usr/src/man/man1/sleep.1 (Diff revision 1)
Show all issues
```
BSIGALARM?
```

Ship it!

usr/src/cmd/sleep/sleep.c (Diff revision 1)

Show all issues

Shouldn't this read "sleep time[suffix]" instead?

usr/src/cmd/sleep/sleep.c (Diff revision 1)

Show all issues

I suggest to add || eptr == argv[1] here to cover two cases:
1) sleep s will properly report "failed to parse time s" instead of silently succeed.

2) sleep "", so you could remove the explicit test for this case above.

rm Oct. 29, 2019, 1:11 a.m.
```
Thanks, that's a good suggestion.
```

usr/src/cmd/sleep/sleep.c (Diff revision 1)

Show all issues

We should fail for sec < 0 (man page says: non-negative).  We should also fail for cases like sleep -0.2.

rm Oct. 29, 2019, 1:11 a.m.

There were several other issues parsing some of this, which I've gone and fixed up here, thanks.

usr/src/man/man1/sleep.1 (Diff revision 1)
Show all issues
```
hours, not minutes
```

usr/src/cmd/sleep/Makefile (Diff revision 1)
Show all issues
```
lint is no more
```
usr/src/test/util-tests/tests/sleep/sleeptest.ksh (Diff revision 1)
Show all issues
```
infinity, not infinite
```

Commit:

-	fa8f68dc291e8baae4b469e1c5bc864a3a1b8759
+	27f9b18df75046ecf67addbf5f426fb53ba8226e

Diff:

Revision 2 (+608 -190)

Show changes

	usr/src/cmd/Makefile
	usr/src/cmd/ksh/builtins/Makefile
	usr/src/cmd/sleep/Makefile
	usr/src/cmd/sleep/sleep.c
	usr/src/man/man1/sleep.1
	usr/src/pkg/manifests/SUNWcs.mf
	usr/src/pkg/manifests/system-test-utiltest.mf
	usr/src/test/util-tests/runfiles/default.run
	5 more

Change Summary:

Update for gettext(3C).

Commit:

-	27f9b18df75046ecf67addbf5f426fb53ba8226e
+	8aa81612174bed736494f50cfc2cdda71b3f4c18

Diff:

Revision 3 (+619 -190)

Show changes

	usr/src/cmd/Makefile
	usr/src/cmd/ksh/builtins/Makefile
	usr/src/cmd/sleep/Makefile
	usr/src/cmd/sleep/sleep.c
	usr/src/man/man1/sleep.1
	usr/src/pkg/manifests/SUNWcs.mf
	usr/src/pkg/manifests/system-test-utiltest.mf
	usr/src/test/util-tests/runfiles/default.run
	5 more

Ship it!

```
Ship It!
```

usr/src/cmd/sleep/sleep.c (Diff revision 3)

Show all issues

Please test what does:

sleep " "
sleep " s"
sleep " -s"
sleep " +s"
sleep "1 "

I suspect at least some of them won't work as expected.

There is also general question: Should sleep " 1" succeed or fail? It looks like both GNU sleep and ksh sleep succeeds here.

Also, it looks like /usr/bin/sleep "1 " (ksh sleep) sleeps forever, while /usr/gnu/bin/sleep "1 " (GNU sleep) simply fails.

rm Oct. 31, 2019, 3:47 p.m.

All of the ones that you have there properly fail:

rm@turin:/ws/rm/sleep/usr/src/cmd/sleep$ ./sleep " "
sleep: failed to parse time ' '
rm@turin:/ws/rm/sleep/usr/src/cmd/sleep$ ./sleep " s"
sleep: failed to parse time ' s'
rm@turin:/ws/rm/sleep/usr/src/cmd/sleep$ ./sleep " -s"
sleep: failed to parse time ' -s'
rm@turin:/ws/rm/sleep/usr/src/cmd/sleep$ ./sleep " +s"
sleep: failed to parse time ' +s'
rm@turin:/ws/rm/sleep/usr/src/cmd/sleep$ ./sleep "1 "
sleep: failed to parse time 1

sleep " 1" does suceeed like ksh and GNU. I can add these to the test suite, but can you be more explicit about what your concern is? It seems like you suspect something is broken in the line of code highlighted.

marcel Oct. 31, 2019, 4:17 p.m.

Okay, thanks for tests.
I somehow expected that with " s" the strtold() will set eptr to point to 's', not to leading ' '.  But this expectation was wrong, apparently (even the strtold(3c) man page specifies that).

usr/src/cmd/sleep/sleep.c (Diff revision 3)

At line 103 you compare with 0.0 when you want to compare with zero. I think these constants should be unified.

Ship it!

```
Ship It!
```

Change Summary:

Fix locale override.

Commit:

-	8aa81612174bed736494f50cfc2cdda71b3f4c18
+	cf0ee79bc42c4e656720c78917bd4c278758cd3b

Diff:

Revision 4 (+619 -190)

Show changes

	usr/src/cmd/Makefile
	usr/src/cmd/ksh/builtins/Makefile
	usr/src/cmd/sleep/Makefile
	usr/src/cmd/sleep/sleep.c
	usr/src/man/man1/sleep.1
	usr/src/pkg/manifests/SUNWcs.mf
	usr/src/pkg/manifests/system-test-utiltest.mf
	usr/src/test/util-tests/runfiles/default.run
	5 more

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

11876 Want a native sleep

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)