Re: [PATCH v3 2/2] timezone: Replace Localtime file copy with symbolic link

Hi Andrea,

On 6/3/25 8:45 AM, Andrea Ricchi wrote:
> 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.
> 
> Signed-off-by: Andrea Ricchi <[email protected]>

nit: ConnMan doesn't use this tag, so please leave it out.

> ---
> 
>   Changes v2 -> v3:
>    - Splitted commit
> 
>   Changes v1 -> v2:
>    - Change commit message
> 
>   src/timezone.c | 49 +++++++++++++------------------------------------
>   1 file changed, 13 insertions(+), 36 deletions(-)
> 
> diff --git a/src/timezone.c b/src/timezone.c
> index 89c44895..9b820477 100644
> --- a/src/timezone.c
> +++ b/src/timezone.c
> @@ -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

No C++ style comments please

> +	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);
>   

It is really unlikely that the use of snprintf will cause problems, but 
generally its use along with PATH_MAX is discouraged.  Given that this is off 
the hot path, I'd recommend using something like:

g_autofree char *pathname = NULL;
...
pathname = g_strdup_printf("%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

C++ comments again :)

> +	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"));

Might as well just use:

return write_symlink(...) and drop 'err' variable entirely.

>   
>   	return err;
>   }

Regards,
-Denis