Eric Blake
2023-Aug-30 22:21 UTC
[Libguestfs] [nbdkit PATCH] sh: Allow pwrite to not consume all data
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.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
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
Richard W.M. Jones
2023-Aug-31 08:02 UTC
[Libguestfs] [nbdkit PATCH] sh: Allow pwrite to not consume all data
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. 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. 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 at redhat.com> > --- > > 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 at redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
Laszlo Ersek
2023-Aug-31 08:40 UTC
[Libguestfs] [nbdkit PATCH] sh: Allow pwrite to not consume all data
On 8/31/23 00:21, 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. > > Signed-off-by: Eric Blake <eblake at redhat.com> > --- > > 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); > }The patch appears to do what it says on the tin, but I'm unconvinced we should relax the requirement. The "may indicate a bug" debug message is easy to miss. A hard failure upon EPIPE is noticeable, and as you write the script can just drain unwanted input with "cat >/dev/null". I think I may not be appreciating this patch due to not understanding how difficult it could be to update such a script. Can you elaborate on that? Can you provide an example where adding "cat >/dev/null" to the pwrite script is a disproportionate chore? Thanks, Laszlo
Maybe Matching Threads
- [nbdkit PATCH] sh: Allow pwrite to not consume all data
- [nbdkit PATCH] sh: Allow pwrite to not consume all data
- [nbdkit PATCH] sh: Allow pwrite to not consume all data
- [nbdkit PATCH] sh: Allow pwrite to not consume all data
- [nbdkit PATCH] sh: Allow pwrite to not consume all data