Re: [PATCH v2 1/1] timezone: Replace Localtime file copy with symbolic link

Hi Andrea and all.



On 6/3/25 16:10, Andrea Ricchi wrote:
> [You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Based on the `man 5 localtime` documentation the `/etc/localtime`
> file is expected to be a symbolic link, 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 also fix `compare_file` as is intended to work with symlinks, enabling
> full and accurate matching between the requested timezone and the zoneinfo
> database.

I'd suggest doing two commits. First to revert 
f20ccd19a62bd01aa117bfc8e7c388d2a16cde05 - then to have these other 
changes in their own commit. For me it would be a clear process here.

> 
> Signed-off-by: Andrea Ricchi <[email protected]>
> ---
> 
>   Changes v1 -> v2:
>     - Change comit message
> 
>   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;

Suggestion: this part as a revert commit.

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

Suggestion: and this ^^ as its own commit.


Cheers,
  Jussi