std.os read/write functions + sendfile by andrewrk · Pull Request #4611 · ziglang/zig

andrewrk · 2020-03-03T07:28:01Z

This merges #4131 and then does the following:

rework os.sendfile and add macosx support, and a fallback
implementation for any OS.
fix sendto compile error
std.os write functions support partial writes. closesstd.os.writev does not handle partial writes #3443.
std.os pread / pwrite functions can now return error.Unseekable.
std.fs.File read/write functions now have readAll/writeAll variants
which loop to complete operations even when partial reads/writes
happen.
Audit std.os read/write functions with respect to Linux returning
EINVAL for lengths greater than 0x7fff0000.
std.os read/write shim functions do not unnecessarily loop. Since
partial reads/writes are part of the API, the caller will be forced
to loop anyway, and so that would just be code bloat.
Improve doc comments
Add a non-trivial test for std.os.sendfile
Fix std.os.pread on 32 bit Linux
Add missing SYS_sendfile bit on aarch64

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

* rework os.sendfile and add macosx support, and a fallback implementation for any OS. * fix sendto compile error * std.os write functions support partial writes. closes#3443. * std.os pread / pwrite functions can now return `error.Unseekable`. * std.fs.File read/write functions now have readAll/writeAll variants which loop to complete operations even when partial reads/writes happen. * Audit std.os read/write functions with respect to Linux returning EINVAL for lengths greater than 0x7fff0000. * std.os read/write shim functions do not unnecessarily loop. Since partial reads/writes are part of the API, the caller will be forced to loop anyway, and so that would just be code bloat. * Improve doc comments * Add a non-trivial test for std.os.sendfile * Fix std.os.pread on 32 bit Linux * Add missing SYS_sendfile bit on aarch64

daurnimator · 2020-03-03T07:37:23Z

- if (amt_read == 0) return off; // EOF
- } else unreachable; // TODO https://github.com/ziglang/zig/issues/707
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to read an io vector?


yes. ReadFileScatter

Thanks- I will open an issue

daurnimator · 2020-03-03T07:38:54Z

