search

kdave / btrfs-progs

Development of userspace BTRFS tools
GNU General Public License v2.0
544 stars 240 forks source link

Static code analysis of version 5.1 [cleanup][reference] #174

Open unquietwiki opened 5 years ago

unquietwiki commented 5 years ago

cppcheck 1.86, installed on Ubuntu 18.04.2 LTS; installed uuid-dev, zlib1g-dev, and Kernel 5.1.4 headers.

cppcheck --enable=all --inconclusive --std=c11 -I=/usr/include -I=/usr/src/linux-headers-5.1.4-050104/include/uapi/linux --suppress=missingIncludeSystem -rp . &> ~/cppcheck-186-c11-all.txt

cppcheck-186-c11-all.txt

unquietwiki commented 5 years ago

Looking over it, there are a few instances of unsigned long long, vs long long typecasts, that might explain some of the issues with larger volumes. https://en.wikipedia.org/wiki/C_data_types says there's a difference as of C99. There are also a lot of variable scope issues flagged; which makes it possible for unwanted values to end up in the wrong place.

kdave commented 5 years ago

Thanks, a lot of them look worth fixing. Unifying the printf formats and variable types would be a good start. Variable scope reduction is more a matter of style, can't say we need to clean i up everwhere or not.

I've run cpphceck here, so maybe got other results. Some of the variable shadowing warnings are bogus (check/main.c:2103] -> [check/main.c:1522]: (style) Local variable ret shadows outer variable), but they're in 2 functions. The "%Lx" format is confusing it ([disk-io.c:815]: (warning) %Lx in format string (no. 1) requires 'unsigned long long' but the argument type is 'unsigned long long'.).

unquietwiki commented 5 years ago

@kdave My latest copy attempt crapped out after 4 days for NFS/networking glitch, so using this as an opportunity to try fixing this...

unquietwiki commented 5 years ago

Making progress.... ran a new analysis after the compiler bonked on my first attempt.

cppcheck --enable=all --force --inconclusive --std=c11 -I /usr/include -I /usr/src/linux-headers-5.1.6-xanmod4/include/uapi/linux --suppress=missingIncludeSystem -rp . &> ~/cppcheck-187-c11-all.txt

References used in first attempt...

cppcheck-187-c11-all.txt

Side note: took over 2hrs to get the re-analysis. It's still thinking past the 100% mark...

Edit: it returned the part it was waiting on....

