[libvirt] [PATCH] test_driver: implement virDomainInterfaceAddresses
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
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; +}
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
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
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
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
participants (3)
-
Erik Skultety -
Ilias Stamatis -
Michal Privoznik