From 76806080ef1de85537fec0adc33304c719cb4cc4 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Thu, 3 Oct 2024 00:14:26 +0200 Subject: [PATCH 01/21] fix os2.process_exec on non-windows and add a smoke test --- core/os/os2/pipe_posix.odin | 8 ++-- core/os/os2/process.odin | 76 ++++++++++++++++++++-------------- tests/core/normal.odin | 1 + tests/core/os/os2/process.odin | 28 +++++++++++++ 4 files changed, 78 insertions(+), 35 deletions(-) create mode 100644 tests/core/os/os2/process.odin diff --git a/core/os/os2/pipe_posix.odin b/core/os/os2/pipe_posix.odin index 463f29f01..df9425339 100644 --- a/core/os/os2/pipe_posix.odin +++ b/core/os/os2/pipe_posix.odin @@ -49,7 +49,7 @@ _pipe_has_data :: proc(r: ^File) -> (ok: bool, err: Error) { if r == nil || r.impl == nil { return false, nil } - fd := posix.FD((^File_Impl)(r.impl).fd) + fd := __fd(r) poll_fds := []posix.pollfd { posix.pollfd { fd = fd, @@ -57,8 +57,10 @@ _pipe_has_data :: proc(r: ^File) -> (ok: bool, err: Error) { }, } n := posix.poll(raw_data(poll_fds), u32(len(poll_fds)), 0) - if n != 1 { + if n < 0 { return false, _get_platform_error() + } else if n != 1 { + return false, nil } pipe_events := poll_fds[0].revents if pipe_events >= {.IN} { @@ -68,4 +70,4 @@ _pipe_has_data :: proc(r: ^File) -> (ok: bool, err: Error) { return false, .Broken_Pipe } return false, nil -} \ No newline at end of file +} diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index fd06dca74..94d9732cb 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -371,16 +371,18 @@ process_exec :: proc( loc := #caller_location, ) -> ( state: Process_State, - stdout: []u8, - stderr: []u8, + stdout: []byte, + stderr: []byte, err: Error, ) { assert(desc.stdout == nil, "Cannot redirect stdout when it's being captured", loc) assert(desc.stderr == nil, "Cannot redirect stderr when it's being captured", loc) + stdout_r, stdout_w := pipe() or_return defer close(stdout_r) stderr_r, stderr_w := pipe() or_return defer close(stdout_w) + process: Process { // NOTE(flysand): Make sure the write-ends are closed, regardless @@ -392,44 +394,54 @@ process_exec :: proc( desc.stderr = stderr_w process = process_start(desc) or_return } + stdout_builder := strings.builder_make(allocator) or_return stderr_builder := strings.builder_make(allocator) or_return - read_data: for { - buf: [1024]u8 + + has_stdout, has_stderr: bool + read_data: for !has_stdout || !has_stderr { + buf: [1024]u8 = --- n: int has_data: bool - hangup := false - has_data, err = pipe_has_data(stdout_r) - if has_data { - n, err = read(stdout_r, buf[:]) - strings.write_bytes(&stdout_builder, buf[:n]) + + if !has_stdout { + has_data, err = pipe_has_data(stdout_r) + if has_data { + n, err = read(stdout_r, buf[:]) + if strings.write_bytes(&stdout_builder, buf[:n]) != n { + err = .Out_Of_Memory + } + } + switch err { + case nil: // nothing + case .EOF, .Broken_Pipe: + stdout = stdout_builder.buf[:] + has_stdout = true + case: + return + } } - switch err { - case nil: // nothing - case .Broken_Pipe: - hangup = true - case: - return - } - has_data, err = pipe_has_data(stderr_r) - if has_data { - n, err = read(stderr_r, buf[:]) - strings.write_bytes(&stderr_builder, buf[:n]) - } - switch err { - case nil: // nothing - case .Broken_Pipe: - hangup = true - case: - return - } - if hangup { - break read_data + + if !has_stderr { + has_data, err = pipe_has_data(stderr_r) + if has_data { + n, err = read(stderr_r, buf[:]) + if strings.write_bytes(&stderr_builder, buf[:n]) != n { + err = .Out_Of_Memory + } + } + switch err { + case nil: // nothing + case .EOF, .Broken_Pipe: + stderr = stderr_builder.buf[:] + has_stderr = true + case: + return + } } } + err = nil - stdout = transmute([]u8) strings.to_string(stdout_builder) - stderr = transmute([]u8) strings.to_string(stderr_builder) state = process_wait(process) or_return return } diff --git a/tests/core/normal.odin b/tests/core/normal.odin index 0670842ac..5bc73bd24 100644 --- a/tests/core/normal.odin +++ b/tests/core/normal.odin @@ -33,6 +33,7 @@ download_assets :: proc() { @(require) import "net" @(require) import "odin" @(require) import "os" +@(require) import "os/os2" @(require) import "path/filepath" @(require) import "reflect" @(require) import "runtime" diff --git a/tests/core/os/os2/process.odin b/tests/core/os/os2/process.odin new file mode 100644 index 000000000..510a35037 --- /dev/null +++ b/tests/core/os/os2/process.odin @@ -0,0 +1,28 @@ +package tests_core_os_os2 + +import "base:runtime" + +import "core:log" +import os "core:os/os2" +import "core:testing" + +_ :: log + +@(test) +test_process_exec :: proc(t: ^testing.T) { + state, stdout, stderr, err := os.process_exec({ + command = {"echo", "hellope"}, + }, context.allocator) + defer delete(stdout) + defer delete(stderr) + + when (ODIN_OS not_in runtime.Odin_OS_Types{.Linux, .Darwin, .Windows}) { + testing.expect_value(t, err, os.General_Error.Unsupported) + _ = state + } else { + testing.expect_value(t, state.exited, true) + testing.expect_value(t, state.success, true) + testing.expect_value(t, err, nil) + testing.expect_value(t, string(stdout), "hellope\n") + } +} From a78cd48aa3cb7ef29ee971b235859e1f5ee5481c Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Thu, 3 Oct 2024 13:41:10 +0200 Subject: [PATCH 02/21] remove posix signal test, it isn't thread safe --- tests/core/sys/posix/posix.odin | 48 --------------------------------- 1 file changed, 48 deletions(-) diff --git a/tests/core/sys/posix/posix.odin b/tests/core/sys/posix/posix.odin index 760ddc1fb..d73e49ffb 100644 --- a/tests/core/sys/posix/posix.odin +++ b/tests/core/sys/posix/posix.odin @@ -1,8 +1,6 @@ #+build darwin, freebsd, openbsd, netbsd package tests_core_posix -import "base:runtime" - import "core:log" import "core:path/filepath" import "core:strings" @@ -217,52 +215,6 @@ test_termios :: proc(t: ^testing.T) { testing.expect_value(t, transmute(posix.COutput_Flags)posix.tcflag_t(posix._FFDLY), posix.FFDLY) } -@(test) -test_signal :: proc(t: ^testing.T) { - @static tt: ^testing.T - tt = t - - @static ctx: runtime.Context - ctx = context - - act: posix.sigaction_t - act.sa_flags = {.SIGINFO, .RESETHAND} - act.sa_sigaction = handler - testing.expect_value(t, posix.sigaction(.SIGCHLD, &act, nil), posix.result.OK) - - handler :: proc "c" (sig: posix.Signal, info: ^posix.siginfo_t, address: rawptr) { - context = ctx - testing.expect_value(tt, sig, posix.Signal.SIGCHLD) - testing.expect_value(tt, info.si_signo, posix.Signal.SIGCHLD) - testing.expect_value(tt, info.si_status, 69) - testing.expect_value(tt, info.si_code.chld, posix.CLD_Code.EXITED) - } - - switch pid := posix.fork(); pid { - case -1: - log.errorf("fork() failure: %v", posix.strerror()) - case 0: - posix.exit(69) - case: - for { - status: i32 - res := posix.waitpid(pid, &status, {}) - if res == -1 { - if !testing.expect_value(t, posix.errno(), posix.Errno.EINTR) { - break - } - continue - } - - if posix.WIFEXITED(status) || posix.WIFSIGNALED(status) { - testing.expect(t, posix.WIFEXITED(status)) - testing.expect(t, posix.WEXITSTATUS(status) == 69) - break - } - } - } -} - @(test) test_pthreads :: proc(t: ^testing.T) { testing.set_fail_timeout(t, time.Second) From 77780f9ce8d1ba41a90d709d9315ddffec340013 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Thu, 3 Oct 2024 14:24:00 +0200 Subject: [PATCH 03/21] fix use-after-free - closing wrong pipe --- core/os/os2/process.odin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index 94d9732cb..c034a9fea 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -381,7 +381,7 @@ process_exec :: proc( stdout_r, stdout_w := pipe() or_return defer close(stdout_r) stderr_r, stderr_w := pipe() or_return - defer close(stdout_w) + defer close(stderr_r) process: Process { From 76764805260da5cf10936afcde886f50eb2e2d3f Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Thu, 3 Oct 2024 14:39:54 +0200 Subject: [PATCH 04/21] fix temp allocator guard bug --- core/os/os2/allocators.odin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/os/os2/allocators.odin b/core/os/os2/allocators.odin index a205cae48..864532850 100644 --- a/core/os/os2/allocators.odin +++ b/core/os/os2/allocators.odin @@ -62,8 +62,8 @@ TEMP_ALLOCATOR_GUARD_END :: proc(temp: runtime.Arena_Temp, loc := #caller_locati @(deferred_out=TEMP_ALLOCATOR_GUARD_END) TEMP_ALLOCATOR_GUARD :: #force_inline proc(loc := #caller_location) -> (runtime.Arena_Temp, runtime.Source_Code_Location) { - tmp := temp_allocator_temp_begin(loc) global_default_temp_allocator_index = (global_default_temp_allocator_index+1)%MAX_TEMP_ARENA_COUNT + tmp := temp_allocator_temp_begin(loc) return tmp, loc } From af8b592bf67db322332aa68447d684b120eb3801 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Thu, 3 Oct 2024 14:45:45 +0200 Subject: [PATCH 05/21] enable test on bsds --- tests/core/os/os2/process.odin | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/tests/core/os/os2/process.odin b/tests/core/os/os2/process.odin index 510a35037..944863925 100644 --- a/tests/core/os/os2/process.odin +++ b/tests/core/os/os2/process.odin @@ -1,13 +1,8 @@ package tests_core_os_os2 -import "base:runtime" - -import "core:log" import os "core:os/os2" import "core:testing" -_ :: log - @(test) test_process_exec :: proc(t: ^testing.T) { state, stdout, stderr, err := os.process_exec({ @@ -16,13 +11,8 @@ test_process_exec :: proc(t: ^testing.T) { defer delete(stdout) defer delete(stderr) - when (ODIN_OS not_in runtime.Odin_OS_Types{.Linux, .Darwin, .Windows}) { - testing.expect_value(t, err, os.General_Error.Unsupported) - _ = state - } else { - testing.expect_value(t, state.exited, true) - testing.expect_value(t, state.success, true) - testing.expect_value(t, err, nil) - testing.expect_value(t, string(stdout), "hellope\n") - } + testing.expect_value(t, state.exited, true) + testing.expect_value(t, state.success, true) + testing.expect_value(t, err, nil) + testing.expect_value(t, string(stdout), "hellope\n") } From 5d556fe277c924b910308383f63785f9c5376cc3 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Thu, 3 Oct 2024 15:25:51 +0200 Subject: [PATCH 06/21] fix idtype definition --- core/sys/posix/sys_wait.odin | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/core/sys/posix/sys_wait.odin b/core/sys/posix/sys_wait.odin index 8421c8bfd..e0e2ae21b 100644 --- a/core/sys/posix/sys_wait.odin +++ b/core/sys/posix/sys_wait.odin @@ -124,11 +124,11 @@ WIFCONTINUED :: #force_inline proc "contextless" (x: c.int) -> bool { idtype_t :: enum c.int { // Wait for any children and `id` is ignored. - P_ALL, + P_ALL = _P_ALL, // Wait for any child wiith a process group ID equal to `id`. - P_PID, + P_PID = _P_PID, // Wait for any child with a process group ID equal to `id`. - P_PGID, + P_PGID = _P_PGID, } Wait_Flag_Bits :: enum c.int { @@ -166,6 +166,10 @@ when ODIN_OS == .Darwin { WNOWAIT :: 0x00000020 WSTOPPED :: 0x00000008 + _P_ALL :: 0 + _P_PID :: 1 + _P_PGID :: 2 + @(private) _WSTATUS :: #force_inline proc "contextless" (x: c.int) -> c.int { return x & 0o177 @@ -221,6 +225,10 @@ when ODIN_OS == .Darwin { WNOWAIT :: 8 WSTOPPED :: 2 + _P_ALL :: 7 + _P_PID :: 0 + _P_PGID :: 2 + @(private) _WSTATUS :: #force_inline proc "contextless" (x: c.int) -> c.int { return x & 0o177 @@ -275,6 +283,10 @@ when ODIN_OS == .Darwin { WNOWAIT :: 0x00010000 WSTOPPED :: 0x00000002 + _P_ALL :: 0 + _P_PID :: 1 + _P_PGID :: 2 + @(private) _WSTATUS :: #force_inline proc "contextless" (x: c.int) -> c.int { return x & 0o177 @@ -330,6 +342,10 @@ when ODIN_OS == .Darwin { WNOWAIT :: 0x00010000 WSTOPPED :: 0x00000002 + _P_ALL :: 0 + _P_PID :: 2 + _P_PGID :: 1 + @(private) _WSTATUS :: #force_inline proc "contextless" (x: c.int) -> c.int { return x & 0o177 From 77b033cf96af6a2cafcee23e52ea6e220f1beae4 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Fri, 4 Oct 2024 10:38:47 +0200 Subject: [PATCH 07/21] kill process if there was an error during reading to not leave a zombie --- core/os/os2/process.odin | 79 +++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 38 deletions(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index c034a9fea..4e361f1d9 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -395,54 +395,57 @@ process_exec :: proc( process = process_start(desc) or_return } - stdout_builder := strings.builder_make(allocator) or_return - stderr_builder := strings.builder_make(allocator) or_return + { + defer if err != nil { _ = process_kill(process) } - has_stdout, has_stderr: bool - read_data: for !has_stdout || !has_stderr { - buf: [1024]u8 = --- - n: int - has_data: bool + stdout_builder := strings.builder_make(allocator) or_return + stderr_builder := strings.builder_make(allocator) or_return - if !has_stdout { - has_data, err = pipe_has_data(stdout_r) - if has_data { - n, err = read(stdout_r, buf[:]) - if strings.write_bytes(&stdout_builder, buf[:n]) != n { - err = .Out_Of_Memory + has_stdout, has_stderr: bool + read_data: for !has_stdout || !has_stderr { + buf: [1024]u8 = --- + n: int + has_data: bool + + if !has_stdout { + has_data, err = pipe_has_data(stdout_r) + if has_data { + n, err = read(stdout_r, buf[:]) + if strings.write_bytes(&stdout_builder, buf[:n]) != n { + err = .Out_Of_Memory + } + } + switch err { + case nil: // nothing + case .EOF, .Broken_Pipe: + stdout = stdout_builder.buf[:] + has_stdout = true + case: + return } } - switch err { - case nil: // nothing - case .EOF, .Broken_Pipe: - stdout = stdout_builder.buf[:] - has_stdout = true - case: - return - } - } - if !has_stderr { - has_data, err = pipe_has_data(stderr_r) - if has_data { - n, err = read(stderr_r, buf[:]) - if strings.write_bytes(&stderr_builder, buf[:n]) != n { - err = .Out_Of_Memory + if !has_stderr { + has_data, err = pipe_has_data(stderr_r) + if has_data { + n, err = read(stderr_r, buf[:]) + if strings.write_bytes(&stderr_builder, buf[:n]) != n { + err = .Out_Of_Memory + } + } + switch err { + case nil: // nothing + case .EOF, .Broken_Pipe: + stderr = stderr_builder.buf[:] + has_stderr = true + case: + return } - } - switch err { - case nil: // nothing - case .EOF, .Broken_Pipe: - stderr = stderr_builder.buf[:] - has_stderr = true - case: - return } } } - err = nil - state = process_wait(process) or_return + state, err = process_wait(process) return } From 0b5cd3400f062e470b02f1417b154117feef8b87 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Fri, 4 Oct 2024 10:43:38 +0200 Subject: [PATCH 08/21] use dynamic array instead of string builder --- core/os/os2/process.odin | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index 4e361f1d9..a071fd288 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -398,8 +398,11 @@ process_exec :: proc( { defer if err != nil { _ = process_kill(process) } - stdout_builder := strings.builder_make(allocator) or_return - stderr_builder := strings.builder_make(allocator) or_return + stdout_b: [dynamic]byte + stdout_b.allocator = allocator + + stderr_b: [dynamic]byte + stderr_b.allocator = allocator has_stdout, has_stderr: bool read_data: for !has_stdout || !has_stderr { @@ -411,14 +414,12 @@ process_exec :: proc( has_data, err = pipe_has_data(stdout_r) if has_data { n, err = read(stdout_r, buf[:]) - if strings.write_bytes(&stdout_builder, buf[:n]) != n { - err = .Out_Of_Memory - } + append(&stdout_b, ..buf[:n]) or_return } switch err { case nil: // nothing case .EOF, .Broken_Pipe: - stdout = stdout_builder.buf[:] + stdout = stdout_b[:] has_stdout = true case: return @@ -429,14 +430,12 @@ process_exec :: proc( has_data, err = pipe_has_data(stderr_r) if has_data { n, err = read(stderr_r, buf[:]) - if strings.write_bytes(&stderr_builder, buf[:n]) != n { - err = .Out_Of_Memory - } + append(&stderr_b, ..buf[:n]) or_return } switch err { case nil: // nothing case .EOF, .Broken_Pipe: - stderr = stderr_builder.buf[:] + stderr = stderr_b[:] has_stderr = true case: return From ae69f4b74955cde3024cee7c2741b67445476c28 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Fri, 4 Oct 2024 10:50:14 +0200 Subject: [PATCH 09/21] general cleanup --- core/os/os2/process.odin | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index a071fd288..c8f91fdb7 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -404,13 +404,13 @@ process_exec :: proc( stderr_b: [dynamic]byte stderr_b.allocator = allocator - has_stdout, has_stderr: bool - read_data: for !has_stdout || !has_stderr { - buf: [1024]u8 = --- - n: int - has_data: bool + buf: [1024]u8 = --- + n: int - if !has_stdout { + stdout_done, stderr_done, has_data: bool + for !stdout_done || !stderr_done { + + if !stdout_done { has_data, err = pipe_has_data(stdout_r) if has_data { n, err = read(stdout_r, buf[:]) @@ -419,14 +419,14 @@ process_exec :: proc( switch err { case nil: // nothing case .EOF, .Broken_Pipe: - stdout = stdout_b[:] - has_stdout = true + stdout = stdout_b[:] + stdout_done = true case: return } } - if !has_stderr { + if !stderr_done { has_data, err = pipe_has_data(stderr_r) if has_data { n, err = read(stderr_r, buf[:]) @@ -435,8 +435,8 @@ process_exec :: proc( switch err { case nil: // nothing case .EOF, .Broken_Pipe: - stderr = stderr_b[:] - has_stderr = true + stderr = stderr_b[:] + stderr_done = true case: return } From 1d29dfd03755fccc35f9468297019ce50f3c0251 Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Fri, 4 Oct 2024 10:51:40 +0200 Subject: [PATCH 10/21] kill process if waiting didn't make it exit to avoid a zombie --- core/os/os2/process.odin | 1 + 1 file changed, 1 insertion(+) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index c8f91fdb7..bba449de5 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -445,6 +445,7 @@ process_exec :: proc( } state, err = process_wait(process) + if !state.exited { _ = process_kill(process) } return } From 386f144ccab3d0081f5ec30f8c88b771def7a87e Mon Sep 17 00:00:00 2001 From: Laytan Laats Date: Fri, 4 Oct 2024 10:54:27 +0200 Subject: [PATCH 11/21] satisfy -vet --- core/os/os2/process.odin | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index bba449de5..d628acd36 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -1,7 +1,7 @@ package os2 import "base:runtime" -import "core:strings" + import "core:time" /* From 563ed69c28fd60ca3c7bcb61f29bbe6cf4ab09bd Mon Sep 17 00:00:00 2001 From: Laytan Date: Fri, 4 Oct 2024 13:08:56 +0200 Subject: [PATCH 12/21] fix deadlock when in write_errno_to_parent_and_abort state --- core/os/os2/process_linux.odin | 2 +- core/os/os2/process_posix.odin | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/os/os2/process_linux.odin b/core/os/os2/process_linux.odin index ea5ee41b1..7eb4dfa44 100644 --- a/core/os/os2/process_linux.odin +++ b/core/os/os2/process_linux.odin @@ -523,7 +523,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { write_errno_to_parent_and_abort :: proc(parent_fd: linux.Fd, errno: linux.Errno) -> ! { error_byte: [1]u8 = { u8(errno) } linux.write(parent_fd, error_byte[:]) - intrinsics.trap() + linux.exit(126) } stdin_fd: linux.Fd diff --git a/core/os/os2/process_posix.odin b/core/os/os2/process_posix.odin index 5ac6babc1..776decb0d 100644 --- a/core/os/os2/process_posix.odin +++ b/core/os/os2/process_posix.odin @@ -163,7 +163,7 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { #assert(len(posix.Errno) < max(u8)) errno := u8(posix.errno()) posix.write(parent_fd, &errno, 1) - runtime.trap() + posix.exit(126) } null := posix.open("/dev/null", {.RDWR}) From 64508e477be6c3151c8748a42dc68921a6bd0921 Mon Sep 17 00:00:00 2001 From: Laytan Date: Fri, 4 Oct 2024 13:11:42 +0200 Subject: [PATCH 13/21] add unsupported check in process test --- core/os/os2/errors_linux.odin | 2 ++ tests/core/os/os2/process.odin | 6 ++++++ 2 files changed, 8 insertions(+) diff --git a/core/os/os2/errors_linux.odin b/core/os/os2/errors_linux.odin index 09492110d..a7556c306 100644 --- a/core/os/os2/errors_linux.odin +++ b/core/os/os2/errors_linux.odin @@ -162,6 +162,8 @@ _get_platform_error :: proc(errno: linux.Errno) -> Error { return .Invalid_File case .ENOMEM: return .Out_Of_Memory + case .ENOSYS: + return .Unsupported } return Platform_Error(i32(errno)) diff --git a/tests/core/os/os2/process.odin b/tests/core/os/os2/process.odin index 944863925..8e5fd8eb8 100644 --- a/tests/core/os/os2/process.odin +++ b/tests/core/os/os2/process.odin @@ -1,6 +1,7 @@ package tests_core_os_os2 import os "core:os/os2" +import "core:log" import "core:testing" @(test) @@ -11,6 +12,11 @@ test_process_exec :: proc(t: ^testing.T) { defer delete(stdout) defer delete(stderr) + if err == .Unsupported { + log.warn("process_exec unsupported") + return + } + testing.expect_value(t, state.exited, true) testing.expect_value(t, state.success, true) testing.expect_value(t, err, nil) From d9cfe692a974457681ddb14b5e5a957d28396e9f Mon Sep 17 00:00:00 2001 From: Laytan Date: Fri, 4 Oct 2024 13:24:40 +0200 Subject: [PATCH 14/21] make sure stdout and stderr always point to allocation --- core/os/os2/process.odin | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index d628acd36..65412171f 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -400,9 +400,11 @@ process_exec :: proc( stdout_b: [dynamic]byte stdout_b.allocator = allocator + defer stdout = stdout_b[:] stderr_b: [dynamic]byte stderr_b.allocator = allocator + defer stderr = stderr_b[:] buf: [1024]u8 = --- n: int @@ -419,7 +421,6 @@ process_exec :: proc( switch err { case nil: // nothing case .EOF, .Broken_Pipe: - stdout = stdout_b[:] stdout_done = true case: return @@ -435,7 +436,6 @@ process_exec :: proc( switch err { case nil: // nothing case .EOF, .Broken_Pipe: - stderr = stderr_b[:] stderr_done = true case: return From 31ee829b44be7b1e789864b0fa616a78db98b672 Mon Sep 17 00:00:00 2001 From: Laytan Date: Fri, 4 Oct 2024 13:40:25 +0200 Subject: [PATCH 15/21] add some debug logs --- core/os/os2/process.odin | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index 65412171f..6269ce94d 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -348,6 +348,8 @@ process_start :: proc(desc: Process_Desc) -> (Process, Error) { return _process_start(desc) } +import "core:log" + /* Execute the process and capture stdout and stderr streams. @@ -383,6 +385,8 @@ process_exec :: proc( stderr_r, stderr_w := pipe() or_return defer close(stderr_r) + log.info("starting process") + process: Process { // NOTE(flysand): Make sure the write-ends are closed, regardless @@ -395,6 +399,8 @@ process_exec :: proc( process = process_start(desc) or_return } + log.info("reading output") + { defer if err != nil { _ = process_kill(process) } @@ -423,6 +429,7 @@ process_exec :: proc( case .EOF, .Broken_Pipe: stdout_done = true case: + log.infof("read stdout error: %v", err) return } } @@ -438,13 +445,16 @@ process_exec :: proc( case .EOF, .Broken_Pipe: stderr_done = true case: + log.infof("read stderr error: %v", err) return } } } } + log.info("waiting for exit") state, err = process_wait(process) + log.info(state, err) if !state.exited { _ = process_kill(process) } return } From 424dc590a33b6d8f893c61f35cd8ac1396e98d3d Mon Sep 17 00:00:00 2001 From: Laytan Date: Fri, 4 Oct 2024 13:52:44 +0200 Subject: [PATCH 16/21] fix bsds process_open --- core/os/os2/process_posix.odin | 1 - core/os/os2/process_posix_other.odin | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/core/os/os2/process_posix.odin b/core/os/os2/process_posix.odin index 776decb0d..b54374cec 100644 --- a/core/os/os2/process_posix.odin +++ b/core/os/os2/process_posix.odin @@ -223,7 +223,6 @@ _process_start :: proc(desc: Process_Desc) -> (process: Process, err: Error) { return } - process.pid = int(pid) process, _ = _process_open(int(pid), {}) return } diff --git a/core/os/os2/process_posix_other.odin b/core/os/os2/process_posix_other.odin index 77dbfa7eb..65da3e9e2 100644 --- a/core/os/os2/process_posix_other.odin +++ b/core/os/os2/process_posix_other.odin @@ -15,6 +15,7 @@ _process_list :: proc(allocator: runtime.Allocator) -> (list: []int, err: Error) } _process_open :: proc(pid: int, flags: Process_Open_Flags) -> (process: Process, err: Error) { + process.pid = pid err = .Unsupported return } From 59086a24a142be7bfd07b4c3327cac749bf366e2 Mon Sep 17 00:00:00 2001 From: Laytan Date: Fri, 4 Oct 2024 13:52:54 +0200 Subject: [PATCH 17/21] add .ENOSYS == .Unsupported for posix too --- core/os/os2/errors_posix.odin | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/os/os2/errors_posix.odin b/core/os/os2/errors_posix.odin index 59f0ba5f1..0b5876c0b 100644 --- a/core/os/os2/errors_posix.odin +++ b/core/os/os2/errors_posix.odin @@ -26,6 +26,8 @@ _get_platform_error :: proc() -> Error { return .Invalid_File case .ENOMEM: return .Out_Of_Memory + case .ENOSYS: + return .Unsupported case: return Platform_Error(errno) } From 861efa4e54b56feeca705404613a6c57cc3fc714 Mon Sep 17 00:00:00 2001 From: Laytan Date: Fri, 4 Oct 2024 13:58:59 +0200 Subject: [PATCH 18/21] Revert "add some debug logs" This reverts commit 31ee829b44be7b1e789864b0fa616a78db98b672. --- core/os/os2/process.odin | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index 6269ce94d..65412171f 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -348,8 +348,6 @@ process_start :: proc(desc: Process_Desc) -> (Process, Error) { return _process_start(desc) } -import "core:log" - /* Execute the process and capture stdout and stderr streams. @@ -385,8 +383,6 @@ process_exec :: proc( stderr_r, stderr_w := pipe() or_return defer close(stderr_r) - log.info("starting process") - process: Process { // NOTE(flysand): Make sure the write-ends are closed, regardless @@ -399,8 +395,6 @@ process_exec :: proc( process = process_start(desc) or_return } - log.info("reading output") - { defer if err != nil { _ = process_kill(process) } @@ -429,7 +423,6 @@ process_exec :: proc( case .EOF, .Broken_Pipe: stdout_done = true case: - log.infof("read stdout error: %v", err) return } } @@ -445,16 +438,13 @@ process_exec :: proc( case .EOF, .Broken_Pipe: stderr_done = true case: - log.infof("read stderr error: %v", err) return } } } } - log.info("waiting for exit") state, err = process_wait(process) - log.info(state, err) if !state.exited { _ = process_kill(process) } return } From cf705d4b295b3eb67f6c1253af494ae5c857cded Mon Sep 17 00:00:00 2001 From: Laytan Date: Fri, 4 Oct 2024 14:50:26 +0200 Subject: [PATCH 19/21] wait instead of kill --- core/os/os2/process.odin | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index 65412171f..8ed2f0dcf 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -396,7 +396,7 @@ process_exec :: proc( } { - defer if err != nil { _ = process_kill(process) } + defer if err != nil { _, _ = process_wait(process) } stdout_b: [dynamic]byte stdout_b.allocator = allocator @@ -445,7 +445,6 @@ process_exec :: proc( } state, err = process_wait(process) - if !state.exited { _ = process_kill(process) } return } From a3c3e5c822ef1ac87a16578661bcc65d187b7a10 Mon Sep 17 00:00:00 2001 From: Laytan Date: Fri, 4 Oct 2024 14:53:16 +0200 Subject: [PATCH 20/21] reset err --- core/os/os2/process.odin | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index 8ed2f0dcf..71c07e3bf 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -422,6 +422,7 @@ process_exec :: proc( case nil: // nothing case .EOF, .Broken_Pipe: stdout_done = true + err = nil case: return } @@ -437,6 +438,7 @@ process_exec :: proc( case nil: // nothing case .EOF, .Broken_Pipe: stderr_done = true + err = nil case: return } From 54ffd6df0663fe561c72eee841c853417cca676f Mon Sep 17 00:00:00 2001 From: Laytan Date: Fri, 4 Oct 2024 15:00:17 +0200 Subject: [PATCH 21/21] better error handling --- core/os/os2/process.odin | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/core/os/os2/process.odin b/core/os/os2/process.odin index 71c07e3bf..5ff8a8c02 100644 --- a/core/os/os2/process.odin +++ b/core/os/os2/process.odin @@ -396,8 +396,6 @@ process_exec :: proc( } { - defer if err != nil { _, _ = process_wait(process) } - stdout_b: [dynamic]byte stdout_b.allocator = allocator defer stdout = stdout_b[:] @@ -410,42 +408,49 @@ process_exec :: proc( n: int stdout_done, stderr_done, has_data: bool - for !stdout_done || !stderr_done { + for err == nil && (!stdout_done || !stderr_done) { if !stdout_done { has_data, err = pipe_has_data(stdout_r) if has_data { n, err = read(stdout_r, buf[:]) - append(&stdout_b, ..buf[:n]) or_return } + switch err { - case nil: // nothing + case nil: + _, err = append(&stdout_b, ..buf[:n]) case .EOF, .Broken_Pipe: stdout_done = true err = nil - case: - return } } - if !stderr_done { + if err == nil && !stderr_done { has_data, err = pipe_has_data(stderr_r) if has_data { n, err = read(stderr_r, buf[:]) - append(&stderr_b, ..buf[:n]) or_return } + switch err { - case nil: // nothing + case nil: + _, err = append(&stderr_b, ..buf[:n]) case .EOF, .Broken_Pipe: stderr_done = true err = nil - case: - return } } } } + if err != nil { + state, _ = process_wait(process, timeout=0) + if !state.exited { + _ = process_kill(process) + state, _ = process_wait(process) + } + return + } + state, err = process_wait(process) return }