On Fri, Apr 24, 2009 at 12:36:31PM -0400, Brian Utterback wrote: > Nicolas Williams wrote: > > What does --enable-debugging do? > > Uh, enable debugging? 8-) It compiles in a set of printf statements > that are enabled by a command line switch. The current xntpd has this > switched on as well, and has proven invaluable in supporting NTP.
I was trying to see if enabling debugging would have undesired effects (as it did in the case of SQLite3). > > Does /var/ntp need to be documented? > > Hmm. It is listed in the example configure files and in the ntpd man > page, but there is nothing special about the path; the user could > change it in the config files. Good enough. > > Could you patch configure.ac and run autoconf instead? > > Maybe, but the environment where the tarball is created is a bit > tricky to get right. It needs particular versions of autoconf and it > also needs autogen. I did try to do it about 8 months ago and wasn't > able to get it to work. So, I would hate to spend time on getting it > to work, only to find that the requirements changed some time down the > line. Understood. It might be a good idea to include a patch for configure.ac in addition to the one for configure, but don't run autoconf. The idea: to help the next maintainer. > > - Could the mDNS registration/unregistration not be done entirely in > > the SMF start/stop methods? > > No. The mdns daemon keeps track of its registrants by keeping a socket > open to the process that registered the service. When that process > exits, the service is de-registered. As it happens, the patch I have > here was submitted to the NTP project and the latest tarball has the > fix, so the next webrev will not need the mdns patch. Ah, got it. (It'd be nice if the mDNS stack had an option to not require this -- the SMF stop method could be used instead to handl unregistration.) > > - Could the start method use ppriv(1) to run ntpd with even fewer > > privs? I understand why the start method needs proc_fork,proc_exec, > > but surely ntpd does not (right?). > > The ntpd forks a child during daemonization, and it does name service > look up asynchronously by forking. I think that the only one we could > really get rid of would be proc_exec. I think that I would prefer to > make the NTP project code base Solaris priv aware. It is already sort > of Linux Caps aware. If you make it priv aware then you can drop everything but the sys_time (or do you need net_privaddr even after initializing?), and setgid/setuid(noaccess or daemon). That could be a follow-on RFE. > > - $SRC/cmd/ntpd/Solaris/ntp.xml:61 > > > > 61 timeout_seconds='1800'> > > > > 1800 seconds?!! > > That is the number currently used, I just carried it over. Does seem a > tad long, huh? In the current xntpd code, it only comes into play when > ntpdate can't find a server, since xntpd daemonizes immediately. The > "-w" option Sun added to ntpdate quickly moves to a 300 second retry, > so I guess it was a reasonable value. > > With NTPv4, we don't have a ntpdate wait, of course. But we do now > have a ntp-wait option. In general, it shouldn't take more than 6 > minutes to get to first sync of the clock, but that could be longer if > the naming service isn't online yet. And it could be much shorter. How > about 600 instead? I separately argued that you should throw the ntpdate (or ntpd -gq) back in. I'm not sure what a good timeout would be. A long timeout would mask issues in starting ntpd. We can revisit this later, but I'm thinking that the ntp-wait should be in a separate, transient service with a long timeout. > > - $SRC/cmd/ntpd/Solaris/ntp:52-53 > > > > 52 # If this is a start then set the panic gate. Not if it is a refresh. > > 53 [ "$1" = "start" ] && args="-g" > > > > But ntp.xml defines a restart method using the same script as the > > start method. Which is it, a restart or a refresh? > > As the code reflects. If it is anything but a "start", don't use the > "-g". So, restart, refresh, anything but "start", means any offset of > more than 17 minutes will put the service into maintenance. But you're looking for "first start", right? You can't know if this is a "first start." But I guess that's OK. > > - $SRC/cmd/ntpd/Solaris/ntp > > > > Any chance this could be re-written to be a ksh script? > > I could be I suppose. Looking at the startup methods already in > /lib/svc/method, most use /sbin/sh, some use /usr/bin/ksh and some use > /usr/sbin/ksh93. Is there a reason to use ksh? Send me a copy, I'll re-write it for you. > > - $SRC/cmd/ntpd/Solaris/ntp > > > > Values produced by svcprop need some validation/checking. Consider > > these lines: > > > > 87 # Set up debugging. > > 88 deb=`svcprop -c -p config/debuglevel $SMF_FMRI` > > 89 debfile=`svcprop -c -p config/debugfile $SMF_FMRI` > > ... > > 94 /usr/lib/inet/ntpd $args --set-debug-level=$deb >$debfile & > > > > What if $debfile is set to something nasty? Privilege escalation? > > > >Nico > > Ouch. Really good point. I can't think of a way to validate this > adequately. I this one needs to be hard coded to /var/ntp/ntp.debug. Hmmm, why do you need that when SMF captures the stdout and stderr of the service into a per-service log file anyways? Nico --
