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

[Sidong Yang <realwakka@gmail.com>]

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.
> 
> 
> > 
> > Graham
> >