diff for duplicates of <00bb01d59dcd$4b1791b0$e146b510$@samsung.com> diff --git a/a/1.txt b/N1/1.txt index 18426f7..9eec2f9 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,94 +1,94 @@ -> … -> > +++ b/fs/exfat/super.c -> … -> > +static int exfat_show_options(struct seq_file *m, struct dentry *root) -> > +{ -> … -> > + seq_printf(m, ",fmask=%04o", opts->fs_fmask); -> > + seq_printf(m, ",dmask=%04o", opts->fs_dmask); -> -> How do you think about to combine these two function calls into a single one? -> -> -> > +static int __exfat_fill_super(struct super_block *sb) -> > +{ -> … -> > + exfat_msg(sb, KERN_ERR, "unable to read boot sector"); -> > + ret = -EIO; -> > + goto out; -> … -> -> Would you like to simplify this place? -> -> + return -EIO; -> -> -> … -> > + exfat_msg(sb, KERN_ERR, "failed to load upcase table"); -> > + goto out; -> -> Would you like to omit this label? -> -> + return ret; -> -> -> > +static int exfat_fill_super(struct super_block *sb, struct fs_context *fc) -> > +{ -> … -> > + exfat_msg(sb, KERN_ERR, "failed to recognize exfat type"); -> > + goto failed_mount; -> -> The local variable “root_inode” contains still a null pointer at this place. -> -> * Thus I would find a jump target like “reset_s_root” more appropriate. -> -> * Can the corresponding pointer initialisation be omitted then? -> -> -> … -> > +failed_mount: -> > + if (root_inode) -> > + iput(root_inode); -> … -> -> I am informed in the way that this function tolerates the passing -> of null pointers. -> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/i -> node.c?id=1d4c79ed324ad780cfc3ad38364ba1fd585dd2a8#n1567 -> https://protect2.fireeye.com/url?k=34e5568f-697957ef-34e4ddc0-0cc47a31307c- -> 7f9b30869a6ffaa4&u=https://elixir.bootlin.com/linux/v5.4- -> rc7/source/fs/inode.c#L1567 -> -> Thus I suggest to omit the extra pointer check also at this place. -> -> -> > +static int __init init_exfat_fs(void) -> > +{ -> … -> + err = exfat_cache_init(); -> + if (err) -> + goto error; -> -> Can it be nicer to return directly? -> -> -> … -> > + if (!exfat_inode_cachep) -> > + goto error; -> -> Can an other jump target like “shutdown_cache” be more appropriate? -> -> -> > + err = register_filesystem(&exfat_fs_type); -> > + if (err) -> > + goto error; -> … -> -> Can the label “destroy_cache” be more appropriate? -> -> -I checked your all points, Will fix them on V2. -Thanks for your review! - -> Regards, +> …\r +> > +++ b/fs/exfat/super.c\r +> …\r +> > +static int exfat_show_options(struct seq_file *m, struct dentry *root)\r +> > +{\r +> …\r +> > + seq_printf(m, ",fmask=%04o", opts->fs_fmask);\r +> > + seq_printf(m, ",dmask=%04o", opts->fs_dmask);\r +> \r +> How do you think about to combine these two function calls into a single one?\r +> \r +> \r +> > +static int __exfat_fill_super(struct super_block *sb)\r +> > +{\r +> …\r +> > + exfat_msg(sb, KERN_ERR, "unable to read boot sector");\r +> > + ret = -EIO;\r +> > + goto out;\r +> …\r +> \r +> Would you like to simplify this place?\r +> \r +> + return -EIO;\r +> \r +> \r +> …\r +> > + exfat_msg(sb, KERN_ERR, "failed to load upcase table");\r +> > + goto out;\r +> \r +> Would you like to omit this label?\r +> \r +> + return ret;\r +> \r +> \r +> > +static int exfat_fill_super(struct super_block *sb, struct fs_context *fc)\r +> > +{\r +> …\r +> > + exfat_msg(sb, KERN_ERR, "failed to recognize exfat type");\r +> > + goto failed_mount;\r +> \r +> The local variable “root_inode” contains still a null pointer at this place.\r +> \r +> * Thus I would find a jump target like “reset_s_root” more appropriate.\r +> \r +> * Can the corresponding pointer initialisation be omitted then?\r +> \r +> \r +> …\r +> > +failed_mount:\r +> > + if (root_inode)\r +> > + iput(root_inode);\r +> …\r +> \r +> I am informed in the way that this function tolerates the passing\r +> of null pointers.\r +> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/i\r +> node.c?id=1d4c79ed324ad780cfc3ad38364ba1fd585dd2a8#n1567\r +> https://protect2.fireeye.com/url?k=34e5568f-697957ef-34e4ddc0-0cc47a31307c-\r +> 7f9b30869a6ffaa4&u=https://elixir.bootlin.com/linux/v5.4-\r +> rc7/source/fs/inode.c#L1567\r +> \r +> Thus I suggest to omit the extra pointer check also at this place.\r +> \r +> \r +> > +static int __init init_exfat_fs(void)\r +> > +{\r +> …\r +> + err = exfat_cache_init();\r +> + if (err)\r +> + goto error;\r +> \r +> Can it be nicer to return directly?\r +> \r +> \r +> …\r +> > + if (!exfat_inode_cachep)\r +> > + goto error;\r +> \r +> Can an other jump target like “shutdown_cache” be more appropriate?\r +> \r +> \r +> > + err = register_filesystem(&exfat_fs_type);\r +> > + if (err)\r +> > + goto error;\r +> …\r +> \r +> Can the label “destroy_cache” be more appropriate?\r +> \r +> \r +I checked your all points, Will fix them on V2.\r +Thanks for your review!\r +\r +> Regards,\r > Markus diff --git a/a/content_digest b/N1/content_digest index 58ea9e8..00f70b3 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -3,9 +3,9 @@ "ref\09e9bac40-109c-3349-24da-532c540638c2@web.de\0" "From\0Namjae Jeon <namjae.jeon@samsung.com>\0" "Subject\0RE: [PATCH 02/13] exfat: add super block operations\0" - "Date\0Mon, 18 Nov 2019 05:01:52 +0000\0" + "Date\0Mon, 18 Nov 2019 14:01:52 +0900\0" "To\0'Markus Elfring' <Markus.Elfring@web.de>" - " linux-fsdevel@vger.kernel.org\0" + " <linux-fsdevel@vger.kernel.org>\0" "Cc\0linux-kernel@vger.kernel.org" kernel-janitors@vger.kernel.org 'Christoph Hellwig' <hch@lst.de> @@ -15,99 +15,99 @@ " linkinjeon@gmail.com\0" "\00:1\0" "b\0" - "> \342\200\246\n" - "> > +++ b/fs/exfat/super.c\n" - "> \342\200\246\n" - "> > +static int exfat_show_options(struct seq_file *m, struct dentry *root)\n" - "> > +{\n" - "> \342\200\246\n" - "> > +\tseq_printf(m, \",fmask=%04o\", opts->fs_fmask);\n" - "> > +\tseq_printf(m, \",dmask=%04o\", opts->fs_dmask);\n" - "> \n" - "> How do you think about to combine these two function calls into a single one?\n" - "> \n" - "> \n" - "> > +static int __exfat_fill_super(struct super_block *sb)\n" - "> > +{\n" - "> \342\200\246\n" - "> > +\t\texfat_msg(sb, KERN_ERR, \"unable to read boot sector\");\n" - "> > +\t\tret = -EIO;\n" - "> > +\t\tgoto out;\n" - "> \342\200\246\n" - "> \n" - "> Would you like to simplify this place?\n" - "> \n" - "> +\t\treturn -EIO;\n" - "> \n" - "> \n" - "> \342\200\246\n" - "> > +\t\texfat_msg(sb, KERN_ERR, \"failed to load upcase table\");\n" - "> > +\t\tgoto out;\n" - "> \n" - "> Would you like to omit this label?\n" - "> \n" - "> +\t\treturn ret;\n" - "> \n" - "> \n" - "> > +static int exfat_fill_super(struct super_block *sb, struct fs_context *fc)\n" - "> > +{\n" - "> \342\200\246\n" - "> > +\t\texfat_msg(sb, KERN_ERR, \"failed to recognize exfat type\");\n" - "> > +\t\tgoto failed_mount;\n" - "> \n" - "> The local variable \342\200\234root_inode\342\200\235 contains still a null pointer at this place.\n" - "> \n" - "> * Thus I would find a jump target like \342\200\234reset_s_root\342\200\235 more appropriate.\n" - "> \n" - "> * Can the corresponding pointer initialisation be omitted then?\n" - "> \n" - "> \n" - "> \342\200\246\n" - "> > +failed_mount:\n" - "> > +\tif (root_inode)\n" - "> > +\t\tiput(root_inode);\n" - "> \342\200\246\n" - "> \n" - "> I am informed in the way that this function tolerates the passing\n" - "> of null pointers.\n" - "> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/i\n" - "> node.c?id=1d4c79ed324ad780cfc3ad38364ba1fd585dd2a8#n1567\n" - "> https://protect2.fireeye.com/url?k=34e5568f-697957ef-34e4ddc0-0cc47a31307c-\n" - "> 7f9b30869a6ffaa4&u=https://elixir.bootlin.com/linux/v5.4-\n" - "> rc7/source/fs/inode.c#L1567\n" - "> \n" - "> Thus I suggest to omit the extra pointer check also at this place.\n" - "> \n" - "> \n" - "> > +static int __init init_exfat_fs(void)\n" - "> > +{\n" - "> \342\200\246\n" - "> +\terr = exfat_cache_init();\n" - "> +\tif (err)\n" - "> +\t\tgoto error;\n" - "> \n" - "> Can it be nicer to return directly?\n" - "> \n" - "> \n" - "> \342\200\246\n" - "> > +\tif (!exfat_inode_cachep)\n" - "> > +\t\tgoto error;\n" - "> \n" - "> Can an other jump target like \342\200\234shutdown_cache\342\200\235 be more appropriate?\n" - "> \n" - "> \n" - "> > +\terr = register_filesystem(&exfat_fs_type);\n" - "> > +\tif (err)\n" - "> > +\t\tgoto error;\n" - "> \342\200\246\n" - "> \n" - "> Can the label \342\200\234destroy_cache\342\200\235 be more appropriate?\n" - "> \n" - "> \n" - "I checked your all points, Will fix them on V2.\n" - "Thanks for your review!\n" - "\n" - "> Regards,\n" + "> \342\200\246\r\n" + "> > +++ b/fs/exfat/super.c\r\n" + "> \342\200\246\r\n" + "> > +static int exfat_show_options(struct seq_file *m, struct dentry *root)\r\n" + "> > +{\r\n" + "> \342\200\246\r\n" + "> > +\tseq_printf(m, \",fmask=%04o\", opts->fs_fmask);\r\n" + "> > +\tseq_printf(m, \",dmask=%04o\", opts->fs_dmask);\r\n" + "> \r\n" + "> How do you think about to combine these two function calls into a single one?\r\n" + "> \r\n" + "> \r\n" + "> > +static int __exfat_fill_super(struct super_block *sb)\r\n" + "> > +{\r\n" + "> \342\200\246\r\n" + "> > +\t\texfat_msg(sb, KERN_ERR, \"unable to read boot sector\");\r\n" + "> > +\t\tret = -EIO;\r\n" + "> > +\t\tgoto out;\r\n" + "> \342\200\246\r\n" + "> \r\n" + "> Would you like to simplify this place?\r\n" + "> \r\n" + "> +\t\treturn -EIO;\r\n" + "> \r\n" + "> \r\n" + "> \342\200\246\r\n" + "> > +\t\texfat_msg(sb, KERN_ERR, \"failed to load upcase table\");\r\n" + "> > +\t\tgoto out;\r\n" + "> \r\n" + "> Would you like to omit this label?\r\n" + "> \r\n" + "> +\t\treturn ret;\r\n" + "> \r\n" + "> \r\n" + "> > +static int exfat_fill_super(struct super_block *sb, struct fs_context *fc)\r\n" + "> > +{\r\n" + "> \342\200\246\r\n" + "> > +\t\texfat_msg(sb, KERN_ERR, \"failed to recognize exfat type\");\r\n" + "> > +\t\tgoto failed_mount;\r\n" + "> \r\n" + "> The local variable \342\200\234root_inode\342\200\235 contains still a null pointer at this place.\r\n" + "> \r\n" + "> * Thus I would find a jump target like \342\200\234reset_s_root\342\200\235 more appropriate.\r\n" + "> \r\n" + "> * Can the corresponding pointer initialisation be omitted then?\r\n" + "> \r\n" + "> \r\n" + "> \342\200\246\r\n" + "> > +failed_mount:\r\n" + "> > +\tif (root_inode)\r\n" + "> > +\t\tiput(root_inode);\r\n" + "> \342\200\246\r\n" + "> \r\n" + "> I am informed in the way that this function tolerates the passing\r\n" + "> of null pointers.\r\n" + "> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/i\r\n" + "> node.c?id=1d4c79ed324ad780cfc3ad38364ba1fd585dd2a8#n1567\r\n" + "> https://protect2.fireeye.com/url?k=34e5568f-697957ef-34e4ddc0-0cc47a31307c-\r\n" + "> 7f9b30869a6ffaa4&u=https://elixir.bootlin.com/linux/v5.4-\r\n" + "> rc7/source/fs/inode.c#L1567\r\n" + "> \r\n" + "> Thus I suggest to omit the extra pointer check also at this place.\r\n" + "> \r\n" + "> \r\n" + "> > +static int __init init_exfat_fs(void)\r\n" + "> > +{\r\n" + "> \342\200\246\r\n" + "> +\terr = exfat_cache_init();\r\n" + "> +\tif (err)\r\n" + "> +\t\tgoto error;\r\n" + "> \r\n" + "> Can it be nicer to return directly?\r\n" + "> \r\n" + "> \r\n" + "> \342\200\246\r\n" + "> > +\tif (!exfat_inode_cachep)\r\n" + "> > +\t\tgoto error;\r\n" + "> \r\n" + "> Can an other jump target like \342\200\234shutdown_cache\342\200\235 be more appropriate?\r\n" + "> \r\n" + "> \r\n" + "> > +\terr = register_filesystem(&exfat_fs_type);\r\n" + "> > +\tif (err)\r\n" + "> > +\t\tgoto error;\r\n" + "> \342\200\246\r\n" + "> \r\n" + "> Can the label \342\200\234destroy_cache\342\200\235 be more appropriate?\r\n" + "> \r\n" + "> \r\n" + "I checked your all points, Will fix them on V2.\r\n" + "Thanks for your review!\r\n" + "\r\n" + "> Regards,\r\n" > Markus -feb76f6b59f63bbfc329f87eaa1276bea26eb975f68e9f63da4fe677852c922f +737ef900d1481bd9dda30b1c7d2d7d703e2894f3b0f3fc52ab2571b3b4b55de4
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.