Comment 4 for bug 1153633

Reply to some of the points raised:

- Doesn't daemonize as usual -- is this intentional to ease upstart
  tracking the service?

It's not required becaues the service is only ever started as a result of D-Bus activation. D-Bus is okay with that.

- running_in_chroot() is liable to not function reliably in new namespaces;
  expect this to work only for traditional chroot environments
- detect_container() may give silly results if kernel command line includes
  "container=..." string or if the userns namespace started init with funny
  environment variables

This code is copied straight out of upstream systemd sources -- I'm trying to emulate what they do here.

- _set_using_ntpdate() uses '/bin/mv -f' rather than rename(2)
- _set_using_ntpdate() does not stop ntpd before starting ntpdate
- _set_using_ntpdate() returns TRUE if ntpdate is not available
- _set_using_ntpd() returns TRUE if ntpd is not available
- _set_using_ntp_debian() returns TRUE if:
  (a) ntpd doesn't exist and ntpdate doesn't exist
  (b) ntpd doesn't exist and ntpdate started once
  (c) ntpd doesn't exist and if-up.d/ntpdate and if-up.d/ntpdate.disabled
      don't exist
  (d) ntpd was (re)started or stopped and ntpdate doesn't exist
  (e) ntpd was (re)started or stopped and ntpdate started once
  (f) ntpd was (re)started or stopped and if-up.d/ntpdate and
      if-up.d/ntpdate.disabled don't exist
  returns FALSE if errors returned while executing:
  (a) g_strconcat ("/usr/sbin/update-rc.d ntp ", using_ntp ? "enable" : "disable", NULL);
  (b) g_strconcat ("/usr/sbin/service ntp ", using_ntp ? "restart" : "stop", NULL);;
  (c) "/bin/mv -f "NTPDATE_DISABLED" "NTPDATE_ENABLED;
  (d) "/bin/mv -f "NTPDATE_ENABLED" "NTPDATE_DISABLED;
  (e) "/etc/network/if-up.d/ntpdate"

The NTP code is gross but we already have it in Ubuntu as part of the gnome-settings-daemon datetime mechanism. It's true that this could use some cleaning up, but I wanted to go with 'tried and tested' rather than rewriting it too much.

- return value of detect_virtualization() is ignored

This is a good catch. I ignored the return value because in the case of failure I was just going to return the empty string, but on closer reading of the code I now see that detect_virtualization() doesn't actually set the return value in the failure case. I'll fix this by initialising to "".

- shim_get_property allocates memory with g_variant_new(), appears leaked.
  Is this the intentional way to use DBus?

From the GDBus documentation for the callback type of this function:

Returns :

A GVariant with the value for property_name or NULL if error is set. If the returned GVariant is floating, it is consumed - otherwise its reference count is decreased by one.

I'll fix up the virtualisation thing, but I think we should avoid refactoring the NTP code for now and clean it up after raring.