- 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.
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) ntpdate( ) does not stop ntpd before starting ntpdate ntpdate( ) returns TRUE if ntpdate is not available ntp_debian( ) returns TRUE if: ntpdate. disabled up.d/ntpdate. disabled don't exist sbin/update- rc.d ntp ", using_ntp ? "enable" : "disable", NULL); if-up.d/ ntpdate"
- _set_using_
- _set_using_
- _set_using_ntpd() returns TRUE if ntpd is not available
- _set_using_
(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/
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-
returns FALSE if errors returned while executing:
(a) g_strconcat ("/usr/
(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/
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.