libc: clean up 'clock_gettime' handling

This patch untangles the dependency of VFS operations that need RTC
information from the 'clock_gettime' libc function that must never be
called from the libc kernel context.

- The 'Rtc' class uses the VFS directly for reading the rtc file instead
  of relying on libc functions.

- The 'Rtc' instance has become part of the 'Kernel' instead of
  being construced as a side effect of the first call of
  'clock_gettime'.

- Changed 'Rtc::read' to return a timespec value, which has a higher
  precision than the formerly used time_t value.

- The 'Rtc::read' returns a value with the relative 'current_time'
  already applied. The former handling of subsequent rtc-value
  updates has been rewritten to become more logical.

- The 'Vfs_plugin' no longer calls 'clock_gettime' but the new
  kernel-level 'Current_real_time' interface.

Issue #2635

repos/libports/src/lib/libc/internal/current_time.h

@@ -26,6 +26,13 @@ namespace Libc {
 	{
 		virtual Duration current_time() = 0;
 	};
+
+	struct Current_real_time : Interface
+	{
+		virtual bool has_real_time() const = 0;
+
+		virtual timespec current_real_time() = 0;
+	};
 }
 
 #endif /* _LIBC__INTERNAL__CURRENT_TIME_H_ */

repos/libports/src/lib/libc/internal/init.h

