* [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied @ 2021-11-21 15:15 Sidong Yang 2021-11-22 7:20 ` Nikolay Borisov 0 siblings, 1 reply; 9+ messages in thread From: Sidong Yang @ 2021-11-21 15:15 UTC (permalink / raw) To: linux-btrfs, Nikolay Borisov; +Cc: Sidong Yang 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> --- 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) { -- 2.25.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied 2021-11-21 15:15 [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied Sidong Yang @ 2021-11-22 7:20 ` Nikolay Borisov 2021-11-22 8:32 ` Sidong Yang 0 siblings, 1 reply; 9+ messages in thread From: Nikolay Borisov @ 2021-11-22 7:20 UTC (permalink / raw) To: Sidong Yang, linux-btrfs, David Sterba 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. > --- > 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) { > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied 2021-11-22 7:20 ` Nikolay Borisov @ 2021-11-22 8:32 ` Sidong Yang 2021-11-22 9:32 ` Nikolay Borisov 0 siblings, 1 reply; 9+ messages in thread From: Sidong Yang @ 2021-11-22 8:32 UTC (permalink / raw) To: Nikolay Borisov; +Cc: linux-btrfs, David Sterba 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? > > > > --- > > 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) { > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied 2021-11-22 8:32 ` Sidong Yang @ 2021-11-22 9:32 ` Nikolay Borisov 2021-11-22 9:53 ` Graham Cobb 0 siblings, 1 reply; 9+ messages in thread From: Nikolay Borisov @ 2021-11-22 9:32 UTC (permalink / raw) To: Sidong Yang; +Cc: linux-btrfs, David Sterba 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) { >>> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied 2021-11-22 9:32 ` Nikolay Borisov @ 2021-11-22 9:53 ` Graham Cobb 2021-11-22 10:57 ` Nikolay Borisov 0 siblings, 1 reply; 9+ messages in thread From: Graham Cobb @ 2021-11-22 9:53 UTC (permalink / raw) To: Nikolay Borisov, Sidong Yang; +Cc: linux-btrfs, David Sterba 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied 2021-11-22 9:53 ` Graham Cobb @ 2021-11-22 10:57 ` Nikolay Borisov 2021-11-22 15:10 ` Sidong Yang 0 siblings, 1 reply; 9+ messages in thread From: Nikolay Borisov @ 2021-11-22 10:57 UTC (permalink / raw) To: Graham Cobb, Sidong Yang; +Cc: linux-btrfs, David Sterba 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 :) > > Graham > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied 2021-11-22 10:57 ` Nikolay Borisov @ 2021-11-22 15:10 ` Sidong Yang 2021-11-22 15:20 ` Nikolay Borisov 0 siblings, 1 reply; 9+ messages in thread From: Sidong Yang @ 2021-11-22 15:10 UTC (permalink / raw) To: Nikolay Borisov; +Cc: Graham Cobb, linux-btrfs, David Sterba 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied 2021-11-22 15:10 ` Sidong Yang @ 2021-11-22 15:20 ` Nikolay Borisov 2021-11-22 15:51 ` Sidong Yang 0 siblings, 1 reply; 9+ messages in thread From: Nikolay Borisov @ 2021-11-22 15:20 UTC (permalink / raw) To: Sidong Yang; +Cc: Graham Cobb, linux-btrfs, David Sterba 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 >>> > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied 2021-11-22 15:20 ` Nikolay Borisov @ 2021-11-22 15:51 ` Sidong Yang 0 siblings, 0 replies; 9+ messages in thread From: Sidong Yang @ 2021-11-22 15:51 UTC (permalink / raw) To: Nikolay Borisov; +Cc: Graham Cobb, linux-btrfs, David Sterba 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 > >>> > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-11-22 15:51 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-21 15:15 [PATCH v2] btrfs-progs: filesystem: du: skip file that permission denied Sidong Yang 2021-11-22 7:20 ` Nikolay Borisov 2021-11-22 8:32 ` Sidong Yang 2021-11-22 9:32 ` Nikolay Borisov 2021-11-22 9:53 ` Graham Cobb 2021-11-22 10:57 ` Nikolay Borisov 2021-11-22 15:10 ` Sidong Yang 2021-11-22 15:20 ` Nikolay Borisov 2021-11-22 15:51 ` Sidong Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox