GNOME Bugzilla – Bug 644821
Debian/Ubuntu time support
Last modified: 2011-05-18 16:59:01 UTC
Hello! I recently finished patching gnome-settings-daemon to support Debian/Ubuntu style ntp support. This involved a slight change in how we query/configure ntp as well as support for querying/configuring ntpdate alongside ntpd. This is useful because in Ubuntu at least, ntpd is not installed by default, but ntpdate is. So... How best to craft this patch for you? I have a modified .c file, but how were you maintainers envisioning supporting multiple distros? #ifdefs, runtime querying, some sort of extensible plugin system?
Run-time is best, thanks.
Ah, yes. I see you already have some of Debian detection. Hadn't looked at trunk in a bit. OK, will work on further patch for ntpdate support.
Created attachment 183570 [details] [review] look at ntpdate too This patch configures Debian/Ubuntu's use of ntpdate too. If enabling ntp, both ntpd and ntpdate are turned on (ntpdate via if-up.d script). If disabling, both are turned off. If either is active, ntp is reported as 'on'. The diff is a bit complicated because I had to rework the flow a bit so that errors in enabling or the inability to enable one didn't affect the other.
That looks wrong. You can't have ntpdate *and* ntpd running at the same time. If ntpdate is the one that's installed by default, then we should enable just ntpdate, otherwise just ntpd. In fact, ntpdate is a one-shot application, whereas ntpd is a daemon. When ntpd is running, ntpdate can't be run.
In Debian, ntpdate installs an if-up.d script that runs ntpdate whenever an Internet connection comes up. This script pauses ntpd if it's running. That is, Debian has two completely independent ways to sync with Internet time using ntp, and both need to be turned on or off together. Ubuntu installs ntpdate by default, not ntpd. Though the user can easily install ntpd and have both enabled. So when a user says "I want manual time", if either nptd or the ntpdate if-up.d script is still active, obviously, they won't really get manual time.
Review of attachment 183570 [details] [review]: I can't see any gross errors in the patch, but the code could really do with being split up into functions. Also, do you really want to return errors if ntpd is correctly enabled, but not ntpdate?
I think it's appropriate as long as the error is from actually trying to enable ntpdate, but not if ntpdate isn't installed in the first place. But I don't feel strongly about it. I'll work on a function-oriented version.
Created attachment 183977 [details] [review] Function-oriented version How's this?
Michael, I've split the code so that all the distribution specific code lives in separate files (which I think is much cleaner). Once you've updated your patch to make use of the split files, you can commit your changes on the master branch as you wish (as long as you're only making trivial changes to the main file, for example adding a check for Ubuntu's release file).
K, will gladly do so, but not for over a week as I'm going on vacation. Just FYI.
commit 2958ce3f9035e3dc158d4390c63e4e9415296aee Author: Michael Terry <michael.terry@canonical.com> Date: Wed May 18 12:41:58 2011 -0400 datetime: Fix ntp logic on Debian to include ntpdate as well as ntpd https://bugzilla.gnome.org/show_bug.cgi?id=644821
(In reply to comment #11) > commit 2958ce3f9035e3dc158d4390c63e4e9415296aee > Author: Michael Terry <michael.terry@canonical.com> > Date: Wed May 18 12:41:58 2011 -0400 > > datetime: Fix ntp logic on Debian to include ntpdate as well as ntpd > > https://bugzilla.gnome.org/show_bug.cgi?id=644821 FWIW, it's likely that that portion of code will go away, to be replaced by a D-Bus interface provided by systemd. I would certainly expect systemd not being required as the init system for this to work, so it would just be another package to include.
Sure, I've been reading that systemd thread with interest too. :) But presumably this code can just be moved into whatever dbus daemon gets created for it and not rewritten again.