After commit 987219ad34a6 ("staging: r8188eu: remove lines from Makefile
that silence build warnings"), clang warns:
drivers/staging/r8188eu/os_dep/usb_intf.c:726:6: warning: variable
'status' is used uninitialized whenever 'if' condition is true
[-Wsometimes-uninitialized]
if (!if1) {
^~~~
drivers/staging/r8188eu/os_dep/usb_intf.c:741:6: note: uninitialized use
occurs here
if (status != _SUCCESS)
^~~~~~
drivers/staging/r8188eu/os_dep/usb_intf.c:726:2: note: remove the 'if'
if its condition is always false
if (!if1) {
^~~~~~~~~~~
drivers/staging/r8188eu/os_dep/usb_intf.c:714:12: note: initialize the
variable 'status' to silence this warning
int status;
^
= 0
1 warning generated.
status is not initialized if the call to usb_dvobj_init() or
rtw_usb_if1_init() fails.
Looking at the error function as a whole, the error handling is odd
compared to the rest of the kernel, which prefers to set error codes on
goto paths, rather than a global "status" variable which determines the
error code at the end of the function and function calls in the case of
error.
Rearrange the error handling of this function to bring it more inline
with how the kernel does it in most cases, which helps readability.
The call to rtw_usb_if1_deinit() is eliminated because it is not
possible to ever hit it; if rtw_usb_if1_init() fails, the goto call
jumps over the call to rtw_usb_if1_deinit() and in the success case,
status is set to _SUCCESS.
Signed-off-by: Nathan Chancellor <[email protected]>
---
drivers/staging/r8188eu/os_dep/usb_intf.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
index a462cb6f3005..667f41125a87 100644
--- a/drivers/staging/r8188eu/os_dep/usb_intf.c
+++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
@@ -704,20 +704,23 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device_id *pdid)
{
struct adapter *if1 = NULL;
- int status;
struct dvobj_priv *dvobj;
+ int ret;
/* step 0. */
process_spec_devid(pdid);
/* Initialize dvobj_priv */
dvobj = usb_dvobj_init(pusb_intf);
- if (!dvobj)
- goto exit;
+ if (!dvobj) {
+ ret = -ENODEV;
+ goto err;
+ }
if1 = rtw_usb_if1_init(dvobj, pusb_intf);
if (!if1) {
DBG_88E("rtw_init_primarystruct adapter Failed!\n");
+ ret = -ENODEV;
goto free_dvobj;
}
@@ -726,15 +729,12 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
rtw_signal_process(ui_pid[1], SIGUSR2);
}
- status = _SUCCESS;
+ return 0;
- if (status != _SUCCESS && if1)
- rtw_usb_if1_deinit(if1);
free_dvobj:
- if (status != _SUCCESS)
- usb_dvobj_deinit(pusb_intf);
-exit:
- return status == _SUCCESS ? 0 : -ENODEV;
+ usb_dvobj_deinit(pusb_intf);
+err:
+ return ret;
}
/*
--
2.33.0.rc2
On Thu, 12 Aug 2021 at 21:40, Nathan Chancellor <[email protected]> wrote:
>
> After commit 987219ad34a6 ("staging: r8188eu: remove lines from Makefile
> that silence build warnings"), clang warns:
>
> drivers/staging/r8188eu/os_dep/usb_intf.c:726:6: warning: variable
> 'status' is used uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
> if (!if1) {
> ^~~~
> drivers/staging/r8188eu/os_dep/usb_intf.c:741:6: note: uninitialized use
> occurs here
> if (status != _SUCCESS)
> ^~~~~~
> drivers/staging/r8188eu/os_dep/usb_intf.c:726:2: note: remove the 'if'
> if its condition is always false
> if (!if1) {
> ^~~~~~~~~~~
> drivers/staging/r8188eu/os_dep/usb_intf.c:714:12: note: initialize the
> variable 'status' to silence this warning
> int status;
> ^
> = 0
> 1 warning generated.
>
> status is not initialized if the call to usb_dvobj_init() or
> rtw_usb_if1_init() fails.
>
> Looking at the error function as a whole, the error handling is odd
> compared to the rest of the kernel, which prefers to set error codes on
> goto paths, rather than a global "status" variable which determines the
> error code at the end of the function and function calls in the case of
> error.
>
> Rearrange the error handling of this function to bring it more inline
> with how the kernel does it in most cases, which helps readability.
>
> The call to rtw_usb_if1_deinit() is eliminated because it is not
> possible to ever hit it; if rtw_usb_if1_init() fails, the goto call
> jumps over the call to rtw_usb_if1_deinit() and in the success case,
> status is set to _SUCCESS.
>
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> drivers/staging/r8188eu/os_dep/usb_intf.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/r8188eu/os_dep/usb_intf.c b/drivers/staging/r8188eu/os_dep/usb_intf.c
> index a462cb6f3005..667f41125a87 100644
> --- a/drivers/staging/r8188eu/os_dep/usb_intf.c
> +++ b/drivers/staging/r8188eu/os_dep/usb_intf.c
> @@ -704,20 +704,23 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
> static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device_id *pdid)
> {
> struct adapter *if1 = NULL;
> - int status;
> struct dvobj_priv *dvobj;
> + int ret;
>
> /* step 0. */
> process_spec_devid(pdid);
>
> /* Initialize dvobj_priv */
> dvobj = usb_dvobj_init(pusb_intf);
> - if (!dvobj)
> - goto exit;
> + if (!dvobj) {
> + ret = -ENODEV;
> + goto err;
> + }
>
> if1 = rtw_usb_if1_init(dvobj, pusb_intf);
> if (!if1) {
> DBG_88E("rtw_init_primarystruct adapter Failed!\n");
> + ret = -ENODEV;
> goto free_dvobj;
> }
>
> @@ -726,15 +729,12 @@ static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device
> rtw_signal_process(ui_pid[1], SIGUSR2);
> }
>
> - status = _SUCCESS;
> + return 0;
>
> - if (status != _SUCCESS && if1)
> - rtw_usb_if1_deinit(if1);
> free_dvobj:
> - if (status != _SUCCESS)
> - usb_dvobj_deinit(pusb_intf);
> -exit:
> - return status == _SUCCESS ? 0 : -ENODEV;
> + usb_dvobj_deinit(pusb_intf);
> +err:
> + return ret;
> }
>
> /*
> --
> 2.33.0.rc2
>
Looks good as far as I can see, nicely done. Thanks.
Acked-by: Phillip Potter <[email protected]>
Regards,
Phil
On Thursday, August 12, 2021 10:40:27 PM CEST Nathan Chancellor wrote:
> [...]
> Looking at the error function as a whole, the error handling is odd
> compared to the rest of the kernel, which prefers to set error codes on
> goto paths, rather than a global "status" variable which determines the
> error code at the end of the function and function calls in the case of
> error.
>
"status" is not a global variable, instead it is a local variable with only
in-function visibility and it has an automatic storage duration (i.e., it is
allocated on the stack frame of the function and is destroyed when the stack
is unwound at the return from the function).
According to the above definition there's no difference in storage classes
between the old "status" and the new "ret" (the latter being a merely rename
of the former). However, "ret" is a more appropriate name for that variable.
Furthermore, your handling of errors and return value is more consistent with
best practices.
Therefore, apart that minor misuse of the "global" class in your commit
message, it's a nice work and so...
Acked-by: Fabio M. De Francesco <[email protected]>
Regards,
Fabio
>
> [...]
>
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> drivers/staging/r8188eu/os_dep/usb_intf.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> [...]
>
On 8/12/2021 4:11 PM, Fabio M. De Francesco wrote:
> On Thursday, August 12, 2021 10:40:27 PM CEST Nathan Chancellor wrote:
>> [...]
>> Looking at the error function as a whole, the error handling is odd
>> compared to the rest of the kernel, which prefers to set error codes on
>> goto paths, rather than a global "status" variable which determines the
>> error code at the end of the function and function calls in the case of
>> error.
>>
> "status" is not a global variable, instead it is a local variable with only
> in-function visibility and it has an automatic storage duration (i.e., it is
> allocated on the stack frame of the function and is destroyed when the stack
> is unwound at the return from the function).
>
> According to the above definition there's no difference in storage classes
> between the old "status" and the new "ret" (the latter being a merely rename
> of the former). However, "ret" is a more appropriate name for that variable.
> Furthermore, your handling of errors and return value is more consistent with
> best practices.
Sorry, I meant "global" with regards to the function (as in "was there
an error in the function"), rather than "global" as a storage class.
> Therefore, apart that minor misuse of the "global" class in your commit
> message, it's a nice work and so...
I am happy to redo the commit message if you and others so desire.
> Acked-by: Fabio M. De Francesco <[email protected]>
Thank you for the review and ack!
Cheers,
Nathan
On Friday, August 13, 2021 1:14:01 AM CEST Nathan Chancellor wrote:
> On 8/12/2021 4:11 PM, Fabio M. De Francesco wrote:
> > On Thursday, August 12, 2021 10:40:27 PM CEST Nathan Chancellor wrote:
> > [...]
> > Therefore, apart that minor misuse of the "global" class in your commit
> > message, it's a nice work and so...
>
> I am happy to redo the commit message if you and others so desire.
>
> > Acked-by: Fabio M. De Francesco <[email protected]>
>
> Thank you for the review and ack!
>
> Cheers,
> Nathan
>
Maybe that for that minor misuse of the definition of "global variable" it
isn't worth to redo three patches. If I were you, I'd wait for feedback from
Greg K-H and then I'd act accordingly.
But, at my first read of your patch, I didn't notice that when you return the
error from within the block starting at the "err:" label, "ret" is always set
as "-ENODEV".
So, why not simply "return -ENODEV;" and get rid of the "ret" variable?
Thanks,
Fabio