noeintr_close()

[gperciva]

No description provided.

[gperciva]

Latest iteration of this; now using optional_mutex().

Previous discussions:

• Posix close #300
• noeintr_close #312
  (but I don't think there's any required reading in those)

I also added your twitter comments to the top of noeintr_close.c so that readers can conveniently see it all in one place. You might think that's too verbose, but since this is a non-trivial piece of code, and who knows how long twitter will remain online (or at least, viewable without login), I think it's worth it.

[gperciva]

I don't like this function name (and I'm certain that you'll hate it), but I definitely think it's worth doing this in a separate function, rather than as part of noeintr_close() itself.

[gperciva]

I'm not certain if you'll like having noeintr_close_testing.{c,h} as separate files. I found it useful to shove it away from noeintr_close.c itself, but I suspect that you might prefer it in the same file.

One downside of combining them: we'd need to put void noeintr_close_testing_print_stats(void); in noeintr.h (protected by #ifdef NOEINTR_CLOSE_TESTING), and that looks a bit weird. Or we could go with what I did temporarily in my devel code: dump the void noeintr_close_testing_print_stats(void); declaration in tests/noeintr_close/*/main.c along with a comment "keep this declaration in sync with the actual code in noeintr_close.c". The latter idea feels really icky, but at least it would avoid polluting noeintr.h with a test-only function.

[cperciva]

I would add "POSIX specifies that close(2) may be interrupted by a signal and return -1 with errno set to EINTR, but explicitly does not specify whether the file descriptor has been closed if that occurs".

[cperciva]

s/wrap it in a mutex/wrapped in a mutex/ ?

[gperciva]

I think the tense is correct...? The whole set of instructions are in present tense.

Arguably, the "wrap it in a mutex" should be part of the list of steps. Maybe something like:

 * To close(fd),
 * 1. lock a mutex
 * 2. dup2(s[1], fd) until it succeeds.  (If it fails with !EINTR, bail.)
 * 3. close(fd).
 * 4. If we got EINTR, recv(fd, MSG_PEEK).
 * 5. If we read the random cookie, goto 3.
 * 6. unlock the mutex 

but I think that complicates it unnecessarily -- the "interesting" thing about those instructions are steps 2-5. Adding steps for "lock & unlock" isn't going to clarify the idea.

[cperciva]

Well, it could be "wrap in a mutex", but the word "it" in "wrap it in a mutex" confuses me. By "wrapped in a mutex..." I meant "do the following steps wrapped in a mutex".

[cperciva]

Comment fixes look good. I'm guessing this wants to wait until the optional mutex build stuff is merged though?

[gperciva]

Yeah, I'd rather wait another few minutes, just to keep the git history clean. I'll rebase once that's merged.

... you didn't have any code nitpicks? And you're ok with noeintr_close_testing.{c,h}?

[cperciva]

I can't remember if there were any issues with the code... it has been a few weeks since I last looked at this. I'll take another look before merging, of course.

[gperciva]

Should we be making a noeintr_fclose()?

[cperciva]

Meh, let's not bother with an fclose variant yet; I don't think we use the stdio functions in places where we would need to worry about leaking file descriptors.

[cperciva]

s/handle EINTR appropriately/retry on EINTR if the descriptor was not closed/

Also we should add "Unlike close(2), this function is not async-signal-safe."

[gperciva]

I rebased this on top of master, and included the REBASE commits which did changes that you asked for.

I left two REBASE commits, which were some changes I made after you last looked at this.