[PATCH] btrfs: Fix a C compliance issue

Subsystems: btrfs file system, filesystems (vfs and infrastructure), the rest

STALE3037d

8 messages, 4 authors, 2018-06-20 · open the first message on its own page

[PATCH] btrfs: Fix a C compliance issue

From: Bart Van Assche <hidden>
Date: 2018-06-15 22:37:04 

The C programming language does not allow to use preprocessor statements
inside macro arguments (pr_info() is defined as a macro). Hence rework
the pr_info() statement in btrfs_print_mod_info() such that it becomes
compliant. This patch allows tools like sparse to analyze the BTRFS
source code.

Fixes: 62e855771dac ("btrfs: convert printk(KERN_* to use pr_* calls")
Signed-off-by: Bart Van Assche <redacted>
Cc: Jeff Mahoney <redacted>
Cc: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 81107ad49f3a..dd4980df5b8e 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2369,7 +2369,7 @@ static __cold void btrfs_interface_exit(void)
 
 static void __init btrfs_print_mod_info(void)
 {
-	pr_info("Btrfs loaded, crc32c=%s"
+	static const char fmt[] = KERN_INFO "Btrfs loaded, crc32c=%s"
 #ifdef CONFIG_BTRFS_DEBUG
 			", debug=on"
 #endif
@@ -2382,8 +2382,8 @@ static void __init btrfs_print_mod_info(void)
 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
 			", ref-verify=on"
 #endif
-			"\n",
-			crc32c_impl());
+			"\n";
+	printk(fmt, crc32c_impl());
 }
 
 static int __init init_btrfs_fs(void)
-- 
2.17.0

Re: [PATCH] btrfs: Fix a C compliance issue

From: Nikolay Borisov <hidden>
Date: 2018-06-16 10:28:16 


On 16.06.2018 01:36, Bart Van Assche wrote:
quoted hunk
The C programming language does not allow to use preprocessor statements
inside macro arguments (pr_info() is defined as a macro). Hence rework
the pr_info() statement in btrfs_print_mod_info() such that it becomes
compliant. This patch allows tools like sparse to analyze the BTRFS
source code.

Fixes: 62e855771dac ("btrfs: convert printk(KERN_* to use pr_* calls")
Signed-off-by: Bart Van Assche <redacted>
Cc: Jeff Mahoney <redacted>
Cc: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 81107ad49f3a..dd4980df5b8e 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2369,7 +2369,7 @@ static __cold void btrfs_interface_exit(void)
 
 static void __init btrfs_print_mod_info(void)
 {
-	pr_info("Btrfs loaded, crc32c=%s"
+	static const char fmt[] = KERN_INFO "Btrfs loaded, crc32c=%s"
 #ifdef CONFIG_BTRFS_DEBUG
 			", debug=on"
 #endif
@@ -2382,8 +2382,8 @@ static void __init btrfs_print_mod_info(void)
 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
 			", ref-verify=on"
 #endif
-			"\n",
-			crc32c_impl());
+			"\n";
+	printk(fmt, crc32c_impl());
I'd rather not see more printk being added. Nothing prevents from having
the fmt string being passed to pr_info.
 }
 
 static int __init init_btrfs_fs(void)

Re: [PATCH] btrfs: Fix a C compliance issue

From: David Sterba <hidden>
Date: 2018-06-18 09:29:24 

On Sat, Jun 16, 2018 at 01:28:13PM +0300, Nikolay Borisov wrote:

On 16.06.2018 01:36, Bart Van Assche wrote:
quoted
The C programming language does not allow to use preprocessor statements
inside macro arguments (pr_info() is defined as a macro). Hence rework
the pr_info() statement in btrfs_print_mod_info() such that it becomes
compliant. This patch allows tools like sparse to analyze the BTRFS
source code.

