On Thu, Jun 20, 2019 at 5:57 PM Michal Privoznik <mpriv...@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.ili...@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)
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.
Ilias
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
