Re: [Libguestfs] [nbdkit PATCH v2 1/3] server: Add nbdkit_stdio_safe
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