Remove duplicated and useless code in cmds-restore

PATCH In review Filipe David Borba Manana 3 participants 7 messages Jul 9, 2013 – Aug 3, 2013 Btrfs file system

Share & tools

Filipe David Borba Manana

author

2013-07-09 18:50 UTC #0 lore raw

cover Remove duplicated and useless code in cmds-restore

The following patch series are just a cleanup for cmds-restore.c, removing some duplicated code and code that never gets executed.

Filipe David Borba Manana (2):
Btrfs-progs: remove duplicated code in cmds-restore.c
Btrfs-progs: remove unneeded leaf checks in cmds-restore

 cmds-restore.c |  170 ++++++++++++--------------------------------------------
 1 file changed, 37 insertions(+), 133 deletions(-)

Signature · 1 line

1.7.9.5

Filipe David Borba Manana

author

2013-07-09 18:50 UTC #1 lore raw

PATCH 1/2 Btrfs-progs: remove duplicated code in cmds-restore.c

The module cmds-restore.c was defining its own next_leaf() function, which did exactly the same as btrfs_next_leaf() from ctree.c.

Signed-off-by Filipe David Borba Manana <[email protected]>

---
 cmds-restore.c |   62 +++++---------------------------------------------------
 1 file changed, 5 insertions(+), 57 deletions(-)

cmds-restore.c +5 ~~-57~~

index eca528d..ed4815a 100644
@@ -148,58 +148,6 @@ static int decompress(char *inbuf, char *outbuf, u64 compress_len,
 	return -1;
 }
 