@@ -33,6 +33,7 @@ namespace Libc {
 	struct Monitor;
 	struct Select;
 	struct Current_time;
+	struct Current_real_time;
 	struct Clone_connection;
 	struct Kernel_routine_scheduler;
 	struct Watch;
@@ -101,7 +102,7 @@ namespace Libc {
 	 * Init timing facilities
 	 */
 	void init_sleep(Suspend &);
-	void init_time(Current_time &, Rtc_path const &, Watch &);
+	void init_time(Current_time &, Current_real_time &);
 
 	/**
 	 * Socket fs

repos/libports/src/lib/libc/internal/kernel.h

@@ -43,6 +43,7 @@
 #include <internal/pthread.h>
 #include <internal/cwd.h>
 #include <internal/atexit.h>
+#include <internal/rtc.h>
 
 namespace Libc {
 	class Kernel;
@@ -109,6 +110,7 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
                             Select,
                             Kernel_routine_scheduler,
                             Current_time,
+                            Current_real_time,
                             Watch,
                             Cwd
 {
@@ -158,6 +160,7 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
 		Vfs_plugin _vfs { _libc_env, _libc_env.vfs_env(), _heap, *this,
 		                  _update_mtime ? Vfs_plugin::Update_mtime::YES
 		                                : Vfs_plugin::Update_mtime::NO,
+		                  *this /* current_real_time */,
 		                  _libc_env.config() };
 
 		bool  const _cloned = _libc_env.libc_config().attribute_value("cloned", false);
@@ -181,6 +184,8 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
 
 		Config_attr const _rtc_path = _libc_env.libc_config().attribute_value("rtc", Config_attr());
 
+		Constructible<Rtc> _rtc { };
+
 		/* handler for watching the stdout's info pseudo file */
 		Constructible<Watch_handler<Kernel>> _terminal_resize_handler { };
 
@@ -680,6 +685,24 @@ struct Libc::Kernel final : Vfs::Io_response_handler,
 		Absolute_path &cwd() { return _cwd; }
 
 
+		/*********************************
+		 ** Current_real_time interface **
+		 *********************************/
+
+		bool has_real_time() const override
+		{
+			return (_rtc_path != "");
+		}
+
+		timespec current_real_time() override
+		{
+			if (!_rtc.constructed())
+				_rtc.construct(_vfs, _heap, _rtc_path, *this);
+
+			return _rtc->read(current_time());
+		}
+
+
 		/****************************************
 		 ** Vfs::Io_response_handler interface **
 		 ****************************************/

repos/libports/src/lib/libc/internal/rtc.h

@@ -0,0 +1,125 @@
+/*
+ * \brief  Interface for obtaining real-time clock values
+ * \author Norman Feske
+ * \date   2020-08-18
+ */
+
+/*
+ * Copyright (C) 2020 Genode Labs GmbH
+ *
+ * This file is part of the Genode OS framework, which is distributed
+ * under the terms of the GNU Affero General Public License version 3.
+ */
+
+#ifndef _LIBC__INTERNAL__RTC_H_
+#define _LIBC__INTERNAL__RTC_H_
+
+#include <sys/time.h>
+
+namespace Libc { struct Rtc; }
+
+
+struct Libc::Rtc : Vfs::Watch_response_handler
+{
+	using Allocator = Genode::Allocator;
+
+	Vfs_plugin &_vfs;
+
+	Allocator &_alloc;
+
+	Vfs::Vfs_watch_handle *_watch_handle { nullptr };
+
+	Rtc_path const _rtc_path;
+
+	Watch &_watch;
+
+	time_t _rtc_value { 0 };
+
+	bool _rtc_value_out_of_date { true };
+
+	Milliseconds _msecs_when_rtc_updated { 0 };
+
+	bool const _rtc_path_valid = (_rtc_path != "");
+
+	void _update_rtc_value_from_file()
+	{
+		_vfs.with_root_dir([&] (Directory &root_dir) {
+			try {
+				File_content const content(_alloc, root_dir, _rtc_path.string(),
+				                           File_content::Limit{4096U});
+
+				content.bytes([&] (char const *ptr, size_t size) {
+
+					char buf[32] { };
+					::memcpy(buf, ptr, min(sizeof(buf) - 1, size));
+
+					struct tm tm { };
+					if (strptime(buf, "%Y-%m-%d %R", &tm)) {
+						_rtc_value = mktime(&tm);
+						if (_rtc_value == (time_t)-1)
+							_rtc_value = 0;
+					}
+				});
+			} catch (...) {
+				warning(_rtc_path, " not readable, returning ", _rtc_value);
+			}
+		});
+	}
+
+	Rtc(Vfs_plugin &vfs, Allocator &alloc, Rtc_path const &rtc_path, Watch &watch)
+	:
+		_vfs(vfs), _alloc(alloc), _rtc_path(rtc_path), _watch(watch)
+	{
+		if (!_rtc_path_valid) {
+			warning("rtc not configured, returning ", _rtc_value);
+			return;
+		}
+
+		_watch_handle = _watch.alloc_watch_handle(_rtc_path.string());
+
+		if (_watch_handle)
+			_watch_handle->handler(this);
+	}
+
+
+	/******************************************
+	 ** Vfs::Watch_reponse_handler interface **
+	 ******************************************/
+
+	void watch_response() override
+	{
+		_rtc_value_out_of_date = true;
+	}
+
+	timespec read(Duration current_time)
+	{
+		struct timespec result { };
+
+		if (!_rtc_path_valid)
+			return result;
+
+		if (_rtc_value_out_of_date) {
+			_update_rtc_value_from_file();
+			_msecs_when_rtc_updated = current_time.trunc_to_plain_ms();
+			_rtc_value_out_of_date = false;
+		}
+
+		/*
+		 * Return time as sum of cached RTC value and relative 'current_time'
+		 */
+		Milliseconds const current_msecs = current_time.trunc_to_plain_ms();
+
+		Milliseconds const msecs_since_rtc_update {
+			current_msecs.value - _msecs_when_rtc_updated.value };
+
+		uint64_t const seconds_since_rtc_update =
+			msecs_since_rtc_update.value / 1000;
+
+		result.tv_sec  = _rtc_value + seconds_since_rtc_update;
+		result.tv_nsec = (current_msecs.value % 1000) * (1000*1000);
+
+		return result;
+	}
+};
+
+#endif /* _LIBC__INTERNAL__RTC_H_ */

repos/libports/src/lib/libc/internal/vfs_plugin.h

@@ -78,6 +78,7 @@ class Libc::Vfs_plugin : public Plugin
 		Constructible<Genode::Directory> _root_dir { };
 		Vfs::Io_response_handler        &_response_handler;
 		Update_mtime               const _update_mtime;
+		Current_real_time               &_current_real_time;
 		bool                       const _pipe_configured;
 
 		/**
@@ -118,12 +119,14 @@ class Libc::Vfs_plugin : public Plugin
 		           Genode::Allocator        &alloc,
 		           Vfs::Io_response_handler &handler,
 		           Update_mtime              update_mtime,
+		           Current_real_time        &current_real_time,
 		           Xml_node                  config)
 		:
 			_alloc(alloc),
 			_root_fs(env.vfs()),
 			_response_handler(handler),
 			_update_mtime(update_mtime),
+			_current_real_time(current_real_time),
 			_pipe_configured(_init_pipe_configured(config))
 		{
 			if (config.has_sub_node("libc"))

repos/libports/src/lib/libc/kernel.cc

@@ -536,7 +536,7 @@ Libc::Kernel::Kernel(Genode::Env &env, Genode::Allocator &heap)
 	init_sleep(*this);
 	init_vfs_plugin(*this);
 	init_file_operations(*this);
-	init_time(*this, _rtc_path, *this);
+	init_time(*this, *this);
 	init_select(*this, *this, *this, _signal);
 	init_socket_fs(*this);
 	init_passwd(_passwd_config());

repos/libports/src/lib/libc/time.cc

@@ -33,107 +33,18 @@
 #include <internal/current_time.h>
 #include <internal/watch.h>
 
-static Libc::Current_time *_current_time_ptr;
-static char const         *_rtc_path;
-static Libc::Watch        *_watch_ptr;
+static Libc::Current_time      *_current_time_ptr;
+static Libc::Current_real_time *_current_real_time_ptr;
 
 
-void Libc::init_time(Current_time   &current_time,
-                     Rtc_path const &rtc_path,
-                     Watch          &watch)
+void Libc::init_time(Current_time      &current_time,
+                     Current_real_time &current_real_time)
 {
-	static Rtc_path rtc_path_inst = rtc_path;
-
-	_current_time_ptr = &current_time;
-	_rtc_path         =  rtc_path_inst.string();
-	_watch_ptr        = &watch;
+	_current_time_ptr      = &current_time;
+	_current_real_time_ptr = &current_real_time;
 }
 
 
-namespace Libc { struct Rtc; }
-
-
-struct Libc::Rtc : Vfs::Watch_response_handler
-{
-	Vfs::Vfs_watch_handle *_watch_handle { nullptr };
-
-	Rtc_path const _rtc_path;
-
-	Watch &_watch;
-
-	bool   _read_file { true };
-	time_t _rtc_value { 0 };
-
-	bool const _rtc_path_valid = (_rtc_path != "");
-
-	Rtc(Rtc_path const &rtc_path, Watch &watch)
-	:
-		_rtc_path(rtc_path), _watch(watch)
-	{
-		if (!_rtc_path_valid) {
-			warning("rtc not configured, returning ", _rtc_value);
-			return;
-		}
-
-		_watch_handle = _watch.alloc_watch_handle(_rtc_path.string());
-		if (_watch_handle) {
-			_watch_handle->handler(this);
-		}
-	}
-
-	/******************************************
-	 ** Vfs::Watch_reponse_handler interface **
-	 ******************************************/
-
-	void watch_response() override
-	{
-		_read_file = true;
-	}
-
-	time_t read()
-	{
-		if (!_rtc_path_valid) { return 0; }
-
-		/* return old value */
-		if (!_read_file) { return _rtc_value; }
-
-		_read_file = false;
-
-		int fd = open(_rtc_path.string(), O_RDONLY);
-		if (fd == -1) {
-			warning(_rtc_path, " not readable, returning ", _rtc_value);
-			return _rtc_value;
-		}
-
-		char buf[32];
-		ssize_t n = ::read(fd, buf, sizeof(buf));
-		if (n > 0) {
-			buf[n - 1] = '\0';
-			struct tm tm;
-			memset(&tm, 0, sizeof(tm));
-
-			if (strptime(buf, "%Y-%m-%d %R", &tm)) {
-				_rtc_value = mktime(&tm);
-				if (_rtc_value == (time_t)-1)
-					_rtc_value = 0;
-			}
-		}
-
-		close(fd);
-
-		struct Missing_call_of_init_time : Exception { };
-		if (!_current_time_ptr)
-			throw Missing_call_of_init_time();
-
-		uint64_t const ts_value =
-			_current_time_ptr->current_time().trunc_to_plain_ms().value;
-		_rtc_value += (time_t)ts_value / 1000;
-
-		return _rtc_value;
-	}
-};
-
-
 using namespace Libc;
 
 
