From: Theodore Ts'o <tytso@mit.edu>
To: Zheng Liu <gnehzuil.liu@gmail.com>
Cc: linux-ext4@vger.kernel.org,
"Darrick J. Wong" <darrick.wong@oracle.com>,
Zheng Liu <wenqing.lz@taobao.com>
Subject: Re: [PATCH v3 11/30] libext2fs: handle inline data in dir iterator function
Date: Mon, 3 Mar 2014 17:16:53 -0500 [thread overview]
Message-ID: <20140303221653.GA4452@thunk.org> (raw)
In-Reply-To: <1386323897-2354-12-git-send-email-wenqing.lz@taobao.com>
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);
}
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;
}
next prev parent reply other threads:[~2014-03-03 22:17 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-06 9:57 [PATCH v3 00/30] e2fsprogs: inline data refinement patch set Zheng Liu
2013-12-06 9:57 ` [PATCH v3 01/30] libext2fs: support modifying arbitrary extended attributes Zheng Liu
2013-12-06 9:57 ` [PATCH v3 02/30] libext2fs: various tweaks to the xattr editor APIs Zheng Liu
2013-12-06 9:57 ` [PATCH v3 03/30] libext2fs: extend xattr api to query number of attrs Zheng Liu
2013-12-06 9:57 ` [PATCH v3 04/30] libext2fs: fix memory leaks in extended attribute code Zheng Liu
2013-12-06 9:57 ` [PATCH v3 05/30] libext2fs: fix block leak when releasing xattr block Zheng Liu
2013-12-06 9:57 ` [PATCH v3 06/30] libext2fs: remove redundant code Zheng Liu
2013-12-06 9:57 ` [PATCH v3 07/30] libext2fs: free key/value pairs before reading Zheng Liu
2013-12-06 9:57 ` [PATCH v3 08/30] debugfs: dump all extended attributes Zheng Liu
2013-12-06 9:57 ` [PATCH v3 09/30] libext2fs: check inline_data in ext2fs_xattrs_read/write Zheng Liu
2013-12-06 21:38 ` Darrick J. Wong
2013-12-07 3:45 ` Zheng Liu
2014-03-03 23:33 ` Darrick J. Wong
2013-12-06 9:57 ` [PATCH v3 10/30] libext2fs: define new dirent_swab interfaces for inline data Zheng Liu
2014-03-03 15:49 ` Theodore Ts'o
2014-03-13 16:32 ` jon ernst
2014-03-14 2:44 ` Theodore Ts'o
2013-12-06 9:57 ` [PATCH v3 11/30] libext2fs: handle inline data in dir iterator function Zheng Liu
2014-03-03 22:16 ` Theodore Ts'o [this message]
2014-03-03 23:31 ` Darrick J. Wong
2014-03-04 13:56 ` Theodore Ts'o
2014-03-04 4:48 ` Zheng Liu
2013-12-06 9:57 ` [PATCH v3 12/30] libext2fs: handle inline_data in block " Zheng Liu
2013-12-06 9:58 ` [PATCH v3 13/30] debugfs: make stat command support inline data Zheng Liu
2013-12-06 9:58 ` [PATCH v3 14/30] debugfs: make expand " Zheng Liu
2013-12-06 9:58 ` [PATCH v3 15/30] debugfs: make mkdir " Zheng Liu
2013-12-06 9:58 ` [PATCH v3 16/30] debugfs: make lsdel " Zheng Liu
2013-12-06 9:58 ` [PATCH v3 17/30] debugfs: handle inline_data feature in bmap command Zheng Liu
2013-12-06 9:58 ` [PATCH v3 18/30] debugfs: handle inline data feature in punch command Zheng Liu
2013-12-06 9:58 ` [PATCH v3 19/30] libext2fs: handle inline data in read/write function Zheng Liu
2014-03-03 23:57 ` Darrick J. Wong
2013-12-06 9:58 ` [PATCH v3 20/30] libext2fs: add inline_data feature into EXT2_LIB_FEATURE_INCOMPAT_SUPP Zheng Liu
2013-12-06 9:58 ` [PATCH v3 21/30] mke2fs: add inline_data support in mke2fs Zheng Liu
2013-12-06 9:58 ` [PATCH v3 22/30] tune2fs: add inline_data feature in tune2fs Zheng Liu
2013-12-10 21:06 ` Darrick J. Wong
2013-12-06 9:58 ` [PATCH v3 23/30] e2fsck: add problem descriptions and check inline data feature Zheng Liu
2013-12-06 9:58 ` [PATCH v3 24/30] e2fsck: check inline_data in pass1 Zheng Liu
2014-03-03 22:18 ` Theodore Ts'o
2014-03-04 4:51 ` Zheng Liu
2013-12-06 9:58 ` [PATCH v3 25/30] e2fsck: check inline_data in pass2 Zheng Liu
2013-12-06 9:58 ` [PATCH v3 26/30] e2fsck: check inline_data in pass3 Zheng Liu
2013-12-06 9:58 ` [PATCH v3 27/30] tests: change result in f_bad_disconnected_inode Zheng Liu
2013-12-06 9:58 ` [PATCH v3 28/30] mke2fs: enable inline_data feature on ext4dev filesystem Zheng Liu
2013-12-06 9:58 ` [PATCH v3 29/30] libext2fs: export inode cahce creation function Zheng Liu
2013-12-06 9:58 ` [PATCH v3 30/30] libext2fs: add a unit test for inline data Zheng Liu
2013-12-07 1:53 ` [PATCH v3 00/30] e2fsprogs: inline data refinement patch set Darrick J. Wong
2013-12-07 3:43 ` Zheng Liu
-- strict thread matches above, loose matches on Subject: below --
2014-03-06 16:31 [PATCH v3 11/30] libext2fs: handle inline data in dir iterator function Theodore Ts'o
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140303221653.GA4452@thunk.org \
--to=tytso@mit.edu \
--cc=darrick.wong@oracle.com \
--cc=gnehzuil.liu@gmail.com \
--cc=linux-ext4@vger.kernel.org \
--cc=wenqing.lz@taobao.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.