:ext4 these lines are too long while reading
diff mbox

Message ID 57D281D1.6020208@huawei.com
State New
Headers show

Commit Message

Norton.Zhu Sept. 9, 2016, 9:33 a.m. UTC
Hi, all

I'm a freshman in ext4 file system and I'm reading its source code now.
This patch did nothing but make it looks better.(
some lines are too long in vim :( ).

Thanks.

Signed-off-by: Norton <norton.zhu@huawei.com>

Comments

Theodore Y. Ts'o Sept. 9, 2016, 1:47 p.m. UTC | #1
On Fri, Sep 09, 2016 at 05:33:05PM +0800, norton wrote:
> Hi, all
> 
> I'm a freshman in ext4 file system and I'm reading its source code now.
> This patch did nothing but make it looks better.(
> some lines are too long in vim :( ).

We don't take line-remapping patches.  It used to be the case that
checkpatch would complain if lines were too long, and then it started
complaining if the lines were wrapped.  I don't like checkpatch,
because it causes unnecessary patch churn.  :-(

May I suggest instead that you take a look at reconfiguring vim?

http://stackoverflow.com/questions/467739/how-do-you-get-vim-to-display-wrapped-lines-without-inserting-newlines

					- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sandeen Sept. 9, 2016, 2:34 p.m. UTC | #2
On 9/9/16 4:33 AM, norton wrote:
> Hi, all
> 
> I'm a freshman in ext4 file system and I'm reading its source code now.
> This patch did nothing but make it looks better.(
> some lines are too long in vim :( ).

Aside from Ted's point about not taking patches which simply rewrap
lines, it's worth knowing that the better practice, IMHO, when adding
long text strings is not:

+				ext4_msg(sb, KERN_WARNING, "Remounting file "
+					"system with no journal so "
+					"ignoring journalled data option");

but:

+				ext4_msg(sb, KERN_WARNING,
+"Remounting file system with no journal so ignoring journalled data option");

so that the strings can be found by a grep, if needed, while still
avoiding 120-column lines.

At least that's my time-tested opinion :)

So if you ever find yourself in the position of needing to add a string like
that, or are hitting it as part of other work, it's something to consider.

-Eric


--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Dilger Sept. 9, 2016, 8:05 p.m. UTC | #3
On Sep 9, 2016, at 3:33 AM, norton <norton.zhu@huawei.com> wrote:
> 
> Hi, all
> 
> I'm a freshman in ext4 file system and I'm reading its source code now.
> This patch did nothing but make it looks better.(
> some lines are too long in vim :( ).
> 
> Thanks.
> 
> Signed-off-by: Norton <norton.zhu@huawei.com>
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 3ec8708..09314f8 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -109,7 +109,8 @@ static void ext4_clear_request_list(void);
>  * transaction start -> page lock(s) -> i_data_sem (rw)
>  */
> 
> -#if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && defined(CONFIG_EXT4_USE_FOR_EXT2)
> +#if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && \
> +	defined(CONFIG_EXT4_USE_FOR_EXT2)
> static struct file_system_type ext2_fs_type = {
> 	.owner		= THIS_MODULE,
> 	.name		= "ext2",
> @@ -1751,7 +1752,9 @@ static int handle_mount_opt(struct super_block *sb, char *opt, int token,
> 	} else if (m->flags & MOPT_DATAJ) {
> 		if (is_remount) {
> 			if (!sbi->s_journal)
> -				ext4_msg(sb, KERN_WARNING, "Remounting file system with no journal so ignoring journalled data option");
> +				ext4_msg(sb, KERN_WARNING, "Remounting file "
> +					"system with no journal so "
> +					"ignoring journalled data option");
> 			else if (test_opt(sb, DATA_FLAGS) != m->mount_opt) {
> 				ext4_msg(sb, KERN_ERR,
> 					 "Cannot change data mode on remount");
> @@ -5412,7 +5415,8 @@ static struct dentry *ext4_mount(struct file_system_type *fs_type, int flags,
> 	return mount_bdev(fs_type, flags, dev_name, data, ext4_fill_super);
> }
> 
> -#if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && defined(CONFIG_EXT4_USE_FOR_EXT2)
> +#if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && \
> +	defined(CONFIG_EXT4_USE_FOR_EXT2)
> static inline void register_as_ext2(void)
> {
> 	int err = register_filesystem(&ext2_fs_type);
> @@ -5555,7 +5559,8 @@ static void __exit ext4_exit_fs(void)
> 	ext4_exit_es();
> }
> 
> -MODULE_AUTHOR("Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, Theodore Ts'o and others");
> +MODULE_AUTHOR("Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, \
> +		Theodore Ts'o and others");

In addition to the other comments from Ted and Eric, the above change is
actually broken, as it would insert two tabs into the middle of the string.

Firstly, the linefeed escape '\' is only for CPP, and not needed for C.

Secondly, to split a string across lines you need to terminate it, and
the compiler will merge adjacent strings:

+MODULE_AUTHOR("Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, "
+	       "Theodore Ts'o and others");

Don't forget the space inside the string at the end of the first line.

Cheers, Andreas
Norton.Zhu Sept. 12, 2016, 1:35 a.m. UTC | #4
Hi, Theodore
Thanks for your suggest, I looks better now.:)

On 2016/9/9 21:47, Theodore Ts'o wrote:
> On Fri, Sep 09, 2016 at 05:33:05PM +0800, norton wrote:
>> Hi, all
>>
>> I'm a freshman in ext4 file system and I'm reading its source code now.
>> This patch did nothing but make it looks better.(
>> some lines are too long in vim :( ).
> 
> We don't take line-remapping patches.  It used to be the case that
> checkpatch would complain if lines were too long, and then it started
> complaining if the lines were wrapped.  I don't like checkpatch,
> because it causes unnecessary patch churn.  :-(
> 
> May I suggest instead that you take a look at reconfiguring vim?
> 
> http://stackoverflow.com/questions/467739/how-do-you-get-vim-to-display-wrapped-lines-without-inserting-newlines
> 
> 					- Ted
> 
> .
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 3ec8708..09314f8 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -109,7 +109,8 @@  static void ext4_clear_request_list(void);
  * transaction start -> page lock(s) -> i_data_sem (rw)
  */

-#if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && defined(CONFIG_EXT4_USE_FOR_EXT2)
+#if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && \
+	defined(CONFIG_EXT4_USE_FOR_EXT2)
 static struct file_system_type ext2_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "ext2",
@@ -1751,7 +1752,9 @@  static int handle_mount_opt(struct super_block *sb, char *opt, int token,
 	} else if (m->flags & MOPT_DATAJ) {
 		if (is_remount) {
 			if (!sbi->s_journal)
-				ext4_msg(sb, KERN_WARNING, "Remounting file system with no journal so ignoring journalled data option");
+				ext4_msg(sb, KERN_WARNING, "Remounting file "
+					"system with no journal so "
+					"ignoring journalled data option");
 			else if (test_opt(sb, DATA_FLAGS) != m->mount_opt) {
 				ext4_msg(sb, KERN_ERR,
 					 "Cannot change data mode on remount");
@@ -5412,7 +5415,8 @@  static struct dentry *ext4_mount(struct file_system_type *fs_type, int flags,
 	return mount_bdev(fs_type, flags, dev_name, data, ext4_fill_super);
 }

-#if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && defined(CONFIG_EXT4_USE_FOR_EXT2)
+#if !defined(CONFIG_EXT2_FS) && !defined(CONFIG_EXT2_FS_MODULE) && \
+	defined(CONFIG_EXT4_USE_FOR_EXT2)
 static inline void register_as_ext2(void)
 {
 	int err = register_filesystem(&ext2_fs_type);
@@ -5555,7 +5559,8 @@  static void __exit ext4_exit_fs(void)
 	ext4_exit_es();
 }

-MODULE_AUTHOR("Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, Theodore Ts'o and others");
+MODULE_AUTHOR("Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, \
+		Theodore Ts'o and others");
 MODULE_DESCRIPTION("Fourth Extended Filesystem");
 MODULE_LICENSE("GPL");
 module_init(ext4_init_fs)