OS-7991 port_get() and port_getn() implementation disagrees with documentation

[joyent-automation]

OS-7991 port_get() and port_getn() implementation disagrees with documentation

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/6919/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@jjelinek commented at 2019-09-19T13:39:32

Patch Set 2:

(3 comments)

This looks good aside from the copyright nits.

@johnlevon commented at 2019-09-19T14:31:47

Patch Set 2:

(1 comment)

Patch Set 2 code comments

usr/src/head/port.h#27 @jjelinek

This isn't the current copyright style. We should drop the (c) and comma after the year.

usr/src/head/port.h#27 @hrosenfeld

Done

usr/src/lib/libc/port/gen/event_port.c#27 @jjelinek

same problem here

usr/src/lib/libc/port/gen/event_port.c#27 @hrosenfeld

Done

usr/src/uts/common/fs/portfs/port.c#28 @jjelinek

and here

usr/src/uts/common/fs/portfs/port.c#28 @hrosenfeld

Done. Shall I also remove the "All rights reserved.", or is that still ok?

usr/src/uts/common/fs/portfs/port.c#618 @johnlevon

I'm trying to follow this condition and why it differs from the nget > 1 case. Is it that near the end of port_getn(), when we do port_get_timer() again, that resets the ETIME return back to 0 ?

I'm sure this is correct, it just seems weird that port_getn(nget == 1) doesn't itself return with ETIME when the timer times out.

usr/src/uts/common/fs/portfs/port.c#618 @hrosenfeld

The cases that I have observed skip setting the error in port_getn():

• an event is immediately available, and the code proceeds to portnowait. Then we cannot copy out the event because the callback fails and return it to the queue, but we still return no error.
• an all-zero timespec indicates that the code shouldn't block, so it also proceeds to portnowait. If no events are available we just return with no error.

This does also happen for nget > 1, but since in that case the user called port_getn(3C) he is supposed to check nget anyway and no problem exists.

I thought about changing the logic in port_getn() to set errno earlier, but I'd prefer not to as I find the logic a bit convoluted and don't want to accidentally change its behavior. As this really only affects port_get(3C), fixing it here seems more appropriate.

@hrosenfeld commented at 2019-09-19T14:33:31

Patch Set 2:

(3 comments)

@jjelinek commented at 2019-09-20T13:36:18

Patch Set 3:

(1 comment)

Patch Set 3 code comments

usr/src/uts/common/fs/portfs/port.c#28 @jjelinek

Sorry, yes, "all rights reserved" is no longer part of our standard copyright.

@jjelinek commented at 2019-09-23T23:26:57

Patch Set 4: Code-Review+1

Even though I gave +1, I'm curious to hear if you have an answer for John's question?

@hrosenfeld commented at 2019-09-26T12:29:10

Patch Set 2:

(1 comment)

@johnlevon commented at 2019-09-26T15:46:47

Patch Set 4:

Thanks for the explanation. It seems reasonable not to try to touch port_getn(), it's certainly written in a pretty obtuse way. Do you mind adding a comment though with your analysis (and why it's different to the n > 1 case?)

@johnlevon commented at 2019-10-01T16:30:00

Patch Set 5: Code-Review+1

Thanks

@jjelinek commented at 2019-10-01T16:30:55

Patch Set 5: Code-Review+1