[PATCH] delete obsolete function btrfs_print_tree()

[PATCH] delete obsolete function btrfs_print_tree()

[Holger Hoffstätte]

[-- Attachment #1.1: Type: text/plain, Size: 2224 bytes --]

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] delete obsolete function btrfs_print_tree()

[David Sterba]

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.

Re: [PATCH] delete obsolete function btrfs_print_tree()

[Holger Hoffstätte]

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

Re: [PATCH] delete obsolete function btrfs_print_tree()

[Filipe Manana]

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



-- 
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] delete obsolete function btrfs_print_tree()

[Holger Hoffstätte]

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

Re: [PATCH] delete obsolete function btrfs_print_tree()

[Dan Carpenter]

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