[PATCH] libext2fs: do not print any error message from libext2fs

[PATCH] libext2fs: do not print any error message from libext2fs

[Zheng Liu]

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>
---
[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

Re: [PATCH] libext2fs: do not print any error message from libext2fs

[Zheng Liu]

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
> 

Re: [PATCH] libext2fs: do not print any error message from libext2fs

[Theodore Ts'o]

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

Re: [PATCH] libext2fs: do not print any error message from libext2fs

[Zheng Liu]

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

My current version of the inline data patches

[Theodore Ts'o]

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

Re: My current version of the inline data patches

[Zheng Liu]

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