Re: [PATCH] hfsplus: fix NULL dereference in hfsplus_lookup()

["Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>]

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 <slava@dubeyko.com> 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 <wen.xu@gatech.edu>
> > > > > Signed-off-by: Ernesto A. Fernández <ernesto.mnd.fernandez@gmail.com>
> > > > > ---
> > > > 
> > > > 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
> 
>