5 a.m.
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;
+
+ lease->iaid = lease->clientid = NULL;
+
+ if (VIR_INSERT_ELEMENT(leases_ret, nleases, nleases, lease) < 0)
+ goto error;
+ }
+
+ 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;
+}
+
+
/*
* Physical host interface routines
*/
@@ -7114,6 +7231,7 @@ static virNetworkDriver testNetworkDriver = {
.networkGetBridgeName = testNetworkGetBridgeName, /* 0.3.2 */
.networkGetAutostart = testNetworkGetAutostart, /* 0.3.2 */
.networkSetAutostart = testNetworkSetAutostart, /* 0.3.2 */
+ .networkGetDHCPLeases = testNetworkGetDHCPLeases, /* 5.5.0 */
.networkIsActive = testNetworkIsActive, /* 0.7.3 */
.networkIsPersistent = testNetworkIsPersistent, /* 0.7.3 */
};
--
2.21.0
8:25 a.m.
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?
+
+ 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.
+ }
+
+ 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
3:09 p.m.
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.
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
2:17 a.m.
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
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
1988
days inactive
1998
days old
4 comments
3 participants
participants (3)
-
Ilias Stamatis -
Michal Privoznik -
Michal Prívozník