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

Nikolay Borisov
2021-11-22T09:32:42Z

On 22.11.21 г. 10:32, Sidong Yang wrote:
> On Mon, Nov 22, 2021 at 09:20:00AM +0200, Nikolay Borisov wrote:
>>
>>
>> On 21.11.21 г. 17:15, Sidong Yang wrote:
>>> 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 <realwakka@gmail.com>
>>
>>
>> The code itself is fine so :
>>
>>
>> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
>>
>>
>> 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.

>>
>>
>>> ---
>>> 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) {
>>>
>