On Tue 13-11-12 15:40:22, Andrew Morton wrote:
> On Mon, 12 Nov 2012 23:27:28 +0100
> Jan Kara <jack@xxxxxxx> wrote:
>
> > So far FAT either offsets time stamps by sys_tz.minuteswest or leaves them
> > as they are (when tz=UTC mount option is used). However in some cases it
> > is useful if one can specify time stamp offset on his own (e.g. when time
> > zone of the camera connected is different from time zone of the computer,
> > or when HW clock is in UTC and thus sys_tz.minuteswest == 0).
> >
> > So provide a mount option time_offset= which allows user to specify offset in
> > minutes that should be applied to time stamps on the filesystem.
>
>
> Did you test "mount -o remount"?
>
> I suspect it won't work correctly - inodes which are already in
> cache at remount time will not reflect the updated offset?
>
> If so, a quick fix would be to disallow the ability to set time_offset
> via remount (dunno how?) and document it.
fat_remount() is actually:
static int fat_remount(struct super_block *sb, int *flags, char *data)
{
struct msdos_sb_info *sbi = MSDOS_SB(sb);
*flags |= MS_NODIRATIME | (sbi->options.isvfat ? 0 : MS_NOATIME);
return 0;
}
so all option changes are just ignored on remount. But I admit I checked
only now ;) So thanks for asking.
> > ...
> >
> > @@ -1005,8 +1011,17 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
> > case Opt_flush:
> > opts->flush = 1;
> > break;
> > + case Opt_time_offset:
> > + if (match_int(&args[0], &option))
> > + return 0;
> > + if (option < -12 * 60 || option > 12 * 60)
> > + return 0;
>
> Is it correct to return 0 here? 0 means "success"?
Right. I just copied it from other options without checking. Apparently
they are all wrong but logic in match_token() catches most of the faults so
noone noticed. So something like the attached patch?
>
> > + opts->tz_set = 1;
> > + opts->time_offset = option;
> > + break;
> > case Opt_tz_utc:
> > - opts->tz_utc = 1;
> > + opts->tz_set = 1;
> > + opts->time_offset = 0;
> > break;
> > case Opt_err_cont:
> > opts->errors = FAT_ERRORS_CONT;
> >
> > ...
> >
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
>From b1fb4805fc0a40ee924fbdbd1e4e1ea37f2e7456 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@xxxxxxx>
Date: Wed, 14 Nov 2012 01:57:03 +0100
Subject: [PATCH] fat: Fix mount option parsing
parse_options() is supposed to return value < 0 on error however we returned 0
(success) in a lot of cases. This actually was not a problem in practice
because match_token() used by parse_options() is clever and catches most of the
problems for us.
Signed-off-by: Jan Kara <jack@xxxxxxx>
---
fs/fat/inode.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index e3ef664..030bb1e 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -971,41 +971,41 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
break;
case Opt_uid:
if (match_int(&args[0], &option))
- return 0;
+ return -EINVAL;
opts->fs_uid = make_kuid(current_user_ns(), option);
if (!uid_valid(opts->fs_uid))
- return 0;
+ return -EINVAL;
break;
case Opt_gid:
if (match_int(&args[0], &option))
- return 0;
+ return -EINVAL;
opts->fs_gid = make_kgid(current_user_ns(), option);
if (!gid_valid(opts->fs_gid))
- return 0;
+ return -EINVAL;
break;
case Opt_umask:
if (match_octal(&args[0], &option))
- return 0;
+ return -EINVAL;
opts->fs_fmask = opts->fs_dmask = option;
break;
case Opt_dmask:
if (match_octal(&args[0], &option))
- return 0;
+ return -EINVAL;
opts->fs_dmask = option;
break;
case Opt_fmask:
if (match_octal(&args[0], &option))
- return 0;
+ return -EINVAL;
opts->fs_fmask = option;
break;
case Opt_allow_utime:
if (match_octal(&args[0], &option))
- return 0;
+ return -EINVAL;
opts->allow_utime = option & (S_IWGRP | S_IWOTH);
break;
case Opt_codepage:
if (match_int(&args[0], &option))
- return 0;
+ return -EINVAL;
opts->codepage = option;
break;
case Opt_flush:
@@ -1013,9 +1013,9 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
break;
case Opt_time_offset:
if (match_int(&args[0], &option))
- return 0;
+ return -EINVAL;
if (option < -12 * 60 || option > 12 * 60)
- return 0;
+ return -EINVAL;
opts->tz_set = 1;
opts->time_offset = option;
break;
--
1.7.1
