[PATCH] honor group quota if user quota also exists

[Uri Simchoni]

On 2/1/19 2:07 AM, Björn JACKE via samba-technical wrote:
> Hi,
> 
> review appreciated...
> 
> Björn
> 

It's not quite clear to me what the algorithm should be:
- Do we present free disk based on which one (user/group) has less free
disk?
- Do we present free disk based on which one (user/group) has less quota?

The argument for the first way is that it reflects the free disk, The
argument for the second way is that the "disk size" is stable.

It makes me think that maybe the code should remain as-is, and recommend
to set either user quota *or* group quota, but not both.

Having said that, I assume we're using the second method, because that's
what the code seems to be trying to do, and because I don't like the
disk size to be "jumping around" if files are created/destroyed,
although it may still produce free-disk that's not accurate.

I also think it's quite easy to add a test for this - we have
test_dfree_quota.sh which can probably be extended to handle this case.

Finally I have some comments on the fix itself - see below.

> diff --git a/source3/smbd/quotas.c b/source3/smbd/quotas.c
> index c947268..8c89b37 100644
> --- a/source3/smbd/quotas.c
> +++ b/source3/smbd/quotas.c
> @@ -364,6 +364,7 @@ bool disk_quotas(connection_struct *conn, struct smb_filename *fname,
>  	int r;
>  	SMB_DISK_QUOTA D;
>  	unid_t id;
> +	bool ret_user_quota = False;

We use true/false now - this routine already has a mix of old and new
style, so better (IMHO) to use new style and maybe follow up with a
patch to convert the rest.

>  
>  	/*
>  	 * First of all, check whether user quota is
> @@ -428,7 +429,7 @@ bool disk_quotas(connection_struct *conn, struct smb_filename *fname,
>  		*dsize = D.softlimit;
>  	}
>  
> -	return True;
> +	ret_user_quota = True;
>  	
>  try_group_quota:
>  	/*
> @@ -441,10 +442,10 @@ try_group_quota:
>  	r = SMB_VFS_GET_QUOTA(conn, fname, SMB_GROUP_FS_QUOTA_TYPE,
>  			      id, &D);
>  	if (r == -1 && errno != ENOSYS) {
> -		return false;
> +		return ret_user_quota;
>  	}
>  	if (r == 0 && (D.qflags & QUOTAS_DENY_DISK) == 0) {
> -		return false;
> +		return ret_user_quota;
>  	}
>  
>  	id.gid = getegid();
> @@ -454,7 +455,7 @@ try_group_quota:
>  			      &D);
>  
>  	if (r == -1) {
> -		return False;
> +		return ret_user_quota;
>  	}
>  
>  	*bsize = D.bsize;
> @@ -468,13 +469,16 @@ try_group_quota:
>  		*dfree = 0;
>  		*dsize = D.curblocks;
>  	} else if (D.softlimit==0 && D.hardlimit==0) {
> -		return False;
> +		return ret_user_quota;
>  	} else {
>  		if (D.softlimit == 0) {
>  			D.softlimit = D.hardlimit;
>  		}
> -		*dfree = D.softlimit - D.curblocks;
> -		*dsize = D.softlimit;
> +		/* let's see if user quota or group quota is more limiting: */
> +		if (*dfree > D.softlimit) {

That should be *dsize right?

Additionally, if the user quota wasn't successful, *dsize/*dfree are not
initialized (at least by this function). Perhaps a better approach would be:
1. get the group quota
2. decide which of the two to use
3. set all three output parameters based on the chosen quota

> +			*dfree = D.softlimit - D.curblocks;
> +			*dsize = D.softlimit;
> +		}
>  	}
>  
>  	return (True);

Thanks,
Uri.