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

[Nikolay Borisov <nborisov@suse.com>]

On 22.11.21 г. 17:10, Sidong Yang wrote:
> On Mon, Nov 22, 2021 at 12:57:18PM +0200, Nikolay Borisov wrote:
>>
>>
>> On 22.11.21 г. 11:53, Graham Cobb wrote:
>>>
>>> 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.
>>
>>
>> That actually works for me, I'd rather btrfs be as consistent as
>> possible and not give surprises to users. So just mimicking what an
>> omnipresent tool does is as good as it gets :)
> 
> Yeah, I understood that any error should be printed but no need to print
> that it is skipped. I agree. If so, I think the code that print error
> message should like below.
> 
> fprintf(stderr, "failed to walk dir/file: %s: %m\n", entry->d_name);
> 
> I agreed that btrfs command should be like "du" command that familiar
> with users. I wonder if I understood it well.
> 
> If so, I think it would be better that use switch-case than if-else.

let's be inline with du:

fprintf(stderr, "cannot access: '%s:' %m\n", entry->d_name);

Also %m works with the error in errno and in this case the error is in
ret, so you'd need to set errno to ret.

>>
>>
>>>
>>> Graham
>>>
>