Re: [PATCH v2] fat: Provide option for setting timezone offset

[Jan Kara <jack@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]

On Tue 13-11-12 15:40:22, Andrew Morton wrote:
> On Mon, 12 Nov 2012 23:27:28 +0100
> Jan Kara <jack@suse.cz> 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@suse.cz>
SUSE Labs, CR

[-- Attachment #2: 0001-fat-Fix-mount-option-parsing.patch --]
[-- Type: text/x-patch, Size: 2376 bytes --]

>From b1fb4805fc0a40ee924fbdbd1e4e1ea37f2e7456 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
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@suse.cz>
---
 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