[Freeipa-devel] [PATCH 2/3] Save service name on service startup

[Rob Crittenden]

Simo Sorce wrote:
> On Fri, 2012-10-26 at 16:13 -0400, Rob Crittenden wrote:
>> Simo Sorce wrote:
>>> From: Simo Sorce <ssorce at redhat.com>
>>>
>>> This is done as a default action of the ancestor class so that no matter what
>>> platform is currently used this code is always the same and the name is the
>>> wellknown service name.
>>> This information will be used by ipacl to stop only and all the services that
>>> have been started by any ipa tool/install script
>>
>> I think in the start method I'd rather test to see if the file exists
>> before trying to open it and do something (logging, return error,
>> something) if an exception is hit.
>
> Why ?
> If there is any fatal exception it will be caught in the following open().

That's true, just seems kinda pointless to ignore an error only to try 
it again. That's fine, not a deal breaker to me.

>> This will help us out of there are permission, disk space or other
>> problems writing this file.
>
> Yeah but we get that in the next exception anyway.
>
>> Similarly I wonder if there should be a try/except around updating the file.
>
> I've let them explicitly out of try blocks because I want the method to
> fail and report to the caller, would you rather use explict try:/except:
> and a then a raise() again ?

Ok, I think the method docs should state that any exceptions are left 
for the caller to handle.

>> Maybe not a big deal but this patch isn't testable by itself because
>> patch 3 contains the spec addition of /var/run/ipa.
>
> True, should I move the spec file change to this patch ?

No, not a big deal. Once it's all committed it's all good.

The shutdown message when there is no services file is a bit strange though:

# ipactl stop
Failed to read data from Directory Service: [Errno 2] No such file or 
directory: '/var/run/ipa/services.list'
Shutting down

rob