| Message ID | 20130115194330.GF17719@thunk.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 01/15/2013 11:43 AM, Theodore Ts'o wrote: > I'll fix up the this patch before I commit it, but this is a perfect > exhibit about why I request that code submissions come with test > cases. It turns out that there were a couple of problems with > ext2fs_symlink(), that showed up very quickly as soon as I started > writing a test case (where it's important to run e2fsck on the > resulting file system after creating the symlinks --- e2fsck is a > wonderful rep invariant checkers for ext[234] file systems. :-) > > *) i_blocks must be set to 0 for fast symlinks > *) The last argument of ext2fs_inode_alloc_stats() indicates whether > the new inode is a directory or not. So when you cut and pasted > the code from ext2fs_mkdir(), it needed to be changed. > *) Zeroing the entire block before setting the symlink in the case > where it needs to use an external data block makes it a lot > easier to write the regression test. > > So here's the patch I needed to apply on top of your submission.... > Ted, I apologize for not getting to the test case, I meant to, and was still meaning to. I was pulled away on a number of other tasks, took a family vacation etc. I figured I was the only one that cared and it would be me who had to pay the price if things changed while I delayed. THANK YOU, THANK YOU! I really appreciate you writing the test cases and fixing up the remaining issues with the patch. This should now allow us to work with a debugfs script to validate this tooling to populate filesystem images. Once we have done that, we'll want to look at pushing some things (like file copy wrappers) from debugfs into libext2fs and implement the -i (initial dir) option we discused for mke2fs. We'll do our validation and then resurface here. Thanks again! -- Darren -- 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/lib/ext2fs/symlink.c b/lib/ext2fs/symlink.c index fb8b91b..da6e3a8 100644 --- a/lib/ext2fs/symlink.c +++ b/lib/ext2fs/symlink.c @@ -78,7 +78,7 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino, memset(&inode, 0, sizeof(struct ext2_inode)); inode.i_mode = LINUX_S_IFLNK | 0777; inode.i_uid = inode.i_gid = 0; - ext2fs_iblk_set(fs, &inode, 1); + ext2fs_iblk_set(fs, &inode, fastlink ? 0 : 1); inode.i_links_count = 1; inode.i_size = target_len; /* The time fields are set by ext2fs_write_new_inode() */ @@ -88,6 +88,7 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino, strcpy((char *)&inode.i_block, target); } else { /* Slow symlinks, target stored in the first block */ + memset(block_buf, 0, fs->blocksize); strcpy(block_buf, target); if (fs->super->s_feature_incompat & EXT3_FEATURE_INCOMPAT_EXTENTS) { @@ -149,7 +150,7 @@ errcode_t ext2fs_symlink(ext2_filsys fs, ext2_ino_t parent, ext2_ino_t ino, */ if (!fastlink) ext2fs_block_alloc_stats2(fs, blk, +1); - ext2fs_inode_alloc_stats2(fs, ino, +1, 1); + ext2fs_inode_alloc_stats2(fs, ino, +1, 0); cleanup: if (block_buf)