This patchset can be fetched from github: https://github.com/adam900710/btrfs-progs/tree/warning_fixes Which is based on v4.19 tag. This patchset will make "make W=1" reports no warning. This patch will first introduce fix to Makefile.extrawarn to make "cc-disable-warning" works, then disable sign-compare warning completely, as we really don't want extra "unsigned" prefix to slow our typing. Then re-use (ok, in fact rework) Yanjun's patch to disable formwat-truncation warning. Finally, fix all the remaining warnings reported by make W=1. Now, we make "make W=1" great (may 'again' or not, depending on the distribution rolling speed). changelog: v1.1: - Use cc-disable-warning instead of putting -Wno-something to improve compatibility. - Better explaination on the BUG_ON() branch caused uninitialized variable. - Also cleanup free-space-tree.c v2: - Add reviewed-by tags, except the 7th patch, as it goes a different way to fix in v2. - Fix bad port of cc-disable-warning, using $CFLAGS instead of kernel flags. - Make sure fixed warnings still show in W=3. - Use do {} while() loop to replace a while() {} loop, so even compiler doesn't have enough hint for BUG_ON(), it won't report uninitialized variable warning. Qu Wenruo (12): btrfs-progs: Makefile.extrawarn: Import cc-disable-warning btrfs-progs: Makefile.extrawarn: Don't warn on sign compare btrfs-progs: Fix Wempty-body warning btrfs-progs: Fix Wimplicit-fallthrough warning btrfs-progs: Fix Wsuggest-attribute=format warning btrfs-progs: Fix Wmaybe-uninitialized warning btrfs-progs: Fix Wtype-limits warning btrfs-progs: Fix missing-prototypes warning caused by non-static functions btrfs-progs: Move btrfs_check_nodesize to fsfeatures.c to fix missing-prototypes warning btrfs-progs: Introduce rescue.h to resolve missing-prototypes for chunk and super rescue btrfs-progs: Add utils.h include to solve missing-prototypes warning btrfs-progs: free-space-tree: Remove unsued function Su Yanjun (1): btrfs-progs: fix gcc8 default build warning caused by '-Wformat-truncation' Makefile | 5 ++++ Makefile.extrawarn | 10 ++++++++ btrfs.c | 2 +- check/mode-lowmem.c | 6 ++--- chunk-recover.c | 1 + cmds-rescue.c | 4 +-- ctree.c | 4 +-- extent-tree.c | 2 +- free-space-tree.c | 59 ++++++++++++--------------------------------- fsfeatures.c | 23 ++++++++++++++++++ messages.h | 15 ++++++++---- rescue.h | 21 ++++++++++++++++ send-stream.c | 3 +-- string-table.c | 1 + super-recover.c | 1 + utils-lib.c | 1 + utils.c | 53 +++++++++++++++++----------------------- 17 files changed, 119 insertions(+), 92 deletions(-) create mode 100644 rescue.h -- 2.19.2
We imported cc-option but forgot to import cc-disable-warning. Fixes: b556a992c3ad ("btrfs-progs: build: allow to build with various compiler warnings") Signed-off-by: Qu Wenruo <wqu@suse.com> --- Makefile.extrawarn | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Makefile.extrawarn b/Makefile.extrawarn index 1f4bda94a167..18a3a860053e 100644 --- a/Makefile.extrawarn +++ b/Makefile.extrawarn @@ -19,6 +19,12 @@ try-run = $(shell set -e; \ cc-option = $(call try-run,\ $(CC) $(CFLAGS) $(1) -c -x c /dev/null -o "$$TMP",$(1),$(2)) +# cc-disable-warning +# Usage: cflags-y += $(call cc-disable-warning,unused-but-set-variable) +cc-disable-warning = $(call try-run,\ + $(CC) -Werror $(CFLAGS) -W$(strip $(1)) -c -x c /dev/null -o "$$TMP",-Wno-$(strip $(1))) + + # From linux.git/scripts/Makefile.extrawarn # ========================================================================== # -- 2.19.2
From: Su Yanjun <suyj.fnst@cn.fujitsu.com> When using gcc8 + glibc 2.28.5 compiles utils.c, it complains as below: utils.c:852:45: warning: '%s' directive output may be truncated writing up to 4095 bytes into a region of size 4084 [-Wformat-truncation=] snprintf(path, sizeof(path), "/dev/mapper/%s", name); ^~ ~~~~ In file included from /usr/include/stdio.h:873, from utils.c:20: /usr/include/bits/stdio2.h:67:10: note: '__builtin___snprintf_chk' output between 13 and 4108 bytes into a destination of size 4096 return __builtin___snprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ __bos (__s), __fmt, __va_arg_pack ()); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ This isn't a type of warning we care about, particularly when calling snprintf() we expect string to be truncated. Using the GCC option -Wno-format-truncation to disable this for default build and W=1 build, while still keep it for W=2/W=3 build. Signed-off-by: Su Yanjun <suyj.fnst@cn.fujitsu.com> [Use cc-disable-warning to fix the not working CFLAGS setting in configure.ac] [Keep the warning in W=2/W=3 build] Signed-off-by: Qu Wenruo <wqu@suse.com> --- Makefile | 5 +++++ Makefile.extrawarn | 2 ++ 2 files changed, 7 insertions(+) diff --git a/Makefile b/Makefile index f4ab14ea74c8..a9e57fecb6e6 100644 --- a/Makefile +++ b/Makefile @@ -62,6 +62,10 @@ DEBUG_LDFLAGS := ABSTOPDIR = $(shell pwd) TOPDIR := . +# Disable certain GCC 8 + glibc 2.28 warning for snprintf() +# where string truncation for snprintf() is expected. +DISABLE_WARNING_FLAGS := $(call cc-disable-warning, format-truncation) + # Common build flags CFLAGS = $(SUBST_CFLAGS) \ $(CSTD) \ @@ -73,6 +77,7 @@ CFLAGS = $(SUBST_CFLAGS) \ -I$(TOPDIR) \ -I$(TOPDIR)/kernel-lib \ -I$(TOPDIR)/libbtrfsutil \ + $(DISABLE_WARNING_FLAGS) \ $(EXTRAWARN_CFLAGS) \ $(DEBUG_CFLAGS_INTERNAL) \ $(EXTRA_CFLAGS) diff --git a/Makefile.extrawarn b/Makefile.extrawarn index 18a3a860053e..0c11f2450802 100644 --- a/Makefile.extrawarn +++ b/Makefile.extrawarn @@ -53,6 +53,7 @@ warning-1 += -Wold-style-definition warning-1 += $(call cc-option, -Wmissing-include-dirs) warning-1 += $(call cc-option, -Wunused-but-set-variable) warning-1 += $(call cc-disable-warning, missing-field-initializers) +warning-1 += $(call cc-disable-warning, format-truncation) warning-2 := -Waggregate-return warning-2 += -Wcast-align @@ -61,6 +62,7 @@ warning-2 += -Wnested-externs warning-2 += -Wshadow warning-2 += $(call cc-option, -Wlogical-op) warning-2 += $(call cc-option, -Wmissing-field-initializers) +warning-2 += $(call cc-option, -Wformat-truncation) warning-3 := -Wbad-function-cast warning-3 += -Wcast-qual -- 2.19.2
Under most case, we are just using 'int' for 'unsigned int', and doesn't care about the sign. The Wsign-compare is causing tons of false alerts. Suppressing it would make W=1 less noisy so we can focus on real problem, while still allow it in W=3 build. Signed-off-by: Qu Wenruo <wqu@suse.com> --- Makefile.extrawarn | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile.extrawarn b/Makefile.extrawarn index 0c11f2450802..9b4cace01ce4 100644 --- a/Makefile.extrawarn +++ b/Makefile.extrawarn @@ -54,6 +54,7 @@ warning-1 += $(call cc-option, -Wmissing-include-dirs) warning-1 += $(call cc-option, -Wunused-but-set-variable) warning-1 += $(call cc-disable-warning, missing-field-initializers) warning-1 += $(call cc-disable-warning, format-truncation) +warning-1 += $(call cc-disable-warning, sign-compare) warning-2 := -Waggregate-return warning-2 += -Wcast-align @@ -74,6 +75,7 @@ warning-3 += -Wredundant-decls warning-3 += -Wswitch-default warning-3 += $(call cc-option, -Wpacked-bitfield-compat) warning-3 += $(call cc-option, -Wvla) +warning-3 += $(call cc-option, -Wsign-compare) warning := $(warning-$(findstring 1, $(BUILD_ENABLE_EXTRA_GCC_CHECKS))) warning += $(warning-$(findstring 2, $(BUILD_ENABLE_EXTRA_GCC_CHECKS))) -- 2.19.2
messages.h:49:24: warning: suggest braces around empty body in an 'if' statement [-Wempty-body] PRINT_TRACE_ON_ERROR; \ Just extra braces would solve the problem. Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> --- messages.h | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/messages.h b/messages.h index ec7d93381a36..16f650d19a4b 100644 --- a/messages.h +++ b/messages.h @@ -45,13 +45,16 @@ #define error_on(cond, fmt, ...) \ do { \ - if ((cond)) \ + if ((cond)) { \ PRINT_TRACE_ON_ERROR; \ - if ((cond)) \ + } \ + if ((cond)) { \ PRINT_VERBOSE_ERROR; \ + } \ __btrfs_error_on((cond), (fmt), ##__VA_ARGS__); \ - if ((cond)) \ + if ((cond)) { \ DO_ABORT_ON_ERROR; \ + } \ } while (0) #define error_btrfs_util(err) \ @@ -76,10 +79,12 @@ #define warning_on(cond, fmt, ...) \ do { \ - if ((cond)) \ + if ((cond)) { \ PRINT_TRACE_ON_ERROR; \ - if ((cond)) \ + } \ + if ((cond)) { \ PRINT_VERBOSE_ERROR; \ + } \ __btrfs_warning_on((cond), (fmt), ##__VA_ARGS__); \ } while (0) -- 2.19.2
Although most fallthrough case is pretty obvious, we still need to teach the dumb compiler that it's an explicit fallthrough. Also reformat the code to use common indent. Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> --- utils.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/utils.c b/utils.c index a310300829eb..b274f46fdd9d 100644 --- a/utils.c +++ b/utils.c @@ -1134,15 +1134,25 @@ int pretty_size_snprintf(u64 size, char *str, size_t str_size, unsigned unit_mod num_divs = 0; last_size = size; switch (unit_mode & UNITS_MODE_MASK) { - case UNITS_TBYTES: base *= mult; num_divs++; - case UNITS_GBYTES: base *= mult; num_divs++; - case UNITS_MBYTES: base *= mult; num_divs++; - case UNITS_KBYTES: num_divs++; - break; + case UNITS_TBYTES: + base *= mult; + num_divs++; + __attribute__ ((fallthrough)); + case UNITS_GBYTES: + base *= mult; + num_divs++; + __attribute__ ((fallthrough)); + case UNITS_MBYTES: + base *= mult; + num_divs++; + __attribute__ ((fallthrough)); + case UNITS_KBYTES: + num_divs++; + break; case UNITS_BYTES: - base = 1; - num_divs = 0; - break; + base = 1; + num_divs = 0; + break; default: if (negative) { s64 ssize = (s64)size; @@ -1907,13 +1917,17 @@ int test_num_disk_vs_raid(u64 metadata_profile, u64 data_profile, default: case 4: allowed |= BTRFS_BLOCK_GROUP_RAID10; + __attribute__ ((fallthrough)); case 3: allowed |= BTRFS_BLOCK_GROUP_RAID6; + __attribute__ ((fallthrough)); case 2: allowed |= BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1 | BTRFS_BLOCK_GROUP_RAID5; + __attribute__ ((fallthrough)); case 1: allowed |= BTRFS_BLOCK_GROUP_DUP; + __attribute__ ((fallthrough)); } if (dev_cnt > 1 && profile & BTRFS_BLOCK_GROUP_DUP) { -- 2.19.2
Add __attribute__ ((format (printf, 4, 0))) to fix the vprintf calling function. Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> --- string-table.c | 1 + 1 file changed, 1 insertion(+) diff --git a/string-table.c b/string-table.c index 95833768960d..455285702d51 100644 --- a/string-table.c +++ b/string-table.c @@ -48,6 +48,7 @@ struct string_table *table_create(int columns, int rows) * '>' the text is right aligned. If fmt is equal to '=' the text will * be replaced by a '=====' dimensioned on the basis of the column width */ +__attribute__ ((format (printf, 4, 0))) char *table_vprintf(struct string_table *tab, int column, int row, const char *fmt, va_list ap) { -- 2.19.2
GCC 8.2.1 will report the following warning with "make W=1": ctree.c: In function 'btrfs_next_sibling_tree_block': ctree.c:2990:21: warning: 'slot' may be used uninitialized in this function [-Wmaybe-uninitialized] path->slots[level] = slot; ~~~~~~~~~~~~~~~~~~~^~~~~~ The culprit is the following code: int slot; << Not initialized int level = path->lowest_level + 1; BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); while(level < BTRFS_MAX_LEVEL) { slot = path->slots[level] + 1; ^^^^^^ but we initialize @slot here. ... } path->slots[level] = slot; It's possible that compiler doesn't get enough hint for BUG_ON() on lowest_level + 1 >= BTRFS_MAX_LEVEL case. Fix it by using a do {} while() loop other than while() {} loop, to ensure we will run the loop for at least once. Signed-off-by: Qu Wenruo <wqu@suse.com> --- ctree.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ctree.c b/ctree.c index 46e2ccedc0bf..867e8b60b199 100644 --- a/ctree.c +++ b/ctree.c @@ -2966,7 +2966,7 @@ int btrfs_next_sibling_tree_block(struct btrfs_fs_info *fs_info, struct extent_buffer *next = NULL; BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); - while(level < BTRFS_MAX_LEVEL) { + do { if (!path->nodes[level]) return 1; @@ -2986,7 +2986,7 @@ int btrfs_next_sibling_tree_block(struct btrfs_fs_info *fs_info, if (!extent_buffer_uptodate(next)) return -EIO; break; - } + } while (level < BTRFS_MAX_LEVEL); path->slots[level] = slot; while(1) { level--; -- 2.19.2
The only hit is the following code: tlv_len = le16_to_cpu(tlv_hdr->tlv_len); if (tlv_type == 0 || tlv_type > BTRFS_SEND_A_MAX || tlv_len > BTRFS_SEND_BUF_SIZE) { error("invalid tlv in cmd tlv_type = %hu, tlv_len = %hu", tlv_type, tlv_len); @tlv_len is u16, while BTRFS_SEND_BUF_SIZE is 64K. u16 MAX is 64K - 1, so the final check is always false. Just remove it. Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> --- send-stream.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/send-stream.c b/send-stream.c index 3b8e39c9486a..25461e92c37b 100644 --- a/send-stream.c +++ b/send-stream.c @@ -157,8 +157,7 @@ static int read_cmd(struct btrfs_send_stream *sctx) tlv_type = le16_to_cpu(tlv_hdr->tlv_type); tlv_len = le16_to_cpu(tlv_hdr->tlv_len); - if (tlv_type == 0 || tlv_type > BTRFS_SEND_A_MAX - || tlv_len > BTRFS_SEND_BUF_SIZE) { + if (tlv_type == 0 || tlv_type > BTRFS_SEND_A_MAX) { error("invalid tlv in cmd tlv_type = %hu, tlv_len = %hu", tlv_type, tlv_len); ret = -EINVAL; -- 2.19.2
Make the following functions static to avoid missing-prototypes warning: - btrfs.c::handle_special_globals() - check/mode-lowmem.c::repair_ternary_lowmem() - extent-tree.c::btrfs_search_overlap_extent() - free-space-tree.c::convert_free_space_to_bitmaps() - free-space-tree.c::convert_free_space_to_extents() - free-space-tree.c::__remove_from_free_space_tree() - free-space-tree.c::__add_to_free_space_tree() - free-space-tree.c::btrfs_create_tree() Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> --- btrfs.c | 2 +- check/mode-lowmem.c | 6 +++--- extent-tree.c | 2 +- free-space-tree.c | 30 +++++++++++++++--------------- 4 files changed, 20 insertions(+), 20 deletions(-) diff --git a/btrfs.c b/btrfs.c index 2d39f2ced3e8..78c468d2e050 100644 --- a/btrfs.c +++ b/btrfs.c @@ -210,7 +210,7 @@ static int handle_global_options(int argc, char **argv) return shift; } -void handle_special_globals(int shift, int argc, char **argv) +static void handle_special_globals(int shift, int argc, char **argv) { int has_help = 0; int has_full = 0; diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c index 14bbc9ee6cb6..f56b5e8d45dc 100644 --- a/check/mode-lowmem.c +++ b/check/mode-lowmem.c @@ -953,9 +953,9 @@ out: * returns 0 means success. * returns not 0 means on error; */ -int repair_ternary_lowmem(struct btrfs_root *root, u64 dir_ino, u64 ino, - u64 index, char *name, int name_len, u8 filetype, - int err) +static int repair_ternary_lowmem(struct btrfs_root *root, u64 dir_ino, u64 ino, + u64 index, char *name, int name_len, + u8 filetype, int err) { struct btrfs_trans_handle *trans; int stage = 0; diff --git a/extent-tree.c b/extent-tree.c index cd98633992ac..8c9cdeff3b02 100644 --- a/extent-tree.c +++ b/extent-tree.c @@ -3749,7 +3749,7 @@ static void __get_extent_size(struct btrfs_root *root, struct btrfs_path *path, * Return >0 for not found. * Return <0 for err */ -int btrfs_search_overlap_extent(struct btrfs_root *root, +static int btrfs_search_overlap_extent(struct btrfs_root *root, struct btrfs_path *path, u64 bytenr, u64 len) { struct btrfs_key key; diff --git a/free-space-tree.c b/free-space-tree.c index 6641cdfa42ba..b3ffa90f704c 100644 --- a/free-space-tree.c +++ b/free-space-tree.c @@ -202,9 +202,9 @@ static void le_bitmap_set(unsigned long *map, unsigned int start, int len) } } -int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans, - struct btrfs_block_group_cache *block_group, - struct btrfs_path *path) +static int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans, + struct btrfs_block_group_cache *block_group, + struct btrfs_path *path) { struct btrfs_fs_info *fs_info = trans->fs_info; struct btrfs_root *root = fs_info->free_space_root; @@ -341,9 +341,9 @@ out: return ret; } -int convert_free_space_to_extents(struct btrfs_trans_handle *trans, - struct btrfs_block_group_cache *block_group, - struct btrfs_path *path) +static int convert_free_space_to_extents(struct btrfs_trans_handle *trans, + struct btrfs_block_group_cache *block_group, + struct btrfs_path *path) { struct btrfs_fs_info *fs_info = trans->fs_info; struct btrfs_root *root = fs_info->free_space_root; @@ -780,9 +780,9 @@ out: return ret; } -int __remove_from_free_space_tree(struct btrfs_trans_handle *trans, - struct btrfs_block_group_cache *block_group, - struct btrfs_path *path, u64 start, u64 size) +static int __remove_from_free_space_tree(struct btrfs_trans_handle *trans, + struct btrfs_block_group_cache *block_group, + struct btrfs_path *path, u64 start, u64 size) { struct btrfs_free_space_info *info; u32 flags; @@ -960,9 +960,9 @@ out: return ret; } -int __add_to_free_space_tree(struct btrfs_trans_handle *trans, - struct btrfs_block_group_cache *block_group, - struct btrfs_path *path, u64 start, u64 size) +static int __add_to_free_space_tree(struct btrfs_trans_handle *trans, + struct btrfs_block_group_cache *block_group, + struct btrfs_path *path, u64 start, u64 size) { struct btrfs_fs_info *fs_info = trans->fs_info; struct btrfs_free_space_info *info; @@ -1420,9 +1420,9 @@ out: return ret; } -struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans, - struct btrfs_fs_info *fs_info, - u64 objectid) +static struct btrfs_root *btrfs_create_tree(struct btrfs_trans_handle *trans, + struct btrfs_fs_info *fs_info, + u64 objectid) { struct extent_buffer *leaf; struct btrfs_root *tree_root = fs_info->tree_root; -- 2.19.2
And fsfeatures.c is indeed a better location for that function. Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> --- fsfeatures.c | 23 +++++++++++++++++++++++ utils.c | 23 ----------------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/fsfeatures.c b/fsfeatures.c index 7d85d60f1277..13ad030870cd 100644 --- a/fsfeatures.c +++ b/fsfeatures.c @@ -225,3 +225,26 @@ u32 get_running_kernel_version(void) return version; } +int btrfs_check_nodesize(u32 nodesize, u32 sectorsize, u64 features) +{ + if (nodesize < sectorsize) { + error("illegal nodesize %u (smaller than %u)", + nodesize, sectorsize); + return -1; + } else if (nodesize > BTRFS_MAX_METADATA_BLOCKSIZE) { + error("illegal nodesize %u (larger than %u)", + nodesize, BTRFS_MAX_METADATA_BLOCKSIZE); + return -1; + } else if (nodesize & (sectorsize - 1)) { + error("illegal nodesize %u (not aligned to %u)", + nodesize, sectorsize); + return -1; + } else if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS && + nodesize != sectorsize) { + error( + "illegal nodesize %u (not equal to %u for mixed block group)", + nodesize, sectorsize); + return -1; + } + return 0; +} diff --git a/utils.c b/utils.c index b274f46fdd9d..a7e34b804551 100644 --- a/utils.c +++ b/utils.c @@ -2266,29 +2266,6 @@ int btrfs_tree_search2_ioctl_supported(int fd) return ret; } -int btrfs_check_nodesize(u32 nodesize, u32 sectorsize, u64 features) -{ - if (nodesize < sectorsize) { - error("illegal nodesize %u (smaller than %u)", - nodesize, sectorsize); - return -1; - } else if (nodesize > BTRFS_MAX_METADATA_BLOCKSIZE) { - error("illegal nodesize %u (larger than %u)", - nodesize, BTRFS_MAX_METADATA_BLOCKSIZE); - return -1; - } else if (nodesize & (sectorsize - 1)) { - error("illegal nodesize %u (not aligned to %u)", - nodesize, sectorsize); - return -1; - } else if (features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS && - nodesize != sectorsize) { - error("illegal nodesize %u (not equal to %u for mixed block group)", - nodesize, sectorsize); - return -1; - } - return 0; -} - /* * Copy a path argument from SRC to DEST and check the SRC length if it's at * most PATH_MAX and fits into DEST. DESTLEN is supposed to be exact size of -- 2.19.2
We don't have any header declaring btrfs_recover_chunk_tree() nor btrfs_recover_superblocks(), thus W=1 gives missing-prototypes warning on them. Fix it by introducing a new header, rescue.h for these two functions, so make W=1 could be much happier. Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> --- chunk-recover.c | 1 + cmds-rescue.c | 4 +--- rescue.h | 21 +++++++++++++++++++++ super-recover.c | 1 + 4 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 rescue.h diff --git a/chunk-recover.c b/chunk-recover.c index 1d30db51d8ed..1e554b8e8750 100644 --- a/chunk-recover.c +++ b/chunk-recover.c @@ -40,6 +40,7 @@ #include "utils.h" #include "btrfsck.h" #include "commands.h" +#include "rescue.h" struct recover_control { int verbose; diff --git a/cmds-rescue.c b/cmds-rescue.c index 2bc50c0841ed..36e9e1277e40 100644 --- a/cmds-rescue.c +++ b/cmds-rescue.c @@ -26,15 +26,13 @@ #include "commands.h" #include "utils.h" #include "help.h" +#include "rescue.h" static const char * const rescue_cmd_group_usage[] = { "btrfs rescue <command> [options] <path>", NULL }; -int btrfs_recover_chunk_tree(const char *path, int verbose, int yes); -int btrfs_recover_superblocks(const char *path, int verbose, int yes); - static const char * const cmd_rescue_chunk_recover_usage[] = { "btrfs rescue chunk-recover [options] <device>", "Recover the chunk tree by scanning the devices one by one.", diff --git a/rescue.h b/rescue.h new file mode 100644 index 000000000000..de486e2e2004 --- /dev/null +++ b/rescue.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2018 SUSE. All rights reserved. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public + * License v2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ + +#ifndef __BTRFS_RESCUE_H__ +#define __BTRFS_RESCUE_H__ + +int btrfs_recover_superblocks(const char *path, int verbose, int yes); +int btrfs_recover_chunk_tree(const char *path, int verbose, int yes); + +#endif diff --git a/super-recover.c b/super-recover.c index 86b3df9867dc..a1af71786034 100644 --- a/super-recover.c +++ b/super-recover.c @@ -34,6 +34,7 @@ #include "crc32c.h" #include "volumes.h" #include "commands.h" +#include "rescue.h" struct btrfs_recover_superblock { struct btrfs_fs_devices *fs_devices; -- 2.19.2
Prototypes for arg_strtou64() and lookup_path_rootid() are included in utils.c, resulting make W=1 warning for them. Just include that header to make W=1 happier. Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> --- utils-lib.c | 1 + 1 file changed, 1 insertion(+) diff --git a/utils-lib.c b/utils-lib.c index 044f93fc4446..5bb89f2f1a8d 100644 --- a/utils-lib.c +++ b/utils-lib.c @@ -1,4 +1,5 @@ #include "kerncompat.h" +#include "utils.h" #include <unistd.h> #include <stdlib.h> #include <limits.h> -- 2.19.2
set_free_space_tree_thresholds() is never used, just remove it to solve the missing-prototypes warning from make W=1. Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com> --- free-space-tree.c | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/free-space-tree.c b/free-space-tree.c index b3ffa90f704c..af141e6e611a 100644 --- a/free-space-tree.c +++ b/free-space-tree.c @@ -24,35 +24,6 @@ #include "bitops.h" #include "internal.h" -void set_free_space_tree_thresholds(struct btrfs_block_group_cache *cache, - u64 sectorsize) -{ - u32 bitmap_range; - size_t bitmap_size; - u64 num_bitmaps, total_bitmap_size; - - /* - * We convert to bitmaps when the disk space required for using extents - * exceeds that required for using bitmaps. - */ - bitmap_range = sectorsize * BTRFS_FREE_SPACE_BITMAP_BITS; - num_bitmaps = div_u64(cache->key.offset + bitmap_range - 1, - bitmap_range); - bitmap_size = sizeof(struct btrfs_item) + BTRFS_FREE_SPACE_BITMAP_SIZE; - total_bitmap_size = num_bitmaps * bitmap_size; - cache->bitmap_high_thresh = div_u64(total_bitmap_size, - sizeof(struct btrfs_item)); - - /* - * We allow for a small buffer between the high threshold and low - * threshold to avoid thrashing back and forth between the two formats. - */ - if (cache->bitmap_high_thresh > 100) - cache->bitmap_low_thresh = cache->bitmap_high_thresh - 100; - else - cache->bitmap_low_thresh = 0; -} - static struct btrfs_free_space_info * search_free_space_info(struct btrfs_trans_handle *trans, struct btrfs_fs_info *fs_info, -- 2.19.2
On Wed, Dec 05, 2018 at 02:40:12PM +0800, Qu Wenruo wrote:
> GCC 8.2.1 will report the following warning with "make W=1":
>
> ctree.c: In function 'btrfs_next_sibling_tree_block':
> ctree.c:2990:21: warning: 'slot' may be used uninitialized in this function [-Wmaybe-uninitialized]
> path->slots[level] = slot;
> ~~~~~~~~~~~~~~~~~~~^~~~~~
>
> The culprit is the following code:
>
> int slot; << Not initialized
> int level = path->lowest_level + 1;
> BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL);
> while(level < BTRFS_MAX_LEVEL) {
> slot = path->slots[level] + 1;
> ^^^^^^ but we initialize @slot here.
> ...
> }
> path->slots[level] = slot;
>
> It's possible that compiler doesn't get enough hint for BUG_ON() on
> lowest_level + 1 >= BTRFS_MAX_LEVEL case.
>
> Fix it by using a do {} while() loop other than while() {} loop, to
> ensure we will run the loop for at least once.
I was hoping that we can actually add the hint to BUG_ON so the code
does not continue if the condition is true.
[-- Attachment #1.1: Type: text/plain, Size: 1713 bytes --] On 2018/12/5 下午9:40, David Sterba wrote: > On Wed, Dec 05, 2018 at 02:40:12PM +0800, Qu Wenruo wrote: >> GCC 8.2.1 will report the following warning with "make W=1": >> >> ctree.c: In function 'btrfs_next_sibling_tree_block': >> ctree.c:2990:21: warning: 'slot' may be used uninitialized in this function [-Wmaybe-uninitialized] >> path->slots[level] = slot; >> ~~~~~~~~~~~~~~~~~~~^~~~~~ >> >> The culprit is the following code: >> >> int slot; << Not initialized >> int level = path->lowest_level + 1; >> BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL); >> while(level < BTRFS_MAX_LEVEL) { >> slot = path->slots[level] + 1; >> ^^^^^^ but we initialize @slot here. >> ... >> } >> path->slots[level] = slot; >> >> It's possible that compiler doesn't get enough hint for BUG_ON() on >> lowest_level + 1 >= BTRFS_MAX_LEVEL case. >> >> Fix it by using a do {} while() loop other than while() {} loop, to >> ensure we will run the loop for at least once. > > I was hoping that we can actually add the hint to BUG_ON so the code > does not continue if the condition is true. > I checked that method, but I'm not that confident about things like: bugon_trace() { if (!val) return; __bugon_trace(); } __attribute__ ((noreturn)) static inline void __bugon_trace(); This is as simple as just one extra function call, but the original problem is just one more function call before hitting abort(). So I just give up screwing up things I'm not comfort enough to tweaking. The current do {} while() loop is the most direct solution, if gcc one day still gives such warning then I could say some harsh word then. Thanks, Qu [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --]
On Wed, Dec 05, 2018 at 02:40:09PM +0800, Qu Wenruo wrote:
> messages.h:49:24: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
> PRINT_TRACE_ON_ERROR; \
>
> Just extra braces would solve the problem.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> messages.h | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/messages.h b/messages.h
> index ec7d93381a36..16f650d19a4b 100644
> --- a/messages.h
> +++ b/messages.h
> @@ -45,13 +45,16 @@
>
> #define error_on(cond, fmt, ...) \
> do { \
> - if ((cond)) \
> + if ((cond)) { \
> PRINT_TRACE_ON_ERROR; \
I think the definition of PRINT_TRACE_ON_ERROR should be fixed to always
emit a statement, a "do { } while (0)" in the other case. Otherwise it's
"don't forget to enclose in { } if used", the proposed change looks
lightly safer to me. I'll update the patch, no need to resend, please
let me know if it's ok for you.
[-- Attachment #1.1: Type: text/plain, Size: 1268 bytes --] On 2018/12/14 上午2:59, David Sterba wrote: > On Wed, Dec 05, 2018 at 02:40:09PM +0800, Qu Wenruo wrote: >> messages.h:49:24: warning: suggest braces around empty body in an 'if' statement [-Wempty-body] >> PRINT_TRACE_ON_ERROR; \ >> >> Just extra braces would solve the problem. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> Reviewed-by: Nikolay Borisov <nborisov@suse.com> >> --- >> messages.h | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/messages.h b/messages.h >> index ec7d93381a36..16f650d19a4b 100644 >> --- a/messages.h >> +++ b/messages.h >> @@ -45,13 +45,16 @@ >> >> #define error_on(cond, fmt, ...) \ >> do { \ >> - if ((cond)) \ >> + if ((cond)) { \ >> PRINT_TRACE_ON_ERROR; \ > > I think the definition of PRINT_TRACE_ON_ERROR should be fixed to always > emit a statement, a "do { } while (0)" in the other case. Looks much better than my braces solution. > Otherwise it's > "don't forget to enclose in { } if used", the proposed change looks > lightly safer to me. I'll update the patch, no need to resend, please > let me know if it's ok for you. > I'm completely OK with such better solution. Thanks, Qu [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --]
On Wed, Dec 05, 2018 at 10:03:30PM +0800, Qu Wenruo wrote:
>
>
> On 2018/12/5 下午9:40, David Sterba wrote:
> > On Wed, Dec 05, 2018 at 02:40:12PM +0800, Qu Wenruo wrote:
> >> GCC 8.2.1 will report the following warning with "make W=1":
> >>
> >> ctree.c: In function 'btrfs_next_sibling_tree_block':
> >> ctree.c:2990:21: warning: 'slot' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >> path->slots[level] = slot;
> >> ~~~~~~~~~~~~~~~~~~~^~~~~~
> >>
> >> The culprit is the following code:
> >>
> >> int slot; << Not initialized
> >> int level = path->lowest_level + 1;
> >> BUG_ON(path->lowest_level + 1 >= BTRFS_MAX_LEVEL);
> >> while(level < BTRFS_MAX_LEVEL) {
> >> slot = path->slots[level] + 1;
> >> ^^^^^^ but we initialize @slot here.
> >> ...
> >> }
> >> path->slots[level] = slot;
> >>
> >> It's possible that compiler doesn't get enough hint for BUG_ON() on
> >> lowest_level + 1 >= BTRFS_MAX_LEVEL case.
> >>
> >> Fix it by using a do {} while() loop other than while() {} loop, to
> >> ensure we will run the loop for at least once.
> >
> > I was hoping that we can actually add the hint to BUG_ON so the code
> > does not continue if the condition is true.
> >
> I checked that method, but I'm not that confident about things like:
>
> bugon_trace()
> {
> if (!val)
> return;
> __bugon_trace();
> }
>
> __attribute__ ((noreturn))
> static inline void __bugon_trace();
>
> This is as simple as just one extra function call, but the original
> problem is just one more function call before hitting abort().
>
> So I just give up screwing up things I'm not comfort enough to tweaking.
>
> The current do {} while() loop is the most direct solution, if gcc one
> day still gives such warning then I could say some harsh word then.
I was not able to make it work properly, patch applied so we rid of the
warning. Thanks.