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

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

[Atul Kumar Pant]

Updating the check of return value from debugfs_create_dir
to use IS_ERR.

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;
@@ -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;
 
 	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;
 
 	pd->dfs_f_info = debugfs_create_file("info", 0444,
-- 
2.25.1

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

[Greg KH]

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?

Also, you never responded to our previous review comments, why not?  To
ignore people is not generally considered a good idea :(

greg k-h

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

[Atul Kumar Pant]

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.

> 
> 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.
    I have one confusion though, regarding the comments that you are
    referring to. Are you mentioning about this patch? Re: [PATCH v5] selftests: rtc: Improve rtctest error handling
    Here I got the following response from your email bot -
    Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all.

    Maybe I misunderstood this comment and hence did not reply/do
    anything in response to Markus's comments.
    If you were referring to some other patch then if possible, can you please tell me the
    suject of the patch? I will reply to your comments and will make the
    fixes accordingly.

> 
> greg k-h

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

[Greg KH]

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

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

[Atul Kumar Pant]

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