On Wed, 7 May 2014, Theodore Ts'o wrote:
> Date: Wed, 7 May 2014 08:39:13 -0400
> From: Theodore Ts'o <tytso@xxxxxxx>
> To: Lukáš Czerner <lczerner@xxxxxxxxxx>
> Cc: Ext4 Developers List <linux-ext4@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH -v2 2/3] mke2fs: print extra information about existing
> ext2/3/4 file systemsG
>
> On Wed, May 07, 2014 at 10:15:56AM +0200, Lukáš Czerner wrote:
> > > Yes, that's a cut and paste typo, thanks for spotting it. It should
> > > have been:
> > >
> > > } else if (sb->s_mkfs_time) {
> > > tm = sb->s_mkfs_time;
> > > printf(_("\tcreated on %s"), ctime(&tm));
> > > } else if (sb->s_mtime) { <========
> >
> > But you're already checking for sb->s_mtime in the first condition,
> > am I missing something ?
>
> The basic idea is to give one bit of context, and whatever would be
> the most useful. In order of preference, it's:
>
> 1) Last mount directory (if available) and last mount time
> 2) Time file system was created
> 3) Time file system was written
>
> #2 or #3 is only needed if the file system was never mounted.
>
> #3 is only needed for those file systems that (a) were never mounted,
> (b) was modified/filled via e2tools or debugfs. (Or Windows FS SDK
> based hacks, etc.)
>
> - Ted
>
I understand that, but here is what is in your patch:
+ if (sb->s_mtime) {
+ tm = sb->s_mtime;
+ if (sb->s_last_mounted[0]) {
+ memset(buf, 0, sizeof(buf));
+ strncpy(buf, sb->s_last_mounted,
+ sizeof(sb->s_last_mounted));
+ printf(_("\tlast mounted on %s on %s"), buf,
+ ctime(&tm));
+ } else
+ printf(_("\tlast mounted on %s"), ctime(&tm));
+ } else if (sb->s_mkfs_time) {
+ tm = sb->s_mkfs_time;
+ printf(_("\tcreated on %s"), ctime(&tm));
+ } else if (sb->s_mkfs_time) {
+ tm = sb->s_mtime;
+ printf(_("\tlast modified on %s"), ctime(&tm));
+ }
Now you're saying that the last condition should really be
} else if (sb->s_mtime) { <========
But that does not make sense because it's the same as the first
condition, so it would either never get there, or never be true.
So it really should be
} else if (sb->s_wtime) {
so the whole thing should look like:
+ if (sb->s_mtime) {
+ tm = sb->s_mtime;
+ if (sb->s_last_mounted[0]) {
+ memset(buf, 0, sizeof(buf));
+ strncpy(buf, sb->s_last_mounted,
+ sizeof(sb->s_last_mounted));
+ printf(_("\tlast mounted on %s on %s"), buf,
+ ctime(&tm));
+ } else
+ printf(_("\tlast mounted on %s"), ctime(&tm));
+ } else if (sb->s_mkfs_time) {
+ tm = sb->s_mkfs_time;
+ printf(_("\tcreated on %s"), ctime(&tm));
+ } else if (sb->s_wtime) {
+ tm = sb->s_wtime;
+ printf(_("\tlast modified on %s"), ctime(&tm));
+ }
Also I wonder whether we need to use 'tm' variable, can't we use the
sb->s_*time directly ? But that's nitpicking.
Thanks!
-Lukas
