[libvirt] [PATCH v1] 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 addreses 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 the appropriate error. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- 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..0ee5e3604f 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; }
$SUBJ: util: Fix addres On 05/15/2018 04:20 AM, 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
which produces what error?
- the local interfaces have that one IPv4 address assigned, but not any of the IPv6 addresses
same question
- the local interfaces have just IPv6 link-local addresses
same question
In this case the resolver returns not only the IPv4 addreses but also
s/addreses/addresses
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 the appropriate error.
A bit tough to read for this English as my first language, but I think it can be word-smithed a bit... once I understand more about what's being done.
Signed-off-by: Olaf Hering <olaf@aepfle.de> --- src/rpc/virnetsocket.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
git am on the patch produces: Applying: Loop through all resolved addresses in virNetSocketNewListenTCP .git/rebase-apply/patch:40: trailing whitespace. else warning: 1 line adds whitespace errors. which means syntax-check fails: 4.51 prohibit_empty_first_line src/rpc/virnetsocket.c:414: else maint.mk: found trailing blank(s) 4.51 prohibit_fork_wrappers make: *** [maint.mk:750: sc_trailing_blank] Error 1 make: *** Waiting for unfinished jobs.... but it also fails for: GEN public-submodule-commit GEN spacing-check No whitespace after keyword: src/rpc/virnetsocket.c:412: else if(addrInUse) maint.mk: incorrect formatting make: *** [cfg.mk:1117: spacing-check] Error 1 Before posting please be sure that: make check syntax-check passes.
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 7087abec9c..0ee5e3604f 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;
So this to me reads as if we're just ignoring all errors, but we do care if the address was in use already and we'll save that bit of information for later. Does that mean there is some "other" sub class of errors for which it's desired to retry? Is there a finite list? It doesn't imply to me what was stated in the commit message, but I'm not deep in the throes of debugging a problem here either!
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;
bind says this is returned when "The address argument is a null pointer." - so essentially we're making up a failure?
+ virReportSystemError(errno, "%s", _("Unable to bind to port"));
This is all not formatted properly Indents should be 4 spaces and this last virReportError probably shouldn't be indented. Please rework and repost. Tks, John
goto error; }
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Am Wed, 16 May 2018 18:44:32 -0400 schrieb John Ferlan <jferlan@redhat.com>:
On 05/15/2018 04:20 AM, 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 which produces what error? IPv6. Binding the IPv6 address will obviously fail. But this terminates the entire loop, even if binding to IPv4 succeeded.
What happens if a hostname resolves to more than one address, but none of the interfaces has some of the addresses assigned? bind() will fail. Just try it yourself, remove one address with 'ip addr del ...' (whatever the required syntax is) and do a migration to that host. For me it happens with BOOTPROTO='dhcp4' instead of 'dhcp' in ifcfg-br0.
@@ -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;
So this to me reads as if we're just ignoring all errors, but we do care if the address was in use already and we'll save that bit of information for later.
Why would any error matter here, beside the two that this function actually handles in some way? It seems to me the intent of that function is to bind to at least one of the addresses for the hostname passed to this function. There is no point in failing the entire operation just because there are more addresses in the resolved list. A client that actually tries to connect to that hostname will also cycle through all addresses until one succeeds.
bind says this is returned when "The address argument is a null pointer." - so essentially we're making up a failure?
Yes. There was no success trying to bind() to any of the resolved addresses. There are a few places that can lead to nsocks==0, not just failures of bind(). Olaf
On 05/17/2018 02:14 AM, Olaf Hering wrote:
Am Wed, 16 May 2018 18:44:32 -0400 schrieb John Ferlan <jferlan@redhat.com>:
On 05/15/2018 04:20 AM, 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 which produces what error? IPv6. Binding the IPv6 address will obviously fail. But this terminates the entire loop, even if binding to IPv4 succeeded.
What happens if a hostname resolves to more than one address, but none of the interfaces has some of the addresses assigned? bind() will fail.
Just try it yourself, remove one address with 'ip addr del ...' (whatever the required syntax is) and do a migration to that host.
For me it happens with BOOTPROTO='dhcp4' instead of 'dhcp' in ifcfg-br0.
Sorry, I guess it just wasn't clear enough to me what the issue was and the proposed resolution. I wasn't in the throes of debugging...
@@ -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;
So this to me reads as if we're just ignoring all errors, but we do care if the address was in use already and we'll save that bit of information for later.
Why would any error matter here, beside the two that this function actually handles in some way? It seems to me the intent of that function is to bind to at least one of the addresses for the hostname passed to this function. There is no point in failing the entire operation just because there are more addresses in the resolved list. A client that actually tries to connect to that hostname will also cycle through all addresses until one succeeds.
If there was some other kind of configuration issue we theoretically could make a few bind calls that are failing in exactly the same way and perhaps that error is more fatal than others. There's lots of reasons for bind to fail and lots of different settings for errno. I figured you had been debugging and would know there were a few that were being seeing and those could be filtered like EADDRINUSE currently is. I wasn't in the middle of debugging this so I couldn't picture why we'd want to go from filtering one error to filtering all errors. Maybe we should VIR_DEBUG the errno for debugging purposes since you'll possibly change it later to EDESTADDRREQ.
bind says this is returned when "The address argument is a null pointer." - so essentially we're making up a failure?
Yes. There was no success trying to bind() to any of the resolved addresses. There are a few places that can lead to nsocks==0, not just failures of bind().
Fair enough - almost makes me wonder if we craft a different error message when nsocks == 0 and familyNotSupport or addrInUse is not set. John
participants (2)
-
John Ferlan -
Olaf Hering