[Freeipa-devel] [PATCH] 0046 Properly handle non-existent CA file

[Jan Cholasta]

On 18.7.2013 13:02, Ana Krivokapic wrote:
> On 07/18/2013 09:25 AM, Jan Cholasta wrote:
>> On 17.7.2013 19:43, Ana Krivokapic wrote:
>>> On 07/17/2013 06:04 PM, Jan Cholasta wrote:
>>>> On 17.7.2013 17:39, Ana Krivokapic wrote:
>>>>> On 07/17/2013 04:57 PM, Jan Cholasta wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 17.7.2013 16:38, Ana Krivokapic wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> This patch addresses ticket https://fedorahosted.org/freeipa/ticket/3785.
>>>>>>>
>>>>>>
>>>>>> NACK, this results in an unnecessarily ugly error message "[Errno 2] No such
>>>>>> file or directory: 'file'".
>>>>>>
>>>>>> I would suggest something like this instead:
>>>>>>
>>>>>> except IOError as e:
>>>>>>        raise ScriptError("Failed to open %s: %s" % (ca_cert_name, e.strerror))
>>>>>
>>>>> Fixed.
>>>>
>>>> Hmm, seeing how RuntimeError is used for this kind of thing in import_pkcs12,
>>>> I think it would make sense to catch the IOError right in import_pem_cert and
>>>> re-raise it as RuntimeError and then handle that RuntimeError in check_pkcs12
>>>> (sorry for misleading you into doing something else in my previous mail).
>>>
>>> I don't see much value in doing that - it just adds complexity. I would rather
>>> leave it as it is.
>>
>> All the other NSSDatabase methods raise RuntimeErrors when something goes
>> wrong, including I/O errors. IMO having consistent API is preferable to saving
>> 2 lines of code.
>>
>>>>
>>>>>>
>>>>>> Can you please also check what happens if you pass non-existent filename to
>>>>>> --dirsrv_pkcs12 and --http_pkcs12?
>>>>>>
>>>>>> Honza
>>>>>>
>>>>>
>>>>> I added a more specific error message to cover these cases.
>>>>
>>>> Can you please also add it to find_root_cert_from_pkcs12?
>>>
>>> Done, thanks for catching that.
>>>>
>>>>>
>>>>> Updated patch attached.
>>>>>
>>>>
>>>> Honza
>>>>
>>>
>>> Updated patch is attached.
>>>
>>
>> Honza
>>
>
> Ok, fixed.
>

Thanks! ACK.

Honza

-- 
Jan Cholasta