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

[Ana Krivokapic]

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.

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-akrivoka-0046-04-Properly-handle-non-existent-cert-files.patch
Type: text/x-patch
Size: 2752 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130718/f9535623/attachment.bin>