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

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

[Markus Elfring]

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

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

[Namjae Jeon]

> > +	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?
Okay.
> 
> 
> > +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.
Okay.
> 
> 2. Can a variable initialisation be converted to the assignment “ret = -EIO;”
>    in an if branch?
Okay, Will fix it on v5.

Thanks for your review!
> 
> Regards,
> Markus

Re: [PATCH v4 00/13] add the latest exfat driver

[Markus Elfring]

…
> v2:
…
>  - Rename proper goto labels in several places.

I find an other wording more appropriate.
A renaming would have not been needed if these identifiers were “proper” before.

Were any update candidates left over according to this change pattern?

Regards,
Markus

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

[Markus Elfring]

…
> +++ b/fs/exfat/super.c
…
> +static int __init init_exfat_fs(void)
> +{
…
> +	err = -ENOMEM;
> +	exfat_inode_cachep = kmem_cache_create("exfat_inode_cache",
> +			sizeof(struct exfat_inode_info),
> +			0, SLAB_RECLAIM_ACCOUNT | SLAB_MEM_SPREAD,
> +			exfat_inode_init_once);
> +	if (!exfat_inode_cachep)
> +		goto shutdown_cache;

Should such an error code assignment be performed only after a failed function call?

Regards,
Markus

Re: [PATCH v4 03/13] exfat: add inode operations

[Markus Elfring]

…
> +++ b/fs/exfat/inode.c
…
> +static int exfat_bmap(struct inode *inode, sector_t sector, sector_t *phys,
> +		unsigned long *mapped_blocks, int *create)
> +{
…
> +	err = exfat_map_cluster(inode, clu_offset, &cluster,
> +		*create & BMAP_ADD_CLUSTER);

I find an other indentation more appropriate.
Please align the last parameter below (or besides) the opening parenthesis.


> +	if (err) {
> +		if (err != -ENOSPC)
> +			return -EIO;
> +		return err;
> +	}

Can such source code become more succinct?

+	if (err)
+		return err != -ENOSPC ? -EIO : err;

Regards,
Markus

Re: [PATCH v4 03/13] exfat: add inode operations

[Greg Kroah-Hartman]

On Thu, Nov 21, 2019 at 11:40:28AM +0100, Markus Elfring wrote:
> > +	if (err) {
> > +		if (err != -ENOSPC)
> > +			return -EIO;
> > +		return err;
> > +	}
> 
> Can such source code become more succinct?
> 
> +	if (err)
> +		return err != -ENOSPC ? -EIO : err;

No, the original is best here.  Never use ? : if you can ever help it.

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

Re: [PATCH v4 05/13] exfat: add file operations

[Markus Elfring]

…
> +++ b/fs/exfat/file.c
…
> +void exfat_truncate(struct inode *inode, loff_t size)
> +{
…
> +out:
> +	aligned_size = i_size_read(inode);

Can a label like “read_size” be more helpful?

Regards,
Markus

Re: [PATCH v4 06/13] exfat: add exfat entry operations

[Markus Elfring]

…
> +++ b/fs/exfat/fatent.c
…
> +int exfat_free_cluster(struct inode *inode, struct exfat_chain *p_chain)
> +{
…
> +out:
> +	sbi->used_clusters -= num_clusters;

Can a label like “decrement_counter” be more helpful?


…
> +int exfat_mirror_bh(struct super_block *sb, sector_t sec,
> +		struct buffer_head *bh)
> +{
…
> +		if (!c_bh) {
> +			err = -ENOMEM;
> +			goto out;
> +		}

Can it be nicer to return directly?

…
> +out:
> +	return err;
> +}

Would you like to omit such a label for this function implementation?

Regards,
Markus

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

[Namjae Jeon]

> 
> …
> > +++ 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)?
Not sure it is good. Other review comments are okay. Will fix them on v5.
> 
> +			if (!ep)
> +				goto e_io;
> 
> 
> …
> > +free_clu:
> > +	kfree(clu);
> > +	return ret;
> 
> +
> +e_io:
> +	ret = -EIO;
> +	goto free_clu;
> 
> > +}

Re: [PATCH v4 03/13] exfat: add inode operations

[Christoph Hellwig]

On Thu, Nov 21, 2019 at 11:40:28AM +0100, Markus Elfring wrote:
> > +	err = exfat_map_cluster(inode, clu_offset, &cluster,
> > +		*create & BMAP_ADD_CLUSTER);
> 
> I find an other indentation more appropriate.
> Please align the last parameter below (or besides) the opening parenthesis.

It is great that you find that, but I think we can leave this to
the code author.  We have plenty of example for opening brace aligned,
one or two tabs alignments in the code, and it is up to the author /
maintainer to chose one.  No need to to nitpick their choices inside the
boundaries of the normal coding space.

> > +	if (err) {
> > +		if (err != -ENOSPC)
> > +			return -EIO;
> > +		return err;
> > +	}
> 
> Can such source code become more succinct?
> 
> +	if (err)
> +		return err != -ENOSPC ? -EIO : err;

If it sufficient but a lot harder to follow.

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)?
> Not sure it is good.

The software development opinions can vary also for this change pattern
according to different design goals.
Is such a transformation just another possibility to reduce duplicate
source code a bit?

Regards,
Markus

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

[Namjae Jeon]

> >> …
> >>> +++ 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)?
> > Not sure it is good.
> 
> The software development opinions can vary also for this change pattern
> according to different design goals.
> Is such a transformation just another possibility to reduce duplicate
> source code a bit?
I changed it without unnecessary goto abuse. Look at the next version later.
Thanks!
> 
> Regards,
> Markus

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

[Dan Carpenter]

On Fri, Nov 22, 2019 at 05:21:34PM +0900, Namjae Jeon wrote:
> > The software development opinions can vary also for this change pattern
> > according to different design goals.
> > Is such a transformation just another possibility to reduce duplicate
> > source code a bit?
> I changed it without unnecessary goto abuse. Look at the next version later.
> Thanks!

Markus, could you stop adding kernel-janitors to the CC list?  None of
this is at all related to kernel-janitors.  We can't see the context and
we will never see the "next version" mentioned here.

:P

regards,
dan carpenter

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

[Christoph Hellwig]

Markus,

please leave people alone with your opinions.  They actually do
create value and don't need your personal opinion on coding style
trolling.