Re: [PATCH v1] drivers: block: Updates return value check
On Mon, Aug 07, 2023 at 05:14:20PM +0530, Atul Kumar Pant wrote:
> On Sun, Aug 06, 2023 at 03:36:18PM +0200, Greg KH wrote:
> > On Sun, Aug 06, 2023 at 05:53:51PM +0530, Atul Kumar Pant wrote:
> > > Updating the check of return value from debugfs_create_dir
> > > to use IS_ERR.
> >
> > Why?
> >
> > >
> > > Signed-off-by: Atul Kumar Pant <atulpant.linux@gmail.com>
> > > ---
> > > drivers/block/nbd.c | 4 ++--
> > > drivers/block/pktcdvd.c | 2 +-
> > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > > index 9c35c958f2c8..65ecde3e2a5b 100644
> > > --- a/drivers/block/nbd.c
> > > +++ b/drivers/block/nbd.c
> > > @@ -1666,7 +1666,7 @@ static int nbd_dev_dbg_init(struct nbd_device *nbd)
> > > return -EIO;
> > >
> > > dir = debugfs_create_dir(nbd_name(nbd), nbd_dbg_dir);
> > > - if (!dir) {
> > > + if (IS_ERR(dir)) {
> > > dev_err(nbd_to_dev(nbd), "Failed to create debugfs dir for '%s'\n",
> > > nbd_name(nbd));
> > > return -EIO;
> >
> > This isn't correct, sorry. Please do not make this change.
> >
> > > @@ -1692,7 +1692,7 @@ static int nbd_dbg_init(void)
> > > struct dentry *dbg_dir;
> > >
> > > dbg_dir = debugfs_create_dir("nbd", NULL);
> > > - if (!dbg_dir)
> > > + if (IS_ERR(dbg_dir))
> > > return -EIO;
> >
> > Again, not corrct.
> >
> > > nbd_dbg_dir = dbg_dir;
> > > diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
> > > index d5d7884cedd4..69e5a100b3cf 100644
> > > --- a/drivers/block/pktcdvd.c
> > > +++ b/drivers/block/pktcdvd.c
> > > @@ -451,7 +451,7 @@ static void pkt_debugfs_dev_new(struct pktcdvd_device *pd)
> > > if (!pkt_debugfs_root)
> > > return;
> > > pd->dfs_d_root = debugfs_create_dir(pd->name, pkt_debugfs_root);
> > > - if (!pd->dfs_d_root)
> > > + if (IS_ERR(pd->dfs_d_root))
> > > return;
> >
> > Also not correct.
> >
> > Why check the return value at all? As this check has always been wrong,
> > why are you wanting to keep it?
>
> I'll check the code again. I was not aware that this check is wrong,
> so just tried to fix this based on return value of
> debugfs_create_dir.
The return value of debugfs_create_dir() should never need to be checked
at all. The value passed in can be later used in any debugfs call
safely, be it an error or success. The kernel logic should NOT change
based on if debugfs is working properly or not.
So for stuff like this, where the check is obviously wrong (i.e. it's
never caught an error, it's even more of a good idea to remove the
check.
> >
> > Also, you never responded to our previous review comments, why not? To
> > ignore people is not generally considered a good idea :(
>
> I might have missed seeing your comments hence I did not reply back.
> Please accept my sincere apologies for this.
Oops, nope, my apologies, this was my fault. I got you confused with a
different developer sending patches to the kernel-mentees mailing list
with the same first name. I should have checked better, again my fault,
sorry.
So all is good with your responses, but you should fix these up to NOT
check the return value at all.
thanks,
greg k-h
Reply to: