Re: [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied

On 22.11.21 г. 10:32, Sidong Yang wrote:

On Mon, Nov 22, 2021 at 09:20:00AM +0200, Nikolay Borisov wrote:

quoted

On 21.11.21 г. 17:15, Sidong Yang wrote:

quoted

This patch handles issue #421. Filesystem du command fails and exit
when it access file that has permission denied. But it can continue the
command except the files. This patch recovers ret value when permission
denied.

Signed-off-by: Sidong Yang <redacted>

The code itself is fine so :


Reviewed-by: Nikolay Borisov <redacted>


OTOH when I looked at the code rather than just the patch I can't help
but wonder shouldn't we actually structure this the way you initially
proposed but also add a debug output along the lines of "skipping
file/dir XXXX due to permission denied", otherwise users might not be
able to account for some space and they can possibly wonder that
something is wrong with btrfs fi du command.

You mean that it would be better that print some debug message than
skipping silently. I agree. So I would add this code in condition.

fprintf(stderr, "skipping file/dir: %s : %m\n", entry->d_name);

I think it's okay that it prints when ENOTTY occurs. Is this code what
you meant?

I meant to print only if we have EACCESS, but now that I think about it,
printing something when we have a non-fatal error and simply skipping
some dirs/files makes sense. OTOH printing it by default might be too
verbose so perhaps usuing a pr_verbose call would be more appropriate.

This is one of those things which don't have a clear-cut answers so it's
useful to get as many perspective as possible to arrive at some workable
solution to a wider number of people.

quoted

quoted

---
 cmds/filesystem-du.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds/filesystem-du.c b/cmds/filesystem-du.c
index 5865335d..fb7753ca 100644
--- a/cmds/filesystem-du.c
+++ b/cmds/filesystem-du.c

@@ -403,7 +403,7 @@ static int du_walk_dir(struct du_dir_ctxt *ctxt, struct rb_root *shared_extents)
 						  dirfd(dirstream),
 						  shared_extents, &tot, &shr,
 						  0);
-				if (ret == -ENOTTY) {
+				if (ret == -ENOTTY || ret == -EACCES) {
 					ret = 0;
 					continue;
 				} else if (ret) {