Re: [U-Boot] [PATCH 2/2] tools: env: Implement atomic replace for filesystem

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