From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Cristian Sicilia <sicilia.cristian@gmail.com>
Cc: gaoxiang25@huawei.com, yuchao0@huawei.com,
linux-erofs@lists.ozlabs.org, devel@driverdev.osuosl.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] staging: erofs: unzip_vle.c: Replace comparison to NULL.
Date: Mon, 12 Nov 2018 15:46:03 -0800 [thread overview]
Message-ID: <20181112234603.GB16079@kroah.com> (raw)
In-Reply-To: <CACU=YuX3U50uVaitV1Mhn3Dn8pXhPZt-1K7HX4Bn7MUCrgE35w@mail.gmail.com>
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@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@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
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
next prev parent reply other threads:[~2018-11-12 23:46 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-12 20:43 [PATCH 0/3] Clean up some syntax issue on unzip_vle.c Cristian Sicilia
2018-11-12 20:43 ` [PATCH 1/3] staging: erofs: unzip_vle.c: Replace comparison to NULL Cristian Sicilia
2018-11-12 22:46 ` Greg Kroah-Hartman
[not found] ` <CACU=YuX3U50uVaitV1Mhn3Dn8pXhPZt-1K7HX4Bn7MUCrgE35w@mail.gmail.com>
2018-11-12 23:46 ` Greg Kroah-Hartman [this message]
2018-11-13 0:38 ` Gao Xiang
2018-11-13 0:15 ` Gao Xiang
2018-11-16 3:07 ` Chao Yu
2018-11-12 20:43 ` [PATCH 2/3] staging: erofs: unzip_vle.c: Constant in comparison on right side Cristian Sicilia
2018-11-13 0:02 ` Gao Xiang
2018-11-16 3:07 ` Chao Yu
2018-11-12 20:43 ` [PATCH 3/3] staging: erofs: unzip_vle.c: Align parameter to the parentesis Cristian Sicilia
2018-11-13 0:00 ` Gao Xiang
2018-11-16 3:07 ` Chao Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181112234603.GB16079@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=devel@driverdev.osuosl.org \
--cc=gaoxiang25@huawei.com \
--cc=linux-erofs@lists.ozlabs.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sicilia.cristian@gmail.com \
--cc=yuchao0@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).