[libvirt] [PATCH] test_driver: implement virNetworkGetDHCPLeases

[Ilias Stamatis]

On Sat, Jun 15, 2019 at 3:25 PM Michal Prívozník <mprivozn at redhat.com> 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 at gmail.com>
> > ---
> >  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.

Ilias

>
> > +
> > +            lease->iaid = lease->clientid = NULL;
> > +
> > +            if (VIR_INSERT_ELEMENT(leases_ret, nleases, nleases, lease) < 0)
> > +                goto error;
>
> I'm wondering if we can use VIR_AUTOPTR in some way. It looks like it'll
> simplify the code a lot.

For which variable? leases_ret?
>
> > +        }
> > +
> > +        VIR_FREE(hostname);
> > +        virDomainObjEndAPI(&vm);
> > +    }
> > +
> > +    if (leases_ret) {
> > +        /* NULL terminated array */
> > +        ignore_value(VIR_REALLOC_N(leases_ret, nleases + 1));
> > +        VIR_STEAL_PTR(*leases, leases_ret);
> > +    }
> > +
> > +    ret = nleases;
> > +
> > + cleanup:
> > +    for (i = 0; i < ndomains; i++)
> > +        virDomainFree(domains[i]);
> > +    VIR_FREE(domains);
> > +
> > +    VIR_FREE(hostname);
> > +    virNetworkObjEndAPI(&obj);
> > +    return ret;
> > +
> > + error:
> > +    VIR_FREE(lease);
> > +
> > +    if (leases_ret) {
> > +        for (i = 0; i < nleases; i++)
> > +            virNetworkDHCPLeaseFree(leases_ret[i]);
> > +        VIR_FREE(leases_ret);
> > +    }
> > +
> > +    virDomainObjEndAPI(&vm);
> > +    goto cleanup;
> > +}
>
> Otherwise looking good.
>
> Michal