Re: [PATCH v2 2/2] fat: Add FS_IOC_SETFSLABEL ioctl

[OGAWA Hirofumi]

Ethan Ferguson <ethan.ferguson@xxxxxxxxxx> writes:

>> I didn't check how to know the label though, the label is only if
>> ATTR_VOLUME? IOW, any other attributes are disallowed?
> I'm pretty sure ATTR_VOLUME is disallowed except for:
> * Volume labels, where it is the only flag present
> * Long file name entries, where it is /not/ the only flag present
> This is why I check if attr == ATTR_VOLUME, not attr & ATTR_VOLUME

What happen on Windows if ATTR_VOLUME | ATTR_ARCH, for example? There
are many strange behavior tools for FAT.

>> What if label is marked as deleted?
> As far as I know, a Volume label can never be marked as deleted, but if you want me to change the behavior of my patch, just let me know how you would like me to handle it and I'd be happy to change it.

That state is easily happen too, I'd like to emulate Windows behavior if
possible.

>> I'm not sure though, no need to update timestamps? (need to investigate
>> spec or windows behavior)
> It's not in the spec that I know either, I'm happy to remove if you deem this unnecessary.

Rather I'd like to know if Windows updates it or not.

>>> +static int fat_convert_volume_label_str(struct msdos_sb_info *sbi, char *in,
>>> +                    char *out)
>>> +{
>>> +    int ret, in_len = max(strnlen(in, FSLABEL_MAX), 11);
>>> +    char *needle;
>> 
>> Silently truncate is the common way for this ioctl?
> When I implemented this in exfat, I returned -EINVAL for names that were longer than allowed, but only after converting from nls to UTF16. I can copy this behavior here as well.

I think we should avoid userspace adds workaround for per fs
behavior. So if possible, ioctl should behave same behavior or meaning
of error code.

>>> +    ret = msdos_format_name(in, in_len, out, &sbi->options);
>>> +    if (ret)
>>> +        return ret;
>> 
>>> +    /*
>>> +     * msdos_format_name assumes we're translating an 8.3 name, but
>>> +     * we can handle 11 chars
>>> +     */
>>> +    if (in_len > 8)
>>> +        ret = msdos_format_name(in + 8, in_len - 8, out + 8,
>>> +                    &sbi->options);
>>> +    return ret;
>> 
>> fat module should not import msdos module.
> Fair. How would you implement checking the validity of the new volume label?

For example, move label verifier to fat module instead, and export to
msdos module if required.

>> This rename will have to take same or similar locks with rename(2)?
> Sure, so should I only lock on sbi->s_lock through the whole function?

Hm, I expected to take same locking with vfs what does for rename(2)
syscall path. Otherwise, this would be able to race with normal dir rename.

>>> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
>>> index 6f9a8cc1ad2a..a7528937383b 100644
>>> --- a/fs/fat/inode.c
>>> +++ b/fs/fat/inode.c
>>> @@ -736,6 +736,21 @@ static void delayed_free(struct rcu_head *p)
>>> static void fat_put_super(struct super_block *sb)
>>> {
>>>  struct msdos_sb_info *sbi = MSDOS_SB(sb);
>>> +    struct buffer_head *bh = NULL;
>>> +    struct fat_boot_sector *bs;
>>> +
>>> +    bh = sb_bread(sb, 0);
>>> +    if (bh == NULL)
>>> +        fat_msg(sb, KERN_ERR, "unable to read boot sector");
>>> +    else if (!sb_rdonly(sb)) {
>>> +        bs = (struct fat_boot_sector *)bh->b_data;
>>> +        if (is_fat32(sbi))
>>> +            memcpy(bs->fat32.vol_label, sbi->vol_label, MSDOS_NAME);
>>> +        else
>>> +            memcpy(bs->fat16.vol_label, sbi->vol_label, MSDOS_NAME);
>>> +        mark_buffer_dirty(bh);
>>> +    }
>>> +    brelse(bh);
>> 
>> Why this unconditionally update the vol_label at unmount?
> I can add a dirty bit to msdos_sb_info, and only write if it's present.

And why this update at unmount? Looks like it is natural to update like
normal rename.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@xxxxxxxxxxxxxxxxxx>