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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.00.1405071534070.2128@localhost.localdomain>
Date:	Wed, 7 May 2014 15:43:09 +0200 (CEST)
From:	Lukáš Czerner <lczerner@...hat.com>
To:	"Theodore Ts'o" <tytso@....edu>
cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH -v2 2/3] mke2fs: print extra information about existing
 ext2/3/4 file systemsGjj

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

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