[Skiboot] [PATCH 13/17] external/pflash: Remove use of exit() and fix memory leaks
Samuel Mendoza-Jonas
sam at mendozajonas.com
Wed Jul 26 16:08:56 AEST 2017
On Fri, 2017-07-21 at 16:36 +1000, Cyril Bur wrote:
> Using exit() all over the place has lead to a huge mess of leaving all
> sorts of dangling references to malloc()ed memory, to blocklevel_devices
> and even sometimes file descriptors.
>
> Stop using exit() and simply report everything back to the main where
> everything can be freed on the way back out.
>
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
> external/pflash/pflash.c | 365 ++++++++++++++++++++++++++---------------------
> 1 file changed, 200 insertions(+), 165 deletions(-)
>
> diff --git a/external/pflash/pflash.c b/external/pflash/pflash.c
> index 2cec6d69..80675c85 100644
> --- a/external/pflash/pflash.c
> +++ b/external/pflash/pflash.c
> @@ -57,28 +57,23 @@ const char *flashfilename = NULL;
> static bool must_confirm = true;
> static bool dummy_run;
> static bool bmc_flash;
> -static uint32_t ffs_toc = 0;
> -static int flash_side = 0;
>
> #define FILE_BUF_SIZE 0x10000
> static uint8_t file_buf[FILE_BUF_SIZE] __aligned(0x1000);
>
> -static struct ffs_handle *ffsh;
> -static int32_t ffs_index = -1;
> -
> -static void check_confirm(void)
> +static bool check_confirm(void)
> {
> char yes[8], *p;
>
> if (!must_confirm)
> - return;
> + return true;
>
> printf("WARNING ! This will modify your %s flash chip content !\n",
> bmc_flash ? "BMC" : "HOST");
> printf("Enter \"yes\" to confirm:");
> memset(yes, 0, sizeof(yes));
> if (!fgets(yes, 7, stdin))
> - exit(1);
> + return false;
> p = strchr(yes, 10);
> if (p)
> *p = 0;
> @@ -87,40 +82,33 @@ static void check_confirm(void)
> *p = 0;
> if (strcmp(yes, "yes")) {
> printf("Operation cancelled !\n");
> - exit(1);
> + return false;
> }
> must_confirm = false;
> + return true;
> }
>
> -static void print_ffs_info(struct flash_details *flash, uint32_t toc_offset)
> +static uint32_t print_ffs_info(struct ffs_handle *ffsh, uint32_t toc)
> {
> - struct ffs_handle *ffs_handle;
> - uint32_t other_side_offset = 0;
> struct ffs_entry *ent;
> + uint32_t next_toc = toc;
> int rc;
> - uint32_t i;
> + int i;
>
> printf("\n");
> - printf("TOC at 0x%08x Partitions:\n", toc_offset);
> + printf("TOC at 0x%08x Partitions:\n", toc);
> printf("-----------\n");
>
> - rc = ffs_init(toc_offset, flash->total_size, flash->bl,
> - &ffs_handle, 0);
> - if (rc) {
> - fprintf(stderr, "Error %d opening ffs !\n", rc);
> - return;
> - }
> -
> - for (i = 0; rc == 0; i++) {
> + for (i = 0;; i++) {
> uint32_t start, size, act, end;
> char *name = NULL, *flags;
> int l;
>
> - rc = ffs_part_info(ffs_handle, i, &name, &start, &size, &act, NULL);
> + rc = ffs_part_info(ffsh, i, &name, &start, &size, &act, NULL);
> if (rc == FFS_ERR_PART_NOT_FOUND)
> break;
>
> - ent = ffs_entry_get(ffs_handle, i);
> + ent = ffs_entry_get(ffsh, i);
> if (rc || !ent) {
> fprintf(stderr, "Error %d scanning partitions\n",
> !ent ? FFS_ERR_PART_NOT_FOUND : rc);
> @@ -141,22 +129,41 @@ static void print_ffs_info(struct flash_details *flash, uint32_t toc_offset)
> i, name, start, end, act, flags);
>
> if (strcmp(name, "OTHER_SIDE") == 0)
> - other_side_offset = start;
> + next_toc = start;
>
> free(flags);
> out:
> free(name);
> }
>
> - ffs_close(ffs_handle);
> -
> - if (other_side_offset)
> - print_ffs_info(flash, other_side_offset);
> + return next_toc;
> }
>
> +static struct ffs_handle *open_ffs(struct flash_details *flash)
> +{
> + struct ffs_handle *ffsh;
> + int rc;
>
> -static void print_flash_info(struct flash_details *flash, uint32_t toc)
> + rc = ffs_init(flash->toc, flash->total_size,
> + flash->bl, &ffsh, 0);
> + if (rc) {
> + fprintf(stderr, "Error %d opening ffs !\n", rc);
> + if (flash->toc) {
> + fprintf(stderr, "You specified 0x%08lx as the libffs TOC\n"
> + "Looks like it doesn't exist\n", flash->toc);
> + return NULL;
> + }
> + }
> +
> + return ffsh;
> +}
> +
> +static void print_flash_info(struct flash_details *flash)
> {
> + struct ffs_handle *ffsh;
> + uint32_t next_toc;
> + uint32_t toc;
> +
> printf("Flash info:\n");
> printf("-----------\n");
> printf("Name = %s\n", flash->name);
> @@ -166,114 +173,137 @@ static void print_flash_info(struct flash_details *flash, uint32_t toc)
> if (bmc_flash)
> return;
>
> - print_ffs_info(flash, toc);
> + toc = flash->toc;
> +
> + ffsh = open_ffs(flash);
> + if (!ffsh)
> + return;
> +
> + next_toc = print_ffs_info(ffsh, toc);
> + while(next_toc != toc) {
> + struct ffs_handle *next_ffsh;
> +
> + flash->toc = next_toc;
> + next_ffsh = open_ffs(flash);
> + if (!next_ffsh)
> + break;
> + next_toc = print_ffs_info(next_ffsh, next_toc);
> + }
> + flash->toc = toc;
> }
>
> -static int open_partition(struct flash_details *flash, const char *name)
> +static struct ffs_handle *open_partition(struct flash_details *flash,
> + const char *name, uint32_t *index)
> {
> - uint32_t index;
> + struct ffs_handle *ffsh;
> int rc;
>
> - /* Open libffs if needed */
> - if (!ffsh) {
> - rc = ffs_init(flash->toc, flash->total_size, flash->bl, &ffsh, 0);
> - if (rc) {
> - fprintf(stderr, "Error %d opening ffs !\n", rc);
> - if (flash->toc)
> - fprintf(stderr, "You specified 0x%08lx as the libffs TOC"
> - " looks like it doesn't exist\n", flash->toc);
> - return rc;
> - }
> - }
> + ffsh = open_ffs(flash);
> + if (!ffsh)
> + return NULL;
> +
> + if (!name)
> + /* Just open the FFS */
> + return ffsh;
>
> /* Find partition */
> - rc = ffs_lookup_part(ffsh, name, &index);
> + rc = ffs_lookup_part(ffsh, name, index);
> if (rc == FFS_ERR_PART_NOT_FOUND) {
> fprintf(stderr, "Partition '%s' not found !\n", name);
> - return rc;
> + goto out;
> }
> if (rc) {
> fprintf(stderr, "Error %d looking for partition '%s' !\n",
> rc, name);
> - return rc;
> + goto out;
> }
> + return ffsh;
> +out:
> + ffs_close(ffsh);
> + return NULL;
> +}
>
> - ffs_index = index;
> - return 0;
> +static struct ffs_handle *lookup_partition_at_toc(struct flash_details *flash,
> + const char *name, uint32_t *index)
> +{
> + return open_partition(flash, name, index);
> }
>
> -static void lookup_partition(struct flash_details *flash, const char *name)
> +static struct ffs_handle *lookup_partition_at_side(struct flash_details *flash,
> + int side, const char *name, uint32_t *index)
> {
> + uint32_t toc = 0;
> int rc;
>
> - if (flash_side == 1) {
> - uint32_t other_side_offset;
> + if (side == 1) {
> + struct ffs_handle *ffsh;
> + uint32_t side_index;
>
> - rc = open_partition(flash, "OTHER_SIDE");
> - if (rc == FFS_ERR_PART_NOT_FOUND)
> - fprintf(stderr, "side 1 was specified but there doesn't appear"
> - " to be a second side to this flash\n");
> - if (rc)
> - exit(1);
> + ffsh = open_partition(flash, "OTHER_SIDE", &side_index);
> + if (!ffsh)
> + return NULL;
>
> /* Just need to know where it starts */
> - rc = ffs_part_info(ffsh, ffs_index, NULL, &other_side_offset, NULL, NULL, NULL);
> - if (rc)
> - exit(1);
> -
> + rc = ffs_part_info(ffsh, side_index, NULL, &toc, NULL, NULL, NULL);
> ffs_close(ffsh);
> - ffsh = NULL;
> -
> - ffs_toc = other_side_offset;
> + if (rc)
> + return NULL;
> }
>
> - rc = open_partition(flash, name);
> - if (rc)
> - exit(1);
> + flash->toc = toc;
> + return lookup_partition_at_toc(flash, name, index);
> }
>
> -static void erase_chip(struct blocklevel_device *bl)
> +static int erase_chip(struct blocklevel_device *bl)
> {
> + bool confirm;
> int rc;
>
> printf("About to erase chip !\n");
> - check_confirm();
> + confirm = check_confirm();
> + if (!confirm)
> + return 1;
>
> printf("Erasing... (may take a while !) ");
> fflush(stdout);
>
> if (dummy_run) {
> printf("skipped (dummy)\n");
> - return;
> + return 1;
> }
>
> rc = arch_flash_erase_chip(bl);
> if (rc) {
> fprintf(stderr, "Error %d erasing chip\n", rc);
> - exit(1);
> + return 1;
> }
>
> printf("done !\n");
> + return 0;
> }
>
> -static void erase_range(struct blocklevel_device *bl,
> - uint32_t start, uint32_t size, bool will_program)
> +static int erase_range(struct blocklevel_device *bl,
> + uint32_t start, uint32_t size, bool will_program,
> + struct ffs_handle *ffsh, int ffs_index)
> {
> + bool confirm;
> int rc;
>
> printf("About to erase 0x%08x..0x%08x !\n", start, start + size);
> - check_confirm();
> + confirm = check_confirm();
> + if (!confirm)
> + return 1;
>
> if (dummy_run) {
> printf("skipped (dummy)\n");
> - return;
> + return 1;
> }
>
> printf("Erasing...\n");
> rc = blocklevel_smart_erase(bl, start, size);
> if (rc) {
> fprintf(stderr, "Failed to blocklevel_smart_erase(): %d\n", rc);
> - return;
> + return 1;
> }
>
> /* If this is a flash partition, mark it empty if we aren't
> @@ -283,15 +313,21 @@ static void erase_range(struct blocklevel_device *bl,
> printf("Updating actual size in partition header...\n");
> ffs_update_act_size(ffsh, ffs_index, 0);
> }
> + return 0;
> }
> -static void set_ecc(struct blocklevel_device *bl, uint32_t start, uint32_t size)
> +
> +static int set_ecc(struct blocklevel_device *bl, uint32_t start, uint32_t size)
> {
> uint32_t i = start + 8;
> uint8_t ecc = 0;
> + bool confirm;
>
> printf("About to erase and set ECC bits in region 0x%08x to 0x%08x\n", start, start + size);
> - check_confirm();
> - erase_range(bl, start, size, true);
> + confirm = check_confirm();
> + if (!confirm)
> + return 1;
> +
> + erase_range(bl, start, size, true, NULL, 0);
>
> printf("Programming ECC bits...\n");
> progress_init(size);
> @@ -301,39 +337,46 @@ static void set_ecc(struct blocklevel_device *bl, uint32_t start, uint32_t size)
> progress_tick(i - start);
> }
> progress_end();
> + return 0;
> }
>
> -static void program_file(struct blocklevel_device *bl,
> - const char *file, uint32_t start, uint32_t size)
> +static int program_file(struct blocklevel_device *bl,
> + const char *file, uint32_t start, uint32_t size,
> + struct ffs_handle *ffsh, int ffs_index)
> {
> - int fd;
> + bool confirm;
> + int fd, rc = 0;
> uint32_t actual_size = 0;
>
> fd = open(file, O_RDONLY);
> if (fd == -1) {
> perror("Failed to open file");
> - exit(1);
> + return 1;
> }
> printf("About to program \"%s\" at 0x%08x..0x%08x !\n",
> file, start, start + size);
> - check_confirm();
> + confirm = check_confirm();
> + if (!confirm) {
> + rc = 1;
> + goto out;
> + }
>
> if (dummy_run) {
> printf("skipped (dummy)\n");
> - close(fd);
> - return;
> + rc = 1;
> + goto out;
> }
>
> printf("Programming & Verifying...\n");
> progress_init(size >> 8);
> while(size) {
> ssize_t len;
> - int rc;
>
> len = read(fd, file_buf, FILE_BUF_SIZE);
> if (len < 0) {
> perror("Error reading file");
> - exit(1);
> + rc = 1;
> + goto out;
> }
> if (len == 0)
> break;
> @@ -349,31 +392,33 @@ static void program_file(struct blocklevel_device *bl,
> else
> fprintf(stderr, "Flash write error %d for"
> " chunk at 0x%08x\n", rc, start);
> - exit(1);
> + goto out;
> }
> start += len;
> progress_tick(actual_size >> 8);
> }
> progress_end();
> - close(fd);
>
> /* If this is a flash partition, adjust its size */
> if (ffsh && ffs_index >= 0) {
> printf("Updating actual size in partition header...\n");
> ffs_update_act_size(ffsh, ffs_index, actual_size);
> }
> +out:
> + close(fd);
> + return rc;
> }
>
> -static void do_read_file(struct blocklevel_device *bl, const char *file,
> +static int do_read_file(struct blocklevel_device *bl, const char *file,
> uint32_t start, uint32_t size)
> {
> - int fd;
> + int fd, rc = 0;
> uint32_t done = 0;
>
> fd = open(file, O_WRONLY | O_TRUNC | O_CREAT, 00666);
> if (fd == -1) {
> perror("Failed to open file");
> - exit(1);
> + return 1;
> }
> printf("Reading to \"%s\" from 0x%08x..0x%08x !\n",
> file, start, start + size);
> @@ -381,19 +426,18 @@ static void do_read_file(struct blocklevel_device *bl, const char *file,
> progress_init(size >> 8);
> while(size) {
> ssize_t len;
> - int rc;
>
> len = size > FILE_BUF_SIZE ? FILE_BUF_SIZE : size;
> rc = blocklevel_read(bl, start, file_buf, len);
> if (rc) {
> fprintf(stderr, "Flash read error %d for"
> " chunk at 0x%08x\n", rc, start);
> - exit(1);
> + break;
> }
> rc = write(fd, file_buf, len);
> if (rc < 0) {
> perror("Error writing file");
> - exit(1);
> + break;
> }
> start += len;
> size -= len;
> @@ -402,6 +446,7 @@ static void do_read_file(struct blocklevel_device *bl, const char *file,
> }
> progress_end();
> close(fd);
> + return rc;
> }
>
> static void enable_4B_addresses(struct blocklevel_device *bl)
> @@ -417,7 +462,6 @@ static void enable_4B_addresses(struct blocklevel_device *bl)
> } else {
> fprintf(stderr, "Error %d enabling 4b mode\n", rc);
> }
> - exit(1);
> }
> }
>
> @@ -434,55 +478,28 @@ static void disable_4B_addresses(struct blocklevel_device *bl)
> } else {
> fprintf(stderr, "Error %d enabling 4b mode\n", rc);
> }
> - exit(1);
> }
> }
Unless I'm missing something in main(), don't these two need to return
rc?
>
> -static void print_partition_detail(struct flash_details *flash,
> - uint32_t toc_offset, uint32_t part_id, char *name)
> +static void print_partition_detail(struct ffs_handle *ffsh, uint32_t part_id)
> {
> uint32_t start, size, act, end;
> - struct ffs_handle *ffs_handle;
> char *ent_name = NULL, *flags;
> struct ffs_entry *ent;
> int rc, l;
>
> - rc = ffs_init(toc_offset, flash->total_size, flash->bl, &ffs_handle, 0);
> + rc = ffs_part_info(ffsh, part_id, &ent_name, &start, &size,
> + &act, NULL);
> if (rc) {
> - fprintf(stderr, "Error %d opening ffs !\n", rc);
> - return;
> + fprintf(stderr, "Partition with ID %d doesn't exist error: %d\n",
> + part_id, rc);
> + goto out;
> }
>
> - if (name) {
> - uint32_t i;
> -
> - for (i = 0;;i++) {
> - rc = ffs_part_info(ffs_handle, i, &ent_name, &start, &size, &act,
> - NULL);
> - if (rc == FFS_ERR_PART_NOT_FOUND) {
> - fprintf(stderr, "Partition with name %s doesn't exist within TOC at 0x%08x\n", name, toc_offset);
> - goto out;
> - }
> - if (rc || strncmp(ent_name, name, FFS_PART_NAME_MAX) == 0) {
> - part_id = i;
> - break;
> - }
> - free(ent_name);
> - ent_name = NULL;
> - }
> - } else {
> - rc = ffs_part_info(ffs_handle, part_id, &ent_name, &start, &size, &act,
> - NULL);
> - if (rc == FFS_ERR_PART_NOT_FOUND) {
> - fprintf(stderr, "Partition with ID %d doesn't exist within TOC at 0x%08x\n",
> - part_id, toc_offset);
> - goto out;
> - }
> - }
> - ent = ffs_entry_get(ffs_handle, part_id);
> - if (rc || !ent) {
> - fprintf(stderr, "Error %d scanning partitions\n", !ent ?
> - FFS_ERR_PART_NOT_FOUND : rc);
> + ent = ffs_entry_get(ffsh, part_id);
> + if (!ent) {
> + rc = FFS_ERR_PART_NOT_FOUND;
> + fprintf(stderr, "Couldn't open partition entry\n");
> goto out;
> }
>
> @@ -510,7 +527,6 @@ static void print_partition_detail(struct flash_details *flash,
> free(flags);
>
> out:
> - ffs_close(ffs_handle);
> free(ent_name);
> }
>
> @@ -608,8 +624,10 @@ static void print_help(const char *pname)
>
> int main(int argc, char *argv[])
> {
> - struct flash_details flash = { 0 };
> const char *pname = argv[0];
> + struct flash_details flash = { 0 };
> + static struct ffs_handle *ffsh = NULL;
> + uint32_t ffs_index;
> uint32_t address = 0, read_size = 0, write_size = 0, detail_id = UINT_MAX;
> bool erase = false, do_clear = false;
> bool program = false, erase_all = false, info = false, do_read = false;
> @@ -618,6 +636,7 @@ int main(int argc, char *argv[])
> bool no_action = false, tune = false;
> char *write_file = NULL, *read_file = NULL, *part_name = NULL;
> bool ffs_toc_seen = false, direct = false, print_detail = false;
> + int flash_side = 0;
> int rc = 0;
>
> while(1) {
> @@ -732,7 +751,7 @@ int main(int argc, char *argv[])
> break;
> case 'T':
> ffs_toc_seen = true;
> - ffs_toc = strtoul(optarg, &endptr, 0);
> + flash.toc = strtoul(optarg, &endptr, 0);
> if (*endptr != '\0') {
> rc = 1;
> no_action = true;
> @@ -919,7 +938,7 @@ int main(int argc, char *argv[])
> if (rc) {
> fprintf(stderr, "Error %d getting flash info\n", rc);
> rc = 1;
> - goto out;
> + goto close;
> }
>
> /* If file specified but not size, get size from file */
> @@ -929,7 +948,7 @@ int main(int argc, char *argv[])
> if (stat(write_file, &stbuf)) {
> perror("Failed to get file size");
> rc = 1;
> - goto out;
> + goto close;
> }
> write_size = stbuf.st_size;
> }
> @@ -938,27 +957,38 @@ int main(int argc, char *argv[])
> if (do_read && !read_size && !part_name)
> read_size = flash.total_size;
>
> - /* We have a partition specified, grab the details */
> - if (part_name)
> - lookup_partition(&flash, part_name);
> -
> /* We have a partition, adjust read/write size if needed */
> - if (ffsh && ffs_index >= 0) {
> + if (part_name || print_detail) {
> uint32_t pstart, pmaxsz, pactsize;
> - bool ecc;
> - int rc;
> + bool ecc, confirm;
> +
> + if (ffs_toc_seen)
> + ffsh = lookup_partition_at_toc(&flash,
> + part_name, &ffs_index);
> + else
> + ffsh = lookup_partition_at_side(&flash, flash_side,
> + part_name, &ffs_index);
> + if (!ffsh)
> + goto close;
> +
> + if (!part_name)
> + ffs_index = detail_id;
>
> rc = ffs_part_info(ffsh, ffs_index, NULL,
> &pstart, &pmaxsz, &pactsize, &ecc);
> if (rc) {
> fprintf(stderr,"Failed to get partition info\n");
> - goto out;
> + goto close;
> }
>
> if (!ecc && do_clear) {
> fprintf(stderr, "The partition on which to do --clear "
> "does not have ECC, are you sure?\n");
> - check_confirm();
> + confirm = check_confirm();
> + if (!confirm) {
> + rc = 1;
> + goto close;
> + }
> /* Still confirm later on */
> must_confirm = true;
> }
> @@ -981,7 +1011,7 @@ int main(int argc, char *argv[])
> printf("ERROR: Size (%d bytes) larger than partition"
> " (%d bytes). Use --force to force\n",
> write_size, pmaxsz);
> - goto out;
> + goto close;
> }
>
> /* Set address */
> @@ -992,7 +1022,7 @@ int main(int argc, char *argv[])
> printf("ERROR: Erase at 0x%08x for 0x%08x isn't erase block aligned\n",
> address, write_size);
> printf("Use --force to force\n");
> - goto out;
> + goto close;
> } else {
> printf("WARNING: Erase at 0x%08x for 0x%08x isn't erase block aligned\n",
> address, write_size);
> @@ -1011,11 +1041,11 @@ int main(int argc, char *argv[])
> * because of --size, but still respect if it came from --toc (we
> * assume the user knows what they're doing in that case)
> */
> - print_flash_info(&flash, flash_side ? 0 : ffs_toc);
> + print_flash_info(&flash);
> }
>
> if (print_detail)
> - print_partition_detail(&flash, flash_side ? 0 : ffs_toc, detail_id, part_name);
> + print_partition_detail(ffsh, ffs_index);
>
> /* Unlock flash (PNOR only) */
> if ((erase || program || do_clear) && !bmc_flash && !flashfilename) {
> @@ -1023,24 +1053,29 @@ int main(int argc, char *argv[])
> if (flash.need_relock == -1) {
> fprintf(stderr, "Architecture doesn't support write protection on flash\n");
> flash.need_relock = 0;
> - goto out;
> + goto close;
> }
> }
> - if (do_read)
> - do_read_file(flash.bl, read_file, address, read_size);
> - if (erase_all)
> - erase_chip(flash.bl);
> - else if (erase)
> - erase_range(flash.bl, address, write_size, program);
> - if (program)
> - program_file(flash.bl, write_file, address, write_size);
> - if (do_clear)
> - set_ecc(flash.bl, address, write_size);
> rc = 0;
> -
> + if (do_read)
> + rc = do_read_file(flash.bl, read_file, address, read_size);
> + if (!rc && erase_all)
> + rc = erase_chip(flash.bl);
> + else if (!rc && erase)
> + rc = erase_range(flash.bl, address, write_size,
> + program, ffsh, ffs_index);
> + if (!rc && program)
> + rc = program_file(flash.bl, write_file, address, write_size,
> + ffsh, ffs_index);
> + if (!rc && do_clear)
> + rc = set_ecc(flash.bl, address, write_size);
> +
> +close:
> if (flash.need_relock)
> arch_flash_set_wrprotect(flash.bl, 1);
> arch_flash_close(flash.bl, flashfilename);
> + if (ffsh)
> + ffs_close(ffsh);
> out:
> free(part_name);
> free(read_file);
More information about the Skiboot
mailing list