[PATCH preview][] erofs-utils:introduce fixed timestamp using "-T"

[Gao Xiang]

Hi Guifu,

On Thu, Oct 10, 2019 at 12:43:36AM +0800, Li Guifu wrote:
> Introduce option "-T" for timestamp.
> 
> Signed-off-by: Li Guifu <blucerlee at gmail.com>

A few suggestions here...

It should be better marked the version such as "PATCH",
"PATCH v2", "PATCH vN" in the subject line
"[PATCH preview][] erofs-utils:introduce fixed timestamp using",
e.g.
[PATCH v2] erofs-utils: introduce fixed timestamp using

And the subject prefix is odd and no whitespace after semicolon
...
You could take care of it if you then do any sorts of kernel
development when you send out patches...

> ---
>  include/erofs/config.h |  3 +++
>  lib/config.c           |  1 +
>  mkfs/main.c            | 14 ++++++++++++--
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/erofs/config.h b/include/erofs/config.h
> index fde936c..61f5c71 100644
> --- a/include/erofs/config.h
> +++ b/include/erofs/config.h
> @@ -11,6 +11,8 @@
>  
>  #include "defs.h"
>  
> +#define EROFS_FIXED_TIMESTAMP 1569859200 // 2019/10/1 00:00:00
> +

It seems not very useful.. Could we just remove it?

>  enum {
>  	FORCE_INODE_COMPACT = 1,
>  	FORCE_INODE_EXTENDED,
> @@ -30,6 +32,7 @@ struct erofs_configure {
>  	int c_force_inodeversion;
>  	/* < 0, xattr disabled and INT_MAX, always use inline xattrs */
>  	int c_inline_xattr_tolerance;
> +	long long c_timestamp;
>  };
>  
>  extern struct erofs_configure cfg;
> diff --git a/lib/config.c b/lib/config.c
> index dc10754..6141497 100644
> --- a/lib/config.c
> +++ b/lib/config.c
> @@ -24,6 +24,7 @@ void erofs_init_configure(void)
>  	sbi.feature_incompat = EROFS_FEATURE_INCOMPAT_LZ4_0PADDING;
>  	cfg.c_force_inodeversion = 0;
>  	cfg.c_inline_xattr_tolerance = 2;
> +	cfg.c_timestamp = -1;
>  }
>  
>  void erofs_show_config(void)
> diff --git a/mkfs/main.c b/mkfs/main.c
> index 978c5b4..85b8f34 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -30,6 +30,7 @@ static void usage(void)
>  	fprintf(stderr, " -zX[,Y]   X=compressor (Y=compression level, optional)\n");
>  	fprintf(stderr, " -d#       set output message level to # (maximum 9)\n");
>  	fprintf(stderr, " -EX[,...] X=extended options\n");
> +	fprintf(stderr, " -T#       set a fixed timestamp to files and dirs\n");

fprintf(stderr, " -T#       set a fixed UNIX timestamp # to all files\n");

>  }
>  
>  static int parse_extended_opts(const char *opts)
> @@ -93,7 +94,7 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
>  {
>  	int opt, i;
>  
> -	while ((opt = getopt(argc, argv, "d:z:E:")) != -1) {
> +	while ((opt = getopt(argc, argv, "d:z:E:T::")) != -1) {

Can we avoid two colons since this is a GNU extension?
It seems we don't need an optional arg here.

>  		switch (opt) {
>  		case 'z':
>  			if (!optarg) {
> @@ -126,6 +127,12 @@ static int mkfs_parse_options_cfg(int argc, char *argv[])
>  			if (opt)
>  				return opt;
>  			break;
> +		case 'T':
> +			if (optarg)
> +				cfg.c_timestamp = strtoll(optarg, NULL, 10);
> +			else
> +				cfg.c_timestamp = EROFS_FIXED_TIMESTAMP;

no EROFS_FIXED_TIMESTAMP here, useless.

Thanks,
Gao Xiang

> +			break;
>  
>  		default: /* '?' */
>  			return -EINVAL;
> @@ -224,7 +231,10 @@ int main(int argc, char **argv)
>  		return 1;
>  	}
>  
> -	if (!gettimeofday(&t, NULL)) {
> +	if (cfg.c_timestamp != -1) {
> +		sbi.build_time      = cfg.c_timestamp;
> +		sbi.build_time_nsec = 0;
> +	} else if (!gettimeofday(&t, NULL)) {
>  		sbi.build_time      = t.tv_sec;
>  		sbi.build_time_nsec = t.tv_usec;
>  	}
> -- 
> 2.17.1
>