Re: [PATCH] btrfs-progs: subvolume: Add warning on deleting default subvolume

[Sidong Yang <realwakka@gmail.com>]

On Wed, Sep 30, 2020 at 09:33:31AM +0800, Su Yue wrote:
> 
> On Mon 28 Sep 2020 at 23:07, Sidong Yang <realwakka@gmail.com> wrote:
> 
> > This patch add warning messages when user try to delete default
> > subvolume. When deleting default subvolume, kernel will not allow and
> > make error message on syslog. but there is only message that permission
> > denied on userspace. User can be noticed the reason by this warning
> > message.
> > 
> > This patch implements github issue.
> > https://github.com/kdave/btrfs-progs/issues/274
> > 
> > Signed-off-by: Sidong Yang <realwakka@gmail.com>
> > ---
> >  cmds/subvolume.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/cmds/subvolume.c b/cmds/subvolume.c
> > index 2020e486..0cdf7a68 100644
> > --- a/cmds/subvolume.c
> > +++ b/cmds/subvolume.c
> > @@ -264,6 +264,7 @@ static int cmd_subvol_delete(const struct cmd_struct
> > *cmd,
> >  	struct seen_fsid *seen_fsid_hash[SEEN_FSID_HASH_SIZE] = {  NULL, };
> >  	enum { COMMIT_AFTER = 1, COMMIT_EACH = 2 };
> >  	enum btrfs_util_error err;
> > +	uint64_t default_subvol_id = 0, target_subvol_id = 0;
> > 
> >  	optind = 0;
> >  	while (1) {
> > @@ -360,6 +361,25 @@ again:
> >  		goto out;
> >  	}
> > 
> > +	err = btrfs_util_get_default_subvolume_fd(fd, &default_subvol_id);
> > +	if (fd < 0) {
> > 
> Should it be
> "     if (err) { |
>         error_btrfs_util(err);
>         ...
> "?

Hi Su! Thanks for review!

Yeah, I think it's definitely wrong. My mistake.

> 
> > +		ret = 1;
> > +		goto out;
> > +	}
> > +
> > +	if (subvolid > 0)
> > +		target_subvol_id = subvolid;
> > +	else {
> > +		err = btrfs_util_subvolume_id(path, &target_subvol_id);
> > +		if (fd < 0) {
> > 
> And here.
> 

It's wrong too.

Dave, maybe this patch needs for Su's review.
Usually in this case, Could you fix it directly ?
or do I need to send a new patch?

Thanks,
Sidong

> > +			ret = 1;
> > +			goto out;
> > +		}
> > +	}
> > +
> > +	if (target_subvol_id == default_subvol_id)
> > +		warning("trying to delete default subvolume.");
> > +
> >  	pr_verbose(MUST_LOG, "Delete subvolume (%s): ",
> >  		commit_mode == COMMIT_EACH ||
> >  		(commit_mode == COMMIT_AFTER && cnt + 1 == argc) ?
>