| Message ID | 1373395794-29140-2-git-send-email-fdmanana@gmail.com (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Headers | show |
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 -- 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
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." -- 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
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 -- 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
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 > -- 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
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);
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(-)