[Libguestfs] [libnbd PATCH v4 1/3] lib/utils: introduce xwritel() as a more robust and convenient write()

While nbd_internal_fork_safe_perror() must indeed call write(), and arguably justifiedly ignores the return value of write(), we can still make the write operations slightly more robust and convenient. Let's do that by introducing xwritel(): - Let the caller pass a list of NUL-terminated strings, via stdarg / ellipsis notation in the parameter list. - Handle partial writes. - Cope with EINTR and EAGAIN errors. (A busy loop around EAGAIN on a non-blocking file is not great in the general case, but it's good enough for writing out diagnostics before giving up.) - In the common case, handle an nbd_internal_fork_safe_perror() call with a single xwritel() -> writev() call chain, rather than with four separate write() calls. In practice, this tends to make the error message written to a regular file contiguous, even if other threads are writing to the same file. Multiple separate write() calls tend to interleave chunks of data from different threads. As a side bonus, remove the path in nbd_internal_fork_safe_perror() where at least one of the first two write() syscalls fails, and overwrites "errno", before we get to formatting the error string from "errno". Thanks to Eric Blake for helping me understand the scope of Austin Group bug reports. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: v4: - Rework with <stdarg.h> and writev(). - Don't split the output into chunks of SSIZE_MAX bytes. In v3, the goal of that chunking was to avoid implementation-defined behavior. However, POSIX requires writev() to fail cleanly when more than SSIZE_MAX bytes would be transfered in a single call. Hence the original goal (avoiding implementation-defined behavior) is ensured simply by switching to writev(). The SSIZE_MAX limit is not expected to be hit in practice (_POSIX_SSIZE_MAX is 32767). - As a "bonus", remove the pre-patch possibility to trample "errno" before formatting the error string. - Refresh the commit message. - The "contiguous output" from a single xwritel() -> writev() call (as opposed to the "interleaved output" from multiple xwrite() -> write() calls in v3) is easily testable in practice (on my end) with the following small patch, even though this "contiguity" is of course not guaranteed: It results in the following snippet in "tests/connect-systemd-socket-activation.log": Note that the child process's output is on an isolated line. - Do not pick up R-b tags from Eric and Rich due to significant changes in v4. context:-U5 lib/utils.c | 87 +++++++++++++++++++- 1 file changed, 83 insertions(+), 4 deletions(-) diff --git a/lib/utils.c b/lib/utils.c index 6df4f14ce9f4..62b4bfdda5c3 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -23,11 +23,14 @@ #include <string.h> #include <unistd.h> #include <ctype.h> #include <errno.h> #include <fcntl.h> +#include <stdarg.h> +#include <sys/uio.h> +#include "array-size.h" #include "minmax.h" #include "internal.h" void @@ -179,33 +182,109 @@ nbd_internal_fork_safe_itoa (long v, char *buf, size_t bufsize) #if defined (__GNUC__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-result" #endif +/* "Best effort" function for writing out a list of NUL-terminated strings to a + * file descriptor (without the NUL-terminators). The list is terminated with + * (char *)NULL. Partial writes, and EINTR and EAGAIN failures are handled + * internally. No value is returned; only call this function for writing + * diagnostic data on error paths, when giving up on a higher-level action + * anyway. + * + * No more than 16 strings, excluding the NULL terminator, will be written. (As + * of POSIX Issue 7 + TC2, _XOPEN_IOV_MAX is 16.) + * + * The function is supposed to remain async-signal-safe. + * + * (The va_*() macros, while not marked async-signal-safe in Issue 7 + TC2, are + * considered such, per <https://www.austingroupbugs.net/view.php?id=711>;, which + * is binding for Issue 7 implementations via the Interpretations Track. + * + * Furthermore, writev(), while also not marked async-signal-safe in Issue 7 + + * TC2, is considered such, per + * <https://www.austingroupbugs.net/view.php?id=1455>;, which is slated for + * inclusion in Issue 7 TC3 (if there's going to be a TC3), and in Issue 8.) + */ +static void +xwritel (int fildes, ...) +{ + /* Open-code the current value of _XOPEN_IOV_MAX, in order to contain stack + * footprint, should _XOPEN_IOV_MAX grow in the future. + */ + struct iovec iovec[16], *filled, *end, *pos; + va_list ap; + char *arg; + + /* Translate the variable argument list to IO vectors. Note that we cast away + * const-ness intentionally. + */ + filled = iovec; + end = iovec + ARRAY_SIZE (iovec); + va_start (ap, fildes); + while (filled < end && (arg = va_arg (ap, char *)) != NULL) + *filled++ = (struct iovec){ .iov_base = arg, .iov_len = strlen (arg) }; + va_end (ap); + + /* Write out the IO vectors. */ + pos = iovec; + while (pos < filled) { + ssize_t written; + + /* Skip any empty vectors at the front. */ + if (pos->iov_len == 0) { + ++pos; + continue; + } + + /* Write out the vectors. */ + do + written = writev (fildes, pos, filled - pos); + while (written == -1 && (errno == EINTR || errno == EAGAIN)); + + if (written == -1) + return; + + /* Consume the vectors that have been written out (fully, or in part). Note + * that "written" is positive here. + */ + do { + size_t advance; + + advance = MIN (written, pos->iov_len); + /* Note that "advance" is positive here iff "pos->iov_len" is positive. */ + pos->iov_base = (char *)pos->iov_base + advance; + pos->iov_len -= advance; + written -= advance; + + /* At least one of "written" and "pos->iov_len" is zero here. */ + if (pos->iov_len == 0) + ++pos; + } while (written > 0); + } +} + /* Fork-safe version of perror. ONLY use this after fork and before * exec, the rest of the time use set_error(). */ void nbd_internal_fork_safe_perror (const char *s) { const int err = errno; const char *m = NULL; char buf[32]; - write (2, s, strlen (s)); - write (2, ": ", 2); #ifdef HAVE_STRERRORDESC_NP m = strerrordesc_np (errno); #else #if HAVE_SYS_ERRLIST /* NB Don't use #ifdef */ m = errno >= 0 && errno < sys_nerr ? sys_errlist[errno] : NULL; #endif #endif if (!m) m = nbd_internal_fork_safe_itoa ((long) errno, buf, sizeof buf); - write (2, m, strlen (m)); - write (2, "\n", 1); + xwritel (STDERR_FILENO, s, ": ", m, "\n", (char *)NULL); /* Restore original errno in case it was disturbed by the system * calls above. */ errno = err;