Re: [libvirt] [PATCH] test_driver: implement virNetworkGetDHCPLeases

On Sat, Jun 15, 2019 at 3:25 PM Michal Prívozník <mprivozn(a)redhat.com&gt; wrote: > > On 6/8/19 12:00 PM, Ilias Stamatis wrote: >> Return infinite time DHCP leases for fixed IPV4 addresses. >> >> Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com&gt; >> --- >> src/test/test_driver.c | 118 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 118 insertions(+) >> >> diff --git a/src/test/test_driver.c b/src/test/test_driver.c >> index 1aa79ce898..b7f8f6ecf2 100644 >> --- a/src/test/test_driver.c >> +++ b/src/test/test_driver.c >> @@ -3831,6 +3831,123 @@ testNetworkSetAutostart(virNetworkPtr net, >> } >> >> >> +static int >> +testNetworkGetDHCPLeases(virNetworkPtr net, >> + const char *mac, >> + virNetworkDHCPLeasePtr **leases, >> + unsigned int flags) >> +{ >> + int ret = -1; >> + int ndomains = 0; >> + size_t i, j; >> + size_t nleases = 0; >> + bool need_results = !!leases; >> + char *hostname = NULL; >> + char mac_str[VIR_MAC_STRING_BUFLEN]; >> + virMacAddr mac_addr; >> + virDomainObjPtr vm = NULL; >> + virDomainPtr *domains = NULL; >> + virDomainNetDefPtr inf = NULL; >> + virNetworkObjPtr obj = NULL; >> + virNetworkDefPtr obj_def = NULL; >> + virNetworkDHCPLeasePtr lease = NULL; >> + virNetworkDHCPLeasePtr *leases_ret = NULL; >> + testDriverPtr privconn = net->conn->privateData; >> + >> + virCheckFlags(0, -1); >> + >> + if (mac && virMacAddrParse(mac, &mac_addr) < 0) { >> + virReportError(VIR_ERR_INVALID_MAC, "%s", mac); >> + return -1; >> + } >> + >> + if (!(obj = testNetworkObjFindByName(privconn, net->name))) >> + goto cleanup; >> + obj_def = virNetworkObjGetDef(obj); >> + >> + ndomains = virDomainObjListExport(privconn->domains, net->conn, &domains, >> + NULL, VIR_CONNECT_LIST_DOMAINS_ACTIVE); >> + if (ndomains < 0) >> + goto cleanup; >> + >> + for (i = 0; i < ndomains; i++) { >> + /* the following must be called before testDomObjFromDomain */ >> + hostname = testDomainGetHostname(domains[i], 0); >> + >> + if (!(vm = testDomObjFromDomain(domains[i]))) >> + continue; >> + >> + for (j = 0; j < vm->def->nnets; j++) { >> + inf = vm->def->nets[j]; >> + if (STRNEQ(inf->data.network.name, obj_def->name)) >> + continue; >> + >> + virMacAddrFormat(&inf->mac, mac_str); >> + if (mac && virMacAddrCompare(mac, mac_str)) >> + continue; >> + >> + if (!need_results) { >> + nleases++; >> + continue; >> + } >> + >> + if (VIR_ALLOC(lease) < 0) >> + goto error; >> + >> + /* the lease is for infinite time */ >> + lease->expirytime = 0; >> + >> + if ((VIR_STRDUP(lease->mac, mac_str) < 0) || >> + (VIR_STRDUP(lease->iface, obj_def->bridge) < 0) || >> + (VIR_STRDUP(lease->hostname, hostname) < 0)) >> + goto error; >> + >> + lease->prefix = 24; >> + lease->type = VIR_IP_ADDR_TYPE_IPV4; >> + if (virAsprintf(&lease->ipaddr, "192.168.0.%zu", 1 + j) < 0) >> + goto error; > > This doesn't look right. What is the nestwork has a different range defined? Right. We had a longer discussion about it with Erik and Pavel, sorry I didn't include any thoughts about this. So, as you already noticed testDomainInterfaceAddresses returns some hard-coded IP addresses at the moment, ignoring any defined DHCP ranges. We could make it such as it returns the first N addresses starting from dhcp_range_start. And then from within testNetworkGetDHCPLeases we can call testDomainInterfaceAddresses and filter out those that belong to different networks based on the subnet mask. However, if we have for example 2 domains using the same network with 1 address each, then testNetworkGetDHCPLeases will return the same IP lease twice and we already have an inconsistency. Maybe one idea would be for testDomainInterfaceAddresses to also take into account the domain id in such a way that it will return different addresses for different domains. But many of the things mentioned before would make the implementations more complex. Since there was a recent discussion about the scope of the test driver, we started wondering if it worths the effort. The conclusion of the "scope of the test driver" discussion was that it should only serve as a proof, that your app integrates with the API well and can process the info. Having everything interconnected and return some sane data would lead to full stack integration testing, which was never the intention. So we thought that for testing purposes returning hardcoded values is probably good enough. However, now that I'm thinking about it again, maybe it wouldn't take a huge effort to make it return some saner values so I can probably re-write it. Any additional thoughts would be useful.