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

Graham Cobb
2021-11-22T09:54:29Z

On 22/11/2021 09:32, Nikolay Borisov wrote:
>
>
> 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.

I must admit I just assumed it worked the same way as /bin/du. I have
just created an inaccessible directory and got:

$ du -sh ~
du: cannot read directory '/home/cobb/permtest': Permission denied
61G /home/cobb

And when the directory was accessible but the file in it was not, I got:

$ du -sh ~
du: cannot access '/home/cobb/permtest/file': Permission denied
61G /home/cobb

In other words, I think any error should be printed but the error is
then skipped and the du continues. No need to tell people the file is
being skipped - that is obvious. But the error must be printed by
default (if there are really cases where the error should not be printed
but reasons not to redirect stderr to /dev/null then add an option to
suppress printing it).

Just my opinion.

Graham