[libvirt] [PATCH 3/3] test_driver: consider DHCP ranges in testDomainInterfaceAddresses

[Ilias Stamatis]

On Wed, Jun 19, 2019 at 3:48 PM Michal Privoznik <mprivozn at redhat.com> wrote:
>
> On 6/19/19 1:18 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, return IPv6 addresses too when needed.
> >
> > Signed-off-by: Ilias Stamatis <stamatis.iliass at gmail.com>
> > ---
> >   src/test/test_driver.c | 78 +++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 73 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> > index 0c2cfdd2f7..96142b549c 100644
> > --- a/src/test/test_driver.c
> > +++ b/src/test/test_driver.c
> > @@ -27,6 +27,7 @@
> >   #include <sys/stat.h>
> >   #include <libxml/xmlsave.h>
> >   #include <libxml/xpathInternals.h>
> > +#include <arpa/inet.h>
> >
> >
> >   #include "virerror.h"
> > @@ -3380,6 +3381,11 @@ static int testDomainBlockStats(virDomainPtr domain,
> >       return ret;
> >   }
> >
> > +
> > +static virNetworkObjPtr testNetworkObjFindByName(testDriverPtr privconn, const char *name);
> > +static virNetworkPtr testNetworkLookupByName(virConnectPtr conn, const char *name);
> > +
> > +
> >   static int
> >   testDomainInterfaceAddresses(virDomainPtr dom,
> >                                virDomainInterfacePtr **ifaces,
> > @@ -3388,11 +3394,18 @@ testDomainInterfaceAddresses(virDomainPtr dom,
> >   {
> >       size_t i;
> >       size_t ifaces_count = 0;
> > +    size_t addr_offset;
> >       int ret = -1;
> >       char macaddr[VIR_MAC_STRING_BUFLEN];
> > +    char ip_str[INET6_ADDRSTRLEN];
> > +    struct sockaddr_in ipv4_addr;
> > +    struct sockaddr_in6 ipv6_addr;
> >       virDomainObjPtr vm = NULL;
> >       virDomainInterfacePtr iface = NULL;
> >       virDomainInterfacePtr *ifaces_ret = NULL;
> > +    virNetworkPtr net = NULL;
> > +    virNetworkObjPtr net_obj = NULL;
> > +    virNetworkDefPtr net_obj_def = NULL;
> >
> >       virCheckFlags(0, -1);
> >
> > @@ -3413,6 +3426,16 @@ testDomainInterfaceAddresses(virDomainPtr dom,
> >           if (vm->def->nets[i]->type != VIR_DOMAIN_NET_TYPE_NETWORK)
> >               continue;
> >
> > +        virObjectUnref(net);
> > +        if (!(net = testNetworkLookupByName(dom->conn,
> > +                                            vm->def->nets[i]->data.network.name)))
> > +            goto cleanup;
> > +
> > +        if (!(net_obj = testNetworkObjFindByName(net->conn->privateData, net->name)))
> > +            goto cleanup;
>
> This is needless IMO. I mean, @net variable looks redundant to me, why
> not look up the network object directly? For instance:
>
>    if (!(net_obj = testNetworkObjFindByName(dom->conn->privateData,
>
> vm->def->nets[i]->data.network.name)))
>        goto cleanup;

Aah, right. I will remove this.

>
> But looking further at next hunk, I wonder if it makes sense to separate
> it into a separate function. Otherwise the code looks good. A bit
> complicated, but that was expected, since dealing with IP addresses is
> never a few lines of code :-(

I managed to do it just a tiny bit simpler by using a virSocketAddr
instead of the 2 struct sockaddr_in ones.

But which part do you want me to separate exactly?

>
> Michal