Re: [PATCH 2/2 V2] fat: Convert to new mount api

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Eric Sandeen <sandeen@redhat.com> writes:

[...]

> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> V2: Ignore all options during remount via
>
> 	if (fc->purpose == FS_CONTEXT_FOR_RECONFIGURE)
> 		return 0;

Thanks, basically looks good. However I tested a bit and found a bug,
and small comments.

> +extern const struct fs_parameter_spec fat_param_spec[];
> +extern int fat_init_fs_context(struct fs_context *fc, bool is_vfat);
> +extern void fat_free_fc(struct fs_context *fc);
> +
> +int fat_parse_param(struct fs_context *fc, struct fs_parameter *param,
> +		    int is_vfat);
> +extern int fat_reconfigure(struct fs_context *fc);

Let's remove extern from new one.

> +int fat_parse_param(struct fs_context *fc, struct fs_parameter *param,
> +			   int is_vfat)

Maybe better to use bool (and true/false) instead of int for is_vfat?

> +{
> +	struct fat_mount_options *opts = fc->fs_private;
> +	struct fs_parse_result result;
> +	int opt;
> +	char buf[50];

[...]	

> +	case Opt_codepage:
> +		sprintf(buf, "cp%d", result.uint_32);

"buf" is unused.

> +	/* obsolete mount options */
> +	case Opt_obsolete:
> +		infof(fc, "\"%s\" option is obsolete, not supported now",
> +		      param->key);
> +		break;

I'm not sure though, "Opt_obsolete" should use fs_param_deprecated?

> +	default:
> +		return -EINVAL;

I'm not sure though, "default:" should not happen anymore?

>  	}
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(fat_parse_param);

[...]

> +	/* If user doesn't specify allow_utime, it's initialized from dmask. */
> +	if (opts->allow_utime == (unsigned short)-1)
> +		opts->allow_utime = ~opts->fs_dmask & (S_IWGRP | S_IWOTH);
> +	if (opts->unicode_xlate)
> +		opts->utf8 = 0;

Probably, this should move to fat_parse_param()?

> +	/* Apply pparsed options to sbi */
> +	sbi->options = *opts;

	/* Transfer ownership of iocharset to sbi->options */
	opts->iocharset = NULL;
	opts = &sbi->options;

opts->iocharset is freed by both of opts and sbi->options, we should fix
it like above or such.

> -	sprintf(buf, "cp%d", sbi->options.codepage);
> +	sprintf(buf, "cp%d", opts->codepage);

[...]

>  	/* FIXME: utf8 is using iocharset for upper/lower conversion */
>  	if (sbi->options.isvfat) {
> -		sbi->nls_io = load_nls(sbi->options.iocharset);
> +		sbi->nls_io = load_nls(opts->iocharset);
>  		if (!sbi->nls_io) {
>  			fat_msg(sb, KERN_ERR, "IO charset %s not found",
> -			       sbi->options.iocharset);
> +			       opts->iocharset);
>  			goto out_fail;

Revert above to remove opts usage to not touch after ownership transfer
if we fix the bug like that way.

> +static int msdos_parse_param(struct fs_context *fc, struct fs_parameter *param)
> +{
> +	return fat_parse_param(fc, param, 0);

If we changed int to bool, 0 to false.

> +static int msdos_init_fs_context(struct fs_context *fc)
> +{
> +	int err;
> +
> +	/* Initialize with isvfat == 0 */
> +	err = fat_init_fs_context(fc, 0);

If we changed int to bool, 0 to false.

> +static int vfat_parse_param(struct fs_context *fc, struct fs_parameter *param)
> +{
> +	return fat_parse_param(fc, param, 1);

If we changed int to bool, 0 to true.

> +static int vfat_init_fs_context(struct fs_context *fc)
> +{
> +	int err;
> +
> +	/* Initialize with isvfat == 1 */
> +	err = fat_init_fs_context(fc, 1);

If we changed int to bool, 0 to true.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>