diff mbox series

[04/10] block/rnbd-srv: no need to check sess_dev

Message ID 20230523075331.32250-5-guoqing.jiang@linux.dev (mailing list archive)
State New, archived
Headers show
Series misc patches for rnbd | expand

Commit Message

Guoqing Jiang May 23, 2023, 7:53 a.m. UTC 
Check ret is enough since if sess_dev is NULL which also
implies ret should be 0.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/block/rnbd/rnbd-srv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jinpu Wang May 23, 2023, 9:27 a.m. UTC | #1 
On Tue, May 23, 2023 at 9:53 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> Check ret is enough since if sess_dev is NULL which also
> implies ret should be 0.
>
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/block/rnbd/rnbd-srv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> index e9ef6bd4b50c..c4122e65b36a 100644
> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -96,7 +96,7 @@ rnbd_get_sess_dev(int dev_id, struct rnbd_srv_session *srv_sess)
>                 ret = kref_get_unless_zero(&sess_dev->kref);
>         rcu_read_unlock();
>
> -       if (!sess_dev || !ret)
> +       if (!ret)
>                 return ERR_PTR(-ENXIO);
>
>         return sess_dev;
> --
> 2.35.3

This looks wrong, if you drop the check for !sess_dev, you have to
check it later with IS_ERR_OR_NULL when call rnbd_get_sess_dev
Guoqing Jiang May 24, 2023, 1:35 a.m. UTC | #2 
On 5/23/23 17:27, Jinpu Wang wrote:
> On Tue, May 23, 2023 at 9:53 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>> Check ret is enough since if sess_dev is NULL which also
>> implies ret should be 0.
>>
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
>> ---
>>   drivers/block/rnbd/rnbd-srv.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
>> index e9ef6bd4b50c..c4122e65b36a 100644
>> --- a/drivers/block/rnbd/rnbd-srv.c
>> +++ b/drivers/block/rnbd/rnbd-srv.c
>> @@ -96,7 +96,7 @@ rnbd_get_sess_dev(int dev_id, struct rnbd_srv_session *srv_sess)
>>                  ret = kref_get_unless_zero(&sess_dev->kref);
>>          rcu_read_unlock();
>>
>> -       if (!sess_dev || !ret)
>> +       if (!ret)
>>                  return ERR_PTR(-ENXIO);
>>
>>          return sess_dev;
>> --
>> 2.35.3
> This looks wrong, if you drop the check for !sess_dev, you have to
> check it later with IS_ERR_OR_NULL when call rnbd_get_sess_dev

How can it return NULL? Pls correct me if I am wrong.

1. If sess_dev is NULL then ret is 0, so it just returns ERR_PTR(-ENXIO).
2. If sess_dev is not NULL, we still rely on the checking of ret.

But I can drop it as well if you think it is obscure.

Thanks,
Guoqing
Jinpu Wang May 24, 2023, 6:32 a.m. UTC | #3 
On Wed, May 24, 2023 at 3:35 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
>
>
> On 5/23/23 17:27, Jinpu Wang wrote:
> > On Tue, May 23, 2023 at 9:53 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
> >> Check ret is enough since if sess_dev is NULL which also
> >> implies ret should be 0.
> >>
> >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> >> ---
> >>   drivers/block/rnbd/rnbd-srv.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> >> index e9ef6bd4b50c..c4122e65b36a 100644
> >> --- a/drivers/block/rnbd/rnbd-srv.c
> >> +++ b/drivers/block/rnbd/rnbd-srv.c
> >> @@ -96,7 +96,7 @@ rnbd_get_sess_dev(int dev_id, struct rnbd_srv_session *srv_sess)
> >>                  ret = kref_get_unless_zero(&sess_dev->kref);
> >>          rcu_read_unlock();
> >>
> >> -       if (!sess_dev || !ret)
> >> +       if (!ret)
> >>                  return ERR_PTR(-ENXIO);
> >>
> >>          return sess_dev;
> >> --
> >> 2.35.3
> > This looks wrong, if you drop the check for !sess_dev, you have to
> > check it later with IS_ERR_OR_NULL when call rnbd_get_sess_dev
>
> How can it return NULL? Pls correct me if I am wrong.
>
> 1. If sess_dev is NULL then ret is 0, so it just returns ERR_PTR(-ENXIO).
> 2. If sess_dev is not NULL, we still rely on the checking of ret.
Ah, you are right.

Acked-by: Jack Wang <jinpu.wang@ionos.com>
thx!
diff mbox series

Patch

diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
index e9ef6bd4b50c..c4122e65b36a 100644
--- a/drivers/block/rnbd/rnbd-srv.c
+++ b/drivers/block/rnbd/rnbd-srv.c
@@ -96,7 +96,7 @@  rnbd_get_sess_dev(int dev_id, struct rnbd_srv_session *srv_sess)
 		ret = kref_get_unless_zero(&sess_dev->kref);
 	rcu_read_unlock();
 
-	if (!sess_dev || !ret)
+	if (!ret)
 		return ERR_PTR(-ENXIO);
 
 	return sess_dev;