Re: [Libguestfs] question on nbdkit sh plugin
On Mon, Sep 10, 2018 at 08:57:25AM -0500, Eric Blake wrote:
In the just-added nbdkit-sh-plugin.pod, you documented that unlike
other plugins (where introspection or struct member population) can
be used to determine which functionalities are supported, the shell
plugin has to implement can_write and friends to return a special
status 2 to document not supported, and 3 to indicate false, in part
because you can't detect if a pwrite function is present without
running it.
But since pwrite normally takes additional parameters, could we
instead document that nbdkit calling '/path/to/script pwrite
<handle>' while intentionally omitting <count> and <offset> is
sufficient for that to return specific status (2 if pwrite in
general is not handled, 3 if it is handled but not appropriate for
the given handle, and 0 if it works), without needing a 'can_write'
entry point? I guess the only difference is that all the logic would
go through the single pwrite interface and you wouln't need a
can_write.
I certainly thought about this approach. However it complicates the
shell scripts, which would have to remember to do:
pwrite)
if [ $# -ge 1 ]; then
do the dd command
fi
;;
(I never remember how $# is supposed to work, and so I suppose others
don't either.)
While it's just pushing around the complexity (the shell scripts need
to remember to implement can_write) I believe the way it is now is a
little less complicated.
Also, I spotted a potential bug in sh.c:
+/* This is the generic function that calls the script. It can
+ * optionally write to the script's stdin and read from the script's
+ * stdout and stderr. It returns the raw error code and does no error
+ * processing.
+ */
+static int
+call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
+ char **rbuf, size_t *rbuflen, /* read from stdout */
+ char **ebuf, size_t *ebuflen, /* read from stderr */
+ const char **argv) /* script + parameters */
+{
...
+
+ execvp (argv[0], (char **) argv);
+ perror (argv[0]);
+ _exit (EXIT_FAILURE);
+ }
perror() is not async-safe, but since nbdkit may be multithreaded,
calling a non-async-safe function in between fork() and successful
exec/exit() can deadlock or cause other problems. I don't know if
it is worth trying to make that code safer, though.
Do you know if there's a reasonable fix for this? We do it absolutely
everywhere in libguestfs too, eg:
https://github.com/libguestfs/libguestfs/blob/2c349a00d27957911ea0ee77044...
(You'll find at least another half dozen in the libguestfs sources,
the above just happened to be the first one in ‘git grep’).
This is on an error path so it should be reasonably rare. If it was
doing this on the normal path then it's something we should urgently
address, but on the error path - not so much.
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