diff mbox

libext2fs: add new test: tst_inode_size

Message ID 1316020593-19050-1-git-send-email-tytso@mit.edu
State Accepted, archived
Headers show

Commit Message

Theodore Ts'o Sept. 14, 2011, 5:16 p.m. UTC 
This test makes sure the size of the ext2_inode is what we expect

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 lib/ext2fs/Makefile.in      |    8 ++++-
 lib/ext2fs/tst_inode_size.c |   80 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 1 deletions(-)
 create mode 100644 lib/ext2fs/tst_inode_size.c

Comments

Andreas Dilger Sept. 14, 2011, 8:47 p.m. UTC | #1 
On 2011-09-14, at 11:16 AM, Theodore Ts'o wrote:
> This test makes sure the size of the ext2_inode is what we expect
> 
> +#define offsetof(type, member)  __builtin_offsetof (type, member)
> +#define check_field(x) cur_offset = do_field(#x, sizeof(inode.x),	\
> +				offsetof(struct ext2_inode, x), \
> +				cur_offset)

One thing I noticed with this check_field() macro is that it doesn't
actually detect the case if the size of a field is changed.  This hit
me when I was making a cleanup to the large journal patch which renamed
s_jnl_blocks[15] to s_jnl_size_lo and s_jnl_blocks[16] to s_jnl_size_hi
for clarity.  The tst_super_size test passed just fine, but the e2fsck
test scripts failed in weird and wonderful ways.

A better solution might be to explicitly pass the expected field size
instead of getting both the size and offset from the structure itself.
Since these structures change very rarely it isn't much maintenance,
but it would be lousy if code was released that had some incorrect
field offset because someone increased or decreased an earlier field
without thinking enough, and those fields weren't used in normal tests.

I can submit a patch if you are interested.

> +static int do_field(const char *field, size_t size, int offset, int cur_offset)
> +{
> +	if (offset != cur_offset) {
> +		printf("Warning!  Unexpected offset at %s\n", field);
> +		exit(1);
> +	}
> +	printf("%8d %-30s %3u\n", offset, field, (unsigned) size);
> +	return offset + size;
> +}
> +
> +void check_structure_fields()
> +{
> +#if (__GNUC__ >= 4)
> +	int cur_offset = 0;
> +
> +	printf("%8s %-30s %3s\n", "offset", "field", "size");
> +	check_field(i_mode);
> +	check_field(i_uid);
> +	check_field(i_size);
> +	check_field(i_atime);
> +	check_field(i_ctime);
> +	check_field(i_mtime);
> +	check_field(i_dtime);
> +	check_field(i_gid);
> +	check_field(i_links_count);
> +	check_field(i_blocks);
> +	check_field(i_flags);
> +	check_field(osd1.linux1.l_i_version);
> +	check_field(i_block);
> +	check_field(i_generation);
> +	check_field(i_file_acl);
> +	check_field(i_size_high);
> +	check_field(i_faddr);
> +	check_field(osd2.linux2.l_i_blocks_hi);
> +	check_field(osd2.linux2.l_i_file_acl_high);
> +	check_field(osd2.linux2.l_i_uid_high);
> +	check_field(osd2.linux2.l_i_gid_high);
> +	check_field(osd2.linux2.l_i_reserved2);
> +	printf("Ending offset is %d\n\n", cur_offset);
> +#endif
> +}
> +
> +
> +int main(int argc, char **argv)
> +{
> +	int l = sizeof(struct ext2_inode);
> +
> +	check_structure_fields();
> +	printf("Size of struct ext2_inode is %d\n", l);
> +	if (l != 256) {
> +		exit(1);
> +	}
> +	exit(0);
> +}
> -- 
> 1.7.4.1.22.gec8e1.dirty
> 
> --
> 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


Cheers, Andreas





--
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
Theodore Ts'o Sept. 14, 2011, 10:15 p.m. UTC | #2 
On Wed, Sep 14, 2011 at 02:47:08PM -0600, Andreas Dilger wrote:
> One thing I noticed with this check_field() macro is that it doesn't
> actually detect the case if the size of a field is changed.  This hit
> me when I was making a cleanup to the large journal patch which renamed
> s_jnl_blocks[15] to s_jnl_size_lo and s_jnl_blocks[16] to s_jnl_size_hi
> for clarity.  The tst_super_size test passed just fine, but the e2fsck
> test scripts failed in weird and wonderful ways.
> 
> A better solution might be to explicitly pass the expected field size
> instead of getting both the size and offset from the structure itself.
> Since these structures change very rarely it isn't much maintenance,
> but it would be lousy if code was released that had some incorrect
> field offset because someone increased or decreased an earlier field
> without thinking enough, and those fields weren't used in normal tests.
> 
> I can submit a patch if you are interested.

Good point.  Yes, I agree it would be worth while to do this.

     	     	    	     	      - 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
Andreas Dilger March 17, 2012, 3:07 a.m. UTC | #3 
On 2011-09-14, at 4:15 PM, Ted Ts'o wrote:
> On Wed, Sep 14, 2011 at 02:47:08PM -0600, Andreas Dilger wrote:
>> One thing I noticed with this check_field() macro is that it doesn't
>> actually detect the case if the size of a field is changed.  This hit
>> me when I was making a cleanup to the large journal patch which renamed
>> s_jnl_blocks[15] to s_jnl_size_lo and s_jnl_blocks[16] to s_jnl_size_hi
>> for clarity.  The tst_super_size test passed just fine, but the e2fsck
>> test scripts failed in weird and wonderful ways.
>> 
>> A better solution might be to explicitly pass the expected field size
>> instead of getting both the size and offset from the structure itself.
>> Since these structures change very rarely it isn't much maintenance,
>> but it would be lousy if code was released that had some incorrect
>> field offset because someone increased or decreased an earlier field
>> without thinking enough, and those fields weren't used in normal tests.
>> 
>> I can submit a patch if you are interested.
> 
> Good point.  Yes, I agree it would be worth while to do this.

