After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 449267 - Time and Date Settings creates empty /etc/ntp.conf
Time and Date Settings creates empty /etc/ntp.conf
Status: RESOLVED FIXED
Product: gnome-system-tools
Classification: Deprecated
Component: time-admin
2.18.x
Other Linux
: Normal normal
: ---
Assigned To: Carlos Garnacho
Carlos Garnacho
Depends on:
Blocks:
 
 
Reported: 2007-06-19 20:48 UTC by Josselin Mouette
Modified: 2010-12-07 06:35 UTC
See Also:
GNOME target: ---
GNOME version: 2.17/2.18


Attachments
Don't create empty NTP configuration files (892 bytes, patch)
2010-12-06 11:44 UTC, Colin Watson
none Details | Review
Don't create empty NTP configuration files (v2) (908 bytes, patch)
2010-12-06 13:18 UTC, Colin Watson
none Details | Review

Description Josselin Mouette 2007-06-19 20:48:49 UTC
[ forwarded from http://bugs.debian.org/397648 by Olaf van der Spek ]

Time and Date Settings appears to creates an empty /etc/ntp.conf if the ntp package isn't installed.                                                                               
Installing that package then asks about a user-modified conf file.
Comment 1 Colin Watson 2010-12-06 11:44:17 UTC
Created attachment 175912 [details] [review]
Don't create empty NTP configuration files

This is very simple.  Here's a patch.  It doesn't make anything any worse, since it's not as if system-tools-backends was managing to actually set any configuration in a previously non-existent file anyway.
Comment 2 Milan Bouchet-Valat 2010-12-06 12:37:27 UTC
...and yet another downstream patch in Ubuntu. (Sigh)

Thanks for the patch, but you could have waited for my reply before adding that downstream.

There's a problem with it: of course, the stb don't create a valid ntp.conf file if it was empty before, but if we don't create that file, we won't save the servers the user might add. Not sure what is the worse. I'd rather improve the patch to avoid creating the file only if (it doesn't exist) && (the servers list is empty). This will work for most cases, and the list will be saved if the user actually provided a list.
Comment 3 Colin Watson 2010-12-06 13:18:26 UTC
Sorry I didn't wait.  I'm happy to amend the downstream patch as needed.

The problem you describe exists already.  The NTP backend only edits existing lines in /etc/ntp.conf - it never adds new lines.  As such, no matter whether the list of servers is empty or not, it will create an empty file if the file didn't previously exist.  Creating an empty file doesn't save the provided list of servers any more than not creating an empty file does, so this patch does not make that problem any worse.

My understanding is that generally if you're actually setting servers then g-s-t will have gone to the effort to install the ntp package, which in Debian and Ubuntu ships a real /etc/ntp.conf.  As far as I could see the path involving an empty file only happens if you hit the "Synchronise now" button rather than doing full NTP configuration, and in that case there won't be any servers to configure anyway.

I feel rather cautious about creating an ntp.conf when none exists already.  It seems to me that that would create substantial confusion when you get around to installing the ntp package.  Right now, it's possible for the ntp package to say "oh, ntp.conf seems to be empty, I'll just delete it and install my own".  This logic is substantially trickier when there's real content in the configuration file, and I'm not convinced that Debian's ntp maintainers would want to accept a change like that.  In general I do not think that offering the ability to set NTP servers before installing the package that creates ntp.conf is a good thing to do.

Still, your suggestion to change the conditional to (it doesn't exist) && (the servers list is empty) is fair enough.  How about the attached patch?
Comment 4 Colin Watson 2010-12-06 13:18:57 UTC
Created attachment 175916 [details] [review]
Don't create empty NTP configuration files (v2)
Comment 5 Colin Watson 2010-12-06 13:21:52 UTC
Hmm.  I think I misread the logic in Time/NTP.pm - I missed the "foreach $i (@$value)" block at the end.  In that case I withdraw my comment that the bug exists already, and amending the conditional definitely makes sense, although I still think that situations where the backend does manage to create this file from scratch will cause practical problems later.
Comment 6 Milan Bouchet-Valat 2010-12-06 17:36:40 UTC
Yes, the current situation is not really nice, but I consider the issue of not writing data to /etc/ntp.conf to be something for the GUI, and the backends to be responsible for writing that data when asked. They really can't do better than that anyway, if the GUI hasn't taken care of installing NTP before configuring it. With the patch, asking the backends to run ntpdate will always work without creating that silly empty file, so it's OK.

I've checked that the patch works, added a comment, fixed styling and pushed as 3a6c2f9. This should be released with 2.10.2 before the end of the Natty cycle. Thanks for the help - I seldom get contributions from people that both care about the stb and aren't afraid by perl! :-)
Comment 7 Colin Watson 2010-12-07 06:35:30 UTC
Thanks!  I've synced up the Ubuntu package with the commit as amended by you, and as you say we should pick up 2.10.2 naturally anyway.