Re: [PATCH 1/3] staging: erofs: unzip_vle.c: Replace comparison to NULL.

[Greg Kroah-Hartman]

On Tue, Nov 13, 2018 at 12:31:58AM +0100, Cristian Sicilia wrote:
> On Mon, Nov 12, 2018 at 11:46 PM Greg Kroah-Hartman <
> gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> > On Mon, Nov 12, 2018 at 09:43:57PM +0100, Cristian Sicilia wrote:
> > > Replace equal to NULL with logic unary operator,
> > > and removing not equal to NULL comparison.
> > >
> > > Signed-off-by: Cristian Sicilia <sicilia.cristian@xxxxxxxxx>
> > > ---
> > >  drivers/staging/erofs/unzip_vle.c | 86
> > +++++++++++++++++++--------------------
> > >  1 file changed, 43 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/drivers/staging/erofs/unzip_vle.c
> > b/drivers/staging/erofs/unzip_vle.c
> > > index 79d3ba6..1ffeeaa 100644
> > > --- a/drivers/staging/erofs/unzip_vle.c
> > > +++ b/drivers/staging/erofs/unzip_vle.c
> > > @@ -20,8 +20,8 @@ static struct kmem_cache *z_erofs_workgroup_cachep
> > __read_mostly;
> > >
> > >  void z_erofs_exit_zip_subsystem(void)
> > >  {
> > > -     BUG_ON(z_erofs_workqueue == NULL);
> > > -     BUG_ON(z_erofs_workgroup_cachep == NULL);
> > > +     BUG_ON(!z_erofs_workqueue);
> > > +     BUG_ON(!z_erofs_workgroup_cachep);
> >
> > Long-term, all of these BUG_ON need to be removed as they imply that the
> > developer has no idea what went wrong and can not recover.  For
> > something like this, we "know" these will be fine and odds are they can
> > just be removed entirely.
> >
> >
> Right, I'm watching how replace the BUG_ON with WARN_ON and check who call
> this functions

No, why would WARN_ON be any better?  Systems run with "panic on warn"
enabled and this would cause the machine to reboot.  Why are these
things even being checked in the first place if they are impossible to
hit?

If they really are impossible, remove the check.  If they are not, then
properly handle the logic if they are true.

Linus said the other day something like "programmers who use BUG_ON()
don't know what their code does", and I agree.  They are a crutch that
we need to fix up in the whole kernel, not just staging.

> > >       destroy_workqueue(z_erofs_workqueue);
> > >       kmem_cache_destroy(z_erofs_workgroup_cachep);
> > > @@ -39,7 +39,7 @@ static inline int init_unzip_workqueue(void)
> > >               WQ_UNBOUND | WQ_HIGHPRI | WQ_CPU_INTENSIVE,
> > >               onlinecpus + onlinecpus / 4);
> > >
> > > -     return z_erofs_workqueue != NULL ? 0 : -ENOMEM;
> > > +     return z_erofs_workqueue ? 0 : -ENOMEM;
> >
> > I hate ?: notation that is not in a function parameter, any way you can
> > just change this to:
> >         if (z_erofs_workqueue)
> >                 return 0;
> >         return -ENOMEM;
> >
> >
> I will replace the ?: too
> 
> 
> > Christian, this isn't your fault at all, I'm not rejecting this patch,
> > just providing hints on what else you can do here :)
> >
> 
> 
> but (if I well understand) I will send a different patch for both fix,
> right?

Yes, nothing wrong with this one that I could see.  I'll let the erofs
maintainers review it first before applying it in a few days to my tree.

thanks,

greg k-h