Re: [libvirt] [PATCH 3/3] test_driver: consider DHCP ranges in testDomainInterfaceAddresses

On Wed, Jun 19, 2019 at 3:48 PM Michal Privoznik <mprivozn(a)redhat.com&gt; 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(a)gmail.com&gt; >> --- >> 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?