Add f3write --keep and f3read --delete parameters #140

brianpow · 2020-07-19T13:16:17Z

No description provided.

AltraMayor · 2020-07-20T14:07:42Z

Before we go through code review, could you describe the use case that this pull request intends to help? It doesn't need to be formal or long, I'm just looking for a brief description of the situation in which someone would want to use the parameter --keep. I understood what the parameter does, but I couldn't come up with a situation that I'd use it.

Thank you.

brianpow · 2020-07-20T14:14:35Z

Hi @brianpow,

Before we go through code review, could you describe the use case that this pull request intends to help? It doesn't need to be formal or long, I'm just looking for a brief description of the situation in which someone would want to use the parameter --keep. I understood what the parameter does, but I couldn't come up with a situation that I'd use it.

Thank you.

Hi. If the flash memory is overheated, your computer is suspended or the test is interrupted accidentally, we can keep the existing files and continue f3write instead of creating everything from beginning.

…detected

brianpow · 2020-07-20T15:26:27Z

now f3read can delete corrupted NUM.h2w. It makes my life easier if corrupted file was found and I want to tested again by recreating only those corrupted file. So I can easily run these command without counting around the start or end number.
f3read -d /dev/XXX ; f3write -k /dev/XXX; f3read /dev/XXX

for f3probe, with --fix parameter, again user doesn't need to mark down the sectors and run f3fix.

Both changes are good for unattended test/fix too

AltraMayor · 2020-07-23T13:48:55Z

Having f3read --delete does add meaning to f3write --keep. I'm going to work with you to review the code and merge it.

Let's start making this pull request coherent. First, create a second pull request and move patch add --fix parameter to run f3fix automatically if counterfeit memory detected. I intend to accept this second pull request as well, but let's get a thing done at a time.

Please add f3read: , f3write: , f3probe: to the title of the patches accordingly. You can find examples in the log of the repository.

AltraMayor

add --keep parameter to keep existing NUM.h2w files

f3write.c

AltraMayor

add --delete parameter to delete corrupted NUM.h2w file

Please add an s to file in the title of this patch.

f3read.c

AltraMayor · 2020-07-28T14:13:10Z

f3read.c

@@ -48,6 +50,7 @@ struct args {
 	long        end_at;
 	long        max_read_rate;
 	int	    show_progress;
+	bool		delete;


Align indentation with other fields.

You've missed this item.

f3read.c

AltraMayor

add --fix parameter to run f3fix automatically if counterfeit memory detected

A shorter title: add --fix parameter to run f3fix when needed

f3probe.c

AltraMayor · 2020-07-28T14:42:14Z

f3probe.c

@@ -373,8 +382,8 @@ static int test_device(struct args *args)
 	uint64_t read_count, read_time_us;
 	uint64_t write_count, write_time_us;
 	uint64_t reset_count, reset_time_us;
+	uint64_t last_good_sector;


Instead of moving the declaration of last_good_sector here, declare char *fix_cmd = NULL;

More details in other comments.

You've missed this item.

To clarify, the declaration of last_good_sector should be moved back to the body of the switch..case.

f3probe.c

AltraMayor

f3write: code cleanup

f3write.c

AltraMayor · 2020-07-29T17:45:16Z

f3write.c

-		printf("Skipping file %s\n", filename);
-		return false;
-	}
+


Style: drop this new blank line.

AltraMayor · 2020-07-29T17:46:56Z

f3write.c

+				free(full_fn);
+				return false;
+			}
+			err(errno, "Unexpectedly found file %s, but option --keep is not in use", full_fn);


Could replace the but with and? My fault, sorry.

AltraMayor

f3read: code cleanup

AltraMayor · 2020-07-29T17:54:02Z

f3read.c

