From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Morton Subject: Re: [PATCH v2] fat: Provide option for setting timezone offset Date: Tue, 13 Nov 2012 17:11:17 -0800 Message-ID: <20121113171117.79ddefa7.akpm@linux-foundation.org> References: <1352759248-9447-1-git-send-email-jack@suse.cz> <20121113154022.4bd68ecd.akpm@linux-foundation.org> <20121114010402.GA30196@quack.suse.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: OGAWA Hirofumi , linux-fsdevel@vger.kernel.org To: Jan Kara Return-path: Received: from mail.linuxfoundation.org ([140.211.169.12]:57406 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932161Ab2KNBLS (ORCPT ); Tue, 13 Nov 2012 20:11:18 -0500 In-Reply-To: <20121114010402.GA30196@quack.suse.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, 14 Nov 2012 02:04:02 +0100 Jan Kara wrote: > 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. Silently ignored? That's a bit nasty. > > > @@ -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? How well tested was that patch? :)