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

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

[Markus Elfring]

> …, an a random previous

Does this wording contain a typo?


> We plan to treat this version as the future upstream for the code base
> once merged, and all new features and bug fixes will go upstream first.

Were the following mentioned issues occasionally reviewed already
by other developers before?


> v3:
>  - fix wrong sbi->s_dirt set.
>
> v2:
>  - Check the bitmap count up to the total clusters.
>  - Rename proper goto labels in seveal place.

Would you like to avoid further typos in such change descriptions?


>  - Change time mode type with enumeration.

How do you think about to increase the usage of enumerations
at any more source code places?


>  - Directly return error instead of goto at first error check.
>  - Combine seq_printfs calls into a single one.

Please refer to the correct function name.


Thanks for your positive feedback.

Regards,
Markus

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

[Namjae Jeon]

2019-11-19 21:15 GMT+09:00, Markus Elfring <Markus.Elfring@web.de>:
>> …, an a random previous
>
> Does this wording contain a typo?
Yes, Will fix it on next series.

>
>
>> We plan to treat this version as the future upstream for the code base
>> once merged, and all new features and bug fixes will go upstream first.
>
> Were the following mentioned issues occasionally reviewed already
> by other developers before?
https://marc.info/?l=linux-fsdevel&m=156985252507812&w=2

>
>
>> v3:
>>  - fix wrong sbi->s_dirt set.
>>
>> v2:
>>  - Check the bitmap count up to the total clusters.
>>  - Rename proper goto labels in seveal place.
>
> Would you like to avoid further typos in such change descriptions?
Will fix on next series.
>
>
>>  - Change time mode type with enumeration.
>
> How do you think about to increase the usage of enumerations
> at any more source code places?
I will check.
>
>
>>  - Directly return error instead of goto at first error check.
>>  - Combine seq_printfs calls into a single one.
>
> Please refer to the correct function name.
Okay:)
>
>
> Thanks for your positive feedback.
Thanks for your review!
>
> Regards,
> Markus
>

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

[Markus Elfring]

