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

[Jan Kara <jack@suse.cz>]

On Tue 13-11-12 17:11:17, Andrew Morton wrote:
> On Wed, 14 Nov 2012 02:04:02 +0100
> Jan Kara <jack@suse.cz> wrote:
> 
> > 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.
> 
> Silently ignored?  That's a bit nasty.
  Yes. But people apparently don't tend to remount FAT...

> > > > @@ -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? :)
  Umm, emm... not at all at that moment I'm afraid. Now I've tried and it
works as expected ;).

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR