[PATCH][TAKE-1] fs/btrfs: Return EPERM for rmdir on subvolumes and snapshots

[PATCH][TAKE-1] fs/btrfs: Return EPERM for rmdir on subvolumes and snapshots

[Harshavardhana]

Break the conditional to return EPERM for subvolumes,snapshots and 
ENOTEMPTY for normal directories with files.

Signed-off-by: Harshavardhana <harsha@gluster.com>
---
 fs/btrfs/inode.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a85b90c..465c3de 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -2591,9 +2591,11 @@ static int btrfs_rmdir(struct inode *dir, struct dentry *dentry)
 	struct btrfs_trans_handle *trans;
 	unsigned long nr = 0;
 
-	if (inode->i_size > BTRFS_EMPTY_DIR_SIZE ||
-	    inode->i_ino == BTRFS_FIRST_FREE_OBJECTID)
-		return -ENOTEMPTY;
+	if (inode->i_size > BTRFS_EMPTY_DIR_SIZE) 
+                return -ENOTEMPTY;
+
+        if (inode->i_ino == BTRFS_FIRST_FREE_OBJECTID)
+		return -EPERM;
 
 	ret = btrfs_reserve_metadata_space(root, 5);
 	if (ret)
-- 
1.6.6.1

Re: [PATCH][TAKE-1] fs/btrfs: Return EPERM for rmdir on subvolumes and snapshots

[Goffredo Baroncelli]

Hi all,

Can I suggest to return -EINVAL instead of -EPERM ?
To me EPERM seems that the user don't have the right to perform an action. But 
the problem is that "rm" is not the right command to use in order to delete a 
subvolume.

As side note, what is the reason for which an user is able to create a 
subvolume, but not to destroy it ?

BR
Goffredo

On Friday 09 April 2010, Harshavardhana wrote:
> Break the conditional to return EPERM for subvolumes,snapshots and 
> ENOTEMPTY for normal directories with files.
> 
> Signed-off-by: Harshavardhana <harsha@gluster.com>
> ---
>  fs/btrfs/inode.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index a85b90c..465c3de 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2591,9 +2591,11 @@ static int btrfs_rmdir(struct inode *dir, struct 
dentry *dentry)
>  	struct btrfs_trans_handle *trans;
>  	unsigned long nr = 0;
>  
> -	if (inode->i_size > BTRFS_EMPTY_DIR_SIZE ||
> -	    inode->i_ino == BTRFS_FIRST_FREE_OBJECTID)
> -		return -ENOTEMPTY;
> +	if (inode->i_size > BTRFS_EMPTY_DIR_SIZE) 
> +                return -ENOTEMPTY;
> +
> +        if (inode->i_ino == BTRFS_FIRST_FREE_OBJECTID)
> +		return -EPERM;
>  
>  	ret = btrfs_reserve_metadata_space(root, 5);
>  	if (ret)
> -- 
> 1.6.6.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijackATinwind.it>
Key fingerprint = 4769 7E51 5293 D36C 814E  C054 BF04 F161 3DC5 0512

Re: [PATCH][TAKE-1] fs/btrfs: Return EPERM for rmdir on subvolumes and snapshots

[Harshavardhana]

On 04/09/2010 10:58 AM, Goffredo Baroncelli wrote:
> Can I suggest to return -EINVAL instead of -EPERM ?
> To me EPERM seems that the user don't have the right to perform an action. But
> the problem is that "rm" is not the right command to use in order to delete a
> subvolume.
>
> As side note, what is the reason for which an user is able to create a
> subvolume, but not to destroy it ?
>
> BR
> Goffredo
>
>    
We can decide on that,  but let me explaing in more detail.

 From what i understood is that since when you are not allowed to delete 
the volume or a snapshot by "rmdir" (conventional means). Then it is 
perfectly safe to call it EPERM as it would be as if suggesting 
"pathname" doesn't support removal of directories.

EINVAL is a for other needs which i feel is not suitable in the current 
case.

There is another scenario of what if btrfs_rmdir itself can call subvol 
removal ioctl for snapshots and subvolumes in case someone issues a 
rmdir on such directories.

Suggestions please.

Regards

-- 
Harshavardhana
http://www.gluster.com

Re: [PATCH][TAKE-1] fs/btrfs: Return EPERM for rmdir on subvolumes and snapshots

[Goffredo Baroncelli]

Ok,

I read the unlink (2) man page which says:

[...]
       EBUSY (not on Linux)
              The file pathname cannot be unlinked because it is being used by
              the system or another process and the  implementation  considers
              this an error.

[...]
       EPERM  The system does not allow unlinking of directories, or unlinking
              of directories requires  privileges  that  the  calling  process
              doesn't  have.   (This  is the POSIX prescribed error return; as
              noted above, Linux returns EISDIR for this case.)

       EPERM (Linux only)
              The file system does not allow unlinking of files.

