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

[Sidong Yang <realwakka@gmail.com>]

On Mon, Nov 22, 2021 at 05:20:30PM +0200, Nikolay Borisov wrote:
> 
> 
> 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.

Thanks for a tip. I'll write next version of this patch.
> 
> >>
> >>
> >>>
> >>> Graham
> >>>
> >