| Message ID | 20250603131034.60946-1-andrea.ricchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Andrea and all. On 6/3/25 16:10, Andrea Ricchi wrote: > [You don't often get email from andrea.ricchi@amarulasolutions.com. 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 <andrea.ricchi@amarulasolutions.com> > --- > > 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 To unsubscribe from this group and stop receiving emails from it, send an email to linux-amarula+unsubscribe@amarulasolutions.com.
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; }
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. Signed-off-by: Andrea Ricchi <andrea.ricchi@amarulasolutions.com> --- Changes v1 -> v2: - Change comit message src/timezone.c | 53 ++++++++++++++------------------------------------ 1 file changed, 15 insertions(+), 38 deletions(-)