10:40 a.m.
This goes on top of:
[PATCH v2 0/2] Report an error in virPortAllocatorAcquire if all ports are used
https://www.redhat.com/archives/libvir-list/2013-October/msg01289.html
v1: Support IPv6 in port allocator
https://www.redhat.com/archives/libvir-list/2013-October/msg00007.html
Ján Tomko (2):
Split out bind() from virPortAllocatorAcquire
Support IPv6 in port allocator
src/util/virportallocator.c | 107 ++++++++++++++++++++++++++++++++------------
1 file changed, 78 insertions(+), 29 deletions(-)
--
1.8.1.5
10:40 a.m.
New subject: [libvirt] [PATCH 1/2] Split out bind() from virPortAllocatorAcquire
---
src/util/virportallocator.c | 72 ++++++++++++++++++++++++++++-----------------
1 file changed, 45 insertions(+), 27 deletions(-)
diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c
index 42e1f28..1922ea6 100644
--- a/src/util/virportallocator.c
+++ b/src/util/virportallocator.c
@@ -98,19 +98,59 @@ virPortAllocatorPtr virPortAllocatorNew(const char *name,
return pa;
}
+static int virPortAllocatorBindToPort(bool *used,
+ unsigned short port)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_port = htons(port),
+ .sin_addr.s_addr = htonl(INADDR_ANY)
+ };
+ int reuse = 1;
+ int ret = -1;
+ int fd = -1;
+
+ *used = false;
+
+ fd = socket(PF_INET, SOCK_STREAM, 0);
+ if (fd < 0) {
+ virReportSystemError(errno, "%s", _("Unable to open test
socket"));
+ goto cleanup;
+ }
+
+ if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (void*)&reuse,
+ sizeof(reuse)) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to set socket reuse addr flag"));
+ goto cleanup;
+ }
+
+ if (bind(fd, (struct sockaddr*)&addr, sizeof(addr)) < 0) {
+ if (errno == EADDRINUSE) {
+ *used = true;
+ ret = 0;
+ } else {
+ virReportSystemError(errno, _("Unable to bind to port %d"), port);
+ }
+ goto cleanup;
+ }
+
+ ret = 0;
+cleanup:
+ VIR_FORCE_CLOSE(fd);
+ return ret;
+}
+
int virPortAllocatorAcquire(virPortAllocatorPtr pa,
unsigned short *port)
{
int ret = -1;
size_t i;
- int fd = -1;
*port = 0;
virObjectLock(pa);
for (i = pa->start; i <= pa->end && !*port; i++) {
- int reuse = 1;
- struct sockaddr_in addr;
bool used = false;
if (virBitmapGetBit(pa->bitmap,
@@ -123,31 +163,10 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa,
if (used)
continue;
- addr.sin_family = AF_INET;
- addr.sin_port = htons(i);
- addr.sin_addr.s_addr = htonl(INADDR_ANY);
- fd = socket(PF_INET, SOCK_STREAM, 0);
- if (fd < 0) {
- virReportSystemError(errno, "%s",
- _("Unable to open test socket"));
+ if (virPortAllocatorBindToPort(&used, i) < 0)
goto cleanup;
- }
-
- if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, (void*)&reuse, sizeof(reuse))
< 0) {
- virReportSystemError(errno, "%s",
- _("Unable to set socket reuse addr flag"));
- goto cleanup;
- }
- if (bind(fd, (struct sockaddr*)&addr, sizeof(addr)) < 0) {
- if (errno != EADDRINUSE) {
- virReportSystemError(errno,
- _("Unable to bind to port %zu"), i);
- goto cleanup;
- }
- /* In use, try next */
- VIR_FORCE_CLOSE(fd);
- } else {
+ if (!used) {
/* Add port to bitmap of reserved ports */
if (virBitmapSetBit(pa->bitmap,
i - pa->start) < 0) {
@@ -167,7 +186,6 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa,
}
cleanup:
virObjectUnlock(pa);
- VIR_FORCE_CLOSE(fd);
return ret;
}
--
1.8.1.5
12:29 p.m.
New subject: [libvirt] [PATCH 1/2] Split out bind() from virPortAllocatorAcquire
On 10/31/2013 09:40 AM, Ján Tomko wrote:
---
src/util/virportallocator.c | 72 ++++++++++++++++++++++++++++-----------------
1 file changed, 45 insertions(+), 27 deletions(-)
Looks larger, but makes later patches possible. ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:40 a.m.
New subject: [libvirt] [PATCH 2/2] Support IPv6 in port allocator
Also try to bind on IPv6 to check if the port is occupied.
https://bugzilla.redhat.com/show_bug.cgi?id=1025407
---
src/util/virportallocator.c | 45 ++++++++++++++++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 7 deletions(-)
diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c
index 1922ea6..24b4bff 100644
--- a/src/util/virportallocator.c
+++ b/src/util/virportallocator.c
@@ -99,21 +99,45 @@ virPortAllocatorPtr virPortAllocatorNew(const char *name,
}
static int virPortAllocatorBindToPort(bool *used,
- unsigned short port)
+ unsigned short port,
+ int family)
{
- struct sockaddr_in addr = {
+ struct sockaddr_in6 addr6 = {
+ .sin6_family = AF_INET6,
+ .sin6_port = htons(port),
+ .sin6_addr = IN6ADDR_ANY_INIT,
+ };
+ struct sockaddr_in addr4 = {
.sin_family = AF_INET,
.sin_port = htons(port),
.sin_addr.s_addr = htonl(INADDR_ANY)
};
+ struct sockaddr* addr;
+ size_t addrlen;
+ int v6only = 1;
int reuse = 1;
int ret = -1;
int fd = -1;
+ bool ipv6 = false;
+
+ if (family == AF_INET6) {
+ addr = (struct sockaddr*)&addr6;
+ addrlen = sizeof(addr6);
+ ipv6 = true;
+ } else if (family == AF_INET) {
+ addr = (struct sockaddr*)&addr4;
+ addrlen = sizeof(addr4);
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown family %d"),
family);
+ return -1;
+ }
*used = false;
fd = socket(PF_INET, SOCK_STREAM, 0);
if (fd < 0) {
+ if (errno == EAFNOSUPPORT)
+ return 0;
virReportSystemError(errno, "%s", _("Unable to open test
socket"));
goto cleanup;
}
@@ -125,7 +149,14 @@ static int virPortAllocatorBindToPort(bool *used,
goto cleanup;
}
- if (bind(fd, (struct sockaddr*)&addr, sizeof(addr)) < 0) {
+ if (ipv6 && setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, (void*)&v6only,
+ sizeof(v6only)) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to set IPV6_V6ONLY flag"));
+ goto cleanup;
+ }
+
+ if (bind(fd, addr, addrlen) < 0) {
if (errno == EADDRINUSE) {
*used = true;
ret = 0;
@@ -151,7 +182,7 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa,
virObjectLock(pa);
for (i = pa->start; i <= pa->end && !*port; i++) {
- bool used = false;
+ bool used = false, v6used = false;
if (virBitmapGetBit(pa->bitmap,
i - pa->start, &used) < 0) {
@@ -163,10 +194,10 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa,
if (used)
continue;
- if (virPortAllocatorBindToPort(&used, i) < 0)
+ if (virPortAllocatorBindToPort(&v6used, i, AF_INET6) < 0 ||
+ virPortAllocatorBindToPort(&used, i, AF_INET) < 0)
goto cleanup;
-
- if (!used) {
+ if (!used && !v6used) {
/* Add port to bitmap of reserved ports */
if (virBitmapSetBit(pa->bitmap,
i - pa->start) < 0) {
--
1.8.1.5
12:33 p.m.
New subject: [libvirt] [PATCH 2/2] Support IPv6 in port allocator
On Thu, Oct 31, 2013 at 04:40:11PM +0100, Ján Tomko wrote:
Also try to bind on IPv6 to check if the port is occupied.
https://bugzilla.redhat.com/show_bug.cgi?id=1025407
---
src/util/virportallocator.c | 45 ++++++++++++++++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 7 deletions(-)
Have you tested this on a system where the IPv6 kernel module is
not present, and on a system with IPv6 kmod is present, but no
interfaces have any IPv6 addresses (not even link-local ones).
I'm concerned this is going to trigger failures on IPv4 only
hosts.
diff --git a/src/util/virportallocator.c
b/src/util/virportallocator.c
index 1922ea6..24b4bff 100644
--- a/src/util/virportallocator.c
+++ b/src/util/virportallocator.c
@@ -99,21 +99,45 @@ virPortAllocatorPtr virPortAllocatorNew(const char *name,
}
static int virPortAllocatorBindToPort(bool *used,
- unsigned short port)
+ unsigned short port,
+ int family)
{
- struct sockaddr_in addr = {
+ struct sockaddr_in6 addr6 = {
+ .sin6_family = AF_INET6,
+ .sin6_port = htons(port),
+ .sin6_addr = IN6ADDR_ANY_INIT,
+ };
+ struct sockaddr_in addr4 = {
.sin_family = AF_INET,
.sin_port = htons(port),
.sin_addr.s_addr = htonl(INADDR_ANY)
};
+ struct sockaddr* addr;
+ size_t addrlen;
+ int v6only = 1;
int reuse = 1;
int ret = -1;
int fd = -1;
+ bool ipv6 = false;
+
+ if (family == AF_INET6) {
+ addr = (struct sockaddr*)&addr6;
+ addrlen = sizeof(addr6);
+ ipv6 = true;
+ } else if (family == AF_INET) {
+ addr = (struct sockaddr*)&addr4;
+ addrlen = sizeof(addr4);
family can be AF_INET or AF_INET6 here...
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown family %d"),
family);
+ return -1;
+ }
*used = false;
fd = socket(PF_INET, SOCK_STREAM, 0);
...but you're hardcoding PF_INET here. I'm not sure it is valid
to use an PF_INET socket with an AF_INET6 address in bind().
if (fd < 0) {
+ if (errno == EAFNOSUPPORT)
+ return 0;
virReportSystemError(errno, "%s", _("Unable to open test
socket"));
goto cleanup;
}
@@ -125,7 +149,14 @@ static int virPortAllocatorBindToPort(bool *used,
goto cleanup;
}
- if (bind(fd, (struct sockaddr*)&addr, sizeof(addr)) < 0) {
+ if (ipv6 && setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, (void*)&v6only,
+ sizeof(v6only)) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to set IPV6_V6ONLY flag"));
+ goto cleanup;
+ }
+
+ if (bind(fd, addr, addrlen) < 0) {
if (errno == EADDRINUSE) {
*used = true;
ret = 0;
@@ -151,7 +182,7 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa,
virObjectLock(pa);
for (i = pa->start; i <= pa->end && !*port; i++) {
- bool used = false;
+ bool used = false, v6used = false;
if (virBitmapGetBit(pa->bitmap,
i - pa->start, &used) < 0) {
@@ -163,10 +194,10 @@ int virPortAllocatorAcquire(virPortAllocatorPtr pa,
if (used)
continue;
- if (virPortAllocatorBindToPort(&used, i) < 0)
+ if (virPortAllocatorBindToPort(&v6used, i, AF_INET6) < 0 ||
+ virPortAllocatorBindToPort(&used, i, AF_INET) < 0)
goto cleanup;
-
- if (!used) {
+ if (!used && !v6used) {
/* Add port to bitmap of reserved ports */
if (virBitmapSetBit(pa->bitmap,
i - pa->start) < 0) {
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4:58 a.m.
New subject: [libvirt] [PATCH 2/2] Support IPv6 in port allocator
On 10/31/2013 06:33 PM, Daniel P. Berrange wrote:
On Thu, Oct 31, 2013 at 04:40:11PM +0100, Ján Tomko wrote:
> Also try to bind on IPv6 to check if the port is occupied.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1025407
> ---
> src/util/virportallocator.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 7 deletions(-)
Have you tested this on a system where the IPv6 kernel module is
not present, and on a system with IPv6 kmod is present, but no
interfaces have any IPv6 addresses (not even link-local ones).
I'm concerned this is going to trigger failures on IPv4 only
hosts.
When the ipv6 module is not present, socket(AF_INET6) returns EAFNOSUPPORT.
With sysctl net.ipv6.conf.all.disable_ipv6=1 even bind succeeds.
> diff --git a/src/util/virportallocator.c b/src/util/virportallocator.c
> index 1922ea6..24b4bff 100644
> --- a/src/util/virportallocator.c
> +++ b/src/util/virportallocator.c
> + if (family == AF_INET6) {
> + addr = (struct sockaddr*)&addr6;
> + addrlen = sizeof(addr6);
> + ipv6 = true;
> + } else if (family == AF_INET) {
> + addr = (struct sockaddr*)&addr4;
> + addrlen = sizeof(addr4);
family can be AF_INET or AF_INET6 here...
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown family %d"),
family);
> + return -1;
> + }
>
> *used = false;
>
> fd = socket(PF_INET, SOCK_STREAM, 0);
...but you're hardcoding PF_INET here. I'm not sure it is valid
to use an PF_INET socket with an AF_INET6 address in bind().
Ouch, rebase error. That can't work.
s/PF_INET/family/
> if (fd < 0) {
> + if (errno == EAFNOSUPPORT)
> + return 0;
> virReportSystemError(errno, "%s", _("Unable to open test
socket"));
> goto cleanup;
> }
Jan
12:40 p.m.
New subject: [libvirt] [PATCH 2/2] Support IPv6 in port allocator
On 10/31/2013 09:40 AM, Ján Tomko wrote:
Also try to bind on IPv6 to check if the port is occupied.
https://bugzilla.redhat.com/show_bug.cgi?id=1025407
---
src/util/virportallocator.c | 45 ++++++++++++++++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 7 deletions(-)
Since it is listed as a bugzilla fix, I could probably live with this in
1.1.4; but it would be nice to get a second opinion (after all, this is
not a regression, so it could be argued that we are finally implementing
a missing feature).
fd = socket(PF_INET, SOCK_STREAM, 0);
if (fd < 0) {
+ if (errno == EAFNOSUPPORT)
+ return 0;
As to the code, I didn't spot any errors. I do wonder if checking for
EAFNOSUPPORT on every iteration for finding a free port could be made
more efficient by remembering up front if ipv6 is even enabled, and
skipping the attempt in that case (that is, a running system is unlikely
to change which subset of [ipv4, ipv6] that it currently supports except
at boot). But there's probably more in the code base that we could
optimize if we know that a particular family is unsupported for the
given process.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:58 a.m.
New subject: [libvirt] [PATCH 2/2] Support IPv6 in port allocator
On 10/31/2013 06:40 PM, Eric Blake wrote:
On 10/31/2013 09:40 AM, Ján Tomko wrote:
> Also try to bind on IPv6 to check if the port is occupied.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1025407
> ---
> src/util/virportallocator.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 7 deletions(-)
Since it is listed as a bugzilla fix, I could probably live with this in
1.1.4; but it would be nice to get a second opinion (after all, this is
not a regression, so it could be argued that we are finally implementing
a missing feature).
> fd = socket(PF_INET, SOCK_STREAM, 0);
> if (fd < 0) {
> + if (errno == EAFNOSUPPORT)
> + return 0;
As to the code, I didn't spot any errors. I do wonder if checking for
EAFNOSUPPORT on every iteration for finding a free port could be made
more efficient by remembering up front if ipv6 is even enabled, and
skipping the attempt in that case (that is, a running system is unlikely
to change which subset of [ipv4, ipv6] that it currently supports except
at boot). But there's probably more in the code base that we could
optimize if we know that a particular family is unsupported for the
given process.
I've tried to check up front if ipv6 is enabled in v1:
https://www.redhat.com/archives/libvir-list/2013-October/msg00010.html
Jan
4073
days inactive
4074
days old
7 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko