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

[Martin Kosek]

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.

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:

> 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.

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.
b) In this case, do not use "raise e", but just "raise" to keep the exception
stack trace intact for better debugging.

Both a) and b) should be fixed in other occasions and places.

4) Feel free to add yourself to Authors section of this module. You refactored
it greatly to earn it :-)

Martin