| Message ID | 1399338133-21373-2-git-send-email-tytso@mit.edu |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Mon, 5 May 2014, Theodore Ts'o wrote: > Date: Mon, 5 May 2014 21:02:12 -0400 > From: Theodore Ts'o <tytso@mit.edu> > To: Ext4 Developers List <linux-ext4@vger.kernel.org> > Cc: Theodore Ts'o <tytso@mit.edu> > Subject: [PATCH -v2 2/3] mke2fs: print extra information about existing > ext2/3/4 file systems > > The basic idea is to provide a bit more context in this situation: > > % ./misc/mke2fs -t ext4 /dev/sdc3 > mke2fs 1.42.9 (4-Feb-2014) > /dev/sdc3 contains a ext4 file system > Proceed anyway? (y,n) > > ... by adding this bit of context: > > % ./misc/mke2fs -t ext4 /dev/sdc3 > mke2fs 1.42.9 (4-Feb-2014) > /dev/sdc3 contains a ext4 file system > last mounted on /SOX-backups on Mon May 5 08:59:53 2014 > Proceed anyway? (y,n) > > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> > --- > misc/util.c | 37 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/misc/util.c b/misc/util.c > index 15b4ce5..d63e21b 100644 > --- a/misc/util.c > +++ b/misc/util.c > @@ -105,6 +105,41 @@ void proceed_question(int delay) > signal(SIGALRM, SIG_IGN); > } > > +static void print_ext2_info(const char *device) > + > +{ > + struct ext2_super_block *sb; > + ext2_filsys fs; > + errcode_t retval; > + time_t tm; > + char buf[80]; > + > + retval = ext2fs_open2(device, 0, 0, 0, 0, unix_io_manager, &fs); Sorry for not noticing this earlier, but we might want to pass EXT2_FLAG_64BITS. > + if (retval) > + return; > + sb = fs->super; > + > + 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) { This does not seem right, you've already checked for s_mkfs_time and if was not set if you got here. I guess, it should be something else ? s_wtime perhaps ? But, can this be set when the fs was not mounted (like using ext2fs library ?) > + tm = sb->s_mtime; If you got here, then again this is not set. Thanks! -Lukas > + printf(_("\tlast modified on %s"), ctime(&tm)); > + } > + ext2fs_close(fs); > +} > + > + > /* > * return 1 if the device looks plausible, creating the file if necessary > */ > @@ -168,6 +203,8 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev) > else > printf(_("%s contains a %s file system\n"), device, > fs_type); > + if (strncmp(fs_type, "ext", 3) == 0) > + print_ext2_info(device); > free(fs_type); > free(fs_label); > return 0; > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 06, 2014 at 03:42:37PM +0200, Lukáš Czerner wrote: > > + retval = ext2fs_open2(device, 0, 0, 0, 0, unix_io_manager, &fs); > > Sorry for not noticing this earlier, but we might want to pass > EXT2_FLAG_64BITS. Done. > > + } else if (sb->s_mkfs_time) { > > + tm = sb->s_mkfs_time; > > + printf(_("\tcreated on %s"), ctime(&tm)); > > + } else if (sb->s_mkfs_time) { > > This does not seem right, you've already checked for s_mkfs_time and > if was not set if you got here. I guess, it should be something else > ? s_wtime perhaps ? But, can this be set when the fs was not mounted > (like using ext2fs library ?) > > > + tm = sb->s_mtime; > > If you got here, then again this is not set. 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) { <======== tm = sb->s_mtime; printf(_("\tlast modified on %s"), ctime(&tm)); } Cheers, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 6 May 2014, Theodore Ts'o wrote: > Date: Tue, 6 May 2014 22:46:21 -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 systems > > On Tue, May 06, 2014 at 03:42:37PM +0200, Lukáš Czerner wrote: > > > + retval = ext2fs_open2(device, 0, 0, 0, 0, unix_io_manager, &fs); > > > > Sorry for not noticing this earlier, but we might want to pass > > EXT2_FLAG_64BITS. > > Done. > > > > + } else if (sb->s_mkfs_time) { > > > + tm = sb->s_mkfs_time; > > > + printf(_("\tcreated on %s"), ctime(&tm)); > > > + } else if (sb->s_mkfs_time) { > > > > This does not seem right, you've already checked for s_mkfs_time and > > if was not set if you got here. I guess, it should be something else > > ? s_wtime perhaps ? But, can this be set when the fs was not mounted > > (like using ext2fs library ?) > > > > > + tm = sb->s_mtime; > > > > If you got here, then again this is not set. > > > 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 ? Thanks! -Lukas > tm = sb->s_mtime; > printf(_("\tlast modified on %s"), ctime(&tm)); > } > > > Cheers, > > - Ted > -- > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
On Wed, May 07, 2014 at 03:43:09PM +0200, Lukáš Czerner wrote: > 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)); > + } Sorry, that's in fact what I have. > Also I wonder whether we need to use 'tm' variable, can't we use the > sb->s_*time directly ? But that's nitpicking. That's because sb->s_mkfs_time is a __u32, and time_t isn't necessarily be a 32-bit type (and in fact isn't on x86_64, x32, apropos of current discussion happening on ksummit-discuss.) Yes, that means the ext4 superblock has a 2038 problem, but that's a separate issue that we should fix one of these days.... - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/misc/util.c b/misc/util.c index 15b4ce5..d63e21b 100644 --- a/misc/util.c +++ b/misc/util.c @@ -105,6 +105,41 @@ void proceed_question(int delay) signal(SIGALRM, SIG_IGN); } +static void print_ext2_info(const char *device) + +{ + struct ext2_super_block *sb; + ext2_filsys fs; + errcode_t retval; + time_t tm; + char buf[80]; + + retval = ext2fs_open2(device, 0, 0, 0, 0, unix_io_manager, &fs); + if (retval) + return; + sb = fs->super; + + 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)); + } + ext2fs_close(fs); +} + + /* * return 1 if the device looks plausible, creating the file if necessary */ @@ -168,6 +203,8 @@ int check_plausibility(const char *device, int flags, int *ret_is_dev) else printf(_("%s contains a %s file system\n"), device, fs_type); + if (strncmp(fs_type, "ext", 3) == 0) + print_ext2_info(device); free(fs_type); free(fs_label); return 0;
The basic idea is to provide a bit more context in this situation: % ./misc/mke2fs -t ext4 /dev/sdc3 mke2fs 1.42.9 (4-Feb-2014) /dev/sdc3 contains a ext4 file system Proceed anyway? (y,n) ... by adding this bit of context: % ./misc/mke2fs -t ext4 /dev/sdc3 mke2fs 1.42.9 (4-Feb-2014) /dev/sdc3 contains a ext4 file system last mounted on /SOX-backups on Mon May 5 08:59:53 2014 Proceed anyway? (y,n) Signed-off-by: "Theodore Ts'o" <tytso@mit.edu> --- misc/util.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)