Re: [PATCH v1] drivers: block: Updates return value check

[Atul Kumar Pant <atulpant.linux@gmail.com>]

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@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.

	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