Patch to improve handling of server sockets

Hi,

I am maintaining the tightvnc package for openSUSE and was recently 
confronted with an alleged vnc problem with QWMU that turned out to be 
a shortcoming in QEMU's code for handling TCP server sockets, which is 
used by the vnc and char modules.

The problem occurs when the address to listen on is given as a name 
which resolves to multiple IP addresses the most prominent example 
being "localhost" resolving to 127.0.0.1 and ::1 .

The existing code stopped walking the list of addresses returned by 
getaddrinfo() as soon as one socket was successfully opened and bound. 
The result was that a qemu instance started with "-vnc localhost:42" 
only listened on ::1, wasn't reachable through 127.0.0.1. The fact 
that the code set the IPV6_V6ONLY socket option didn't help, because 
that option only works when the socket gets bound to the IPv6 wildcard 
address (::), but is useless for explicit address bindings.

The attached patch against QEMU 0.11.0 extends inet_listen() to create 
sockets for as many addresses from the address list as possible and 
adapts its callers and their data structures to deal with a linked 
list of socket FDs rather than a single file descriptor.

So far I've only done some testing with the -vnc option. More testing 
is needed in the qemu-char area and for the parts of the code that get 
triggered from QEMU's Monitor.


Please review and comment.


cu
 	Reinhard

P.S. Please keep me in Cc when replying.

On 05/04/2010 08:49 AM, Reinhard Max wrote:
> Hi,
>
> I am maintaining the tightvnc package for openSUSE and was recently 
> confronted with an alleged vnc problem with QWMU that turned out to be 
> a shortcoming in QEMU's code for handling TCP server sockets, which is 
> used by the vnc and char modules.
>
> The problem occurs when the address to listen on is given as a name 
> which resolves to multiple IP addresses the most prominent example 
> being "localhost" resolving to 127.0.0.1 and ::1 .
>
> The existing code stopped walking the list of addresses returned by 
> getaddrinfo() as soon as one socket was successfully opened and bound. 
> The result was that a qemu instance started with "-vnc localhost:42" 
> only listened on ::1, wasn't reachable through 127.0.0.1. The fact 
> that the code set the IPV6_V6ONLY socket option didn't help, because 
> that option only works when the socket gets bound to the IPv6 wildcard 
> address (::), but is useless for explicit address bindings.
>
> The attached patch against QEMU 0.11.0 extends inet_listen() to create 
> sockets for as many addresses from the address list as possible and 
> adapts its callers and their data structures to deal with a linked 
> list of socket FDs rather than a single file descriptor.
>
> So far I've only done some testing with the -vnc option. More testing 
> is needed in the qemu-char area and for the parts of the code that get 
> triggered from QEMU's Monitor.

0.11.0 is pretty old.  Please update your patch against the latest git.

But that said, I'm not sure we're doing the wrong thing right now.  
Gerd, what do you think about this behavior?

Regards,

Anthony Liguori

>
> Please review and comment.
>
>
> cu
>     Reinhard
>
> P.S. Please keep me in Cc when replying.

Hi,

On Tue, 4 May 2010 at 11:23, Anthony Liguori wrote:

> 0.11.0 is pretty old.  Please update your patch against the latest git.

Ok, will do.

> I'm not sure we're doing the wrong thing right now.

Well, I think it just can't be correct, that an IPv6-enabled process 
running on a dual-stack machine when it gets told to listen on 
"localhost", ends up only listening on ::1 and doesn't accept 
connections to 127.0.0.1.


cu
 	Reinhard

On 05/04/2010 03:44 PM, Reinhard Max wrote:
> Hi,
>
> On Tue, 4 May 2010 at 11:23, Anthony Liguori wrote:
>
>> 0.11.0 is pretty old.  Please update your patch against the latest git.
>
> Ok, will do.
>
>> I'm not sure we're doing the wrong thing right now.
>
> Well, I think it just can't be correct, that an IPv6-enabled process 
> running on a dual-stack machine when it gets told to listen on 
> "localhost", ends up only listening on ::1

My understanding is that for the majority of systems, 127.0.0.1 should 
still connect to ::1.  The reason we have the ipv4 and ipv6 options is 
because this doesn't work 100% of the time.  Maybe your configuration is 
an example of this.

Honestly, I don't understand how ipv4 compat is supposed to work outside 
of qemu so I'm looking for some additional input.

Regards,

Anthony Liguori

> and doesn't accept connections to 127.0.0.1.
>
>
> cu
>     Reinhard

