Libguestfs - Jan 2022 - [nbdkit PATCH 2/2] server/sockets: get rid of AI_ADDRCONFIG

On Tue, Jan 18, 2022 at 02:48:33PM +0100, Laszlo Ersek
wrote:
> The AI_ADDRCONFIG hint of getaddrinfo() is supposed to restrict the name
> resolution to such address families (IPv4 vs. IPv6) for which the
> resolving host has publicly routable addresses assigned.
> 
> The main problem with AI_ADDRCONFIG can be shown with the following
> command line:
> 
> $ nbdkit -f -p 32776 -P pidfile -i ::1 -exit-with-parent null
s/-exit/--exit/
> 
> On a host where ::1 is the only IPv6 address assigned (namely to the
> loopback interface), the command fails with
> 
> > nbdkit: getaddrinfo: ::1: 32776: Address family for hostname not
> > supported
> 
> due to the "publicly routable" requirement.
> 
> Remove AI_ADDRCONFIG from the getaddrinfo() hints, and as a replacement,
> introduce the "-4" and "-6" options, similarly to
netcat and ssh.
Hmm, I said in my earlier reply that it may not be worth the extra
effort, but now that you've done it, it would be a shame to rip it
out.
> 
> (1) This makes options of the form:
> 
>     -i 127.0.0.1
>     -i ::1
> 
>     work regardless of "public" IPv6 / IPv4 connectivity;
> 
> (2) options of the form
> 
>     -i localhost
>     -i FQDN
> 
>     will bind both IPv4 and IPv6 addresses of the desired interface(s);
> 
> (3) omitting the option "-i" will bind both IPv4 and IPv6
wildcard
>     addresses (0.0.0.0 and ::);
> 
> (4) the configurations in (2) and (3) can be restricted to IPv4 or IPv6
>     addresses by adding the "-4" or "-6" option,
respectively.
> 
> Importantly, this change allows the "connect-tcp6" test case of
libnbd to
> pass on such hosts that have no IPv6 connectivity (i.e., where the only
> assigned IPv6 address is ::1, namely on the loopback interface).
> 
> Ref:
https://listman.redhat.com/archives/libguestfs/2022-January/msg00110.html
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
> +++ b/server/main.c
> @@ -86,6 +86,7 @@ static void error_if_stdio_closed (void);
>  static void switch_stdio (void);
>  static void winsock_init (void);
>  
> +int tcpip_sock_af = AF_UNSPEC;  /* -4, -6 */
>  struct debug_flag *debug_flags; /* -D */
>  bool exit_with_parent;          /* --exit-with-parent */
>  const char *export_name;        /* -e */
> @@ -367,6 +368,14 @@ main (int argc, char *argv[])
>        exit (EXIT_FAILURE);
>  #endif
>  
> +    case '4':
> +      tcpip_sock_af = AF_INET;
> +      break;
> +
> +    case '6':
> +      tcpip_sock_af = AF_INET6;
> +      break;
Thus, if the user uses nbdkit -46 (or -64), the last one specified
silently overrides the earlier one, rather than being diagnosed as
conflicting or explicitly permitting both.  The override effect
matches the long-option naming --ipv4-only, so I can live with it, but
I'm also open to the idea of explicitly adding code to diagnose both
options at the same time as an error, if we think that's friendlier
than silent override.
> +++ b/docs/nbdkit.pod
> @@ -173,6 +173,24 @@ Display information about nbdkit or a specific plugin:
>  
>  Display brief command line usage information and exit.
>  
> +=item B<-4>
> +
> +=item B<--ipv4-only>
> +
> +=item B<-6>
> +
> +=item B<--ipv6-only>
> +
> +When a non-numeric argument is passed to the I<-i> option (such as a
> +Fully Qualified Domain Name, or a host name from C</etc/hosts>),
> +restrict the name resolution to IPv4 or IPv6 addresses.
> +
> +When the I<-i> option is omitted, listen on only the IPv4 or IPv6
> +address of all interfaces (C<0.0.0.0> or C<::>, respectively).
This effect (when -i is not used) is good justification for having
-4/-6 support.
> +
> +When both I<-4> and I<-6> options are present on the command
line, the
> +last one takes effect.
Ah, so you documented the override as intentional.  It is possible to
use a git orderfile to create patches with documentation changes
appearing first in the output (see scripts/git.orderfile), which can
make it easier to review related changes in logical (rather than
alphabetical filename) order.

So with that, you have my ACK for the series, after addressing the
minor typo I spotted.

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

