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@gmail.com>
---
 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)))
+            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;
-
         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);
     }

     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++)
--
2.22.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@gmail.com>
> ---
>   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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Jun 20, 2019 at 5:57 PM Michal Privoznik <mprivozn@redhat.com> 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@gmail.com>
> > ---
> >   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)

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.

Ilias

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 6/20/19 7:36 PM, Ilias Stamatis wrote:
> On Thu, Jun 20, 2019 at 5:57 PM Michal Privoznik <mprivozn@redhat.com> 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@gmail.com>
>>> ---
>>>    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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Jun 21, 2019 at 10:03 AM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> On 6/20/19 7:36 PM, Ilias Stamatis wrote:
> > On Thu, Jun 20, 2019 at 5:57 PM Michal Privoznik <mprivozn@redhat.com> 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@gmail.com>
> >>> ---
> >>>    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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 6/21/19 11:49 AM, Ilias Stamatis wrote:
 > <snip/>
> 
> 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.

Yeah, we have a mixture of c89 and c99. Honestly, I don't know why. And 
also, I can see us using c99 style loops in some cases, i.e. small, self 
contained loop that does not need a rollback if a function called from 
within its body fails. But then again, inconsistency hurts code 
readability much more than suboptimal consistency.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list