[backref.c:328] -> [ctree.c:1158] -> [ctree.c:376]: (error) Null pointer dereference: trans
[qgroup-verify.c:457] -> [qgroup-verify.c:450] -> [qgroup-verify.c:110]: (warning) Null pointer dereference: c
[libbtrfsutil/python/module.c:285]: (style) The function 'PyInit_btrfsutil' is never used.
[tests/sha224-256.c:160]: (style) The function 'SHA224FinalBits' is never used.
[tests/sha224-256.c:135]: (style) The function 'SHA224Input' is never used.
[tests/sha224-256.c:110]: (style) The function 'SHA224Reset' is never used.
[tests/sha224-256.c:185]: (style) The function 'SHA224Result' is never used.
[free-space-cache.c:768]: (style) The function 'btrfs_dump_free_space' is never used.
[btrfs-list.c:1580]: (style) The function 'btrfs_get_subvol' is never used.
[btrfs-list.c:1550]: (style) The function 'btrfs_get_toplevel_subvol' is never used.
[btrfs-list.c:916]: (style) The function 'btrfs_list_get_default_subvolume' is never used.
[inode-item.c:230]: (style) The function 'btrfs_lookup_inode_extref' is never used.
[extent-tree.c:3204]: (style) The function 'btrfs_make_block_groups' is never used.
[fsfeatures.c:127]: (style) The function 'btrfs_process_fs_features' is never used.
[utils.c:2244]: (style) The function 'btrfs_tree_search2_ioctl_supported' is never used.
[kernel-lib/crc32c.c:219]: (style) The function 'crc32c_le' is never used.
[extent_io.c:43]: (style) The function 'extent_io_tree_init_cache_max' is never used.
[backref.c:1652]: (style) The function 'free_ipath' is never used.
[backref.c:1629]: (style) The function 'init_ipath' is never used.
[backref.c:972]: (style) The function 'inode_item_info' is never used.
[mkfs/common.c:776]: (style) The function 'is_vol_small' is never used.
[backref.c:1372]: (style) The function 'iterate_inodes_from_logical' is never used.
[backref.c:1593]: (style) The function 'paths_from_inode' is never used.
[qgroup-verify.c:162]: (style) The function 'print_all_refs' is never used.
[qgroup-verify.c:657]: (style) The function 'print_all_tree_blocks' is never used.
[fsfeatures.c:181]: (style) The function 'print_kernel_version' is never used.
[cmds-balance.c:199]: (style) The function 'print_range' is never used.
[kernel-lib/radix-tree.c:688]: (style) The function 'radix_tree_gang_lookup_tag' is never used.
[kernel-lib/radix-tree.c:351]: (style) The function 'radix_tree_lookup' is never used.
[kernel-lib/radix-tree.c:339]: (style) The function 'radix_tree_lookup_slot' is never used.
[kernel-lib/radix-tree.c:479]: (style) The function 'radix_tree_tag_get' is never used.
[kernel-lib/radix-tree.c:372]: (style) The function 'radix_tree_tag_set' is never used.
[kernel-lib/radix-tree.c:823]: (style) The function 'radix_tree_tagged' is never used.
[kernel-lib/raid56.c:285]: (style) The function 'raid56_recov' is never used.
[utils.c:2540]: (style) The function 'rand_int' is never used.
[utils.c:2555]: (style) The function 'rand_u16' is never used.
[kernel-lib/rbtree.c:542]: (style) The function 'rb_first_postorder' is never used.
[kernel-lib/rbtree.c:524]: (style) The function 'rb_next_postorder' is never used.
[kernel-lib/rbtree.c:496]: (style) The function 'rb_replace_node' is never used.
[free-space-tree.c:1076]: (style) The function 'remove_block_group_free_space' is never used.
[task-utils.c:139]: (style) The function 'task_period_stop' is never used.
[backref.c:1261]: (style) The function 'tree_backref_for_extent' is never used.
[kernel-shared/ulist.c:228]: (style) The function 'ulist_del' is never used.
(information) Cppcheck cannot find all the include files (use --check-config for details)
unquietwiki commented 5 years ago

@kdave I finally got it to compile without errors or warnings. I still need to run another cppcheck to verify if I missed anything there. C is not my first language, and the excessive Git activity can be chalked up to syncing with my storage & build system. I tried to notate changes that impacted #59 in separate commits.

unquietwiki commented 5 years ago

cppcheck-187-c11-all-v2.txt

cppcheck found 5 warnings & 6 errors after clearing out the compilation alerts. This is definitely in the territory of "stuff that might break"; at least the current set of fixes appear to be working on my restore run (200GB transferred in the past few hours)...

Can't fix

Can try to fix

References

kdave commented 5 years ago

Can't fix

* [/usr/include/btrfs/kerncompat.h:96]: (warning) %d in format string (no. 2) requires 'int' but the argument type is 'unsigned int'.

This can be fixed, kerncompat.h is part of btrfs-progs, but that it includes the system one means you haven't used the define that picks the git headers and not from system. BTRFS_FLAT_INCLUDES

unquietwiki commented 5 years ago

@kdave Checking in: restore of over 100TB nearing completion. RAM usage seems to have peaked at ~11GB; not sure if that's from it keeping track of all it has restored (if it's restoring serially, then I can't see it using that much RAM), or an actual memory leak.

kdave commented 5 years ago

The memory usage scales with size of metadata, for a 100TB filesystem I'd say 11G is not that bad.

unquietwiki commented 5 years ago

@kdave That makes sense. btw, the recovery finished a few days ago, and the memory did de-allocate after the run.

unquietwiki commented 3 years ago

@kdave hey, I was looking this issue up for someone asking me about it, and realized it was still open. Should we close this, since you said you had integrated some code fixes a while back? Hope all is well.