@@ -165,21 +76,13 @@ int clock_gettime(clockid_t clk_id, struct timespec *ts)
 	case CLOCK_REALTIME:
 	case CLOCK_SECOND: /* FreeBSD specific */
 	{
-		if (!_watch_ptr)
+		if (!_current_real_time_ptr)
 			throw Missing_call_of_init_time();
 
-		/*
-		 * XXX move instance to Libc::Kernel
-		 */
-		Rtc &rtc = *unmanaged_singleton<Rtc>(_rtc_path, *_watch_ptr);
+		if (!_current_real_time_ptr->has_real_time())
+			return Errno(EINVAL);
 
-		time_t const rtc_value = rtc.read();
-		if (!rtc_value) return Errno(EINVAL);
-
-		uint64_t const time = current_time().trunc_to_plain_ms().value;
-
-		ts->tv_sec  = rtc_value + time/1000;
-		ts->tv_nsec = (time % 1000) * (1000*1000);
+		*ts = _current_real_time_ptr->current_real_time();
 		break;
 	}
 

repos/libports/src/lib/libc/vfs_plugin.cc

@@ -43,6 +43,7 @@
 #include <internal/init.h>
 #include <internal/legacy.h>
 #include <internal/monitor.h>
+#include <internal/current_time.h>
 
 
 static Libc::Monitor *_monitor_ptr;
