Page MenuHomeFreeBSD

Add [initial] functional tests for sendfile(2) as lib/libc/sys/sendfile
ClosedPublic

Authored by ngie on Dec 20 2018, 7:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 9, 3:13 AM
Unknown Object (File)
Tue, Apr 9, 3:13 AM
Unknown Object (File)
Tue, Apr 9, 3:13 AM
Unknown Object (File)
Tue, Apr 9, 12:53 AM
Unknown Object (File)
Tue, Mar 26, 12:18 PM
Unknown Object (File)
Mar 15 2024, 8:22 PM
Unknown Object (File)
Mar 2 2024, 3:46 AM
Unknown Object (File)
Feb 11 2024, 5:24 AM
Subscribers

Details

Summary

These testcases exercise a number of functional requirements for sendfile(2).

The testcases use IPv4 and IPv6 domain sockets with TCP, and were confirmed
functional on UFS and ZFS. UDP address family sockets cannot be used per the
sendfile(2) contract, thus using UDP sockets is outside the scope of
testing the syscall in positive cases. As seen in
:s_negative_udp_socket_test, UDP is used to test the sendfile(2) contract
to ensure that EINVAL is returned by sendfile(2).

The testcases added explicitly avoid testing out SF_SYNC due to the
complexity of verifying that support. However, this is a good next logical
item to verify.

The hdtr_positive* testcases work to a certain degree (the header
testcases pass), but the trailer testcases do not work (it is an expected
failure). In particular, the value received by the mock server doesn't match
the expected value, and instead looks something like the following (using
python array notation):

trailer[:]message[1:]

instead of:

message[:]trailer[:]

This makes me think there's a buffer overrun issue or problem with the
offset somewhere in the sendfile(2) system call, but I need to do some
other testing first to verify that the code is indeed sane, and my
assumptions/code isn't buggy.

The sbytes_negative testcases that check sbytes being set to an
invalid value resulting in EFAULT fails today as the other change
(which checks copyout(9)) has not been committed [1]. Thus, it
should remain an expected failure (see bug 232210 for more details
on this item).

Todo items for testing sendfile(2):

  1. Fix the header/trailer testcases.
  2. Setup if_tap interface and test with it, instead of using "localhost", per @asomers suggestions.
  3. Handle short recv(2)'s in server_cat(..).
  4. Add SF_SYNC support.
  5. Add some more negative tests outside the scope of the functional contract.

PR: 232210
MFC after: 1 month
Sponsored by: Netflix, Inc

Test Plan
$ make obj; make; sudo make install
$ kyua test -k /usr/tests/lib/libc/sys/Kyuafile sendfile_test

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

