From 57c0854b4f071a1a57b4e84d6d951f959bde286b Mon Sep 17 00:00:00 2001 From: Michael Tremer Date: Tue, 18 Mar 2025 18:02:26 +0000 Subject: [PATCH] pty: Refactor how we capture output This will now avoid copying all the data around between the kernel and userland and will also simply use our own callbacks which helps us to keep the code a little bit less of an if/else hell. Signed-off-by: Michael Tremer --- src/pakfire/jail.c | 17 +++---- src/pakfire/pty.c | 98 +++++++++++------------------------------ src/pakfire/pty.h | 2 +- tests/libpakfire/jail.c | 3 +- 4 files changed, 38 insertions(+), 82 deletions(-) diff --git a/src/pakfire/jail.c b/src/pakfire/jail.c index 55898878..d1d992dd 100644 --- a/src/pakfire/jail.c +++ b/src/pakfire/jail.c @@ -1409,10 +1409,6 @@ static int __pakfire_jail_exec(struct pakfire_jail* jail, } else if (stdout_callback) { // Nothing - // Capture Output? - } else if (output) { - pty_flags |= PAKFIRE_PTY_CAPTURE_OUTPUT; - // Otherwise we dump everything to the console } else { // XXX Need to find a solution about what to do here... @@ -1433,6 +1429,15 @@ static int __pakfire_jail_exec(struct pakfire_jail* jail, if (r) goto ERROR; + // Capture Output? + if (output) { + r = pakfire_pty_capture_output(ctx.pty, output, output_length); + if (r < 0) { + ERROR(jail->ctx, "Failed to set up output capture: %s\n", strerror(-r)); + goto ERROR; + } + } + // Configure the callbacks if (stdin_callback) pakfire_pty_set_stdin_callback(ctx.pty, stdin_callback, stdin_data); @@ -1541,10 +1546,6 @@ static int __pakfire_jail_exec(struct pakfire_jail* jail, goto ERROR; } - // Return the output - if (output) - *output = pakfire_pty_output(ctx.pty, output_length); - // Return the exit code r = ctx.exit; diff --git a/src/pakfire/pty.c b/src/pakfire/pty.c index ed5c5634..edbb100d 100644 --- a/src/pakfire/pty.c +++ b/src/pakfire/pty.c @@ -109,48 +109,14 @@ struct pakfire_pty { PAKFIRE_PTY_STATE_DONE, } state; - // Captured Output - struct iovec output; + // Stream for output + FILE* output; }; static int pakfire_pty_has_flag(struct pakfire_pty* pty, int flag) { return pty->flags & flag; } -static int pakfire_pty_store_output(struct pakfire_pty* pty) { - struct stat buffer; - int r; - - // Check if we have a file descriptor - if (pty->stdout.fd < 0) - return -EBADF; - - // Stat the buffer - r = fstat(pty->stdout.fd, &buffer); - if (r < 0) { - ERROR(pty->ctx, "Could not stat the output buffer: %m\n"); - return -errno; - } - - DEBUG(pty->ctx, "Read %jd byte(s) of output\n", buffer.st_size); - - // Don't try to map an empty buffer - if (!buffer.st_size) - return 0; - - // Map the output into memory - pty->output.iov_base = mmap(NULL, buffer.st_size, PROT_READ, MAP_SHARED, pty->stdout.fd, 0); - if (pty->output.iov_base == MAP_FAILED) { - ERROR(pty->ctx, "Could not map the output into memory: %m\n"); - return -errno; - } - - // Store the size of the buffer - pty->output.iov_len = buffer.st_size; - - return 0; -} - static int pakfire_pty_same_inode(struct pakfire_pty* pty, int fd1, int fd2) { struct stat stat1; struct stat stat2; @@ -318,19 +284,14 @@ static int pakfire_pty_drained(struct pakfire_pty* pty) { } static int pakfire_pty_done(struct pakfire_pty* pty, int code) { - int r; - // Don't run this more than once if (pty->state == PAKFIRE_PTY_STATE_DONE) return 0; - // Store the output - if (pakfire_pty_has_flag(pty, PAKFIRE_PTY_CAPTURE_OUTPUT)) { - r = pakfire_pty_store_output(pty); - if (r < 0) { - ERROR(pty->ctx, "Could not store output: %s\n", strerror(-r)); - return r; - } + // Close the output + if (pty->output) { + fclose(pty->output); + pty->output = NULL; } // Disconnect @@ -1305,14 +1266,12 @@ static void pakfire_pty_free(struct pakfire_pty* pty) { if (pty->socket[1] >= 0) close(pty->socket[1]); - // Output - if (pty->output.iov_base) - munmap(pty->output.iov_base, pty->output.iov_len); - if (pty->loop) sd_event_unref(pty->loop); if (pty->ctx) pakfire_ctx_unref(pty->ctx); + if (pty->output) + fclose(pty->output); free(pty); } @@ -1557,34 +1516,29 @@ int pakfire_pty_open(struct pakfire_pty* pty) { return 0; } -/* - Creates an independent copy of the output buffer -*/ -char* pakfire_pty_output(struct pakfire_pty* pty, size_t* length) { - char* buffer = NULL; - - // Is not operation supported? - if (!pakfire_pty_has_flag(pty, PAKFIRE_PTY_CAPTURE_OUTPUT)) { - errno = -ENOTSUP; - return NULL; - } +static int __pakfire_pty_capture_output(struct pakfire_ctx* ctx, + void* data, const char* line, const size_t length) { + struct pakfire_pty* self = data; - if (!pty->output.iov_base) - return NULL; + // Write to the buffer + return fwrite(line, 1, length, self->output); +} - // Allocate a new buffer - buffer = calloc(pty->output.iov_len + 1, sizeof(*buffer)); - if (!buffer) - return NULL; +int pakfire_pty_capture_output(struct pakfire_pty* self, char** output, size_t* length) { + if (!output || !length) + return -EINVAL; - // Copy the output - memcpy(buffer, pty->output.iov_base, pty->output.iov_len); + // Open the memory stream + self->output = open_memstream(output, length); + if (!self->output) { + ERROR(self->ctx, "Failed to open memory stream: %m\n"); + return -errno; + } - // Return the length - if (length) - *length = pty->output.iov_len; + // Set the callback + pakfire_pty_set_stdout_callback(self, __pakfire_pty_capture_output, self); - return buffer; + return 0; } /* diff --git a/src/pakfire/pty.h b/src/pakfire/pty.h index 3627e1f1..c4af39b4 100644 --- a/src/pakfire/pty.h +++ b/src/pakfire/pty.h @@ -43,7 +43,7 @@ struct pakfire_pty* pakfire_pty_unref(struct pakfire_pty* pty); int pakfire_pty_open(struct pakfire_pty* pty); -char* pakfire_pty_output(struct pakfire_pty* pty, size_t* length); +int pakfire_pty_capture_output(struct pakfire_pty* pty, char** output, size_t* length); typedef ssize_t (*pakfire_pty_stdin_callback)( struct pakfire_ctx* ctx, void* data, char* buffer, size_t length); diff --git a/tests/libpakfire/jail.c b/tests/libpakfire/jail.c index 1d22abb9..8f541032 100644 --- a/tests/libpakfire/jail.c +++ b/tests/libpakfire/jail.c @@ -166,6 +166,7 @@ FAIL: static int test_nice(const struct test* t) { struct pakfire_jail* jail = NULL; char* output = NULL; + size_t length = 0; int r = EXIT_FAILURE; const char* argv[] = { @@ -183,7 +184,7 @@ static int test_nice(const struct test* t) { ASSERT_SUCCESS(pakfire_jail_nice(jail, 5)); // Check if the nice level has been set - ASSERT_SUCCESS(pakfire_jail_exec_capture_output(jail, argv, NULL, 0, &output, NULL)); + ASSERT_SUCCESS(pakfire_jail_exec_capture_output(jail, argv, NULL, 0, &output, &length)); ASSERT_STRING_EQUALS(output, "5\n"); // Success -- 2.39.5