Re: [PATCH] Btrfs-progs: check, fix return value check of is_child_root()

Re: [PATCH] Btrfs-progs: check, fix return value check of is_child_root()

[David Sterba]

On Wed, Oct 15, 2014 at 10:58:40PM +0100, Filipe Manana wrote:
> This affects only the 3.17 release candidates (3.16 and older releases
> don't have this issue).

Thank you, I'm adding this to the branch and tagging rc4.

[PATCH] Btrfs-progs: check, fix return value check of is_child_root()

[Filipe Manana]

The following commit:

    "btrfs-progs: fsck: remove unfriendly BUG_ON() for searching tree failure"
    f495a2ac66116f0a1b15e73380c8cbca6e0a4ca0

introduced a regression, detected through xfstests/btrfs/054, where
previously a negative return value (-1) was used to mean a particular
root didn't had any parent root, and now, after that change, a negative
value is also used to mean that an error happened. That change also made
the only caller of is_child_root() interpret any negative return value
as an error and therefore incorrectly made the caller leave with an
error, instead of jumping to its "skip" label.

Since the return value that means "the root with id child_root_id doesn't
have any parent root" isn't used by the only caller of is_child_root(),
just get rid of it and make is_child_root() return 0 if parent_root_id
isn't a parent of child_root_id, return 1 if it is, and a negative value
on error.

This affects only the 3.17 release candidates (3.16 and older releases
don't have this issue).

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 cmds-check.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 99d1a94..002d3e9 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -901,7 +901,6 @@ static int is_child_root(struct btrfs_root *root, u64 parent_root_id,
 	struct btrfs_path path;
 	struct btrfs_key key;
 	struct extent_buffer *leaf;
-	int has_parent = 0;
 	int ret;
 
 	btrfs_init_path(&path);
@@ -939,8 +938,6 @@ static int is_child_root(struct btrfs_root *root, u64 parent_root_id,
 		    key.type != BTRFS_ROOT_BACKREF_KEY)
 			break;
 
-		has_parent = 1;
-
 		if (key.offset == parent_root_id) {
 			btrfs_release_path(&path);
 			return 1;
@@ -952,7 +949,7 @@ out:
 	btrfs_release_path(&path);
 	if (ret < 0)
 		return ret;
-	return has_parent? 0 : -1;
+	return 0;
 }
 
 static int process_dir_item(struct btrfs_root *root,
-- 
1.9.1

[PATCH v2] Btrfs-progs: check, fix return value check of is_child_root()

[Filipe Manana]

The following commit:

    "btrfs-progs: fsck: remove unfriendly BUG_ON() for searching tree failure"
    f495a2ac66116f0a1b15e73380c8cbca6e0a4ca0

introduced a regression, detected through xfstests/btrfs/054, where
previously a negative return value (-1) was used to mean a particular
root didn't had any parent root, and now, after that change, a negative
value is also used to mean that an error happened. That change also made
the only caller of is_child_root() interpret any negative return value
as an error and therefore incorrectly made the caller leave with an
error, instead of continuing.

This affects only the 3.17 release candidates (3.16 and older releases
don't have this issue).

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---

V2: Made it return 2 (instead of -1) when the root child_root_id doesn't
    have any parent roots, in order to behave exactly like the code
    pre-commit f495a2ac66116f0a1b15e73380c8cbca6e0a4ca0.

 cmds-check.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/cmds-check.c b/cmds-check.c
index 99d1a94..310eb2a 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -895,6 +895,14 @@ static int leave_shared_node(struct btrfs_root *root,
 	return 0;
 }
 
+/*
+ * Returns:
+ * < 0 - on error
+ * 1   - if the root with id child_root_id is a child of root parent_root_id
+ * 0   - if the root child_root_id isn't a child of the root parent_root_id but
+ *       has other root(s) as parent(s)
+ * 2   - if the root child_root_id doesn't have any parent roots
+ */
 static int is_child_root(struct btrfs_root *root, u64 parent_root_id,
 			 u64 child_root_id)
 {
@@ -952,7 +960,7 @@ out:
 	btrfs_release_path(&path);
 	if (ret < 0)
 		return ret;
-	return has_parent? 0 : -1;
+	return has_parent ? 0 : 2;
 }
 
 static int process_dir_item(struct btrfs_root *root,
-- 
1.9.1

Re: [PATCH v2] Btrfs-progs: check, fix return value check of is_child_root()

[Wang Shilong]

> The following commit:
> 
>    "btrfs-progs: fsck: remove unfriendly BUG_ON() for searching tree failure"
>    f495a2ac66116f0a1b15e73380c8cbca6e0a4ca0
> 
> introduced a regression, detected through xfstests/btrfs/054, where
> previously a negative return value (-1) was used to mean a particular
> root didn't had any parent root, and now, after that change, a negative
> value is also used to mean that an error happened. That change also made
> the only caller of is_child_root() interpret any negative return value
> as an error and therefore incorrectly made the caller leave with an
> error, instead of continuing.
> 
> This affects only the 3.17 release candidates (3.16 and older releases
> don't have this issue).
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Thanks for Fixing this, Filipe!

Reviewed-by: Wang Shilong <wangshilong1991@gmail.com>

> ---
> 
> V2: Made it return 2 (instead of -1) when the root child_root_id doesn't
>    have any parent roots, in order to behave exactly like the code
>    pre-commit f495a2ac66116f0a1b15e73380c8cbca6e0a4ca0.
> 
> cmds-check.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/cmds-check.c b/cmds-check.c
> index 99d1a94..310eb2a 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -895,6 +895,14 @@ static int leave_shared_node(struct btrfs_root *root,
> 	return 0;
> }
> 
> +/*
> + * Returns:
> + * < 0 - on error
> + * 1   - if the root with id child_root_id is a child of root parent_root_id
> + * 0   - if the root child_root_id isn't a child of the root parent_root_id but
> + *       has other root(s) as parent(s)
> + * 2   - if the root child_root_id doesn't have any parent roots
> + */
> static int is_child_root(struct btrfs_root *root, u64 parent_root_id,
> 			 u64 child_root_id)
> {
> @@ -952,7 +960,7 @@ out:
> 	btrfs_release_path(&path);
> 	if (ret < 0)
> 		return ret;
> -	return has_parent? 0 : -1;
> +	return has_parent ? 0 : 2;
> }
> 
> static int process_dir_item(struct btrfs_root *root,
> -- 
> 1.9.1
> 
> --
> 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

Best Regards,
Wang Shilong