From: Zheng Liu Subject: Re: [PATCH v3 11/30] libext2fs: handle inline data in dir iterator function Date: Tue, 4 Mar 2014 12:48:55 +0800 Message-ID: <20140304044855.GA6630@gmail.com> References: <1386323897-2354-1-git-send-email-wenqing.lz@taobao.com> <1386323897-2354-12-git-send-email-wenqing.lz@taobao.com> <20140303221653.GA4452@thunk.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-ext4@vger.kernel.org, "Darrick J. Wong" , Zheng Liu To: Theodore Ts'o Return-path: Received: from mail-pb0-f51.google.com ([209.85.160.51]:47291 "EHLO mail-pb0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755944AbaCDEn0 (ORCPT ); Mon, 3 Mar 2014 23:43:26 -0500 Received: by mail-pb0-f51.google.com with SMTP id uo5so3996081pbc.38 for ; Mon, 03 Mar 2014 20:43:26 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140303221653.GA4452@thunk.org> Sender: linux-ext4-owner@vger.kernel.org List-ID: On Mon, Mar 03, 2014 at 05:16:53PM -0500, Theodore Ts'o wrote: > As I mentioend on today's call, there are some pretty serious issues > with ext2fs_inline_data_dir_iterate(). In some places, it is > returning an errcode_t, and in some cases, it was returning the > BLOCK_ABORT, et. al flags (which would be an int). > > Also, all of the callers of ext2fs_inline_data_dir_iterate() pass in > ext2fs_process_dir_block(), and given that > ext2fs_inline-data_dir_iterate() is a non-public function, it makes > the code simpler and more maintable to call ext2fs_process_dir_block() > directly. I think we would be better having a > ext2fs_process_dir_buffer() function, instead of complicating the > ext2fs_process_dir_block() interface, but that's a cleanup that we can > do in a different commit. > > Here are the diffs that I had to apply to make everything be > consistent. It was a pretty significant change, so I'd appreciate a > second pair of eyes to sanity check what I changed. This diff is > versus the state of the tree after applying your PATCH v3 11/30 diff. > > - Ted > > diff --git a/lib/ext2fs/dir_iterate.c b/lib/ext2fs/dir_iterate.c > index 6bdfae5..8afccbe 100644 > --- a/lib/ext2fs/dir_iterate.c > +++ b/lib/ext2fs/dir_iterate.c > @@ -129,9 +129,7 @@ errcode_t ext2fs_dir_iterate2(ext2_filsys fs, > ext2fs_free_mem(&ctx.buf); > if (retval == EXT2_ET_INLINE_DATA_CANT_ITERATE) { > ctx.flags |= DIRENT_FLAG_INCLUDE_INLINE_DATA; > - retval = ext2fs_inline_data_dir_iterate(fs, dir, > - ext2fs_process_dir_block, > - &ctx); > + (void) ext2fs_inline_data_dir_iterate(fs, dir, &ctx); retval = 0; Here we need to clear EXT2_ET_INLINE_DATA_CANT_ITERATE error. Otherwise, ext2fs_dir_iterate2() will always return this error when we iterate a dir with inline data. The patch looks good to me. Thanks for improving this. Reviewed-by: Zheng Liu - Zheng > } > if (retval) > return retval; > diff --git a/lib/ext2fs/ext2fsP.h b/lib/ext2fs/ext2fsP.h > index d910f42..b2afd15 100644 > --- a/lib/ext2fs/ext2fsP.h > +++ b/lib/ext2fs/ext2fsP.h > @@ -88,15 +88,9 @@ extern int ext2fs_process_dir_block(ext2_filsys fs, > int ref_offset, > void *priv_data); > > -extern errcode_t ext2fs_inline_data_dir_iterate(ext2_filsys fs, > - ext2_ino_t ino, > - int (*func)(ext2_filsys fs, > - blk64_t *blocknr, > - e2_blkcnt_t blockcnt, > - blk64_t ref_blk, > - int ref_offset, > - void *priv_data), > - void *priv_data); > +extern int ext2fs_inline_data_dir_iterate(ext2_filsys fs, > + ext2_ino_t ino, > + void *priv_data); > > /* Generic numeric progress meter */ > > diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c > index b768b9a..db1bc03 100644 > --- a/lib/ext2fs/inline_data.c > +++ b/lib/ext2fs/inline_data.c > @@ -78,36 +78,32 @@ err: > } > > > -errcode_t ext2fs_inline_data_dir_iterate(ext2_filsys fs, > - ext2_ino_t ino, > - int (*func)(ext2_filsys fs, > - blk64_t *blocknr, > - e2_blkcnt_t blockcnt, > - blk64_t ref_blk, > - int ref_offset, > - void *priv_data), > - void *priv_data) > +int ext2fs_inline_data_dir_iterate(ext2_filsys fs, ext2_ino_t ino, > + void *priv_data) > { > struct dir_context *ctx; > struct ext2_inode inode; > struct ext2_dir_entry dirent; > struct ext2_inline_data data; > - errcode_t retval = 0; > + int ret = BLOCK_ABORT; > e2_blkcnt_t blockcnt = 0; > > ctx = (struct dir_context *)priv_data; > > - retval = ext2fs_read_inode(fs, ino, &inode); > - if (retval) > + ctx->errcode = ext2fs_read_inode(fs, ino, &inode); > + if (ctx->errcode) > goto out; > > - if (!(inode.i_flags & EXT4_INLINE_DATA_FL)) > - return EXT2_ET_NO_INLINE_DATA; > + if (!(inode.i_flags & EXT4_INLINE_DATA_FL)) { > + ctx->errcode = EXT2_ET_NO_INLINE_DATA; > + goto out; > + } > > if (!LINUX_S_ISDIR(inode.i_mode)) { > - retval = EXT2_ET_NO_DIRECTORY; > + ctx->errcode = EXT2_ET_NO_DIRECTORY; > goto out; > } > + ret = 0; > > /* we first check '.' and '..' dir */ > dirent.inode = ino; > @@ -117,8 +113,8 @@ errcode_t ext2fs_inline_data_dir_iterate(ext2_filsys fs, > dirent.name[1] = '\0'; > ctx->buf = (char *)&dirent; > ext2fs_get_rec_len(fs, &dirent, &ctx->buflen); > - retval |= (*func)(fs, 0, blockcnt++, 0, 0, priv_data); > - if (retval & BLOCK_ABORT) > + ret |= ext2fs_process_dir_block(fs, 0, blockcnt++, 0, 0, priv_data); > + if (ret & BLOCK_ABORT) > goto out; > > dirent.inode = ext2fs_le32_to_cpu(inode.i_block[0]); > @@ -129,8 +125,8 @@ errcode_t ext2fs_inline_data_dir_iterate(ext2_filsys fs, > dirent.name[2] = '\0'; > ctx->buf = (char *)&dirent; > ext2fs_get_rec_len(fs, &dirent, &ctx->buflen); > - retval |= (*func)(fs, 0, blockcnt++, 0, 0, priv_data); > - if (retval & BLOCK_INLINE_DATA_CHANGED) { > + ret |= ext2fs_process_dir_block(fs, 0, blockcnt++, 0, 0, priv_data); > + if (ret & BLOCK_INLINE_DATA_CHANGED) { > errcode_t err; > > inode.i_block[0] = ext2fs_cpu_to_le32(dirent.inode); > @@ -138,62 +134,68 @@ errcode_t ext2fs_inline_data_dir_iterate(ext2_filsys fs, > if (err) > goto out; > } > - if (retval & BLOCK_ABORT) > + if (ret & BLOCK_ABORT) > goto out; > > ctx->buf = (char *)inode.i_block + EXT4_INLINE_DATA_DOTDOT_SIZE; > ctx->buflen = EXT4_MIN_INLINE_DATA_SIZE - EXT4_INLINE_DATA_DOTDOT_SIZE; > #ifdef WORDS_BIGENDIAN > - retval = ext2fs_dirent_swab_in2(fs, ctx->buf, ctx->buflen, 0); > - if (retval) > + ctx->errcode = ext2fs_dirent_swab_in2(fs, ctx->buf, ctx->buflen, 0); > + if (ctx->errcode) { > + ret |= BLOCK_ABORT; > goto out; > + } > #endif > - retval |= (*func)(fs, 0, blockcnt++, 0, 0, priv_data); > - if (retval & BLOCK_INLINE_DATA_CHANGED) { > - errcode_t err; > - > + ret |= ext2fs_process_dir_block(fs, 0, blockcnt++, 0, 0, priv_data); > + if (ret & BLOCK_INLINE_DATA_CHANGED) { > #ifdef WORDS_BIGENDIAN > - err = ext2fs_dirent_swab_out2(fs, ctx->buf, ctx->buflen, 0); > - if (err) > + ctx->errcode = ext2fs_dirent_swab_out2(fs, ctx->buf, > + ctx->buflen, 0); > + if (ctx->errcode) { > + ret |= BLOCK_ABORT; > goto out; > + } > #endif > - err = ext2fs_write_inode(fs, ino, &inode); > - if (err) > - goto out; > + ctx->errcode = ext2fs_write_inode(fs, ino, &inode); > + if (ctx->errcode) > + ret |= BLOCK_ABORT; > } > - if (retval & BLOCK_ABORT) > + if (ret & BLOCK_ABORT) > goto out; > > data.fs = fs; > data.ino = ino; > - retval = ext2fs_inline_data_ea_get(&data); > - if (retval) > + ctx->errcode = ext2fs_inline_data_ea_get(&data); > + if (ctx->errcode) { > + ret |= BLOCK_ABORT; > goto out; > + } > if (data.ea_size <= 0) > goto out; > > ctx->buf = data.ea_data; > ctx->buflen = data.ea_size; > #ifdef WORDS_BIGENDIAN > - retval = ext2fs_dirent_swab_in2(fs, ctx->buf, ctx->buflen, 0); > - if (retval) > + ctx.errcode = ext2fs_dirent_swab_in2(fs, ctx->buf, ctx->buflen, 0); > + if (ctx.errcode) { > + ret |= BLOCK_ABORT; > goto out; > + } > #endif > > - retval |= (*func)(fs, 0, blockcnt++, 0, 0, priv_data); > - if (retval & BLOCK_INLINE_DATA_CHANGED) { > - errcode_t err; > - > + ret |= ext2fs_process_dir_block(fs, 0, blockcnt++, 0, 0, priv_data); > + if (ret & BLOCK_INLINE_DATA_CHANGED) { > #ifdef WORDS_BIGENDIAN > - err = ext2fs_dirent_swab_out2(fs, ctx->buf, ctx->buflen, 0); > - if (err) { > - retval = err; > + ctx->errcode = ext2fs_dirent_swab_out2(fs, ctx->buf, > + ctx->buflen, 0); > + if (ctx->errcode) { > + ret |= BLOCK_ABORT; > goto out1; > } > #endif > - err = ext2fs_inline_data_ea_set(&data); > - if (err) > - retval = err; > + ctx->errcode = ext2fs_inline_data_ea_set(&data); > + if (ctx->errcode) > + ret |= BLOCK_ABORT; > } > > out1: > @@ -201,6 +203,6 @@ out1: > ctx->buf = 0; > > out: > - retval &= ~(BLOCK_ABORT | BLOCK_INLINE_DATA_CHANGED); > - return retval & BLOCK_ERROR ? ctx->errcode : retval; > + ret &= ~(BLOCK_ABORT | BLOCK_INLINE_DATA_CHANGED); > + return ret; > }