Skip to contentSkip to conversation

Btrfs: add missing error check to find_parent_nodes

PATCH Superseded Filipe David Borba Manana 3 participants 5 messages Jul 30, 2013 – Jul 31, 2013 Btrfs file system

lore
Share & tools
lore

Filipe David Borba Manana

author
#0 lore raw

PATCH 1/1 Btrfs: add missing error check to find_parent_nodes

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 fs/btrfs/backref.c |    2 ++
 1 file changed, 2 insertions(+)
fs/btrfs/backref.c +2 -0
index 8bc5e8c..1ba87c5 100644
@@ -935,6 +935,8 @@ again:
 				}
 				ret = find_extent_in_eb(eb, bytenr,
 							*extent_item_pos, &eie);
+				if (ret < 0)
+					goto out;
 				ref->inode_list = eie;
 				free_extent_buffer(eb);
 			}
Signature · 1 line
1.7.9.5
#1 lore raw

fs/btrfs/backref.c again: · line 942

Quoted 13 lines
fs/btrfs/backref.c +2 -0
index 8bc5e8c..1ba87c5 100644
@@ -935,6 +935,8 @@ again:
 				}
 				ret = find_extent_in_eb(eb, bytenr,
 							*extent_item_pos, &eie);
+				if (ret < 0)
+					goto out;
 				ref->inode_list = eie;
 				free_extent_buffer(eb);
 			}

The eb needs to be freed before the goto.

And that there's now *three* eb freeing paths in this block should be a hint that it should be refactored into a function that frees the eb before returning.

- z

author
#2 lore raw
Quoted 16 lines · Zach Brown

fs/btrfs/backref.c again: · line 942

Quoted 13 lines
fs/btrfs/backref.c +2 -0
index 8bc5e8c..1ba87c5 100644
@@ -935,6 +935,8 @@ again:
                              }
                              ret = find_extent_in_eb(eb, bytenr,
                                                      *extent_item_pos, &eie);
+                             if (ret < 0)
+                                     goto out;
                              ref->inode_list = eie;
                              free_extent_buffer(eb);
                      }

The eb needs to be freed before the goto.

Quoted 3 lines

And that there's now *three* eb freeing paths in this block should be a hint that it should be refactored into a function that frees the eb before returning.

Thanks, missed that previously.

Quoted 2 lines

- z

Signature · 5 lines
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."
author
#3 lore raw

PATCH v2 1/1 Btrfs: add missing error check to find_parent_nodes

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

---

V2: Ensure extent buffer is freed on error.

 fs/btrfs/backref.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/btrfs/backref.c +3 -1
index 8bc5e8c..980e85a 100644
@@ -935,8 +935,10 @@ again:
 				}
 				ret = find_extent_in_eb(eb, bytenr,
 							*extent_item_pos, &eie);
-				ref->inode_list = eie;
 				free_extent_buffer(eb);
+				if (ret < 0)
+					goto out;
+				ref->inode_list = eie;
 			}
 			ret = ulist_add_merge(refs, ref->parent,
 					      (uintptr_t)ref->inode_list,
Signature · 1 line
1.7.9.5
Reviewed-by on v2 1/1
#4 lore raw
Quoted 15 lines · Filipe David Borba Manana
Signed-off-by Filipe David Borba Manana <[email protected]>

---

V2: Ensure extent buffer is freed on error.

 fs/btrfs/backref.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/btrfs/backref.c +0 -0
index 8bc5e8c..980e85a 100644
@@ -935,8 +935,10 @@ again:
 				}

fs/btrfs/backref.c again: · line 944

Quoted 10 lines
fs/btrfs/backref.c +3 -1
 				ret = find_extent_in_eb(eb, bytenr,
 							*extent_item_pos, &eie);
-				ref->inode_list = eie;
 				free_extent_buffer(eb);
+				if (ret < 0)
+					goto out;
+				ref->inode_list = eie;
 			}
 			ret = ulist_add_merge(refs, ref->parent,
 					      (uintptr_t)ref->inode_list,

The only ret < 0 I'm seeing is ENOMEM, so that should be safe.

Reviewed-by Jan Schmidt <[email protected]>

Thanks,
-Jan