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.

...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.

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?

Created attachment 175916 [details] [review]
Don't create empty NTP configuration files (v2)

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.

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! :-)

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.