[libvirt] [PATCH] test_driver: implement virNetworkGetDHCPLeases

[Michal Privoznik]

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