[Freeipa-devel] [PATCH] 1067 clear out certmonger requests

[Jan Cholasta]

On 31.10.2012 16:28, Rob Crittenden wrote:
> Jan Cholasta wrote:
>> On 29.10.2012 20:11, Rob Crittenden wrote:
>>> Jan Cholasta wrote:
>>>> Hi,
>>>>
>>>> On 24.10.2012 21:22, Rob Crittenden wrote:
>>>>> If uninstall fails in certain ways it is possible that some
>>>>> certificates
>>>>> could still be tracked by certmonger (even if the NSS database is now
>>>>> gone). This will loop through the directories we care about and warn
>>>>> the
>>>>> user if there is anything left over.
>>>>>
>>>>> I added some basic test instructions to the ticket.
>>>>>
>>>>> rob
>>>>>
>>>>
>>>> You should check the return value of find_request_value, it can be None
>>>> in case of error.
>>>>
>>>> I would prefer if you used "os.path.join(REQUEST_DIR, file)" instead of
>>>> "'%s/%s' % (REQUEST_DIR, file)".
>>>>
>>>> There is a trailing whitespace in the patch on line 75.
>>>>
>>>> Honza
>>>>
>>>
>>> fixed.
>>>
>>> rob
>>
>> This will still break if find_request_value returns None:
>>
>> +    for file in fileList:
>> +        rv = find_request_value(os.path.join(REQUEST_DIR, file),
>> +                                'cert_storage_location')
>> +        if rv:
>> +            rv = os.path.abspath(rv)
>> +        if rv.rstrip() == dir:
>> +            id = find_request_value(os.path.join(REQUEST_DIR, file),
>> 'id').rstrip()
>> +            if id is not None:
>> +                reqid.append(id)
>>
>> I would suggest to do it like this instead:
>>
>> +    for file in fileList:
>> +        rv = find_request_value(os.path.join(REQUEST_DIR, file),
>> +                                'cert_storage_location')
>> +        if rv is None:
>> +            continue
>> +        rv = os.path.abspath(rv).rstrip()
>> +        if rv != dir:
>> +            continue
>> +        id = find_request_value(os.path.join(REQUEST_DIR, file), 'id')
>> +        if id is not None:
>> +            reqid.append(id.rstrip())
>>
>> Honza
>>
>
> OK, great suggestion, sorry I didn't grok it sooner. Updated patch
> attached.
>
> rob

You missed one spot:

+        id = find_request_value(os.path.join(REQUEST_DIR, file), 'id')
+        if id is not None:
+            reqid.append(id.rstrip())

Also there is a trailing whitespace on line 73 of the patch.

Once you fix this, ACK.

Honza

-- 
Jan Cholasta