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 <sso...@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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
