timezone: Replace Localtime file copy with symbolic link
diff mbox series

Message ID 20250519071912.7682-1-andrea.ricchi@amarulasolutions.com
State New
Headers show
Series
  • timezone: Replace Localtime file copy with symbolic link
Related show

Commit Message

Andrea Ricchi May 19, 2025, 7:19 a.m. UTC 
The `/etc/localtime` file is expected to be a symbolic link to a
timezone file under `/usr/share/zoneinfo`, not a standalone
regular file.

This change replaces `write_file` with `write_symlink`, avoiding
unnecessary file I/O and memory mapping. It ensures Localtime accurately
reflects the selected timezone without duplicating data. This also
fixes `__connman_timezone_lookup()`, which requires Localtime to be a
symlink to function correctly.

This reverts commit f20ccd19a62bd01aa117bfc8e7c388d2a16cde05, since
`compare_file` is intended to work with symlinks, enabling full and
accurate matching between the requested timezone and the zoneinfo database.

---
 src/timezone.c | 53 ++++++++++++++------------------------------------
 1 file changed, 15 insertions(+), 38 deletions(-)

Comments

Michael Trimarchi May 30, 2025, 9:03 a.m. UTC | #1 
Hi Andrea

On Mon, May 19, 2025 at 9:19 AM Andrea Ricchi
<andrea.ricchi@amarulasolutions.com> wrote:
>
> The `/etc/localtime` file is expected to be a symbolic link to a
> timezone file under `/usr/share/zoneinfo`, not a standalone
> regular file.
>
> This change replaces `write_file` with `write_symlink`, avoiding
> unnecessary file I/O and memory mapping. It ensures Localtime accurately
> reflects the selected timezone without duplicating data. This also
> fixes `__connman_timezone_lookup()`, which requires Localtime to be a
> symlink to function correctly.

It's better that the usage of the link is reported by the manual of localtime
and that distro use it

/etc/localtime -> /usr/share/zoneinfo/Europe/Rome

>
> This reverts commit f20ccd19a62bd01aa117bfc8e7c388d2a16cde05, since

This really does not revert, because you are changing the behavior. I would like
to say that compare_file it's not the right thing to do because
timezone can be the same
for different zoneinfo

> `compare_file` is intended to work with symlinks, enabling full and
> accurate matching between the requested timezone and the zoneinfo database.
>

For the rest
Reviewed-by: Michael Trimarchi <michael@amarulasolutions.com>

Michael

