| Message ID | 1520509926-15837-3-git-send-email-alex.kiernan@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Add atomic write to fw_setenv for environments on filesystems | expand |
Hi alex, On 08/03/2018 12:52, Alex Kiernan wrote: > If the U-Boot environment is stored in a regular file and redundant > operation isn't set, then write to a temporary file and perform an > atomic rename. > Even if it is not explicitely set (IMHO it should), this code can be used as library and linked to own application. That means that concurrency can happens. I mean: > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> > --- > > tools/env/fw_env.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 78 insertions(+), 3 deletions(-) > > diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c > index 2df3504..b814c4e 100644 > --- a/tools/env/fw_env.c > +++ b/tools/env/fw_env.c > @@ -14,6 +14,7 @@ > #include <errno.h> > #include <env_flags.h> > #include <fcntl.h> > +#include <libgen.h> > #include <linux/fs.h> > #include <linux/stringify.h> > #include <ctype.h> > @@ -1229,9 +1230,46 @@ static int flash_read (int fd) > return 0; > } > > +static int flash_open_tempfile(const char **dname, const char **target_temp) > +{ > + char *dup_name = strdup(DEVNAME (dev_current)); > + char *temp_name = NULL; > + int rc = -1; > + > + if (!dup_name) > + return -1; > + > + *dname = dirname(dup_name); > + if (!*dname) > + goto err; > + > + rc = asprintf(&temp_name, "%s/uboot.tmp", *dname); Filename is fixed - should we not use mkstemp ? > + if (rc == -1) > + goto err; > + > + rc = open(temp_name, O_RDWR | O_CREAT | O_TRUNC, 0700); > + if (rc == -1) { > + /* fall back to in place write */ > + free(temp_name); > + } else { > + *target_temp = temp_name; > + /* deliberately leak dup_name as dname /might/ point into > + * it and we need it for our caller > + */ > + dup_name = NULL; > + } > + > +err: > + if (dup_name) > + free(dup_name); > + > + return rc; > +} > + > static int flash_io_write (int fd_current) > { > - int fd_target, rc, dev_target; > + int fd_target = -1, rc, dev_target; > + const char *dname, *target_temp = NULL; > > if (HaveRedundEnv) { > /* switch to next partition for writing */ > @@ -1247,8 +1285,17 @@ static int flash_io_write (int fd_current) > goto exit; > } > } else { > + struct stat sb; > + > + if (fstat(fd_current, &sb) == 0 && S_ISREG(sb.st_mode)) { > + /* if any part of flash_open_tempfile() fails we fall > + * back to in-place writes > + */ > + fd_target = flash_open_tempfile(&dname, &target_temp); > + } > dev_target = dev_current; > - fd_target = fd_current; > + if (fd_target == -1) > + fd_target = fd_current; > } > > rc = flash_write (fd_current, fd_target, dev_target); > @@ -1260,7 +1307,7 @@ static int flash_io_write (int fd_current) > DEVNAME (dev_current), strerror (errno)); > } > > - if (HaveRedundEnv) { > + if (fd_current != fd_target) { > if (fsync(fd_target) && > !(errno == EINVAL || errno == EROFS)) { > fprintf (stderr, > @@ -1275,6 +1322,34 @@ static int flash_io_write (int fd_current) > strerror (errno)); > rc = -1; > } > + > + if (target_temp) { > + int dir_fd; > + > + dir_fd = open(dname, O_DIRECTORY | O_RDONLY); > + if (dir_fd == -1) > + fprintf (stderr, > + "Can't open %s: %s\n", > + dname, strerror (errno)); > + > + if (rename(target_temp, DEVNAME(dev_target))) { > + fprintf (stderr, > + "rename failed %s => %s: %s\n", > + target_temp, DEVNAME(dev_target), > + strerror (errno)); > + rc = -1; > + } > + > + if (dir_fd != -1 && fsync(dir_fd)) > + fprintf (stderr, > + "fsync failed on %s: %s\n", > + dname, strerror (errno)); > + > + if (dir_fd != -1 && close(dir_fd)) > + fprintf (stderr, > + "I/O error on %s: %s\n", > + dname, strerror (errno)); > + } > } > exit: > return rc; > Best regards, Stefano
On Thu, Mar 8, 2018 at 5:04 PM, Stefano Babic <sbabic@denx.de> wrote: > Hi alex, > > On 08/03/2018 12:52, Alex Kiernan wrote: >> If the U-Boot environment is stored in a regular file and redundant >> operation isn't set, then write to a temporary file and perform an >> atomic rename. >> > > Even if it is not explicitely set (IMHO it should), this code can > be used as library and linked to own application. That means that > concurrency can happens. I mean: > At this point you're writing the new environment, so we should hold a lock to avoid concurrent writes. >> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> >> --- >> >> tools/env/fw_env.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 78 insertions(+), 3 deletions(-) >> >> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c >> index 2df3504..b814c4e 100644 >> --- a/tools/env/fw_env.c >> +++ b/tools/env/fw_env.c >> @@ -14,6 +14,7 @@ >> #include <errno.h> >> #include <env_flags.h> >> #include <fcntl.h> >> +#include <libgen.h> >> #include <linux/fs.h> >> #include <linux/stringify.h> >> #include <ctype.h> >> @@ -1229,9 +1230,46 @@ static int flash_read (int fd) >> return 0; >> } >> >> +static int flash_open_tempfile(const char **dname, const char **target_temp) >> +{ >> + char *dup_name = strdup(DEVNAME (dev_current)); >> + char *temp_name = NULL; >> + int rc = -1; >> + >> + if (!dup_name) >> + return -1; >> + >> + *dname = dirname(dup_name); >> + if (!*dname) >> + goto err; >> + >> + rc = asprintf(&temp_name, "%s/uboot.tmp", *dname); > > Filename is fixed - should we not use mkstemp ? > I went round all the temp functions before in the end deciding to fix it. However it looks like I totally misread the contract that mkstemp delivers - I'd thought it didn't pass you the name back, but it clearly does; I'll go update it.
On 09/03/2018 10:54, Alex Kiernan wrote: > On Thu, Mar 8, 2018 at 5:04 PM, Stefano Babic <sbabic@denx.de> wrote: >> Hi alex, >> >> On 08/03/2018 12:52, Alex Kiernan wrote: >>> If the U-Boot environment is stored in a regular file and redundant >>> operation isn't set, then write to a temporary file and perform an >>> atomic rename. >>> >> >> Even if it is not explicitely set (IMHO it should), this code can >> be used as library and linked to own application. That means that >> concurrency can happens. I mean: >> > > At this point you're writing the new environment, so we should hold a > lock to avoid concurrent writes. This is already done, even if not in the way I like. tools/env/fw_env_main.c calls flock() to acquire the resource. This works using fw_printenv / fw_setenv, but not as library. Library's users as me have to provide themselves a lock before calling the fw_* functions. > >>> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> >>> --- >>> >>> tools/env/fw_env.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- >>> 1 file changed, 78 insertions(+), 3 deletions(-) >>> >>> diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c >>> index 2df3504..b814c4e 100644 >>> --- a/tools/env/fw_env.c >>> +++ b/tools/env/fw_env.c >>> @@ -14,6 +14,7 @@ >>> #include <errno.h> >>> #include <env_flags.h> >>> #include <fcntl.h> >>> +#include <libgen.h> >>> #include <linux/fs.h> >>> #include <linux/stringify.h> >>> #include <ctype.h> >>> @@ -1229,9 +1230,46 @@ static int flash_read (int fd) >>> return 0; >>> } >>> >>> +static int flash_open_tempfile(const char **dname, const char **target_temp) >>> +{ >>> + char *dup_name = strdup(DEVNAME (dev_current)); >>> + char *temp_name = NULL; >>> + int rc = -1; >>> + >>> + if (!dup_name) >>> + return -1; >>> + >>> + *dname = dirname(dup_name); >>> + if (!*dname) >>> + goto err; >>> + >>> + rc = asprintf(&temp_name, "%s/uboot.tmp", *dname); >> >> Filename is fixed - should we not use mkstemp ? >> > > I went round all the temp functions before in the end deciding to fix > it. However it looks like I totally misread the contract that mkstemp > delivers - I'd thought it didn't pass you the name back, but it > clearly does; I'll go update it. > Best regards, Stefano Babic
On Fri, Mar 9, 2018 at 10:03 AM, Stefano Babic <sbabic@denx.de> wrote: > On 09/03/2018 10:54, Alex Kiernan wrote: >> On Thu, Mar 8, 2018 at 5:04 PM, Stefano Babic <sbabic@denx.de> wrote: >>> Hi alex, >>> >>> On 08/03/2018 12:52, Alex Kiernan wrote: >>>> If the U-Boot environment is stored in a regular file and redundant >>>> operation isn't set, then write to a temporary file and perform an >>>> atomic rename. >>>> >>> >>> Even if it is not explicitely set (IMHO it should), this code can >>> be used as library and linked to own application. That means that >>> concurrency can happens. I mean: >>> >> >> At this point you're writing the new environment, so we should hold a >> lock to avoid concurrent writes. > > This is already done, even if not in the way I like. > tools/env/fw_env_main.c calls flock() to acquire the resource. This > works using fw_printenv / fw_setenv, but not as library. Library's users > as me have to provide themselves a lock before calling the fw_* functions. > True... that particular implementation also causes me a different problem in that it fails on a read-only fs and I have a need for a call to fw_printenv before the filesystem has gone read-write.
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 2df3504..b814c4e 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -14,6 +14,7 @@ #include <errno.h> #include <env_flags.h> #include <fcntl.h> +#include <libgen.h> #include <linux/fs.h> #include <linux/stringify.h> #include <ctype.h> @@ -1229,9 +1230,46 @@ static int flash_read (int fd) return 0; } +static int flash_open_tempfile(const char **dname, const char **target_temp) +{ + char *dup_name = strdup(DEVNAME (dev_current)); + char *temp_name = NULL; + int rc = -1; + + if (!dup_name) + return -1; + + *dname = dirname(dup_name); + if (!*dname) + goto err; + + rc = asprintf(&temp_name, "%s/uboot.tmp", *dname); + if (rc == -1) + goto err; + + rc = open(temp_name, O_RDWR | O_CREAT | O_TRUNC, 0700); + if (rc == -1) { + /* fall back to in place write */ + free(temp_name); + } else { + *target_temp = temp_name; + /* deliberately leak dup_name as dname /might/ point into + * it and we need it for our caller + */ + dup_name = NULL; + } + +err: + if (dup_name) + free(dup_name); + + return rc; +} + static int flash_io_write (int fd_current) { - int fd_target, rc, dev_target; + int fd_target = -1, rc, dev_target; + const char *dname, *target_temp = NULL; if (HaveRedundEnv) { /* switch to next partition for writing */ @@ -1247,8 +1285,17 @@ static int flash_io_write (int fd_current) goto exit; } } else { + struct stat sb; + + if (fstat(fd_current, &sb) == 0 && S_ISREG(sb.st_mode)) { + /* if any part of flash_open_tempfile() fails we fall + * back to in-place writes + */ + fd_target = flash_open_tempfile(&dname, &target_temp); + } dev_target = dev_current; - fd_target = fd_current; + if (fd_target == -1) + fd_target = fd_current; } rc = flash_write (fd_current, fd_target, dev_target); @@ -1260,7 +1307,7 @@ static int flash_io_write (int fd_current) DEVNAME (dev_current), strerror (errno)); } - if (HaveRedundEnv) { + if (fd_current != fd_target) { if (fsync(fd_target) && !(errno == EINVAL || errno == EROFS)) { fprintf (stderr, @@ -1275,6 +1322,34 @@ static int flash_io_write (int fd_current) strerror (errno)); rc = -1; } + + if (target_temp) { + int dir_fd; + + dir_fd = open(dname, O_DIRECTORY | O_RDONLY); + if (dir_fd == -1) + fprintf (stderr, + "Can't open %s: %s\n", + dname, strerror (errno)); + + if (rename(target_temp, DEVNAME(dev_target))) { + fprintf (stderr, + "rename failed %s => %s: %s\n", + target_temp, DEVNAME(dev_target), + strerror (errno)); + rc = -1; + } + + if (dir_fd != -1 && fsync(dir_fd)) + fprintf (stderr, + "fsync failed on %s: %s\n", + dname, strerror (errno)); + + if (dir_fd != -1 && close(dir_fd)) + fprintf (stderr, + "I/O error on %s: %s\n", + dname, strerror (errno)); + } } exit: return rc;
If the U-Boot environment is stored in a regular file and redundant operation isn't set, then write to a temporary file and perform an atomic rename. Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> --- tools/env/fw_env.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 78 insertions(+), 3 deletions(-)