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

[Jan Cholasta]

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

-- 
Jan Cholasta