[libvirt] [PATCH v3] Loop through all resolved addresses in virNetSocketNewListenTCP
Currently virNetSocketNewListenTCP bails out early under the following conditions: - the hostname resolves to at least one IPv4 and at least one IPv6 address - the local interfaces have that one IPv4 address assigned, but not any of the IPv6 addresses - the local interfaces have just IPv6 link-local addresses In this case the resolver returns not only the IPv4 addresses but also IPv6. Binding the IPv6 address will obviously fail. But this terminates the entire loop, even if binding to IPv4 succeeded. To fix this error, just keep going and loop through all returned addresses. In case none of the attempts to bind to some address succeeded, try to report some appropriate error. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- v3: more whitespace fixes, as suggested by Daniel P. Berrang�� v2: whitespace fixes, as suggested by John Ferlan src/rpc/virnetsocket.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 7087abec9c..3a055acd39 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -382,11 +382,8 @@ int virNetSocketNewListenTCP(const char *nodename, #endif if (bind(fd, runp->ai_addr, runp->ai_addrlen) < 0) { - if (errno != EADDRINUSE) { - virReportSystemError(errno, "%s", _("Unable to bind to port")); - goto error; - } - addrInUse = true; + if (errno == EADDRINUSE) + addrInUse = true; VIR_FORCE_CLOSE(fd); runp = runp->ai_next; continue; @@ -409,14 +406,14 @@ int virNetSocketNewListenTCP(const char *nodename, fd = -1; } - if (nsocks == 0 && familyNotSupported) { - virReportSystemError(EAFNOSUPPORT, "%s", _("Unable to bind to port")); - goto error; - } - - if (nsocks == 0 && - addrInUse) { - virReportSystemError(EADDRINUSE, "%s", _("Unable to bind to port")); + if (nsocks == 0) { + if (familyNotSupported) + errno = EAFNOSUPPORT; + else if (addrInUse) + errno = EADDRINUSE; + else + errno = EDESTADDRREQ; + virReportSystemError(errno, "%s", _("Unable to bind to port")); goto error; }
On Mon, Jun 04, 2018 at 12:29:37PM +0200, Olaf Hering wrote:
Currently virNetSocketNewListenTCP bails out early under the following conditions: - the hostname resolves to at least one IPv4 and at least one IPv6 address - the local interfaces have that one IPv4 address assigned, but not any of the IPv6 addresses - the local interfaces have just IPv6 link-local addresses
In this case the resolver returns not only the IPv4 addresses but also IPv6. Binding the IPv6 address will obviously fail. But this terminates the entire loop, even if binding to IPv4 succeeded.
To fix this error, just keep going and loop through all returned addresses. In case none of the attempts to bind to some address succeeded, try to report some appropriate error.
Signed-off-by: Olaf Hering <olaf@aepfle.de> ---
v3: more whitespace fixes, as suggested by Daniel P. Berrangé v2: whitespace fixes, as suggested by John Ferlan
src/rpc/virnetsocket.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 7087abec9c..3a055acd39 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -382,11 +382,8 @@ int virNetSocketNewListenTCP(const char *nodename, #endif
if (bind(fd, runp->ai_addr, runp->ai_addrlen) < 0) { - if (errno != EADDRINUSE) { - virReportSystemError(errno, "%s", _("Unable to bind to port")); - goto error; - } - addrInUse = true; + if (errno == EADDRINUSE) + addrInUse = true;
This skips all errors, not just EADDRNOTAVAIL. Saving the errno here...
VIR_FORCE_CLOSE(fd); runp = runp->ai_next; continue; @@ -409,14 +406,14 @@ int virNetSocketNewListenTCP(const char *nodename, fd = -1; }
- if (nsocks == 0 && familyNotSupported) { - virReportSystemError(EAFNOSUPPORT, "%s", _("Unable to bind to port")); - goto error; - } - - if (nsocks == 0 && - addrInUse) { - virReportSystemError(EADDRINUSE, "%s", _("Unable to bind to port")); + if (nsocks == 0) { + if (familyNotSupported) + errno = EAFNOSUPPORT; + else if (addrInUse) + errno = EADDRINUSE; + else + errno = EDESTADDRREQ;
...and using it here if needed would feel less random than picking one at random. Also, why the use of global errno variable to pass a value within a single block? Jano
Am Mon, 4 Jun 2018 21:16:17 +0200 schrieb Ján Tomko <jtomko@redhat.com>:
On Mon, Jun 04, 2018 at 12:29:37PM +0200, Olaf Hering wrote:
- goto error; This skips all errors, not just EADDRNOTAVAIL. Saving the errno here...
Why would any error matter here? Why was that one 'goto error;' ever correct? There is more cake on the table, no need to stop here.
+ if (nsocks == 0) { Also, why the use of global errno variable to pass a value within a single block?
I have no idea, likely to have a common error available outside the loop. Another cleanup for pretty error handling could be done. If anyone really feels it is needed. Olaf
Am Mon, 4 Jun 2018 12:29:37 +0200 schrieb Olaf Hering <olaf@aepfle.de>:
Currently virNetSocketNewListenTCP bails out early under the following conditions: - the hostname resolves to at least one IPv4 and at least one IPv6 address - the local interfaces have that one IPv4 address assigned, but not any of the IPv6 addresses - the local interfaces have just IPv6 link-local addresses
Ping? Olaf
On Mon, Jun 04, 2018 at 12:29:37PM +0200, Olaf Hering wrote:
Currently virNetSocketNewListenTCP bails out early under the following conditions: - the hostname resolves to at least one IPv4 and at least one IPv6 address - the local interfaces have that one IPv4 address assigned, but not any of the IPv6 addresses - the local interfaces have just IPv6 link-local addresses
In this case the resolver returns not only the IPv4 addresses but also IPv6. Binding the IPv6 address will obviously fail. But this terminates the entire loop, even if binding to IPv4 succeeded.
Could you elaborate on this a bit more, as on reflection I'm not sure I understand the flaw here. Could you show the 'ip addr' output and corresponding hostname resolution results, and say what errno you are seeing from bind().
To fix this error, just keep going and loop through all returned addresses. In case none of the attempts to bind to some address succeeded, try to report some appropriate error.
Signed-off-by: Olaf Hering <olaf@aepfle.de> ---
v3: more whitespace fixes, as suggested by Daniel P. Berrangé v2: whitespace fixes, as suggested by John Ferlan
src/rpc/virnetsocket.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 7087abec9c..3a055acd39 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -382,11 +382,8 @@ int virNetSocketNewListenTCP(const char *nodename, #endif
if (bind(fd, runp->ai_addr, runp->ai_addrlen) < 0) { - if (errno != EADDRINUSE) { - virReportSystemError(errno, "%s", _("Unable to bind to port")); - goto error; - } - addrInUse = true; + if (errno == EADDRINUSE) + addrInUse = true; VIR_FORCE_CLOSE(fd); runp = runp->ai_next; continue; @@ -409,14 +406,14 @@ int virNetSocketNewListenTCP(const char *nodename, fd = -1; }
- if (nsocks == 0 && familyNotSupported) { - virReportSystemError(EAFNOSUPPORT, "%s", _("Unable to bind to port")); - goto error; - } - - if (nsocks == 0 && - addrInUse) { - virReportSystemError(EADDRINUSE, "%s", _("Unable to bind to port")); + if (nsocks == 0) { + if (familyNotSupported) + errno = EAFNOSUPPORT; + else if (addrInUse) + errno = EADDRINUSE; + else + errno = EDESTADDRREQ;
This EDESTADDRREQ feels a bit odd to use - "Destination address required" doesn't make much sense for something that has to be a local address.
+ virReportSystemError(errno, "%s", _("Unable to bind to port")); goto error; }
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Am Wed, 4 Jul 2018 11:57:58 +0100 schrieb Daniel P. Berrangé <berrange@redhat.com>:
On Mon, Jun 04, 2018 at 12:29:37PM +0200, Olaf Hering wrote:
In this case the resolver returns not only the IPv4 addresses but also IPv6. Binding the IPv6 address will obviously fail. But this terminates the entire loop, even if binding to IPv4 succeeded. Could you elaborate on this a bit more, as on reflection I'm not sure I understand the flaw here. Could you show the 'ip addr' output and corresponding hostname resolution results, and say what errno you are seeing from bind().
Not sure what is unclear about this statement. The hostname resolves to ipv4+ipv6. Binding to ipv4 works because this address is available on the interface. Binding to ipv6 fails because this address not not available on any interface. Since bind() already succeeded (or will succeed) for one address, there is no reason to error out. Anyone who connects to an resolved ipv6 address will get an error, and moves on to try the resolved ipv4 address. libvirt is overdoing things here.
This EDESTADDRREQ feels a bit odd to use - "Destination address required" doesn't make much sense for something that has to be a local address.
Maybe that depends on the context. The whole function does not try hard to preserve all possible errors for the nsocks==0 case. This specific new case just assumes that bind() was the thing that resulted in nsocks==0. There are other error paths. What errno value do you suggest for nsocks=0? Olaf
On Wed, Jul 04, 2018 at 01:48:56PM +0200, Olaf Hering wrote:
Am Wed, 4 Jul 2018 11:57:58 +0100 schrieb Daniel P. Berrangé <berrange@redhat.com>:
On Mon, Jun 04, 2018 at 12:29:37PM +0200, Olaf Hering wrote:
In this case the resolver returns not only the IPv4 addresses but also IPv6. Binding the IPv6 address will obviously fail. But this terminates the entire loop, even if binding to IPv4 succeeded. Could you elaborate on this a bit more, as on reflection I'm not sure I understand the flaw here. Could you show the 'ip addr' output and corresponding hostname resolution results, and say what errno you are seeing from bind().
Not sure what is unclear about this statement. The hostname resolves to ipv4+ipv6. Binding to ipv4 works because this address is available on the interface. Binding to ipv6 fails because this address not not available on any interface.
So you've configured an IPv6 address in the hostname that doesn't actually exist at all on any interface ? In the text you cut out in your reply it was saying you have NICs with IPv4 and NICs with IPv6 link local, so it sounded like this problem was somehow related to multiple NICs.
Since bind() already succeeded (or will succeed) for one address, there is no reason to error out. Anyone who connects to an resolved ipv6 address will get an error, and moves on to try the resolved ipv4 address. libvirt is overdoing things here.
Many of the errnos reported by bind() indicate problems we should not be ignoring IMHO. So I'd rather handle the actual errnos that indicate non-fatal problems.
This EDESTADDRREQ feels a bit odd to use - "Destination address required" doesn't make much sense for something that has to be a local address.
Maybe that depends on the context. The whole function does not try hard to preserve all possible errors for the nsocks==0 case. This specific new case just assumes that bind() was the thing that resulted in nsocks==0. There are other error paths.
What errno value do you suggest for nsocks=0?
Well what errno are you getting from bind() when it fails ? It sounds like it might be EADDRNOTAVAIL. Preserving that looks like best option to me, rather than arbitrarily picking a different/fixed errno. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, Jul 04, Daniel P. Berrangé wrote:
On Wed, Jul 04, 2018 at 01:48:56PM +0200, Olaf Hering wrote:
Since bind() already succeeded (or will succeed) for one address, there is no reason to error out. Anyone who connects to an resolved ipv6 address will get an error, and moves on to try the resolved ipv4 address. libvirt is overdoing things here. Many of the errnos reported by bind() indicate problems we should not be ignoring IMHO. So I'd rather handle the actual errnos that indicate non-fatal problems.
So you prefer to keep the existing behavior that all errors from bind() are fatal, even if nsocks!=0? What is the reason for that behavior? So far noone was willing to answer that question. Olaf
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Olaf Hering