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

[Cristian Sicilia]

On Mon, Nov 12, 2018 at 11:46 PM Greg Kroah-Hartman <
gregkh at linuxfoundation.org> 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 at gmail.com>
> > ---
> >  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



> >
> >       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?



>
> thanks,
>
>
Thanks for your hints



> greg k-h
>


-- 



Cristian Sicilia

mail: sicilia.cristian at gmail.com

phone: +39 392 20 61 63 1

web:http://www.siciliacristian.com


On Mon, Nov 12, 2018 at 11:46 PM Greg Kroah-Hartman <
gregkh at linuxfoundation.org> 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 at gmail.com>
> > ---
> >  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.
>
> >
> >       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;
>
> Christian, this isn't your fault at all, I'm not rejecting this patch,
> just providing hints on what else you can do here :)
>
> thanks,
>
> greg k-h
>


-- 



Cristian Sicilia

mail: sicilia.cristian at gmail.com

phone: +39 392 20 61 63 1

web:http://www.siciliacristian.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-erofs/attachments/20181113/15380401/attachment.html>