[Freeipa-devel] [PATCHES 681-682] cert: speed up cert-find, do not crash on invalid data in cert-find

[Martin Basti]

On 17.08.2016 10:27, Jan Cholasta wrote:
> On 16.8.2016 17:01, Martin Basti wrote:
>>
>>
>> On 16.08.2016 17:00, Pavel Vomacka wrote:
>>>
>>>
>>> On 08/12/2016 08:29 AM, Jan Cholasta wrote:
>>>> On 11.8.2016 19:43, Martin Basti wrote:
>>>>>
>>>>>
>>>>> On 11.08.2016 16:09, Jan Cholasta wrote:
>>>>>> On 11.8.2016 14:27, Martin Basti wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 01.08.2016 10:27, Jan Cholasta wrote:
>>>>>>>> On 1.8.2016 10:19, Jan Cholasta wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> the attached patches fix
>>>>>>>>> <https://fedorahosted.org/freeipa/ticket/6098>
>>>>>>>>> and <https://fedorahosted.org/freeipa/ticket/6150>.
>>>>>>>>
>>>>>>>> Self-NACK, proper patches attached.
>>>>>>>>
>>>>>>>> Honza
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> IMHO this is caused by your patches, test_cert_plugin.py:
>>>>>>
>>>>>> Fixed.
>>>>>>
>>>>>> Updated and rebased patches attached.
>>>>>>
>>>>> Hello,
>>>>>
>>>>> It works for me, but:
>>>>>
>>>>> 1)
>>>>> Is this py2/3 compatible?
>>>>> ra_obj = ra.get_certificate(str(serial_number))
>>>>
>>>> I don't see why not. Do you have any particular incompatibility in 
>>>> mind?
>>>>
>>>>>
>>>>> 2)
>>>>> Are you sure you need tuple() here?
>>>>> +                for key in tuple(six.iterkeys(result)):
>>>>
>>>> Yes, I'm modifying `result` inside the loop.
>>>>
>>>> I don't need the six.iterkeys() though.
>>>>
>>>>>
>>>>> 3)
>>>>>       if cert is not None:
>>>>>             filter = ldap.make_filter_from_attr('usercertificate',
>>>>> value)
>>>>>             filters.append(filter)
>>>>>
>>>>> Variable "value" may be referenced before assignment
>>>>
>>>> Right, it should be `cert`, not `value`.
>>>>
>>>>>
>>>>> I haven't tested performace improvements yet, and it is quite big
>>>>> change
>>>>> so I will continue with testing tomorrow.
>>> I tested performance improvements and cert-find in webui with 71
>>> certificates took about 10 seconds before these patches, now it is
>>> about 400ms (even with more certs) . So works for me perfectly. From
>>> CLI it took about 10 seconds now around 4 seconds.
>>
>> Please fix reported issues by me, and we can push it.
>
> Here you go.
>

ACK

master:
* c718ef058847bb39e78236e8af0ad69ac961bbcf cert: speed up cert-find
* 8ad03259fe770b222e70286fd00c3416b4ed197d cert: do not crash on invalid 
data in cert-find