[U-Boot] [PATCH v2 0/4] Add atomic write to fw_setenv for environments on filesystems
For environments stored on filesystems where you can't have a redundant configuration, rather than just over-writing the existing environment in fw_setenv, do the tradtional create temporary file, rename, sync, sync directory dance to achieve ACID semantics when writing through fw_setenv.
fw_env.c has had whitespace modified and HaveRedundEnv renamed in order to avoid triggering large numbers of checkpatch warnings because of the existing code style:
warning: space prohibited between function name and open parenthesis '(' check: Avoid CamelCase: <HaveRedundEnv>
Changes in v2: - Clean style violations in existing code - Acommodate white space changes from predecessor commit - Use mkstemp() to create temporary filename, not a fixed one
Alex Kiernan (4): tools: env: Pass through indent tools: env: Fix CamelCasing style violation tools: env: Refactor write path of flash_io() tools: env: Implement atomic replace for filesystem
tools/env/fw_env.c | 483 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 281 insertions(+), 202 deletions(-)
Pass tools/env/fw_env.c through indent to correct style violations. This commit consists of only one non-whitespace change:
tools/env/fw_env.c:549: error: do not use assignment in if condition
Signed-off-by: Alex Kiernan alex.kiernan@gmail.com ---
Changes in v2: - Clean style violations in existing code
tools/env/fw_env.c | 346 ++++++++++++++++++++++++++--------------------------- 1 file changed, 170 insertions(+), 176 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 0e3e343..9d5ccfc 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -64,14 +64,14 @@ struct envdev_s { int is_ubi; /* set if we use UBI volume */ };
-static struct envdev_s envdevices[2] = -{ +static struct envdev_s envdevices[2] = { { .mtd_type = MTD_ABSENT, }, { .mtd_type = MTD_ABSENT, }, }; + static int dev_current;
#define DEVNAME(i) envdevices[(i)].devname @@ -88,14 +88,14 @@ static unsigned long usable_envsize; #define ENV_SIZE usable_envsize
struct env_image_single { - uint32_t crc; /* CRC32 over data bytes */ - char data[]; + uint32_t crc; /* CRC32 over data bytes */ + char data[]; };
struct env_image_redundant { - uint32_t crc; /* CRC32 over data bytes */ - unsigned char flags; /* active or obsolete */ - char data[]; + uint32_t crc; /* CRC32 over data bytes */ + unsigned char flags; /* active or obsolete */ + char data[]; };
enum flag_scheme { @@ -105,11 +105,11 @@ enum flag_scheme { };
struct environment { - void *image; - uint32_t *crc; - unsigned char *flags; - char *data; - enum flag_scheme flag_scheme; + void *image; + uint32_t *crc; + unsigned char *flags; + char *data; + enum flag_scheme flag_scheme; };
static struct environment environment = { @@ -347,11 +347,11 @@ static int ubi_write(int fd, const void *buf, size_t count) return 0; }
-static int flash_io (int mode); +static int flash_io(int mode); static int parse_config(struct env_opts *opts);
#if defined(CONFIG_FILE) -static int get_config (char *); +static int get_config(char *); #endif
static char *skip_chars(char *s) @@ -394,7 +394,7 @@ static char *envmatch(char *s1, char *s2) * Search the environment for a variable. * Return the value, if found, or NULL, if not found. */ -char *fw_getenv (char *name) +char *fw_getenv(char *name) { char *env, *nxt;
@@ -403,12 +403,12 @@ char *fw_getenv (char *name)
for (nxt = env; *nxt; ++nxt) { if (nxt >= &environment.data[ENV_SIZE]) { - fprintf (stderr, "## Error: " + fprintf(stderr, "## Error: " "environment not terminated\n"); return NULL; } } - val = envmatch (name, env); + val = envmatch(name, env); if (!val) continue; return val; @@ -462,18 +462,18 @@ int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts) if (fw_env_open(opts)) return -1;
- if (argc == 0) { /* Print all env variables */ + if (argc == 0) { /* Print all env variables */ char *env, *nxt; for (env = environment.data; *env; env = nxt + 1) { for (nxt = env; *nxt; ++nxt) { if (nxt >= &environment.data[ENV_SIZE]) { - fprintf (stderr, "## Error: " + fprintf(stderr, "## Error: " "environment not terminated\n"); return -1; } }
- printf ("%s\n", env); + printf("%s\n", env); } fw_env_close(opts); return 0; @@ -485,7 +485,7 @@ int fw_printenv(int argc, char *argv[], int value_only, struct env_opts *opts)
val = fw_getenv(name); if (!val) { - fprintf (stderr, "## Error: "%s" not defined\n", name); + fprintf(stderr, "## Error: "%s" not defined\n", name); rc = -1; continue; } @@ -515,15 +515,13 @@ int fw_env_flush(struct env_opts *opts)
/* write environment back to flash */ if (flash_io(O_RDWR)) { - fprintf(stderr, - "Error: can't write fw_env to flash\n"); - return -1; + fprintf(stderr, "Error: can't write fw_env to flash\n"); + return -1; }
return 0; }
- /* * Set/Clear a single variable in the environment. * This is called in sequence to update the environment @@ -548,7 +546,8 @@ int fw_env_write(char *name, char *value) return -1; } } - if ((oldval = envmatch (name, env)) != NULL) + oldval = envmatch(name, env); + if (oldval) break; }
@@ -571,7 +570,7 @@ int fw_env_write(char *name, char *value) errno = EROFS; return -1; } else if (env_flags_validate_varaccess(name, - ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR)) { + ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR)) { const char *defval = fw_getdefenv(name);
if (defval == NULL) @@ -615,21 +614,21 @@ int fw_env_write(char *name, char *value) /* * Append new definition at the end */ - for (env = environment.data; *env || *(env + 1); ++env); + for (env = environment.data; *env || *(env + 1); ++env) + ; if (env > environment.data) ++env; /* * Overflow when: * "name" + "=" + "val" +"\0\0" > CUR_ENVSIZE - (env-environment) */ - len = strlen (name) + 2; + len = strlen(name) + 2; /* add '=' for first arg, ' ' for all others */ len += strlen(value) + 1;
if (len > (&environment.data[ENV_SIZE] - env)) { - fprintf (stderr, - "Error: environment overflow, "%s" deleted\n", - name); + fprintf(stderr, + "Error: environment overflow, "%s" deleted\n", name); return -1; }
@@ -759,7 +758,7 @@ int fw_parse_script(char *fname, struct env_opts *opts) fp = fopen(fname, "r"); if (fp == NULL) { fprintf(stderr, "I cannot open %s for reading\n", - fname); + fname); return -1; } } @@ -774,7 +773,7 @@ int fw_parse_script(char *fname, struct env_opts *opts) */ if (dump[len - 1] != '\n') { fprintf(stderr, - "Line %d not corrected terminated or too long\n", + "Line %d not corrected terminated or too long\n", lineno); ret = -1; break; @@ -807,7 +806,6 @@ int fw_parse_script(char *fname, struct env_opts *opts) else val = NULL; } - #ifdef DEBUG fprintf(stderr, "Setting %s : %s\n", name, val ? val : " removed"); @@ -824,7 +822,7 @@ int fw_parse_script(char *fname, struct env_opts *opts) */ if (fw_env_write(name, val)) { fprintf(stderr, - "fw_env_write returns with error : %s\n", + "fw_env_write returns with error : %s\n", strerror(errno)); ret = -1; break; @@ -867,13 +865,13 @@ static int flash_bad_block(int fd, uint8_t mtd_type, loff_t blockstart) int badblock = ioctl(fd, MEMGETBADBLOCK, &blockstart);
if (badblock < 0) { - perror ("Cannot read bad block mark"); + perror("Cannot read bad block mark"); return badblock; }
if (badblock) { #ifdef DEBUG - fprintf (stderr, "Bad block at 0x%llx, skipping\n", + fprintf(stderr, "Bad block at 0x%llx, skipping\n", (unsigned long long)blockstart); #endif return badblock; @@ -888,8 +886,8 @@ static int flash_bad_block(int fd, uint8_t mtd_type, loff_t blockstart) * bad blocks but makes sure it stays within ENVSECTORS (dev) starting from * the DEVOFFSET (dev) block. On NOR the loop is only run once. */ -static int flash_read_buf (int dev, int fd, void *buf, size_t count, - off_t offset) +static int flash_read_buf(int dev, int fd, void *buf, size_t count, + off_t offset) { size_t blocklen; /* erase / write length - one block on NAND, 0 on NOR */ @@ -901,7 +899,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count, MEMGETBADBLOCK needs 64 bits */ int rc;
- blockstart = (offset / DEVESIZE (dev)) * DEVESIZE (dev); + blockstart = (offset / DEVESIZE(dev)) * DEVESIZE(dev);
/* Offset inside a block */ block_seek = offset - blockstart; @@ -911,7 +909,7 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count, * NAND: calculate which blocks we are reading. We have * to read one block at a time to skip bad blocks. */ - blocklen = DEVESIZE (dev); + blocklen = DEVESIZE(dev);
/* Limit to one block for the first read */ if (readlen > blocklen - block_seek) @@ -923,17 +921,16 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count, /* This only runs once on NOR flash */ while (processed < count) { rc = flash_bad_block(fd, DEVTYPE(dev), blockstart); - if (rc < 0) /* block test failed */ + if (rc < 0) /* block test failed */ return -1;
if (blockstart + block_seek + readlen > environment_end(dev)) { /* End of range is reached */ - fprintf (stderr, - "Too few good blocks within range\n"); + fprintf(stderr, "Too few good blocks within range\n"); return -1; }
- if (rc) { /* block is bad */ + if (rc) { /* block is bad */ blockstart += blocklen; continue; } @@ -942,21 +939,21 @@ static int flash_read_buf (int dev, int fd, void *buf, size_t count, * If a block is bad, we retry in the next block at the same * offset - see env/nand.c::writeenv() */ - lseek (fd, blockstart + block_seek, SEEK_SET); + lseek(fd, blockstart + block_seek, SEEK_SET);
- rc = read (fd, buf + processed, readlen); + rc = read(fd, buf + processed, readlen); if (rc != readlen) { - fprintf (stderr, "Read error on %s: %s\n", - DEVNAME (dev), strerror (errno)); + fprintf(stderr, "Read error on %s: %s\n", + DEVNAME(dev), strerror(errno)); return -1; } #ifdef DEBUG fprintf(stderr, "Read 0x%x bytes at 0x%llx on %s\n", - rc, (unsigned long long) blockstart + block_seek, + rc, (unsigned long long)blockstart + block_seek, DEVNAME(dev)); #endif processed += readlen; - readlen = min (blocklen, count - processed); + readlen = min(blocklen, count - processed); block_seek = 0; blockstart += blocklen; } @@ -1018,7 +1015,7 @@ static int flash_write_buf(int dev, int fd, void *buf, size_t count) * to the end of the block */ write_total = ((block_seek + count + blocklen - 1) / - blocklen) * blocklen; + blocklen) * blocklen; }
/* @@ -1027,11 +1024,11 @@ static int flash_write_buf(int dev, int fd, void *buf, size_t count) * block back again. */ if (write_total > count) { - data = malloc (erase_len); + data = malloc(erase_len); if (!data) { - fprintf (stderr, - "Cannot malloc %zu bytes: %s\n", - erase_len, strerror (errno)); + fprintf(stderr, + "Cannot malloc %zu bytes: %s\n", + erase_len, strerror(errno)); return -1; }
@@ -1047,13 +1044,13 @@ static int flash_write_buf(int dev, int fd, void *buf, size_t count) if (block_seek != 0) fprintf(stderr, " and "); fprintf(stderr, "0x%lx - 0x%lx", - (unsigned long) block_seek + count, - (unsigned long) write_total - 1); + (unsigned long)block_seek + count, + (unsigned long)write_total - 1); } fprintf(stderr, "\n"); #endif /* Overwrite the old environment */ - memcpy (data + block_seek, buf, count); + memcpy(data + block_seek, buf, count); } else { /* * We get here, iff offset is block-aligned and count is a @@ -1077,15 +1074,15 @@ static int flash_write_buf(int dev, int fd, void *buf, size_t count) /* This only runs once on NOR flash and SPI-dataflash */ while (processed < write_total) { rc = flash_bad_block(fd, DEVTYPE(dev), blockstart); - if (rc < 0) /* block test failed */ + if (rc < 0) /* block test failed */ return rc;
if (blockstart + erasesize > environment_end(dev)) { - fprintf (stderr, "End of range reached, aborting\n"); + fprintf(stderr, "End of range reached, aborting\n"); return -1; }
- if (rc) { /* block is bad */ + if (rc) { /* block is bad */ blockstart += blocklen; continue; } @@ -1103,34 +1100,33 @@ static int flash_write_buf(int dev, int fd, void *buf, size_t count) } }
- if (lseek (fd, blockstart, SEEK_SET) == -1) { - fprintf (stderr, - "Seek error on %s: %s\n", - DEVNAME (dev), strerror (errno)); + if (lseek(fd, blockstart, SEEK_SET) == -1) { + fprintf(stderr, + "Seek error on %s: %s\n", + DEVNAME(dev), strerror(errno)); return -1; } - #ifdef DEBUG fprintf(stderr, "Write 0x%llx bytes at 0x%llx\n", - (unsigned long long) erasesize, - (unsigned long long) blockstart); + (unsigned long long)erasesize, + (unsigned long long)blockstart); #endif - if (write (fd, data + processed, erasesize) != erasesize) { - fprintf (stderr, "Write error on %s: %s\n", - DEVNAME (dev), strerror (errno)); + if (write(fd, data + processed, erasesize) != erasesize) { + fprintf(stderr, "Write error on %s: %s\n", + DEVNAME(dev), strerror(errno)); return -1; }
if (DEVTYPE(dev) != MTD_ABSENT) ioctl(fd, MEMLOCK, &erase);
- processed += erasesize; + processed += erasesize; block_seek = 0; blockstart += erasesize; }
if (write_total > count) - free (data); + free(data);
return processed; } @@ -1138,30 +1134,30 @@ static int flash_write_buf(int dev, int fd, void *buf, size_t count) /* * Set obsolete flag at offset - NOR flash only */ -static int flash_flag_obsolete (int dev, int fd, off_t offset) +static int flash_flag_obsolete(int dev, int fd, off_t offset) { int rc; struct erase_info_user erase;
- erase.start = DEVOFFSET (dev); - erase.length = DEVESIZE (dev); + erase.start = DEVOFFSET(dev); + erase.length = DEVESIZE(dev); /* This relies on the fact, that obsolete_flag == 0 */ - rc = lseek (fd, offset, SEEK_SET); + rc = lseek(fd, offset, SEEK_SET); if (rc < 0) { - fprintf (stderr, "Cannot seek to set the flag on %s \n", - DEVNAME (dev)); + fprintf(stderr, "Cannot seek to set the flag on %s\n", + DEVNAME(dev)); return rc; } - ioctl (fd, MEMUNLOCK, &erase); - rc = write (fd, &obsolete_flag, sizeof (obsolete_flag)); - ioctl (fd, MEMLOCK, &erase); + ioctl(fd, MEMUNLOCK, &erase); + rc = write(fd, &obsolete_flag, sizeof(obsolete_flag)); + ioctl(fd, MEMLOCK, &erase); if (rc < 0) - perror ("Could not set obsolete flag"); + perror("Could not set obsolete flag");
return rc; }
-static int flash_write (int fd_current, int fd_target, int dev_target) +static int flash_write(int fd_current, int fd_target, int dev_target) { int rc;
@@ -1175,14 +1171,14 @@ static int flash_write (int fd_current, int fd_target, int dev_target) *environment.flags = active_flag; break; default: - fprintf (stderr, "Unimplemented flash scheme %u \n", - environment.flag_scheme); + fprintf(stderr, "Unimplemented flash scheme %u\n", + environment.flag_scheme); return -1; }
#ifdef DEBUG fprintf(stderr, "Writing new environment at 0x%llx on %s\n", - DEVOFFSET (dev_target), DEVNAME (dev_target)); + DEVOFFSET(dev_target), DEVNAME(dev_target)); #endif
if (IS_UBI(dev_target)) { @@ -1198,20 +1194,20 @@ static int flash_write (int fd_current, int fd_target, int dev_target)
if (environment.flag_scheme == FLAG_BOOLEAN) { /* Have to set obsolete flag */ - off_t offset = DEVOFFSET (dev_current) + - offsetof (struct env_image_redundant, flags); + off_t offset = DEVOFFSET(dev_current) + + offsetof(struct env_image_redundant, flags); #ifdef DEBUG fprintf(stderr, "Setting obsolete flag in environment at 0x%llx on %s\n", - DEVOFFSET (dev_current), DEVNAME (dev_current)); + DEVOFFSET(dev_current), DEVNAME(dev_current)); #endif - flash_flag_obsolete (dev_current, fd_current, offset); + flash_flag_obsolete(dev_current, fd_current, offset); }
return 0; }
-static int flash_read (int fd) +static int flash_read(int fd) { int rc;
@@ -1229,16 +1225,16 @@ static int flash_read (int fd) return 0; }
-static int flash_io (int mode) +static int flash_io(int mode) { int fd_current, fd_target, rc, dev_target;
/* dev_current: fd_current, erase_current */ - fd_current = open (DEVNAME (dev_current), mode); + fd_current = open(DEVNAME(dev_current), mode); if (fd_current < 0) { - fprintf (stderr, - "Can't open %s: %s\n", - DEVNAME (dev_current), strerror (errno)); + fprintf(stderr, + "Can't open %s: %s\n", + DEVNAME(dev_current), strerror(errno)); return -1; }
@@ -1247,12 +1243,11 @@ static int flash_io (int mode) /* switch to next partition for writing */ dev_target = !dev_current; /* dev_target: fd_target, erase_target */ - fd_target = open (DEVNAME (dev_target), mode); + fd_target = open(DEVNAME(dev_target), mode); if (fd_target < 0) { - fprintf (stderr, - "Can't open %s: %s\n", - DEVNAME (dev_target), - strerror (errno)); + fprintf(stderr, + "Can't open %s: %s\n", + DEVNAME(dev_target), strerror(errno)); rc = -1; goto exit; } @@ -1261,40 +1256,38 @@ static int flash_io (int mode) 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) && !(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)); + fprintf(stderr, + "fsync failed on %s: %s\n", + DEVNAME(dev_current), strerror(errno)); }
- if (close (fd_target)) { - fprintf (stderr, + if (close(fd_target)) { + fprintf(stderr, "I/O error on %s: %s\n", - DEVNAME (dev_target), - strerror (errno)); + DEVNAME(dev_target), strerror(errno)); rc = -1; } } } else { - rc = flash_read (fd_current); + rc = flash_read(fd_current); }
-exit: - if (close (fd_current)) { - fprintf (stderr, - "I/O error on %s: %s\n", - DEVNAME (dev_current), strerror (errno)); + exit: + if (close(fd_current)) { + fprintf(stderr, + "I/O error on %s: %s\n", + DEVNAME(dev_current), strerror(errno)); return -1; }
@@ -1322,7 +1315,7 @@ int fw_env_open(struct env_opts *opts) if (!opts) opts = &default_opts;
- if (parse_config(opts)) /* should fill envdevices */ + if (parse_config(opts)) /* should fill envdevices */ return -EINVAL;
addr0 = calloc(1, CUR_ENVSIZE); @@ -1339,14 +1332,14 @@ int fw_env_open(struct env_opts *opts)
if (HaveRedundEnv) { redundant = addr0; - environment.crc = &redundant->crc; - environment.flags = &redundant->flags; - environment.data = redundant->data; + environment.crc = &redundant->crc; + environment.flags = &redundant->flags; + environment.data = redundant->data; } else { single = addr0; - environment.crc = &single->crc; - environment.flags = NULL; - environment.data = single->data; + environment.crc = &single->crc; + environment.flags = NULL; + environment.data = single->data; }
dev_current = 0; @@ -1355,14 +1348,15 @@ int fw_env_open(struct env_opts *opts) goto open_cleanup; }
- crc0 = crc32 (0, (uint8_t *) environment.data, ENV_SIZE); + crc0 = crc32(0, (uint8_t *)environment.data, ENV_SIZE);
crc0_ok = (crc0 == *environment.crc); if (!HaveRedundEnv) { if (!crc0_ok) { - fprintf (stderr, + fprintf(stderr, "Warning: Bad CRC, using default environment\n"); - memcpy(environment.data, default_environment, sizeof default_environment); + memcpy(environment.data, default_environment, + sizeof(default_environment)); } } else { flag0 = *environment.flags; @@ -1406,12 +1400,12 @@ int fw_env_open(struct env_opts *opts) IS_UBI(dev_current) == IS_UBI(!dev_current)) { environment.flag_scheme = FLAG_INCREMENTAL; } else { - fprintf (stderr, "Incompatible flash types!\n"); + fprintf(stderr, "Incompatible flash types!\n"); ret = -EINVAL; goto open_cleanup; }
- crc1 = crc32 (0, (uint8_t *) redundant->data, ENV_SIZE); + crc1 = crc32(0, (uint8_t *)redundant->data, ENV_SIZE);
crc1_ok = (crc1 == redundant->crc); flag1 = redundant->flags; @@ -1421,10 +1415,10 @@ int fw_env_open(struct env_opts *opts) } else if (!crc0_ok && crc1_ok) { dev_current = 1; } else if (!crc0_ok && !crc1_ok) { - fprintf (stderr, + fprintf(stderr, "Warning: Bad CRC, using default environment\n"); - memcpy (environment.data, default_environment, - sizeof default_environment); + memcpy(environment.data, default_environment, + sizeof(default_environment)); dev_current = 0; } else { switch (environment.flag_scheme) { @@ -1451,12 +1445,12 @@ int fw_env_open(struct env_opts *opts) else if ((flag1 == 255 && flag0 == 0) || flag0 >= flag1) dev_current = 0; - else /* flag1 > flag0 */ + else /* flag1 > flag0 */ dev_current = 1; break; default: - fprintf (stderr, "Unknown flag scheme %u \n", - environment.flag_scheme); + fprintf(stderr, "Unknown flag scheme %u\n", + environment.flag_scheme); return -1; } } @@ -1467,15 +1461,15 @@ int fw_env_open(struct env_opts *opts) * flags before writing out */ if (dev_current) { - environment.image = addr1; - environment.crc = &redundant->crc; - environment.flags = &redundant->flags; - environment.data = redundant->data; - free (addr0); + environment.image = addr1; + environment.crc = &redundant->crc; + environment.flags = &redundant->flags; + environment.data = redundant->data; + free(addr0); } else { - environment.image = addr0; + environment.image = addr0; /* Other pointers are already set */ - free (addr1); + free(addr1); } #ifdef DEBUG fprintf(stderr, "Selected env in %s\n", DEVNAME(dev_current)); @@ -1483,7 +1477,7 @@ int fw_env_open(struct env_opts *opts) } return 0;
-open_cleanup: + open_cleanup: if (addr0) free(addr0);
@@ -1518,15 +1512,13 @@ static int check_device_config(int dev) fd = open(DEVNAME(dev), O_RDONLY); if (fd < 0) { fprintf(stderr, - "Cannot open %s: %s\n", - DEVNAME(dev), strerror(errno)); + "Cannot open %s: %s\n", DEVNAME(dev), strerror(errno)); return -1; }
rc = fstat(fd, &st); if (rc < 0) { - fprintf(stderr, "Cannot stat the file %s\n", - DEVNAME(dev)); + fprintf(stderr, "Cannot stat the file %s\n", DEVNAME(dev)); goto err; }
@@ -1571,14 +1563,16 @@ static int check_device_config(int dev) if (DEVOFFSET(dev) < 0) { rc = ioctl(fd, BLKGETSIZE64, &size); if (rc < 0) { - fprintf(stderr, "Could not get block device size on %s\n", + fprintf(stderr, + "Could not get block device size on %s\n", DEVNAME(dev)); goto err; }
DEVOFFSET(dev) = DEVOFFSET(dev) + size; #ifdef DEBUG - fprintf(stderr, "Calculated device offset 0x%llx on %s\n", + fprintf(stderr, + "Calculated device offset 0x%llx on %s\n", DEVOFFSET(dev), DEVNAME(dev)); #endif } @@ -1589,18 +1583,20 @@ static int check_device_config(int dev) ENVSECTORS(dev) = DIV_ROUND_UP(ENVSIZE(dev), DEVESIZE(dev));
if (DEVOFFSET(dev) % DEVESIZE(dev) != 0) { - fprintf(stderr, "Environment does not start on (erase) block boundary\n"); + fprintf(stderr, + "Environment does not start on (erase) block boundary\n"); errno = EINVAL; return -1; }
if (ENVSIZE(dev) > ENVSECTORS(dev) * DEVESIZE(dev)) { - fprintf(stderr, "Environment does not fit into available sectors\n"); + fprintf(stderr, + "Environment does not fit into available sectors\n"); errno = EINVAL; return -1; }
-err: + err: close(fd); return rc; } @@ -1620,33 +1616,33 @@ static int parse_config(struct env_opts *opts) return -1; } #else - DEVNAME (0) = DEVICE1_NAME; - DEVOFFSET (0) = DEVICE1_OFFSET; - ENVSIZE (0) = ENV1_SIZE; + DEVNAME(0) = DEVICE1_NAME; + DEVOFFSET(0) = DEVICE1_OFFSET; + ENVSIZE(0) = ENV1_SIZE;
/* Set defaults for DEVESIZE, ENVSECTORS later once we * know DEVTYPE */ #ifdef DEVICE1_ESIZE - DEVESIZE (0) = DEVICE1_ESIZE; + DEVESIZE(0) = DEVICE1_ESIZE; #endif #ifdef DEVICE1_ENVSECTORS - ENVSECTORS (0) = DEVICE1_ENVSECTORS; + ENVSECTORS(0) = DEVICE1_ENVSECTORS; #endif
#ifdef HAVE_REDUND - DEVNAME (1) = DEVICE2_NAME; - DEVOFFSET (1) = DEVICE2_OFFSET; - ENVSIZE (1) = ENV2_SIZE; + DEVNAME(1) = DEVICE2_NAME; + DEVOFFSET(1) = DEVICE2_OFFSET; + ENVSIZE(1) = ENV2_SIZE;
/* Set defaults for DEVESIZE, ENVSECTORS later once we * know DEVTYPE */ #ifdef DEVICE2_ESIZE - DEVESIZE (1) = DEVICE2_ESIZE; + DEVESIZE(1) = DEVICE2_ESIZE; #endif #ifdef DEVICE2_ENVSECTORS - ENVSECTORS (1) = DEVICE2_ENVSECTORS; + ENVSECTORS(1) = DEVICE2_ENVSECTORS; #endif HaveRedundEnv = 1; #endif @@ -1675,7 +1671,7 @@ static int parse_config(struct env_opts *opts) }
#if defined(CONFIG_FILE) -static int get_config (char *fname) +static int get_config(char *fname) { FILE *fp; int i = 0; @@ -1683,11 +1679,11 @@ static int get_config (char *fname) char dump[128]; char *devname;
- fp = fopen (fname, "r"); + fp = fopen(fname, "r"); if (fp == NULL) return -1;
- while (i < 2 && fgets (dump, sizeof (dump), fp)) { + while (i < 2 && fgets(dump, sizeof(dump), fp)) { /* Skip incomplete conversions and comment strings */ if (dump[0] == '#') continue; @@ -1695,9 +1691,7 @@ static int get_config (char *fname) rc = sscanf(dump, "%ms %lli %lx %lx %lx", &devname, &DEVOFFSET(i), - &ENVSIZE(i), - &DEVESIZE(i), - &ENVSECTORS(i)); + &ENVSIZE(i), &DEVESIZE(i), &ENVSECTORS(i));
if (rc < 3) continue; @@ -1710,10 +1704,10 @@ static int get_config (char *fname)
i++; } - fclose (fp); + fclose(fp);
HaveRedundEnv = i - 1; - if (!i) { /* No valid entries found */ + if (!i) { /* No valid entries found */ errno = EINVAL; return -1; } else
On Fri, Mar 09, 2018 at 12:12:59PM +0000, Alex Kiernan wrote:
Pass tools/env/fw_env.c through indent to correct style violations. This commit consists of only one non-whitespace change:
tools/env/fw_env.c:549: error: do not use assignment in if condition
Signed-off-by: Alex Kiernan alex.kiernan@gmail.com
Applied to u-boot/master, thanks!
Replace HaveRedundEnv with have_redund_env to fix style violation.
Signed-off-by: Alex Kiernan alex.kiernan@gmail.com ---
Changes in v2: - new
tools/env/fw_env.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 9d5ccfc..f3bfee4 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -116,7 +116,7 @@ static struct environment environment = { .flag_scheme = FLAG_NONE, };
-static int HaveRedundEnv = 0; +static int have_redund_env;
static unsigned char active_flag = 1; /* obsolete_flag must be 0 to efficiently set it on NOR flash without erasing */ @@ -1239,7 +1239,7 @@ static int flash_io(int mode) }
if (mode == O_RDWR) { - if (HaveRedundEnv) { + if (have_redund_env) { /* switch to next partition for writing */ dev_target = !dev_current; /* dev_target: fd_target, erase_target */ @@ -1264,7 +1264,7 @@ static int flash_io(int mode) DEVNAME(dev_current), strerror(errno)); }
- if (HaveRedundEnv) { + if (have_redund_env) { if (fsync(fd_target) && !(errno == EINVAL || errno == EROFS)) { fprintf(stderr, @@ -1330,7 +1330,7 @@ int fw_env_open(struct env_opts *opts) /* read environment from FLASH to local buffer */ environment.image = addr0;
- if (HaveRedundEnv) { + if (have_redund_env) { redundant = addr0; environment.crc = &redundant->crc; environment.flags = &redundant->flags; @@ -1351,7 +1351,7 @@ int fw_env_open(struct env_opts *opts) crc0 = crc32(0, (uint8_t *)environment.data, ENV_SIZE);
crc0_ok = (crc0 == *environment.crc); - if (!HaveRedundEnv) { + if (!have_redund_env) { if (!crc0_ok) { fprintf(stderr, "Warning: Bad CRC, using default environment\n"); @@ -1644,14 +1644,14 @@ static int parse_config(struct env_opts *opts) #ifdef DEVICE2_ENVSECTORS ENVSECTORS(1) = DEVICE2_ENVSECTORS; #endif - HaveRedundEnv = 1; + have_redund_env = 1; #endif #endif rc = check_device_config(0); if (rc < 0) return rc;
- if (HaveRedundEnv) { + if (have_redund_env) { rc = check_device_config(1); if (rc < 0) return rc; @@ -1664,7 +1664,7 @@ static int parse_config(struct env_opts *opts) }
usable_envsize = CUR_ENVSIZE - sizeof(uint32_t); - if (HaveRedundEnv) + if (have_redund_env) usable_envsize -= sizeof(char);
return 0; @@ -1706,7 +1706,7 @@ static int get_config(char *fname) } fclose(fp);
- HaveRedundEnv = i - 1; + have_redund_env = i - 1; if (!i) { /* No valid entries found */ errno = EINVAL; return -1;
On Fri, Mar 09, 2018 at 12:13:00PM +0000, Alex Kiernan wrote:
Replace HaveRedundEnv with have_redund_env to fix style violation.
Signed-off-by: Alex Kiernan alex.kiernan@gmail.com
Applied to u-boot/master, thanks!
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 Reviewed-by: Stefano Babic sbabic@denx.de ---
Changes in v2: - Acommodate white space changes from predecessor commit
tools/env/fw_env.c | 92 +++++++++++++++++++++++++++++------------------------- 1 file changed, 50 insertions(+), 42 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index f3bfee4..600fe5d 100644 --- a/tools/env/fw_env.c +++ b/tools/env/fw_env.c @@ -1225,9 +1225,57 @@ static int flash_read(int fd) return 0; }
+static int flash_io_write(int fd_current) +{ + int fd_target, rc, dev_target; + + if (have_redund_env) { + /* 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; + } + + 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 (have_redund_env) { + 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; + } + } + exit: + return rc; +} + static int flash_io(int mode) { - int fd_current, fd_target, rc, dev_target; + int fd_current, rc;
/* dev_current: fd_current, erase_current */ fd_current = open(DEVNAME(dev_current), mode); @@ -1239,51 +1287,11 @@ static int flash_io(int mode) }
if (mode == O_RDWR) { - if (have_redund_env) { - /* 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); - - if (fsync(fd_current) && !(errno == EINVAL || errno == EROFS)) { - fprintf(stderr, - "fsync failed on %s: %s\n", - DEVNAME(dev_current), strerror(errno)); - } - - if (have_redund_env) { - 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; - } - } + 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",
On Fri, Mar 09, 2018 at 12:13:01PM +0000, 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 Reviewed-by: Stefano Babic sbabic@denx.de
Applied to u-boot/master, thanks!
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 ---
Changes in v2: - Use mkstemp() to create temporary filename, not a fixed one
tools/env/fw_env.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 3 deletions(-)
diff --git a/tools/env/fw_env.c b/tools/env/fw_env.c index 600fe5d..77eac3d 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> @@ -1225,9 +1226,48 @@ 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/XXXXXX", *dname); + if (rc == -1) + goto err; + + rc = mkstemp(temp_name); + if (rc == -1) { + /* fall back to in place write */ + fprintf(stderr, + "Can't create %s: %s\n", temp_name, strerror(errno)); + 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 (have_redund_env) { /* switch to next partition for writing */ @@ -1242,8 +1282,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); @@ -1254,7 +1303,7 @@ static int flash_io_write(int fd_current) DEVNAME(dev_current), strerror(errno)); }
- if (have_redund_env) { + if (fd_current != fd_target) { if (fsync(fd_target) && !(errno == EINVAL || errno == EROFS)) { fprintf(stderr, @@ -1268,6 +1317,34 @@ static int flash_io_write(int fd_current) DEVNAME(dev_target), 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;
On Fri, Mar 09, 2018 at 12:13:02PM +0000, 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.
Signed-off-by: Alex Kiernan alex.kiernan@gmail.com
Applied to u-boot/master, thanks!
Dear Alex,
In message 1520597582-12979-1-git-send-email-alex.kiernan@gmail.com you wrote:
For environments stored on filesystems where you can't have a redundant configuration, rather than just over-writing the existing environment in fw_setenv, do the tradtional create temporary file, rename, sync, sync directory dance to achieve ACID semantics when writing through fw_setenv.
I isagree with the statement that we _can't have_ redundant environments in a file system. It may not be supported by the current implementation, but there is actually nothing that would prevent us to come up with more powerful code that supports exactly such a feature.
[This does not mean that I disagree with your patch - on contary, I think it's good.]
Best regards,
Wolfgang Denk
On Fri, Mar 9, 2018 at 12:30 PM, Wolfgang Denk wd@denx.de wrote:
Dear Alex,
In message 1520597582-12979-1-git-send-email-alex.kiernan@gmail.com you wrote:
For environments stored on filesystems where you can't have a redundant configuration, rather than just over-writing the existing environment in fw_setenv, do the tradtional create temporary file, rename, sync, sync directory dance to achieve ACID semantics when writing through fw_setenv.
I isagree with the statement that we _can't have_ redundant
Yeah... sorry, sloppy wording on my part.
environments in a file system. It may not be supported by the current implementation, but there is actually nothing that would prevent us to come up with more powerful code that supports exactly such a feature.
[This does not mean that I disagree with your patch - on contary, I think it's good.]
Thanks!
participants (3)
-
Alex Kiernan -
Tom Rini -
Wolfgang Denk