…
> +++ b/fs/exfat/super.c
…
> +static int __exfat_fill_super(struct super_block *sb)
> +{
…
> +free_upcase:
> +	exfat_free_upcase_table(sb);

Label alternatives?
* free_table
* free_upcase_table


…
> +static int exfat_fill_super(struct super_block *sb, struct fs_context *fc)
> +{
…
> +	if (EXFAT_SB(sb)->options.case_sensitive)
> +		sb->s_d_op = &exfat_dentry_ops;
> +	else
> +		sb->s_d_op = &exfat_ci_dentry_ops;

How do you think about the usage of conditional operators at similar places?

+	sb->s_d_op = EXFAT_SB(sb)->options.case_sensitive
+		     ? &exfat_dentry_ops;
+		     : &exfat_ci_dentry_ops;


…
> +failed_mount3:
> +	iput(root_inode);

I find the label “put_inode” more appropriate.


…
> +failed_mount2:
> +	exfat_free_upcase_table(sb);

I find the label “free_table” more helpful.


…
> +failed_mount:
> +	if (sbi->nls_io)
> +		unload_nls(sbi->nls_io);

Can the label “check_nls_io” be nicer?


…
> +static int __init init_exfat_fs(void)
> +{
…
> +shutdown_cache:
> +	exfat_cache_shutdown();
> +
> +	return err;

Would you like to omit blank lines at similar places?

Regards,
Markus

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

[Markus Elfring]

…
> +++ b/fs/exfat/fatent.c
…
> +int exfat_zeroed_cluster(struct inode *dir, unsigned int clu)
> +{
…
> +error:
> +	exfat_msg(sb, KERN_ERR, "failed zeroed sect %llu\n",
> +		(unsigned long long)blknr);
…

Can the label “report_failure” be more helpful?


…
> +int exfat_alloc_cluster(struct inode *inode, unsigned int num_alloc,
> +		struct exfat_chain *p_chain)
> +{
…
> +error:
> +	if (num_clusters)
> +		exfat_free_cluster(inode, p_chain);

Can a label like “check_num_clusters” be nicer?

Regards,
Markus

Re: [PATCH v3 08/13] exfat: add exfat cache

[Markus Elfring]

…
> +++ b/fs/exfat/cache.c
…
> +static void exfat_cache_add(struct inode *inode,
> +		struct exfat_cache_id *new)
> +{
…
> +out:
> +	spin_unlock(&ei->cache_lru_lock);
…

Can the label “unlock” be more helpful?

Regards,
Markus

Re: [PATCH v3 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)
> +{
…
> +error:
> +	if (bh)
> +		brelse(bh);

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/include/linux/buffer_head.h?id=af42d3466bdc8f39806b26f593604fdc54140bcb#n292
https://elixir.bootlin.com/linux/v5.4-rc8/source/include/linux/buffer_head.h#L292

Thus I suggest to omit the extra pointer check also at similar places.

Can the label “release_bh” be more helpful?

Regards,
Markus

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

[Namjae Jeon]

> …
> > +++ 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)
> > +{
> …
> > +error:
> > +	if (bh)
> > +		brelse(bh);
> 
> I am informed in the way that this function tolerates the passing
> of null pointers.
> https://protect2.fireeye.com/url?k=58476862-058969b1-5846e32d-000babff317b-
> 2bdcc1db1dc57528&u=https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/
> linux.git/tree/include/linux/buffer_head.h?id=af42d3466bdc8f39806b26f593604f
> dc54140bcb#n292
> https://protect2.fireeye.com/url?k=625424d5-3f9a2506-6255af9a-000babff317b-
> a544a35424b18c18&u=https://elixir.bootlin.com/linux/v5.4-
> rc8/source/include/linux/buffer_head.h#L292
> 
> Thus I suggest to omit the extra pointer check also at similar places.
> 
> Can the label “release_bh” be more helpful?
Hi Markus,

I checked not only review point but also your review points in
other patches, I will fix them on v4.

Thanks for your review!
> 
> Regards,
> Markus

Re: [PATCH v3 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)
>>> +{
>> …
>>> +error:
>>> +	if (bh)
>>> +		brelse(bh);
…
>> Can the label “release_bh” be more helpful?
…
> I checked not only review point but also your review points in
> other patches, I will fix them on v4.
…

Another software design alternative would be to use a jump target
like “free_table” instead at this source code place, wouldn't it?

Regards,
Markus

Re: [PATCH v3 01/13] exfat: add in-memory and on-disk structures and headers

[Markus Elfring]

…
> +++ b/fs/exfat/exfat_fs.h
…
> +/* balloc.c */
> +int exfat_load_bitmap(struct super_block *sb);
…

Such function declarations were grouped according to the mentioned
source file.
How do you think about to include them from separate header files
for the desired programming interface?

Regards,
Markus

Re: [PATCH v3 01/13] exfat: add in-memory and on-disk structures and headers

[Greg Kroah-Hartman]

On Wed, Nov 20, 2019 at 09:21:13PM +0100, Markus Elfring wrote:
> …
> > +++ b/fs/exfat/exfat_fs.h
> …
> > +/* balloc.c */
> > +int exfat_load_bitmap(struct super_block *sb);
> …
> 
> Such function declarations were grouped according to the mentioned
> source file.
> How do you think about to include them from separate header files
> for the desired programming interface?

No.

Re: [PATCH v3 07/13] exfat: add bitmap operations

[Markus Elfring]

…
> +++ b/fs/exfat/balloc.c
…
> +int exfat_load_bitmap(struct super_block *sb)
> +{
…
> +	struct exfat_dentry *ep = NULL;
…
> +	while (clu.dir != EOF_CLUSTER) {
> +		for (i = 0; i < sbi->dentries_per_clu; i++) {
> +			ep = exfat_get_dentry(sb, &clu, i, &bh, NULL);
> +			if (!ep)
> +				return -EIO;
…

How do you think about to move the definition for the variable “ep”
to the beginning of the for loop so that the extra pointer initialisation
can be omitted?

Regards,
Markus