[libvirt] [PATCH] test_driver: implement virNetworkGetDHCPLeases

[Ilias Stamatis]

On Tue, Jun 18, 2019 at 9:17 AM Michal Privoznik <mprivozn at redhat.com> 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 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.
>
> 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