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/2c349a00d27957911ea0ee7704420a... (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