3:03 a.m.
On 6/20/19 7:36 PM, Ilias Stamatis wrote:
On Thu, Jun 20, 2019 at 5:57 PM Michal Privoznik
<mprivozn(a)redhat.com> wrote:
>
> On 6/19/19 6:45 PM, Ilias Stamatis wrote:
>> testDomainInterfaceAddresses always returns the same hard-coded
>> addresses. Change the behavior such as if there is a DHCP range defined,
>> addresses are returned from that pool.
>>
>> The specific address returned depends on both the domain id and the
>> specific guest interface in an attempt to return unique addresses *most
>> of the time*.
>>
>> Additionally, properly handle IPv6 networks which were previously
>> ignored completely.
>>
>> Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com>
>> ---
>> src/test/test_driver.c | 44 +++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 39 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
>> index 2a0ffbc6c5..21bd95941e 100755
>> --- a/src/test/test_driver.c
>> +++ b/src/test/test_driver.c
>> @@ -3414,6 +3414,10 @@ static int testDomainBlockStats(virDomainPtr domain,
>> return ret;
>> }
>>
>> +
>> +static virNetworkObjPtr testNetworkObjFindByName(testDriverPtr privconn, const
char *name);
>> +
>> +
>> static int
>> testDomainInterfaceAddresses(virDomainPtr dom,
>> virDomainInterfacePtr **ifaces,
>> @@ -3422,11 +3426,15 @@ testDomainInterfaceAddresses(virDomainPtr dom,
>> {
>> size_t i;
>> size_t ifaces_count = 0;
>> + size_t addr_offset;
>> int ret = -1;
>> char macaddr[VIR_MAC_STRING_BUFLEN];
>> virDomainObjPtr vm = NULL;
>> virDomainInterfacePtr iface = NULL;
>> virDomainInterfacePtr *ifaces_ret = NULL;
>> + virSocketAddr addr;
>> + virNetworkObjPtr net = NULL;
>> + virNetworkDefPtr net_def = NULL;
>>
>> virCheckFlags(0, -1);
>>
>> @@ -3447,6 +3455,12 @@ testDomainInterfaceAddresses(virDomainPtr dom,
>> goto cleanup;
>>
>> for (i = 0; i < vm->def->nnets; i++) {
>> + if (!(net = testNetworkObjFindByName(dom->conn->privateData,
>> +
vm->def->nets[i]->data.network.name)))
>
> This is unsafe. We can access ->data.network iff type is NETWORK.
>
>> + goto cleanup;
>> +
>> + net_def = virNetworkObjGetDef(net);
>> +
>> if (VIR_ALLOC(iface) < 0)
>> goto cleanup;
>>
>> @@ -3460,14 +3474,33 @@ testDomainInterfaceAddresses(virDomainPtr dom,
>> if (VIR_ALLOC(iface->addrs) < 0)
>> goto cleanup;
>>
>> - iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV4;
>> - iface->addrs[0].prefix = 24;
>> - if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu",
1 + i) < 0)
>> - goto cleanup;
>> -
>
> Instead of removing, we can use this for !NETWORK types.
>
>> iface->naddrs = 1;
>> + iface->addrs[0].prefix =
virSocketAddrGetIPPrefix(&net_def->ips->address,
>> +
&net_def->ips->netmask,
>> +
net_def->ips->prefix);
>> +
>> + if (net_def->ips->nranges > 0)
>> + addr = net_def->ips->ranges[0].start;
>> + else
>> + addr = net_def->ips->address;
>> +
>> + /* try using different addresses per different inf and domain */
>> + addr_offset = 20 * (vm->def->id - 1) + i + 1;
>> +
>> + if (net_def->ips->family &&
STREQ(net_def->ips->family, "ipv6")) {
>> + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV6;
>> + addr.data.inet6.sin6_addr.s6_addr[15] += addr_offset;
>> + } else {
>> + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV4;
>> + addr.data.inet4.sin_addr.s_addr = \
>> + htonl(ntohl(addr.data.inet4.sin_addr.s_addr) + addr_offset);
>> + }
>> +
>> + if (!(iface->addrs[0].addr = virSocketAddrFormat(&addr)))
>> + goto cleanup;
>>
>> VIR_APPEND_ELEMENT_INPLACE(ifaces_ret, ifaces_count, iface);
>> + virNetworkObjEndAPI(&net);
>
> This should be moved into a separate function.
>
>> }
>>
>> VIR_STEAL_PTR(*ifaces, ifaces_ret);
>> @@ -3475,6 +3508,7 @@ testDomainInterfaceAddresses(virDomainPtr dom,
>>
>> cleanup:
>> virDomainObjEndAPI(&vm);
>> + virNetworkObjEndAPI(&net);
>>
>> if (ifaces_ret) {
>> for (i = 0; i < ifaces_count; i++)
>
>
> With all that fixed, I've ACKed and pushed this patch. Thank you for
> taking care of this.
>
> Michal
Just a tiny nitpick by me as well on the code you pushed.
The addr_offset can be used also for the non-network infs in order to
attempt always having unique ips.
ie instead of:
if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", 1 + i) < 0)
it can be:
if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", addr_offset)
< 0)
Ah right.
Also, I don't know how strict we are on enforcing the coding
guidelines but 2 variables are not declared in the beginning of the
function but later.
Ideally (and honestly I don't know how our coding style is specified in
this regard O:-)), any variable would be defined at the lowest possible
scope. For instance, if a variable is used only within a loop body, then
it increases code readability if the variable is defined only inside the
loop (at its beginning of course). Because when I'm reading the code and
find such variable I can be sure that it's not used outside of the loop
and thus if I'm not interested in the loop I can skip it and don't have
to track the variable or find out its meaning.
Of course, this is not always possible, for instance, if there's a
variable that holds a pointer to an object that needs to be unrefed at
the end of every iteration and there are some 'goto cleanup'-s inside
the loop body. Just like in your patch. Then of course the variable
needs to be defined for the whole function so that 'cleanup' label can
do the unref.
A picture is worth a thousand words:
void f() {
int i;
int tmp;
for (i = 0; i < 100; i++) {
tmp = rand() % 10;
printf("sleeping for %d seconds\n", tmp);
sleep(tmp);
}
/* some buggy area here */
}
versus:
void f() {
int i;
for (i = 0; i < 100; i++) {
int tmp = rand() % 10;
printf("sleeping for %d seconds\n", tmp);
sleep(tmp);
}
/* some buggy area here */
}
IMO, the second version is more readable.
Michal