On 05/04/10 18:23, Anthony Liguori wrote:
> On 05/04/2010 08:49 AM, Reinhard Max wrote:
>> Hi,
>>
>> I am maintaining the tightvnc package for openSUSE and was recently
>> confronted with an alleged vnc problem with QWMU that turned out to be
>> a shortcoming in QEMU's code for handling TCP server sockets, which is
>> used by the vnc and char modules.
>>
>> The problem occurs when the address to listen on is given as a name
>> which resolves to multiple IP addresses the most prominent example
>> being "localhost" resolving to 127.0.0.1 and ::1 .

My tigervnc (tightvnc successor) has IPv6 support and handles this just 
fine.  There is also the option to force qemu to listen on ipv4 (or 
ipv6) only.

>> The existing code stopped walking the list of addresses returned by
>> getaddrinfo() as soon as one socket was successfully opened and bound.
>> The result was that a qemu instance started with "-vnc localhost:42"
>> only listened on ::1, wasn't reachable through 127.0.0.1. The fact
>> that the code set the IPV6_V6ONLY socket option didn't help, because
>> that option only works when the socket gets bound to the IPv6 wildcard
>> address (::), but is useless for explicit address bindings.

Indeed.

> But that said, I'm not sure we're doing the wrong thing right now. Gerd,
> what do you think about this behavior?

Reinhard is correct.  If a hostname resolves to multiple addresses like 
this ...

    zweiblum kraxel ~# host zweiblum
    zweiblum.home.kraxel.org has address 192.168.2.101
    zweiblum.home.kraxel.org has IPv6 address 
2001:6f8:1cb3:2:216:41ff:fee1:3d40

... qemu should listen on all addresses returned.  Which in turn 
requires multiple listening sockets.

Changing this is a big deal though, thats why I've taken the somewhat 
unclean shortcut to listen on the first match only when implementing 
this.  Clients are supposed to try to connect to all addresses returned 
by the lookup (and they do if they got IPv6 support), thus this usually 
doesn't cause trouble in practice.

When going for multiple listening sockets in qemu we have to figure how 
we'll handle this in a number of places as there is no single listening 
address any more.  Reporting the vnc server address in QMP is one.  I'm 
sure there are more.

cheers,
   Gerd

On 05/04/2010 04:47 PM, Gerd Hoffmann wrote:
> On 05/04/10 18:23, Anthony Liguori wrote:
>> On 05/04/2010 08:49 AM, Reinhard Max wrote:
>>> Hi,
>>>
>>> I am maintaining the tightvnc package for openSUSE and was recently
>>> confronted with an alleged vnc problem with QWMU that turned out to be
>>> a shortcoming in QEMU's code for handling TCP server sockets, which is
>>> used by the vnc and char modules.
>>>
>>> The problem occurs when the address to listen on is given as a name
>>> which resolves to multiple IP addresses the most prominent example
>>> being "localhost" resolving to 127.0.0.1 and ::1 .
>
> My tigervnc (tightvnc successor) has IPv6 support and handles this 
> just fine.  There is also the option to force qemu to listen on ipv4 
> (or ipv6) only.
>
>>> The existing code stopped walking the list of addresses returned by
>>> getaddrinfo() as soon as one socket was successfully opened and bound.
>>> The result was that a qemu instance started with "-vnc localhost:42"
>>> only listened on ::1, wasn't reachable through 127.0.0.1. The fact
>>> that the code set the IPV6_V6ONLY socket option didn't help, because
>>> that option only works when the socket gets bound to the IPv6 wildcard
>>> address (::), but is useless for explicit address bindings.
>
> Indeed.
>
>> But that said, I'm not sure we're doing the wrong thing right now. Gerd,
>> what do you think about this behavior?
>
> Reinhard is correct.  If a hostname resolves to multiple addresses 
> like this ...
>
>    zweiblum kraxel ~# host zweiblum
>    zweiblum.home.kraxel.org has address 192.168.2.101
>    zweiblum.home.kraxel.org has IPv6 address 
> 2001:6f8:1cb3:2:216:41ff:fee1:3d40
>
> ... qemu should listen on all addresses returned.  Which in turn 
> requires multiple listening sockets.
>
> Changing this is a big deal though, thats why I've taken the somewhat 
> unclean shortcut to listen on the first match only when implementing 
> this.  Clients are supposed to try to connect to all addresses 
> returned by the lookup (and they do if they got IPv6 support), thus 
> this usually doesn't cause trouble in practice.
>
> When going for multiple listening sockets in qemu we have to figure 
> how we'll handle this in a number of places as there is no single 
> listening address any more.  Reporting the vnc server address in QMP 
> is one.  I'm sure there are more.

