[PATCH v2 00/13] btrfs-progs: Make W=1 great (no "again")

[PATCH v2 00/13] btrfs-progs: Make W=1 great (no "again")

[Qu Wenruo]

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

[PATCH v2 01/13] btrfs-progs: Makefile.extrawarn: Import cc-disable-warning

[Qu Wenruo]

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

[PATCH v2 02/13] btrfs-progs: fix gcc8 default build warning caused by '-Wformat-truncation'

[Qu Wenruo]

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

[PATCH v2 03/13] btrfs-progs: Makefile.extrawarn: Don't warn on sign compare

[Qu Wenruo]

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

[PATCH v2 04/13] btrfs-progs: Fix Wempty-body warning

[Qu Wenruo]

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

Re: [PATCH v2 04/13] btrfs-progs: Fix Wempty-body warning

[David Sterba]

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.

Re: [PATCH v2 04/13] btrfs-progs: Fix Wempty-body warning

[Qu Wenruo]

[-- 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 --]

[PATCH v2 05/13] btrfs-progs: Fix Wimplicit-fallthrough warning

[Qu Wenruo]

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

[PATCH v2 06/13] btrfs-progs: Fix Wsuggest-attribute=format warning

[Qu Wenruo]

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

[PATCH v2 07/13] btrfs-progs: Fix Wmaybe-uninitialized warning

[Qu Wenruo]

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

Re: [PATCH v2 07/13] btrfs-progs: Fix Wmaybe-uninitialized warning

[David Sterba]

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.

Re: [PATCH v2 07/13] btrfs-progs: Fix Wmaybe-uninitialized warning

[Qu Wenruo]

[-- 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 --]

Re: [PATCH v2 07/13] btrfs-progs: Fix Wmaybe-uninitialized warning

[David Sterba]

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.

[PATCH v2 08/13] btrfs-progs: Fix Wtype-limits warning

[Qu Wenruo]

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

[PATCH v2 09/13] btrfs-progs: Fix missing-prototypes warning caused by non-static functions

[Qu Wenruo]

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

[PATCH v2 10/13] btrfs-progs: Move btrfs_check_nodesize to fsfeatures.c to fix missing-prototypes warning

[Qu Wenruo]

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

[PATCH v2 11/13] btrfs-progs: Introduce rescue.h to resolve missing-prototypes for chunk and super rescue

[Qu Wenruo]

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

[PATCH v2 12/13] btrfs-progs: Add utils.h include to solve missing-prototypes warning

[Qu Wenruo]

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

[PATCH v2 13/13] btrfs-progs: free-space-tree: Remove unsued function

[Qu Wenruo]

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