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

On 6/19/19 4:04 PM, Ilias Stamatis wrote: > 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. 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