On Tue, Jun 18, 2019 at 9:17 AM Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
>
> On 6/17/19 10:09 PM, Ilias Stamatis wrote:
> > On Sat, Jun 15, 2019 at 3:25 PM Michal Prívozník <mprivozn@xxxxxxxxxx> 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@xxxxxxxxx>
> >>> ---
> >>> 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.
>
> I think returning the same IP address fro two distinct domains is a good
> trade off between code complexity and sane API behaviour. IOW, making
> the API return an IP address from the network range is a matter of a few
> lines of code but making sure it's unique across all domains would be
> much bigger task and probably not worth it.
>
> I can cook the patch if you want me to. After all, I should have raised
> this sooner.
>
> Michal
That's fine. I'll send a patch fixing testDomainInterfaceAddresses and
then I'll send a v2 for testNetworkGetDHCPLeases.
On which variable did you suggest using VIR_AUTOPTR finally?
Ilias
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