On 01/18/22 18:19, Eric Blake wrote:
> On Tue, Jan 18, 2022 at 02:48:33PM +0100, Laszlo Ersek wrote:
>> The AI_ADDRCONFIG hint of getaddrinfo() is supposed to restrict the
name
>> resolution to such address families (IPv4 vs. IPv6) for which the
>> resolving host has publicly routable addresses assigned.
>>
>> The main problem with AI_ADDRCONFIG can be shown with the following
>> command line:
>>
>> $ nbdkit -f -p 32776 -P pidfile -i ::1 -exit-with-parent null
> 
> s/-exit/--exit/
Ah yes. TBH I noticed this myself yesterday, but not in the commit
message -- in the terminal where I copied & pasted the command line
from. There must have been something up with UTF-8 and the clipboard or
whatever, because the terminal kept somehow mangling the double-dash at
the front of "--exit-with-parent". So I fixed that ultimately in the
terminal, but not in the commit message.
> 
>>
>> On a host where ::1 is the only IPv6 address assigned (namely to the
>> loopback interface), the command fails with
>>
>>> nbdkit: getaddrinfo: ::1: 32776: Address family for hostname not
>>> supported
>>
>> due to the "publicly routable" requirement.
>>
>> Remove AI_ADDRCONFIG from the getaddrinfo() hints, and as a
replacement,
>> introduce the "-4" and "-6" options, similarly to
netcat and ssh.
> 
> Hmm, I said in my earlier reply that it may not be worth the extra
> effort, but now that you've done it, it would be a shame to rip it
> out.
Thanks, I feel the same way! :)
> 
>>
>> (1) This makes options of the form:
>>
>>     -i 127.0.0.1
>>     -i ::1
>>
>>     work regardless of "public" IPv6 / IPv4 connectivity;
>>
>> (2) options of the form
>>
>>     -i localhost
>>     -i FQDN
>>
>>     will bind both IPv4 and IPv6 addresses of the desired interface(s);
>>
>> (3) omitting the option "-i" will bind both IPv4 and IPv6
wildcard
>>     addresses (0.0.0.0 and ::);
>>
>> (4) the configurations in (2) and (3) can be restricted to IPv4 or IPv6
>>     addresses by adding the "-4" or "-6" option,
respectively.
>>
>> Importantly, this change allows the "connect-tcp6" test case
of libnbd to
>> pass on such hosts that have no IPv6 connectivity (i.e., where the only
>> assigned IPv6 address is ::1, namely on the loopback interface).
>>
>> Ref:
https://listman.redhat.com/archives/libguestfs/2022-January/msg00110.html
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
> 
>> +++ b/server/main.c
>> @@ -86,6 +86,7 @@ static void error_if_stdio_closed (void);
>>  static void switch_stdio (void);
>>  static void winsock_init (void);
>>  
>> +int tcpip_sock_af = AF_UNSPEC;  /* -4, -6 */
>>  struct debug_flag *debug_flags; /* -D */
>>  bool exit_with_parent;          /* --exit-with-parent */
>>  const char *export_name;        /* -e */
>> @@ -367,6 +368,14 @@ main (int argc, char *argv[])
>>        exit (EXIT_FAILURE);
>>  #endif
>>  
>> +    case '4':
>> +      tcpip_sock_af = AF_INET;
>> +      break;
>> +
>> +    case '6':
>> +      tcpip_sock_af = AF_INET6;
>> +      break;
> 
> Thus, if the user uses nbdkit -46 (or -64), the last one specified
> silently overrides the earlier one, rather than being diagnosed as
> conflicting or explicitly permitting both.  The override effect
> matches the long-option naming --ipv4-only, so I can live with it, but
> I'm also open to the idea of explicitly adding code to diagnose both
> options at the same time as an error, if we think that's friendlier
> than silent override.
> 
>> +++ b/docs/nbdkit.pod
>> @@ -173,6 +173,24 @@ Display information about nbdkit or a specific
plugin:
>>  
>>  Display brief command line usage information and exit.
>>  
>> +=item B<-4>
>> +
>> +=item B<--ipv4-only>
>> +
>> +=item B<-6>
>> +
>> +=item B<--ipv6-only>
>> +
>> +When a non-numeric argument is passed to the I<-i> option (such
as a
>> +Fully Qualified Domain Name, or a host name from C</etc/hosts>),
>> +restrict the name resolution to IPv4 or IPv6 addresses.
>> +
>> +When the I<-i> option is omitted, listen on only the IPv4 or
IPv6
>> +address of all interfaces (C<0.0.0.0> or C<::>,
respectively).
> 
> This effect (when -i is not used) is good justification for having
> -4/-6 support.
> 
>> +
>> +When both I<-4> and I<-6> options are present on the
command line, the
>> +last one takes effect.
> 
> Ah, so you documented the override as intentional.
Yes, I had written some getopt() loops in the distant past, and
remembered this aspect.
> It is possible to
> use a git orderfile to create patches with documentation changes
> appearing first in the output (see scripts/git.orderfile), which can
> make it easier to review related changes in logical (rather than
> alphabetical filename) order.
Absolutely! My top-level $HOME/.gitconfig actually sets
"diff.orderFile"
to an order file I created as my "global default"; it's just that
(a)
this file does not deal with *.pod files at all yet (I don't think I've
edited a POD file before), plus (b) I didn't know about
"scripts/git.orderfile" in nbdkit, and so I had not added a
"diff.orderFile" override to .git/config in my clone.
> So with that, you have my ACK for the series, after addressing the
> minor typo I spotted.
Thank you!
Laszlo

On Tue, Jan 18, 2022 at 11:19:18AM -0600, Eric Blake
wrote:
> On Tue, Jan 18, 2022 at 02:48:33PM +0100, Laszlo Ersek wrote:
> > +    case '4':
> > +      tcpip_sock_af = AF_INET;
> > +      break;
> > +
> > +    case '6':
> > +      tcpip_sock_af = AF_INET6;
> > +      break;
> 
> Thus, if the user uses nbdkit -46 (or -64), the last one specified
> silently overrides the earlier one, rather than being diagnosed as
> conflicting or explicitly permitting both.  The override effect
> matches the long-option naming --ipv4-only, so I can live with it, but
> I'm also open to the idea of explicitly adding code to diagnose both
> options at the same time as an error, if we think that's friendlier
> than silent override.
Is that actually an error?  -4 -6 says you want to listen on IPv4 and
IPv6 which is redundant but not wrong.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v