asomers added inline comments.
lib/libc/tests/sys/sendfile_test.c
70 ↗(On Diff #52203)

Why bother with malloc, since you already know that you're reading into something of size int? You could just read into the return value instead.

83 ↗(On Diff #52203)

Could you eliminate the need for random ports entirely if you used deterministic ports and a freshly created tap(4) or tun(4) interface? That's what tests/sys/netinet/fibs_test.sh does. That would also allow you to use hardcoded IP addresses, eliminating all of the DNS stuff.

101 ↗(On Diff #52203)

Easier to do rand() % (portrange_last - portrange_first) / portrange_first, no?

202 ↗(On Diff #52203)

This function could use some commenting. It's not obvious what's going on.

225 ↗(On Diff #52203)

Calling recv when you aren't expecting to receive anything is sufficiently weird that I think you should explain why you're doing it.

277 ↗(On Diff #52203)

The === DEST FILE === bars are redundant with the dest file: prefix. I would eliminate the latter.

291 ↗(On Diff #52203)

If 0 is a special flag value, then it should be called out in the function's comment block.

304 ↗(On Diff #52203)

Does offset refer to the source file or destination file? If it's the same offset that you supplied to sendfile(2), then it should refer to the source file. In that case, the offset argument to mmap should almost certainly be 0.

317 ↗(On Diff #52203)

It's not important in this case, but it's nice to release your resources in LIFO order. Also, you leak dest_fd here.

341 ↗(On Diff #52203)

My personal preference would be to put the _exit(0) here rather than inside of server_cat. Or if not, to leave the rest of the function inside of the else clause.

406 ↗(On Diff #52203)

What's the purpose of the lseek?

521 ↗(On Diff #52203)

What about SF_NODISKIO?

657 ↗(On Diff #52203)

s/trailer/hdtr/

670 ↗(On Diff #52203)

Without else clauses, hdtr will be used uninitialized.

686 ↗(On Diff #52203)

No check for fd2?

707 ↗(On Diff #52203)

Double close.

804 ↗(On Diff #52203)

Isn't "negative" the same thing as "less than zero"?

This revision now requires changes to proceed.Dec 27 2018, 11:01 PM
ngie marked 6 inline comments as done.Dec 29 2018, 9:46 PM
ngie added inline comments.
lib/libc/tests/sys/sendfile_test.c
83 ↗(On Diff #52203)

I understand what you mean, but not everyone has tap/tun in their kernel. I don't rely on either; when I tried using them back in 9.x I ran into a number of issues related to IPv6 support, etc, and learned not to trust those interfaces. I realize this was 4 years ago, but I don't want sendfile(2) support to be contingent on the reliability of the IPv4/IPv6 implementation of tap/tun. This system call should be testable using lo*(4).

317 ↗(On Diff #52203)

Wow. Good catch -- thank you!

406 ↗(On Diff #52203)

lseek(2) rewinds the file pointer so it can be reused in sendfile(2) below.

521 ↗(On Diff #52203)

I will add a comment about this testcase being missing.

Long story short, verifying the functionality of this flag is non-trivial.

707 ↗(On Diff #52203)

ACK. I got that logic backwards.

804 ↗(On Diff #52203)

Negative as in negative testcase, not a real number less than 0.

ngie marked 10 inline comments as done.Dec 29 2018, 9:52 PM
ngie added inline comments.
lib/libc/tests/sys/sendfile_test.c
70 ↗(On Diff #52203)

Fair enough. I try to avoid screwing up userspace applications like this through additional sanity checks, but I understand what you mean.

101 ↗(On Diff #52203)

Yeah. A whole lot easier :D.

341 ↗(On Diff #52203)

Yeah, good call. I was trying to avoid duplication, but not putting the _exit in a more appropriate place obfuscates what's going on.

670 ↗(On Diff #52203)

Good catch. I need to set the *_cnt fields to 0 to avoid this case.

686 ↗(On Diff #52203)

Derp. Yeah.

ngie marked 5 inline comments as done.Dec 31 2018, 4:41 PM
ngie added inline comments.
lib/libc/tests/sys/sendfile_test.c
101 ↗(On Diff #52203)

I tried this out and unfortunately it doesn't work (it results in a SIGFPE).

lib/libc/tests/sys/sendfile_test.c
83 ↗(On Diff #52203)

This method of using random port numbers cannot be reliable, because there's nothing to prevent collisions. The fibs_test has been using tap for over 4 years now. It's never caused any tap-related problems in CI. So I think it's safe to use here.

101 ↗(On Diff #52203)

Uhh, that should've been a + portrange_first at the end, not / portrange_first

406 ↗(On Diff #52203)

But what advances the file pointer in the first place?

804 ↗(On Diff #52203)

Ahh, makes sense.

ngie marked 7 inline comments as done.Dec 31 2018, 6:16 PM
ngie added inline comments.
lib/libc/tests/sys/sendfile_test.c
83 ↗(On Diff #52203)

Fair enough. If we're going to go this route, I need to implement a generic library for setting up if_tap interfaces and I need to finish up the setup test fixtures support I've had kicking around for a week or so now.

304 ↗(On Diff #52203)

You're right. Fixed.

406 ↗(On Diff #52203)

Ah, you're right. It's superfluous since I was using mmap instead of using something like write(2).

804 ↗(On Diff #52203)

I renamed the functions so it's a bit clearer what negative means in this case :).

ngie marked 4 inline comments as done.

Update based on comments/discussion with @asomers

lib/libc/tests/sys/sendfile_test.c
563 ↗(On Diff #52455)

Varying across FreeBSD versions shouldn't be a problem for tests that are in the base system.

You haven't written tests for SF_NODISKIO yet just because they're hard, right?

662 ↗(On Diff #52455)

Is there a PR for this failure? I always like to put PR numbers in atf_tc_expect_fail statements.

ngie marked 2 inline comments as done.

Address comments from @asomers about failing header/trailer testcases and add a simple positive case for SF_NODISKIO (which passes on ffs/ufs today).

lib/libc/tests/sys/sendfile_test.c
563 ↗(On Diff #52455)

The former part is definitely true... just a reminder about the nuance between versions (mostly for the reader/me).

The positive case is trivial to test; the negative case however, is non-trivial. The other catch is that providing SF_NODISKIO might cause the test to fail if the pages can't be paged in because they're being modified by a different region. From ERRORS under sendfile(2):

SF_NODISKIO
        This flag causes sendfile to return EBUSY instead of
        blocking when a busy page is encountered.  This rare
        situation can happen if some other process is now working
        with the same region of the file.  It is advised to retry
        the operation after a short period.

        Note that in older FreeBSD versions the SF_NODISKIO had
        slightly different notion.  The flag prevented sendfile to
        run I/O operations in case if an invalid (not cached) page
        is encountered, thus avoiding blocking on I/O.  Starting
        with FreeBSD 11 sendfile sending files off the ffs(7)
        filesystem does not block on I/O (see IMPLEMENTATION NOTES
        ), so the condition no longer applies.  However, it is safe
        if an application utilizes SF_NODISKIO and on EBUSY
        performs the same action as it did in older FreeBSD
        versions, e.g., aio_read(2), read(2) or sendfile in a
        different context.

I think it would probably be best to devise a way of locking the entire file via mlock (directly) or mmap (indirectly), and have it be a separate case.

I can add the simple positive case though -- you're correct.

asomers added inline comments.
lib/libc/tests/sys/sendfile_test.c
83 ↗(On Diff #52203)

I'll approve this DR as is, but I still think it would be best to go the tap/tun route.

This revision is now accepted and ready to land.Jan 12 2019, 4:28 PM
ngie marked an inline comment as done.Jan 13 2019, 2:58 AM
ngie added inline comments.
lib/libc/tests/sys/sendfile_test.c
83 ↗(On Diff #52203)

I really miss being able to file tasks from Phabricator :D...)

ngie marked an inline comment as done.Jan 23 2019, 8:48 PM
ngie added inline comments.
lib/libc/tests/sys/sendfile_test.c
921 ↗(On Diff #52718)

This case fails because the change I have out for review in https://reviews.freebsd.org/D18623 has not landed. I will mark this as an expected failure and work on moving that change forward by restructuring the API to match Linux's API (return the value in ssize_t, instead of having a separate field for sbytes). This can make the sendfile(2) API resemble read(2)/write(2)'s semantics.

Expect sbytes_negative to fail today for reasons cited in bug 232210.

D18623 has relevant discussion surrounding fixing bug 232210; please see that review for more details.

This revision now requires review to proceed.Jan 23 2019, 9:30 PM
This revision is now accepted and ready to land.Jan 23 2019, 9:30 PM
This revision was automatically updated to reflect the committed changes.