From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-f195.google.com ([209.85.220.195]:37816 "EHLO mail-qk0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727007AbeHWBF2 (ORCPT ); Wed, 22 Aug 2018 21:05:28 -0400 Received: by mail-qk0-f195.google.com with SMTP id f17-v6so2302464qkh.4 for ; Wed, 22 Aug 2018 14:38:52 -0700 (PDT) Date: Wed, 22 Aug 2018 18:38:47 -0300 From: Ernesto =?utf-8?Q?A=2E_Fern=C3=A1ndez?= To: Viacheslav Dubeyko Cc: Andrew Morton , linux-fsdevel@vger.kernel.org, "Xu, Wen" Subject: Re: [PATCH] hfsplus: fix NULL dereference in hfsplus_lookup() Message-ID: <20180822213847.lob454eebulasbjg@eaf> References: <20180712215344.q44dyrhymm4ajkao@eaf> <1531436133.22955.4.camel@slavad-ubuntu-14.04> <20180821160525.83c7cb950e26164803977857@linux-foundation.org> <20180822184616.3cm43dxzsijfayq7@eaf> <1534969119.2878.6.camel@slavad-ubuntu-14.04> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1534969119.2878.6.camel@slavad-ubuntu-14.04> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Aug 22, 2018 at 01:18:39PM -0700, Viacheslav Dubeyko wrote: > On Wed, 2018-08-22 at 15:46 -0300, Ernesto A. Fernández wrote: > > On Tue, Aug 21, 2018 at 04:05:25PM -0700, Andrew Morton wrote: > > > On Thu, 12 Jul 2018 15:55:33 -0700 Viacheslav Dubeyko wrote: > > > > > > > On Thu, 2018-07-12 at 18:53 -0300, Ernesto A. Fernández wrote: > > > > > Check that the hidden directory is not NULL before using it, instead of > > > > > after. > > > > > > > > > > Reported-by: Wen Xu > > > > > Signed-off-by: Ernesto A. Fernández > > > > > --- > > > > > > > > It's really hard to understand this simple patch. I believe it makes > > > > sense to rework the patch slightly with the goal to make it more clear. > > > > Also, it will be great to add a short comment in the code to explain > > > > what's wrong. > > > > I don't think it's reasonable to expect a comment explaining why we can't > > dereference NULL. > > > > The good comment is always really important part of the patch. That's your idea of a good comment? > > > > I think it makes sense to split this long check condition on something > > > > more clear, simple and elegant. > > > > The long check condition may not be ideal, but there's a lot of code in > > the module that could use style improvements. I don't think that should be > > a priority right now, with plenty of serious bugs left to fix. > > > > Bad style of code is one of the reason of bugs. If you don't try to > improve the code then you can simply create an another serious bug and > nobody will be able to understand your fix. All this does is reorder a check. Where could I possibly introduce a bug? Large unnecessary rewrites with little testing in unmaintained code are far more likely to cause trouble. I will be more comfortable with such things once I get the module to pass xfstests, but there's still plenty of bugs in the way. > The bad style of code in the > module is not the excuse at all. It's the way of open-source community > to achieve the good style of code by means of the discussion. Moreover, > the goal of bug fix is the improvement of code style too but not only to > resolve the issue. Another guys need to understand your way of the fix > too. This patch couldn't be any simpler. Anybody who finds it confusing would be completely overwhelmed by a big rewrite. > Thanks, > Vyacheslav Dubeyko. > > > > > > > No response, causing this patch to be stuck in limbo land? > > > > I believe I sent a second version of this patch. > > > > > > Ernest > >