xfs-masters
[Top] [All Lists]

[xfs-masters] Re: [PATCH] XFS: remove pointless conditional testing 'nmp

To: "Nathan Scott" <nathans@xxxxxxx>
Subject: [xfs-masters] Re: [PATCH] XFS: remove pointless conditional testing 'nmp' vs NULL in fs/xfs/xfs_rtalloc.c::xfs_growfs_rt()
From: "Jesper Juhl" <jesper.juhl@xxxxxxxxx>
Date: Mon, 14 Aug 2006 09:25:53 +0200
Cc: linux-kernel@xxxxxxxxxxxxxxx, xfs-masters@xxxxxxxxxxx, xfs@xxxxxxxxxxx
Domainkey-signature: a=rsa-sha1; q=dns; c=nofws; s=beta; d=gmail.com; h=received:message-id:date:from:to:subject:cc:in-reply-to:mime-version:content-type:content-transfer-encoding:content-disposition:references; b=as0DO8vyvsjkyPMWS0rPkCw7GZeLfuVr3oZpOh/9JEgaeFVZKPGm3jyoGvQBg0CAg/2MNh6pjV3ACzSldkf9NXmcJjg3/01qfnCALLcXSS7nShByvzeNiKSMjunEhyHham8hNcyZ4fXVVY6m01eKCHqz6tF6BevV1bKOUKIjClI=
In-reply-to: <20060814110942.C2698880@xxxxxxxxxxxxxxxxxxxxxxxx>
References: <200608130016.51136.jesper.juhl@xxxxxxxxx> <20060814110942.C2698880@xxxxxxxxxxxxxxxxxxxxxxxx>
Reply-to: xfs-masters@xxxxxxxxxxx
Sender: xfs-masters-bounce@xxxxxxxxxxx
On 14/08/06, Nathan Scott <nathans@xxxxxxx> wrote:
> On Sun, Aug 13, 2006 at 12:16:50AM +0200, Jesper Juhl wrote:
> > In fs/xfs/xfs_rtalloc.c::xfs_growfs_rt() there's a completely useless
> > conditional at the error_exit label.
> > The 'if (nmp)' check is pointless and might as well be removed for two
> > reasons.
> >
> > 1) if 'nmp' is NULL then kmem_free() will end up calling kfree() with a NULL
> >    argument - which in turn will just cause a return from kfree(). No harm
> >    done.
>
> Thats valid.
>
> > 2) At the beginning of the function there's an assignment; '*nmp = *mp;' so
>
> Thats not.  Theres no assignment at the start of the function;
> theres one inside the main body of the loop 20+ lines into it,
> and right after a mem alloc with flags requiring no failure.
> Later that local variable is freed then set to NULL inside the
> loop, before continuing the next iteration...
>
> Really this code would be better if reworked slightly to just
> allocate nmp once before entering the loop, and then free it
> once at the end... we wouldn't need a goto, just a few breaks
> in the loop and a conditional transaction cancel.
>
> > This patch gets rid of the pointless check.
>
> Hmm, seems like code churn that makes the code slightly less
> obvious, but thats just me... I'd prefer a tested patch that
> implements the above suggestion, to be honest. :)
>
Ok, I'll see what I can come up with.

-- 
Jesper Juhl <jesper.juhl@xxxxxxxxx>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html


<Prev in Thread] Current Thread [Next in Thread>