src/rpc/virnetsocket.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)
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;
}
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-listOn 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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.