Nicolas Williams wrote: > On Wed, Apr 22, 2009 at 03:30:48PM -0400, Brian Utterback wrote: >> http://cr.opensolaris.org/~blu/ntpv4/sfwnv-webrev/ > > - $SRC/cmd/ntpd/Makefile:41 > > 41 CONFIGURE_OPTIONS += --with-openssl-libdir=/usr/sfw/lib > > a) OpenSSL has moved out of /usr/sfw and into /usr. Don't forget to > fix LD_OPTIONS too...
Good point. It was in /usr/sfw when I started, but of course with it in /lib having the wrong -L didn't hurt. I'll fix that. > b) You didn't mention OpenSSL in the ARC case. I think you still > need to sign a contract in order to use it. Actually, it is listed in the checklist. It is linking with libcrypto.so, which is listed in the imported interfaces list and is shown as a contracted interface. So yes, we will need a contract. > > - $SRC/cmd/ntpd/Makefile:43 > > 43 CONFIGURE_OPTIONS += --enable-debugging > > 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. > > - Just out of curiosity, what configure options are you NOT enabling? Oh, there are a lot. NTP has a lot of features and can be built on a lot of platforms. But in the grand scheme of things, we are in fact enabling pretty much everything we can. > > - I never knew about .patched/PATCHES thing. Cool. I thought so too. When I started this, it didn't exist, so for the last week or so before posting these webrevs, I ripped out all of the Makefile stuff I had before and converted it to use the newer, simpler stuff. Cut the Makefile in half. > > What's not cool, IMO, is that it doesn't use unified diffs. Such is life. May it could, since it is just calling gpatch. > > - $SRC/cmd/ntpd/Patches/etcfix.patch > > 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. > > - $SRC/cmd/ntpd/Patches/noextra.patch > > 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. > > Why remove ntpsnmpd when there's already a ./configure > --without-ntpsnmpd option (and you're using it)? I found that --without-ntpsnmpd wasn't enough, it still found the SNMP agent paths and tried to build ntpsnmpd even though it would not install it. I left the without in the config opts because I wanted to make it obvious that ntpsnmpd is not installed. I plan to do a follow on case later to add this feature, but it is relatively new and I couldn't get it to work right yet. > > What about the others? Are there --without-* options for them? > Or is this about build times (looking at the fact that you're having > to modify SUBDIRS I'm guessing that's what this is about)? Not everything had without options. And, it turns out that there are a couple of subdirs that have their own config scripts that are run. By removing them this why it drastically reduces the build time, as you noted. The actual compile time for NTP is tiny compared to the config script time. > > - 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. > > - $SRC/cmd/ntpd/Patches/ntpwait.patch > > Is there a need to update the comment at 23? I don't think it is really needed, but if you want for me to fix it, I am happy to. This patch has been submitted to the NTP project. > > - $SRC/cmd/ntpd/Solaris/ntp.server > > It'd be nice if we had a page where people could report what time > source hardware works for them on Solaris... We can do that, I think. I can set up a page at the NTP.org wiki and point to it. > > References to VME... wow! :) Yes, I know. The VME refclock driver is not compiled into the xntpd in Solaris now, but over time I have found that assumptions about what will not be needed have often been found to be wrong. There have been several bugs reported against Solaris over the years complaining about this or that driver that wasn't configured for what probably seemed like a good idea at the time. I would prefer to just enable everything and let the the lack of the hardware prevent a particular driver from being used. > > - 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. > > - $SRC/pkgdefs/SUNWntpu/depend > > What about a dependency on SUNWopenssl? Good point. I'll add SUNWopensslr. > > - $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? > > - $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. > > - $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? > > > - $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. -- blu "Mark my words, nanotechnology is going to be huge!" ---------------------------------------------------------------------- Brian Utterback - Solaris RPE, Sun Microsystems, Inc. Ph:877-259-7345, Em:brian.utterback-at-ess-you-enn-dot-kom
