| Message ID | 20250603134542.67627-2-andrea.ricchi@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 <andrea.ricchi@amarulasolutions.com> 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 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 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 + 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. Signed-off-by: Andrea Ricchi <andrea.ricchi@amarulasolutions.com> --- Changes v2 -> v3: - Splitted commit Changes v1 -> v2: - Change commit message src/timezone.c | 49 +++++++++++++------------------------------------ 1 file changed, 13 insertions(+), 36 deletions(-)