* [PATCH v1] drivers: block: Updates return value check @ 2023-08-06 12:23 Atul Kumar Pant 2023-08-06 13:36 ` Greg KH 0 siblings, 1 reply; 5+ messages in thread From: Atul Kumar Pant @ 2023-08-06 12:23 UTC (permalink / raw) To: josef, axboe Cc: Atul Kumar Pant, linux-block, nbd, shuah, linux-kernel-mentees 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1] drivers: block: Updates return value check 2023-08-06 12:23 [PATCH v1] drivers: block: Updates return value check Atul Kumar Pant @ 2023-08-06 13:36 ` Greg KH 2023-08-07 11:44 ` Atul Kumar Pant 0 siblings, 1 reply; 5+ messages in thread From: Greg KH @ 2023-08-06 13:36 UTC (permalink / raw) To: Atul Kumar Pant Cc: josef, axboe, linux-block, shuah, linux-kernel-mentees, nbd 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] drivers: block: Updates return value check 2023-08-06 13:36 ` Greg KH @ 2023-08-07 11:44 ` Atul Kumar Pant 2023-08-08 8:08 ` Greg KH 0 siblings, 1 reply; 5+ messages in thread From: Atul Kumar Pant @ 2023-08-07 11:44 UTC (permalink / raw) To: Greg KH; +Cc: josef, axboe, linux-block, shuah, linux-kernel-mentees, nbd 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] drivers: block: Updates return value check 2023-08-07 11:44 ` Atul Kumar Pant @ 2023-08-08 8:08 ` Greg KH 2023-08-15 20:32 ` Atul Kumar Pant 0 siblings, 1 reply; 5+ messages in thread From: Greg KH @ 2023-08-08 8:08 UTC (permalink / raw) To: Atul Kumar Pant Cc: josef, axboe, linux-block, shuah, linux-kernel-mentees, nbd 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] drivers: block: Updates return value check 2023-08-08 8:08 ` Greg KH @ 2023-08-15 20:32 ` Atul Kumar Pant 0 siblings, 0 replies; 5+ messages in thread From: Atul Kumar Pant @ 2023-08-15 20:32 UTC (permalink / raw) To: Greg KH; +Cc: josef, axboe, linux-block, shuah, linux-kernel-mentees, nbd 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-08-15 20:34 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-06 12:23 [PATCH v1] drivers: block: Updates return value check Atul Kumar Pant 2023-08-06 13:36 ` Greg KH 2023-08-07 11:44 ` Atul Kumar Pant 2023-08-08 8:08 ` Greg KH 2023-08-15 20:32 ` Atul Kumar Pant
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).