On Wed, Feb 15, 2012 at 02:25:37PM -0500, Ted Ts'o wrote:
> On Fri, Jan 06, 2012 at 08:47:43PM -0800, Darrick J. Wong wrote:
> > The ext2fs_link helper function link_proc does not check the value of ls->done,
> > which means that if the function finds multiple empty spaces that will fit the
> > new directory entry, it will create a directory entry in each of the spaces.
> > Instead of doing that, check the done value and don't do anything more if we've
> > already added the directory entry.
> >
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> This should't necessary, since when we insert the directory entry, we
> return with the DIRENT_ABORT bit set:
>
> dirent->inode = ls->inode;
> dirent->name_len = ls->namelen;
> strncpy(dirent->name, ls->name, ls->namelen);
> if (ls->sb->s_feature_incompat & EXT2_FEATURE_INCOMPAT_FILETYPE)
> dirent->name_len |= (ls->flags & 0x7) << 8;
>
> ls->done++;
> return DIRENT_ABORT|DIRENT_CHANGED;
>
> Did you actually observe this happening?
Yes. I took a look at the block iteration code again, and I'm wondering, do we
need one of these:
if (ret & BLOCK_ABORT)
break;
...around lib/ext2fs/block.c, line 450? This is the code blob:
if (!(extent.e_flags & EXT2_EXTENT_FLAGS_LEAF)) {
if (ctx.flags & BLOCK_FLAG_DATA_ONLY)
continue;
if ((!(extent.e_flags &
EXT2_EXTENT_FLAGS_SECOND_VISIT) &&
!(ctx.flags & BLOCK_FLAG_DEPTH_TRAVERSE)) ||
((extent.e_flags &
EXT2_EXTENT_FLAGS_SECOND_VISIT) &&
(ctx.flags & BLOCK_FLAG_DEPTH_TRAVERSE))) {
ret |= (*ctx.func)(fs, &blk,
-1, 0, 0, priv_data);
if (ret & BLOCK_CHANGED) {
extent.e_pblk = blk;
ctx.errcode =
ext2fs_extent_replace(handle, 0, &extent);
if (ctx.errcode)
break;
}
/* INSERT HERE? */
}
continue;
}
It looks to me that when ctx.func returns BLOCK_ABORT, the continue causes
the code to loop around and get the next extent, which isn't what we want.
--D
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
