| Message ID | 1367059481-6090-1-git-send-email-wenqing.lz@taobao.com |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Apr 27, 2013 at 06:44:41PM +0800, Zheng Liu wrote: > From: Zheng Liu <wenqing.lz@taobao.com> > > In libext2fs we don't print any error message form it except that OMIT_COM_ERR > is defined. In lib/ext2fs/inline_data.c it doesn't obey this rule. So fix it. > Meanwhile when we find an dir entry in inline data, we will print the message in > debugfs rather than in libext2fs. > > Cc: "Theodore Ts'o" <tytso@mit.edu> > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com> Hi Ted, As we discuss a while ago, until now, I only submit this patch for improving the inline data patch series for e2fsprogs. IIRC, you have mentioned that we need to check ext2_ext_attr_ibody_header in libext2fs and improve some codes in e2fsck (Please let me know if I forgot something). As you said, you also have some fixes for this, but it is still too un-stable to send them out. So I think maybe we could find a proper public place to share our works in order to avoid some duplicated works. I am happy to improve these stuff. Please let me know if I need to do something. Thanks, - Zheng > --- > [This patch is against pu branch of e2fsprogs tree] > > debugfs/htree.c | 5 ++++- > lib/ext2fs/inline_data.c | 6 +++--- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/debugfs/htree.c b/debugfs/htree.c > index ca39b4b..e042fd7 100644 > --- a/debugfs/htree.c > +++ b/debugfs/htree.c > @@ -388,8 +388,11 @@ void do_dirsearch(int argc, char *argv[]) > pb.len = strlen(pb.search_name); > > if (ext2fs_inode_has_inline_data(current_fs, inode)) { > - ext2fs_inline_data_dirsearch(current_fs, inode, > + errcode_t retval; > + retval = ext2fs_inline_data_dirsearch(current_fs, inode, > argv[2], strlen(argv[2])); > + if (retval) > + printf("Entry found at inline data\n"); > goto out; > } > > diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c > index ea3736c..6d5cf52 100644 > --- a/lib/ext2fs/inline_data.c > +++ b/lib/ext2fs/inline_data.c > @@ -134,16 +134,16 @@ static int do_search_dir(ext2_filsys fs, void *start, unsigned int size, > de = (struct ext2_dir_entry *)(start + offset); > errcode = ext2fs_get_rec_len(fs, de, &rec_len); > if (errcode) { > +#ifndef OMIT_COM_ERR > com_err("search_dir_inline_data", errcode, > "while getting rec_len for inline data"); > +#endif > return errcode; > } > if (de->inode && > len == (de->name_len & 0xFF) && > - strncmp(name, de->name, len) == 0) { > - printf("Entry found at inline data\n"); > + strncmp(name, de->name, len) == 0) > return 1; > - } > offset += rec_len; > } > return 0; > -- > 1.7.12.rc2.18.g61b472e > -- 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
On Sat, Apr 27, 2013 at 06:44:41PM +0800, Zheng Liu wrote: > diff --git a/debugfs/htree.c b/debugfs/htree.c > index ca39b4b..e042fd7 100644 > --- a/debugfs/htree.c > +++ b/debugfs/htree.c > @@ -388,8 +388,11 @@ void do_dirsearch(int argc, char *argv[]) > pb.len = strlen(pb.search_name); > > if (ext2fs_inode_has_inline_data(current_fs, inode)) { > - ext2fs_inline_data_dirsearch(current_fs, inode, > + errcode_t retval; > + retval = ext2fs_inline_data_dirsearch(current_fs, inode, > argv[2], strlen(argv[2])); > + if (retval) > + printf("Entry found at inline data\n"); > goto out; > } The problem with this is that ext2_inline_data_dirsearch() also returns a non-zero error code. So it returns 0 if the entry is not found in the inline data, 1 if it is found in the inline data, and some other error code --- and if some system call returns EPERM (which is one) there will be a collision. This is an example of a badly designed library interface; and since I don't want to break backwards compatibility by removing a library interface once we've added one, we need to be very careful for each new interface that we are thinking of adding. In this particular case, it's not clear to me that this library interface is at all useful, or that dirsearch needs to support inline directories at all in the first place. The only reason dirsearch exists is to debug very large htree directories where the index has gotten corrupted. For a small inline directory, it's just as easy to check to see what's in the directory by using the "ls" command. So having dirsearch simply give an error and not support inline data directory, or to have dirsearch work by iterating over each of the entries in the inline data directory would both be acceptable alternatives. And we need to carry out a similar very careful examination of all of the other new interfaces added to inline_data.c. Sometimes it helps to document exactly what it is that this interface is doing, and what does it return under what circumstance. Then think about what the best generic interface would be for all possible future users of that interface, not just the one or ones in the current e2fsprogs tree. And if there is only one caller of a particular interface, then it may be fair to ask the question whether it should be in the library at all. Regards, - 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
On Mon, May 20, 2013 at 10:36:36PM -0400, Theodore Ts'o wrote: > On Sat, Apr 27, 2013 at 06:44:41PM +0800, Zheng Liu wrote: > > diff --git a/debugfs/htree.c b/debugfs/htree.c > > index ca39b4b..e042fd7 100644 > > --- a/debugfs/htree.c > > +++ b/debugfs/htree.c > > @@ -388,8 +388,11 @@ void do_dirsearch(int argc, char *argv[]) > > pb.len = strlen(pb.search_name); > > > > if (ext2fs_inode_has_inline_data(current_fs, inode)) { > > - ext2fs_inline_data_dirsearch(current_fs, inode, > > + errcode_t retval; > > + retval = ext2fs_inline_data_dirsearch(current_fs, inode, > > argv[2], strlen(argv[2])); > > + if (retval) > > + printf("Entry found at inline data\n"); > > goto out; > > } > > The problem with this is that ext2_inline_data_dirsearch() also > returns a non-zero error code. So it returns 0 if the entry is not > found in the inline data, 1 if it is found in the inline data, and > some other error code --- and if some system call returns EPERM (which > is one) there will be a collision. > > This is an example of a badly designed library interface; and since I > don't want to break backwards compatibility by removing a library > interface once we've added one, we need to be very careful for each > new interface that we are thinking of adding. > > In this particular case, it's not clear to me that this library > interface is at all useful, or that dirsearch needs to support inline > directories at all in the first place. The only reason dirsearch > exists is to debug very large htree directories where the index has > gotten corrupted. For a small inline directory, it's just as easy to > check to see what's in the directory by using the "ls" command. > > So having dirsearch simply give an error and not support inline data > directory, or to have dirsearch work by iterating over each of the > entries in the inline data directory would both be acceptable > alternatives. > > And we need to carry out a similar very careful examination of all of > the other new interfaces added to inline_data.c. Sometimes it helps > to document exactly what it is that this interface is doing, and what > does it return under what circumstance. Then think about what the > best generic interface would be for all possible future users of that > interface, not just the one or ones in the current e2fsprogs tree. > And if there is only one caller of a particular interface, then it may > be fair to ask the question whether it should be in the library at > all. Thanks for teaching me. I will revise all new interfaces in inline_data.c, and re-think about whether they really need to be added or not. If there are other things that I missed, please let me know. Thanks, - Zheng -- 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
The inlinie data patches, which have been updated to the latest master branch, can be found here: https://github.com/tytso/e2fsprogs-inline-patch-queue or git://github.com/tytso/e2fsprogs-inline-patch-queue.git You can look at the history if you want to see what I've done. The main thing to note is that while we are maintaining patches in a patch series, and while we are still trying to clean up the function interfaces, to fold any changes into parent patches. So for example, I've folded all of the gcc -Wall cleanups that had been in "libext2fs-fix-some-warnings-from-gcc-Wall" into the patches that introduced the gcc warnings. This makes it easier to review the patches, since we don't have the problem of a patch which has bugs or warnings which is fixed in a later patch. When I did that, I found a change which was not related to gcc -Wall cleanups hiding in that patch. I think I remember seeing other places where unrelated changes were glommed together into a single patch. As we find them, we should split them into separate patches, again for ease of patch maintenance and improvements. Again, my apologies for taking so long in getting these patches out there. I really wanted to give you some examples in the kind of patch factoring and interface cleanups that I was hoping for before merging this support upstream. Unfortuantely, I've been crazy busy these last few weeks. I had hoped to put more work into this today, but I lost at least two hours due to a burst gas main which forced the evacuations of the offices. :-( (Turns out it's not just network fibers which emit backhoe pheromones, gas mains do as well. :-) I'll let you take the next whack at improving the patches. My suggestion is that you create your own github repository, and update the patches using git. What I generally do is to apply the patches using guilt (I suggest using v0.35 from git://repo.or.cz/guilt.git) by pulling down the github repository into an e2fsprogs repository at ".git/patches/inline". Then take a look at first line of .git/patches/inline/series, which currently reads: # BASE 4cf7a7014a209b82aada9b5d83ecdb9b07e60a1a In the top of the e2fsprogs repository, type the commands: % pushd .git/patches/inline ; sh timestamps ; popd % git branch inline 4cf7a7014a209b82aada9b5d83ecdb9b07e60a1a % git checkout inline % guilt push -a This will apply all of the patches in .git/patches/inline (which you pulled down from github) to the e2fsprogs tree. You can go back and forth by using the commands "guilt pop" and "guilt push". To modify a patch, just edit the e2fsprogs files, and then use the command "guilt refresh --diffstat" to merge your edits into the patch file. You can then commit changes to the patch queue by cd'ing into .git/patches/inline directory, and running standard "git commit" commands. This way, when you send me an updated set of patches, I can look at the git log to see what changes you've made. Does this make sense? If not, please feel free to send me any questions you have. Thanks, - 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
On Mon, May 20, 2013 at 11:26:57PM -0400, Theodore Ts'o wrote: > The inlinie data patches, which have been updated to the latest master > branch, can be found here: > > https://github.com/tytso/e2fsprogs-inline-patch-queue > > or > > git://github.com/tytso/e2fsprogs-inline-patch-queue.git > > > You can look at the history if you want to see what I've done. The > main thing to note is that while we are maintaining patches in a patch > series, and while we are still trying to clean up the function > interfaces, to fold any changes into parent patches. > > So for example, I've folded all of the gcc -Wall cleanups that had > been in "libext2fs-fix-some-warnings-from-gcc-Wall" into the patches > that introduced the gcc warnings. This makes it easier to review the > patches, since we don't have the problem of a patch which has bugs or > warnings which is fixed in a later patch. > > When I did that, I found a change which was not related to gcc -Wall > cleanups hiding in that patch. > > I think I remember seeing other places where unrelated changes were > glommed together into a single patch. As we find them, we should > split them into separate patches, again for ease of patch maintenance > and improvements. > > Again, my apologies for taking so long in getting these patches out > there. I really wanted to give you some examples in the kind of patch > factoring and interface cleanups that I was hoping for before merging > this support upstream. Unfortuantely, I've been crazy busy these last > few weeks. I had hoped to put more work into this today, but I lost > at least two hours due to a burst gas main which forced the > evacuations of the offices. :-( > > (Turns out it's not just network fibers which emit backhoe pheromones, > gas mains do as well. :-) > > I'll let you take the next whack at improving the patches. My > suggestion is that you create your own github repository, and update > the patches using git. What I generally do is to apply the patches > using guilt (I suggest using v0.35 from git://repo.or.cz/guilt.git) by > pulling down the github repository into an e2fsprogs repository at > ".git/patches/inline". Then take a look at first line of > .git/patches/inline/series, which currently reads: > > # BASE 4cf7a7014a209b82aada9b5d83ecdb9b07e60a1a > > In the top of the e2fsprogs repository, type the commands: > > % pushd .git/patches/inline ; sh timestamps ; popd > % git branch inline 4cf7a7014a209b82aada9b5d83ecdb9b07e60a1a > % git checkout inline > % guilt push -a > > This will apply all of the patches in .git/patches/inline (which you > pulled down from github) to the e2fsprogs tree. You can go back and > forth by using the commands "guilt pop" and "guilt push". To modify a > patch, just edit the e2fsprogs files, and then use the command "guilt > refresh --diffstat" to merge your edits into the patch file. You can > then commit changes to the patch queue by cd'ing into > .git/patches/inline directory, and running standard "git commit" > commands. This way, when you send me an updated set of patches, I can > look at the git log to see what changes you've made. > > Does this make sense? If not, please feel free to send me any > questions you have. Hi Ted, Definitely it makes sense to me. I really appreciate and thank you for your suggestion. Yes, I have used guilt to manage my patches for a long time. That is really useful for me. I will revise every single commit, and improve them. Thanks, - Zheng -- 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
diff --git a/debugfs/htree.c b/debugfs/htree.c index ca39b4b..e042fd7 100644 --- a/debugfs/htree.c +++ b/debugfs/htree.c @@ -388,8 +388,11 @@ void do_dirsearch(int argc, char *argv[]) pb.len = strlen(pb.search_name); if (ext2fs_inode_has_inline_data(current_fs, inode)) { - ext2fs_inline_data_dirsearch(current_fs, inode, + errcode_t retval; + retval = ext2fs_inline_data_dirsearch(current_fs, inode, argv[2], strlen(argv[2])); + if (retval) + printf("Entry found at inline data\n"); goto out; } diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c index ea3736c..6d5cf52 100644 --- a/lib/ext2fs/inline_data.c +++ b/lib/ext2fs/inline_data.c @@ -134,16 +134,16 @@ static int do_search_dir(ext2_filsys fs, void *start, unsigned int size, de = (struct ext2_dir_entry *)(start + offset); errcode = ext2fs_get_rec_len(fs, de, &rec_len); if (errcode) { +#ifndef OMIT_COM_ERR com_err("search_dir_inline_data", errcode, "while getting rec_len for inline data"); +#endif return errcode; } if (de->inode && len == (de->name_len & 0xFF) && - strncmp(name, de->name, len) == 0) { - printf("Entry found at inline data\n"); + strncmp(name, de->name, len) == 0) return 1; - } offset += rec_len; } return 0;