[...]

In fact when I tried to unlink a directory where a filesystem is mounted, I 
got -EBUSY. So for consistency EBUSY may be another error which may be 
returned.


On Friday 09 April 2010, Harshavardhana wrote:
> On 04/09/2010 10:58 AM, Goffredo Baroncelli wrote:
> > Can I suggest to return -EINVAL instead of -EPERM ?
> > To me EPERM seems that the user don't have the right to perform an action. 
But
> > the problem is that "rm" is not the right command to use in order to 
delete a
> > subvolume.
> >
> > As side note, what is the reason for which an user is able to create a
> > subvolume, but not to destroy it ?
> >
> > BR
> > Goffredo
> >
> >    
> We can decide on that,  but let me explaing in more detail.
> 
>  From what i understood is that since when you are not allowed to delete 
> the volume or a snapshot by "rmdir" (conventional means). Then it is 
> perfectly safe to call it EPERM as it would be as if suggesting 
> "pathname" doesn't support removal of directories.
> 
> EINVAL is a for other needs which i feel is not suitable in the current 
> case.
> 
> There is another scenario of what if btrfs_rmdir itself can call subvol 
> removal ioctl for snapshots and subvolumes in case someone issues a 
> rmdir on such directories.
> 
> Suggestions please.
> 
> Regards
> 
> -- 
> Harshavardhana
> http://www.gluster.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijack@inwind.it>
Key fingerprint = 4769 7E51 5293 D36C 814E  C054 BF04 F161 3DC5 0512

Re: [PATCH][TAKE-1] fs/btrfs: Return EPERM for rmdir on subvolumes and snapshots

[Harshavardhana]

On 04/09/2010 11:54 AM, Goffredo Baroncelli wrote:
>         EBUSY (not on Linux)
>                The file pathname cannot be unlinked because it is being used by
>                the system or another process and the  implementation  considers
>                this an error.
>
> [...]
>         EPERM  The system does not allow unlinking of directories, or unlinking
>                of directories requires  privileges  that  the  calling  process
>                doesn't  have.   (This  is the POSIX prescribed error return; as
>                noted above, Linux returns EISDIR for this case.)
>
>         EPERM (Linux only)
>                The file system does not allow unlinking of files.
>
> [...]
>
> In fact when I tried to unlink a directory where a filesystem is mounted, I
> got -EBUSY. So for consistency EBUSY may be another error which may be
> returned.
>    

EBUSY is again meant for different reason where in a super block is 
being locked or accessed by an Application which would mean unref on 
that block would cause Application to go nuts. In such cases EBUSY is 
returned.


-- 

Harshavardhana
http://www.gluster.com

Re: [PATCH][TAKE-1] fs/btrfs: Return EPERM for rmdir on subvolumes and snapshots

[Harshavardhana]

On 04/09/2010 02:07 PM, Harshavardhana wrote:
> EBUSY is again meant for different reason where in a super block is 
> being locked or accessed by an Application which would mean unref on 
> that block would cause Application to go nuts. In such cases EBUSY is 
> returned. 
Ok i think ENOTSUPP could be another alternative? . This should be ok i 
guess?

Regards

-- 
Harshavardhana
http://www.gluster.com

Re: [PATCH][TAKE-1] fs/btrfs: Return EPERM for rmdir on subvolumes and snapshots

[Goffredo Baroncelli]

On Friday 09 April 2010, Harshavardhana wrote:
> On 04/09/2010 02:07 PM, Harshavardhana wrote:
> > EBUSY is again meant for different reason where in a super block is 
> > being locked or accessed by an Application which would mean unref on 
> > that block would cause Application to go nuts. In such cases EBUSY is 
> > returned. 
> Ok i think ENOTSUPP could be another alternative? . This should be ok i 
> guess?


I prefer ENOTSUP and EINVAL to EPERM. 
But the man page of unlink (2) doesn't cite nor EINVAL nor ENOTSUP.

So at this point I have to admit that EPERM could be the best compromise.

But let me highlight that unlink(2) returns -EBUSY when an user try to delete 
a mount-point. This is quite similar to unlink a subvolume.

So my list in order of preference is

1) EBUSY                   -> because it is used when unlink is called on a
			      mount-point and it is in the unlink (2) man page
2) EPERM		   -> because it is in the unlink man page, and it was
                              already discussed
3) ENOTSUP, EINVAL	   -> to me these make a lot of sense. But the man
			      page doesn't cite both of them

BR
Goffredo

> Regards



> -- 
> Harshavardhana
> http://www.gluster.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijackATinwind.it>
Key fingerprint = 4769 7E51 5293 D36C 814E  C054 BF04 F161 3DC5 0512