[PATCH 0/2] Remove duplicated and useless code in cmds-restore

[PATCH 0/2] Remove duplicated and useless code in cmds-restore

[Filipe David Borba Manana]

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(-)

-- 
1.7.9.5

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

[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.

Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
---
 cmds-restore.c |   62 +++++---------------------------------------------------
 1 file changed, 5 insertions(+), 57 deletions(-)

diff --git a/cmds-restore.c b/cmds-restore.c
index eca528d..ed4815a 100644
--- a/cmds-restore.c
+++ b/cmds-restore.c
@@ -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);
-- 
1.7.9.5

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

[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 <fdmanana@gmail.com>
---
 cmds-restore.c |  118 ++++++++++++++++++--------------------------------------
 1 file changed, 37 insertions(+), 81 deletions(-)

diff --git a/cmds-restore.c b/cmds-restore.c
index ed4815a..baa9cab 100644
--- a/cmds-restore.c
+++ b/cmds-restore.c
@@ -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;
-- 
1.7.9.5

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

[David Sterba]

On Tue, Jul 09, 2013 at 07:49:53PM +0100, Filipe David Borba Manana wrote:
> 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

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

[Filipe David Manana]

On Wed, Jul 10, 2013 at 5:12 PM, David Sterba <dsterba@suse.cz> wrote:
> On Tue, Jul 09, 2013 at 07:49:53PM +0100, Filipe David Borba Manana wrote:
>> 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.

>
> 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

>
> 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."

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

[Eric Sandeen]

On 7/10/13 11:12 AM, David Sterba wrote:
> On Tue, Jul 09, 2013 at 07:49:53PM +0100, Filipe David Borba Manana wrote:
>> 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

Just noticed this.  :(

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

> 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

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

[Eric Sandeen]

On 8/2/13 7:34 PM, Eric Sandeen wrote:
> On 7/10/13 11:12 AM, David Sterba wrote:
>> On Tue, Jul 09, 2013 at 07:49:53PM +0100, Filipe David Borba Manana wrote:
>>> 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
> 
> 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


>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>