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

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

[Jan Kara]

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.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 Documentation/filesystems/vfat.txt |    9 +++++++++
 fs/fat/fat.h                       |    3 ++-
 fs/fat/inode.c                     |   25 ++++++++++++++++++++-----
 fs/fat/misc.c                      |    9 ++++++---
 4 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/Documentation/filesystems/vfat.txt b/Documentation/filesystems/vfat.txt
index de1e6c4..d230dd9 100644
--- a/Documentation/filesystems/vfat.txt
+++ b/Documentation/filesystems/vfat.txt
@@ -111,6 +111,15 @@ tz=UTC        -- Interpret timestamps as UTC rather than local time.
                  useful when mounting devices (like digital cameras)
                  that are set to UTC in order to avoid the pitfalls of
                  local time.
+time_offset=minutes
+	      -- Set offset for conversion of timestamps from local time
+		 used by FAT to UTC. I.e. <minutes> minutes will be subtracted
+		 from each timestamp to convert it to UTC used internally by
+		 Linux. This is useful when time zone set in sys_tz is
+		 not the time zone used by the filesystem. Note that this
+		 option still does not provide correct time stamps in all
+		 cases in presence of DST - time stamps in a different DST
+		 setting will be off by one hour.
 
 showexec      -- If set, the execute permission bits of the file will be
 		 allowed only if the extension part of the name is .EXE,
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 623f36f..12701a5 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -29,6 +29,7 @@ struct fat_mount_options {
 	unsigned short fs_fmask;
 	unsigned short fs_dmask;
 	unsigned short codepage;   /* Codepage for shortname conversions */
+	int time_offset;	   /* Offset of timestamps from UTC (in minutes) */
 	char *iocharset;           /* Charset used for filename input/display */
 	unsigned short shortname;  /* flags for shortname display/create rule */
 	unsigned char name_check;  /* r = relaxed, n = normal, s = strict */
@@ -45,7 +46,7 @@ struct fat_mount_options {
 		 flush:1,	   /* write things quickly */
 		 nocase:1,	   /* Does this need case conversion? 0=need case conversion*/
 		 usefree:1,	   /* Use free_clusters for FAT32 */
-		 tz_utc:1,	   /* Filesystem timestamps are in UTC */
+		 tz_set:1,	   /* Filesystem timestamps' offset set */
 		 rodir:1,	   /* allow ATTR_RO for directory */
 		 discard:1,	   /* Issue discard requests on deletions */
 		 nfs:1;		   /* Do extra work needed for NFS export */
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 5bafaad..e3ef664 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -777,8 +777,12 @@ static int fat_show_options(struct seq_file *m, struct dentry *root)
 	}
 	if (opts->flush)
 		seq_puts(m, ",flush");
-	if (opts->tz_utc)
-		seq_puts(m, ",tz=UTC");
+	if (opts->tz_set) {
+		if (opts->time_offset)
+			seq_printf(m, ",time_offset=%d", opts->time_offset);
+		else
+			seq_puts(m, ",tz=UTC");
+	}
 	if (opts->errors == FAT_ERRORS_CONT)
 		seq_puts(m, ",errors=continue");
 	else if (opts->errors == FAT_ERRORS_PANIC)
@@ -800,7 +804,8 @@ enum {
 	Opt_shortname_winnt, Opt_shortname_mixed, Opt_utf8_no, Opt_utf8_yes,
 	Opt_uni_xl_no, Opt_uni_xl_yes, Opt_nonumtail_no, Opt_nonumtail_yes,
 	Opt_obsolete, Opt_flush, Opt_tz_utc, Opt_rodir, Opt_err_cont,
-	Opt_err_panic, Opt_err_ro, Opt_discard, Opt_nfs, Opt_err,
+	Opt_err_panic, Opt_err_ro, Opt_discard, Opt_nfs, Opt_time_offset,
+	Opt_err,
 };
 
 static const match_table_t fat_tokens = {
@@ -825,6 +830,7 @@ static const match_table_t fat_tokens = {
 	{Opt_immutable, "sys_immutable"},
 	{Opt_flush, "flush"},
 	{Opt_tz_utc, "tz=UTC"},
+	{Opt_time_offset, "time_offset=%d"},
 	{Opt_err_cont, "errors=continue"},
 	{Opt_err_panic, "errors=panic"},
 	{Opt_err_ro, "errors=remount-ro"},
@@ -909,7 +915,7 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
 	opts->utf8 = opts->unicode_xlate = 0;
 	opts->numtail = 1;
 	opts->usefree = opts->nocase = 0;
-	opts->tz_utc = 0;
+	opts->tz_set = 0;
 	opts->nfs = 0;
 	opts->errors = FAT_ERRORS_RO;
 	*debug = 0;
@@ -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;
+			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;
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index 6d93360..5eb600d 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -212,8 +212,10 @@ void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts,
 		   + days_in_year[month] + day
 		   + DAYS_DELTA) * SECS_PER_DAY;
 
-	if (!sbi->options.tz_utc)
+	if (!sbi->options.tz_set)
 		second += sys_tz.tz_minuteswest * SECS_PER_MIN;
+	else
+		second -= sbi->options.time_offset * SECS_PER_MIN;
 
 	if (time_cs) {
 		ts->tv_sec = second + (time_cs / 100);
@@ -229,8 +231,9 @@ void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec *ts,
 		       __le16 *time, __le16 *date, u8 *time_cs)
 {
 	struct tm tm;
-	time_to_tm(ts->tv_sec, sbi->options.tz_utc ? 0 :
-		   -sys_tz.tz_minuteswest * 60, &tm);
+	time_to_tm(ts->tv_sec,
+		   (sbi->options.tz_set ? sbi->options.time_offset :
+		   -sys_tz.tz_minuteswest) * SECS_PER_MIN, &tm);
 
 	/*  FAT can only support year between 1980 to 2107 */
 	if (tm.tm_year < 1980 - 1900) {
-- 
1.7.1

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

[OGAWA Hirofumi]

Jan Kara <jack@suse.cz> writes:

> 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.

Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  Documentation/filesystems/vfat.txt |    9 +++++++++
>  fs/fat/fat.h                       |    3 ++-
>  fs/fat/inode.c                     |   25 ++++++++++++++++++++-----
>  fs/fat/misc.c                      |    9 ++++++---
>  4 files changed, 37 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/filesystems/vfat.txt b/Documentation/filesystems/vfat.txt
> index de1e6c4..d230dd9 100644
> --- a/Documentation/filesystems/vfat.txt
> +++ b/Documentation/filesystems/vfat.txt
> @@ -111,6 +111,15 @@ tz=UTC        -- Interpret timestamps as UTC rather than local time.
>                   useful when mounting devices (like digital cameras)
>                   that are set to UTC in order to avoid the pitfalls of
>                   local time.
> +time_offset=minutes
> +	      -- Set offset for conversion of timestamps from local time
> +		 used by FAT to UTC. I.e. <minutes> minutes will be subtracted
> +		 from each timestamp to convert it to UTC used internally by
> +		 Linux. This is useful when time zone set in sys_tz is
> +		 not the time zone used by the filesystem. Note that this
> +		 option still does not provide correct time stamps in all
> +		 cases in presence of DST - time stamps in a different DST
> +		 setting will be off by one hour.
>  
>  showexec      -- If set, the execute permission bits of the file will be
>  		 allowed only if the extension part of the name is .EXE,
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index 623f36f..12701a5 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -29,6 +29,7 @@ struct fat_mount_options {
>  	unsigned short fs_fmask;
>  	unsigned short fs_dmask;
>  	unsigned short codepage;   /* Codepage for shortname conversions */
> +	int time_offset;	   /* Offset of timestamps from UTC (in minutes) */
>  	char *iocharset;           /* Charset used for filename input/display */
>  	unsigned short shortname;  /* flags for shortname display/create rule */
>  	unsigned char name_check;  /* r = relaxed, n = normal, s = strict */
> @@ -45,7 +46,7 @@ struct fat_mount_options {
>  		 flush:1,	   /* write things quickly */
>  		 nocase:1,	   /* Does this need case conversion? 0=need case conversion*/
>  		 usefree:1,	   /* Use free_clusters for FAT32 */
> -		 tz_utc:1,	   /* Filesystem timestamps are in UTC */
> +		 tz_set:1,	   /* Filesystem timestamps' offset set */
>  		 rodir:1,	   /* allow ATTR_RO for directory */
>  		 discard:1,	   /* Issue discard requests on deletions */
>  		 nfs:1;		   /* Do extra work needed for NFS export */
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index 5bafaad..e3ef664 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -777,8 +777,12 @@ static int fat_show_options(struct seq_file *m, struct dentry *root)
>  	}
>  	if (opts->flush)
>  		seq_puts(m, ",flush");
> -	if (opts->tz_utc)
> -		seq_puts(m, ",tz=UTC");
> +	if (opts->tz_set) {
> +		if (opts->time_offset)
> +			seq_printf(m, ",time_offset=%d", opts->time_offset);
> +		else
> +			seq_puts(m, ",tz=UTC");
> +	}
>  	if (opts->errors == FAT_ERRORS_CONT)
>  		seq_puts(m, ",errors=continue");
>  	else if (opts->errors == FAT_ERRORS_PANIC)
> @@ -800,7 +804,8 @@ enum {
>  	Opt_shortname_winnt, Opt_shortname_mixed, Opt_utf8_no, Opt_utf8_yes,
>  	Opt_uni_xl_no, Opt_uni_xl_yes, Opt_nonumtail_no, Opt_nonumtail_yes,
>  	Opt_obsolete, Opt_flush, Opt_tz_utc, Opt_rodir, Opt_err_cont,
> -	Opt_err_panic, Opt_err_ro, Opt_discard, Opt_nfs, Opt_err,
> +	Opt_err_panic, Opt_err_ro, Opt_discard, Opt_nfs, Opt_time_offset,
> +	Opt_err,
>  };
>  
>  static const match_table_t fat_tokens = {
> @@ -825,6 +830,7 @@ static const match_table_t fat_tokens = {
>  	{Opt_immutable, "sys_immutable"},
>  	{Opt_flush, "flush"},
>  	{Opt_tz_utc, "tz=UTC"},
> +	{Opt_time_offset, "time_offset=%d"},
>  	{Opt_err_cont, "errors=continue"},
>  	{Opt_err_panic, "errors=panic"},
>  	{Opt_err_ro, "errors=remount-ro"},
> @@ -909,7 +915,7 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
>  	opts->utf8 = opts->unicode_xlate = 0;
>  	opts->numtail = 1;
>  	opts->usefree = opts->nocase = 0;
> -	opts->tz_utc = 0;
> +	opts->tz_set = 0;
>  	opts->nfs = 0;
>  	opts->errors = FAT_ERRORS_RO;
>  	*debug = 0;
> @@ -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;
> +			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;
> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index 6d93360..5eb600d 100644
> --- a/fs/fat/misc.c
> +++ b/fs/fat/misc.c
> @@ -212,8 +212,10 @@ void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts,
>  		   + days_in_year[month] + day
>  		   + DAYS_DELTA) * SECS_PER_DAY;
>  
> -	if (!sbi->options.tz_utc)
> +	if (!sbi->options.tz_set)
>  		second += sys_tz.tz_minuteswest * SECS_PER_MIN;
> +	else
> +		second -= sbi->options.time_offset * SECS_PER_MIN;
>  
>  	if (time_cs) {
>  		ts->tv_sec = second + (time_cs / 100);
> @@ -229,8 +231,9 @@ void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec *ts,
>  		       __le16 *time, __le16 *date, u8 *time_cs)
>  {
>  	struct tm tm;
> -	time_to_tm(ts->tv_sec, sbi->options.tz_utc ? 0 :
> -		   -sys_tz.tz_minuteswest * 60, &tm);
> +	time_to_tm(ts->tv_sec,
> +		   (sbi->options.tz_set ? sbi->options.time_offset :
> +		   -sys_tz.tz_minuteswest) * SECS_PER_MIN, &tm);
>  
>  	/*  FAT can only support year between 1980 to 2107 */
>  	if (tm.tm_year < 1980 - 1900) {

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

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

[Jan Kara]

On Tue 13-11-12 11:23:29, OGAWA Hirofumi wrote:
> Jan Kara <jack@suse.cz> writes:
> 
> > 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.
> 
> Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
  Thanks! Since you CCed Andrew, I presume you want him to pick up the
patch and push it to Linus?

								Honza

> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  Documentation/filesystems/vfat.txt |    9 +++++++++
> >  fs/fat/fat.h                       |    3 ++-
> >  fs/fat/inode.c                     |   25 ++++++++++++++++++++-----
> >  fs/fat/misc.c                      |    9 ++++++---
> >  4 files changed, 37 insertions(+), 9 deletions(-)
> >
> > diff --git a/Documentation/filesystems/vfat.txt b/Documentation/filesystems/vfat.txt
> > index de1e6c4..d230dd9 100644
> > --- a/Documentation/filesystems/vfat.txt
> > +++ b/Documentation/filesystems/vfat.txt
> > @@ -111,6 +111,15 @@ tz=UTC        -- Interpret timestamps as UTC rather than local time.
> >                   useful when mounting devices (like digital cameras)
> >                   that are set to UTC in order to avoid the pitfalls of
> >                   local time.
> > +time_offset=minutes
> > +	      -- Set offset for conversion of timestamps from local time
> > +		 used by FAT to UTC. I.e. <minutes> minutes will be subtracted
> > +		 from each timestamp to convert it to UTC used internally by
> > +		 Linux. This is useful when time zone set in sys_tz is
> > +		 not the time zone used by the filesystem. Note that this
> > +		 option still does not provide correct time stamps in all
> > +		 cases in presence of DST - time stamps in a different DST
> > +		 setting will be off by one hour.
> >  
> >  showexec      -- If set, the execute permission bits of the file will be
> >  		 allowed only if the extension part of the name is .EXE,
> > diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> > index 623f36f..12701a5 100644
> > --- a/fs/fat/fat.h
> > +++ b/fs/fat/fat.h
> > @@ -29,6 +29,7 @@ struct fat_mount_options {
> >  	unsigned short fs_fmask;
> >  	unsigned short fs_dmask;
> >  	unsigned short codepage;   /* Codepage for shortname conversions */
> > +	int time_offset;	   /* Offset of timestamps from UTC (in minutes) */
> >  	char *iocharset;           /* Charset used for filename input/display */
> >  	unsigned short shortname;  /* flags for shortname display/create rule */
> >  	unsigned char name_check;  /* r = relaxed, n = normal, s = strict */
> > @@ -45,7 +46,7 @@ struct fat_mount_options {
> >  		 flush:1,	   /* write things quickly */
> >  		 nocase:1,	   /* Does this need case conversion? 0=need case conversion*/
> >  		 usefree:1,	   /* Use free_clusters for FAT32 */
> > -		 tz_utc:1,	   /* Filesystem timestamps are in UTC */
> > +		 tz_set:1,	   /* Filesystem timestamps' offset set */
> >  		 rodir:1,	   /* allow ATTR_RO for directory */
> >  		 discard:1,	   /* Issue discard requests on deletions */
> >  		 nfs:1;		   /* Do extra work needed for NFS export */
> > diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> > index 5bafaad..e3ef664 100644
> > --- a/fs/fat/inode.c
> > +++ b/fs/fat/inode.c
> > @@ -777,8 +777,12 @@ static int fat_show_options(struct seq_file *m, struct dentry *root)
> >  	}
> >  	if (opts->flush)
> >  		seq_puts(m, ",flush");
> > -	if (opts->tz_utc)
> > -		seq_puts(m, ",tz=UTC");
> > +	if (opts->tz_set) {
> > +		if (opts->time_offset)
> > +			seq_printf(m, ",time_offset=%d", opts->time_offset);
> > +		else
> > +			seq_puts(m, ",tz=UTC");
> > +	}
> >  	if (opts->errors == FAT_ERRORS_CONT)
> >  		seq_puts(m, ",errors=continue");
> >  	else if (opts->errors == FAT_ERRORS_PANIC)
> > @@ -800,7 +804,8 @@ enum {
> >  	Opt_shortname_winnt, Opt_shortname_mixed, Opt_utf8_no, Opt_utf8_yes,
> >  	Opt_uni_xl_no, Opt_uni_xl_yes, Opt_nonumtail_no, Opt_nonumtail_yes,
> >  	Opt_obsolete, Opt_flush, Opt_tz_utc, Opt_rodir, Opt_err_cont,
> > -	Opt_err_panic, Opt_err_ro, Opt_discard, Opt_nfs, Opt_err,
> > +	Opt_err_panic, Opt_err_ro, Opt_discard, Opt_nfs, Opt_time_offset,
> > +	Opt_err,
> >  };
> >  
> >  static const match_table_t fat_tokens = {
> > @@ -825,6 +830,7 @@ static const match_table_t fat_tokens = {
> >  	{Opt_immutable, "sys_immutable"},
> >  	{Opt_flush, "flush"},
> >  	{Opt_tz_utc, "tz=UTC"},
> > +	{Opt_time_offset, "time_offset=%d"},
> >  	{Opt_err_cont, "errors=continue"},
> >  	{Opt_err_panic, "errors=panic"},
> >  	{Opt_err_ro, "errors=remount-ro"},
> > @@ -909,7 +915,7 @@ static int parse_options(struct super_block *sb, char *options, int is_vfat,
> >  	opts->utf8 = opts->unicode_xlate = 0;
> >  	opts->numtail = 1;
> >  	opts->usefree = opts->nocase = 0;
> > -	opts->tz_utc = 0;
> > +	opts->tz_set = 0;
> >  	opts->nfs = 0;
> >  	opts->errors = FAT_ERRORS_RO;
> >  	*debug = 0;
> > @@ -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;
> > +			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;
> > diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> > index 6d93360..5eb600d 100644
> > --- a/fs/fat/misc.c
> > +++ b/fs/fat/misc.c
> > @@ -212,8 +212,10 @@ void fat_time_fat2unix(struct msdos_sb_info *sbi, struct timespec *ts,
> >  		   + days_in_year[month] + day
> >  		   + DAYS_DELTA) * SECS_PER_DAY;
> >  
> > -	if (!sbi->options.tz_utc)
> > +	if (!sbi->options.tz_set)
> >  		second += sys_tz.tz_minuteswest * SECS_PER_MIN;
> > +	else
> > +		second -= sbi->options.time_offset * SECS_PER_MIN;
> >  
> >  	if (time_cs) {
> >  		ts->tv_sec = second + (time_cs / 100);
> > @@ -229,8 +231,9 @@ void fat_time_unix2fat(struct msdos_sb_info *sbi, struct timespec *ts,
> >  		       __le16 *time, __le16 *date, u8 *time_cs)
> >  {
> >  	struct tm tm;
> > -	time_to_tm(ts->tv_sec, sbi->options.tz_utc ? 0 :
> > -		   -sys_tz.tz_minuteswest * 60, &tm);
> > +	time_to_tm(ts->tv_sec,
> > +		   (sbi->options.tz_set ? sbi->options.time_offset :
> > +		   -sys_tz.tz_minuteswest) * SECS_PER_MIN, &tm);
> >  
> >  	/*  FAT can only support year between 1980 to 2107 */
> >  	if (tm.tm_year < 1980 - 1900) {
> 
> -- 
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

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

[OGAWA Hirofumi]

Jan Kara <jack@suse.cz> writes:

> On Tue 13-11-12 11:23:29, OGAWA Hirofumi wrote:
>> Jan Kara <jack@suse.cz> writes:
>> 
>> > 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.
>> 
>> Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>   Thanks! Since you CCed Andrew, I presume you want him to pick up the
> patch and push it to Linus?

Yes. I don't have FAT repo after kernel.org break-in.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

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

[Andrew Morton]

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.

>
> ...
>
> @@ -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"?

> +			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;
>
> ...
>

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

[Jan Kara]

[-- 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

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

[Andrew Morton]

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.

> > > @@ -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? :)

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

[Jan Kara]

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