Fixes: 62e855771dac ("btrfs: convert printk(KERN_* to use pr_* calls")
Signed-off-by: Bart Van Assche <redacted>
Cc: Jeff Mahoney <redacted>
Cc: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 81107ad49f3a..dd4980df5b8e 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2369,7 +2369,7 @@ static __cold void btrfs_interface_exit(void)
 
 static void __init btrfs_print_mod_info(void)
 {
-	pr_info("Btrfs loaded, crc32c=%s"
+	static const char fmt[] = KERN_INFO "Btrfs loaded, crc32c=%s"
 #ifdef CONFIG_BTRFS_DEBUG
 			", debug=on"
 #endif
@@ -2382,8 +2382,8 @@ static void __init btrfs_print_mod_info(void)
 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
 			", ref-verify=on"
 #endif
-			"\n",
-			crc32c_impl());
+			"\n";
+	printk(fmt, crc32c_impl());
I'd rather not see more printk being added. Nothing prevents from having
the fmt string being passed to pr_info.
So you mean to do

+	static const char fmt[] = "Btrfs loaded, crc32c=%s"
+	pr_info(fmt);

?

Re: [PATCH] btrfs: Fix a C compliance issue

From: Nikolay Borisov <hidden>
Date: 2018-06-18 09:31:24 


On 18.06.2018 12:26, David Sterba wrote:
On Sat, Jun 16, 2018 at 01:28:13PM +0300, Nikolay Borisov wrote:
quoted

On 16.06.2018 01:36, Bart Van Assche wrote:
quoted
The C programming language does not allow to use preprocessor statements
inside macro arguments (pr_info() is defined as a macro). Hence rework
the pr_info() statement in btrfs_print_mod_info() such that it becomes
compliant. This patch allows tools like sparse to analyze the BTRFS
source code.

Fixes: 62e855771dac ("btrfs: convert printk(KERN_* to use pr_* calls")
Signed-off-by: Bart Van Assche <redacted>
Cc: Jeff Mahoney <redacted>
Cc: David Sterba <dsterba@suse.com>
---
 fs/btrfs/super.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 81107ad49f3a..dd4980df5b8e 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2369,7 +2369,7 @@ static __cold void btrfs_interface_exit(void)
 
 static void __init btrfs_print_mod_info(void)
 {
-	pr_info("Btrfs loaded, crc32c=%s"
+	static const char fmt[] = KERN_INFO "Btrfs loaded, crc32c=%s"
 #ifdef CONFIG_BTRFS_DEBUG
 			", debug=on"
 #endif
@@ -2382,8 +2382,8 @@ static void __init btrfs_print_mod_info(void)
 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
 			", ref-verify=on"
 #endif
-			"\n",
-			crc32c_impl());
+			"\n";
+	printk(fmt, crc32c_impl());
I'd rather not see more printk being added. Nothing prevents from having
the fmt string being passed to pr_info.
So you mean to do

+	static const char fmt[] = "Btrfs loaded, crc32c=%s"
+	pr_info(fmt);
Pretty much, something along the lines of

pr_info(fmt, crc32c_impl).

printk requires having the KERN_INFO in the format string, which I see
no point in doing, correct me if I'm wrong?
?

Re: [PATCH] btrfs: Fix a C compliance issue

From: Bart Van Assche <hidden>
Date: 2018-06-20 16:44:58 

On Mon, 2018-06-18 at 12:31 +0300, Nikolay Borisov wrote:
On 18.06.2018 12:26, David Sterba wrote:
quoted
On Sat, Jun 16, 2018 at 01:28:13PM +0300, Nikolay Borisov wrote:
quoted
I'd rather not see more printk being added. Nothing prevents from having
the fmt string being passed to pr_info.
So you mean to do

+	static const char fmt[] = "Btrfs loaded, crc32c=%s"
+	pr_info(fmt);
Pretty much, something along the lines of

pr_info(fmt, crc32c_impl).

printk requires having the KERN_INFO in the format string, which I see
no point in doing, correct me if I'm wrong?
You should know that what you proposed doesn't compile because pr_info()
relies on string concatenation and hence requires that its first argument is
a string constant instead of a const char pointer. Anyway, I will rework this
patch such that it uses pr_info() instead of printk().

Bart.


���{.n�+�������+%�����ݶ��w��{.n�+����{���k~���^n�r���z���h����&���z��z�ޗ�+��+zf���h���~����i��������z_��j:+v���)ߣ�m

Re: [PATCH] btrfs: Fix a C compliance issue

From: David Sterba <hidden>
Date: 2018-06-20 16:58:48 

On Wed, Jun 20, 2018 at 04:44:54PM +0000, Bart Van Assche wrote:
On Mon, 2018-06-18 at 12:31 +0300, Nikolay Borisov wrote:
quoted
On 18.06.2018 12:26, David Sterba wrote:
quoted
On Sat, Jun 16, 2018 at 01:28:13PM +0300, Nikolay Borisov wrote:
quoted
I'd rather not see more printk being added. Nothing prevents from having
the fmt string being passed to pr_info.
So you mean to do

+	static const char fmt[] = "Btrfs loaded, crc32c=%s"
+	pr_info(fmt);
Pretty much, something along the lines of

pr_info(fmt, crc32c_impl).

printk requires having the KERN_INFO in the format string, which I see
no point in doing, correct me if I'm wrong?
You should know that what you proposed doesn't compile because pr_info()
relies on string concatenation and hence requires that its first argument is
a string constant instead of a const char pointer. Anyway, I will rework this
patch such that it uses pr_info() instead of printk().
Right, the pr_info(fmt,...) does not compile. The closest version I got to is
below. It does not look pretty, but I can't think of a better version right
now.

--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2369,7 +2369,8 @@ static __cold void btrfs_interface_exit(void)
 
 static void __init btrfs_print_mod_info(void)
 {
-       static const char fmt[] = KERN_INFO "Btrfs loaded, crc32c=%s"
+       static const char fmt1[] = "Btrfs loaded, crc32c=";
+       static const char fmt2[] =
 #ifdef CONFIG_BTRFS_DEBUG
                        ", debug=on"
 #endif
@@ -2383,7 +2384,7 @@ static void __init btrfs_print_mod_info(void)
                        ", ref-verify=on"
 #endif
                        "\n";
-       printk(fmt, crc32c_impl());
+       pr_info("%s%s%s", fmt1, crc32c_impl(), fmt2);
 }
 
 static int __init init_btrfs_fs(void)

Re: [PATCH] btrfs: Fix a C compliance issue

From: Jeff Mahoney <hidden>
Date: 2018-06-20 17:20:00 

On 6/20/18 12:55 PM, David Sterba wrote:
quoted hunk
On Wed, Jun 20, 2018 at 04:44:54PM +0000, Bart Van Assche wrote:
quoted
On Mon, 2018-06-18 at 12:31 +0300, Nikolay Borisov wrote:
quoted
On 18.06.2018 12:26, David Sterba wrote:
quoted
On Sat, Jun 16, 2018 at 01:28:13PM +0300, Nikolay Borisov wrote:
quoted
I'd rather not see more printk being added. Nothing prevents from having
the fmt string being passed to pr_info.
So you mean to do

+	static const char fmt[] = "Btrfs loaded, crc32c=%s"
+	pr_info(fmt);
Pretty much, something along the lines of

pr_info(fmt, crc32c_impl).

printk requires having the KERN_INFO in the format string, which I see
no point in doing, correct me if I'm wrong?
You should know that what you proposed doesn't compile because pr_info()
relies on string concatenation and hence requires that its first argument is
a string constant instead of a const char pointer. Anyway, I will rework this
patch such that it uses pr_info() instead of printk().
Right, the pr_info(fmt,...) does not compile. The closest version I got to is
below. It does not look pretty, but I can't think of a better version right
now.

--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2369,7 +2369,8 @@ static __cold void btrfs_interface_exit(void)
 
 static void __init btrfs_print_mod_info(void)
 {
-       static const char fmt[] = KERN_INFO "Btrfs loaded, crc32c=%s"
+       static const char fmt1[] = "Btrfs loaded, crc32c=";
+       static const char fmt2[] =
 #ifdef CONFIG_BTRFS_DEBUG
                        ", debug=on"
 #endif
@@ -2383,7 +2384,7 @@ static void __init btrfs_print_mod_info(void)
                        ", ref-verify=on"
 #endif
                        "\n";
-       printk(fmt, crc32c_impl());
+       pr_info("%s%s%s", fmt1, crc32c_impl(), fmt2);
 }
 
 static int __init init_btrfs_fs(void)
The shed should be yellow.

-Jeff

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 891cd2ed5dd4..57c9da0b459f 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2375,21 +2375,20 @@ static __cold void btrfs_interface_exit(void)

 static void __init btrfs_print_mod_info(void)
 {
-	pr_info("Btrfs loaded, crc32c=%s"
+	pr_info("Btrfs loaded, crc32c=%s", crc32c_impl());
 #ifdef CONFIG_BTRFS_DEBUG
-			", debug=on"
+	pr_cont(", debug=on");
 #endif
 #ifdef CONFIG_BTRFS_ASSERT
-			", assert=on"
+	pr_cont(", assert=on");
 #endif
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
-			", integrity-checker=on"
+	pr_cont(", integrity-checker=on");
 #endif
 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
-			", ref-verify=on"
+	pr_cont(", ref-verify=on")
 #endif
-			"\n",
-			crc32c_impl());
+	pr_cont("\n");
 }

 static int null_open(struct block_device *bdev, fmode_t mode)



-- 
Jeff Mahoney
SUSE Labs

Re: [PATCH] btrfs: Fix a C compliance issue

From: Bart Van Assche <hidden>
Date: 2018-06-20 17:58:58 

On Wed, 2018-06-20 at 13:19 -0400, Jeff Mahoney wrote:
quoted hunk
The shed should be yellow.

-Jeff

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 891cd2ed5dd4..57c9da0b459f 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2375,21 +2375,20 @@ static __cold void btrfs_interface_exit(void)

 static void __init btrfs_print_mod_info(void)
 {
-	pr_info("Btrfs loaded, crc32c=%s"
+	pr_info("Btrfs loaded, crc32c=%s", crc32c_impl());
 #ifdef CONFIG_BTRFS_DEBUG
-			", debug=on"
+	pr_cont(", debug=on");
 #endif
 #ifdef CONFIG_BTRFS_ASSERT
-			", assert=on"
+	pr_cont(", assert=on");
 #endif
 #ifdef CONFIG_BTRFS_FS_CHECK_INTEGRITY
-			", integrity-checker=on"
+	pr_cont(", integrity-checker=on");
 #endif
 #ifdef CONFIG_BTRFS_FS_REF_VERIFY
-			", ref-verify=on"
+	pr_cont(", ref-verify=on")
 #endif
-			"\n",
-			crc32c_impl());
+	pr_cont("\n");
 }

 static int null_open(struct block_device *bdev, fmode_t mode)
Since we are doing bikeshedding, let me contribute to it :-)

From scripts/checkpatch.pl:
                if ($line =~ /\bprintk\s*\(\s*KERN_CONT\b|\bpr_cont\s*\(/) {
                        WARN("LOGGING_CONTINUATION",
                             "Avoid logging continuation uses where feasible\n" . $herecurr);
                }

Bart.





���{.n�+�������+%�����ݶ��w��{.n�+����{���k~���^n�r���z���h����&���z��z�ޗ�+��+zf���h���~����i��������z_��j:+v���)ߣ�m
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help