On 08/03/2018 12:52, Alex Kiernan wrote:

Extract write path of flash_io() into a separate function. This patch should be a functional no-op.

Signed-off-by: Alex Kiernan alex.kiernan@gmail.com

tools/env/fw_env.c | 98 +++++++++++++++++++++++++++++------------------------- 1 file changed, 53 insertions(+), 45 deletions(-)

diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 0e3e343..2df3504 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -1229,68 +1229,76 @@ static int flash_read (int fd) return 0; }

-static int flash_io (int mode) +static int flash_io_write (int fd_current) {

• int fd_current, fd_target, rc, dev_target;

• int fd_target, rc, dev_target;

• /* dev_current: fd_current, erase_current */
• fd_current = open (DEVNAME (dev_current), mode);
• if (fd_current < 0) {

• fprintf (stderr,
  

• 	 "Can't open %s: %s\n",
  

• 	 DEVNAME (dev_current), strerror (errno));
  

• return -1;
  

• if (HaveRedundEnv) {

• /* switch to next partition for writing */
  

• dev_target = !dev_current;
  

• /* dev_target: fd_target, erase_target */
  

• fd_target = open (DEVNAME (dev_target), O_RDWR);
  

• if (fd_target < 0) {
  

• 	fprintf (stderr,
  

• 		 "Can't open %s: %s\n",
  

• 		 DEVNAME (dev_target),
  

• 		 strerror (errno));
  

• 	rc = -1;
  

• 	goto exit;
  

• }
  

• } else {

• dev_target = dev_current;
  

• fd_target = fd_current;
  

  }

• if (mode == O_RDWR) {

• if (HaveRedundEnv) {
  

• 	/* switch to next partition for writing */
  

• 	dev_target = !dev_current;
  

• 	/* dev_target: fd_target, erase_target */
  

• 	fd_target = open (DEVNAME (dev_target), mode);
  

• 	if (fd_target < 0) {
  

• 		fprintf (stderr,
  

• 			 "Can't open %s: %s\n",
  

• 			 DEVNAME (dev_target),
  

• 			 strerror (errno));
  

• 		rc = -1;
  

• 		goto exit;
  

• 	}
  

• } else {
  

• 	dev_target = dev_current;
  

• 	fd_target = fd_current;
  

• }
  

• rc = flash_write (fd_current, fd_target, dev_target);

• rc = flash_write (fd_current, fd_target, dev_target);
  

• if (fsync(fd_current) &&

•    !(errno == EINVAL || errno == EROFS)) {
  

• fprintf (stderr,
  

• 	 "fsync failed on %s: %s\n",
  

• 	 DEVNAME (dev_current), strerror (errno));
  

• }

• if (fsync(fd_current) &&
  

• if (HaveRedundEnv) {

• if (fsync(fd_target) &&
  

  !(errno == EINVAL || errno == EROFS)) { fprintf (stderr, "fsync failed on %s: %s\n", DEVNAME (dev_current), strerror (errno)); }

• if (HaveRedundEnv) {
  

• 	if (fsync(fd_target) &&
  

• 	    !(errno == EINVAL || errno == EROFS)) {
  

• 		fprintf (stderr,
  

• 			 "fsync failed on %s: %s\n",
  

• 			 DEVNAME (dev_current), strerror (errno));
  

• 	}
  

• 	if (close (fd_target)) {
  

• 		fprintf (stderr,
  

• 			"I/O error on %s: %s\n",
  

• 			DEVNAME (dev_target),
  

• 			strerror (errno));
  

• 		rc = -1;
  

• 	}
  

• if (close (fd_target)) {
  

• 	fprintf (stderr,
  

• 		 "I/O error on %s: %s\n",
  

• 		 DEVNAME (dev_target),
  

• 		 strerror (errno));
  

• 	rc = -1;
  

  }
• }

+exit:

• return rc;

+}

+static int flash_io (int mode) +{

• int fd_current, rc;
• /* dev_current: fd_current, erase_current */
• fd_current = open (DEVNAME (dev_current), mode);
• if (fd_current < 0) {

• fprintf (stderr,
  

• 	 "Can't open %s: %s\n",
  

• 	 DEVNAME (dev_current), strerror (errno));
  

• return -1;
  

• }
• if (mode == O_RDWR) {

• rc = flash_io_write(fd_current);
  

  } else { rc = flash_read (fd_current); }

-exit: if (close (fd_current)) { fprintf (stderr, "I/O error on %s: %s\n",

Reviewed-by: Stefano Babic sbabic@denx.de

Best regards, Stefano Babic

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 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