- }
+pub fn preadv(fd: fd_t, iov: []const iovec, offset: u64) PReadError!usize {
+ const have_pread_but_not_preadv = switch (std.Target.current.os.tag) {
+ .windows, .macosx, .ios, .watchos, .tvos => true,


windows has NtReadFileScatter

daurnimator · 2020-03-03T07:39:32Z

+/// If `iov.len` is larger than will fit in a `u31`, a partial write will occur.
+pub fn writev(fd: fd_t, iov: []const iovec_const) WriteError!usize {
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to write an io vector?


WriteFileGather

daurnimator · 2020-03-03T07:41:46Z

+ }
+
+ rw: {
+ var buf: [8 * 4096]u8 = undefined;


Should use a LinearFifo here

It lets you gracefully handle partial reads and partial writes (which helps in both speed, number of syscalls and correctness)

This code is intentionally doing exactly one read/write and returning a partial write. The burden of proof is on you to show why LinearFifo would be better than this.

ah sorry, I thought there was a loop here.
That'll come in the fallback path for copy_file_range later :)

Even copy_file_range can return a partial write, so I'm not sure that's true- the loopy code is in std.fs.FilereadAll() and writeAll() related functions. And std.io.OutStream.write.

Wait... we do need a loop here. A partial read (non 0) should be followed by another read right?

Nope, any looping is redundant since the function can return partial writes, and so the callsite must do looping or itself return partials. In fact, it's actually providing more power and information to return on the first partial read or partial write, since that exposes how the underlying mechanism works. As an example, this is exploited by command line input reading a line from the terminal.

daurnimator · 2020-03-03T07:47:11Z

 pub const SYS_pwrite64 = 68;
 pub const SYS_preadv = 69;
 pub const SYS_pwritev = 70;
+pub const SYS_sendfile = 71;


o.o how odd that this was missed!

yeah... i made some confused noises out loud when I saw that, lol

daurnimator · 2020-03-03T07:51:53Z

+ const amt_read = try pread(in_fd, buf[0..adjusted_count], in_offset);
+ if (amt_read == 0) {
+ if (count == 0) {
+ // We have detected EOF from `in_fd`.


I think we should still write 0 bytes here.
This would have an effect if e.g. the previous "write" was a sendto without MSG_EOR

I think there's value in considering this. Can you make that a separate proposal? I think all the read/write functions would need to be audited if this were the policy.

andrewrk · 2020-03-03T07:54:32Z

🤦‍♂️ CI is failing because / in branch name

daurnimator · 2020-03-03T07:58:03Z

Will also fail on freebsd due to upstream bug (see my other pr)

andrewrk · 2020-03-03T07:58:04Z

=> #4612

andrewrk · 2020-03-03T07:28:01Z

This merges #4131 and then does the following:

rework os.sendfile and add macosx support, and a fallback
implementation for any OS.
fix sendto compile error
std.os write functions support partial writes. closesstd.os.writev does not handle partial writes #3443.
std.os pread / pwrite functions can now return error.Unseekable.
std.fs.File read/write functions now have readAll/writeAll variants
which loop to complete operations even when partial reads/writes
happen.
Audit std.os read/write functions with respect to Linux returning
EINVAL for lengths greater than 0x7fff0000.
std.os read/write shim functions do not unnecessarily loop. Since
partial reads/writes are part of the API, the caller will be forced
to loop anyway, and so that would just be code bloat.
Improve doc comments
Add a non-trivial test for std.os.sendfile
Fix std.os.pread on 32 bit Linux
Add missing SYS_sendfile bit on aarch64

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

* rework os.sendfile and add macosx support, and a fallback implementation for any OS. * fix sendto compile error * std.os write functions support partial writes. closes#3443. * std.os pread / pwrite functions can now return `error.Unseekable`. * std.fs.File read/write functions now have readAll/writeAll variants which loop to complete operations even when partial reads/writes happen. * Audit std.os read/write functions with respect to Linux returning EINVAL for lengths greater than 0x7fff0000. * std.os read/write shim functions do not unnecessarily loop. Since partial reads/writes are part of the API, the caller will be forced to loop anyway, and so that would just be code bloat. * Improve doc comments * Add a non-trivial test for std.os.sendfile * Fix std.os.pread on 32 bit Linux * Add missing SYS_sendfile bit on aarch64

daurnimator · 2020-03-03T07:37:23Z

- if (amt_read == 0) return off; // EOF
- } else unreachable; // TODO https://github.com/ziglang/zig/issues/707
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to read an io vector?


yes. ReadFileScatter

Thanks- I will open an issue

daurnimator · 2020-03-03T07:38:54Z

- }
+pub fn preadv(fd: fd_t, iov: []const iovec, offset: u64) PReadError!usize {
+ const have_pread_but_not_preadv = switch (std.Target.current.os.tag) {
+ .windows, .macosx, .ios, .watchos, .tvos => true,


windows has NtReadFileScatter

daurnimator · 2020-03-03T07:39:32Z

+/// If `iov.len` is larger than will fit in a `u31`, a partial write will occur.
+pub fn writev(fd: fd_t, iov: []const iovec_const) WriteError!usize {
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to write an io vector?


WriteFileGather

daurnimator · 2020-03-03T07:41:46Z

+ }
+
+ rw: {
+ var buf: [8 * 4096]u8 = undefined;


Should use a LinearFifo here

It lets you gracefully handle partial reads and partial writes (which helps in both speed, number of syscalls and correctness)

This code is intentionally doing exactly one read/write and returning a partial write. The burden of proof is on you to show why LinearFifo would be better than this.

ah sorry, I thought there was a loop here.
That'll come in the fallback path for copy_file_range later :)

Even copy_file_range can return a partial write, so I'm not sure that's true- the loopy code is in std.fs.FilereadAll() and writeAll() related functions. And std.io.OutStream.write.

Wait... we do need a loop here. A partial read (non 0) should be followed by another read right?

Nope, any looping is redundant since the function can return partial writes, and so the callsite must do looping or itself return partials. In fact, it's actually providing more power and information to return on the first partial read or partial write, since that exposes how the underlying mechanism works. As an example, this is exploited by command line input reading a line from the terminal.

daurnimator · 2020-03-03T07:47:11Z

 pub const SYS_pwrite64 = 68;
 pub const SYS_preadv = 69;
 pub const SYS_pwritev = 70;
+pub const SYS_sendfile = 71;


o.o how odd that this was missed!

yeah... i made some confused noises out loud when I saw that, lol

daurnimator · 2020-03-03T07:51:53Z

+ const amt_read = try pread(in_fd, buf[0..adjusted_count], in_offset);
+ if (amt_read == 0) {
+ if (count == 0) {
+ // We have detected EOF from `in_fd`.


I think we should still write 0 bytes here.
This would have an effect if e.g. the previous "write" was a sendto without MSG_EOR

I think there's value in considering this. Can you make that a separate proposal? I think all the read/write functions would need to be audited if this were the policy.

andrewrk · 2020-03-03T07:54:32Z

🤦‍♂️ CI is failing because / in branch name

daurnimator · 2020-03-03T07:58:03Z

Will also fail on freebsd due to upstream bug (see my other pr)

andrewrk · 2020-03-03T07:58:04Z

=> #4612

andrewrk · 2020-03-03T07:28:01Z

This merges #4131 and then does the following:

rework os.sendfile and add macosx support, and a fallback
implementation for any OS.
fix sendto compile error
std.os write functions support partial writes. closesstd.os.writev does not handle partial writes #3443.
std.os pread / pwrite functions can now return error.Unseekable.
std.fs.File read/write functions now have readAll/writeAll variants
which loop to complete operations even when partial reads/writes
happen.
Audit std.os read/write functions with respect to Linux returning
EINVAL for lengths greater than 0x7fff0000.
std.os read/write shim functions do not unnecessarily loop. Since
partial reads/writes are part of the API, the caller will be forced
to loop anyway, and so that would just be code bloat.
Improve doc comments
Add a non-trivial test for std.os.sendfile
Fix std.os.pread on 32 bit Linux
Add missing SYS_sendfile bit on aarch64

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

* rework os.sendfile and add macosx support, and a fallback implementation for any OS. * fix sendto compile error * std.os write functions support partial writes. closes#3443. * std.os pread / pwrite functions can now return `error.Unseekable`. * std.fs.File read/write functions now have readAll/writeAll variants which loop to complete operations even when partial reads/writes happen. * Audit std.os read/write functions with respect to Linux returning EINVAL for lengths greater than 0x7fff0000. * std.os read/write shim functions do not unnecessarily loop. Since partial reads/writes are part of the API, the caller will be forced to loop anyway, and so that would just be code bloat. * Improve doc comments * Add a non-trivial test for std.os.sendfile * Fix std.os.pread on 32 bit Linux * Add missing SYS_sendfile bit on aarch64

daurnimator · 2020-03-03T07:37:23Z

- if (amt_read == 0) return off; // EOF
- } else unreachable; // TODO https://github.com/ziglang/zig/issues/707
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to read an io vector?


yes. ReadFileScatter

Thanks- I will open an issue

daurnimator · 2020-03-03T07:38:54Z

- }
+pub fn preadv(fd: fd_t, iov: []const iovec, offset: u64) PReadError!usize {
+ const have_pread_but_not_preadv = switch (std.Target.current.os.tag) {
+ .windows, .macosx, .ios, .watchos, .tvos => true,


windows has NtReadFileScatter

daurnimator · 2020-03-03T07:39:32Z

+/// If `iov.len` is larger than will fit in a `u31`, a partial write will occur.
+pub fn writev(fd: fd_t, iov: []const iovec_const) WriteError!usize {
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to write an io vector?


WriteFileGather

daurnimator · 2020-03-03T07:41:46Z

+ }
+
+ rw: {
+ var buf: [8 * 4096]u8 = undefined;


Should use a LinearFifo here

It lets you gracefully handle partial reads and partial writes (which helps in both speed, number of syscalls and correctness)

This code is intentionally doing exactly one read/write and returning a partial write. The burden of proof is on you to show why LinearFifo would be better than this.

ah sorry, I thought there was a loop here.
That'll come in the fallback path for copy_file_range later :)

Even copy_file_range can return a partial write, so I'm not sure that's true- the loopy code is in std.fs.FilereadAll() and writeAll() related functions. And std.io.OutStream.write.

Wait... we do need a loop here. A partial read (non 0) should be followed by another read right?

Nope, any looping is redundant since the function can return partial writes, and so the callsite must do looping or itself return partials. In fact, it's actually providing more power and information to return on the first partial read or partial write, since that exposes how the underlying mechanism works. As an example, this is exploited by command line input reading a line from the terminal.

daurnimator · 2020-03-03T07:47:11Z

 pub const SYS_pwrite64 = 68;
 pub const SYS_preadv = 69;
 pub const SYS_pwritev = 70;
+pub const SYS_sendfile = 71;


o.o how odd that this was missed!

yeah... i made some confused noises out loud when I saw that, lol

daurnimator · 2020-03-03T07:51:53Z

+ const amt_read = try pread(in_fd, buf[0..adjusted_count], in_offset);
+ if (amt_read == 0) {
+ if (count == 0) {
+ // We have detected EOF from `in_fd`.


I think we should still write 0 bytes here.
This would have an effect if e.g. the previous "write" was a sendto without MSG_EOR

I think there's value in considering this. Can you make that a separate proposal? I think all the read/write functions would need to be audited if this were the policy.

andrewrk · 2020-03-03T07:54:32Z

🤦‍♂️ CI is failing because / in branch name

daurnimator · 2020-03-03T07:58:03Z

Will also fail on freebsd due to upstream bug (see my other pr)

andrewrk · 2020-03-03T07:58:04Z

=> #4612

andrewrk · 2020-03-03T07:28:01Z

This merges #4131 and then does the following:

rework os.sendfile and add macosx support, and a fallback
implementation for any OS.
fix sendto compile error
std.os write functions support partial writes. closesstd.os.writev does not handle partial writes #3443.
std.os pread / pwrite functions can now return error.Unseekable.
std.fs.File read/write functions now have readAll/writeAll variants
which loop to complete operations even when partial reads/writes
happen.
Audit std.os read/write functions with respect to Linux returning
EINVAL for lengths greater than 0x7fff0000.
std.os read/write shim functions do not unnecessarily loop. Since
partial reads/writes are part of the API, the caller will be forced
to loop anyway, and so that would just be code bloat.
Improve doc comments
Add a non-trivial test for std.os.sendfile
Fix std.os.pread on 32 bit Linux
Add missing SYS_sendfile bit on aarch64

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

* rework os.sendfile and add macosx support, and a fallback implementation for any OS. * fix sendto compile error * std.os write functions support partial writes. closes#3443. * std.os pread / pwrite functions can now return `error.Unseekable`. * std.fs.File read/write functions now have readAll/writeAll variants which loop to complete operations even when partial reads/writes happen. * Audit std.os read/write functions with respect to Linux returning EINVAL for lengths greater than 0x7fff0000. * std.os read/write shim functions do not unnecessarily loop. Since partial reads/writes are part of the API, the caller will be forced to loop anyway, and so that would just be code bloat. * Improve doc comments * Add a non-trivial test for std.os.sendfile * Fix std.os.pread on 32 bit Linux * Add missing SYS_sendfile bit on aarch64

daurnimator · 2020-03-03T07:37:23Z

- if (amt_read == 0) return off; // EOF
- } else unreachable; // TODO https://github.com/ziglang/zig/issues/707
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to read an io vector?


yes. ReadFileScatter

Thanks- I will open an issue

daurnimator · 2020-03-03T07:38:54Z

- }
+pub fn preadv(fd: fd_t, iov: []const iovec, offset: u64) PReadError!usize {
+ const have_pread_but_not_preadv = switch (std.Target.current.os.tag) {
+ .windows, .macosx, .ios, .watchos, .tvos => true,


windows has NtReadFileScatter

daurnimator · 2020-03-03T07:39:32Z

+/// If `iov.len` is larger than will fit in a `u31`, a partial write will occur.
+pub fn writev(fd: fd_t, iov: []const iovec_const) WriteError!usize {
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to write an io vector?


WriteFileGather

daurnimator · 2020-03-03T07:41:46Z

+ }
+
+ rw: {
+ var buf: [8 * 4096]u8 = undefined;


Should use a LinearFifo here

It lets you gracefully handle partial reads and partial writes (which helps in both speed, number of syscalls and correctness)

This code is intentionally doing exactly one read/write and returning a partial write. The burden of proof is on you to show why LinearFifo would be better than this.

ah sorry, I thought there was a loop here.
That'll come in the fallback path for copy_file_range later :)

Even copy_file_range can return a partial write, so I'm not sure that's true- the loopy code is in std.fs.FilereadAll() and writeAll() related functions. And std.io.OutStream.write.

Wait... we do need a loop here. A partial read (non 0) should be followed by another read right?

Nope, any looping is redundant since the function can return partial writes, and so the callsite must do looping or itself return partials. In fact, it's actually providing more power and information to return on the first partial read or partial write, since that exposes how the underlying mechanism works. As an example, this is exploited by command line input reading a line from the terminal.

daurnimator · 2020-03-03T07:47:11Z

 pub const SYS_pwrite64 = 68;
 pub const SYS_preadv = 69;
 pub const SYS_pwritev = 70;
+pub const SYS_sendfile = 71;


o.o how odd that this was missed!

yeah... i made some confused noises out loud when I saw that, lol

daurnimator · 2020-03-03T07:51:53Z

+ const amt_read = try pread(in_fd, buf[0..adjusted_count], in_offset);
+ if (amt_read == 0) {
+ if (count == 0) {
+ // We have detected EOF from `in_fd`.


I think we should still write 0 bytes here.
This would have an effect if e.g. the previous "write" was a sendto without MSG_EOR

I think there's value in considering this. Can you make that a separate proposal? I think all the read/write functions would need to be audited if this were the policy.

andrewrk · 2020-03-03T07:54:32Z

🤦‍♂️ CI is failing because / in branch name

daurnimator · 2020-03-03T07:58:03Z

Will also fail on freebsd due to upstream bug (see my other pr)

andrewrk · 2020-03-03T07:58:04Z

=> #4612

andrewrk · 2020-03-03T07:28:01Z

This merges #4131 and then does the following:

rework os.sendfile and add macosx support, and a fallback
implementation for any OS.
fix sendto compile error
std.os write functions support partial writes. closesstd.os.writev does not handle partial writes #3443.
std.os pread / pwrite functions can now return error.Unseekable.
std.fs.File read/write functions now have readAll/writeAll variants
which loop to complete operations even when partial reads/writes
happen.
Audit std.os read/write functions with respect to Linux returning
EINVAL for lengths greater than 0x7fff0000.
std.os read/write shim functions do not unnecessarily loop. Since
partial reads/writes are part of the API, the caller will be forced
to loop anyway, and so that would just be code bloat.
Improve doc comments
Add a non-trivial test for std.os.sendfile
Fix std.os.pread on 32 bit Linux
Add missing SYS_sendfile bit on aarch64

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

* rework os.sendfile and add macosx support, and a fallback implementation for any OS. * fix sendto compile error * std.os write functions support partial writes. closes#3443. * std.os pread / pwrite functions can now return `error.Unseekable`. * std.fs.File read/write functions now have readAll/writeAll variants which loop to complete operations even when partial reads/writes happen. * Audit std.os read/write functions with respect to Linux returning EINVAL for lengths greater than 0x7fff0000. * std.os read/write shim functions do not unnecessarily loop. Since partial reads/writes are part of the API, the caller will be forced to loop anyway, and so that would just be code bloat. * Improve doc comments * Add a non-trivial test for std.os.sendfile * Fix std.os.pread on 32 bit Linux * Add missing SYS_sendfile bit on aarch64

daurnimator · 2020-03-03T07:37:23Z

- if (amt_read == 0) return off; // EOF
- } else unreachable; // TODO https://github.com/ziglang/zig/issues/707
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to read an io vector?


yes. ReadFileScatter

Thanks- I will open an issue

daurnimator · 2020-03-03T07:38:54Z

- }
+pub fn preadv(fd: fd_t, iov: []const iovec, offset: u64) PReadError!usize {
+ const have_pread_but_not_preadv = switch (std.Target.current.os.tag) {
+ .windows, .macosx, .ios, .watchos, .tvos => true,


windows has NtReadFileScatter

daurnimator · 2020-03-03T07:39:32Z

+/// If `iov.len` is larger than will fit in a `u31`, a partial write will occur.
+pub fn writev(fd: fd_t, iov: []const iovec_const) WriteError!usize {
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to write an io vector?


WriteFileGather

daurnimator · 2020-03-03T07:41:46Z

+ }
+
+ rw: {
+ var buf: [8 * 4096]u8 = undefined;


Should use a LinearFifo here

It lets you gracefully handle partial reads and partial writes (which helps in both speed, number of syscalls and correctness)

This code is intentionally doing exactly one read/write and returning a partial write. The burden of proof is on you to show why LinearFifo would be better than this.

ah sorry, I thought there was a loop here.
That'll come in the fallback path for copy_file_range later :)

Even copy_file_range can return a partial write, so I'm not sure that's true- the loopy code is in std.fs.FilereadAll() and writeAll() related functions. And std.io.OutStream.write.

Wait... we do need a loop here. A partial read (non 0) should be followed by another read right?

Nope, any looping is redundant since the function can return partial writes, and so the callsite must do looping or itself return partials. In fact, it's actually providing more power and information to return on the first partial read or partial write, since that exposes how the underlying mechanism works. As an example, this is exploited by command line input reading a line from the terminal.

daurnimator · 2020-03-03T07:47:11Z

 pub const SYS_pwrite64 = 68;
 pub const SYS_preadv = 69;
 pub const SYS_pwritev = 70;
+pub const SYS_sendfile = 71;


o.o how odd that this was missed!

yeah... i made some confused noises out loud when I saw that, lol

daurnimator · 2020-03-03T07:51:53Z

+ const amt_read = try pread(in_fd, buf[0..adjusted_count], in_offset);
+ if (amt_read == 0) {
+ if (count == 0) {
+ // We have detected EOF from `in_fd`.


I think we should still write 0 bytes here.
This would have an effect if e.g. the previous "write" was a sendto without MSG_EOR

I think there's value in considering this. Can you make that a separate proposal? I think all the read/write functions would need to be audited if this were the policy.

andrewrk · 2020-03-03T07:54:32Z

🤦‍♂️ CI is failing because / in branch name

daurnimator · 2020-03-03T07:58:03Z

Will also fail on freebsd due to upstream bug (see my other pr)

andrewrk · 2020-03-03T07:58:04Z

=> #4612

andrewrk · 2020-03-03T07:28:01Z

This merges #4131 and then does the following:

rework os.sendfile and add macosx support, and a fallback
implementation for any OS.
fix sendto compile error
std.os write functions support partial writes. closesstd.os.writev does not handle partial writes #3443.
std.os pread / pwrite functions can now return error.Unseekable.
std.fs.File read/write functions now have readAll/writeAll variants
which loop to complete operations even when partial reads/writes
happen.
Audit std.os read/write functions with respect to Linux returning
EINVAL for lengths greater than 0x7fff0000.
std.os read/write shim functions do not unnecessarily loop. Since
partial reads/writes are part of the API, the caller will be forced
to loop anyway, and so that would just be code bloat.
Improve doc comments
Add a non-trivial test for std.os.sendfile
Fix std.os.pread on 32 bit Linux
Add missing SYS_sendfile bit on aarch64

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

* rework os.sendfile and add macosx support, and a fallback implementation for any OS. * fix sendto compile error * std.os write functions support partial writes. closes#3443. * std.os pread / pwrite functions can now return `error.Unseekable`. * std.fs.File read/write functions now have readAll/writeAll variants which loop to complete operations even when partial reads/writes happen. * Audit std.os read/write functions with respect to Linux returning EINVAL for lengths greater than 0x7fff0000. * std.os read/write shim functions do not unnecessarily loop. Since partial reads/writes are part of the API, the caller will be forced to loop anyway, and so that would just be code bloat. * Improve doc comments * Add a non-trivial test for std.os.sendfile * Fix std.os.pread on 32 bit Linux * Add missing SYS_sendfile bit on aarch64

daurnimator · 2020-03-03T07:37:23Z

- if (amt_read == 0) return off; // EOF
- } else unreachable; // TODO https://github.com/ziglang/zig/issues/707
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to read an io vector?


yes. ReadFileScatter

Thanks- I will open an issue

daurnimator · 2020-03-03T07:38:54Z

- }
+pub fn preadv(fd: fd_t, iov: []const iovec, offset: u64) PReadError!usize {
+ const have_pread_but_not_preadv = switch (std.Target.current.os.tag) {
+ .windows, .macosx, .ios, .watchos, .tvos => true,


windows has NtReadFileScatter

daurnimator · 2020-03-03T07:39:32Z

+/// If `iov.len` is larger than will fit in a `u31`, a partial write will occur.
+pub fn writev(fd: fd_t, iov: []const iovec_const) WriteError!usize {
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to write an io vector?


WriteFileGather

daurnimator · 2020-03-03T07:41:46Z

+ }
+
+ rw: {
+ var buf: [8 * 4096]u8 = undefined;


Should use a LinearFifo here

It lets you gracefully handle partial reads and partial writes (which helps in both speed, number of syscalls and correctness)

This code is intentionally doing exactly one read/write and returning a partial write. The burden of proof is on you to show why LinearFifo would be better than this.

ah sorry, I thought there was a loop here.
That'll come in the fallback path for copy_file_range later :)

Even copy_file_range can return a partial write, so I'm not sure that's true- the loopy code is in std.fs.FilereadAll() and writeAll() related functions. And std.io.OutStream.write.

Wait... we do need a loop here. A partial read (non 0) should be followed by another read right?

Nope, any looping is redundant since the function can return partial writes, and so the callsite must do looping or itself return partials. In fact, it's actually providing more power and information to return on the first partial read or partial write, since that exposes how the underlying mechanism works. As an example, this is exploited by command line input reading a line from the terminal.

daurnimator · 2020-03-03T07:47:11Z

 pub const SYS_pwrite64 = 68;
 pub const SYS_preadv = 69;
 pub const SYS_pwritev = 70;
+pub const SYS_sendfile = 71;


o.o how odd that this was missed!

yeah... i made some confused noises out loud when I saw that, lol

daurnimator · 2020-03-03T07:51:53Z

+ const amt_read = try pread(in_fd, buf[0..adjusted_count], in_offset);
+ if (amt_read == 0) {
+ if (count == 0) {
+ // We have detected EOF from `in_fd`.


I think we should still write 0 bytes here.
This would have an effect if e.g. the previous "write" was a sendto without MSG_EOR

I think there's value in considering this. Can you make that a separate proposal? I think all the read/write functions would need to be audited if this were the policy.

andrewrk · 2020-03-03T07:54:32Z

🤦‍♂️ CI is failing because / in branch name

daurnimator · 2020-03-03T07:58:03Z

Will also fail on freebsd due to upstream bug (see my other pr)

andrewrk · 2020-03-03T07:58:04Z

=> #4612

andrewrk · 2020-03-03T07:28:01Z

This merges #4131 and then does the following:

rework os.sendfile and add macosx support, and a fallback
implementation for any OS.
fix sendto compile error
std.os write functions support partial writes. closesstd.os.writev does not handle partial writes #3443.
std.os pread / pwrite functions can now return error.Unseekable.
std.fs.File read/write functions now have readAll/writeAll variants
which loop to complete operations even when partial reads/writes
happen.
Audit std.os read/write functions with respect to Linux returning
EINVAL for lengths greater than 0x7fff0000.
std.os read/write shim functions do not unnecessarily loop. Since
partial reads/writes are part of the API, the caller will be forced
to loop anyway, and so that would just be code bloat.
Improve doc comments
Add a non-trivial test for std.os.sendfile
Fix std.os.pread on 32 bit Linux
Add missing SYS_sendfile bit on aarch64

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

* rework os.sendfile and add macosx support, and a fallback implementation for any OS. * fix sendto compile error * std.os write functions support partial writes. closes#3443. * std.os pread / pwrite functions can now return `error.Unseekable`. * std.fs.File read/write functions now have readAll/writeAll variants which loop to complete operations even when partial reads/writes happen. * Audit std.os read/write functions with respect to Linux returning EINVAL for lengths greater than 0x7fff0000. * std.os read/write shim functions do not unnecessarily loop. Since partial reads/writes are part of the API, the caller will be forced to loop anyway, and so that would just be code bloat. * Improve doc comments * Add a non-trivial test for std.os.sendfile * Fix std.os.pread on 32 bit Linux * Add missing SYS_sendfile bit on aarch64

daurnimator · 2020-03-03T07:37:23Z

- if (amt_read == 0) return off; // EOF
- } else unreachable; // TODO https://github.com/ziglang/zig/issues/707
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to read an io vector?


yes. ReadFileScatter

Thanks- I will open an issue

daurnimator · 2020-03-03T07:38:54Z

- }
+pub fn preadv(fd: fd_t, iov: []const iovec, offset: u64) PReadError!usize {
+ const have_pread_but_not_preadv = switch (std.Target.current.os.tag) {
+ .windows, .macosx, .ios, .watchos, .tvos => true,


windows has NtReadFileScatter

daurnimator · 2020-03-03T07:39:32Z

+/// If `iov.len` is larger than will fit in a `u31`, a partial write will occur.
+pub fn writev(fd: fd_t, iov: []const iovec_const) WriteError!usize {
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to write an io vector?


WriteFileGather

daurnimator · 2020-03-03T07:41:46Z

+ }
+
+ rw: {
+ var buf: [8 * 4096]u8 = undefined;


Should use a LinearFifo here

It lets you gracefully handle partial reads and partial writes (which helps in both speed, number of syscalls and correctness)

This code is intentionally doing exactly one read/write and returning a partial write. The burden of proof is on you to show why LinearFifo would be better than this.

ah sorry, I thought there was a loop here.
That'll come in the fallback path for copy_file_range later :)

Even copy_file_range can return a partial write, so I'm not sure that's true- the loopy code is in std.fs.FilereadAll() and writeAll() related functions. And std.io.OutStream.write.

Wait... we do need a loop here. A partial read (non 0) should be followed by another read right?

Nope, any looping is redundant since the function can return partial writes, and so the callsite must do looping or itself return partials. In fact, it's actually providing more power and information to return on the first partial read or partial write, since that exposes how the underlying mechanism works. As an example, this is exploited by command line input reading a line from the terminal.

daurnimator · 2020-03-03T07:47:11Z

 pub const SYS_pwrite64 = 68;
 pub const SYS_preadv = 69;
 pub const SYS_pwritev = 70;
+pub const SYS_sendfile = 71;


o.o how odd that this was missed!

yeah... i made some confused noises out loud when I saw that, lol

daurnimator · 2020-03-03T07:51:53Z

+ const amt_read = try pread(in_fd, buf[0..adjusted_count], in_offset);
+ if (amt_read == 0) {
+ if (count == 0) {
+ // We have detected EOF from `in_fd`.


I think we should still write 0 bytes here.
This would have an effect if e.g. the previous "write" was a sendto without MSG_EOR

I think there's value in considering this. Can you make that a separate proposal? I think all the read/write functions would need to be audited if this were the policy.

andrewrk · 2020-03-03T07:54:32Z

🤦‍♂️ CI is failing because / in branch name

daurnimator · 2020-03-03T07:58:03Z

Will also fail on freebsd due to upstream bug (see my other pr)

andrewrk · 2020-03-03T07:58:04Z

=> #4612

andrewrk · 2020-03-03T07:28:01Z

This merges #4131 and then does the following:

rework os.sendfile and add macosx support, and a fallback
implementation for any OS.
fix sendto compile error
std.os write functions support partial writes. closesstd.os.writev does not handle partial writes #3443.
std.os pread / pwrite functions can now return error.Unseekable.
std.fs.File read/write functions now have readAll/writeAll variants
which loop to complete operations even when partial reads/writes
happen.
Audit std.os read/write functions with respect to Linux returning
EINVAL for lengths greater than 0x7fff0000.
std.os read/write shim functions do not unnecessarily loop. Since
partial reads/writes are part of the API, the caller will be forced
to loop anyway, and so that would just be code bloat.
Improve doc comments
Add a non-trivial test for std.os.sendfile
Fix std.os.pread on 32 bit Linux
Add missing SYS_sendfile bit on aarch64

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

* rework os.sendfile and add macosx support, and a fallback implementation for any OS. * fix sendto compile error * std.os write functions support partial writes. closes#3443. * std.os pread / pwrite functions can now return `error.Unseekable`. * std.fs.File read/write functions now have readAll/writeAll variants which loop to complete operations even when partial reads/writes happen. * Audit std.os read/write functions with respect to Linux returning EINVAL for lengths greater than 0x7fff0000. * std.os read/write shim functions do not unnecessarily loop. Since partial reads/writes are part of the API, the caller will be forced to loop anyway, and so that would just be code bloat. * Improve doc comments * Add a non-trivial test for std.os.sendfile * Fix std.os.pread on 32 bit Linux * Add missing SYS_sendfile bit on aarch64

daurnimator · 2020-03-03T07:37:23Z

- if (amt_read == 0) return off; // EOF
- } else unreachable; // TODO https://github.com/ziglang/zig/issues/707
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to read an io vector?


yes. ReadFileScatter

Thanks- I will open an issue

daurnimator · 2020-03-03T07:38:54Z

- }
+pub fn preadv(fd: fd_t, iov: []const iovec, offset: u64) PReadError!usize {
+ const have_pread_but_not_preadv = switch (std.Target.current.os.tag) {
+ .windows, .macosx, .ios, .watchos, .tvos => true,


windows has NtReadFileScatter

daurnimator · 2020-03-03T07:39:32Z

+/// If `iov.len` is larger than will fit in a `u31`, a partial write will occur.
+pub fn writev(fd: fd_t, iov: []const iovec_const) WriteError!usize {
+ if (std.Target.current.os.tag == .windows) {
+ // TODO does Windows have a way to write an io vector?


WriteFileGather

daurnimator · 2020-03-03T07:41:46Z

+ }
+
+ rw: {
+ var buf: [8 * 4096]u8 = undefined;


Should use a LinearFifo here

It lets you gracefully handle partial reads and partial writes (which helps in both speed, number of syscalls and correctness)

This code is intentionally doing exactly one read/write and returning a partial write. The burden of proof is on you to show why LinearFifo would be better than this.

ah sorry, I thought there was a loop here.
That'll come in the fallback path for copy_file_range later :)

Even copy_file_range can return a partial write, so I'm not sure that's true- the loopy code is in std.fs.FilereadAll() and writeAll() related functions. And std.io.OutStream.write.

Wait... we do need a loop here. A partial read (non 0) should be followed by another read right?

Nope, any looping is redundant since the function can return partial writes, and so the callsite must do looping or itself return partials. In fact, it's actually providing more power and information to return on the first partial read or partial write, since that exposes how the underlying mechanism works. As an example, this is exploited by command line input reading a line from the terminal.

daurnimator · 2020-03-03T07:47:11Z

 pub const SYS_pwrite64 = 68;
 pub const SYS_preadv = 69;
 pub const SYS_pwritev = 70;
+pub const SYS_sendfile = 71;


o.o how odd that this was missed!

yeah... i made some confused noises out loud when I saw that, lol

daurnimator · 2020-03-03T07:51:53Z

+ const amt_read = try pread(in_fd, buf[0..adjusted_count], in_offset);
+ if (amt_read == 0) {
+ if (count == 0) {
+ // We have detected EOF from `in_fd`.


I think we should still write 0 bytes here.
This would have an effect if e.g. the previous "write" was a sendto without MSG_EOR

I think there's value in considering this. Can you make that a separate proposal? I think all the read/write functions would need to be audited if this were the policy.

andrewrk · 2020-03-03T07:54:32Z

🤦‍♂️ CI is failing because / in branch name

daurnimator · 2020-03-03T07:58:03Z

Will also fail on freebsd due to upstream bug (see my other pr)

andrewrk · 2020-03-03T07:58:04Z

=> #4612

terinjokesand others added 2 commits March 2, 2020 12:54

andrewrk mentioned this pull request Mar 3, 2020
std: implement sendfile on linux #4131
Closed

daurnimator suggested changes Mar 3, 2020
View reviewed changes

daurnimator reviewed Mar 3, 2020
View reviewed changes

andrewrk closed this Mar 3, 2020

andrewrk deleted the terinjokes-patches/sendfile branch March 3, 2020 07:58

terinjokesand others added 2 commits March 2, 2020 12:54

andrewrk mentioned this pull request Mar 3, 2020
std: implement sendfile on linux #4131
Closed

daurnimator suggested changes Mar 3, 2020
View reviewed changes

daurnimator reviewed Mar 3, 2020
View reviewed changes

andrewrk closed this Mar 3, 2020

andrewrk deleted the terinjokes-patches/sendfile branch March 3, 2020 07:58

terinjokesand others added 2 commits March 2, 2020 12:54

andrewrk mentioned this pull request Mar 3, 2020
std: implement sendfile on linux #4131
Closed

daurnimator suggested changes Mar 3, 2020
View reviewed changes

daurnimator reviewed Mar 3, 2020
View reviewed changes

andrewrk closed this Mar 3, 2020

andrewrk deleted the terinjokes-patches/sendfile branch March 3, 2020 07:58

terinjokesand others added 2 commits March 2, 2020 12:54

andrewrk mentioned this pull request Mar 3, 2020
std: implement sendfile on linux #4131
Closed

daurnimator suggested changes Mar 3, 2020
View reviewed changes

daurnimator reviewed Mar 3, 2020
View reviewed changes

andrewrk closed this Mar 3, 2020

andrewrk deleted the terinjokes-patches/sendfile branch March 3, 2020 07:58

terinjokesand others added 2 commits March 2, 2020 12:54

andrewrk mentioned this pull request Mar 3, 2020
std: implement sendfile on linux #4131
Closed

daurnimator suggested changes Mar 3, 2020
View reviewed changes

daurnimator reviewed Mar 3, 2020
View reviewed changes

andrewrk closed this Mar 3, 2020

andrewrk deleted the terinjokes-patches/sendfile branch March 3, 2020 07:58

terinjokesand others added 2 commits March 2, 2020 12:54

andrewrk mentioned this pull request Mar 3, 2020
std: implement sendfile on linux #4131
Closed

daurnimator suggested changes Mar 3, 2020
View reviewed changes

daurnimator reviewed Mar 3, 2020
View reviewed changes

andrewrk closed this Mar 3, 2020

andrewrk deleted the terinjokes-patches/sendfile branch March 3, 2020 07:58

terinjokesand others added 2 commits March 2, 2020 12:54

andrewrk mentioned this pull request Mar 3, 2020
std: implement sendfile on linux #4131
Closed

daurnimator suggested changes Mar 3, 2020
View reviewed changes

daurnimator reviewed Mar 3, 2020
View reviewed changes

andrewrk closed this Mar 3, 2020

andrewrk deleted the terinjokes-patches/sendfile branch March 3, 2020 07:58

terinjokesand others added 2 commits March 2, 2020 12:54

andrewrk mentioned this pull request Mar 3, 2020
std: implement sendfile on linux #4131
Closed

daurnimator suggested changes Mar 3, 2020
View reviewed changes

daurnimator reviewed Mar 3, 2020
View reviewed changes

andrewrk closed this Mar 3, 2020

andrewrk deleted the terinjokes-patches/sendfile branch March 3, 2020 07:58

Uh oh!

Uh oh!

Conversation

andrewrk commented Mar 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewrkMar 3, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewrk commented Mar 3, 2020

Uh oh!

daurnimator commented Mar 3, 2020

Uh oh!

andrewrk commented Mar 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

andrewrk commented Mar 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewrkMar 3, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewrkMar 3, 2020•
edited
Loading

andrewrkMar 3, 2020•
edited
Loading