Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 29 Jan 2018 15:18:42 +0200 | Subject | Re: [PATCH 3/3] fs: fat: add ioctl method in fat filesystem driver |
| |
+Cc: Pali, who AFAIRC is interested in FAT labeling mess.
On Wed, Jan 17, 2018 at 12:43 PM, ChenGuanqiao <chen.chenchacha@foxmail.com> wrote:
Commit message?
> Signed-off-by: ChenGuanqiao <chen.chenchacha@foxmail.com>
> #include <linux/fsnotify.h> > #include <linux/security.h> > #include <linux/falloc.h> > +#include <linux/ctype.h>
It would be better to squeeze it to have order (to some extent) preserved.
> +static int fat_check_d_characters(char *label, unsigned long len) > +{ > + int i; > + > + for (i = 0; i < len; ++i) {
Oy vey, unsigned long len vs. int i.
> + switch (label[i]) { > + case 'a' ... 'z': > + label[i] = __toupper(label[i]); > + case 'A' ... 'Z': > + case '0' ... '9': > + case '_': > + case 0x20: > + continue;
I'm not sure this is best approach. And since there is no commit message I can't be constructive.
> + default: > + return -EINVAL; > + } > + } > + > + return 0; > +}
> +static int fat_ioctl_get_volume_label(struct inode *inode, > + u8 __user *vol_label) > +{
> + int err = 0;
Redundant assignment.
> + struct buffer_head *bh; > + struct msdos_dir_entry *de; > + struct super_block *sb = inode->i_sb;
Moreover, perhaps reversed tree order for the definition block?
> + > + inode = d_inode(sb->s_root); > + inode_lock_shared(inode); > + > + err = fat_get_volume_label_entry(inode, &bh, &de); > + if (err) > + goto out; > + > + if (copy_to_user(vol_label, de->name, MSDOS_NAME)) > + err = -EFAULT; > + > + brelse(bh); > +out: > + inode_unlock_shared(inode); > + > + return err; > +} > +
> +static int fat_ioctl_set_volume_label(struct file *file, > + u8 __user *vol_label)
Perhaps vol_label -> label, and fit one line?
> +{ > + int err = 0; > + u8 label[MSDOS_NAME]; > + struct timespec ts; > + struct buffer_head *boot_bh; > + struct buffer_head *vol_bh; > + struct msdos_dir_entry *de; > + struct fat_boot_sector *b; > + struct inode *inode = file_inode(file); > + struct super_block *sb = inode->i_sb; > + struct msdos_sb_info *sbi = MSDOS_SB(sb); > + > + inode = d_inode(sb->s_root); > + > + if (copy_from_user(label, vol_label, sizeof(label))) { > + err = -EFAULT; > + goto out; > + } > + > + err = fat_check_d_characters(label, sizeof(label)); > + if (err) > + goto out; > + > + err = mnt_want_write_file(file); > + if (err) > + goto out; > + > + down_write(&sb->s_umount); > + > + /* Updates root directory's vol_label */ > + err = fat_get_volume_label_entry(inode, &vol_bh, &de); > + ts = current_time(inode); > + if (err) { > + /* Create volume label entry */ > + err = fat_add_volume_label_entry(inode, label, &ts); > + if (err) > + goto out_vol_brelse; > + } else { > + /* Write to root directory */ > + memcpy(de->name, label, sizeof(de->name)); > + inode->i_ctime = inode->i_mtime = inode->i_atime = ts; > + > + mark_buffer_dirty(vol_bh); > + } > + > + /* Update sector's vol_label */ > + boot_bh = sb_bread(sb, 0); > + if (boot_bh == NULL) { > + fat_msg(sb, KERN_ERR, > + "unable to read boot sector to write volume label"); > + err = -EIO; > + goto out_boot_brelse; > + } > + > + b = (struct fat_boot_sector *)boot_bh->b_data; > + if (sbi->fat_bits == 32) > + memcpy(b->fat32.vol_label, label, sizeof(label)); > + else > + memcpy(b->fat16.vol_label, label, sizeof(label)); > + > + mark_buffer_dirty(boot_bh); > + > + /* Synchronize the data together */ > + err = sync_dirty_buffer(boot_bh); > + if (err) > + goto out_boot_brelse; > + > +out_boot_brelse: > + brelse(boot_bh); > +out_vol_brelse: > + brelse(vol_bh); > + > + up_write(&sb->s_umount); > + mnt_drop_write_file(file); > +out: > + return err; > +}
-- With Best Regards, Andy Shevchenko
| |