3:10 p.m.
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>
---
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
2:46 a.m.
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>
---
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;
+}
4:36 a.m.
On Thu, May 23, 2019 at 9:46 AM Erik Skultety <eskultet(a)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(a)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
5:04 a.m.
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(a)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(a)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
4:12 a.m.
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(a)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
3:20 p.m.
On Mon, Jun 17, 2019 at 11:12 AM Michal Privoznik <mprivozn(a)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(a)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
1950
days inactive
1976
days old
5 comments
3 participants
participants (3)
-
Erik Skultety -
Ilias Stamatis -
Michal Privoznik