[libvirt] [PATCH] Don't drop expired lease while reading custom leases file

[Michal Privoznik]

On 30.09.2016 14:22, Nehal J Wani wrote:
> On Fri, Sep 30, 2016 at 5:47 PM, Michal Privoznik <mprivozn at 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