Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std: implement sendfile on linux #4131

Closed
wants to merge 1 commit into from

Conversation

terinjokes
Copy link
Contributor

This changset adds a sendfile(2) syscall bindings to the linux bits
component. Where available, the sendfile64(2) syscall will be
transparently called.

A wrapping function has also been added to the std.os to transform
errno returns to Zig errors.

Copy link
Contributor

@daurnimator daurnimator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also add a prototype in std/c.zig so that libc's function is used when linking against libc.

lib/std/os.zig Outdated
while (true) {
var rc: usize = undefined;
var err: usize = undefined;
if (builtin.os == .linux) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be an unconditional system.sendfile. If someone attempts to call this function on a system without it then they will get a compile error indicating as much.

@daurnimator daurnimator added os-linux standard library This issue involves writing Zig code for the standard library. labels Jan 10, 2020
@terinjokes
Copy link
Contributor Author

@daurnimator I had added it to c.zig locally, but building anything linking against C fails with an unrelated error, so I can't test if my changes are good.

@terinjokes
Copy link
Contributor Author

/usr/lib/zig/std/os.zig:3301:60: error: cast discards const qualifier
    switch (errno(system.setsockopt(fd, level, optname, opt.ptr, @intCast(socklen_t, opt.len)))) {

@terinjokes
Copy link
Contributor Author

terinjokes commented Jan 10, 2020

Per discussion on IRC, this PR can likely be extended with macOS and FreeBSD support:

  • Extend the os.zig wrapper to accept hdr, sbytes, and flags parameters.
  • Handle writing headers and trailers on systems where this isn't the default. Document what systems they're written atomically.
  • The BSD and Linux handle updates to the offset pointer different, so I may need to also document the behavior and what would be portable.

Windows has TransmitFile as part of mswsock, which looks similar. Will have to investigate farther.

lib/std/os.zig Outdated
Comment on lines 3118 to 3119
/// There was insufficient memory for reading from infd.
NoMem,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised (and disappointed) this error is even possible; the kernel should be able to make this "just work" even if it is forced to use a small stack-based buffer due to heap memory pressure.

By convention, ENOMEM in zig turns into one of these two things:

  • error.OutOfMemory if it is userspace heap memory that is exhausted
  • error.SystemResources if it is kernel space memory or other resources that are unavailable.

In this case I believe it would be error.SystemResources.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading through the kernel sources, this actually comes from files that have a mandatory lock, and the underlying splice() cannot return ENOMEM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shawnl Thanks, I can update the comment. I'll likely take @andrewrk's recommendation of using error.SystemResources in this case.

@terinjokes
Copy link
Contributor Author

I've pushed updates to the Linux support to accept the wider *BSD parameters, and to send the header and trailer iovecs. I doubt I have implemented the behavior to match correctly.

Next up to to add support sendfile(2) from FreeBSD libc. Then I'll be in a better place to verify the behavior is the same.

This changset adds a `sendfile(2)` syscall bindings to the linux bits
component. Where available, the `sendfile64(2)` syscall will be
transparently called.

A wrapping function has also been added to the std.os to transform
errno returns to Zig errors.

Change-Id: I86769fc4382c0771e3656e7b21137bafd99a4411
trailers: [*]iovec_const,
trl_cnt: c_int,
};
pub extern "c" fn sendfile(fd: c_int, s: c_int, offset: u64, nbytes: usize, sf_hdtr: ?*sf_hdtr, sbytes: ?*u64, flags: c_int) c_int;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to double check that the offset parameters here should be u64 instead of off_t.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think zig should support 32-bit off_t, in which case they are the same, but yeah probably still use off_t even if it must be u64.

@andrewrk
Copy link
Member

andrewrk commented Mar 3, 2020

Thanks! This is merged into #4611.

@andrewrk andrewrk closed this Mar 3, 2020
@terinjokes
Copy link
Contributor Author

Sorry for not getting this as polished as I liked. Thank you for taking the time to carry it through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os-linux standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants