[patchset] quota fixes, cleanups and sysquotas for AIX

[Uri Simchoni]

On 2/13/19 2:08 PM, Björn JACKE via samba-technical wrote:
> Hi Uri,
> 
> On 2019-02-13 at 07:22 +0200 Uri Simchoni via samba-technical sent off:
>> I haven't gone through it all (and I currently don't know to what extent
>> I can review the JFS stuff) but I do want to flag the following:
> 
> If you think you can't then someone here at SerNet can review that also, we
> have an AIX machine for Samba development. Just the order of patches matters
> obviously here.
> 
>> patch 12/13 might break the build for HP-UX, because maybe (I'm not
>> sure...) their configure sets WITH_QUOTAS to 1 and then, with this
>> patch, provides no disk_quotas() function. Perhaps rather than removing
>> the whole function, you can remove the AIX parts only.
>>
>> I don't have positive evidence that HP-UX has disk-free quota support
>> integrated into Samba, but we've had reports and bugfixes for HP-UX in
>> recent versions of Samba.
> 
> HP-UX is what sysquotas_4A is for and there is no other platform anymore using
> the old quota code here.
> 
> 
>> In general we have tests/oldquotas.c for configure-time test of quota
>> support in pre-sysquotas systems, you may want to clean up AIX there too.
> 
> this will be addressed soon in a 2nd patchset, which will implement sysquotas
> for Solaris also, which is after this patch set the last system which is still
> using the old quota code.  I have the patch removing oldquotas.c and friends
> already waiting here. It's really time that we get rid of the legacy quota
> interface and have clean sysquotas generally in Samba.
> 
> Björn
> 

Ack on the HP-UX thing.

1/13 - RB+ me.
2/13 - RB+ me (the "else" branch is dead code IMHO but it's not obvious
so I think it's good to keep it)
3/13 - RB+ me
4/13 - RB+ me
5/13 - RB+ me
6/13 - A consequence of this change is that setting the
storage-quota/inode-limit ratio depends on compile-time platform, which
is probably not desirable (see vfs_set_ntquota() and how inode limit is
calculated). Perhaps the ratio should have its own constant (I'm not
sure what's the rationale of allowing 2K per inode in the original code).

Also:
> +#elif defined(HAVE_UFS_UFS_QUOTA_H)           /* BSDs */
> +#define QUOTABLOCK_SIZE 512

In the 4B code before change, DEV_BSIZE is used. Maybe 512 is just as
good...

7/13 - RB+ me
8/13:
> +#ifdef HAVE_STRUCT_DQBLK_DQB_CURBYTES
> +/* we handle the byte vs. block count dynamically via QUOTABLOCK_SIZE 1 */
> +#define dqb_curbytes dqb_curblocks
> +#endif
> +

Shouldn't it be the other way around? Our code is using dqb_curblocks,
and Darwin recognizes dqb_curbytes...

> +#ifdef HAVE_STRUCT_DQBLK_DQB_CURBYTES
> +	/* we want Darwin 0 byte files not to take 0 space, set a minimum of 1 here */
> +	if (dp->curblocks == 0) {
> +		dp->curblocks = 1;
> +	}

I don't get this - what's wrong with taking zero space?

9/13 - JFS - I suppose it works so RB+ me

10/13 JFS2: - looks OK. I wish we were more consistent in handling of
the QUOTAS_ENABLED / QUOTAS_DENY_DISK flags - some drivers return an
error to signal they are not supported and some return success with
flags cleared. That'll wait for another day I guess...

11/13 - RB+ me

12/13 RB+ me

13/13:
> +		/* let's see if user quota or group quota is more limiting: */
> +		if (*dsize > D.softlimit) {
> +			*dfree = D.softlimit - D.curblocks;
> +			*dsize = D.softlimit;
> +		

My concern here is that we count on the caller to set *dsize to 0 before
calling us, which is not expected for an output parameter. If the user
quota check fails, and goto try_group_quota is executed, then we don't
set *dsize, and then compare whatever's there. It so happens that the
caller initializes this to zero because of Samba coding conventions, but
I don't think this convention is universal enough. To honor the UNIX
tradition that out parameters are not modified on error we need a local
variable set to 0, and then set to user disk size if that's successful.

It would also be nice to add tests for this case where both group and
user quota are defined - see test_dfree_quota.sh - I think it amounts to
adding a couple of lines, no need for additional infrastructure. Best
thing is added test + knownfail first, then fix and removal from knownfail.

Thanks,
Uri.