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

[Simo Sorce]

On Mon, 2012-10-29 at 16:19 -0400, Rob Crittenden wrote:
> 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.

I will add a note I guess, where should I do it given this class is
abstract ? in base.py ?

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

Ah thanks, this was a copy/paste error, fixed in the patches I am
testing now.

Simo.


-- 
Simo Sorce * Red Hat, Inc * New York