On Fri, Jun 21, 2019 at 10:03 AM Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
>
> On 6/20/19 7:36 PM, Ilias Stamatis wrote:
> > On Thu, Jun 20, 2019 at 5:57 PM Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
> >>
> >> On 6/19/19 6:45 PM, Ilias Stamatis wrote:
> >>> testDomainInterfaceAddresses always returns the same hard-coded
> >>> addresses. Change the behavior such as if there is a DHCP range defined,
> >>> addresses are returned from that pool.
> >>>
> >>> The specific address returned depends on both the domain id and the
> >>> specific guest interface in an attempt to return unique addresses *most
> >>> of the time*.
> >>>
> >>> Additionally, properly handle IPv6 networks which were previously
> >>> ignored completely.
> >>>
> >>> Signed-off-by: Ilias Stamatis <stamatis.iliass@xxxxxxxxx>
> >>> ---
> >>> src/test/test_driver.c | 44 +++++++++++++++++++++++++++++++++++++-----
> >>> 1 file changed, 39 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/src/test/test_driver.c b/src/test/test_driver.c
> >>> index 2a0ffbc6c5..21bd95941e 100755
> >>> --- a/src/test/test_driver.c
> >>> +++ b/src/test/test_driver.c
> >>> @@ -3414,6 +3414,10 @@ static int testDomainBlockStats(virDomainPtr domain,
> >>> return ret;
> >>> }
> >>>
> >>> +
> >>> +static virNetworkObjPtr testNetworkObjFindByName(testDriverPtr privconn, const char *name);
> >>> +
> >>> +
> >>> static int
> >>> testDomainInterfaceAddresses(virDomainPtr dom,
> >>> virDomainInterfacePtr **ifaces,
> >>> @@ -3422,11 +3426,15 @@ testDomainInterfaceAddresses(virDomainPtr dom,
> >>> {
> >>> size_t i;
> >>> size_t ifaces_count = 0;
> >>> + size_t addr_offset;
> >>> int ret = -1;
> >>> char macaddr[VIR_MAC_STRING_BUFLEN];
> >>> virDomainObjPtr vm = NULL;
> >>> virDomainInterfacePtr iface = NULL;
> >>> virDomainInterfacePtr *ifaces_ret = NULL;
> >>> + virSocketAddr addr;
> >>> + virNetworkObjPtr net = NULL;
> >>> + virNetworkDefPtr net_def = NULL;
> >>>
> >>> virCheckFlags(0, -1);
> >>>
> >>> @@ -3447,6 +3455,12 @@ testDomainInterfaceAddresses(virDomainPtr dom,
> >>> goto cleanup;
> >>>
> >>> for (i = 0; i < vm->def->nnets; i++) {
> >>> + if (!(net = testNetworkObjFindByName(dom->conn->privateData,
> >>> + vm->def->nets[i]->data.network.name)))
> >>
> >> This is unsafe. We can access ->data.network iff type is NETWORK.
> >>
> >>> + goto cleanup;
> >>> +
> >>> + net_def = virNetworkObjGetDef(net);
> >>> +
> >>> if (VIR_ALLOC(iface) < 0)
> >>> goto cleanup;
> >>>
> >>> @@ -3460,14 +3474,33 @@ testDomainInterfaceAddresses(virDomainPtr dom,
> >>> if (VIR_ALLOC(iface->addrs) < 0)
> >>> goto cleanup;
> >>>
> >>> - iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV4;
> >>> - iface->addrs[0].prefix = 24;
> >>> - if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", 1 + i) < 0)
> >>> - goto cleanup;
> >>> -
> >>
> >> Instead of removing, we can use this for !NETWORK types.
> >>
> >>> iface->naddrs = 1;
> >>> + iface->addrs[0].prefix = virSocketAddrGetIPPrefix(&net_def->ips->address,
> >>> + &net_def->ips->netmask,
> >>> + net_def->ips->prefix);
> >>> +
> >>> + if (net_def->ips->nranges > 0)
> >>> + addr = net_def->ips->ranges[0].start;
> >>> + else
> >>> + addr = net_def->ips->address;
> >>> +
> >>> + /* try using different addresses per different inf and domain */
> >>> + addr_offset = 20 * (vm->def->id - 1) + i + 1;
> >>> +
> >>> + if (net_def->ips->family && STREQ(net_def->ips->family, "ipv6")) {
> >>> + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV6;
> >>> + addr.data.inet6.sin6_addr.s6_addr[15] += addr_offset;
> >>> + } else {
> >>> + iface->addrs[0].type = VIR_IP_ADDR_TYPE_IPV4;
> >>> + addr.data.inet4.sin_addr.s_addr = \
> >>> + htonl(ntohl(addr.data.inet4.sin_addr.s_addr) + addr_offset);
> >>> + }
> >>> +
> >>> + if (!(iface->addrs[0].addr = virSocketAddrFormat(&addr)))
> >>> + goto cleanup;
> >>>
> >>> VIR_APPEND_ELEMENT_INPLACE(ifaces_ret, ifaces_count, iface);
> >>> + virNetworkObjEndAPI(&net);
> >>
> >> This should be moved into a separate function.
> >>
> >>> }
> >>>
> >>> VIR_STEAL_PTR(*ifaces, ifaces_ret);
> >>> @@ -3475,6 +3508,7 @@ testDomainInterfaceAddresses(virDomainPtr dom,
> >>>
> >>> cleanup:
> >>> virDomainObjEndAPI(&vm);
> >>> + virNetworkObjEndAPI(&net);
> >>>
> >>> if (ifaces_ret) {
> >>> for (i = 0; i < ifaces_count; i++)
> >>
> >>
> >> With all that fixed, I've ACKed and pushed this patch. Thank you for
> >> taking care of this.
> >>
> >> Michal
> >
> > Just a tiny nitpick by me as well on the code you pushed.
> >
> > The addr_offset can be used also for the non-network infs in order to
> > attempt always having unique ips.
> >
> > ie instead of:
> > if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", 1 + i) < 0)
> >
> > it can be:
> > if (virAsprintf(&iface->addrs[0].addr, "192.168.0.%zu", addr_offset) < 0)
>
> Ah right.
>
> >
> > Also, I don't know how strict we are on enforcing the coding
> > guidelines but 2 variables are not declared in the beginning of the
> > function but later.
>
> Ideally (and honestly I don't know how our coding style is specified in
> this regard O:-)), any variable would be defined at the lowest possible
> scope. For instance, if a variable is used only within a loop body, then
> it increases code readability if the variable is defined only inside the
> loop (at its beginning of course). Because when I'm reading the code and
> find such variable I can be sure that it's not used outside of the loop
> and thus if I'm not interested in the loop I can skip it and don't have
> to track the variable or find out its meaning.
>
> Of course, this is not always possible, for instance, if there's a
> variable that holds a pointer to an object that needs to be unrefed at
> the end of every iteration and there are some 'goto cleanup'-s inside
> the loop body. Just like in your patch. Then of course the variable
> needs to be defined for the whole function so that 'cleanup' label can
> do the unref.
>
> A picture is worth a thousand words:
>
> void f() {
> int i;
> int tmp;
>
> for (i = 0; i < 100; i++) {
> tmp = rand() % 10;
> printf("sleeping for %d seconds\n", tmp);
> sleep(tmp);
> }
>
> /* some buggy area here */
> }
>
> versus:
>
> void f() {
> int i;
>
> for (i = 0; i < 100; i++) {
> int tmp = rand() % 10;
> printf("sleeping for %d seconds\n", tmp);
> sleep(tmp);
> }
>
> /* some buggy area here */
> }
>
>
> IMO, the second version is more readable.
>
> Michal
I totally agree with you, it's more readable for me too.
Just so far I haven't encountered in the code. Also e.g. C99 allows
variables to be declared inside a loop ie
for (size_t i = 0; ...
...but I haven't seen that being used anywhere either.
Anyway I just wasn't sure about the guidelines, that's why I brought it up.
Thanks,
Ilias
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
