Re: [PATCH v4 04/13] exfat: add directory operations

[Markus Elfring]

â
> +++ b/fs/exfat/dir.c
â
> +static int exfat_readdir(struct inode *inode, struct exfat_dir_entry *dir_entry)
> +{
â
> +			if (!ep) {
> +				ret = -EIO;
> +				goto free_clu;
> +			}

How do you think about to move a bit of common exception handling code
(at similar places)?

+			if (!ep)
+				goto e_io;


â
> +free_clu:
> +	kfree(clu);
> +	return ret;

+
+e_io:
+	ret = -EIO;
+	goto free_clu;

> +}
â
> +static void exfat_set_entry_type(struct exfat_dentry *ep, unsigned int type)
> +{
> +	if (type == TYPE_UNUSED) {
> +		ep->type = EXFAT_UNUSED;
> +	} else if (type == TYPE_DELETED) {
> +		ep->type &= EXFAT_DELETE;

The other assignments are working without the ampersand.
Thus I would find its usage suspicious at this place.


â
> +int update_dir_chksum(struct inode *inode, struct exfat_chain *p_dir,
> +		int entry)
> +{
â
> +out_unlock:
> +	brelse(fbh);

Can the label ârelease_fbhâ be more helpful?


â
> +struct exfat_entry_set_cache *exfat_get_dentry_set(struct super_block *sb,
> +		struct exfat_chain *p_dir, int entry, unsigned int type,
> +		struct exfat_dentry **file_ep)
> +{
â
> +	/* FIXME : is available in error case? */
> +	if (p_dir->dir == DIR_DELETED) {
> +		exfat_msg(sb, KERN_ERR, "access to deleted dentry\n");
> +		return NULL;
> +	}

Will this place need any further improvements?


â
> +	bh = sb_bread(sb, sec);
> +	if (!bh)
> +		goto err_out;

Can it be nicer to return directly?


â
> +	ep = (struct exfat_dentry *)(bh->b_data + off);
> +	entry_type = exfat_get_entry_type(ep);
> +
> +	if (entry_type != TYPE_FILE && entry_type != TYPE_DIR)
> +		goto err_out;

+		goto release_bh;

â
> +		if (!exfat_validate_entry(exfat_get_entry_type(ep), &mode))
> +			goto err_out;


â
> +	brelse(bh);
> +
> +	return es;
> +err_out:
> +	kfree(es);
> +	brelse(bh);

I suggest to improve the exception handling also for this function implementation.

+	brelse(bh);
+	return es;
+
+free_es:
+	kfree(es);
+release_bh:
+	brelse(bh);


* Would you like to adjust any more jump targets at similar places?

* Can another initialisation be omitted then for a pointer variable?

Regards,
Markus