10:42 p.m.
I'm pushing the first one as blatantly obvious.
The second one is also simple enough, but not enough of a bug for me
to push tonight.
The third is something I noticed while working on sh, but is really
more about docs vs. plugins in general. There, we could either change
the code to match the docs (breaking backwards behavior for a plugin
that set .errno_is_preserved=2) [what my patch did], or change the
docs to match the code (mention that any non-zero value will do).
Preference?
Eric Blake (3):
sh: Fix flags when none are present
sh: Avoid setenv after fork
plugins: Match docs for .errno_is_preserved
docs/nbdkit-plugin.pod | 16 ++++++++++++++++
plugins/sh/call.c | 3 ---
plugins/sh/sh.c | 8 ++++++++
server/plugins.c | 2 +-
tests/test-shell.sh | 15 +++++++++++++++
5 files changed, 40 insertions(+), 4 deletions(-)
--
2.20.1
10:42 p.m.
New subject: [nbdkit PATCH 1/3] sh: Fix flags when none are present
fbuf was used uninitialized, which meant we printed garbage data from
the heap when flags was 0. Update the test to prevent regressions.
Fixes: bcac9c40
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/sh/sh.c | 2 ++
tests/test-shell.sh | 15 +++++++++++++++
2 files changed, 17 insertions(+)
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index aeb01918..737c38cf 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -453,6 +453,8 @@ flags_string (uint32_t flags, char *buf, size_t len)
{
bool comma = false;
+ buf[0] = '\0';
+
if (flags & NBDKIT_FLAG_FUA)
flag_append ("fua", &comma, &buf, &len);
diff --git a/tests/test-shell.sh b/tests/test-shell.sh
index a744b275..53c0b943 100755
--- a/tests/test-shell.sh
+++ b/tests/test-shell.sh
@@ -42,6 +42,11 @@ case "$1" in
;;
pwrite)
+ case $5 in
+ '' | fua) ;;
+ *) echo "garbage flags: '$5'" >&2
+ exit 1;
+ esac
dd oflag=seek_bytes conv=notrunc seek=$4 of=$f || exit 1
;;
can_write)
@@ -54,6 +59,11 @@ case "$1" in
;;
trim)
+ case $5 in
+ '' | fua) ;;
+ *) echo "garbage flags: '$5'" >&2
+ exit 1;
+ esac
fallocate -p -o $4 -l $3 -n $f
;;
can_trim)
@@ -62,6 +72,11 @@ case "$1" in
;;
zero)
+ case $5 in
+ '' | fua | may_trim | fua,may_trim ) ;;
+ *) echo "garbage flags: '$5'" >&2
+ exit 1;
+ esac
fallocate -z -o $4 -l $3 -n $f
;;
can_zero)
--
2.20.1
10:42 p.m.
New subject: [nbdkit PATCH 2/3] sh: Avoid setenv after fork
setenv() is not async-signal-safe and as such should not be used
between fork/exec of a multi-threaded app: if one thread is
manipulating the current environment (which may entail obtaining a
malloc() mutex) when another thread calls fork(), the resulting
child's attempt to use setenv() could deadlock or see a broken environ
because the thread owning the lock no longer exists to release it.
Besides, it is more efficient to update the environment exactly once
in the parent, rather than after every fork().
While at it, check for (unlikely) failure of setenv.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/sh/call.c | 3 ---
plugins/sh/sh.c | 6 ++++++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/plugins/sh/call.c b/plugins/sh/call.c
index 871de5c6..9b8b48e2 100644
--- a/plugins/sh/call.c
+++ b/plugins/sh/call.c
@@ -127,9 +127,6 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
/* Restore SIGPIPE back to SIG_DFL, since shell can't undo SIG_IGN */
signal (SIGPIPE, SIG_DFL);
- /* Set $tmpdir for the script. */
- setenv ("tmpdir", tmpdir, 1);
-
execvp (argv[0], (char **) argv);
perror (argv[0]);
_exit (EXIT_FAILURE);
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 737c38cf..e3d3c2f1 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -60,6 +60,12 @@ sh_load (void)
nbdkit_error ("mkdtemp: /tmp: %m");
exit (EXIT_FAILURE);
}
+ /* Set $tmpdir for the script. */
+ if (setenv ("tmpdir", tmpdir, 1) == -1) {
+ nbdkit_error ("setenv: tmpdir=%s: %m", tmpdir);
+ exit (EXIT_FAILURE);
+ }
+
nbdkit_debug ("sh: load: tmpdir: %s", tmpdir);
}
--
2.20.1
2:41 a.m.
New subject: [nbdkit PATCH 2/3] sh: Avoid setenv after fork
On Thu, Aug 01, 2019 at 10:42:58PM -0500, Eric Blake wrote:
setenv() is not async-signal-safe and as such should not be used
between fork/exec of a multi-threaded app: if one thread is
manipulating the current environment (which may entail obtaining a
malloc() mutex) when another thread calls fork(), the resulting
child's attempt to use setenv() could deadlock or see a broken environ
because the thread owning the lock no longer exists to release it.
Besides, it is more efficient to update the environment exactly once
in the parent, rather than after every fork().
While at it, check for (unlikely) failure of setenv.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/sh/call.c | 3 ---
plugins/sh/sh.c | 6 ++++++
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/plugins/sh/call.c b/plugins/sh/call.c
index 871de5c6..9b8b48e2 100644
--- a/plugins/sh/call.c
+++ b/plugins/sh/call.c
@@ -127,9 +127,6 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
/* Restore SIGPIPE back to SIG_DFL, since shell can't undo SIG_IGN */
signal (SIGPIPE, SIG_DFL);
- /* Set $tmpdir for the script. */
- setenv ("tmpdir", tmpdir, 1);
-
execvp (argv[0], (char **) argv);
perror (argv[0]);
_exit (EXIT_FAILURE);
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 737c38cf..e3d3c2f1 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -60,6 +60,12 @@ sh_load (void)
nbdkit_error ("mkdtemp: /tmp: %m");
exit (EXIT_FAILURE);
}
+ /* Set $tmpdir for the script. */
+ if (setenv ("tmpdir", tmpdir, 1) == -1) {
+ nbdkit_error ("setenv: tmpdir=%s: %m", tmpdir);
+ exit (EXIT_FAILURE);
+ }
+
nbdkit_debug ("sh: load: tmpdir: %s", tmpdir);
}
This sets $tmpdir in the whole nbdkit process ... which may or may not
be a problem. It's probably not a problem in reality, but I wonder if
it's better to to use ‘execvpe’ (or more portably but more difficult
to use ‘execve’) to set the environment in the exec call? I guess
we'd have to copy and extend environ in the parent since we want to
pass existing environment variables through.
Rich.
--
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
6:31 a.m.
New subject: [nbdkit PATCH 2/3] sh: Avoid setenv after fork
On 8/2/19 2:41 AM, Richard W.M. Jones wrote:
On Thu, Aug 01, 2019 at 10:42:58PM -0500, Eric Blake wrote:
> setenv() is not async-signal-safe and as such should not be used
> between fork/exec of a multi-threaded app: if one thread is
> manipulating the current environment (which may entail obtaining a
> malloc() mutex) when another thread calls fork(), the resulting
> child's attempt to use setenv() could deadlock or see a broken environ
> because the thread owning the lock no longer exists to release it.
> Besides, it is more efficient to update the environment exactly once
> in the parent, rather than after every fork().
>
> +++ b/plugins/sh/sh.c
> @@ -60,6 +60,12 @@ sh_load (void)
> nbdkit_error ("mkdtemp: /tmp: %m");
> exit (EXIT_FAILURE);
> }
> + /* Set $tmpdir for the script. */
> + if (setenv ("tmpdir", tmpdir, 1) == -1) {
> + nbdkit_error ("setenv: tmpdir=%s: %m", tmpdir);
> + exit (EXIT_FAILURE);
> + }
> +
> nbdkit_debug ("sh: load: tmpdir: %s", tmpdir);
> }
This sets $tmpdir in the whole nbdkit process ... which may or may not
be a problem.
Correct, but .load is run exactly once, and nbdkit doesn't allow more
than one plugin to be loaded, so I don't see this as being a problem
unless down the road we find a way to load more than one plugin at once
while still honoring semantics that plugins have gotten used to of being
the only thing running.
It's probably not a problem in reality, but I wonder if
it's better to to use ‘execvpe’ (or more portably but more difficult
to use ‘execve’) to set the environment in the exec call? I guess
we'd have to copy and extend environ in the parent since we want to
pass existing environment variables through.
If we were doing a different $tmpdir per connection, then yes, we'd have
to do that. But with the same $tmpdir shared among all connections (the
shell script can then create a different subdir per connection and use
that as its handle, if per-connection differences are desired), I don't
see the point in the extra complexity at this time.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
10:42 p.m.
New subject: [nbdkit PATCH 3/3] plugins: Match docs for .errno_is_preserved
Ever since commit 69ae137f, the docs claimed that we check
.errno_is_preserved == 1, but the code has checked for != 0.
Furthermore, the mention of .errno_is_preserved in the docs was rather
hidden; a new section will make it easier to add future knobs that
likewise affect the plugin as a whole and not an individual
connection.
Fixes: 69ae137f
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
docs/nbdkit-plugin.pod | 16 ++++++++++++++++
server/plugins.c | 2 +-
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index c764f8b0..d778d6af 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -915,6 +915,22 @@ If there is an error, C<.cache> should call
C<nbdkit_error> with an
error message, and C<nbdkit_set_error> to record an appropriate error
(unless C<errno> is sufficient), then return C<-1>.
+=head1 OTHER FIELDS
+
+The plugin struct also contains an integer field used as a
+boolean in C code, but unlikely to be exposed in other language
+bindings:
+
+=over 4
+
+=item C<.errno_is_preserved>
+
+This defaults to 0; if non-zero, nbdkit can reliably use the value of
+C<errno> when a callback reports failure, rather than the plugin
+having to call C<nbdkit_set_error>.
+
+=back
+
=head1 THREADS
Each nbdkit plugin must declare its maximum thread safety model by
diff --git a/server/plugins.c b/server/plugins.c
index 8eed2dd8..7da2329e 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -492,7 +492,7 @@ get_error (struct backend_plugin *p)
{
int ret = threadlocal_get_error ();
- if (!ret && p->plugin.errno_is_preserved)
+ if (!ret && p->plugin.errno_is_preserved == 1)
ret = errno;
return ret ? ret : EIO;
}
--
2.20.1
2:42 a.m.
New subject: [nbdkit PATCH 3/3] plugins: Match docs for .errno_is_preserved
So the series is fine, ACK. I made a comment about an alternate way
to implement patch 2, but I don't know if it's better or not.
Thanks,
Rich.
--
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
1972
days inactive
1972
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones