Re: [PATCH 02/13] exfat: add super block operations

[Markus Elfring]

â
> +++ 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