[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@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
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@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@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
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@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@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
On Thu, Aug 31, 2023 at 11:12:59AM +0200, Laszlo Ersek wrote:
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?
In the error case, yes it would exit with a non-zero exit status. In the "don't care about data" case it would exit with 0. As a concrete example the code currently has to look like this: case "$1") can_write) echo 0 ;; pwrite) ... if [ there is an error ]; then cat >/dev/null # discard stdin echo 'EIO I/O error' >&2 exit 1 else cat >/dev/null # discard stdin exit 0 fi where we're saying that the 'cat >/dev/null' commands are unnecessary complication. There have been cases where we have forgotten to discard stdin on every exit path and this has caused intermittent test failures in CI: https://gitlab.com/nbdkit/libnbd/-/commit/4df70870420d1be9348ac45f4aa467501e... https://gitlab.com/nbdkit/libnbd/-/commit/c713529e9fd0641b2d73f764517b5f9c21... Rich.
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@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@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-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
On 8/31/23 11:47, Richard W.M. Jones wrote:
On Thu, Aug 31, 2023 at 11:12:59AM +0200, Laszlo Ersek wrote:
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?
In the error case, yes it would exit with a non-zero exit status. In the "don't care about data" case it would exit with 0.
As a concrete example the code currently has to look like this:
case "$1") can_write) echo 0 ;; pwrite) ... if [ there is an error ]; then cat >/dev/null # discard stdin echo 'EIO I/O error' >&2 exit 1 else cat >/dev/null # discard stdin exit 0 fi
where we're saying that the 'cat >/dev/null' commands are unnecessary complication.
OK, I'm *starting* to form an understanding, I think. With a C-language plugin, we never have to worry about the plugin *seeing* the input to the pwrite callback. So it is then up to the plugin to do something with the data. If it wants to return success (0) without processing even one byte from the input, that's on the plugin. Furthermore, if the plugin crashes, then all of nbdkit crashes (I think -- same process, right?), and the client (libnbd) notices that. However, with the shell plugin, we have a more foundational problem. The pwrite callback runs in a separate process, and so "parameter passing" becomes risky in its own right. I'd say: - If a shell plugin pwrite exits normally, then ignoring EPIPE in nbdkit should be OK. Given the explicit exit status (zero or nonzero), and the errno stuff on stderr, we can trust the script to have made a conscious decision, and accept that it didn't want to read all input. Fine. - But if the same script crashes, possibly before it made up its mind about reading all input or not, we don't have a normal exit status (zero or nonzero) to rely upon. One way we learn about the script crash is EPIPE. It's not fool-proof (the script might crash right after it reads the last byte from the input), but when it happens, it indicates something wrong. Let me check the shell plugin for exit status handling... Right, this is what I've been missing: if (WIFSIGNALED (status)) { nbdkit_error ("%s: script terminated by signal %d", argv0, WTERMSIG (status)); goto error; } All good. With this, we can say that we're going to asssume that an EPIPE combined with a normal exit (zero or nonzero status) means the script didn't care for the rest of the input, and assume that EPIPE combined with a child crash is just a child crash, which we're catching anyway. Hence EPIPE makes no difference. Thanks Laszlo
There have been cases where we have forgotten to discard stdin on every exit path and this has caused intermittent test failures in CI:
https://gitlab.com/nbdkit/libnbd/-/commit/4df70870420d1be9348ac45f4aa467501e... https://gitlab.com/nbdkit/libnbd/-/commit/c713529e9fd0641b2d73f764517b5f9c21...
Rich.
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@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@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
On Thu, Aug 31, 2023 at 12:05:17PM +0200, Laszlo Ersek wrote:
On 8/31/23 11:47, Richard W.M. Jones wrote:
On Thu, Aug 31, 2023 at 11:12:59AM +0200, Laszlo Ersek wrote:
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?
In the error case, yes it would exit with a non-zero exit status. In the "don't care about data" case it would exit with 0.
As a concrete example the code currently has to look like this:
case "$1") can_write) echo 0 ;; pwrite) ... if [ there is an error ]; then cat >/dev/null # discard stdin echo 'EIO I/O error' >&2 exit 1 else cat >/dev/null # discard stdin exit 0 fi
where we're saying that the 'cat >/dev/null' commands are unnecessary complication.
OK, I'm *starting* to form an understanding, I think.
With a C-language plugin, we never have to worry about the plugin *seeing* the input to the pwrite callback. So it is then up to the plugin to do something with the data. If it wants to return success (0) without processing even one byte from the input, that's on the plugin. Furthermore, if the plugin crashes, then all of nbdkit crashes (I think -- same process, right?), and the client (libnbd) notices that.
However, with the shell plugin, we have a more foundational problem. The pwrite callback runs in a separate process, and so "parameter passing" becomes risky in its own right.
I'd say:
- If a shell plugin pwrite exits normally, then ignoring EPIPE in nbdkit should be OK. Given the explicit exit status (zero or nonzero), and the errno stuff on stderr, we can trust the script to have made a conscious decision, and accept that it didn't want to read all input. Fine.
- But if the same script crashes, possibly before it made up its mind about reading all input or not, we don't have a normal exit status (zero or nonzero) to rely upon. One way we learn about the script crash is EPIPE. It's not fool-proof (the script might crash right after it reads the last byte from the input), but when it happens, it indicates something wrong.
Let me check the shell plugin for exit status handling...
Right, this is what I've been missing:
if (WIFSIGNALED (status)) { nbdkit_error ("%s: script terminated by signal %d", argv0, WTERMSIG (status)); goto error; }
All good. With this, we can say that we're going to asssume that an EPIPE combined with a normal exit (zero or nonzero status) means the script didn't care for the rest of the input, and assume that EPIPE combined with a child crash is just a child crash, which we're catching anyway. Hence EPIPE makes no difference.
Yes, I think this is the case. We'd see some exit code like 139(?) which would be detected by WIFSIGNALED (above) and that should get turned into EIO back over the wire to the NBD client. Actually let's test that ... $ ./nbdkit -fv -U - eval can_write="echo 0" pwrite='kill -s BUS $$' get_size="echo 1M" --run 'nbdsh -u "$uri" -c "h.pwrite(bytearray(512),0)"' [...] nbdkit: eval[1]: debug: calling: /tmp/nbdkitj4ajqs/pwrite pwrite "" 512 0 "" nbdkit: eval[1]: error: /tmp/nbdkitj4ajqs/pwrite: script terminated by signal 7 nbdkit: eval[1]: error: /tmp/nbdkitj4ajqs/pwrite: script exited with error, but did not print an error message on stderr nbdkit: eval[1]: debug: sending error reply: Input/output error nbdsh: command line script failed: nbd_pwrite: write: command failed: Input/output error Seems to work. The third case is where the plugin exits (not crashes) with !0 which we deal with according to the exit code: https://libguestfs.org/nbdkit-sh-plugin.3.html#Exit-codes Rich.
Thanks Laszlo
There have been cases where we have forgotten to discard stdin on every exit path and this has caused intermittent test failures in CI:
https://gitlab.com/nbdkit/libnbd/-/commit/4df70870420d1be9348ac45f4aa467501e... https://gitlab.com/nbdkit/libnbd/-/commit/c713529e9fd0641b2d73f764517b5f9c21...
Rich.
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@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@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-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Thu, Aug 31, 2023 at 11:12:59AM +0200, Laszlo Ersek wrote:
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!
It took me a couple of tries to realize the subtle distinction. If the child process (aka the plugin script) dies with SIGPIPE (and we intentionally do signal(SIGPIPE, SIG_DFL) before exec'ing the child process, as it is notoriously hard to undo an inherited ignored SIGPIPE in shell), the parent process will see WIFSIGNALED and treat that as an EIO error to the NBD client. If the child process chooses to ignore SIGPIPE, but then sees its own EPIPE failure and exits with non-zero status as a result, the parent process will see the non-zero exit status and report an appropriate error to the NBD client (if it can parse an error name out of the plugin's stderr output, it uses that; otherwise it uses EIO). But if the parent process sees EPIPE, that merely means that the plugin script doesn't care to finish consuming stdin. It is indeterminate at that point whether the child has a reason for ignoring the pipe, so it is inappropriate to blindly treat failure to write to the child as a reason to claim the child would have failed with EIO, without giving the child a chance to exit() (or die by signal) first.
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.
Yes, I liked your counter-proposal better than my attempt. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
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@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
On Thu, Aug 31, 2023 at 10:40:53AM +0200, Laszlo Ersek wrote:
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@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?
This is related to the following commit which Eric pushed yesterday: https://gitlab.com/nbdkit/libnbd/-/commit/4df70870420d1be9348ac45f4aa467501e... The commit is correct. But it's an ongoing problem that we forget to discard all the input in pwrite. Note you have to do this on every path out of the pwrite method, including errors or (like this case) where the test just isn't interested in the data. The error is also intermittent because it is a race between two possible paths through the poll(2) loop in plugins/sh/call.c:call3() Also this case only applies to the sh plugin. Other plugins (eg written in C) ignore or only partially consume the buffer in pwrite all the time, and that's never a problem. In my counter-proposal, just posted but not showing up in the mailing list archives yet, I argue that we should simply ignore EPIPE. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
On 8/31/23 10:55, Richard W.M. Jones wrote:
On Thu, Aug 31, 2023 at 10:40:53AM +0200, Laszlo Ersek wrote:
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@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?
This is related to the following commit which Eric pushed yesterday:
https://gitlab.com/nbdkit/libnbd/-/commit/4df70870420d1be9348ac45f4aa467501e...
The commit is correct. But it's an ongoing problem that we forget to discard all the input in pwrite. Note you have to do this on every path out of the pwrite method, including errors or (like this case) where the test just isn't interested in the data. The error is also intermittent because it is a race between two possible paths through the poll(2) loop in plugins/sh/call.c:call3() Also this case only applies to the sh plugin. Other plugins (eg written in C) ignore or only partially consume the buffer in pwrite all the time, and that's never a problem.
I don't understand. https://libguestfs.org/nbdkit-plugin.3.html#pwrite """ The callback must write the whole count bytes if it can. The NBD protocol doesn't allow partial writes (instead, these would be errors). If the whole count bytes was written successfully, the callback should return 0 to indicate there was no error. If there is an error (including a short write which couldn't be recovered from), .pwrite should call nbdkit_error with an error message, and nbdkit_set_error to record an appropriate error (unless errno is sufficient), then return -1. """ How is it safe for a plugin (written in C) to *silently* throw away part of the data that the client wants written? I agree consistency between sh and other plugins is good, but the "baseline" seems unfathomable to me. I probably don't understand it. Laszlo
In my counter-proposal, just posted but not showing up in the mailing list archives yet, I argue that we should simply ignore EPIPE.
Rich.
On Thu, Aug 31, 2023 at 11:18:27AM +0200, Laszlo Ersek wrote:
On 8/31/23 10:55, Richard W.M. Jones wrote:
On Thu, Aug 31, 2023 at 10:40:53AM +0200, Laszlo Ersek wrote:
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@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?
This is related to the following commit which Eric pushed yesterday:
https://gitlab.com/nbdkit/libnbd/-/commit/4df70870420d1be9348ac45f4aa467501e...
The commit is correct. But it's an ongoing problem that we forget to discard all the input in pwrite. Note you have to do this on every path out of the pwrite method, including errors or (like this case) where the test just isn't interested in the data. The error is also intermittent because it is a race between two possible paths through the poll(2) loop in plugins/sh/call.c:call3() Also this case only applies to the sh plugin. Other plugins (eg written in C) ignore or only partially consume the buffer in pwrite all the time, and that's never a problem.
I don't understand.
https://libguestfs.org/nbdkit-plugin.3.html#pwrite
""" The callback must write the whole count bytes if it can. The NBD protocol doesn't allow partial writes (instead, these would be errors). If the whole count bytes was written successfully, the callback should return 0 to indicate there was no error.
If there is an error (including a short write which couldn't be recovered from), .pwrite should call nbdkit_error with an error message, and nbdkit_set_error to record an appropriate error (unless errno is sufficient), then return -1. """
How is it safe for a plugin (written in C) to *silently* throw away part of the data that the client wants written?
Plugins can do what they want and we have plugins which do discard data, silently or otherwise: https://gitlab.com/nbdkit/nbdkit/-/blob/c662932f2bc71b5879b6eb83c93478191e6e... https://gitlab.com/nbdkit/nbdkit/-/blob/c662932f2bc71b5879b6eb83c93478191e6e... https://gitlab.com/nbdkit/nbdkit/-/blob/c662932f2bc71b5879b6eb83c93478191e6e...
I agree consistency between sh and other plugins is good, but the "baseline" seems unfathomable to me. I probably don't understand it.
sh plugin is often used for testing where the data written doesn't matter but we're interested in other metadata like what write calls were issued, eg: https://gitlab.com/nbdkit/libnbd/-/blob/c713529e9fd0641b2d73f764517b5f9c21a7... Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Thu, Aug 31, 2023 at 10:52:59AM +0100, Richard W.M. Jones wrote:
Other plugins (eg written in C) ignore or only partially consume the buffer in pwrite all the time, and that's never a problem.
I don't understand.
https://libguestfs.org/nbdkit-plugin.3.html#pwrite
""" The callback must write the whole count bytes if it can. The NBD protocol doesn't allow partial writes (instead, these would be errors). If the whole count bytes was written successfully, the callback should return 0 to indicate there was no error.
If there is an error (including a short write which couldn't be recovered from), .pwrite should call nbdkit_error with an error message, and nbdkit_set_error to record an appropriate error (unless errno is sufficient), then return -1. """
How is it safe for a plugin (written in C) to *silently* throw away part of the data that the client wants written?
Plugins can do what they want and we have plugins which do discard data, silently or otherwise:
https://gitlab.com/nbdkit/nbdkit/-/blob/c662932f2bc71b5879b6eb83c93478191e6e... https://gitlab.com/nbdkit/nbdkit/-/blob/c662932f2bc71b5879b6eb83c93478191e6e... https://gitlab.com/nbdkit/nbdkit/-/blob/c662932f2bc71b5879b6eb83c93478191e6e...
I agree consistency between sh and other plugins is good, but the "baseline" seems unfathomable to me. I probably don't understand it.
sh plugin is often used for testing where the data written doesn't matter but we're interested in other metadata like what write calls were issued, eg:
https://gitlab.com/nbdkit/libnbd/-/blob/c713529e9fd0641b2d73f764517b5f9c21a7...
I look at it as: the plugin decides HOW to process the bytes that the client requested to write. A traditional plugin will store the data so it can be read back later (in which case, failing to read all of stdin better result in the plugin returning failure). But during testing, a plugin can declare "I am a data sink; I don't care what bytes you write, my behavior is to ignore them and focus on something else" (for example, it can be convenient to throw out a quick sh plugin that has a side effect of counting .pwrite calls). Such a plugin is going to confuse any client that expects to read back what it just wrote, but when you are testing the protocol with a client that only sends writes (and doesn't even attempt to send read commands, because it knows the writes aren't persistent), that is just fine. In other words, as long as your plugin documents that writes are not persistent, then returning success without draining stdin should be acceptable; just the same as a C plugin .pwrite that ignores the input buffer can still return success. Of course, until newer nbdkit has propagated to systems we care about, any unit tests using sh or eval plugins as a data sink still have to inject 'cat >/dev/null' to drain stdin, in order to pass under heavy load when EPIPE even happens in the first place (I was surprised at how hard it is to lose the race and actually get an EPIPE message from nbdkit talking to the sh plugin script on my machine). -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
I pushed this as commit d40eb85b2 including the changes suggested by Laszlo and Eric. Thanks! Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
participants (3)
-
Eric Blake -
Laszlo Ersek -
Richard W.M. Jones