Add f3write --keep and f3read --delete parameters by brianpow · Pull Request #140 · AltraMayor/f3

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

 	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

 	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

 		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

 	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

+		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

 	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

 		"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

Conversation

brianpow commented Jul 19, 2020

Uh oh!

AltraMayor commented Jul 20, 2020

Uh oh!

brianpow commented Jul 20, 2020

Uh oh!

brianpow commented Jul 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AltraMayor commented Jul 23, 2020

Uh oh!

AltraMayor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AltraMayor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AltraMayor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AltraMayor Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AltraMayor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AltraMayor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AltraMayor left a comment

Choose a reason for hiding this comment

Uh oh!

AltraMayor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianpow commented Sep 1, 2020

brianpow commented Jul 20, 2020 •

edited

Loading

AltraMayor left a comment •

edited

Loading

AltraMayor Jul 28, 2020 •

edited

Loading