Okay, that makes sense.  Personally, I'm inclined to agree that this is 
a client problem.

Regards,

Anthony Liguori

> cheers,
>   Gerd
>

On Tue, 4 May 2010 at 16:50, Anthony Liguori wrote:

> Personally, I'm inclined to agree that this is a client problem.

That would be a violation of the "be liberal in what you accept and 
conservative in what you produce" principle and there are plenty of 
scenarios where even a client that Does It Right[tm] would fail, one 
example being that both ends are in networks that use public IPv6 
addresses, but have no IPv6 routing to each other (and yes, I've seen 
such networks).


cu
 	Reinhard

On Tue, 4 May 2010 at 23:47, Gerd Hoffmann wrote:

> My tigervnc (tightvnc successor) has IPv6 support and handles this 
> just fine.

Well, as I wrote, the code is not just used for vnc, but also for 
server sockets that (if I got that right) could be connected from 
telnet or netcat implementations that don't speak IPv6.

> When going for multiple listening sockets in qemu we have to figure 
> how we'll handle this in a number of places as there is no single 
> listening address any more.

Well, that's what my patch is about. Did you take a look at it?

> Reporting the vnc server address in QMP is one.

Not sure what QMP is (this was the first time I looked at QEMU's 
internals), but I think my patch only leaves one place TODO where I 
chose to report only the first address for now, but it shouldn't be 
too hard to fix that as well.

BTW, in some places I circumvented the need for reporting multiple 
addresses by simply reporting the name that was passed to QEMU 
instead.


cu
 	Reinhard

On Tue, May 04, 2010 at 03:49:50PM +0200, Reinhard Max wrote:
> Hi,
> 
> I am maintaining the tightvnc package for openSUSE and was recently 
> confronted with an alleged vnc problem with QWMU that turned out to be 
> a shortcoming in QEMU's code for handling TCP server sockets, which is 
> used by the vnc and char modules.
> 
> The problem occurs when the address to listen on is given as a name 
> which resolves to multiple IP addresses the most prominent example 
> being "localhost" resolving to 127.0.0.1 and ::1 .
> 
> The existing code stopped walking the list of addresses returned by 
> getaddrinfo() as soon as one socket was successfully opened and bound. 
> The result was that a qemu instance started with "-vnc localhost:42" 
> only listened on ::1, wasn't reachable through 127.0.0.1. The fact 
> that the code set the IPV6_V6ONLY socket option didn't help, because 
> that option only works when the socket gets bound to the IPv6 wildcard 
> address (::), but is useless for explicit address bindings.

This seems to be something we overlooked in the initial impl. I don't
see any alternative but to make QEMU listen on multiple sockets in
the scenario you describe. An even clear example is to consider binding
QEMU to a machine with multiple non-localhost addresses, eg

   myserver.com resolving to 192.168.122.41 + 2a00:123:456::1

because there's no way that the kernel can know/decide to automatically
listen on 192.168.122.41, when given the address 2a00:123:456::1 and if
the machine has many NICs, you can't assume the wildcard address is 
suitable either.

> The attached patch against QEMU 0.11.0 extends inet_listen() to create 
> sockets for as many addresses from the address list as possible and 
> adapts its callers and their data structures to deal with a linked 
> list of socket FDs rather than a single file descriptor.

The approach looks reasonable, though the patch is somewhat mangled
by the mix of tabs + spaces

Regards,
Daniel

Hi,

>> When going for multiple listening sockets in qemu we have to figure
>> how we'll handle this in a number of places as there is no single
>> listening address any more.
>
> Well, that's what my patch is about.

Sure.

> Did you take a look at it?

Briefly, yes.  Overall it looks sensible to me.  Devil is in the details 
though, see below.

Noticed that it probably should get a few helper functions to handle 
FdLists to avoid the quite simliar open-coded loop-over-all-fds loops 
all over the place.

>> Reporting the vnc server address in QMP is one.
>
> Not sure what QMP is (this was the first time I looked at QEMU's
> internals),

You'll run into qmp for sure when forward-porting the patches to the 
latest qemu bits.  It is the machine-readable version of the monitor 
protocol (in qemu 0.12+).

> but I think my patch only leaves one place TODO where I
> chose to report only the first address for now, but it shouldn't be too
> hard to fix that as well.

Yea.  I've noticed that TODO ;)

> BTW, in some places I circumvented the need for reporting multiple
> addresses by simply reporting the name that was passed to QEMU instead.

