Re: [libvirt] [PATCH] test_driver: implement virDomainInterfaceAddresses

On Thu, May 23, 2019 at 9:46 AM Erik Skultety <eskultet(a)redhat.com&gt; wrote: > > On Wed, May 22, 2019 at 10:10:34PM +0200, Ilias Stamatis wrote: > > Ignore @source in the case of the test driver and return fixed private > > IPv4 addresses for all the interfaces defined in the domain. > > > > Signed-off-by: Ilias Stamatis <stamatis.iliass(a)gmail.com&gt; > > --- > > > > The default config of the test driver has no guest interfaces defined, > > so this must be tested with a custom config. > > > > Maybe it would be a good idea to add one or more guest interfaces in the > > default config. I could send an additional patch for this. > > I agree and at the same time I think that patch and this one should be part of > the same series. Sure, I'll add that. One interface would be sufficient I suppose? How about the following? <interface type='network'> <mac address='aa:bb:cc:dd:ee:ff'/> <source network='default' bridge='virbr0'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x00' function='0x0'/>

</interface> > > > > > src/test/test_driver.c | 71 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 71 insertions(+) > > > > diff --git a/src/test/test_driver.c b/src/test/test_driver.c > > index a4c17ef0df..3a81f51a88 100644 > > --- a/src/test/test_driver.c > > +++ b/src/test/test_driver.c > > @@ -3220,6 +3220,76 @@ static int testDomainBlockStats(virDomainPtr domain, > > return ret; > > } > > > > +static int > > +testDomainInterfaceAddresses(virDomainPtr dom, > > + virDomainInterfacePtr **ifaces, > > + unsigned int source ATTRIBUTE_UNUSED, > > + unsigned int flags) > > +{ > > + size_t i; > > + size_t ifaces_count = 0; > > + int ret = -1; > > + char ipaddr[32]; > > + char macaddr[VIR_MAC_STRING_BUFLEN]; > > + virDomainObjPtr vm = NULL; > > + virDomainInterfacePtr iface = NULL; > > + virDomainInterfacePtr *ifaces_ret = NULL; > > + > > + virCheckFlags(0, -1); > > + > > + if (!(vm = testDomObjFromDomain(dom))) > > + goto cleanup; > > + > > + if (virDomainObjCheckActive(vm) < 0) > > + goto cleanup; > > + > > + if (VIR_ALLOC_N(ifaces_ret, vm->def->nnets) < 0) > > + goto cleanup; > > + > > + for (i = 0; i < vm->def->nnets; i++) { > > + if (VIR_ALLOC(iface) < 0) > > + goto cleanup; > > + > > + if (VIR_STRDUP(iface->name, vm->def->nets[i]->ifname) < 0) > > + goto cleanup; > > + > > + virMacAddrFormat(&(vm->def->nets[i]->mac), macaddr); > > + if (VIR_STRDUP(iface->hwaddr, macaddr) < 0) > > + goto cleanup; > > + > > + if (VIR_ALLOC(iface->addrs) < 0) > > + goto cleanup; > > + > > + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV4; > > + iface->addrs[0].prefix = 24; > > + > > + sprintf(ipaddr, "192.168.0.%ld", 100 + (i % 155)); > > It's test driver do we expect to be dealing with that many interfaces? I think > '100 + i' or even 'i + i' would be just fine, we don't need to overcomplicate > it. I agree in general, but I don't think it's really overcomplicated here, it's just 5 extra chars. :D Why not reply with results that make sense in all cases?

> > > + if (VIR_STRDUP(iface->addrs[0].addr, ipaddr) < 0) > > + goto cleanup; > > How about using virAsprintf directly into iface->addrs[0].addr? (sprintf is also > blacklisted by the syntax-check) > Aah, great! I wasn't aware of this function, I'll change it to use that. Btw, how can I run this syntax-check you refer to or see which functions are blacklisted?