Apologies for not making this clearer in the review description.

I want to ensure that if sendfile fails with a non-fatal error (EAGAIN/EINTR), the error isn't squashed by the copyout operation failing.

This was a conclusion that I made based on our discussion on the GitHub PR I submitted a few months ago:

@markjdb: How can that be? An error from the copyout() will now be returned when it wasn't before.
I think @bsdimp was referring to the sizeof(off_t) -> sizeof(bytes) portion of the change. The reason why I proposed that change is that it feels really wrong doing a sizeof of a type, which can change for sbytes in the future, and the copyout will break.

@ngie-eign: In terms of the issue, here's a catch with this diff that I'm not comfortable with and I'm going back and forth with @glebius on: if the operation before the copyout fails, but the copyout succeeds, the copyout succeeding will mask the earlier failure.
@ngie-eign: I think it's better to expose the copyout error as long as it doesn't mask errors outside of EAGAIN/EINTR.

Sidenote: reading through the requirements again, it seems that EBUSY would need this treatment as well as it says "Partial data may have been sent":

[EBUSY]            A busy page was encountered and SF_NODISKIO had been
                   specified.  Partial data may have been sent.

I typoed:

I want to ensure ... non-fatal error (EAGAIN/EINTR) ...

... fatal error (not EAGAIN/EINTR) ...

Yeah... I shouldn't have been split across 3 lines. Fixed to be just 2 lines.

As to the comment itself, I'll remove it (you're right -- it's overkill).

Some of the APIs state that they will return the number of bytes sent in sbytes, if interrupted, etc. What if these errors are triggered, but an EFAULT issue occurs because a bad address is passed in? It would make the value returned via sbytes invalid/misleading (hence, the need for the revision).

Per copyout(9), EFAULT is the only error that should be returned on failure:

RETURN VALUES
     The copy functions return 0 on success.  All but copystr() return EFAULT
     if a bad address is encountered.  The copyin_nofault() and
     copyout_nofault() functions return EFAULT if a page fault occurs.  The
     copystr() and copyinstr() functions return ENAMETOOLONG if the string is
     longer than len bytes.

Yes. That's the important point; that's why I chose to have the value from copyout overrides the value from the failed sendfile call.

Right now, if data is sent and sendfile(2) fails with EAGAIN, but the user provided a bogus pointer, it could result in programs calling sendfile(2) making decisions about sbytes being valid, whereas the data in sbytes is totally bogus.

The "real-world" program I have (so far) is the testcase that was just committed in rS343362.

@bdrewery: Bad code that doesn't check the return value of sendfile will just crash if sbytes is invalid. Or it doesn't care about sbytes and works fine. Yes it's bad code but it works? Now forcing an error could break existing code for the sake of something that was going to crash anyway.

This is a good assumption to operate by, in most cases, but it gets marginally harder when dealing with programs that might concurrently modify memory in an malloc'ed arena, poorly (bad locking or pointer errors are what come to mind). I'm mentioning this because I saw similar issues at my most recent employer, just on Linux (and in this case, with libaio, et al). It was even more difficult because the top-layer implementation used C++, but the underlying code was written in C, and the developers/PEs weren't expected to understand or write C code :/...

I'm advocating/optimizing for better developer experience with sendfile(2) and to ensure that the sendfile(2) API contract doesn't become overly conditional, e.g., will return -1/EFAULT, unless you provided me with bogus storage for sbytes, then it may either return -1/EFAULT if there wasn't an error, or crash when you access the memory pointed to by sbytes :(.