Finally had a few minutes to sit down and work on this.  Patch
attached, since I'm offline right now.

Cheers, Andreas
--
Andreas Dilger                       Whamcloud, Inc.
Principal Lustre Engineer            http://www.whamcloud.com/
Theodore Ts'o March 20, 2012, 3:55 a.m. UTC | #4 
On Fri, Mar 16, 2012 at 09:07:34PM -0600, Andreas Dilger wrote:
> > 
> > Good point.  Yes, I agree it would be worth while to do this.
> 
> Finally had a few minutes to sit down and work on this.  Patch
> attached, since I'm offline right now.

Thanks, applied.

				- 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 mbox

Patch

diff --git a/lib/ext2fs/Makefile.in b/lib/ext2fs/Makefile.in
index f6338f0..e374c1c 100644
--- a/lib/ext2fs/Makefile.in
+++ b/lib/ext2fs/Makefile.in
@@ -262,6 +262,12 @@  tst_super_size: tst_super_size.o
 	$(E) "	LD $@"
 	$(Q) $(CC) -o tst_super_size tst_super_size.o 
 
+tst_inode_size.o: $(srcdir)/tst_inode_size.c $(srcdir)/ext2_fs.h
+
+tst_inode_size: tst_inode_size.o
+	$(E) "	LD $@"
+	$(Q) $(CC) -o tst_inode_size tst_inode_size.o 
+
 ext2_tdbtool: tdbtool.o
 	$(E) "	LD $@"
 	$(Q) $(CC) -o ext2_tdbtool tdbtool.o tdb.o
@@ -339,7 +345,7 @@  mkjournal: mkjournal.c $(STATIC_LIBEXT2FS) $(DEPLIBCOM_ERR)
 	$(E) "	LD $@"
 	$(Q) $(CC) -o mkjournal $(srcdir)/mkjournal.c -DDEBUG $(STATIC_LIBEXT2FS) $(LIBCOM_ERR) $(ALL_CFLAGS)
 
-check:: tst_bitops tst_badblocks tst_iscan tst_types tst_icount tst_super_size tst_types tst_csum
+check:: tst_bitops tst_badblocks tst_iscan tst_types tst_icount tst_super_size tst_types tst_inode_size tst_csum
 	LD_LIBRARY_PATH=$(LIB) DYLD_LIBRARY_PATH=$(LIB) ./tst_bitops
 	LD_LIBRARY_PATH=$(LIB) DYLD_LIBRARY_PATH=$(LIB) ./tst_badblocks
 	LD_LIBRARY_PATH=$(LIB) DYLD_LIBRARY_PATH=$(LIB) ./tst_iscan
diff --git a/lib/ext2fs/tst_inode_size.c b/lib/ext2fs/tst_inode_size.c
new file mode 100644
index 0000000..962f1cd
--- /dev/null
+++ b/lib/ext2fs/tst_inode_size.c
@@ -0,0 +1,80 @@ 
+/*
+ * This testing program makes sure the ext2_inode structure is 1024 bytes long
+ *
+ * Copyright (C) 2007 by Theodore Ts'o.
+ *
+ * %Begin-Header%
+ * This file may be redistributed under the terms of the GNU Library
+ * General Public License, version 2.
+ * %End-Header%
+ */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+
+#include "ext2_fs.h"
+
+struct ext2_inode inode;
+
+int verbose = 0;
+
+#define offsetof(type, member)  __builtin_offsetof (type, member)
+#define check_field(x) cur_offset = do_field(#x, sizeof(inode.x),	\
+				offsetof(struct ext2_inode, x), \
+				cur_offset)
+
+static int do_field(const char *field, size_t size, int offset, int cur_offset)
+{
+	if (offset != cur_offset) {
+		printf("Warning!  Unexpected offset at %s\n", field);
+		exit(1);
+	}
+	printf("%8d %-30s %3u\n", offset, field, (unsigned) size);
+	return offset + size;
+}
+
+void check_structure_fields()
+{
+#if (__GNUC__ >= 4)
+	int cur_offset = 0;
+
+	printf("%8s %-30s %3s\n", "offset", "field", "size");
+	check_field(i_mode);
+	check_field(i_uid);
+	check_field(i_size);
+	check_field(i_atime);
+	check_field(i_ctime);
+	check_field(i_mtime);
+	check_field(i_dtime);
+	check_field(i_gid);
+	check_field(i_links_count);
+	check_field(i_blocks);
+	check_field(i_flags);
+	check_field(osd1.linux1.l_i_version);
+	check_field(i_block);
+	check_field(i_generation);
+	check_field(i_file_acl);
+	check_field(i_size_high);
+	check_field(i_faddr);
+	check_field(osd2.linux2.l_i_blocks_hi);
+	check_field(osd2.linux2.l_i_file_acl_high);
+	check_field(osd2.linux2.l_i_uid_high);
+	check_field(osd2.linux2.l_i_gid_high);
+	check_field(osd2.linux2.l_i_reserved2);
+	printf("Ending offset is %d\n\n", cur_offset);
+#endif
+}
+
+
+int main(int argc, char **argv)
+{
+	int l = sizeof(struct ext2_inode);
+
+	check_structure_fields();
+	printf("Size of struct ext2_inode is %d\n", l);
+	if (l != 256) {
+		exit(1);
+	}
+	exit(0);
+}