On Tue, Aug 08, 2023 at 10:08:56AM +0200, Greg KH wrote:
> 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@xxxxxxxxx>
> > > > ---
> > > > 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.
Understood. I'll fix this in a new patch.
>
> > >
> > > 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.
>
No worries!
> 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
