Re: libquota proposal

[David Holland <dholland-tech%netbsd.org@localhost>]

On Fri, Mar 18, 2011 at 08:30:41PM +0100, Manuel Bouyer wrote:
 > looking at pkgsrc/net/netatalk to make it use the new quota interface
 > convinced be that we need a libquota, which can return the quota status
 > for a id in a simple way, as independant as possible from the underlying
 > filesystem. So I propose to hide the magic in a libquota,
 > which propose a:
 > int getfsquota(const char *path, struct ufs_quota_entry *qv, uid_t id,
 >   const char *class)
                                           ^^^

Any fs-independent library should not be using fs-specific
structures.

 > (path is path to a file, not necesserely a mount point.
 > class is "user" or "group" at this time, as defined in sys/quota.h).

These should be constants, not strings.

 > For tools that care, a getufsquota() and getnfsquota() is also accessible
 > (it's free as they are needed for getfsquota() anyway).

This does not make sense. What about e.g. zfs, or whatever else comes
down the pipe? Also, what about fses that have more than (or less
than) just block and inode quotas?

 > ? common/include/quota
 > ? common/lib/libquota
 > ? lib/libquota

helps to use -N when generating diffs for people to read.

 > +bool                prop_array_add_and_rel(prop_array_t, prop_object_t);

I'm going to let someone else worry about whether this is a sensible
public proplib operation.

 > -    ufs_quota.c ufs_quota2.c quota2_subr.c
 > +    ufs_quota.c ufs_quota2.c quota2_subr.c 

noo! not trailing whitespace! :-)

 > --- sys/ufs/ufs/Makefile     6 Mar 2011 17:08:39 -0000       1.7
 > +++ sys/ufs/ufs/Makefile     18 Mar 2011 16:00:30 -0000
 > @@ -2,7 +2,7 @@
 >  
 >  INCSDIR= /usr/include/ufs/ufs
 >  
 > -INCS=       dinode.h dir.h extattr.h inode.h quota.h quota1.h quota2.h \
 > +INCS=       dinode.h dir.h extattr.h inode.h quota.h \
 >      ufs_bswap.h ufs_extern.h ufs_wapbl.h ufsmount.h
 >  
 >  .include <bsd.kinc.mk>

uh... that doesn't seem like a good idea.

 > Index: sys/ufs/ufs/quota.h
 > ===================================================================
 > RCS file: /cvsroot/src/sys/ufs/ufs/quota.h,v
 > retrieving revision 1.26
 > diff -u -r1.26 quota.h
 > --- sys/ufs/ufs/quota.h      6 Mar 2011 17:08:39 -0000       1.26
 > +++ sys/ufs/ufs/quota.h      18 Mar 2011 16:00:30 -0000
 > @@ -36,6 +36,7 @@
 >  
 >  #ifndef     _UFS_UFS_QUOTA_H_
 >  #define     _UFS_UFS_QUOTA_H_
 > +#include <quota/quotaprop.h>

er, in the kernel?

 > +__inline static int __unused
 > +ufsclass2qtype(int class)
 > +{
 > +    switch(class) {
 > +    case QUOTA_CLASS_USER:
 > +            return USRQUOTA;
 > +    case QUOTA_CLASS_GROUP:
 > +            return GRPQUOTA;
 > +    default:
 > +            return -1;
 > +    }
 > +}
 >  
 > +static __inline int __unused
 > +qtype2ufsclass(int type)
 > +{
 > +    switch(type) {
 > +    case USRQUOTA:
 > +            return QUOTA_CLASS_USER;
 > +    case GRPQUOTA:
 > +            return QUOTA_CLASS_GROUP;
 > +    default:
 > +            return -1;
 > +    }
 > +}

Surely these don't have to be exposed from the kernel?


.... I'm having a hard time getting a high-level view from the diffs.
I think I need to go poke around for a while.

-- 
David A. Holland
dholland%netbsd.org@localhost