On Fri, 15 Oct 2010 10:05:50 -0400
Rob Crittenden <rcrit...@redhat.com> wrote:

> Simo Sorce wrote:
> >
> > Instead of replacing the files altogether parse them and add only
> > the options we care about.
> >
> > For ntp.conf those are the server related options.
> > For sysconfig/ntpd we care of adding just -x and -g if missing
> >
> > Simo.
> >
> 
> nack, I don't think this will work.

my tests did work, but I haven't tried all possibilities indeed.

> A few comments on the python. You use some C-like syntax with match =
> 0, should probably be match = False (as you use elsewhere, with
> file_changed).
> 
> You don't need the string module to use split, so instead of:
> 
> opt = string.split(line, " ")
> 
> You can do:
> 
> opt = line.split(" ")
> 
> But what you really want, I think is:
> 
> opt = line.split()

ok, will change these.

> If you split on None it splits on white space, not a single space.
> The different is:
> 
>  >>> line = 'server  127.127.1.0     # local clock'
>  >>> line.split()
> ['server', '127.127.1.0', '#', 'local', 'clock']
>  >>> line.split(' ')
> ['server', '', '127.127.1.0', '', '', '', '', '#', 'local', 'clock']
> 
> Note the extra space after server in the last entry. This would cause 
> your conditional to fail (if opt[1] == srv).

Right, thanks for catching this, my python got a bit rusty in the last
few months :)

> I'm not sure your loop for srv actually does the right thing. I
> wonder if you wanted to set match = 0 within the for loop.

Well, according to my testing it does.
But I'll re-check.

Simo.

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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to