[libvirt] [PATCH 0/3] test_driver: improve testDomainInterfaceAddresses
This patch series introduces the following improvements to the testDomainInterfaceAddresses function: - if a dhcp range is defined for the network, addresses are returned from there (instead of hard-coded addresses that were returned before) - if the network is IPv6 even when a dhcp range is not defined IPv6 addresses will be returned instead of IPv4 - the @source argument is validated - only networks of type VIR_DOMAIN_NET_TYPE_NETWORK are considered Ilias Stamatis (3): test_driver: validate @source in testDomainInterfaceAddresses test_driver: return addresses only for nets of type network test_driver: consider DHCP ranges in testDomainInterfaceAddresses src/test/test_driver.c | 96 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 86 insertions(+), 10 deletions(-) -- 2.22.0
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 8ef843b203..437df9cdf9 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3383,7 +3383,7 @@ static int testDomainBlockStats(virDomainPtr domain, static int testDomainInterfaceAddresses(virDomainPtr dom, virDomainInterfacePtr **ifaces, - unsigned int source ATTRIBUTE_UNUSED, + unsigned int source, unsigned int flags) { size_t i; @@ -3396,6 +3396,13 @@ testDomainInterfaceAddresses(virDomainPtr dom, virCheckFlags(0, -1); + if (source >= VIR_DOMAIN_INTERFACE_ADDRESSES_SRC_LAST) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, + _("Unknown IP address data source %d"), + source); + return -1; + } + if (!(vm = testDomObjFromDomain(dom))) goto cleanup; -- 2.22.0
On 6/19/19 1:18 PM, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
ACKed and pushed. Michal
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 437df9cdf9..0c2cfdd2f7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3409,10 +3409,10 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (virDomainObjCheckActive(vm) < 0) goto cleanup; - if (VIR_ALLOC_N(ifaces_ret, vm->def->nnets) < 0) - goto cleanup; - for (i = 0; i < vm->def->nnets; i++) { + if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) + continue; + if (VIR_ALLOC(iface) < 0) goto cleanup; @@ -3433,7 +3433,8 @@ testDomainInterfaceAddresses(virDomainPtr dom, iface->naddrs = 1; - VIR_APPEND_ELEMENT_INPLACE(ifaces_ret, ifaces_count, iface); + if (VIR_APPEND_ELEMENT(ifaces_ret, ifaces_count, iface) < 0) + goto cleanup; } VIR_STEAL_PTR(*ifaces, ifaces_ret); -- 2.22.0
On 6/19/19 1:18 PM, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 437df9cdf9..0c2cfdd2f7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3409,10 +3409,10 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (virDomainObjCheckActive(vm) < 0) goto cleanup;
- if (VIR_ALLOC_N(ifaces_ret, vm->def->nnets) < 0) - goto cleanup; - for (i = 0; i < vm->def->nnets; i++) { + if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) + continue; + if (VIR_ALLOC(iface) < 0) goto cleanup;
@@ -3433,7 +3433,8 @@ testDomainInterfaceAddresses(virDomainPtr dom,
iface->naddrs = 1;
- VIR_APPEND_ELEMENT_INPLACE(ifaces_ret, ifaces_count, iface); + if (VIR_APPEND_ELEMENT(ifaces_ret, ifaces_count, iface) < 0) + goto cleanup; }
VIR_STEAL_PTR(*ifaces, ifaces_ret);
I understand why you want this, but can't we keep the old behaviour for non-network interfaces? Michal
On Wed, Jun 19, 2019 at 3:48 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 6/19/19 1:18 PM, Ilias Stamatis wrote:
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 437df9cdf9..0c2cfdd2f7 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -3409,10 +3409,10 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (virDomainObjCheckActive(vm) < 0) goto cleanup;
- if (VIR_ALLOC_N(ifaces_ret, vm->def->nnets) < 0) - goto cleanup; - for (i = 0; i < vm->def->nnets; i++) { + if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) + continue; + if (VIR_ALLOC(iface) < 0) goto cleanup;
@@ -3433,7 +3433,8 @@ testDomainInterfaceAddresses(virDomainPtr dom,
iface->naddrs = 1;
- VIR_APPEND_ELEMENT_INPLACE(ifaces_ret, ifaces_count, iface); + if (VIR_APPEND_ELEMENT(ifaces_ret, ifaces_count, iface) < 0) + goto cleanup; }
VIR_STEAL_PTR(*ifaces, ifaces_ret);
I understand why you want this, but can't we keep the old behaviour for non-network interfaces?
Michal
Sure, let's do it like that. I'll include the change in the new patch. Ilias
testDomainInterfaceAddresses always returns the same hard-coded addresses. Change the behavior such as if there is a DHCP range defined, addresses are returned from that pool. The specific address returned depends on both the domain id and the specific guest interface in an attempt to return unique addresses *most of the time*. Additionally, return IPv6 addresses too when needed. Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 78 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 5 deletions(-) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0c2cfdd2f7..96142b549c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -27,6 +27,7 @@ #include <sys/stat.h> #include <libxml/xmlsave.h> #include <libxml/xpathInternals.h> +#include <arpa/inet.h> #include "virerror.h" @@ -3380,6 +3381,11 @@ static int testDomainBlockStats(virDomainPtr domain, return ret; } + +static virNetworkObjPtr testNetworkObjFindByName(testDriverPtr privconn, const char *name); +static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, const char *name); + + static int testDomainInterfaceAddresses(virDomainPtr dom, virDomainInterfacePtr **ifaces, @@ -3388,11 +3394,18 @@ testDomainInterfaceAddresses(virDomainPtr dom, { size_t i; size_t ifaces_count = 0; + size_t addr_offset; int ret = -1; char macaddr[VIR_MAC_STRING_BUFLEN]; + char ip_str[INET6_ADDRSTRLEN]; + struct sockaddr_in ipv4_addr; + struct sockaddr_in6 ipv6_addr; virDomainObjPtr vm = NULL; virDomainInterfacePtr iface = NULL; virDomainInterfacePtr *ifaces_ret = NULL; + virNetworkPtr net = NULL; + virNetworkObjPtr net_obj = NULL; + virNetworkDefPtr net_obj_def = NULL; virCheckFlags(0, -1); @@ -3413,6 +3426,16 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue; + virObjectUnref(net); + if (!(net = testNetworkLookupByName(dom->conn, + vm->def->nets[i]->data.network.name))) + goto cleanup; + + if (!(net_obj = testNetworkObjFindByName(net->conn->privateData, net->name))) + goto cleanup; + + net_obj_def = virNetworkObjGetDef(net_obj); + if (VIR_ALLOC(iface) < 0) goto cleanup; @@ -3426,15 +3449,58 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (VIR_ALLOC(iface->addrs) < 0) goto cleanup; - iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV4; - iface->addrs[0].prefix = 24; - if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", 1 + i) < 0) - goto cleanup; - iface->naddrs = 1; + if (net_obj_def->ips->family && STREQ(net_obj_def->ips->family, "ipv6")) + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV6; + else + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV4; + + /* try using different addresses per different inf and domain */ + addr_offset = 20 * (vm->def->id - 1) + i; + + if (net_obj_def->ips->nranges > 0) { + /* use ip addresses from the defined DHCP range */ + + if (iface->addrs[0].type == VIR_IP_ADDR_TYPE_IPV6) { + ipv6_addr = net_obj_def->ips->ranges[0].start.data.inet6; + ipv6_addr.sin6_addr.s6_addr[15] += addr_offset; + inet_ntop(AF_INET6, &(ipv6_addr.sin6_addr), ip_str, INET6_ADDRSTRLEN); + } else { + ipv4_addr = net_obj_def->ips->ranges[0].start.data.inet4; + ipv4_addr.sin_addr.s_addr = htonl(ntohl(ipv4_addr.sin_addr.s_addr) + + addr_offset); + inet_ntop(AF_INET, &(ipv4_addr.sin_addr), ip_str, INET_ADDRSTRLEN); + } + + if (VIR_STRDUP(iface->addrs[0].addr, ip_str) < 0) + goto cleanup; + + iface->addrs[0].prefix = virSocketAddrGetIPPrefix( + &net_obj_def->ips->ranges[0].start, + &net_obj_def->ips->netmask, + net_obj_def->ips->prefix); + + } else { + /* use hard-coded addresses when a DHCP range isn't defined */ + + if (iface->addrs[0].type == VIR_IP_ADDR_TYPE_IPV6) { + iface->addrs[0].prefix = 64; + if (virAsprintf(&iface->addrs[0].addr, "fc00::%zu", + 1 + addr_offset) < 0) + goto cleanup; + } else { + iface->addrs[0].prefix = 24; + if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", + 1 + addr_offset) < 0) + goto cleanup; + } + } + if (VIR_APPEND_ELEMENT(ifaces_ret, ifaces_count, iface) < 0) goto cleanup; + + virNetworkObjEndAPI(&net_obj); } VIR_STEAL_PTR(*ifaces, ifaces_ret); @@ -3442,6 +3508,8 @@ testDomainInterfaceAddresses(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); + virNetworkObjEndAPI(&net_obj); + virObjectUnref(net); if (ifaces_ret) { for (i = 0; i < ifaces_count; i++) -- 2.22.0
On Wed, Jun 19, 2019 at 1:19 PM Ilias Stamatis <stamatis.iliass@gmail.com> wrote:
testDomainInterfaceAddresses always returns the same hard-coded addresses. Change the behavior such as if there is a DHCP range defined, addresses are returned from that pool.
The specific address returned depends on both the domain id and the specific guest interface in an attempt to return unique addresses *most of the time*.
Additionally, return IPv6 addresses too when needed.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 78 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 5 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0c2cfdd2f7..96142b549c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -27,6 +27,7 @@ #include <sys/stat.h> #include <libxml/xmlsave.h> #include <libxml/xpathInternals.h> +#include <arpa/inet.h>
#include "virerror.h" @@ -3380,6 +3381,11 @@ static int testDomainBlockStats(virDomainPtr domain, return ret; }
+ +static virNetworkObjPtr testNetworkObjFindByName(testDriverPtr privconn, const char *name); +static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, const char *name); + + static int testDomainInterfaceAddresses(virDomainPtr dom, virDomainInterfacePtr **ifaces, @@ -3388,11 +3394,18 @@ testDomainInterfaceAddresses(virDomainPtr dom, { size_t i; size_t ifaces_count = 0; + size_t addr_offset; int ret = -1; char macaddr[VIR_MAC_STRING_BUFLEN]; + char ip_str[INET6_ADDRSTRLEN]; + struct sockaddr_in ipv4_addr; + struct sockaddr_in6 ipv6_addr; virDomainObjPtr vm = NULL; virDomainInterfacePtr iface = NULL; virDomainInterfacePtr *ifaces_ret = NULL; + virNetworkPtr net = NULL; + virNetworkObjPtr net_obj = NULL; + virNetworkDefPtr net_obj_def = NULL;
virCheckFlags(0, -1);
@@ -3413,6 +3426,16 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue;
+ virObjectUnref(net); + if (!(net = testNetworkLookupByName(dom->conn, + vm->def->nets[i]->data.network.name))) + goto cleanup; + + if (!(net_obj = testNetworkObjFindByName(net->conn->privateData, net->name))) + goto cleanup; + + net_obj_def = virNetworkObjGetDef(net_obj); + if (VIR_ALLOC(iface) < 0) goto cleanup;
@@ -3426,15 +3449,58 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (VIR_ALLOC(iface->addrs) < 0) goto cleanup;
- iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV4; - iface->addrs[0].prefix = 24; - if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", 1 + i) < 0) - goto cleanup; - iface->naddrs = 1;
+ if (net_obj_def->ips->family && STREQ(net_obj_def->ips->family, "ipv6")) + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV6; + else + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV4; + + /* try using different addresses per different inf and domain */ + addr_offset = 20 * (vm->def->id - 1) + i; + + if (net_obj_def->ips->nranges > 0) { + /* use ip addresses from the defined DHCP range */ + + if (iface->addrs[0].type == VIR_IP_ADDR_TYPE_IPV6) { + ipv6_addr = net_obj_def->ips->ranges[0].start.data.inet6; + ipv6_addr.sin6_addr.s6_addr[15] += addr_offset; + inet_ntop(AF_INET6, &(ipv6_addr.sin6_addr), ip_str, INET6_ADDRSTRLEN); + } else { + ipv4_addr = net_obj_def->ips->ranges[0].start.data.inet4; + ipv4_addr.sin_addr.s_addr = htonl(ntohl(ipv4_addr.sin_addr.s_addr) + + addr_offset); + inet_ntop(AF_INET, &(ipv4_addr.sin_addr), ip_str, INET_ADDRSTRLEN);
I forgot to run make syntax-check and I found out that it complains about using inet_ntop. So I'll send a new version of this patch.
+ } + + if (VIR_STRDUP(iface->addrs[0].addr, ip_str) < 0) + goto cleanup; + + iface->addrs[0].prefix = virSocketAddrGetIPPrefix( + &net_obj_def->ips->ranges[0].start, + &net_obj_def->ips->netmask, + net_obj_def->ips->prefix); + + } else { + /* use hard-coded addresses when a DHCP range isn't defined */ + + if (iface->addrs[0].type == VIR_IP_ADDR_TYPE_IPV6) { + iface->addrs[0].prefix = 64; + if (virAsprintf(&iface->addrs[0].addr, "fc00::%zu", + 1 + addr_offset) < 0) + goto cleanup; + } else { + iface->addrs[0].prefix = 24; + if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", + 1 + addr_offset) < 0) + goto cleanup; + } + } + if (VIR_APPEND_ELEMENT(ifaces_ret, ifaces_count, iface) < 0) goto cleanup; + + virNetworkObjEndAPI(&net_obj); }
VIR_STEAL_PTR(*ifaces, ifaces_ret); @@ -3442,6 +3508,8 @@ testDomainInterfaceAddresses(virDomainPtr dom,
cleanup: virDomainObjEndAPI(&vm); + virNetworkObjEndAPI(&net_obj); + virObjectUnref(net);
if (ifaces_ret) { for (i = 0; i < ifaces_count; i++) -- 2.22.0
On 6/19/19 1:18 PM, Ilias Stamatis wrote:
testDomainInterfaceAddresses always returns the same hard-coded addresses. Change the behavior such as if there is a DHCP range defined, addresses are returned from that pool.
The specific address returned depends on both the domain id and the specific guest interface in an attempt to return unique addresses *most of the time*.
Additionally, return IPv6 addresses too when needed.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 78 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 5 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0c2cfdd2f7..96142b549c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -27,6 +27,7 @@ #include <sys/stat.h> #include <libxml/xmlsave.h> #include <libxml/xpathInternals.h> +#include <arpa/inet.h>
#include "virerror.h" @@ -3380,6 +3381,11 @@ static int testDomainBlockStats(virDomainPtr domain, return ret; }
+ +static virNetworkObjPtr testNetworkObjFindByName(testDriverPtr privconn, const char *name); +static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, const char *name); + + static int testDomainInterfaceAddresses(virDomainPtr dom, virDomainInterfacePtr **ifaces, @@ -3388,11 +3394,18 @@ testDomainInterfaceAddresses(virDomainPtr dom, { size_t i; size_t ifaces_count = 0; + size_t addr_offset; int ret = -1; char macaddr[VIR_MAC_STRING_BUFLEN]; + char ip_str[INET6_ADDRSTRLEN]; + struct sockaddr_in ipv4_addr; + struct sockaddr_in6 ipv6_addr; virDomainObjPtr vm = NULL; virDomainInterfacePtr iface = NULL; virDomainInterfacePtr *ifaces_ret = NULL; + virNetworkPtr net = NULL; + virNetworkObjPtr net_obj = NULL; + virNetworkDefPtr net_obj_def = NULL;
virCheckFlags(0, -1);
@@ -3413,6 +3426,16 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue;
+ virObjectUnref(net); + if (!(net = testNetworkLookupByName(dom->conn, + vm->def->nets[i]->data.network.name))) + goto cleanup; + + if (!(net_obj = testNetworkObjFindByName(net->conn->privateData, net->name))) + goto cleanup;
This is needless IMO. I mean, @net variable looks redundant to me, why not look up the network object directly? For instance: if (!(net_obj = testNetworkObjFindByName(dom->conn->privateData, vm->def->nets[i]->data.network.name))) goto cleanup; But looking further at next hunk, I wonder if it makes sense to separate it into a separate function. Otherwise the code looks good. A bit complicated, but that was expected, since dealing with IP addresses is never a few lines of code :-( Michal
On Wed, Jun 19, 2019 at 03:47:58PM +0200, Michal Privoznik wrote:
On 6/19/19 1:18 PM, Ilias Stamatis wrote:
[...]
@@ -3413,6 +3426,16 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue;
+ virObjectUnref(net); + if (!(net = testNetworkLookupByName(dom->conn, + vm->def->nets[i]->data.network.name))) + goto cleanup; + + if (!(net_obj = testNetworkObjFindByName(net->conn->privateData, net->name))) + goto cleanup;
This is needless IMO. I mean, @net variable looks redundant to me, why not look up the network object directly? For instance:
if (!(net_obj = testNetworkObjFindByName(dom->conn->privateData,
vm->def->nets[i]->data.network.name))) goto cleanup;
But looking further at next hunk, I wonder if it makes sense to separate it into a separate function.
Yes, please. Jano
Otherwise the code looks good. A bit complicated, but that was expected, since dealing with IP addresses is never a few lines of code :-(
Michal
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jun 19, 2019 at 3:48 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 6/19/19 1:18 PM, Ilias Stamatis wrote:
testDomainInterfaceAddresses always returns the same hard-coded addresses. Change the behavior such as if there is a DHCP range defined, addresses are returned from that pool.
The specific address returned depends on both the domain id and the specific guest interface in an attempt to return unique addresses *most of the time*.
Additionally, return IPv6 addresses too when needed.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 78 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 5 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0c2cfdd2f7..96142b549c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -27,6 +27,7 @@ #include <sys/stat.h> #include <libxml/xmlsave.h> #include <libxml/xpathInternals.h> +#include <arpa/inet.h>
#include "virerror.h" @@ -3380,6 +3381,11 @@ static int testDomainBlockStats(virDomainPtr domain, return ret; }
+ +static virNetworkObjPtr testNetworkObjFindByName(testDriverPtr privconn, const char *name); +static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, const char *name); + + static int testDomainInterfaceAddresses(virDomainPtr dom, virDomainInterfacePtr **ifaces, @@ -3388,11 +3394,18 @@ testDomainInterfaceAddresses(virDomainPtr dom, { size_t i; size_t ifaces_count = 0; + size_t addr_offset; int ret = -1; char macaddr[VIR_MAC_STRING_BUFLEN]; + char ip_str[INET6_ADDRSTRLEN]; + struct sockaddr_in ipv4_addr; + struct sockaddr_in6 ipv6_addr; virDomainObjPtr vm = NULL; virDomainInterfacePtr iface = NULL; virDomainInterfacePtr *ifaces_ret = NULL; + virNetworkPtr net = NULL; + virNetworkObjPtr net_obj = NULL; + virNetworkDefPtr net_obj_def = NULL;
virCheckFlags(0, -1);
@@ -3413,6 +3426,16 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue;
+ virObjectUnref(net); + if (!(net = testNetworkLookupByName(dom->conn, + vm->def->nets[i]->data.network.name))) + goto cleanup; + + if (!(net_obj = testNetworkObjFindByName(net->conn->privateData, net->name))) + goto cleanup;
This is needless IMO. I mean, @net variable looks redundant to me, why not look up the network object directly? For instance:
if (!(net_obj = testNetworkObjFindByName(dom->conn->privateData,
vm->def->nets[i]->data.network.name))) goto cleanup;
Aah, right. I will remove this.
But looking further at next hunk, I wonder if it makes sense to separate it into a separate function. Otherwise the code looks good. A bit complicated, but that was expected, since dealing with IP addresses is never a few lines of code :-(
I managed to do it just a tiny bit simpler by using a virSocketAddr instead of the 2 struct sockaddr_in ones. But which part do you want me to separate exactly?
Michal
On 6/19/19 4:04 PM, Ilias Stamatis wrote:
On Wed, Jun 19, 2019 at 3:48 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 6/19/19 1:18 PM, Ilias Stamatis wrote:
testDomainInterfaceAddresses always returns the same hard-coded addresses. Change the behavior such as if there is a DHCP range defined, addresses are returned from that pool.
The specific address returned depends on both the domain id and the specific guest interface in an attempt to return unique addresses *most of the time*.
Additionally, return IPv6 addresses too when needed.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 78 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 5 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0c2cfdd2f7..96142b549c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -27,6 +27,7 @@ #include <sys/stat.h> #include <libxml/xmlsave.h> #include <libxml/xpathInternals.h> +#include <arpa/inet.h>
#include "virerror.h" @@ -3380,6 +3381,11 @@ static int testDomainBlockStats(virDomainPtr domain, return ret; }
+ +static virNetworkObjPtr testNetworkObjFindByName(testDriverPtr privconn, const char *name); +static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, const char *name); + + static int testDomainInterfaceAddresses(virDomainPtr dom, virDomainInterfacePtr **ifaces, @@ -3388,11 +3394,18 @@ testDomainInterfaceAddresses(virDomainPtr dom, { size_t i; size_t ifaces_count = 0; + size_t addr_offset; int ret = -1; char macaddr[VIR_MAC_STRING_BUFLEN]; + char ip_str[INET6_ADDRSTRLEN]; + struct sockaddr_in ipv4_addr; + struct sockaddr_in6 ipv6_addr; virDomainObjPtr vm = NULL; virDomainInterfacePtr iface = NULL; virDomainInterfacePtr *ifaces_ret = NULL; + virNetworkPtr net = NULL; + virNetworkObjPtr net_obj = NULL; + virNetworkDefPtr net_obj_def = NULL;
virCheckFlags(0, -1);
@@ -3413,6 +3426,16 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue;
+ virObjectUnref(net); + if (!(net = testNetworkLookupByName(dom->conn, + vm->def->nets[i]->data.network.name))) + goto cleanup; + + if (!(net_obj = testNetworkObjFindByName(net->conn->privateData, net->name))) + goto cleanup;
This is needless IMO. I mean, @net variable looks redundant to me, why not look up the network object directly? For instance:
if (!(net_obj = testNetworkObjFindByName(dom->conn->privateData,
vm->def->nets[i]->data.network.name))) goto cleanup;
Aah, right. I will remove this.
But looking further at next hunk, I wonder if it makes sense to separate it into a separate function. Otherwise the code looks good. A bit complicated, but that was expected, since dealing with IP addresses is never a few lines of code :-(
I managed to do it just a tiny bit simpler by using a virSocketAddr instead of the 2 struct sockaddr_in ones.
Awesome!
But which part do you want me to separate exactly?
My idea was to keep the outer line loop, and probably call functions from it. Something among these lines: for (i = 0; i < vm->def->nnets; i++) { if (vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_NETWORK) iface = testDomainInterfaceAddressFromNet(vm->def->nets[i]); else iface = testDomainInterfaceAddressDefault(vm->def->nets[i]); if (!iface) error; VIR_APPEND_ELEMENT_INPLACE(ifaces_ret, ifaces_count, iface); } Alternatively, we might allocate @iface inside the loop, set its name and MAC address also in the loop (so that the above functions don't duplicate code) and then pass it to the functions just to fill iface->addrs. Michal
On Wed, Jun 19, 2019 at 4:19 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 6/19/19 4:04 PM, Ilias Stamatis wrote:
On Wed, Jun 19, 2019 at 3:48 PM Michal Privoznik <mprivozn@redhat.com> wrote:
On 6/19/19 1:18 PM, Ilias Stamatis wrote:
testDomainInterfaceAddresses always returns the same hard-coded addresses. Change the behavior such as if there is a DHCP range defined, addresses are returned from that pool.
The specific address returned depends on both the domain id and the specific guest interface in an attempt to return unique addresses *most of the time*.
Additionally, return IPv6 addresses too when needed.
Signed-off-by: Ilias Stamatis <stamatis.iliass@gmail.com> --- src/test/test_driver.c | 78 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 5 deletions(-)
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 0c2cfdd2f7..96142b549c 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -27,6 +27,7 @@ #include <sys/stat.h> #include <libxml/xmlsave.h> #include <libxml/xpathInternals.h> +#include <arpa/inet.h>
#include "virerror.h" @@ -3380,6 +3381,11 @@ static int testDomainBlockStats(virDomainPtr domain, return ret; }
+ +static virNetworkObjPtr testNetworkObjFindByName(testDriverPtr privconn, const char *name); +static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, const char *name); + + static int testDomainInterfaceAddresses(virDomainPtr dom, virDomainInterfacePtr **ifaces, @@ -3388,11 +3394,18 @@ testDomainInterfaceAddresses(virDomainPtr dom, { size_t i; size_t ifaces_count = 0; + size_t addr_offset; int ret = -1; char macaddr[VIR_MAC_STRING_BUFLEN]; + char ip_str[INET6_ADDRSTRLEN]; + struct sockaddr_in ipv4_addr; + struct sockaddr_in6 ipv6_addr; virDomainObjPtr vm = NULL; virDomainInterfacePtr iface = NULL; virDomainInterfacePtr *ifaces_ret = NULL; + virNetworkPtr net = NULL; + virNetworkObjPtr net_obj = NULL; + virNetworkDefPtr net_obj_def = NULL;
virCheckFlags(0, -1);
@@ -3413,6 +3426,16 @@ testDomainInterfaceAddresses(virDomainPtr dom, if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK) continue;
+ virObjectUnref(net); + if (!(net = testNetworkLookupByName(dom->conn, + vm->def->nets[i]->data.network.name))) + goto cleanup; + + if (!(net_obj = testNetworkObjFindByName(net->conn->privateData, net->name))) + goto cleanup;
This is needless IMO. I mean, @net variable looks redundant to me, why not look up the network object directly? For instance:
if (!(net_obj = testNetworkObjFindByName(dom->conn->privateData,
vm->def->nets[i]->data.network.name))) goto cleanup;
Aah, right. I will remove this.
But looking further at next hunk, I wonder if it makes sense to separate it into a separate function. Otherwise the code looks good. A bit complicated, but that was expected, since dealing with IP addresses is never a few lines of code :-(
I managed to do it just a tiny bit simpler by using a virSocketAddr instead of the 2 struct sockaddr_in ones.
Awesome!
But which part do you want me to separate exactly?
My idea was to keep the outer line loop, and probably call functions from it. Something among these lines:
for (i = 0; i < vm->def->nnets; i++) { if (vm->def->nets[i]->type == VIR_DOMAIN_NET_TYPE_NETWORK) iface = testDomainInterfaceAddressFromNet(vm->def->nets[i]); else iface = testDomainInterfaceAddressDefault(vm->def->nets[i]);
if (!iface) error;
VIR_APPEND_ELEMENT_INPLACE(ifaces_ret, ifaces_count, iface); }
Alternatively, we might allocate @iface inside the loop, set its name and MAC address also in the loop (so that the above functions don't duplicate code) and then pass it to the functions just to fill iface->addrs.
Michal
I just sent a new version in which I re-wrote most of the logic. I think now the patch is much simpler and shorter than before so we don't need to extract anything into separate functions. However if you think it can be even simpler or it can somehow benefit from extracting some code into different functions let me know. Thanks for the review! Ilias
On Wed, Jun 19, 2019 at 1:18 PM Ilias Stamatis <stamatis.iliass@gmail.com> wrote:
This patch series introduces the following improvements to the testDomainInterfaceAddresses function:
- if a dhcp range is defined for the network, addresses are returned from there (instead of hard-coded addresses that were returned before) - if the network is IPv6 even when a dhcp range is not defined IPv6 addresses will be returned instead of IPv4 - the @source argument is validated - only networks of type VIR_DOMAIN_NET_TYPE_NETWORK are considered
Ilias Stamatis (3): test_driver: validate @source in testDomainInterfaceAddresses test_driver: return addresses only for nets of type network test_driver: consider DHCP ranges in testDomainInterfaceAddresses
src/test/test_driver.c | 96 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 86 insertions(+), 10 deletions(-)
-- 2.22.0
self-NACK since I forgot to run make syntax-check and some changes need to be done
participants (3)
-
Ilias Stamatis -
Ján Tomko -
Michal Privoznik