On Tue, Aug 07, 2012 at 02:40:02PM -0400, Theodore Ts'o wrote: > One major comment about the library functions. The reason why we have > functions of the form ext2fs_foo(), ext2fs_foo2(), ext2fs_foo3(), > etc. is to preserve backwards compatibility of the ABI. In general, > ext2fs_foo2() will have an extra parameter which wasn't in > ext2fs_foo(), and ext2fs_foo2() will have a superset of the > functionality of ext2fs_foo(). If later we need to add to add another > parameter to further extend the functionality of the function, we > might have an ext2fs_foo3(), which again will be a supserset of the > functionality of ext2fs_foo2(). > > So in general, we only need to implement ext2fs_fooN(), and then > ext2fs_fooX where 0 <= X <= N simply call ext2fs_fooN with the extra > parameters defaulted out (usually to 0 or NULL). > > It also follows that when we create a new function, there's no need to > do this. So the fact that you have an ext2fs_inlinedata_iterate() > which just calls ext2fs_inline_data_iterate2() is not something you > need to do. > > More seriously, ext2fs_inline_data_iterate3() has an implementation > which is completely different from that of > ext2fs_inline_data_iterate2(), and that's an immediate red flag. This > means that these two functions are semantically different, and so they > should have fundamentally different names --- or you need to make > ext2fs_inline_data_iterate3() a strict superset of the functionality > of ext2fs_inline_data_iterate2(). > > Also, I note that there are a number of patches which basically do > this: > > - retval = ext2fs_dir_iterate2(current_fs, inode_num, 0, > - 0, rmdir_proc, &rds); > + if (ext2fs_has_inline_data(current_fs, inode_num)) > + retval = ext2fs_inline_data_iterate2(current_fs, inode_num, 0, > + 0, rmdir_proc, &rds); > + else > + retval = ext2fs_dir_iterate2(current_fs, inode_num, 0, > + 0, rmdir_proc, &rds); > > The much better thing to do is to make ext2fs_dir_iterate2() check to > see if the inode has inline data, and if so, to call > ext2fs_inline_data_iterate2() directly. This has a couple of benefits. > > The first is it will reduce the number of patches we need to apply. > More importantly, it means that existing programs that don't know > about inline data, but who happen to use ext2fs_dir_iterate(), will be > able to work automatically, without requiring code changes. It's also > much cleaner from a conceptual point of view, since the > ext2fs_dir_iterate abstraction shouldn't need to expose to the user > whether the directory data is inline or in a separate data block. Thanks for patient explanation. I will fix the patches according to your advice. Regards, Zheng -- 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
