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@gmail.com>
---
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.
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));
+ if (VIR_STRDUP(iface->addrs[0].addr, ipaddr) < 0)
+ goto cleanup;
+
+ iface->naddrs = 1;
+
+ VIR_STEAL_PTR(ifaces_ret[i], iface);
+ ifaces_count++;
+ }
+
+ VIR_STEAL_PTR(*ifaces, ifaces_ret);
+ ret = ifaces_count;
+
+ cleanup:
+ virDomainObjEndAPI(&vm);
+
+ if (ifaces_ret) {
+ for (i = 0; i < ifaces_count; i++)
+ virDomainInterfaceFree(ifaces_ret[i]);
+ }
+ virDomainInterfaceFree(iface);
+
+ VIR_FREE(ifaces_ret);
+
+ return ret;
+}
+
static int
testDomainInterfaceStats(virDomainPtr domain,
const char *device,
@@ -6876,6 +6946,7 @@ static virHypervisorDriver testHypervisorDriver = {
.domainSetSchedulerParameters = testDomainSetSchedulerParameters, /* 0.3.2 */
.domainSetSchedulerParametersFlags = testDomainSetSchedulerParametersFlags, /* 0.9.2 */
.domainBlockStats = testDomainBlockStats, /* 0.7.0 */
+ .domainInterfaceAddresses = testDomainInterfaceAddresses, /* 5.4.0 */
.domainInterfaceStats = testDomainInterfaceStats, /* 0.7.0 */
.nodeGetCellsFreeMemory = testNodeGetCellsFreeMemory, /* 0.4.2 */
.connectDomainEventRegister = testConnectDomainEventRegister, /* 0.6.0 */
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@gmail.com> > --- > > 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. > > 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. > + 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) > + > + iface->naddrs = 1; > + > + VIR_STEAL_PTR(ifaces_ret[i], iface); We usually use VIR_APPEND_ELEMENT for ^this, it also takes the ifaces_count counter which it increments for you, but since you already pre-allocated the list earlier, you'd be interested in the _INPLACE version of the same macro. Erik > + ifaces_count++; > + } > + > + VIR_STEAL_PTR(*ifaces, ifaces_ret); > + ret = ifaces_count; > + > + cleanup: > + virDomainObjEndAPI(&vm); > + > + if (ifaces_ret) { > + for (i = 0; i < ifaces_count; i++) > + virDomainInterfaceFree(ifaces_ret[i]); > + } > + virDomainInterfaceFree(iface); > + > + VIR_FREE(ifaces_ret); > + > + return ret; > +} -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, May 23, 2019 at 9:46 AM Erik Skultety <eskultet@redhat.com> 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@gmail.com> > > --- > > > > 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? > > + > > + iface->naddrs = 1; > > + > > + VIR_STEAL_PTR(ifaces_ret[i], iface); > > We usually use VIR_APPEND_ELEMENT for ^this, it also takes the ifaces_count > counter which it increments for you, but since you already pre-allocated the > list earlier, you'd be interested in the _INPLACE version of the same macro. > Cool, I'll implement all these and re-send. Thanks for the review! Ilias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@redhat.com> 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@gmail.com> > > > --- > > > > > > 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 5/22/19 10:10 PM, 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@gmail.com> > --- > > 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. > > 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)); Thinking about this again in terms of your net-dhcp-leases path you posted the other day, and sorry for bringing it up so late, should we have a special case for interfaces that are from a libvirt network with defined IP range? I mean, if there's: <interface type='network> <source network='default'/> </interface> <network connections='1' ipv6='yes'> <name>default</name> <ip address='192.168.122.1' netmask='255.255.255.0'> <dhcp> <range start='192.168.122.2' end='192.168.122.254'/> </dhcp> </ip> </network> it's a bit funny to find reported IP address out of the range. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, Jun 17, 2019 at 11:12 AM Michal Privoznik <mprivozn@redhat.com> wrote: > > On 5/22/19 10:10 PM, 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@gmail.com> > > --- > > > > 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. > > > > 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)); > > Thinking about this again in terms of your net-dhcp-leases path you > posted the other day, and sorry for bringing it up so late, should we > have a special case for interfaces that are from a libvirt network with > defined IP range? I mean, if there's: > > <interface type='network> > <source network='default'/> > </interface> > > <network connections='1' ipv6='yes'> > <name>default</name> > <ip address='192.168.122.1' netmask='255.255.255.0'> > <dhcp> > <range start='192.168.122.2' end='192.168.122.254'/> > </dhcp> > </ip> > </network> > > it's a bit funny to find reported IP address out of the range. > > Michal I brought that up on my reply to your patch review for testNetworkGetDHCPLeases so we can discuss it on that thread. Ilias -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.