[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[David Kupka]

On 08/19/2014 09:58 AM, Martin Kosek wrote:
> On 08/19/2014 09:05 AM, David Kupka wrote:
>> FreeIPA will use certmonger D-Bus API as discussed in this thread
>> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>>
>> This change should prevent hard-to-reproduce bugs like
>> https://fedorahosted.org/freeipa/ticket/4280
>
> Thanks for this effort, the updated certmonger module looks much better! This
> will help us get rid of the non-standard communication with certmonger.
>
> Just couple initial comments from me by reading the code:
>
> 1) Testing needs fixed version of certmonger, right? This needs to be spelled
> out right with the patch.
Yes, certmonger 0.75.13 and above should be fine according ticket 
https://fedorahosted.org/certmonger/ticket/36. Added to patch description.
>
> 2) Description text in patches is cheap, do not be afraid to use it and
> describe what you did and why. Link to the ticket is missing in the description
> as well:
Ok, increased verbosity a bit :-)
>
>> Subject: [PATCH] Use certmonger D-Bus API instead of messing with its files.
>>
>> ---
>
> 3) get_request_id API:
>
>>           criteria = (
>> -            ('cert_storage_location', dogtag_constants.ALIAS_DIR,
>> -             certmonger.NPATH),
>> -            ('cert_nickname', nickname, None),
>> +            ('cert_storage_location', dogtag_constants.ALIAS_DIR),
>> +            ('cert_nickname', nickname),
>>           )
>>           request_id = certmonger.get_request_id(criteria)
>
> Do we want to continue using the "criteria" object or should we rather switch
> to normal function options? I.e. rather using
>
> request_id = certmonger.get_request_id(cert_nickname=nickname,
> cert_storage_location=dogtag_constants.ALIAS_DIR)
>
> ? It would look more consistent with other calls. I am just asking, not insisting.
I've no preference here. It seems to be a very small change. Has anyone 
a reason to do it one way and not the other?
>
> 3) Starting function:
>
>> +    try:
>> +        ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'], skip_output=True)
>> +    except Exception, e:
>> +        root_logger.error('Failed to start certmonger: %s' % e)
>> +        raise e
>
> I see 2 issues related to this code:
> a) Do not call SYSTEMCTL directly. To be platform independent, rather use
> services.knownservices.messagebus.start() that is overridable by someone else
> porting to non-systemd platforms.
Is there anything that can't be done using ipalib/ipapython/ipaplatform?
> b) In this case, do not use "raise e", but just "raise" to keep the exception
> stack trace intact for better debugging.
Every day there's something new to learn about python or FreeIPA.
>
> Both a) and b) should be fixed in other occasions and places.
I found only one occurence of a) issue. Is there some hidden or are you 
talking about the whole FreeIPA project?
>
> 4) Feel free to add yourself to Authors section of this module. You refactored
> it greatly to earn it :-)
Done.
>
> Martin
>

-- 
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0008-2-Use-certmonger-D-Bus-API-instead-of-messing-with-its.patch
Type: text/x-patch
Size: 34905 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140819/5742349b/attachment.bin>