On Thu, May 23, 2019 at 11:36:31AM +0200, Ilias Stamatis wrote:
> On Thu, May 23, 2019 at 9:46 AM Erik Skultety <eskultet@xxxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > >
> > > 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'/>
Just a nitpick ^this would very likely not a be a valid endpoint PCI device
address, as that would indicate a host bridge (on most systems), so I'd use
some other slot number here ;).
> </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?
In the "one" instance specifically? :)
I mean, sure, but strictly speaking anything over 155 ifaces would produce
erroneous results just like 100+i would and moreover you need to compute the
remainder each time for no particular reason, that's what I called
"over-complicating" ;).
>
> >
> > > + 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?
It's one of our make targets => make syntax-check, so when sending patches,
we recommend using make check && make syntax-check before hitting send.
Erik
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
