On 7/1/24 9:15 AM, OGAWA Hirofumi wrote:
> 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.
Thanks!
>> +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.
ok
>> +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?
Ok - I think I just followed the lead of the ints in parse_options() today
but bool does make more sense.
>> +{
>> + 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.
whoops, you are right. thanks.
>> + /* 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?
Ah, that's probably smart. And I will switch this back to fat_msg() so that it's
logged in dmesg as it was before.
>> + default:
>> + return -EINVAL;
>
> I'm not sure though, "default:" should not happen anymore?
That's right. However, even conversions done by dhowells (for example
ec10a24f10c8 vfs: Convert jffs2 to use the new mount API) retain the
default: case. Happy to do this however you like, though keeping the default:
seems reasonably defensive?
>> }
>>
>> 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()?
In my conversions, I have treated parse_param as simply handling one option at
a time, and not dealing with combinations, because we don't have the "full view"
of all options until we are done (previously we parsed everything, and then could
"clean up" at the bottom of the function). So now, I was handling this sort of
checking after parsing was complete, and fill_super seemed an OK place to do it.
But sure - I will look at whether doing it in fat_parse_param makes sense.
>> + /* Apply pparsed options to sbi */
<typo here too>
>> + 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.
Ah, good catch.
>> - 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.
ok
>> +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.
*nod*
>> +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.
*nod*
>> +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.
*nod*
>> +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.
*nod* :)
Thanks for the review, and sorry for the errors.
-Eric
> Thanks.
