1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/5] libvirt_nss: Report newer addresses first
  4. View patch

[libvirt] [PATCH 5/5] libvirt_nss: Report newer addresses first

Michal Privoznik posted 5 patches 6 years, 7 months ago
[libvirt] [PATCH 5/5] libvirt_nss: Report newer addresses first
Posted by Michal Privoznik 6 years, 7 months ago 
Ideally, a software that's translating domain names would iterate
over all addresses the NSS returned, but some software does not
bother (e.g. ping). What happens is that for instance when
installing a guest, it's assigned one IP address but once it's
installed and rebooted it gets a different IP address (because
client ID used for the first DHCP traffic when installing the
guest was generated dynamically and never saved so after reboot
the guest generated new ID which resulted in different IP address
to be assigned). This results in 'ping $domain' not working
properly as it still pings the old IP address. Well, it might -
NSS plugin does not guarantee any order of addresses.

To resolve this problem, we can sort the array just before
returning it to the caller (ping) so that the newer IP addresses
come before older ones.

Reported-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tests/nssdata/virbr0.status |  6 +++---
 tests/nsstest.c             | 21 ++++++++-------------
 tools/nss/libvirt_nss.c     | 30 ++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/tests/nssdata/virbr0.status b/tests/nssdata/virbr0.status
index d040774269..783efb1dfb 100644
--- a/tests/nssdata/virbr0.status
+++ b/tests/nssdata/virbr0.status
@@ -3,19 +3,19 @@
         "ip-address": "192.168.122.197",
         "mac-address": "52:54:00:a4:6f:91",
         "hostname": "fedora",
-        "expiry-time": 1900000000
+        "expiry-time": 1900000003
     },
     {
         "ip-address": "192.168.122.198",
         "mac-address": "52:54:00:a4:6f:92",
         "hostname": "fedora",
-        "expiry-time": 1900000000
+        "expiry-time": 1900000002
     },
     {
         "ip-address": "192.168.122.254",
         "mac-address": "52:54:00:3a:b5:0c",
         "hostname": "gentoo",
-        "expiry-time": 2000000000
+        "expiry-time": 2000000001
     },
     {
         "ip-address": "192.168.122.2",
diff --git a/tests/nsstest.c b/tests/nsstest.c
index d43c59c4a2..4118c31cef 100644
--- a/tests/nsstest.c
+++ b/tests/nsstest.c
@@ -46,7 +46,7 @@ testGetHostByName(const void *opaque)
     char buf[BUF_SIZE] = { 0 };
     char **addrList;
     int rv, tmp_errno = 0, tmp_herrno = 0;
-    size_t i = 0, j = 0;
+    size_t i = 0;
 
     memset(&resolved, 0, sizeof(resolved));
 
@@ -117,6 +117,7 @@ testGetHostByName(const void *opaque)
     }
 
     addrList = resolved.h_addr_list;
+    i = 0;
     while (*addrList) {
         virSocketAddr sa;
         char *ipAddr;
@@ -135,14 +136,10 @@ testGetHostByName(const void *opaque)
             goto cleanup;
         }
 
-        for (j = 0; data->ipAddr[j]; j++) {
-            if (STREQ(data->ipAddr[j], ipAddr))
-                break;
-        }
-
-        if (!data->ipAddr[j]) {
+        if (STRNEQ_NULLABLE(data->ipAddr[i], ipAddr)) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
-                           "Unexpected address %s", ipAddr);
+                           "Unexpected address %s, expecting %s",
+                           ipAddr, NULLSTR(data->ipAddr[i]));
             VIR_FREE(ipAddr);
             goto cleanup;
         }
@@ -152,12 +149,10 @@ testGetHostByName(const void *opaque)
         i++;
     }
 
-    for (j = 0; data->ipAddr[j]; j++)
-        ;
-
-    if (i != j) {
+    if (data->ipAddr[i]) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
-                       "Expected %zu addresses, got %zu", j, i);
+                       "Expected %s address, got NULL",
+                       data->ipAddr[i]);
         goto cleanup;
     }
 
diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c
index b0e118bf37..519046a4e0 100644
--- a/tools/nss/libvirt_nss.c
+++ b/tools/nss/libvirt_nss.c
@@ -76,9 +76,29 @@ do { \
 typedef struct {
     unsigned char addr[16];
     int af;
+    long long expirytime;
 } leaseAddress;
 
 
+static int
+leaseAddressSorter(const void *a,
+                   const void *b)
+{
+    const leaseAddress *la = a;
+    const leaseAddress *lb = b;
+
+    return lb->expirytime - la->expirytime;
+}
+
+
+static void
+sortAddr(leaseAddress *tmpAddress,
+         size_t ntmpAddress)
+{
+    qsort(tmpAddress, ntmpAddress, sizeof(*tmpAddress), leaseAddressSorter);
+}
+
+
 static int
 appendAddr(const char *name ATTRIBUTE_UNUSED,
            leaseAddress **tmpAddress,
@@ -89,6 +109,7 @@ appendAddr(const char *name ATTRIBUTE_UNUSED,
     const char *ipAddr;
     virSocketAddr sa;
     int family;
+    long long expirytime;
     size_t i;
 
     if (!(ipAddr = virJSONValueObjectGetString(lease, "ip-address"))) {
@@ -109,6 +130,12 @@ appendAddr(const char *name ATTRIBUTE_UNUSED,
         return 0;
     }
 
+    if (virJSONValueObjectGetNumberLong(lease, "expiry-time", &expirytime) < 0) {
+        /* A lease cannot be present without expiry-time */
+        ERROR("expiry-time field missing for %s", name);
+        return -1;
+    }
+
     for (i = 0; i < *ntmpAddress; i++) {
         if (memcmp((*tmpAddress)[i].addr,
                    (family == AF_INET ?
@@ -125,6 +152,7 @@ appendAddr(const char *name ATTRIBUTE_UNUSED,
         return -1;
     }
 
+    (*tmpAddress)[*ntmpAddress].expirytime = expirytime;
     (*tmpAddress)[*ntmpAddress].af = family;
     memcpy((*tmpAddress)[*ntmpAddress].addr,
            (family == AF_INET ?
@@ -325,6 +353,8 @@ findLease(const char *name,
 
 #endif /* defined(LIBVIRT_NSS_GUEST) */
 
+    sortAddr(tmpAddress, ntmpAddress);
+
     VIR_STEAL_PTR(*address, tmpAddress);
     *naddress = ntmpAddress;
     ntmpAddress = 0;
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] libvirt_nss: Report newer addresses first
Posted by Erik Skultety 6 years, 6 months ago 
On Sat, Jul 13, 2019 at 12:58:38PM +0200, Michal Privoznik wrote:
> Ideally, a software that's translating domain names would iterate
> over all addresses the NSS returned, but some software does not
> bother (e.g. ping). What happens is that for instance when
> installing a guest, it's assigned one IP address but once it's
> installed and rebooted it gets a different IP address (because
> client ID used for the first DHCP traffic when installing the
> guest was generated dynamically and never saved so after reboot
> the guest generated new ID which resulted in different IP address
> to be assigned). This results in 'ping $domain' not working
> properly as it still pings the old IP address. Well, it might -
> NSS plugin does not guarantee any order of addresses.
>
> To resolve this problem, we can sort the array just before
> returning it to the caller (ping) so that the newer IP addresses
> come before older ones.
>
> Reported-by: Andrea Bolognani <abologna@redhat.com>
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
...

>      {
>          "ip-address": "192.168.122.254",
>          "mac-address": "52:54:00:3a:b5:0c",
>          "hostname": "gentoo",
> -        "expiry-time": 2000000000
> +        "expiry-time": 2000000001

So, why is ^this specific change needed? In the test, you're expecting
192.168.122.[197-199], 192.168.122.199 has expiry time of 1900000000, so the
test would still work.

192.168.122.254 is only assumed in a gentoo test twice, but there are no other
addresses to sort in those cases, so the expiry time in there should not matter
right?

With the explanation of the above:
Reviewed-by: Erik Skultety <eskultet@redhat.com>

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/5] libvirt_nss: Report newer addresses first
Posted by Michal Privoznik 6 years, 6 months ago 
On 7/15/19 10:16 AM, Erik Skultety wrote:
> On Sat, Jul 13, 2019 at 12:58:38PM +0200, Michal Privoznik wrote:
>> Ideally, a software that's translating domain names would iterate
>> over all addresses the NSS returned, but some software does not
>> bother (e.g. ping). What happens is that for instance when
>> installing a guest, it's assigned one IP address but once it's
>> installed and rebooted it gets a different IP address (because
>> client ID used for the first DHCP traffic when installing the
>> guest was generated dynamically and never saved so after reboot
>> the guest generated new ID which resulted in different IP address
>> to be assigned). This results in 'ping $domain' not working
>> properly as it still pings the old IP address. Well, it might -
>> NSS plugin does not guarantee any order of addresses.
>>
>> To resolve this problem, we can sort the array just before
>> returning it to the caller (ping) so that the newer IP addresses
>> come before older ones.
>>
>> Reported-by: Andrea Bolognani <abologna@redhat.com>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
> ...
> 
>>       {
>>           "ip-address": "192.168.122.254",
>>           "mac-address": "52:54:00:3a:b5:0c",
>>           "hostname": "gentoo",
>> -        "expiry-time": 2000000000
>> +        "expiry-time": 2000000001
> 
> So, why is ^this specific change needed? In the test, you're expecting
> 192.168.122.[197-199], 192.168.122.199 has expiry time of 1900000000, so the
> test would still work.
> 
> 192.168.122.254 is only assumed in a gentoo test twice, but there are no other
> addresses to sort in those cases, so the expiry time in there should not matter
> right?
> 
> With the explanation of the above:
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
> 

Oh right, I don't understand how it got there. I'm removing it before 
pushing.

Thanks for the review!

Michal

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.