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

[Martin Kosek]

On 07/18/2013 01:28 PM, Jan Cholasta wrote:
> 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
> 

Pushed to mater, ipa-3-2.

Martin