This is one of the issues which needs to be addressed somehow.

First I think qemu should be self-consistent here, i.e. either report 
the (single) name or the list of addressed everythere.

Second we have to care about the current users (especially libvirt). 
Today qemu usually reports the address I think.  Thus I tend to stick to 
addresses to keep them happy.

We'll have a externally visible change in any case though.  Either the 
switch from the address to the name or the switch from a single address 
to a list of addresses.  Both changes might break existing users.

cheers,
   Gerd

Hi,

On Wed, 5 May 2010 at 10:53, Gerd Hoffmann wrote:

> Noticed that it probably should get a few helper functions to handle 
> FdLists to avoid the quite simliar open-coded loop-over-all-fds 
> loops all over the place.

indeed, thanks for the hint. I now have functions to create a new list 
element from an fd number and to destroy a list. Not sure if straight 
loops over existing lists can be further optimized, though.

> You'll run into qmp for sure when forward-porting the patches to the 
> latest qemu bits.  It is the machine-readable version of the monitor 
> protocol (in qemu 0.12+).

I guess that's the qemu_opt_set() calls at the end of 
inet_listen_opts()?

> First I think qemu should be self-consistent here, i.e. either 
> report the (single) name or the list of addressed everythere.

Yes, this mixture wasn't meant to be final, but it helped me getting 
the initial patch done with a minimal set of changes.

> Second we have to care about the current users (especially libvirt).

Wouldn't the users of that bit of information run it through 
getaddrinfo() anyways when trying to connect? So to them it shouldn't 
matter whether the name or an ASCII representation of the address is 
used.

> Today qemu usually reports the address I think.  Thus I tend to 
> stick to addresses to keep them happy.

But wouldn't going from single address to multiple addresses be a 
bigger change for the users (and likely break them all) while going 
from address to name would only break those that were not using 
getaddrinfo() to translate the address into its binary representation.

OTOH, going for multiple addresses would also allow starting qemu with 
more than a single -vnc option, which doesn't seem to be possible 
right now, and wich might come handy in situations when the set of 
addresses a qemu instance should be listening on is not available as a 
single DNS name.


cu
 	Reinhard

Hi,

>> You'll run into qmp for sure when forward-porting the patches to the
>> latest qemu bits. It is the machine-readable version of the monitor
>> protocol (in qemu 0.12+).
>
> I guess that's the qemu_opt_set() calls at the end of inet_listen_opts()?

