Eric Sandeen <sandeen@xxxxxxxxxx> writes:
[...]
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxxxxxx>
