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

[Eric Sandeen <sandeen@redhat.com>]

On 7/1/24 12:35 PM, Eric Sandeen wrote:
> On 7/1/24 9:15 AM, OGAWA Hirofumi wrote:
>> Eric Sandeen <sandeen@redhat.com> writes:

[...]

>> [...]
>>
>>> +	/* 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.
> 

I don't think that will work.

For example, for the allow_utime adjustment...

Before parsing begins, allow_utime is defaulted to -1 (unset) and
fs_dmask is defaulted to current_umask()

If we put the 

+	if (opts->allow_utime == (unsigned short)-1)
+		opts->allow_utime = ~opts->fs_dmask & (S_IWGRP | S_IWOTH);

test at the bottom of parse_param, then this sequence of parsing:

("mount -o fs_uid=42,fs_dmask=0XYZ")

fs_uid=42
 --> sets opts->allow_utime to (~opts->fs_dmask & (S_IWGRP | S_IWOTH))
     where fs_dmask is default / current_umask()
fs_dmask=0XYZ
 --> changes fs_dmask from default, but does not update allow_utime which
     was set based on the old fs_dmask

leads to different results than:

("mount -o fs_dmask=0XYZ",fs_uid=42)

fs_dmask=0XYZ
 --> changes fs_dmask from the default
     updates allow_utime based on this user-specified fs_dmask rather than default
fs_uid=42
 --> allow_utime is now set, so no further changes are made

IOWS, the final allow_utime value may differ depending on the order of option
parsing, unless we wait until parsing is complete before we inspect and adjust it.

dhowells did, however, suggest that perhaps these adjustments should generally
be done in get_tree rather than fill_super, so I'll give that a shot.

Sound ok?

-Eric