RDMA/rvt: Improve exception handling in rvt_create_qp()
Subsystems (2)
Quoted 2 lines
… The patch fixes this issue by
callingrvt_free_rq().
I suggest to choose another imperative wording for your change description. Will the tag “Fixes” become helpful for the commit message?
…
Review struct ib_qp *rvt_create_qp(struct ib_pd *ibpd, · line 1209
Quoted 9 lines
+++
b/drivers/infiniband/sw/rdmavt/qp.c(quoted diff) +1
-0@@ -1203,6 +1203,7 @@ struct ib_qp *rvt_create_qp(struct ib_pd *ibpd, qp->s_flags = RVT_S_SIGNAL_REQ_WR; err = alloc_ud_wq_attr(qp, rdi->dparms.node); if (err) { + rvt_free_rq(&qp->r_rq); ret = (ERR_PTR(err)); goto bail_driver_priv; }
How do you think about the following code variant with the addition of a jump target?
err = alloc_ud_wq_attr(qp, rdi->dparms.node); if (err) {
ret = (ERR_PTR(err));
- goto bail_driver_priv;
+ goto bail_free_rq;
}
…
bail_rq_wq:
- rvt_free_rq(&qp->r_rq);
free_ud_wq_attr(qp);
+
+bail_free_rq:
+ rvt_free_rq(&qp->r_rq);
bail_driver_priv:
Regards,
Markus
Quoted 39 lines · Markus Elfring
Quoted 2 lines
… The patch fixes this issue by
callingrvt_free_rq().I suggest to choose another imperative wording for your change description. Will the tag “Fixes” become helpful for the commit message?
…
Review struct ib_qp *rvt_create_qp(struct ib_pd *ibpd, · line 1209
Quoted 9 lines
+++
b/drivers/infiniband/sw/rdmavt/qp.c(quoted diff) +1
-0@@ -1203,6 +1203,7 @@ struct ib_qp *rvt_create_qp(struct ib_pd *ibpd, qp->s_flags = RVT_S_SIGNAL_REQ_WR; err = alloc_ud_wq_attr(qp, rdi->dparms.node); if (err) { + rvt_free_rq(&qp->r_rq); ret = (ERR_PTR(err)); goto bail_driver_priv; }How do you think about the following code variant with the addition of a jump target?
(quoted diff) +4
-2err = alloc_ud_wq_attr(qp, rdi->dparms.node); if (err) { ret = (ERR_PTR(err)); - goto bail_driver_priv; + goto bail_free_rq; } bail_rq_wq: - rvt_free_rq(&qp->r_rq); free_ud_wq_attr(qp); + +bail_free_rq: + rvt_free_rq(&qp->r_rq); bail_driver_priv:
Quoted 2 lines
Regards,
Markus
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless review comment to a patch submission on a Linux kernel developer mailing list. I strongly suggest that you not do this anymore. Please do not bother developers who are actively working to produce patches and features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all. The person/bot/AI that sent it is being ignored by almost all Linux kernel maintainers for having a persistent pattern of behavior of producing distracting and pointless commentary, and inability to adapt to feedback. Please feel free to also ignore emails from them.
thanks,
greg k-h's patch email bot
Quoted 21 lines
…
Review struct ib_qp *rvt_create_qp(struct ib_pd *ibpd, · line 1209
Quoted 9 lines
+++
b/drivers/infiniband/sw/rdmavt/qp.c(quoted diff) +1
-0@@ -1203,6 +1203,7 @@ struct ib_qp *rvt_create_qp(struct ib_pd *ibpd, qp->s_flags = RVT_S_SIGNAL_REQ_WR; err = alloc_ud_wq_attr(qp, rdi->dparms.node); if (err) { + rvt_free_rq(&qp->r_rq); ret = (ERR_PTR(err)); goto bail_driver_priv; }How do you think about the following code variant with the addition of a jump target?
(quoted diff) +1
-1err = alloc_ud_wq_attr(qp, rdi->dparms.node); if (err) { ret = (ERR_PTR(err)); - goto bail_driver_priv; + goto bail_free_rq; }
Review
Quoted 10 lines
(quoted diff) +3
-1bail_rq_wq: - rvt_free_rq(&qp->r_rq); free_ud_wq_attr(qp); + +bail_free_rq: + rvt_free_rq(&qp->r_rq); bail_driver_priv:
The improvement of affected implementation details is continued with another update suggestion.
RDMA/rvt: Fix potential memory leak caused by rvt_alloc_rq
https://lore.kernel.org/linux-rdma/[email protected]/
https://lore.kernel.org/patchwork/patch/1255709/
Regards,
Markus