Re: [PATCH v4 10/13] exfat: add nls operations

From: Markus Elfring <Markus.Elfring@web.de>
To: Namjae Jeon <namjae.jeon@samsung.com>, linux-fsdevel@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org,
	"Christoph Hellwig" <hch@lst.de>,
	"Daniel Wagner" <dwagner@suse.de>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Nikolay Borisov" <nborisov@suse.com>,
	"Sungjong Seo" <sj1557.seo@samsung.com>,
	"Valdis Klētnieks" <valdis.kletnieks@vt.edu>,
	linkinjeon@gmail.com
Subject: Re: [PATCH v4 10/13] exfat: add nls operations
Date: Thu, 21 Nov 2019 09:09:10 +0100	[thread overview]
Message-ID: <df089831-038c-3b39-6ec7-684d1f698756@web.de> (raw)
In-Reply-To: <20191121052618.31117-11-namjae.jeon@samsung.com>

…
> +++ b/fs/exfat/nls.c
…
> +static int exfat_load_upcase_table(struct super_block *sb,
> +		sector_t sector, unsigned long long num_sectors,
> +		unsigned int utbl_checksum)
> +{
…
> +	int ret = -EIO;
…
> +	while (sector < num_sectors) {
> +		bh = sb_bread(sb, sector);
> +		if (!bh) {
> +			exfat_msg(sb, KERN_ERR,
> +				"failed to read sector(0x%llx)\n", sector);
> +			goto release_bh;
> +		}
…
> +	}
> +
> +	if (index >= 0xFFFF && utbl_checksum == checksum) {
> +		brelse(bh);
> +		return 0;
> +	}
> +
> +	exfat_msg(sb, KERN_ERR,
> +			"failed to load upcase table (idx : 0x%08x, chksum : 0x%08x, utbl_chksum : 0x%08x)\n",
> +			index, checksum, utbl_checksum);
> +
> +	ret = -EINVAL;

Can a blank line be omitted between the message and the error code?

> +release_bh:
> +	brelse(bh);
> +	exfat_free_upcase_table(sb);
> +	return ret;
> +}

I got the impression that the resource management is still questionable
for this function implementation.

1. Now I suggest to move the call of the function “brelse” to the end
   of the while loop. The label “release_bh” would be renamed to “free_table” then.

2. Can a variable initialisation be converted to the assignment “ret = -EIO;”
   in an if branch?

Regards,
Markus