See docs in QMP/*, the changes in monitor.c and q${type}.[ch]

qemu_opt_set() in inet_listen_opts() is only slightly related.  It is 
used to report back the address we've actually bound to.  Used by 'info 
chardev' and I think vnc too.  Yes, that has to be changed somehow ...

>> Second we have to care about the current users (especially libvirt).
>
> Wouldn't the users of that bit of information run it through
> getaddrinfo() anyways when trying to connect? So to them it shouldn't
> matter whether the name or an ASCII representation of the address is used.

I don't know how it is used.

>> Today qemu usually reports the address I think. Thus I tend to stick
>> to addresses to keep them happy.
>
> But wouldn't going from single address to multiple addresses be a bigger
> change for the users (and likely break them all) while going from
> address to name would only break those that were not using getaddrinfo()
> to translate the address into its binary representation.

It is probably best to bring this up on the libvirt list, this is the
most important user of those interfaces and I think other virtual machine
management folks are reading there too.

I personally don't care too much which way we pick.

> OTOH, going for multiple addresses would also allow starting qemu with
> more than a single -vnc option, which doesn't seem to be possible right
> now, and wich might come handy in situations when the set of addresses a
> qemu instance should be listening on is not available as a single DNS name.

Good point.

cheers,
   Gerd

On Tue, May 04, 2010 at 04:50:34PM -0500, Anthony Liguori wrote:
> On 05/04/2010 04:47 PM, Gerd Hoffmann wrote:
> >On 05/04/10 18:23, Anthony Liguori wrote:
> >>On 05/04/2010 08:49 AM, Reinhard Max wrote:
> >>>Hi,
> >>>
> >>>I am maintaining the tightvnc package for openSUSE and was recently
> >>>confronted with an alleged vnc problem with QWMU that turned out to be
> >>>a shortcoming in QEMU's code for handling TCP server sockets, which is
> >>>used by the vnc and char modules.
> >>>
> >>>The problem occurs when the address to listen on is given as a name
> >>>which resolves to multiple IP addresses the most prominent example
> >>>being "localhost" resolving to 127.0.0.1 and ::1 .
> >
> >My tigervnc (tightvnc successor) has IPv6 support and handles this 
> >just fine.  There is also the option to force qemu to listen on ipv4 
> >(or ipv6) only.
> >
> >>>The existing code stopped walking the list of addresses returned by
> >>>getaddrinfo() as soon as one socket was successfully opened and bound.
> >>>The result was that a qemu instance started with "-vnc localhost:42"
> >>>only listened on ::1, wasn't reachable through 127.0.0.1. The fact
> >>>that the code set the IPV6_V6ONLY socket option didn't help, because
> >>>that option only works when the socket gets bound to the IPv6 wildcard
> >>>address (::), but is useless for explicit address bindings.
> >
> >Indeed.
> >
> >>But that said, I'm not sure we're doing the wrong thing right now. Gerd,
> >>what do you think about this behavior?
> >
> >Reinhard is correct.  If a hostname resolves to multiple addresses 
> >like this ...
> >
> >   zweiblum kraxel ~# host zweiblum
> >   zweiblum.home.kraxel.org has address 192.168.2.101
> >   zweiblum.home.kraxel.org has IPv6 address 
> >2001:6f8:1cb3:2:216:41ff:fee1:3d40
> >
> >... qemu should listen on all addresses returned.  Which in turn 
> >requires multiple listening sockets.
> >
> >Changing this is a big deal though, thats why I've taken the somewhat 
> >unclean shortcut to listen on the first match only when implementing 
> >this.  Clients are supposed to try to connect to all addresses 
> >returned by the lookup (and they do if they got IPv6 support), thus 
> >this usually doesn't cause trouble in practice.
> >
> >When going for multiple listening sockets in qemu we have to figure 
> >how we'll handle this in a number of places as there is no single 
> >listening address any more.  Reporting the vnc server address in QMP 
> >is one.  I'm sure there are more.
> 
> Okay, that makes sense.  Personally, I'm inclined to agree that this is 
> a client problem.

It isn't a client problem if QEMU is not listening on all the required
addresses. In Gerd's case 

> >   zweiblum.home.kraxel.org has address 192.168.2.101
> >   zweiblum.home.kraxel.org has IPv6 address
> >2001:6f8:1cb3:2:216:41ff:fee1:3d40

If QEMU only listens on 2001:6f8:1cb3:2:216:41ff:fee1:3d40, and a VNC
client that only knows IPv4 tries to connect it will fail. There's nothing
the client can do to solve this. QEMU has to be listening on all addresses 
associated with a name for the dual-stack setup to provide correct fallback 
for clients.


Daniel

On Wed, May 05, 2010 at 10:53:00AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> >>When going for multiple listening sockets in qemu we have to figure
> >>how we'll handle this in a number of places as there is no single
> >>listening address any more.
> >
> >Well, that's what my patch is about.
> 
> Sure.
> 
> >Did you take a look at it?
> 
> Briefly, yes.  Overall it looks sensible to me.  Devil is in the details 
> though, see below.
> 
> Noticed that it probably should get a few helper functions to handle 
> FdLists to avoid the quite simliar open-coded loop-over-all-fds loops 
> all over the place.
> 
> >>Reporting the vnc server address in QMP is one.
> >
> >Not sure what QMP is (this was the first time I looked at QEMU's
> >internals),
> 
> You'll run into qmp for sure when forward-porting the patches to the 
> latest qemu bits.  It is the machine-readable version of the monitor 
> protocol (in qemu 0.12+).
> 
> >but I think my patch only leaves one place TODO where I
> >chose to report only the first address for now, but it shouldn't be too
> >hard to fix that as well.
> 
> Yea.  I've noticed that TODO ;)
> 
> >BTW, in some places I circumvented the need for reporting multiple
> >addresses by simply reporting the name that was passed to QEMU instead.
> 
> This is one of the issues which needs to be addressed somehow.
> 
> First I think qemu should be self-consistent here, i.e. either report 
> the (single) name or the list of addressed everythere.
> 
> Second we have to care about the current users (especially libvirt). 
> Today qemu usually reports the address I think.  Thus I tend to stick to 
> addresses to keep them happy.

From a libvirt POV the only place we use the address is in the graphics
notification event(s). For the event we know which specific address out 
of the list to report - the one we just did accept() on. libvirt does not 
use the 'info vnc' or 'query-vnc' output at all, so whether that reports 
IP address of hostname doesn't matter to us. It is probably easiest to 
just make it report the same info that was provided on the CLI to -vnc 


Regards,
Daniel