Libguestfs - Apr 2020 - Re: [nbdkit PATCH v2 1/3] server: Add nbdkit_stdio_safe

On Mon, Apr 13, 2020 at 07:28:59PM -0500, Eric Blake wrote:
[...]

This patch is fine and can be pushed if you want, but I've got some
small comments.
> +If C<nbdkit_stdio_safe> returns true, the value of the configuration
> +parameter may be used to trigger reading additional data through stdin
> +(such as a password or inline script).
I wonder if we want to say "returns 1" rather than true, to give
ourselves wiggle room in future in case we suddenly decided that we
needed this to return an error indication?  On the other hand, maybe
errors can never happen in any conceivable situation.
> @@ -455,6 +467,10 @@ nbdkit_read_password (const char *value, char
**password)
> 
>      if (nbdkit_parse_int ("password file descriptor",
&value[1], &fd) == -1)
>        return -1;
> +    if (!nbdkit_stdio_safe () && fd < STDERR_FILENO) {
I think this could be clearer written the other way around:

    if (fd < STDERR_FILENO && !nbdkit_stdio_safe ()) {

but then thinking about this more, why isn't it this?

    if (fd == STDIN_FILENO && !nbdkit_stdio_safe ()) {

Anyway, these are minor points, ACK.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On 4/14/20 2:47 AM, Richard W.M. Jones wrote:
> On Mon, Apr 13, 2020 at 07:28:59PM -0500, Eric Blake wrote:
> [...]
> 
> This patch is fine and can be pushed if you want, but I've got some
> small comments.
> 
>> +If C<nbdkit_stdio_safe> returns true, the value of the
configuration
>> +parameter may be used to trigger reading additional data through stdin
>> +(such as a password or inline script).
> 
> I wonder if we want to say "returns 1" rather than true, to give
> ourselves wiggle room in future in case we suddenly decided that we
> needed this to return an error indication?  On the other hand, maybe
> errors can never happen in any conceivable situation.
Sure, I can clean up the wording.
> 
>> @@ -455,6 +467,10 @@ nbdkit_read_password (const char *value, char
**password)
>>
>>       if (nbdkit_parse_int ("password file descriptor",
&value[1], &fd) == -1)
>>         return -1;
>> +    if (!nbdkit_stdio_safe () && fd < STDERR_FILENO) {
> 
> I think this could be clearer written the other way around:
> 
>      if (fd < STDERR_FILENO && !nbdkit_stdio_safe ()) {
> 
> but then thinking about this more, why isn't it this?
> 
>      if (fd == STDIN_FILENO && !nbdkit_stdio_safe ()) {
nbdkit_stdio_safe controls not only whether you should avoid reading 
stdin, but also whether you should avoid writing to stdout.  Then again, 
no one in their right mind tries to read a password from stdout (even 
when it is opened bi-directionally), and nbdkit_read_password is only 
about reading.  So limiting to fd == STDIN_FILENO doesn't seem too bad.
> 
> Anyway, these are minor points, ACK.
> 
> Rich.
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 4/14/20 2:47 AM, Richard W.M. Jones wrote:
> On Mon, Apr 13, 2020 at 07:28:59PM -0500, Eric Blake wrote:
> [...]
> 
> This patch is fine and can be pushed if you want, but I've got some
> small comments.
> 
>> +If C<nbdkit_stdio_safe> returns true, the value of the
configuration
>> +parameter may be used to trigger reading additional data through stdin
>> +(such as a password or inline script).
> 
> I wonder if we want to say "returns 1" rather than true, to give
> ourselves wiggle room in future in case we suddenly decided that we
> needed this to return an error indication?  On the other hand, maybe
> errors can never happen in any conceivable situation.
It might be possible to change the function to return -1 when called at 
the wrong time (at .get_ready or later).  As written, the patch merely 
documents that this function is not useful at that point.  But for now, 
I didn't see the point in making that change, but merely leaving it for 
the future.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Apr 14, 2020 at 09:05:57AM -0500, Eric Blake
wrote:
> On 4/14/20 2:47 AM, Richard W.M. Jones wrote:
> >On Mon, Apr 13, 2020 at 07:28:59PM -0500, Eric Blake wrote:
> >[...]
> >
> >This patch is fine and can be pushed if you want, but I've got some
> >small comments.
> >
> >>+If C<nbdkit_stdio_safe> returns true, the value of the
configuration
> >>+parameter may be used to trigger reading additional data through
stdin
> >>+(such as a password or inline script).
> >
> >I wonder if we want to say "returns 1" rather than true, to
give
> >ourselves wiggle room in future in case we suddenly decided that we
> >needed this to return an error indication?  On the other hand, maybe
> >errors can never happen in any conceivable situation.
> 
> It might be possible to change the function to return -1 when called
> at the wrong time (at .get_ready or later).  As written, the patch
> merely documents that this function is not useful at that point.
> But for now, I didn't see the point in making that change, but
> merely leaving it for the future.
It's still wrong for the plugin to read from stdin or write to stdout
even after .get_ready, so AFAICT checking nbdkit_stdio_safe() is
reasonable [shouldn't return -1] after get_ready.

Anyway you decide what's best.  Patch series over all is fine.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/