From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH v2] fat: Provide option for setting timezone offset Date: Wed, 14 Nov 2012 02:04:02 +0100 Message-ID: <20121114010402.GA30196@quack.suse.cz> References: <1352759248-9447-1-git-send-email-jack@suse.cz> <20121113154022.4bd68ecd.akpm@linux-foundation.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="d6Gm4EdcadzBjdND" Cc: Jan Kara , OGAWA Hirofumi , linux-fsdevel@vger.kernel.org To: Andrew Morton Return-path: Received: from cantor2.suse.de ([195.135.220.15]:52811 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756052Ab2KNBEH (ORCPT ); Tue, 13 Nov 2012 20:04:07 -0500 Content-Disposition: inline In-Reply-To: <20121113154022.4bd68ecd.akpm@linux-foundation.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: --d6Gm4EdcadzBjdND Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue 13-11-12 15:40:22, Andrew Morton wrote: > On Mon, 12 Nov 2012 23:27:28 +0100 > Jan Kara 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 SUSE Labs, CR --d6Gm4EdcadzBjdND Content-Type: text/x-patch; charset=us-ascii Content-Disposition: attachment; filename="0001-fat-Fix-mount-option-parsing.patch" >>From b1fb4805fc0a40ee924fbdbd1e4e1ea37f2e7456 Mon Sep 17 00:00:00 2001 From: Jan Kara 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 --- 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 --d6Gm4EdcadzBjdND--