…
> +++ 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/inode.c?id=1d4c79ed324ad780cfc3ad38364ba1fd585dd2a8#n1567
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?
Regards,
Markus
