linux-ext4 - Re: [PATCH V2 1/3] quota: Add reservation support for delayed block allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20081104171451.816d92d0.akpm@linux-foundation.org>
Date:	Tue, 4 Nov 2008 17:14:51 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Mingming Cao <cmm@...ibm.com>
Cc:	jack@...e.cz, tytso@....edu, linux-ext4@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH V2 1/3] quota: Add reservation support for delayed block
 allocation

On Fri, 31 Oct 2008 14:27:22 -0700
Mingming Cao <cmm@...ibm.com> wrote:

> +int dquot_alloc_space(struct inode *inode, qsize_t number, int warn)
> +{
> +	int cnt, ret = QUOTA_OK;
> +
> +	/*
> +	 * First test before acquiring mutex - solves deadlocks when we
> +         * re-enter the quota code and are already holding the mutex
> +         */
> +	if (IS_NOQUOTA(inode)) {
> +		inode_add_bytes(inode, number);
> +		return ret;
> +	}
> +
> +	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> +	if (IS_NOQUOTA(inode))	{
> +		/* Now we can do reliable test... */
> +		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> +		inode_add_bytes(inode, number);
> +		return ret;
> +	}
> +
> +	ret = __dquot_alloc_space(inode, number, warn, 0);
> +	if (ret == NO_QUOTA) {
> +		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> +		return ret;
> +	}
> +
> +	/* Dirtify all the dquots - this can block when journalling */
> +	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
> +		if (inode->i_dquot[cnt])
> +			mark_dquot_dirty(inode->i_dquot[cnt]);
> +	up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> +	inode_add_bytes(inode, number);
> +	return ret;
> +}

I'm going to have to call "ug" on that code.

Multiple return points per function really is a maintenance problem. 
It's a great source of code duplication, locking errors and resource
leaks as the code evolves.

Can we please rework this code (and any other similar code here) to use
the usual `goto out' pattern?


int dquot_alloc_space(struct inode *inode, qsize_t number, int warn)
{
	int cnt, ret = QUOTA_OK;

	/*
	 * First test before acquiring mutex - solves deadlocks when we
         * re-enter the quota code and are already holding the mutex
         */
	if (IS_NOQUOTA(inode))
		goto out;

	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
	if (IS_NOQUOTA(inode))	/* Now we can do reliable test... */
		goto out_unlock;

	ret = __dquot_alloc_space(inode, number, warn, 0);

	if (ret == NO_QUOTA) {
		up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
		return ret;
	}

<<

I don't know what to do here - did we really want to skip the
inode_add_bytes?  If so, we could do

	number = 0;
	goto out_unlock;

>>

	/* Dirtify all the dquots - this can block when journalling */
	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
		if (inode->i_dquot[cnt])
			mark_dquot_dirty(inode->i_dquot[cnt]);
out_unlock:
	up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
out:
	inode_add_bytes(inode, number);
	return ret;
}

or something like that.


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html