| Message ID | 56F550DD.1030200@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 25, 2016 at 03:53:17PM +0100, Holger Hoffstätte wrote: > Dan Carpenter's static checker recently found missing IS_ERR handling > in print-tree.c:btrfs_print_tree(). While looking into this I found that > this function is no longer called anywhere and was moved to btrfs-progs > long ago. It can simply be removed. I'm not sure, the function could be used for debugging, and it's hard to say if we'll ever need it. Printing the whole tree to the system log would produce a lot of text so some manual filtering would be required, the function could serve as a template. The function is not that big that it would save bytes, but putting it under the debug config would help a bit. -- 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 04/04/16 15:56, David Sterba wrote: > On Fri, Mar 25, 2016 at 03:53:17PM +0100, Holger Hoffstätte wrote: >> Dan Carpenter's static checker recently found missing IS_ERR handling >> in print-tree.c:btrfs_print_tree(). While looking into this I found that >> this function is no longer called anywhere and was moved to btrfs-progs >> long ago. It can simply be removed. > > I'm not sure, the function could be used for debugging, and it's hard to ..but is it? So far nobody has complained. > say if we'll ever need it. Printing the whole tree to the system log > would produce a lot of text so some manual filtering would be required, > the function could serve as a template. The original problem of missing error handling from btrfs_read_tree_block() remains as well. I don't remember if that also was true for the btrfs-progs counterpart, but in in any case I didn't really know what to do there. Print an error? silently ignore the stripe? abort? When I realized that the function was not called anywhere, deleting it seemed more effective. Under what circumstances would the in-kernel function be more practical or useful than the userland tool? It does the same, won't disturb or wedge the kernel further, is up-to-date and can be scripted. I agree that in-place filtering (while iterating) would be nice to have, but that's also a whole different problem and would IMHO also be better suited for userland. When in doubt cut it out. Holger -- 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 Mon, Apr 4, 2016 at 4:54 PM, Holger Hoffstätte <holger.hoffstaette@googlemail.com> wrote: > On 04/04/16 15:56, David Sterba wrote: >> On Fri, Mar 25, 2016 at 03:53:17PM +0100, Holger Hoffstätte wrote: >>> Dan Carpenter's static checker recently found missing IS_ERR handling >>> in print-tree.c:btrfs_print_tree(). While looking into this I found that >>> this function is no longer called anywhere and was moved to btrfs-progs >>> long ago. It can simply be removed. >> >> I'm not sure, the function could be used for debugging, and it's hard to > > ..but is it? So far nobody has complained. I will complain. I use this function frequently during development, and there's a good reason to use it instead of the user space tool btrfs-debug-tree. > >> say if we'll ever need it. Printing the whole tree to the system log >> would produce a lot of text so some manual filtering would be required, >> the function could serve as a template. > > The original problem of missing error handling from btrfs_read_tree_block() > remains as well. I don't remember if that also was true for the btrfs-progs > counterpart, but in in any case I didn't really know what to do there. > Print an error? silently ignore the stripe? abort? When I realized that the > function was not called anywhere, deleting it seemed more effective. > > Under what circumstances would the in-kernel function be more > practical or useful than the userland tool? The user land tool requires the btree nodes to be on disk. With the in kernel function we can print nodes that are not yet on disk, very useful during development. So no, we should not delete it in my opinion. It's not serious if it doesn't have all the proper error handling and etc, it's just something for debugging purposes. It does the same, won't disturb > or wedge the kernel further, is up-to-date and can be scripted. > I agree that in-place filtering (while iterating) would be nice to have, > but that's also a whole different problem and would IMHO also be better > suited for userland. > > When in doubt cut it out. When in doubt leave it alone. > > Holger > > -- > 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 04/04/16 18:02, Filipe Manana wrote: > I use this function frequently during development, and there's a good > reason to use it instead of the user space tool btrfs-debug-tree. Good to know, that's why I asked. Printing unwritten extents makes sense. -h -- 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 Mon, Apr 04, 2016 at 05:02:38PM +0100, Filipe Manana wrote: > It's not serious if it doesn't have all the proper error handling > and etc, it's just something for debugging purposes. I'm slowly trying to remove static checker warnings so that we can detect real bugs. People sometimes leave little messages for me in their code because they know I will review the new warning: foo = kmalloc(); /* error handling deliberately left out */ It make me quite annoyed because it's like "Oh. No if we added error handling that would take 40 extra bytes of memory! Such a waste!" But we could use instead use___GFP_NOFAIL instead. Or BUG_ON(!foo)". I have gotten distracted. What was the question again? regards, dan carpenter -- 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/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c index 147dc6c..dc28db8 100644 --- a/fs/btrfs/print-tree.c +++ b/fs/btrfs/print-tree.c @@ -328,41 +328,3 @@ void btrfs_print_leaf(struct btrfs_root *root, struct extent_buffer *l) }; } } - -void btrfs_print_tree(struct btrfs_root *root, struct extent_buffer *c) -{ - int i; u32 nr; - struct btrfs_key key; - int level; - - if (!c) - return; - nr = btrfs_header_nritems(c); - level = btrfs_header_level(c); - if (level == 0) { - btrfs_print_leaf(root, c); - return; - } - btrfs_info(root->fs_info, "node %llu level %d total ptrs %d free spc %u", - btrfs_header_bytenr(c), level, nr, - (u32)BTRFS_NODEPTRS_PER_BLOCK(root) - nr); - for (i = 0; i < nr; i++) { - btrfs_node_key_to_cpu(c, &key, i); - printk(KERN_INFO "\tkey %d (%llu %u %llu) block %llu\n", - i, key.objectid, key.type, key.offset, - btrfs_node_blockptr(c, i)); - } - for (i = 0; i < nr; i++) { - struct extent_buffer *next = read_tree_block(root, - btrfs_node_blockptr(c, i), - btrfs_node_ptr_generation(c, i)); - if (btrfs_is_leaf(next) && - level != 1) - BUG(); - if (btrfs_header_level(next) != - level - 1) - BUG(); - btrfs_print_tree(root, next); - free_extent_buffer(next); - } -} diff --git a/fs/btrfs/print-tree.h b/fs/btrfs/print-tree.h index 7faddfa..9dd56b9 100644 --- a/fs/btrfs/print-tree.h +++ b/fs/btrfs/print-tree.h @@ -19,5 +19,4 @@ #ifndef __PRINT_TREE_ #define __PRINT_TREE_ void btrfs_print_leaf(struct btrfs_root *root, struct extent_buffer *l); -void btrfs_print_tree(struct btrfs_root *root, struct extent_buffer *c); #endif
Dan Carpenter's static checker recently found missing IS_ERR handling in print-tree.c:btrfs_print_tree(). While looking into this I found that this function is no longer called anywhere and was moved to btrfs-progs long ago. It can simply be removed. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Holger Hoffstätte <holger.hoffstaette@googlemail.com> --- fs/btrfs/print-tree.c | 38 -------------------------------------- fs/btrfs/print-tree.h | 1 - 2 files changed, 39 deletions(-)