[Libguestfs] question on nbdkit sh plugin

[Richard W.M. Jones]

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/2c349a00d27957911ea0ee7704420aec0715eea9/daemon/command.c#L332

(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