| Message ID | 87e55a72-71ad-d0a1-b936-12565bbc6508@users.sourceforge.net (mailing list archive) |
|---|---|
| State | Not Applicable, 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); > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> @@ -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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }