[libvirt] [PATCH] Don't drop expired lease while reading custom leases file
Libvirt, on its own, shouldn't decide whether an expired lease should stay in the custom leases database or not. It should rather rely on the 'DEL' event from dnsmasq. --- src/util/virlease.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/util/virlease.c b/src/util/virlease.c index 920ebaf..b49105d 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -57,7 +57,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, { char *lease_entries = NULL; virJSONValuePtr leases_array = NULL; - long long currtime = 0; long long expirytime; int custom_lease_file_len = 0; virJSONValuePtr lease_tmp = NULL; @@ -66,8 +65,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, size_t i; int ret = -1; - currtime = (long long) time(NULL); - /* Read entire contents */ if ((custom_lease_file_len = virFileReadAll(custom_lease_file, VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, @@ -109,11 +106,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, _("failed to parse json")); goto cleanup; } - /* Check whether lease has expired or not */ - if (expirytime < currtime) { - i++; - continue; - } /* Check whether lease has to be included or not */ if (ip_to_delete && STREQ(ip_tmp, ip_to_delete)) { -- 2.7.4
On 28.09.2016 21:39, Nehal J Wani wrote:
Libvirt, on its own, shouldn't decide whether an expired lease should stay in the custom leases database or not. It should rather rely on the 'DEL' event from dnsmasq. --- src/util/virlease.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/src/util/virlease.c b/src/util/virlease.c index 920ebaf..b49105d 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -57,7 +57,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, { char *lease_entries = NULL; virJSONValuePtr leases_array = NULL; - long long currtime = 0; long long expirytime; int custom_lease_file_len = 0; virJSONValuePtr lease_tmp = NULL; @@ -66,8 +65,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, size_t i; int ret = -1;
- currtime = (long long) time(NULL); - /* Read entire contents */ if ((custom_lease_file_len = virFileReadAll(custom_lease_file, VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, @@ -109,11 +106,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, _("failed to parse json")); goto cleanup; } - /* Check whether lease has expired or not */ - if (expirytime < currtime) { - i++; - continue; - }
/* Check whether lease has to be included or not */ if (ip_to_delete && STREQ(ip_tmp, ip_to_delete)) {
Well, I like this patch. I really do. I always felt like read() function should not do any validation and just present called the data. However, this "breaks" the nsstest. We have two options here: either merge the other patch of yours that fixes the NSS module, or temporarily just "fix" the nss test (so that we can undo the change while merging the other patch). Which one of those two approaches do you like more? Michal
On Fri, Sep 30, 2016 at 5:47 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 28.09.2016 21:39, Nehal J Wani wrote:
Libvirt, on its own, shouldn't decide whether an expired lease should stay in the custom leases database or not. It should rather rely on the 'DEL' event from dnsmasq. --- src/util/virlease.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/src/util/virlease.c b/src/util/virlease.c index 920ebaf..b49105d 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -57,7 +57,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, { char *lease_entries = NULL; virJSONValuePtr leases_array = NULL; - long long currtime = 0; long long expirytime; int custom_lease_file_len = 0; virJSONValuePtr lease_tmp = NULL; @@ -66,8 +65,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, size_t i; int ret = -1;
- currtime = (long long) time(NULL); - /* Read entire contents */ if ((custom_lease_file_len = virFileReadAll(custom_lease_file, VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, @@ -109,11 +106,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, _("failed to parse json")); goto cleanup; } - /* Check whether lease has expired or not */ - if (expirytime < currtime) { - i++; - continue; - }
/* Check whether lease has to be included or not */ if (ip_to_delete && STREQ(ip_tmp, ip_to_delete)) {
Well, I like this patch. I really do. I always felt like read() function should not do any validation and just present called the data. However, this "breaks" the nsstest. We have two options here: either merge the other patch of yours that fixes the NSS module, or temporarily just "fix" the nss test (so that we can undo the change while merging the other patch). Which one of those two approaches do you like more?
Michal
I like the approach where the NSS module is fixed first, and then virLeaseReadCustomLeaseFile() is fixed. This way, we won't break anything in between and won't have to change anything temporarily. -- Nehal J Wani
On 30.09.2016 14:22, Nehal J Wani wrote:
On Fri, Sep 30, 2016 at 5:47 PM, Michal Privoznik <mprivozn@redhat.com> wrote:
On 28.09.2016 21:39, Nehal J Wani wrote:
Libvirt, on its own, shouldn't decide whether an expired lease should stay in the custom leases database or not. It should rather rely on the 'DEL' event from dnsmasq. --- src/util/virlease.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/src/util/virlease.c b/src/util/virlease.c index 920ebaf..b49105d 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -57,7 +57,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, { char *lease_entries = NULL; virJSONValuePtr leases_array = NULL; - long long currtime = 0; long long expirytime; int custom_lease_file_len = 0; virJSONValuePtr lease_tmp = NULL; @@ -66,8 +65,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, size_t i; int ret = -1;
- currtime = (long long) time(NULL); - /* Read entire contents */ if ((custom_lease_file_len = virFileReadAll(custom_lease_file, VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, @@ -109,11 +106,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, _("failed to parse json")); goto cleanup; } - /* Check whether lease has expired or not */ - if (expirytime < currtime) { - i++; - continue; - }
/* Check whether lease has to be included or not */ if (ip_to_delete && STREQ(ip_tmp, ip_to_delete)) {
Well, I like this patch. I really do. I always felt like read() function should not do any validation and just present called the data. However, this "breaks" the nsstest. We have two options here: either merge the other patch of yours that fixes the NSS module, or temporarily just "fix" the nss test (so that we can undo the change while merging the other patch). Which one of those two approaches do you like more?
Michal
I like the approach where the NSS module is fixed first, and then virLeaseReadCustomLeaseFile() is fixed. This way, we won't break anything in between and won't have to change anything temporarily.
Ah good point. In that case do you mind fixing all the small nits I've raised and sending these patches not separately but in a series in the correct order? Michal
participants (2)
-
Michal Privoznik -
Nehal J Wani