diff --git a/README.md b/README.md index 25a446b..cac0b64 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ for Linux, written on the kernel's own system-call interface. openkal = "0.9.0" [target.'cfg(os = "linux")'.dependencies] -openkal-linux = "0.7.0" +openkal-linux = "0.7.1" ``` ## Why it does not use a C library diff --git a/mcpp.toml b/mcpp.toml index 1a3ab9d..b037951 100644 --- a/mcpp.toml +++ b/mcpp.toml @@ -1,7 +1,7 @@ [package] namespace = "mcpplibs" name = "openkal-linux" -version = "0.7.0" +version = "0.7.1" description = "The reference implementation of openkal for Linux, written on the kernel's own system-call interface so that it can be placed beneath a C library as well as above one." license = "Apache-2.0" diff --git a/src/handle.h b/src/handle.h index 2ab36b8..f0c0ea5 100644 --- a/src/handle.h +++ b/src/handle.h @@ -24,6 +24,18 @@ inline okl_uptr pack(int fd) { } // Returns the descriptor, or -1 if the word does not name a live one. +// +// THIS ACCEPTS A WORD THAT WAS NEVER PACKED, AND SILENTLY. A bare descriptor N +// has the shape of a packed handle naming N-1 whose generation is still zero, +// so this returns N-1 for it rather than -1. Nothing here can tell the two +// apart: the word is one machine word and carries no tag. +// +// The consequence is that a handle of the OTHER discipline must never reach +// this function. openkal.stream's handles are bare descriptors (stream.cpp +// states why), and src/timeout.cpp used to pass one here and wait upon the +// descriptor below the one it then transferred upon. Owned handles --- kal_file, +// kal_dir, kal_net_listener, kal_net_conn, kal_datagram --- are the whole of +// this function's domain. inline int unpack(okl_uptr h) { const int fd = static_cast(h & 0xffffffffu) - 1; if (fd < 0 || fd >= kMaxDescriptor) return -1; diff --git a/src/timeout.cpp b/src/timeout.cpp index c557c57..7870b9d 100644 --- a/src/timeout.cpp +++ b/src/timeout.cpp @@ -52,6 +52,33 @@ int await(int fd, short events, kal_u64 ns) { } } +// A STREAM HANDLE IS A DESCRIPTOR AND IS NOT DECODED. stream.cpp states it in +// terms, kal_stream_read and kal_stream_write take it as one, and so must the +// wait that precedes them: the wait and the transfer that follows it have to +// name the same object or the wait answers about something else. +// +// THIS FILE USED TO DECODE IT AS AN OWNED HANDLE, AND THE DECODE SUCCEEDED. +// handle.h packs an owned handle as (generation << 32) | (fd + 1), so a bare +// descriptor N has exactly the shape of a packed handle naming N-1, and +// okl::unpack accepts it whenever generation N-1 is still zero -- which is +// every index at which no owned handle has yet been released. The wait was +// therefore performed upon descriptor N-1 and the transfer upon N. +// +// The former reading tested `unpack` and fell back when it failed, on the +// stated ground that the standard streams are not packed. That ground is +// correct and the conclusion drawn from it was not: NO stream handle is packed +// here, and of the three standard ones only kal_stdin, whose handle is zero, +// fails to decode. kal_stdout decoded to descriptor 0 and kal_stderr to 1. +// +// What a caller observed was an expiry for a stream that had bytes waiting, or +// a wait without bound inside an operation that states one, according to what +// happened to occupy the descriptor below. Both are functions of the process's +// descriptor history, so the same program answered differently when run alone +// and when run after something else -- and the index healed for good once an +// owned handle at N-1 had been released, because that advances the generation +// and makes the decode fail correctly. +int stream_fd(kal_stream s) { return static_cast(s.h); } + } // namespace extern "C" { @@ -61,26 +88,24 @@ kal_intptr kal_timeout_read(kal_stream s, void* buf, kal_uintptr len, kal_u64 ns // would turn a call that always succeeds into one that can expire. if (len == 0) return 0; - const int fd = okl::unpack(s.h); - // THE STANDARD STREAMS ARE NOT PACKED HANDLES. openkal.stream reports them - // as the descriptors themselves, so a word that does not unpack is taken to - // be one of those rather than being refused. - const int use = (fd >= 0) ? fd : static_cast(s.h); - - if (const int rc = await(use, okl::poll_in, ns); rc != kal_ok) return -rc; + if (const int rc = await(stream_fd(s), okl::poll_in, ns); rc != kal_ok) return -rc; return kal_stream_read(s, buf, len); } kal_intptr kal_timeout_write(kal_stream s, const void* buf, kal_uintptr len, kal_u64 ns) { if (len == 0) return 0; - const int fd = okl::unpack(s.h); - const int use = (fd >= 0) ? fd : static_cast(s.h); - - if (const int rc = await(use, okl::poll_out, ns); rc != kal_ok) return -rc; + if (const int rc = await(stream_fd(s), okl::poll_out, ns); rc != kal_ok) return -rc; return kal_stream_write(s, buf, len); } +// THE TWO OPERATIONS BELOW DO DECODE, AND THAT IS NOT AN INCONSISTENCY WITH THE +// TWO ABOVE. A listener and a datagram are owned: their handles are made by +// okl::pack and released by okl::retire, so the decode is the operation that +// recovers the descriptor and its failure is how a released handle is refused. +// A stream is borrowed and carries no generation. The four call sites divide +// exactly along that line, and the two that were wrong were the two that had a +// borrowed handle in hand. int kal_timeout_accept(kal_net_listener l, kal_u64 ns, kal_net_conn* out) { if (out == nullptr) return kal_err_invalid; const int fd = okl::unpack(l.h); diff --git a/tests/conformance_v08.cpp b/tests/conformance_v08.cpp index 582ade8..71af4e5 100644 --- a/tests/conformance_v08.cpp +++ b/tests/conformance_v08.cpp @@ -190,6 +190,75 @@ void space_section() { kal_process_close(p.p); } +// A BOUNDED TRANSFER WAITS UPON THE STREAM IT THEN TRANSFERS UPON. +// +// This is the observation that was missing while the two operations it examines +// waited upon the descriptor below the one they transferred upon. Nothing else +// in this file reached them: the two observations in timeout_section below are +// of an accept, whose handle is owned and was decoded correctly, and of a +// zero-length write, which returns before it waits. +// +// WHY IT MAKES SIXTEEN CHANNELS AND NOT ONE. Under the defect the wait was +// performed upon descriptor N-1, and whether that expires depends on what +// occupies N-1. For a channel made after another, N-1 is the previous channel's +// writing end, upon which input is never reported, so the read expires while +// bytes sit in the stream that was asked for. One channel would be answering +// about whichever descriptor happened to precede it; sixteen make the answer a +// property of the implementation rather than of the process that ran it. +// +// AND WHY IT RUNS FIRST. The mistaken decode succeeded only while the +// generation recorded for N-1 was still zero, so it corrected itself for every +// index at which an owned handle had already been released. Placed after the +// sections that create and release listeners and datagrams, this would have +// examined reused descriptors and held upon the defect. +// +// The count is reported rather than the first failure. Sixteen distinguishes a +// correct implementation from one that answers about a neighbouring descriptor; +// the first failure alone would not say which. +void timeout_stream_section() { + constexpr int n = 16; + kal_stream mine[n]{}, theirs[n]{}; + int made = 0; + + for (; made < n; ++made) { + if (kal_process_channel(&mine[made], &theirs[made]) != kal_ok) break; + const char byte = 'x'; + if (kal_stream_write(theirs[made], &byte, 1) != 1) break; + } + check(made == n, "sixteen channels are created and each is written to"); + + int transferred = 0; + for (int i = 0; i < made; ++i) { + char buf[1] = {}; + // One millisecond. Every one of these streams has a byte waiting, so a + // correct implementation does not reach the bound at all; the bound is + // present so that a wait upon the wrong descriptor ends the run rather + // than hanging it. + if (kal::timeout::read(mine[i], buf, 1, 1000000) == 1 && buf[0] == 'x') + ++transferred; + } + if (transferred != made) + std::printf(" %d of %d bounded reads transferred\n", transferred, made); + check(transferred == made, + "a bounded read of a stream that has bytes waiting transfers them"); + + // The mirror. A stream with nothing in it must expire, and must expire + // rather than transfer a byte belonging to another stream. + kal_stream empty_mine{}, empty_theirs{}; + if (kal_process_channel(&empty_mine, &empty_theirs) == kal_ok) { + char buf[1] = {}; + check(kal::timeout::read(empty_mine, buf, 1, 1000000) == -kal_err_again, + "a bounded read of a stream with nothing waiting expires"); + kal_process_channel_close(empty_theirs); + kal_process_channel_close(empty_mine); + } + + for (int i = 0; i < made; ++i) { + kal_process_channel_close(theirs[i]); + kal_process_channel_close(mine[i]); + } +} + // openkal.timeout void timeout_section() { check(kal_timeout_granularity() > 0, @@ -257,6 +326,12 @@ void process_additions_section() { } // namespace int main() { + // FIRST, AND THE ORDER IS LOAD-BEARING. The comment above this section + // records why: it examines a defect that corrects itself for any descriptor + // index at which an owned handle has already been released, so running it + // after the sections that make and release listeners would hold upon the + // defect it exists to detect. + timeout_stream_section(); process_additions_section(); terminal_section(); net_section();