> ---
>  src/timezone.c | 53 ++++++++++++++------------------------------------
>  1 file changed, 15 insertions(+), 38 deletions(-)
>
> diff --git a/src/timezone.c b/src/timezone.c
> index fba8b925..9b820477 100644
> --- a/src/timezone.c
> +++ b/src/timezone.c
> @@ -124,8 +124,8 @@ static int compare_file(void *src_map, struct stat *src_st,
>
>         DBG("real path %s path name %s", real_path, pathname);
>
> -       if (real_path && !g_strcmp0(real_path, pathname))
> -               return 0;
> +       if (real_path && g_strcmp0(real_path, pathname))
> +               return -1;
>
>         fd = open(pathname, O_RDONLY | O_CLOEXEC);
>         if (fd < 0)
> @@ -507,28 +507,20 @@ done:
>         return zone;
>  }
>
> -static int write_file(void *src_map, struct stat *src_st, const char *pathname)
> +static int write_symlink(const char *target, const char *link)
>  {
>         struct stat st;
> -       int fd;
> -       ssize_t written;
>
> -       DBG("pathname %s", pathname);
> +       DBG("pathname %s", link);
>
> -       if (lstat(pathname, &st) == 0) {
> -               if (S_ISLNK(st.st_mode))
> -                       unlink(pathname);
> +       // Remove existing file or symlink if it exists
> +       if (lstat(link, &st) == 0) {
> +               if (S_ISLNK(st.st_mode) || S_ISREG(st.st_mode))
> +                       unlink(link);
>         }
>
> -       fd = open(pathname, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0644);
> -       if (fd < 0)
> -               return -EIO;
> -
> -       written = write(fd, src_map, src_st->st_size);
> -
> -       close(fd);
> -
> -       if (written < 0)
> +       // Create the symbolic link
> +       if (symlink(target, link) < 0)
>                 return -EIO;
>
>         return 0;
> @@ -537,33 +529,18 @@ static int write_file(void *src_map, struct stat *src_st, const char *pathname)
>  int __connman_timezone_change(const char *zone)
>  {
>         struct stat st;
> -       char *map, pathname[PATH_MAX];
> -       int fd, err;
> +       char pathname[PATH_MAX];
> +       int err;
>
>         DBG("zone %s", zone);
>
>         snprintf(pathname, PATH_MAX, "%s/%s", USR_SHARE_ZONEINFO, zone);
>
> -       fd = open(pathname, O_RDONLY | O_CLOEXEC);
> -       if (fd < 0)
> +       // Just check that the file exists and is a regular file
> +       if (stat(pathname, &st) < 0 || !S_ISREG(st.st_mode))
>                 return -EINVAL;
>
> -       if (fstat(fd, &st) < 0) {
> -               close(fd);
> -               return -EIO;
> -       }
> -
> -       map = mmap(0, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
> -       if (!map || map == MAP_FAILED) {
> -               close(fd);
> -               return -EIO;
> -       }
> -
> -       err = write_file(map, &st, connman_setting_get_string("Localtime"));
> -
> -       munmap(map, st.st_size);
> -
> -       close(fd);
> +       err = write_symlink(pathname, connman_setting_get_string("Localtime"));
>
>         return err;
>  }
> --
> 2.34.1
>
>

Patch
diff mbox series 

diff --git a/src/timezone.c b/src/timezone.c
index fba8b925..9b820477 100644
--- a/src/timezone.c
+++ b/src/timezone.c
@@ -124,8 +124,8 @@  static int compare_file(void *src_map, struct stat *src_st,
 
 	DBG("real path %s path name %s", real_path, pathname);
 
-	if (real_path && !g_strcmp0(real_path, pathname))
-		return 0;
+	if (real_path && g_strcmp0(real_path, pathname))
+		return -1;
 
 	fd = open(pathname, O_RDONLY | O_CLOEXEC);
 	if (fd < 0)
@@ -507,28 +507,20 @@  done:
 	return zone;
 }
 
-static int write_file(void *src_map, struct stat *src_st, const char *pathname)
+static int write_symlink(const char *target, const char *link)
 {
 	struct stat st;
-	int fd;
-	ssize_t written;
 
-	DBG("pathname %s", pathname);
+	DBG("pathname %s", link);
 
-	if (lstat(pathname, &st) == 0) {
-		if (S_ISLNK(st.st_mode))
-			unlink(pathname);
+	// Remove existing file or symlink if it exists
+	if (lstat(link, &st) == 0) {
+		if (S_ISLNK(st.st_mode) || S_ISREG(st.st_mode))
+			unlink(link);
 	}
 
-	fd = open(pathname, O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC, 0644);
-	if (fd < 0)
-		return -EIO;
-
-	written = write(fd, src_map, src_st->st_size);
-
-	close(fd);
-
-	if (written < 0)
+	// Create the symbolic link
+	if (symlink(target, link) < 0)
 		return -EIO;
 
 	return 0;
@@ -537,33 +529,18 @@  static int write_file(void *src_map, struct stat *src_st, const char *pathname)
 int __connman_timezone_change(const char *zone)
 {
 	struct stat st;
-	char *map, pathname[PATH_MAX];
-	int fd, err;
+	char pathname[PATH_MAX];
+	int err;
 
 	DBG("zone %s", zone);
 
 	snprintf(pathname, PATH_MAX, "%s/%s", USR_SHARE_ZONEINFO, zone);
 
-	fd = open(pathname, O_RDONLY | O_CLOEXEC);
-	if (fd < 0)
+	// Just check that the file exists and is a regular file
+	if (stat(pathname, &st) < 0 || !S_ISREG(st.st_mode))
 		return -EINVAL;
 
-	if (fstat(fd, &st) < 0) {
-		close(fd);
-		return -EIO;
-	}
-
-	map = mmap(0, st.st_size, PROT_READ, MAP_SHARED, fd, 0);
-	if (!map || map == MAP_FAILED) {
-		close(fd);
-		return -EIO;
-	}
-
-	err = write_file(map, &st, connman_setting_get_string("Localtime"));
-
-	munmap(map, st.st_size);
-
-	close(fd);
+	err = write_symlink(pathname, connman_setting_get_string("Localtime"));
 
 	return err;
 }