diff mbox series

btrfs-progs: subvolume: Add warning on deleting default subvolume

Message ID 20200928150729.2239-1-realwakka@gmail.com
State New
Headers show
Series btrfs-progs: subvolume: Add warning on deleting default subvolume | expand

Commit Message

Sidong Yang Sept. 28, 2020, 3:07 p.m. UTC
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(+)

Comments

Sidong Yang Sept. 24, 2020, 12:45 p.m. UTC | #1
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) ?
>
David Sterba Sept. 29, 2020, 9:37 p.m. UTC | #2
On Mon, Sep 28, 2020 at 03:07:29PM +0000, Sidong Yang 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>

Thanks.

> ---
>  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) {
> +		ret = 1;
> +		goto out;
> +	}
> +
> +	if (subvolid > 0)
> +		target_subvol_id = subvolid;
> +	else {
> +		err = btrfs_util_subvolume_id(path, &target_subvol_id);
> +		if (fd < 0) {
> +			ret = 1;
> +			goto out;
> +		}
> +	}
> +
> +	if (target_subvol_id == default_subvol_id)
> +		warning("trying to delete default subvolume.");

I've changed that to skip the deletion and added id and path to the
message, otherwise it's not clear which one it was. Also I've added a
test.
Su Yue Sept. 30, 2020, 1:33 a.m. UTC | #3
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);
         ...
"?

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

> +			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) ?
David Sterba Sept. 30, 2020, 4:26 p.m. UTC | #4
On Thu, Sep 24, 2020 at 12:45:13PM +0000, Sidong Yang wrote:
> 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:
> > > +	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.
> 
> > > +		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?

Yes I'll fix it in the commit, no need to resend.
David Sterba Sept. 30, 2020, 4:27 p.m. UTC | #5
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> 
> > +	err = btrfs_util_get_default_subvolume_fd(fd, 
> > &default_subvol_id);
> > +	if (fd < 0) {
> >
> Should it be
> "     if (err) { 
> |
>          error_btrfs_util(err);
>          ...
> "?

> > +		err = btrfs_util_subvolume_id(path, &target_subvol_id);
> > +		if (fd < 0) {
> >
> And here.

Thanks for catching it, I missed it too.
diff mbox series

Patch

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) {
+		ret = 1;
+		goto out;
+	}
+
+	if (subvolid > 0)
+		target_subvol_id = subvolid;
+	else {
+		err = btrfs_util_subvolume_id(path, &target_subvol_id);
+		if (fd < 0) {
+			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) ?