| Message ID | 87e55a72-71ad-d0a1-b936-12565bbc6508@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/07/16 22:23, SF Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Sat, 16 Jul 2016 21:42:42 +0200 > > The kfree() function was called in one case by the > scsiback_device_action() function during error handling > even if the passed variable "tmr" contained a null pointer. > > Adjust jump targets according to the Linux coding style convention. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/xen/xen-scsiback.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c > index 4a48c06..7612bc9 100644 > --- a/drivers/xen/xen-scsiback.c > +++ b/drivers/xen/xen-scsiback.c > @@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, > tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL); > if (!tmr) { > target_put_sess_cmd(se_cmd); > - goto err; > + goto do_resp; > } Hmm, I'm not convinced this is an improvement. I'd rather rename the new error label to "put_cmd" and get rid of the braces in above if statement: - if (!tmr) { - target_put_sess_cmd(se_cmd); - goto err; - } + if (!tmr) + goto put_cmd; and then in the error path: -err: +put_cmd: + target_put_sess_cmd(se_cmd); +free_tmr: kfree(tmr); Juergen > > init_waitqueue_head(&tmr->tmr_wait); > @@ -616,7 +616,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, > unpacked_lun, tmr, act, GFP_KERNEL, > tag, TARGET_SCF_ACK_KREF); > if (rc) > - goto err; > + goto free_tmr; > > wait_event(tmr->tmr_wait, atomic_read(&tmr->tmr_complete)); > > @@ -626,8 +626,9 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, > scsiback_do_resp_with_sense(NULL, err, 0, pending_req); > transport_generic_free_cmd(&pending_req->se_cmd, 1); > return; > -err: > +free_tmr: > kfree(tmr); > +do_resp: > scsiback_do_resp_with_sense(NULL, err, 0, pending_req); > } > >
>> @@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, >> tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL); >> if (!tmr) { >> target_put_sess_cmd(se_cmd); >> - goto err; >> + goto do_resp; >> } > > Hmm, I'm not convinced this is an improvement. > > I'd rather rename the new error label to "put_cmd" and get rid of the > braces in above if statement: > > - if (!tmr) { > - target_put_sess_cmd(se_cmd); > - goto err; > - } > + if (!tmr) > + goto put_cmd; > > and then in the error path: > > -err: > +put_cmd: > + target_put_sess_cmd(se_cmd); I am unsure on the relevance of this function on such a source position. Would it make sense to move it further down at the end? > +free_tmr: > kfree(tmr); How do you think about to skip this function call after a memory allocation failure? Regards, Markus
On 19/07/16 16:56, SF Markus Elfring wrote: >>> @@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, >>> tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL); >>> if (!tmr) { >>> target_put_sess_cmd(se_cmd); >>> - goto err; >>> + goto do_resp; >>> } >> >> Hmm, I'm not convinced this is an improvement. >> >> I'd rather rename the new error label to "put_cmd" and get rid of the >> braces in above if statement: >> >> - if (!tmr) { >> - target_put_sess_cmd(se_cmd); >> - goto err; >> - } >> + if (!tmr) >> + goto put_cmd; >> >> and then in the error path: >> >> -err: >> +put_cmd: >> + target_put_sess_cmd(se_cmd); > > I am unsure on the relevance of this function on such a source position. > Would it make sense to move it further down at the end? You only want to call it in the first error case (allocation failure). >> +free_tmr: >> kfree(tmr); > > How do you think about to skip this function call after a memory > allocation failure? I think this just doesn't matter. If it were a hot path, yes. But trying to do micro-optimizations in an error path is just not worth the effort. I like a linear error path containing all the needed cleanups best. Juergen
>>>> @@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, >>>> tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL); >>>> if (!tmr) { >>>> target_put_sess_cmd(se_cmd); >>>> - goto err; >>>> + goto do_resp; >>>> } >>> >>> Hmm, I'm not convinced this is an improvement. >>> >>> I'd rather rename the new error label to "put_cmd" and get rid of the >>> braces in above if statement: >>> >>> - if (!tmr) { >>> - target_put_sess_cmd(se_cmd); >>> - goto err; >>> - } >>> + if (!tmr) >>> + goto put_cmd; >>> >>> and then in the error path: >>> >>> -err: >>> +put_cmd: >>> + target_put_sess_cmd(se_cmd); >> >> I am unsure on the relevance of this function on such a source position. >> Would it make sense to move it further down at the end? > > You only want to call it in the first error case (allocation failure). Thanks for your clarification. I find that my update suggestion (from Saturday) is still appropriate in this case. https://lkml.org/lkml/2016/7/16/172 >>> +free_tmr: >>> kfree(tmr); >> >> How do you think about to skip this function call after a memory >> allocation failure? > > I think this just doesn't matter. If it were a hot path, yes. But trying > to do micro-optimizations in an error path is just not worth the effort. Would you like to reduce also the amount of function calls in such special run-time situations? > I like a linear error path containing all the needed cleanups best. I would prefer to keep the discussed single function call within the basic block of the if statement. Have we got different opinions about the shown implementation details? Regards, Markus
On 20/07/16 07:10, SF Markus Elfring wrote: >>>>> @@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, >>>>> tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL); >>>>> if (!tmr) { >>>>> target_put_sess_cmd(se_cmd); >>>>> - goto err; >>>>> + goto do_resp; >>>>> } >>>> >>>> Hmm, I'm not convinced this is an improvement. >>>> >>>> I'd rather rename the new error label to "put_cmd" and get rid of the >>>> braces in above if statement: >>>> >>>> - if (!tmr) { >>>> - target_put_sess_cmd(se_cmd); >>>> - goto err; >>>> - } >>>> + if (!tmr) >>>> + goto put_cmd; >>>> >>>> and then in the error path: >>>> >>>> -err: >>>> +put_cmd: >>>> + target_put_sess_cmd(se_cmd); >>> >>> I am unsure on the relevance of this function on such a source position. >>> Would it make sense to move it further down at the end? >> >> You only want to call it in the first error case (allocation failure). > > Thanks for your clarification. > > I find that my update suggestion (from Saturday) is still appropriate > in this case. > https://lkml.org/lkml/2016/7/16/172 And I still think it isn't an improvement: Nack >>>> +free_tmr: >>>> kfree(tmr); >>> >>> How do you think about to skip this function call after a memory >>> allocation failure? >> >> I think this just doesn't matter. If it were a hot path, yes. But trying >> to do micro-optimizations in an error path is just not worth the effort. > > Would you like to reduce also the amount of function calls in such special > run-time situations? I just don't care for the extra 2 or 3 nsecs. Readability is more important here. >> I like a linear error path containing all the needed cleanups best. > > I would prefer to keep the discussed single function call within > the basic block of the if statement. > > Have we got different opinions about the shown implementation details? Yes. Juergen
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 20 Jul 2016 13:20:04 +0200
Further update suggestions were taken into account
after a patch was applied from static source code analysis.
Markus Elfring (3):
Delete an unnecessary check before the function call "kfree"
Rename jump labels in scsiback_device_action()
Pass a failure indication as a constant
drivers/xen/xen-scsiback.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c index 4a48c06..7612bc9 100644 --- a/drivers/xen/xen-scsiback.c +++ b/drivers/xen/xen-scsiback.c @@ -606,7 +606,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, tmr = kzalloc(sizeof(struct scsiback_tmr), GFP_KERNEL); if (!tmr) { target_put_sess_cmd(se_cmd); - goto err; + goto do_resp; } init_waitqueue_head(&tmr->tmr_wait); @@ -616,7 +616,7 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, unpacked_lun, tmr, act, GFP_KERNEL, tag, TARGET_SCF_ACK_KREF); if (rc) - goto err; + goto free_tmr; wait_event(tmr->tmr_wait, atomic_read(&tmr->tmr_complete)); @@ -626,8 +626,9 @@ static void scsiback_device_action(struct vscsibk_pend *pending_req, scsiback_do_resp_with_sense(NULL, err, 0, pending_req); transport_generic_free_cmd(&pending_req->se_cmd, 1); return; -err: +free_tmr: kfree(tmr); +do_resp: scsiback_do_resp_with_sense(NULL, err, 0, pending_req); }