@@ -386,11 +386,13 @@ static void iterate_files(const char *path, const long *files,
 		tot_overwritten += stats.secs_overwritten;
 		tot_size += stats.bytes_read;
 		and_read_all = and_read_all && stats.read_all;
-		if (stats.secs_corrupted && delete) {
+		if (delete && (stats.secs_corrupted || tot_changed || tot_overwritten)) {


Please replace tot_changed with stats.secs_changed and tot_overwritten with stats.secs_overwritten. My fault, again.

Break the line as needed to fit within the 80-column limit.

AltraMayor · 2020-07-29T18:00:25Z

f3probe.c

@@ -485,6 +486,7 @@ static int test_device(struct args *args)
 	case FKTY_CHAIN: {
 		last_good_sector = (real_size_byte >> 9) - 1;
 		assert(block_order >= 9);
+		assert(asprintf(&fix_cmd, "f3fix --last-sec=%" PRIu64 " %s", last_good_sector, final_dev_filename) > 0);


Break line to fit within the 80-column limit.

AltraMayor · 2020-07-29T18:02:45Z

f3probe.c

@@ -485,6 +486,7 @@ static int test_device(struct args *args)
 	case FKTY_CHAIN: {
 		last_good_sector = (real_size_byte >> 9) - 1;
 		assert(block_order >= 9);
+		assert(asprintf(&fix_cmd, "f3fix --last-sec=%" PRIu64 " %s", last_good_sector, final_dev_filename) > 0);
 		printf("Bad news: The device `%s' is a counterfeit of type %s\n\n"
 			"You can \"fix\" this device using the following command:\n"
 			"f3fix --last-sec=%" PRIu64 " %s\n",


Replace the line "f3fix --last-sec=%" PRIu64 " %s\n", with "%s\n",.

Replace the line last_good_sector, final_dev_filename); below with fix_cmd);.

AltraMayor · 2020-07-29T18:04:12Z

f3probe.c

+	if (args->fix && fix_cmd) {
+		system(fix_cmd);
+	}


Style: drop the braces since the body of this if statement is a single line.

AltraMayor · 2020-07-29T18:07:01Z

f3probe.c

@@ -373,8 +382,8 @@ static int test_device(struct args *args)
 	uint64_t read_count, read_time_us;
 	uint64_t write_count, write_time_us;
 	uint64_t reset_count, reset_time_us;
+	uint64_t last_good_sector;


To clarify, the declaration of last_good_sector should be moved back to the body of the switch..case.

AltraMayor

f3probe: code cleanup

AltraMayor

f3write: code cleanup

AltraMayor · 2020-07-29T18:09:59Z

f3write.c

@@ -39,8 +39,8 @@ static struct argp_option options[] = {
 		"Maximum write rate",					0},
 	{"show-progress",	'p',	"NUM",		0,
 		"Show progress if NUM is not zero",			0},
-	{"keep",	'k',	0,		0,
-		"Keep existing NUM.h2w file, otherwise all NUM.h2w files will be removed in each run",			0},
+	{"keep",		'k',	0,		0,


Replace the first 0 after 'k' with NULL.

brianpow · 2020-09-01T14:49:15Z

@AltraMayor
Thank you for your comments and guidance. Sorry that I just started a big project. For those glitches, I guess majorities of them won't break the program and hope someone will fix them. Sorry again for my poor programming habit.

AltraMayor · 2020-09-01T15:45:57Z

I'm going to leave this pull request open, so someone else can pick it up if the need for it comes. Good luck on your new project!

add --keep parameter to keep existing NUM.h2w files

d3da945

brianpow added 2 commits July 20, 2020 23:08

add --fix parameter to run f3fix automatically if counterfeit memory …

cb8f7c8

…detected

add --delete parameter to delete corrupted NUM.h2w file

a4f122e

AltraMayor added the Enhancement label Jul 23, 2020

AltraMayor requested changes Jul 28, 2020

View reviewed changes

f3write.c Outdated Show resolved Hide resolved

f3write.c Outdated Show resolved Hide resolved

f3write.c Outdated Show resolved Hide resolved

f3write.c Outdated Show resolved Hide resolved

f3write.c Outdated Show resolved Hide resolved

f3write.c Outdated Show resolved Hide resolved

AltraMayor requested changes Jul 28, 2020

View reviewed changes

AltraMayor changed the title ~~add --keep parameter to keep existing NUM.h2w files~~ add f3write --keep and f3read --delete parameters Jul 28, 2020

AltraMayor changed the title ~~add f3write --keep and f3read --delete parameters~~ Add f3write --keep and f3read --delete parameters Jul 28, 2020

AltraMayor requested changes Jul 28, 2020

View reviewed changes

brianpow added 4 commits July 29, 2020 22:11

f3write: code cleanup

72cda3d

f3read: code cleanup

52423ec

f3probe: code cleanup

9814842

f3read: add missing semicolon

7af01dd

AltraMayor requested changes Jul 29, 2020

View reviewed changes

AltraMayor reviewed Jul 29, 2020

View reviewed changes

AltraMayor requested changes Jul 29, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add f3write --keep and f3read --delete parameters #140

Add f3write --keep and f3read --delete parameters #140

brianpow commented Jul 19, 2020

AltraMayor commented Jul 20, 2020

brianpow commented Jul 20, 2020

brianpow commented Jul 20, 2020 •

edited

Loading

AltraMayor commented Jul 23, 2020

AltraMayor left a comment

AltraMayor left a comment •

edited

Loading

AltraMayor Jul 28, 2020

AltraMayor Jul 29, 2020

AltraMayor left a comment

AltraMayor Jul 28, 2020 •

edited

Loading

AltraMayor Jul 29, 2020

AltraMayor Jul 29, 2020

AltraMayor left a comment

AltraMayor Jul 29, 2020

AltraMayor Jul 29, 2020

AltraMayor left a comment

AltraMayor Jul 29, 2020

AltraMayor Jul 29, 2020

AltraMayor Jul 29, 2020

AltraMayor Jul 29, 2020

AltraMayor Jul 29, 2020

AltraMayor left a comment

AltraMayor left a comment

AltraMayor Jul 29, 2020

brianpow commented Sep 1, 2020

AltraMayor commented Sep 1, 2020

Add f3write --keep and f3read --delete parameters #140

Are you sure you want to change the base?

Add f3write --keep and f3read --delete parameters #140

Conversation

brianpow commented Jul 19, 2020

AltraMayor commented Jul 20, 2020

brianpow commented Jul 20, 2020

brianpow commented Jul 20, 2020 • edited Loading

AltraMayor commented Jul 23, 2020

AltraMayor left a comment

Choose a reason for hiding this comment

AltraMayor left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AltraMayor left a comment

Choose a reason for hiding this comment

AltraMayor Jul 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AltraMayor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AltraMayor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AltraMayor left a comment

Choose a reason for hiding this comment

AltraMayor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianpow commented Sep 1, 2020

AltraMayor commented Sep 1, 2020

brianpow commented Jul 20, 2020 •

edited

Loading

AltraMayor left a comment •

edited

Loading

AltraMayor Jul 28, 2020 •

edited

Loading