@@ -521,17 +522,18 @@ struct Sync
 	Vfs::Vfs_handle &vfs_handle;
 	Vfs::Timestamp   mtime { Vfs::Timestamp::INVALID };
 
-	Sync(Vfs::Vfs_handle &vfs_handle, Libc::Vfs_plugin::Update_mtime update_mtime)
+	Sync(Vfs::Vfs_handle &vfs_handle, Libc::Vfs_plugin::Update_mtime update_mtime,
+	     Libc::Current_real_time &current_real_time)
 	:
 		vfs_handle(vfs_handle)
 	{
-		if (update_mtime == Libc::Vfs_plugin::Update_mtime::NO) {
-			state = TIMESTAMP_UPDATED;
-		} else {
-			timespec ts { 0, 0};
+		if (update_mtime == Libc::Vfs_plugin::Update_mtime::NO
+		 || !current_real_time.has_real_time()) {
 
-			/* XXX using  clock_gettime directly is probably not the best idea */
-			clock_gettime(CLOCK_REALTIME, &ts);
+			state = TIMESTAMP_UPDATED;
+
+		} else {
+			timespec const ts = current_real_time.current_real_time();
 
 			mtime = { .value = (long long)ts.tv_sec };
 		}
@@ -566,7 +568,7 @@ int Libc::Vfs_plugin::close_from_kernel(File_descriptor *fd)
 
 	if ((fd->modified) || (fd->flags & O_CREAT)) {
 		/* XXX mtime not updated here */
-		Sync sync { *handle, Update_mtime::NO };
+		Sync sync { *handle, Update_mtime::NO, _current_real_time };
 
 		while (!sync.complete())
 			Libc::Kernel::kernel().libc_env().ep().wait_and_dispatch_one_io_signal();
@@ -583,7 +585,7 @@ int Libc::Vfs_plugin::close(File_descriptor *fd)
 {
 	Vfs::Vfs_handle *handle = vfs_handle(fd);
 
-	Sync sync { *handle , _update_mtime };
+	Sync sync { *handle , _update_mtime, _current_real_time };
 	Mutex::Guard guard(vfs_mutex());
 
 	monitor().monitor(vfs_mutex(), [&] {
@@ -676,7 +678,7 @@ int Libc::Vfs_plugin::fstat(File_descriptor *fd, struct stat *buf)
 	Vfs::Vfs_handle *handle = vfs_handle(fd);
 
 	if (fd->modified) {
-		Sync sync { *handle , _update_mtime };
+		Sync sync { *handle , _update_mtime, _current_real_time };
 		Mutex::Guard guard(vfs_mutex());
 
 		monitor().monitor(vfs_mutex(), [&] {
@@ -1308,7 +1310,7 @@ int Libc::Vfs_plugin::_legacy_ioctl(File_descriptor *fd, int request, char *argp
 int Libc::Vfs_plugin::ftruncate(File_descriptor *fd, ::off_t length)
 {
 	Vfs::Vfs_handle *handle = vfs_handle(fd);
-	Sync sync { *handle, _update_mtime };
+	Sync sync { *handle, _update_mtime, _current_real_time };
 	Mutex::Guard guard(vfs_mutex());
 
 	bool succeeded = false;
@@ -1381,9 +1383,12 @@ int Libc::Vfs_plugin::fcntl(File_descriptor *fd, int cmd, long arg)
 int Libc::Vfs_plugin::fsync(File_descriptor *fd)
 {
 	Vfs::Vfs_handle *handle = vfs_handle(fd);
-	if (!fd->modified) { return 0; }
 
-	Sync sync { *handle, _update_mtime };
+	if (!fd->modified)
+		return 0;
+
+	Sync sync { *handle, _update_mtime, _current_real_time };
+
 	Mutex::Guard guard(vfs_mutex());
 
 	monitor().monitor(vfs_mutex(), [&] {
@@ -1442,9 +1447,7 @@ int Libc::Vfs_plugin::symlink(const char *target_path, const char *link_path)
 				}
 
 				handle->handler(&_response_handler);
-				/* XXX this could deadlock in rare cases where the clock is
-				 * accessed the first time */
-				sync.construct(*handle, _update_mtime);
+				sync.construct(*handle, _update_mtime, _current_real_time);
 			} stage = Stage::WRITE; [[fallthrough]]
 
 		case Stage::WRITE: