5:39 a.m.
On Tue, Jun 18, 2019 at 9:17 AM Michal Privoznik <mprivozn(a)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(a)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(a)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