Re: [Libguestfs] [nbdkit PATCH] sh: Allow pwrite to not consume all data

On 8/31/23 10:02, Richard W.M. Jones wrote: > > On Wed, Aug 30, 2023 at 05:21:19PM -0500, Eric Blake wrote: >> I hit another transient failure in libnbd CI when a poorly-written >> eval script did not consume all of stdin during .pwrite. As behaving >> as a data sink can be a somewhat reasonable feature of a >> quickly-written sh or eval plugin, we should not be so insistent as >> treating an EPIPE failure as an immediate return of EIO to the client. > > I was thinking about this over night, and came to the conclusion that > it's always fine to ignore EPIPE errors. Interesting; I formed the opposite impression! > For example a script might > be processing the input data gradually and then encounter an error and > want to exit immediately. We also have plenty of plugins that discard > some or all of the written data. But that would be associated with a nonzero exit status, right?

And that way the nbd client would see the pwrite operation as failed. Laszlo > > So my counter-proposal (coming soon) is going to simply turn the EPIPE > error into a debug message and discard the rest of the write buffer. > > Rich. > > >> Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; >> --- >> >> I probably need to add unit test coverage of this before committing >> (although proving that I win the data race on a client process exiting >> faster than the parent can write enough data to hit EPIPE is hard). >> >> plugins/sh/nbdkit-sh-plugin.pod | 8 +++++++ >> plugins/sh/call.c | 38 ++++++++++++++++++++++----------- >> 2 files changed, 34 insertions(+), 12 deletions(-) >> >> diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod >> index b2c946a0..8b83a5b3 100644 >> --- a/plugins/sh/nbdkit-sh-plugin.pod >> +++ b/plugins/sh/nbdkit-sh-plugin.pod >> @@ -505,6 +505,14 @@ Unlike in other languages, if you provide a C<pwrite> method you >> B<must> also provide a C<can_write> method which exits with code C<0> >> (true). >> >> +With nbdkit E<ge> 1.36, this method may return C<0> without consuming >> +any data from stdin, and without producing any output, in order to >> +behave as an intentional data sink. But in older versions, nbdkit >> +would treat any C<EPIPE> failure in writing to your script as an error >> +condition even if your script returns success; to avoid unintended >> +failures, you may want to include C<"cat >/dev/null"> in a script >> +intending to ignore the client's write requests. >> + >> =item C<flush> >> >> /path/to/script flush <handle> >> diff --git a/plugins/sh/call.c b/plugins/sh/call.c >> index 888c6459..79c67a04 100644 >> --- a/plugins/sh/call.c >> +++ b/plugins/sh/call.c >> @@ -34,6 +34,7 @@ >> >> #include <assert.h> >> #include <fcntl.h> >> +#include <stdbool.h> >> #include <stdio.h> >> #include <stdlib.h> >> #include <inttypes.h> >> @@ -130,6 +131,7 @@ debug_call (const char **argv) >> */ >> static int >> call3 (const char *wbuf, size_t wbuflen, /* sent to stdin (can be NULL) */ >> + bool *pipe_full, /* set if wbuf not fully written */ >> string *rbuf, /* read from stdout */ >> string *ebuf, /* read from stderr */ >> const char **argv) /* script + parameters */ >> @@ -275,15 +277,8 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin (can be NULL) */ >> r = write (pfds[0].fd, wbuf, wbuflen); >> if (r == -1) { >> if (errno == EPIPE) { >> - /* We tried to write to the script but it didn't consume >> - * the data. Probably the script exited without reading >> - * from stdin. This is an error in the script. >> - */ >> - nbdkit_error ("%s: write to script failed because of a broken pipe: " >> - "this can happen if the script exits without " >> - "consuming stdin, which usually indicates a bug " >> - "in the script", >> - argv0); >> + *pipe_full = true; >> + r = wbuflen; >> } >> else >> nbdkit_error ("%s: write: %m", argv0); >> @@ -555,7 +550,7 @@ call (const char **argv) >> CLEANUP_FREE_STRING string rbuf = empty_vector; >> CLEANUP_FREE_STRING string ebuf = empty_vector; >> >> - r = call3 (NULL, 0, &rbuf, &ebuf, argv); >> + r = call3 (NULL, 0, NULL, &rbuf, &ebuf, argv); >> return handle_script_error (argv[0], &ebuf, r); >> } >> >> @@ -568,7 +563,7 @@ call_read (string *rbuf, const char **argv) >> int r; >> CLEANUP_FREE_STRING string ebuf = empty_vector; >> >> - r = call3 (NULL, 0, rbuf, &ebuf, argv); >> + r = call3 (NULL, 0, NULL, rbuf, &ebuf, argv); >> r = handle_script_error (argv[0], &ebuf, r); >> if (r == ERROR) >> string_reset (rbuf); >> @@ -584,7 +579,26 @@ call_write (const char *wbuf, size_t wbuflen, const char **argv) >> int r; >> CLEANUP_FREE_STRING string rbuf = empty_vector; >> CLEANUP_FREE_STRING string ebuf = empty_vector; >> + bool pipe_full = false; >> >> - r = call3 (wbuf, wbuflen, &rbuf, &ebuf, argv); >> + r = call3 (wbuf, wbuflen, &pipe_full, &rbuf, &ebuf, argv); >> + if (pipe_full && r == OK) { >> + /* We allow scripts to intentionally ignore data, but they must >> + * have no output when doing so. >> + */ >> + if (rbuf.len > 0 || ebuf.len > 0) { >> + nbdkit_error ("%s: write to script failed because of a broken pipe: " >> + "this can happen if the script exits without " >> + "consuming stdin, which usually indicates a bug " >> + "in the script", >> + argv[0]); >> + r = ERROR; >> + } >> + else >> + nbdkit_debug ("%s: write to script failed because of a broken pipe; " >> + "assuming this was an intentional data sink, although it " >> + "may indicate a bug in the script", >> + argv[0]); >> + } >> return handle_script_error (argv[0], &ebuf, r); >> } >> -- >> 2.41.0 >> >> _______________________________________________ >> Libguestfs mailing list >> Libguestfs(a)redhat.com >> https://listman.redhat.com/mailman/listinfo/libguestfs >