Re: [libvirt] [PATCH v2] test_driver: properly handle DHCP ranges and IPv6 networks in testDomainInterfaceAddresses

On 6/20/19 7:36 PM, Ilias Stamatis wrote: > On Thu, Jun 20, 2019 at 5:57 PM Michal Privoznik <mprivozn(a)redhat.com&gt; wrote: >> >> On 6/19/19 6:45 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, properly handle IPv6 networks which were previously >>> ignored completely. >>> >>> Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com&gt; >>> --- >>> src/test/test_driver.c | 44 +++++++++++++++++++++++++++++++++++++----- >>> 1 file changed, 39 insertions(+), 5 deletions(-) >>> >>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c >>> index 2a0ffbc6c5..21bd95941e 100755 >>> --- a/src/test/test_driver.c >>> +++ b/src/test/test_driver.c >>> @@ -3414,6 +3414,10 @@ static int testDomainBlockStats(virDomainPtr domain, >>> return ret; >>> } >>> >>> + >>> +static virNetworkObjPtr testNetworkObjFindByName(testDriverPtr privconn, const char *name); >>> + >>> + >>> static int >>> testDomainInterfaceAddresses(virDomainPtr dom, >>> virDomainInterfacePtr **ifaces, >>> @@ -3422,11 +3426,15 @@ testDomainInterfaceAddresses(virDomainPtr dom, >>> { >>> size_t i; >>> size_t ifaces_count = 0; >>> + size_t addr_offset; >>> int ret = -1; >>> char macaddr[VIR_MAC_STRING_BUFLEN]; >>> virDomainObjPtr vm = NULL; >>> virDomainInterfacePtr iface = NULL; >>> virDomainInterfacePtr *ifaces_ret = NULL; >>> + virSocketAddr addr; >>> + virNetworkObjPtr net = NULL; >>> + virNetworkDefPtr net_def = NULL; >>> >>> virCheckFlags(0, -1); >>> >>> @@ -3447,6 +3455,12 @@ testDomainInterfaceAddresses(virDomainPtr dom, >>> goto cleanup; >>> >>> for (i = 0; i < vm->def->nnets; i++) { >>> + if (!(net = testNetworkObjFindByName(dom->conn->privateData, >>> + vm->def->nets[i]->data.network.name))) >> >> This is unsafe. We can access ->data.network iff type is NETWORK. >> >>> + goto cleanup; >>> + >>> + net_def = virNetworkObjGetDef(net); >>> + >>> if (VIR_ALLOC(iface) < 0) >>> goto cleanup; >>> >>> @@ -3460,14 +3474,33 @@ 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; >>> - >> >> Instead of removing, we can use this for !NETWORK types. >> >>> iface->naddrs = 1; >>> + iface->addrs[0].prefix = virSocketAddrGetIPPrefix(&net_def->ips->address, >>> + &net_def->ips->netmask, >>> + net_def->ips->prefix); >>> + >>> + if (net_def->ips->nranges > 0) >>> + addr = net_def->ips->ranges[0].start; >>> + else >>> + addr = net_def->ips->address; >>> + >>> + /* try using different addresses per different inf and domain */ >>> + addr_offset = 20 * (vm->def->id - 1) + i + 1; >>> + >>> + if (net_def->ips->family && STREQ(net_def->ips->family, "ipv6")) { >>> + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV6; >>> + addr.data.inet6.sin6_addr.s6_addr[15] += addr_offset; >>> + } else { >>> + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV4; >>> + addr.data.inet4.sin_addr.s_addr = \ >>> + htonl(ntohl(addr.data.inet4.sin_addr.s_addr) + addr_offset); >>> + } >>> + >>> + if (!(iface->addrs[0].addr = virSocketAddrFormat(&addr))) >>> + goto cleanup; >>> >>> VIR_APPEND_ELEMENT_INPLACE(ifaces_ret, ifaces_count, iface); >>> + virNetworkObjEndAPI(&net); >> >> This should be moved into a separate function. >> >>> } >>> >>> VIR_STEAL_PTR(*ifaces, ifaces_ret); >>> @@ -3475,6 +3508,7 @@ testDomainInterfaceAddresses(virDomainPtr dom, >>> >>> cleanup: >>> virDomainObjEndAPI(&vm); >>> + virNetworkObjEndAPI(&net); >>> >>> if (ifaces_ret) { >>> for (i = 0; i < ifaces_count; i++) >> >> >> With all that fixed, I've ACKed and pushed this patch. Thank you for >> taking care of this. >> >> Michal > > Just a tiny nitpick by me as well on the code you pushed. > > The addr_offset can be used also for the non-network infs in order to > attempt always having unique ips. > > ie instead of: > if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", 1 + i) < 0) > > it can be: > if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", addr_offset) < 0) Ah right. > > Also, I don't know how strict we are on enforcing the coding > guidelines but 2 variables are not declared in the beginning of the > function but later. Ideally (and honestly I don't know how our coding style is specified in this regard O:-)), any variable would be defined at the lowest possible scope. For instance, if a variable is used only within a loop body, then it increases code readability if the variable is defined only inside the loop (at its beginning of course). Because when I'm reading the code and find such variable I can be sure that it's not used outside of the loop and thus if I'm not interested in the loop I can skip it and don't have to track the variable or find out its meaning. Of course, this is not always possible, for instance, if there's a variable that holds a pointer to an object that needs to be unrefed at the end of every iteration and there are some 'goto cleanup'-s inside the loop body. Just like in your patch. Then of course the variable needs to be defined for the whole function so that 'cleanup' label can do the unref. A picture is worth a thousand words: void f() { int i; int tmp; for (i = 0; i < 100; i++) { tmp = rand() % 10; printf("sleeping for %d seconds\n", tmp); sleep(tmp); } /* some buggy area here */ } versus: void f() { int i; for (i = 0; i < 100; i++) { int tmp = rand() % 10; printf("sleeping for %d seconds\n", tmp); sleep(tmp); } /* some buggy area here */ } IMO, the second version is more readable. Michal