Re: [PATCH -v2 2/3] mke2fs: print extra information about existing ext2/3/4 file systemsGjj

["Lukáš Czerner" <lczerner@redhat.com>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3301 bytes --]

On Wed, 7 May 2014, Theodore Ts'o wrote:

> Date: Wed, 7 May 2014 08:39:13 -0400
> From: Theodore Ts'o <tytso@mit.edu>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: Ext4 Developers List <linux-ext4@vger.kernel.org>
> 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