From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-f193.google.com ([209.85.160.193]:41639 "EHLO mail-qt1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725829AbeJYHPH (ORCPT ); Thu, 25 Oct 2018 03:15:07 -0400 Received: by mail-qt1-f193.google.com with SMTP id l41-v6so7622884qtl.8 for ; Wed, 24 Oct 2018 15:45:10 -0700 (PDT) Date: Wed, 24 Oct 2018 19:45:04 -0300 From: Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?= To: Andrew Morton Cc: Viacheslav Dubeyko , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH 2/6] hfsplus: fix BUG on bnode parent update Message-ID: <20181024224504.rpk3as66rk3dyiou@eaf> References: <26d882184fc43043a810114258f45277752186c7.1535682461.git.ernesto.mnd.fernandez@gmail.com> <5ee1db09b60373a15890f6a7c835d00e76bf601d.1535682461.git.ernesto.mnd.fernandez@gmail.com> <1540344830.28908.12.camel@slavad-ubuntu-14.04> <20181024024846.hbq6oftilihwumpt@eaf> <20181024143947.4e30cca3ddda937db70237e9@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20181024143947.4e30cca3ddda937db70237e9@linux-foundation.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Oct 24, 2018 at 02:39:47PM -0700, Andrew Morton wrote: > On Tue, 23 Oct 2018 23:48:46 -0300 Ernesto A. Fernández wrote: > > > > Sorry, I don't follow where the new_node is put twice. Could you explain > > > in more details? Currently, it looks unclear. > > > > There is a 'goto again', as I said in the commit message. Follow the code > > and you'll see that hfs_bnode_put() is called again on the same node. > > > > > I like to assign the NULL value to the pointer. > > > > This isn't a matter of taste. > > > > > But are you sure that it's proper place? > > > > Yes, but it's always better if somebody reviews the code... > > > > > Maybe it > > > makes sense to place this assignment in the beginning of the function? > > > > Without knowing what you mean by "beginning of the function", I can't > > tell if your idea would work or not. > > I think it would be clearer to do it this way: > > --- a/fs/hfs/brec.c~a > +++ a/fs/hfs/brec.c > @@ -359,11 +359,11 @@ static int hfs_brec_update_parent(struct > > tree = fd->tree; > node = fd->bnode; > - new_node = NULL; > if (!node->parent) > return 0; > > again: > + new_node = NULL; > parent = hfs_bnode_find(tree, node->parent); > if (IS_ERR(parent)) > return PTR_ERR(parent); > > But it doesn't matter much... Right, that looks better. I don't remember why I did it this way. If you want me to resend I'd rather run some tests again, just in case.