-int next_leaf(struct btrfs_root *root, struct btrfs_path *path)
-{
-	int slot;
-	int level = 1;
-	struct extent_buffer *c;
-	struct extent_buffer *next = NULL;
-
-	for (; level < BTRFS_MAX_LEVEL; level++) {
-		if (path->nodes[level])
-			break;
-	}
-
-	if (level == BTRFS_MAX_LEVEL)
-		return 1;
-
-	slot = path->slots[level] + 1;
-
-	while(level < BTRFS_MAX_LEVEL) {
-		if (!path->nodes[level])
-			return 1;
-
-		slot = path->slots[level] + 1;
-		c = path->nodes[level];
-		if (slot >= btrfs_header_nritems(c)) {
-			level++;
-			if (level == BTRFS_MAX_LEVEL)
-				return 1;
-			continue;
-		}
-
-		if (path->reada)
-			reada_for_search(root, path, level, slot, 0);
-
-		next = read_node_slot(root, c, slot);
-		break;
-	}
-	path->slots[level] = slot;
-	while(1) {
-		level--;
-		c = path->nodes[level];
-		free_extent_buffer(c);
-		path->nodes[level] = next;
-		path->slots[level] = 0;
-		if (!level)
-			break;
-		if (path->reada)
-			reada_for_search(root, path, level, 0, 0);
-		next = read_node_slot(root, next, 0);
-	}
-	return 0;
-}
-
 static int copy_one_inline(int fd, struct btrfs_path *path, u64 pos)
 {
 	struct extent_buffer *leaf = path->nodes[0];

@@ -447,7 +395,7 @@ static int copy_file(struct btrfs_root *root, int fd, struct btrfs_key *key,
 
 	leaf = path->nodes[0];
 	while (!leaf) {
-		ret = next_leaf(root, path);
+		ret = btrfs_next_leaf(root, path);
 		if (ret < 0) {
 			fprintf(stderr, "Error getting next leaf %d\n",
 				ret);

@@ -470,7 +418,7 @@ static int copy_file(struct btrfs_root *root, int fd, struct btrfs_key *key,
 		}
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
 			do {
-				ret = next_leaf(root, path);
+				ret = btrfs_next_leaf(root, path);
 				if (ret < 0) {
 					fprintf(stderr, "Error searching %d\n", ret);
 					btrfs_free_path(path);

@@ -569,7 +517,7 @@ static int search_dir(struct btrfs_root *root, struct btrfs_key *key,
 		if (verbose > 1)
 			printf("No leaf after search, looking for the next "
 			       "leaf\n");
-		ret = next_leaf(root, path);
+		ret = btrfs_next_leaf(root, path);
 		if (ret < 0) {
 			fprintf(stderr, "Error getting next leaf %d\n",
 				ret);

@@ -596,7 +544,7 @@ static int search_dir(struct btrfs_root *root, struct btrfs_key *key,
 
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
 			do {
-				ret = next_leaf(root, path);
+				ret = btrfs_next_leaf(root, path);
 				if (ret < 0) {
 					fprintf(stderr, "Error searching %d\n",
 						ret);

@@ -937,7 +885,7 @@ again:
 		goto out;
 	}
 	do {
-		ret = next_leaf(root, path);
+		ret = btrfs_next_leaf(root, path);
 		if (ret < 0) {
 			fprintf(stderr, "Error getting next leaf %d\n",
 				ret);

Signature · 1 line

1.7.9.5

David Sterba

2013-07-10 16:12 UTC #2 lore raw

Re: [PATCH 1/2] Btrfs-progs: remove duplicated code in cmds-restore.c

Quoted 3 lines · Filipe David Borba Manana

The module cmds-restore.c was defining its own next_leaf() function, which did exactly the same as btrfs_next_leaf() from ctree.c.

This has been removed by Eric's patch present in the integration branches:
Btrfs-progs: remove cut & paste btrfs_next_leaf from restore
http://www.spinics.net/lists/linux-btrfs/msg24477.html

but now Chris has a fix in the master branch,
btrfs-restore: deal with NULL returns from read_node_slot
https://git.kernel.org/cgit/linux/kernel/git/mason/btrfs-progs.git/commit/?id=194aa4a1bd6447bb545286d0bcb0b0be8204d79f

the code of updated next_leaf is not identical to btrfs_next_leaf and I think 'restore' could be more tolerant to partially corrupted structures, so both functions could make sense in the end.

david

Filipe David Manana

author

2013-07-10 16:21 UTC #3 lore raw

Re: [PATCH 1/2] Btrfs-progs: remove duplicated code in cmds-restore.c

Quoted 9 lines · David Sterba

Quoted 3 lines · Filipe David Borba Manana
The module cmds-restore.c was defining its own next_leaf() function, which did exactly the same as btrfs_next_leaf() from ctree.c.
This has been removed by Eric's patch present in the integration branches:
Btrfs-progs: remove cut & paste btrfs_next_leaf from restore
http://www.spinics.net/lists/linux-btrfs/msg24477.html

Oh, didn't notice that.

Quoted 8 lines

but now Chris has a fix in the master branch,
btrfs-restore: deal with NULL returns from read_node_slot
https://git.kernel.org/cgit/linux/kernel/git/mason/btrfs-progs.git/commit/?id=194aa4a1bd6447bb545286d0bcb0b0be8204d79f
the code of updated next_leaf is not identical to btrfs_next_leaf and I think 'restore' could be more tolerant to partially corrupted structures, so both functions could make sense in the end.

Ok, I understand now why both exist.

So please just ignore this patch and the following one
(https://patchwork.kernel.org/patch/2825425/).

thanks

Quoted 2 lines

david

--
Filipe David Manana,

"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men."

Eric Sandeen

2013-08-03 00:34 UTC #4 lore raw

Re: [PATCH 1/2] Btrfs-progs: remove duplicated code in cmds-restore.c

In reply to David Sterba

Quoted 5 lines · David Sterba

Quoted 3 lines · Filipe David Borba Manana
The module cmds-restore.c was defining its own next_leaf() function, which did exactly the same as btrfs_next_leaf() from ctree.c.

Quoted 8 lines

This has been removed by Eric's patch present in the integration branches:
Btrfs-progs: remove cut & paste btrfs_next_leaf from restore
http://www.spinics.net/lists/linux-btrfs/msg24477.html
but now Chris has a fix in the master branch,
btrfs-restore: deal with NULL returns from read_node_slot
https://git.kernel.org/cgit/linux/kernel/git/mason/btrfs-progs.git/commit/?id=194aa4a1bd6447bb545286d0bcb0b0be8204d79f

Just noticed this. :(

Is there some reason that kernelspace should not also get Chris' fix, though?

Quoted 3 lines

the code of updated next_leaf is not identical to btrfs_next_leaf and I think 'restore' could be more tolerant to partially corrupted structures, so both functions could make sense in the end.

Surely kernelspace should be at least as tolerant as userspace; it it seems like Chris's BUG_ON removal patch could benefit kernelspace too, no?

And then we could take one more baby step towards a cleaner, non- cut-and-pasted codebase.

-Eric

Eric Sandeen

2013-08-03 21:37 UTC #5 lore raw

Re: [PATCH 1/2] Btrfs-progs: remove duplicated code in cmds-restore.c

Quoted 11 lines · Eric Sandeen

Quoted 10 lines · David Sterba
Quoted 3 lines · Filipe David Borba Manana
The module cmds-restore.c was defining its own next_leaf() function, which did exactly the same as btrfs_next_leaf() from ctree.c.
This has been removed by Eric's patch present in the integration branches:
Btrfs-progs: remove cut & paste btrfs_next_leaf from restore
http://www.spinics.net/lists/linux-btrfs/msg24477.html

Quoted 7 lines

Quoted 3 lines
but now Chris has a fix in the master branch,
btrfs-restore: deal with NULL returns from read_node_slot
https://git.kernel.org/cgit/linux/kernel/git/mason/btrfs-progs.git/commit/?id=194aa4a1bd6447bb545286d0bcb0b0be8204d79f
Just noticed this. :(
Is there some reason that kernelspace should not also get Chris' fix, though?

Or for that matter the other copy in ctree.c...

Ok, my email didn't make a ton of sense. :/ But there are basically 3 copies of this function now, diverging further - in btrfs-progs' ctree.c and cmds-restore.c, as well as kernelspace ctree.c Should they differ?

Now that I have some time I guess I'll get back to trying to bring userspace in line w/ kernelspace again...

-Eric

Quoted 17 lines

Quoted 3 lines
the code of updated next_leaf is not identical to btrfs_next_leaf and I think 'restore' could be more tolerant to partially corrupted structures, so both functions could make sense in the end.
Surely kernelspace should be at least as tolerant as userspace; it it seems like Chris's BUG_ON removal patch could benefit kernelspace too, no?
And then we could take one more baby step towards a cleaner, non- cut-and-pasted codebase.
-Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Filipe David Borba Manana

author

2013-07-09 18:50 UTC #6 lore raw

PATCH 2/2 Btrfs-progs: remove unneeded leaf checks in cmds-restore

In reply to Filipe David Borba Manana

If btrfs_search_slot() returns a value >= 0, then we can be sure that path->nodes[i] is not NULL for each i between 0 to tree height - 1. The function btrfs_next_leaf() also ensures any path->nodes[i] is not NULL as long as it returns 0.

Signed-off-by Filipe David Borba Manana <[email protected]>

---
 cmds-restore.c |  118 ++++++++++++++++++--------------------------------------
 1 file changed, 37 insertions(+), 81 deletions(-)

cmds-restore.c +37 ~~-81~~

index ed4815a..baa9cab 100644
@@ -394,21 +394,6 @@ static int copy_file(struct btrfs_root *root, int fd, struct btrfs_key *key,
 	}
 
 	leaf = path->nodes[0];
-	while (!leaf) {
-		ret = btrfs_next_leaf(root, path);
-		if (ret < 0) {
-			fprintf(stderr, "Error getting next leaf %d\n",
-				ret);
-			btrfs_free_path(path);
-			return ret;
-		} else if (ret > 0) {
-			/* No more leaves to search */
-			btrfs_free_path(path);
-			return 0;
-		}
-		leaf = path->nodes[0];
-	}
-
 	while (1) {
 		if (loops++ >= 1024) {
 			ret = ask_to_continue(file);

@@ -417,19 +402,17 @@ static int copy_file(struct btrfs_root *root, int fd, struct btrfs_key *key,
 			loops = 0;
 		}
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
-			do {
-				ret = btrfs_next_leaf(root, path);
-				if (ret < 0) {
-					fprintf(stderr, "Error searching %d\n", ret);
-					btrfs_free_path(path);
-					return ret;
-				} else if (ret) {
-					/* No more leaves to search */
-					btrfs_free_path(path);
-					goto set_size;
-				}
-				leaf = path->nodes[0];
-			} while (!leaf);
+			ret = btrfs_next_leaf(root, path);
+			if (ret < 0) {
+				fprintf(stderr, "Error searching %d\n", ret);
+				btrfs_free_path(path);
+				return ret;
+			} else if (ret) {
+				/* No more leaves to search */
+				btrfs_free_path(path);
+				goto set_size;
+			}
+			leaf = path->nodes[0];
 			continue;
 		}
 		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);

@@ -513,27 +496,6 @@ static int search_dir(struct btrfs_root *root, struct btrfs_key *key,
 	}
 
 	leaf = path->nodes[0];
-	while (!leaf) {
-		if (verbose > 1)
-			printf("No leaf after search, looking for the next "
-			       "leaf\n");
-		ret = btrfs_next_leaf(root, path);
-		if (ret < 0) {
-			fprintf(stderr, "Error getting next leaf %d\n",
-				ret);
-			btrfs_free_path(path);
-			return ret;
-		} else if (ret > 0) {
-			/* No more leaves to search */
-			if (verbose)
-				printf("Reached the end of the tree looking "
-				       "for the directory\n");
-			btrfs_free_path(path);
-			return 0;
-		}
-		leaf = path->nodes[0];
-	}
-
 	while (leaf) {
 		if (loops++ >= 1024) {
 			printf("We have looped trying to restore files in %s "

@@ -543,24 +505,22 @@ static int search_dir(struct btrfs_root *root, struct btrfs_key *key,
 		}
 
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
-			do {
-				ret = btrfs_next_leaf(root, path);
-				if (ret < 0) {
-					fprintf(stderr, "Error searching %d\n",
-						ret);
-					btrfs_free_path(path);
-					return ret;
-				} else if (ret > 0) {
-					/* No more leaves to search */
-					if (verbose)
-						printf("Reached the end of "
-						       "the tree searching the"
-						       " directory\n");
-					btrfs_free_path(path);
-					return 0;
-				}
-				leaf = path->nodes[0];
-			} while (!leaf);
+			ret = btrfs_next_leaf(root, path);
+			if (ret < 0) {
+				fprintf(stderr, "Error searching %d\n",
+					ret);
+				btrfs_free_path(path);
+				return ret;
+			} else if (ret > 0) {
+				/* No more leaves to search */
+				if (verbose)
+					printf("Reached the end of "
+					       "the tree searching the"
+					       " directory\n");
+				btrfs_free_path(path);
+				return 0;
+			}
+			leaf = path->nodes[0];
 			continue;
 		}
 		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);

@@ -884,20 +844,16 @@ again:
 		ret = 0;
 		goto out;
 	}
-	do {
-		ret = btrfs_next_leaf(root, path);
-		if (ret < 0) {
-			fprintf(stderr, "Error getting next leaf %d\n",
-				ret);
-			goto out;
-		} else if (ret > 0) {
-			fprintf(stderr, "No more leaves\n");
-			goto out;
-		}
-	} while (!path->nodes[0]);
-	if (path->nodes[0])
-		goto again;
-	printf("Couldn't find a dir index item\n");
+	ret = btrfs_next_leaf(root, path);
+	if (ret < 0) {
+		fprintf(stderr, "Error getting next leaf %d\n",
+			ret);
+		goto out;
+	} else if (ret > 0) {
+		fprintf(stderr, "No more leaves\n");
+		goto out;
+	}
+	goto again;
 out:
 	btrfs_free_path(path);
 	return ret;

Signature · 1 line

1.7.9.5