Messages in this thread | | | Subject | Re: [PATCH 02/13] exfat: add super block operations | From | Markus Elfring <> | Date | Sun, 17 Nov 2019 14:30:22 +0100 |
| |
… > +++ 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
| |