Patchwork [1/3] lib/ext2fs: Add ext2fs_symlink

login
register
mail settings
Submitter Theodore Ts'o
Date Jan. 15, 2013, 7:43 p.m.
Message ID <20130115194330.GF17719@thunk.org>
Download mbox | patch
Permalink /patch/212301/
State Accepted
Headers show

Comments

Theodore Ts'o - Jan. 15, 2013, 7:43 p.m.
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

--
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
Darren Hart - Jan. 19, 2013, 5:29 a.m.
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

Patch

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)