Re: CVS commit: pkgsrc/net/netatalk3/patches

[coypu%SDF.ORG@localhost]

Hi christos!

having patches like patch-etc_afpd_quota.c without a comment in the patch
is a little problematic for other people who may interact with the package.

it appears to be changing runtime behaviour to a more correct one.

even if you sent this upstream right away, the next person to update the
package will need to remove this patch, which is something requiring
confidence that it hasn't broken anything, and from a brief view, it's
not clear how to test that the functionality still works.

(although there is some chance it will show as a reversed patch, so
easier to understand it just needs to be removed)

could you leave a comment explaining at least what it fixes?

thanks

On Sun, Nov 06, 2016 at 07:41:58PM -0500, Christos Zoulas wrote:
> Index: pkgsrc/net/netatalk3/patches/patch-etc_afpd_quota.c
> diff -u /dev/null pkgsrc/net/netatalk3/patches/patch-etc_afpd_quota.c:1.1
> --- /dev/null Sun Nov  6 19:41:58 2016
> +++ pkgsrc/net/netatalk3/patches/patch-etc_afpd_quota.c       Sun Nov  6 19:41:57 2016
> @@ -0,0 +1,161 @@
> +$NetBSD: patch-etc_afpd_quota.c,v 1.1 2016/11/07 00:41:57 christos Exp $
> +
> +--- etc/afpd/quota.c.orig    2013-04-09 12:56:18.000000000 +0000
> ++++ etc/afpd/quota.c
> +@@ -36,10 +36,13 @@
> + 
> + static int
> + getfreespace(const AFPObj *obj, struct vol *vol, VolSpace *bfree, VolSpace *btotal,
> +-         uid_t uid, const char *classq)
> ++         id_t id, int idtype)
> + {
> +-    int retq;
> +-    struct ufs_quota_entry ufsq[QUOTA_NLIMITS];
> ++    uid_t prevuid;
> ++    const char *msg;
> ++    struct quotahandle *qh;
> ++    struct quotakey qk;
> ++    struct quotaval qv;
> +     time_t now;
> + 
> +     if (time(&now) == -1) {
> +@@ -48,64 +51,102 @@ getfreespace(const AFPObj *obj, struct v
> +             return -1;
> +     }
> + 
> ++    prevuid = geteuid();
> ++    if (prevuid == -1) {
> ++            LOG(log_info, logtype_afpd, "geteuid(): %s",
> ++                strerror(errno));
> ++            return -1;
> ++    }
> ++
> +     become_root();
> + 
> +-    if ((retq = getfsquota(obj, vol, ufsq, uid, classq)) < 0) {
> +-            LOG(log_info, logtype_afpd, "getfsquota(%s, %s): %s",
> +-                vol->v_path, classq, strerror(errno));
> +-    }
> +-
> +-    unbecome_root();
> +-
> +-    if (retq < 1)
> +-            return retq;
> +-
> +-    switch(QL_STATUS(quota_check_limit(ufsq[QUOTA_LIMIT_BLOCK].ufsqe_cur, 1,
> +-        ufsq[QUOTA_LIMIT_BLOCK].ufsqe_softlimit,
> +-        ufsq[QUOTA_LIMIT_BLOCK].ufsqe_hardlimit,
> +-        ufsq[QUOTA_LIMIT_BLOCK].ufsqe_time, now))) {
> +-    case QL_S_DENY_HARD:
> +-    case QL_S_DENY_GRACE:
> ++    /*
> ++     * In a tidier world we might keep the quotahandle open for longer...
> ++     */
> ++    qh = quota_open(vol->v_path);
> ++    if (qh == NULL) {
> ++            if (errno == EOPNOTSUPP || errno == ENXIO) {
> ++                    /* no quotas on this volume */
> ++                    seteuid( prevuid );
> ++                    return 0;
> ++            }
> ++       
> ++            LOG(log_info, logtype_afpd, "quota_open(%s): %s", vol->v_path,
> ++                strerror(errno));
> ++            seteuid( prevuid );
> ++            return -1;
> ++    }
> ++    qk.qk_idtype = idtype;
> ++    qk.qk_id = id;
> ++    qk.qk_objtype = QUOTA_OBJTYPE_BLOCKS;
> ++    if (quota_get(qh, &qk, &qv) < 0) {
> ++            if (errno == ENOENT) {
> ++                    /* no quotas for this id */
> ++                    quota_close(qh);
> ++                    seteuid( prevuid );
> ++                    return 0;
> ++            }
> ++            msg = strerror(errno);
> ++            LOG(log_info, logtype_afpd, "quota_get(%s, %s): %s",
> ++                vol->v_path, quota_idtype_getname(qh, idtype), msg);
> ++            quota_close(qh);
> ++            seteuid( prevuid );
> ++            return -1;
> ++    }
> ++
> ++    quota_close(qh);
> ++
> ++     seteuid( prevuid );
> ++
> ++    if (qv.qv_usage >= qv.qv_hardlimit ||
> ++            (qv.qv_usage >= qv.qv_softlimit && now > qv.qv_expiretime)) {
> ++
> +             *bfree = 0;
> +-            *btotal = dbtob(ufsq[QUOTA_LIMIT_BLOCK].ufsqe_cur);
> +-            break;
> +-    default:
> +-            *bfree = dbtob(ufsq[QUOTA_LIMIT_BLOCK].ufsqe_hardlimit -
> +-                ufsq[QUOTA_LIMIT_BLOCK].ufsqe_cur);
> +-            *btotal = dbtob(ufsq[QUOTA_LIMIT_BLOCK].ufsqe_hardlimit);
> +-            break;
> ++            *btotal = dbtob(qv.qv_usage);
> ++    }
> ++    else {
> ++            *bfree = dbtob(qv.qv_hardlimit - qv.qv_usage);
> ++            *btotal = dbtob(qv.qv_hardlimit);
> ++    }
> ++
> +     }
> +     return 1;
> + }
> + 
> + int uquota_getvolspace(const AFPObj *obj, struct vol *vol, VolSpace *bfree, VolSpace *btotal, const u_int32_t bsize)
> + {
> +-    int uretq, gretq;
> ++    int uret, gret;
> +     VolSpace ubfree, ubtotal;
> +     VolSpace gbfree, gbtotal;
> ++    uret = getfreespace(vol, &ubfree, &ubtotal,
> ++        uuid, QUOTA_IDTYPE_USER);
> ++    if (uret == 1) {
> ++            LOG(log_info, logtype_afpd, "quota_get(%s, user): %d %d",
> ++                vol->v_path, (int)ubfree, (int)ubtotal);
> ++    }
> + 
> +-    uretq = getfreespace(obj, vol, &ubfree, &ubtotal,
> +-                         uuid, QUOTADICT_CLASS_USER);
> +-    LOG(log_info, logtype_afpd, "getfsquota(%s): %d %d",
> +-        vol->v_path, (int)ubfree, (int)ubtotal);
> +     if (obj->ngroups >= 1) {
> +-            gretq = getfreespace(vol, &ubfree, &ubtotal,
> +-                obj->groups[0], QUOTADICT_CLASS_GROUP);
> ++            gret = getfreespace(vol, &gbfree, &gbtotal,
> ++                groups[0], QUOTA_IDTYPE_GROUP);
> ++            if (gret == 1) {
> ++                    LOG(log_info, logtype_afpd, "quota_get(%s, group): %d %d",
> ++                        vol->v_path, (int)gbfree, (int)gbtotal);
> ++            }
> +     } else
> +-            gretq = -1;
> +-    if (uretq < 1 && gretq < 1) { /* no quota for this fs */
> ++            gret = 0;
> ++    if (uret < 1 && gret < 1) { /* no quota for this fs */
> +             return AFPERR_PARAM;
> +     }
> +-    if (uretq < 1) {
> +-            /* use group quotas */
> ++    if (uret < 1) {
> ++            /* no user quotas, but group quotas; use them */
> +             *bfree = gbfree;
> +             *btotal = gbtotal;
> +-    } else if (gretq < 1) {
> +-            /* use user quotas */
> ++    } else if (gret < 1) {
> ++            /* no group quotas, but user quotas; use them */
> +             *bfree = ubfree;
> +             *btotal = ubtotal;
> +     } else {
> +-            /* return smallest remaining space of user and group */
> ++            /* both; return smallest remaining space of user and group */
> +             if (ubfree < gbfree) {
> +                     *bfree = ubfree;
> +                     *btotal = ubtotal;