* [PATCH] Bluetooth: Simplify / fix return values from tk_request
@ 2020-04-03 15:02 Guenter Roeck
2020-04-03 15:13 ` Alain Michaud
2020-04-06 19:15 ` Sonny Sasaka
0 siblings, 2 replies; 10+ messages in thread
From: Guenter Roeck @ 2020-04-03 15:02 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Johan Hedberg, David S . Miller, Jakub Kicinski, linux-bluetooth,
netdev, linux-kernel, Guenter Roeck, Sonny Sasaka
Some static checker run by 0day reports a variableScope warning.
net/bluetooth/smp.c:870:6: warning:
The scope of the variable 'err' can be reduced. [variableScope]
There is no need for two separate variables holding return values.
Stick with the existing variable. While at it, don't pre-initialize
'ret' because it is set in each code path.
tk_request() is supposed to return a negative error code on errors,
not a bluetooth return code. The calling code converts the return
value to SMP_UNSPECIFIED if needed.
Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works")
Cc: Sonny Sasaka <sonnysasaka@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
net/bluetooth/smp.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c
index d0b695ee49f6..30e8626dd553 100644
--- a/net/bluetooth/smp.c
+++ b/net/bluetooth/smp.c
@@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
struct l2cap_chan *chan = conn->smp;
struct smp_chan *smp = chan->data;
u32 passkey = 0;
- int ret = 0;
- int err;
+ int ret;
/* Initialize key for JUST WORKS */
memset(smp->tk, 0, sizeof(smp->tk));
@@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth,
/* If Just Works, Continue with Zero TK and ask user-space for
* confirmation */
if (smp->method == JUST_WORKS) {
- err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
+ ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst,
hcon->type,
hcon->dst_type,
passkey, 1);
- if (err)
- return SMP_UNSPECIFIED;
+ if (ret)
+ return ret;
set_bit(SMP_FLAG_WAIT_USER, &smp->flags);
return 0;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request 2020-04-03 15:02 [PATCH] Bluetooth: Simplify / fix return values from tk_request Guenter Roeck @ 2020-04-03 15:13 ` Alain Michaud 2020-04-03 16:42 ` Guenter Roeck 2020-04-06 19:15 ` Sonny Sasaka 1 sibling, 1 reply; 10+ messages in thread From: Alain Michaud @ 2020-04-03 15:13 UTC (permalink / raw) To: Guenter Roeck Cc: Marcel Holtmann, Johan Hedberg, David S . Miller, Jakub Kicinski, BlueZ, netdev, LKML, Sonny Sasaka Hi Guenter/Marcel, On Fri, Apr 3, 2020 at 11:03 AM Guenter Roeck <linux@roeck-us.net> wrote: > > Some static checker run by 0day reports a variableScope warning. > > net/bluetooth/smp.c:870:6: warning: > The scope of the variable 'err' can be reduced. [variableScope] > > There is no need for two separate variables holding return values. > Stick with the existing variable. While at it, don't pre-initialize > 'ret' because it is set in each code path. > > tk_request() is supposed to return a negative error code on errors, > not a bluetooth return code. The calling code converts the return > value to SMP_UNSPECIFIED if needed. > > Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works") > Cc: Sonny Sasaka <sonnysasaka@chromium.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > net/bluetooth/smp.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c > index d0b695ee49f6..30e8626dd553 100644 > --- a/net/bluetooth/smp.c > +++ b/net/bluetooth/smp.c > @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, > struct l2cap_chan *chan = conn->smp; > struct smp_chan *smp = chan->data; > u32 passkey = 0; > - int ret = 0; > - int err; > + int ret; > > /* Initialize key for JUST WORKS */ > memset(smp->tk, 0, sizeof(smp->tk)); > @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, > /* If Just Works, Continue with Zero TK and ask user-space for > * confirmation */ > if (smp->method == JUST_WORKS) { > - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, > + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, > hcon->type, > hcon->dst_type, > passkey, 1); > - if (err) > - return SMP_UNSPECIFIED; > + if (ret) > + return ret; I think there may be some miss match between expected types of error codes here. The SMP error code type seems to be expected throughout this code base, so this change would propagate a potential negative value while the rest of the SMP protocol expects strictly positive error codes. > set_bit(SMP_FLAG_WAIT_USER, &smp->flags); > return 0; > } > -- > 2.17.1 > Thanks, Alain ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request 2020-04-03 15:13 ` Alain Michaud @ 2020-04-03 16:42 ` Guenter Roeck 2020-04-03 16:56 ` Alain Michaud 2020-04-06 12:06 ` Marcel Holtmann 0 siblings, 2 replies; 10+ messages in thread From: Guenter Roeck @ 2020-04-03 16:42 UTC (permalink / raw) To: Alain Michaud Cc: Marcel Holtmann, Johan Hedberg, David S . Miller, Jakub Kicinski, BlueZ, netdev, LKML, Sonny Sasaka On 4/3/20 8:13 AM, Alain Michaud wrote: > Hi Guenter/Marcel, > > > On Fri, Apr 3, 2020 at 11:03 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> Some static checker run by 0day reports a variableScope warning. >> >> net/bluetooth/smp.c:870:6: warning: >> The scope of the variable 'err' can be reduced. [variableScope] >> >> There is no need for two separate variables holding return values. >> Stick with the existing variable. While at it, don't pre-initialize >> 'ret' because it is set in each code path. >> >> tk_request() is supposed to return a negative error code on errors, >> not a bluetooth return code. The calling code converts the return >> value to SMP_UNSPECIFIED if needed. >> >> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works") >> Cc: Sonny Sasaka <sonnysasaka@chromium.org> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >> --- >> net/bluetooth/smp.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c >> index d0b695ee49f6..30e8626dd553 100644 >> --- a/net/bluetooth/smp.c >> +++ b/net/bluetooth/smp.c >> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, >> struct l2cap_chan *chan = conn->smp; >> struct smp_chan *smp = chan->data; >> u32 passkey = 0; >> - int ret = 0; >> - int err; >> + int ret; >> >> /* Initialize key for JUST WORKS */ >> memset(smp->tk, 0, sizeof(smp->tk)); >> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, >> /* If Just Works, Continue with Zero TK and ask user-space for >> * confirmation */ >> if (smp->method == JUST_WORKS) { >> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, >> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, >> hcon->type, >> hcon->dst_type, >> passkey, 1); >> - if (err) >> - return SMP_UNSPECIFIED; >> + if (ret) >> + return ret; > I think there may be some miss match between expected types of error > codes here. The SMP error code type seems to be expected throughout > this code base, so this change would propagate a potential negative > value while the rest of the SMP protocol expects strictly positive > error codes. > Up to the patch introducing the SMP_UNSPECIFIED return value, tk_request() returned negative error codes, and all callers convert it to SMP_UNSPECIFIED. If tk_request() is supposed to return SMP_UNSPECIFIED on error, it should be returned consistently, and its callers don't have to convert it again. Guenter >> set_bit(SMP_FLAG_WAIT_USER, &smp->flags); >> return 0; >> } >> -- >> 2.17.1 >> > > Thanks, > Alain > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request 2020-04-03 16:42 ` Guenter Roeck @ 2020-04-03 16:56 ` Alain Michaud 2020-04-04 0:39 ` Sonny Sasaka 2020-04-06 12:06 ` Marcel Holtmann 1 sibling, 1 reply; 10+ messages in thread From: Alain Michaud @ 2020-04-03 16:56 UTC (permalink / raw) To: Guenter Roeck Cc: Marcel Holtmann, Johan Hedberg, David S . Miller, Jakub Kicinski, BlueZ, netdev, LKML, Sonny Sasaka On Fri, Apr 3, 2020 at 12:43 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 4/3/20 8:13 AM, Alain Michaud wrote: > > Hi Guenter/Marcel, > > > > > > On Fri, Apr 3, 2020 at 11:03 AM Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> Some static checker run by 0day reports a variableScope warning. > >> > >> net/bluetooth/smp.c:870:6: warning: > >> The scope of the variable 'err' can be reduced. [variableScope] > >> > >> There is no need for two separate variables holding return values. > >> Stick with the existing variable. While at it, don't pre-initialize > >> 'ret' because it is set in each code path. > >> > >> tk_request() is supposed to return a negative error code on errors, > >> not a bluetooth return code. The calling code converts the return > >> value to SMP_UNSPECIFIED if needed. > >> > >> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works") > >> Cc: Sonny Sasaka <sonnysasaka@chromium.org> > >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > >> --- > >> net/bluetooth/smp.c | 9 ++++----- > >> 1 file changed, 4 insertions(+), 5 deletions(-) > >> > >> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c > >> index d0b695ee49f6..30e8626dd553 100644 > >> --- a/net/bluetooth/smp.c > >> +++ b/net/bluetooth/smp.c > >> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, > >> struct l2cap_chan *chan = conn->smp; > >> struct smp_chan *smp = chan->data; > >> u32 passkey = 0; > >> - int ret = 0; > >> - int err; > >> + int ret; > >> > >> /* Initialize key for JUST WORKS */ > >> memset(smp->tk, 0, sizeof(smp->tk)); > >> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, > >> /* If Just Works, Continue with Zero TK and ask user-space for > >> * confirmation */ > >> if (smp->method == JUST_WORKS) { > >> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, > >> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, > >> hcon->type, > >> hcon->dst_type, > >> passkey, 1); > >> - if (err) > >> - return SMP_UNSPECIFIED; > >> + if (ret) > >> + return ret; > > I think there may be some miss match between expected types of error > > codes here. The SMP error code type seems to be expected throughout > > this code base, so this change would propagate a potential negative > > value while the rest of the SMP protocol expects strictly positive > > error codes. > > > > Up to the patch introducing the SMP_UNSPECIFIED return value, tk_request() > returned negative error codes, and all callers convert it to SMP_UNSPECIFIED. > > If tk_request() is supposed to return SMP_UNSPECIFIED on error, it should > be returned consistently, and its callers don't have to convert it again. Agreed, the conventions aren't clear here. I'll differ to Marcel to provide guidance in this case where as a long term solution might increase the scope of this patch beyond what would be reasonable. > > Guenter > > >> set_bit(SMP_FLAG_WAIT_USER, &smp->flags); > >> return 0; > >> } > >> -- > >> 2.17.1 > >> > > > > Thanks, > > Alain > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request 2020-04-03 16:56 ` Alain Michaud @ 2020-04-04 0:39 ` Sonny Sasaka 0 siblings, 0 replies; 10+ messages in thread From: Sonny Sasaka @ 2020-04-04 0:39 UTC (permalink / raw) To: Alain Michaud Cc: Guenter Roeck, Marcel Holtmann, Johan Hedberg, David S . Miller, Jakub Kicinski, BlueZ, netdev, LKML The patch looks good to me. Agreed with Guenter's assessment, I made a mistake in the original patch by not being consistent with the function contract. On Fri, Apr 3, 2020 at 9:57 AM Alain Michaud <alainmichaud@google.com> wrote: > > On Fri, Apr 3, 2020 at 12:43 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On 4/3/20 8:13 AM, Alain Michaud wrote: > > > Hi Guenter/Marcel, > > > > > > > > > On Fri, Apr 3, 2020 at 11:03 AM Guenter Roeck <linux@roeck-us.net> wrote: > > >> > > >> Some static checker run by 0day reports a variableScope warning. > > >> > > >> net/bluetooth/smp.c:870:6: warning: > > >> The scope of the variable 'err' can be reduced. [variableScope] > > >> > > >> There is no need for two separate variables holding return values. > > >> Stick with the existing variable. While at it, don't pre-initialize > > >> 'ret' because it is set in each code path. > > >> > > >> tk_request() is supposed to return a negative error code on errors, > > >> not a bluetooth return code. The calling code converts the return > > >> value to SMP_UNSPECIFIED if needed. > > >> > > >> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works") > > >> Cc: Sonny Sasaka <sonnysasaka@chromium.org> > > >> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > >> --- > > >> net/bluetooth/smp.c | 9 ++++----- > > >> 1 file changed, 4 insertions(+), 5 deletions(-) > > >> > > >> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c > > >> index d0b695ee49f6..30e8626dd553 100644 > > >> --- a/net/bluetooth/smp.c > > >> +++ b/net/bluetooth/smp.c > > >> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, > > >> struct l2cap_chan *chan = conn->smp; > > >> struct smp_chan *smp = chan->data; > > >> u32 passkey = 0; > > >> - int ret = 0; > > >> - int err; > > >> + int ret; > > >> > > >> /* Initialize key for JUST WORKS */ > > >> memset(smp->tk, 0, sizeof(smp->tk)); > > >> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, > > >> /* If Just Works, Continue with Zero TK and ask user-space for > > >> * confirmation */ > > >> if (smp->method == JUST_WORKS) { > > >> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, > > >> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, > > >> hcon->type, > > >> hcon->dst_type, > > >> passkey, 1); > > >> - if (err) > > >> - return SMP_UNSPECIFIED; > > >> + if (ret) > > >> + return ret; > > > I think there may be some miss match between expected types of error > > > codes here. The SMP error code type seems to be expected throughout > > > this code base, so this change would propagate a potential negative > > > value while the rest of the SMP protocol expects strictly positive > > > error codes. > > > > > > > Up to the patch introducing the SMP_UNSPECIFIED return value, tk_request() > > returned negative error codes, and all callers convert it to SMP_UNSPECIFIED. > > > > If tk_request() is supposed to return SMP_UNSPECIFIED on error, it should > > be returned consistently, and its callers don't have to convert it again. > Agreed, the conventions aren't clear here. I'll differ to Marcel to > provide guidance in this case where as a long term solution might > increase the scope of this patch beyond what would be reasonable. > > > > Guenter > > > > >> set_bit(SMP_FLAG_WAIT_USER, &smp->flags); > > >> return 0; > > >> } > > >> -- > > >> 2.17.1 > > >> > > > > > > Thanks, > > > Alain > > > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request 2020-04-03 16:42 ` Guenter Roeck 2020-04-03 16:56 ` Alain Michaud @ 2020-04-06 12:06 ` Marcel Holtmann 2020-04-06 18:13 ` Sonny Sasaka 1 sibling, 1 reply; 10+ messages in thread From: Marcel Holtmann @ 2020-04-06 12:06 UTC (permalink / raw) To: Guenter Roeck Cc: Alain Michaud, Johan Hedberg, David S. Miller, Jakub Kicinski, BlueZ, netdev, LKML, Sonny Sasaka Hi Guenter, >>> Some static checker run by 0day reports a variableScope warning. >>> >>> net/bluetooth/smp.c:870:6: warning: >>> The scope of the variable 'err' can be reduced. [variableScope] >>> >>> There is no need for two separate variables holding return values. >>> Stick with the existing variable. While at it, don't pre-initialize >>> 'ret' because it is set in each code path. >>> >>> tk_request() is supposed to return a negative error code on errors, >>> not a bluetooth return code. The calling code converts the return >>> value to SMP_UNSPECIFIED if needed. >>> >>> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works") >>> Cc: Sonny Sasaka <sonnysasaka@chromium.org> >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> >>> --- >>> net/bluetooth/smp.c | 9 ++++----- >>> 1 file changed, 4 insertions(+), 5 deletions(-) >>> >>> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c >>> index d0b695ee49f6..30e8626dd553 100644 >>> --- a/net/bluetooth/smp.c >>> +++ b/net/bluetooth/smp.c >>> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, >>> struct l2cap_chan *chan = conn->smp; >>> struct smp_chan *smp = chan->data; >>> u32 passkey = 0; >>> - int ret = 0; >>> - int err; >>> + int ret; >>> >>> /* Initialize key for JUST WORKS */ >>> memset(smp->tk, 0, sizeof(smp->tk)); >>> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, >>> /* If Just Works, Continue with Zero TK and ask user-space for >>> * confirmation */ >>> if (smp->method == JUST_WORKS) { >>> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, >>> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, >>> hcon->type, >>> hcon->dst_type, >>> passkey, 1); >>> - if (err) >>> - return SMP_UNSPECIFIED; >>> + if (ret) >>> + return ret; >> I think there may be some miss match between expected types of error >> codes here. The SMP error code type seems to be expected throughout >> this code base, so this change would propagate a potential negative >> value while the rest of the SMP protocol expects strictly positive >> error codes. >> > > Up to the patch introducing the SMP_UNSPECIFIED return value, tk_request() > returned negative error codes, and all callers convert it to SMP_UNSPECIFIED. > > If tk_request() is supposed to return SMP_UNSPECIFIED on error, it should > be returned consistently, and its callers don't have to convert it again. maybe we need to fix that initial patch then. Regards Marcel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request 2020-04-06 12:06 ` Marcel Holtmann @ 2020-04-06 18:13 ` Sonny Sasaka 2020-04-06 18:26 ` Marcel Holtmann 0 siblings, 1 reply; 10+ messages in thread From: Sonny Sasaka @ 2020-04-06 18:13 UTC (permalink / raw) To: Marcel Holtmann Cc: Guenter Roeck, Alain Michaud, Johan Hedberg, David S. Miller, Jakub Kicinski, BlueZ, netdev, LKML Hi Marcel, Can this patch be merged? Or do you prefer reverting the original patch and relanding it together with the fix? On Mon, Apr 6, 2020 at 5:06 AM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Guenter, > > >>> Some static checker run by 0day reports a variableScope warning. > >>> > >>> net/bluetooth/smp.c:870:6: warning: > >>> The scope of the variable 'err' can be reduced. [variableScope] > >>> > >>> There is no need for two separate variables holding return values. > >>> Stick with the existing variable. While at it, don't pre-initialize > >>> 'ret' because it is set in each code path. > >>> > >>> tk_request() is supposed to return a negative error code on errors, > >>> not a bluetooth return code. The calling code converts the return > >>> value to SMP_UNSPECIFIED if needed. > >>> > >>> Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works") > >>> Cc: Sonny Sasaka <sonnysasaka@chromium.org> > >>> Signed-off-by: Guenter Roeck <linux@roeck-us.net> > >>> --- > >>> net/bluetooth/smp.c | 9 ++++----- > >>> 1 file changed, 4 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c > >>> index d0b695ee49f6..30e8626dd553 100644 > >>> --- a/net/bluetooth/smp.c > >>> +++ b/net/bluetooth/smp.c > >>> @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, > >>> struct l2cap_chan *chan = conn->smp; > >>> struct smp_chan *smp = chan->data; > >>> u32 passkey = 0; > >>> - int ret = 0; > >>> - int err; > >>> + int ret; > >>> > >>> /* Initialize key for JUST WORKS */ > >>> memset(smp->tk, 0, sizeof(smp->tk)); > >>> @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, > >>> /* If Just Works, Continue with Zero TK and ask user-space for > >>> * confirmation */ > >>> if (smp->method == JUST_WORKS) { > >>> - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, > >>> + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, > >>> hcon->type, > >>> hcon->dst_type, > >>> passkey, 1); > >>> - if (err) > >>> - return SMP_UNSPECIFIED; > >>> + if (ret) > >>> + return ret; > >> I think there may be some miss match between expected types of error > >> codes here. The SMP error code type seems to be expected throughout > >> this code base, so this change would propagate a potential negative > >> value while the rest of the SMP protocol expects strictly positive > >> error codes. > >> > > > > Up to the patch introducing the SMP_UNSPECIFIED return value, tk_request() > > returned negative error codes, and all callers convert it to SMP_UNSPECIFIED. > > > > If tk_request() is supposed to return SMP_UNSPECIFIED on error, it should > > be returned consistently, and its callers don't have to convert it again. > > maybe we need to fix that initial patch then. > > Regards > > Marcel > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request 2020-04-06 18:13 ` Sonny Sasaka @ 2020-04-06 18:26 ` Marcel Holtmann 2020-04-06 18:45 ` Guenter Roeck 0 siblings, 1 reply; 10+ messages in thread From: Marcel Holtmann @ 2020-04-06 18:26 UTC (permalink / raw) To: Sonny Sasaka Cc: Guenter Roeck, Alain Michaud, Johan Hedberg, David S. Miller, Jakub Kicinski, BlueZ, netdev, LKML Hi Sonny, > Can this patch be merged? Or do you prefer reverting the original > patch and relanding it together with the fix? since the original patch is already upstream, I need a patch with Fixes etc. And a Reviewed-By from you preferably. Regards Marcel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request 2020-04-06 18:26 ` Marcel Holtmann @ 2020-04-06 18:45 ` Guenter Roeck 0 siblings, 0 replies; 10+ messages in thread From: Guenter Roeck @ 2020-04-06 18:45 UTC (permalink / raw) To: Marcel Holtmann Cc: Sonny Sasaka, Alain Michaud, Johan Hedberg, David S. Miller, Jakub Kicinski, BlueZ, netdev, LKML On Mon, Apr 06, 2020 at 08:26:55PM +0200, Marcel Holtmann wrote: > Hi Sonny, > > > Can this patch be merged? Or do you prefer reverting the original > > patch and relanding it together with the fix? > > since the original patch is already upstream, I need a patch with Fixes etc. And a Reviewed-By from you preferably. > Isn't that what I sent with https://patchwork.kernel.org/patch/11472991/ ? What is missing (besides Sonny's Reviewed-by: tag) ? Thanks, Guenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] Bluetooth: Simplify / fix return values from tk_request 2020-04-03 15:02 [PATCH] Bluetooth: Simplify / fix return values from tk_request Guenter Roeck 2020-04-03 15:13 ` Alain Michaud @ 2020-04-06 19:15 ` Sonny Sasaka 1 sibling, 0 replies; 10+ messages in thread From: Sonny Sasaka @ 2020-04-06 19:15 UTC (permalink / raw) To: Guenter Roeck Cc: Marcel Holtmann, Johan Hedberg, David S . Miller, Jakub Kicinski, BlueZ, netdev, LKML Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> On Fri, Apr 3, 2020 at 8:02 AM Guenter Roeck <linux@roeck-us.net> wrote: > > Some static checker run by 0day reports a variableScope warning. > > net/bluetooth/smp.c:870:6: warning: > The scope of the variable 'err' can be reduced. [variableScope] > > There is no need for two separate variables holding return values. > Stick with the existing variable. While at it, don't pre-initialize > 'ret' because it is set in each code path. > > tk_request() is supposed to return a negative error code on errors, > not a bluetooth return code. The calling code converts the return > value to SMP_UNSPECIFIED if needed. > > Fixes: 92516cd97fd4 ("Bluetooth: Always request for user confirmation for Just Works") > Cc: Sonny Sasaka <sonnysasaka@chromium.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > net/bluetooth/smp.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/net/bluetooth/smp.c b/net/bluetooth/smp.c > index d0b695ee49f6..30e8626dd553 100644 > --- a/net/bluetooth/smp.c > +++ b/net/bluetooth/smp.c > @@ -854,8 +854,7 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, > struct l2cap_chan *chan = conn->smp; > struct smp_chan *smp = chan->data; > u32 passkey = 0; > - int ret = 0; > - int err; > + int ret; > > /* Initialize key for JUST WORKS */ > memset(smp->tk, 0, sizeof(smp->tk)); > @@ -887,12 +886,12 @@ static int tk_request(struct l2cap_conn *conn, u8 remote_oob, u8 auth, > /* If Just Works, Continue with Zero TK and ask user-space for > * confirmation */ > if (smp->method == JUST_WORKS) { > - err = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, > + ret = mgmt_user_confirm_request(hcon->hdev, &hcon->dst, > hcon->type, > hcon->dst_type, > passkey, 1); > - if (err) > - return SMP_UNSPECIFIED; > + if (ret) > + return ret; > set_bit(SMP_FLAG_WAIT_USER, &smp->flags); > return 0; > } > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-06 19:15 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-03 15:02 [PATCH] Bluetooth: Simplify / fix return values from tk_request Guenter Roeck 2020-04-03 15:13 ` Alain Michaud 2020-04-03 16:42 ` Guenter Roeck 2020-04-03 16:56 ` Alain Michaud 2020-04-04 0:39 ` Sonny Sasaka 2020-04-06 12:06 ` Marcel Holtmann 2020-04-06 18:13 ` Sonny Sasaka 2020-04-06 18:26 ` Marcel Holtmann 2020-04-06 18:45 ` Guenter Roeck 2020-04-06 19:15 ` Sonny Sasaka
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).