On 1/16/25 1:08 PM, Al Viro wrote:
> On Thu, Jan 16, 2025 at 12:49:32PM -0600, Eric Sandeen wrote:
>
>> + switch (opt) {
>> + case Opt_type:
>> + if (reconfigure &&
>> + (ctx->mount_options & UFS_MOUNT_UFSTYPE) != result.uint_32) {
>> + pr_err("ufstype can't be changed during remount\n");
>> + return -EINVAL;
>> }
>> + ufs_clear_opt(ctx->mount_options, UFS_MOUNT_UFSTYPE);
>> + ufs_set_opt(ctx->mount_options, result.uint_32);
>> + break;
>
> Do we really want to support ufstype=foo,ufstype=bar?
well, we already do that today. Old code was:
switch (token) {
case Opt_type_old:
ufs_clear_opt (*mount_options, UFSTYPE);
ufs_set_opt (*mount_options, UFSTYPE_OLD);
break;
case Opt_type_sunx86:
ufs_clear_opt (*mount_options, UFSTYPE);
ufs_set_opt (*mount_options, UFSTYPE_SUNx86);
break;
...
so I was going for a straight conversion for now so that the behavior
was exactly the same (i.e. keep the last-specified type. I know, it's
weird, who would do that? Still. Don't break userspace? And we've been
burned before.)
>
>> +static void ufs_free_fc(struct fs_context *fc)
>> +{
>> + kfree(fc->fs_private);
>> +}
>
> Grr... That's getting really annoying - we have way too many instances doing
> exactly that. Helper, perhaps?
Yeah ... should probably also decide if we should eliminate the default case
for every fs too, we should never hit default: barring programming errors.
And ISTR you asking for another helper a while ago... oh yeah, something like:
static inline bool fc_rdonly(const struct fs_context *fc)
{
return fc->sb_flags & SB_RDONLY;
}
so at some point maybe we should make a treewide pass on this stuff?
>> -#define ufs_set_opt(o,opt) o |= UFS_MOUNT_##opt
>> -#define ufs_test_opt(o,opt) ((o) & UFS_MOUNT_##opt)
>> +#define ufs_clear_opt(o, opt) (o &= ~(opt))
>> +#define ufs_set_opt(o, opt) (o |= (opt))
>> +#define ufs_test_opt(o, opt) ((o) & opt)
>
> I wonder if we would be better off without those macros (note, BTW,
> that ufs_test_opt() is not used at all)...
*shrug* I was just going for the minimal transformation w/o any
"also, while we're at it ..." bits. (tho I guess I did remove the
BUG_ON.)
If there are changes you want to see /now/ though I'm happy to do it.
Thanks,
-Eric
