On 2016-02-10 17:34, Jon Maloy wrote: > > >> -----Original Message----- >> From: Richard Alpe >> Sent: Friday, 05 February, 2016 10:29 >> To: [email protected] >> Cc: Jon Maloy; Ying Xue; [email protected]; Richard Alpe >> Subject: [PATCH tipc-discussion v1] tipc: refactor node xmit and fix memory >> leaks >> >> Refactor tipc_node_xmit() to fail fast and fail early. This commit fixes >> several >> potential memory leaks in unexpected error paths. >> >> -- tipc discussion -- >> Two thinks struck me when looking at this. >> >> 1. We claim to consume buffer chains in a lot of functions, but the error >> path >> is often neglected. > > Admitted. It is certainly due to the fact that I changed the model at a late > stage. Before, we never consumed the buffer chain at this level. > >> >> 2. Listing the potential return values in the function header is a bad idea. >> There is often a discrepancy between the listed and actual return values. In >> the case of tail calls for example, someone has updated the called function >> to >> return a new value, but "forgotten" to backtrack all callers and updating >> there headers. > > Mea culpa again, for same reason as above. > I don't think it is a bad idea per se; it is useful if we are just able to > make it right. After all, this is not a function that we are going to change > every month (and hopefully not for a very long time from now on). The only > code I see missing now is -ENOBUF as return code from tipc_node_xmit(). I think the risk of inconsistency causing confusion outweighs the benefit here.
I think it's fine to have comments like "Returns number of bytes read or a negative error code" "Returns non-zero if the page was successfully invalidated" or perhaps even "Returns zero on success, negative values on failure" These comments explains behaviour and only needs updating if the logic of the function changes completely. For example a read function no longer returning the bytes read. Listing all potential return codes like we do here can and will change if a called function is updated just slightly with a new error code. Imagine if all functions had this. Changing a error code in one single place would force you to backtrack through all callers and there callers, to check if you need to update a comment. I don't think I have seen all potential return codes listed like this anywhere else in the kernel(?) Sorry for the biskshedding.... :-) Regards Richard > > If you fix that return code value (remove "errno" so it fits on one line) you > can add a "Reviewed-by" from me. > > ///jon > >> >> Reported-by: Dmitry Vyukov <[email protected]> >> Signed-off-by: Richard Alpe <[email protected]> >> --- >> net/tipc/link.c | 8 ++++++-- >> net/tipc/node.c | 52 +++++++++++++++++++++++++++++++------------------ >> --- >> 2 files changed, 37 insertions(+), 23 deletions(-) >> >> diff --git a/net/tipc/link.c b/net/tipc/link.c index 0c2944f..d700be4 100644 >> --- a/net/tipc/link.c >> +++ b/net/tipc/link.c >> @@ -904,8 +904,10 @@ int tipc_link_xmit(struct tipc_link *l, struct >> sk_buff_head *list, >> if (unlikely(l->backlog[i].len >= l->backlog[i].limit)) >> return link_schedule_user(l, list); >> } >> - if (unlikely(msg_size(hdr) > mtu)) >> + if (unlikely(msg_size(hdr) > mtu)) { >> + skb_queue_purge(list); >> return -EMSGSIZE; >> + } >> >> /* Prepare each packet for sending, and add to relevant queue: */ >> while (skb_queue_len(list)) { >> @@ -917,8 +919,10 @@ int tipc_link_xmit(struct tipc_link *l, struct >> sk_buff_head *list, >> >> if (likely(skb_queue_len(transmq) < maxwin)) { >> _skb = skb_clone(skb, GFP_ATOMIC); >> - if (!_skb) >> + if (!_skb) { >> + skb_queue_purge(list); >> return -ENOBUFS; >> + } >> __skb_dequeue(list); >> __skb_queue_tail(transmq, skb); >> __skb_queue_tail(xmitq, _skb); >> diff --git a/net/tipc/node.c b/net/tipc/node.c index fa97d96..61024ec 100644 >> --- a/net/tipc/node.c >> +++ b/net/tipc/node.c >> @@ -1174,33 +1174,43 @@ int tipc_node_xmit(struct net *net, struct >> sk_buff_head *list, >> struct tipc_link_entry *le = NULL; >> struct tipc_node *n; >> struct sk_buff_head xmitq; >> - int bearer_id = -1; >> - int rc = -EHOSTUNREACH; >> + int bearer_id; >> + int rc; >> + >> + if (in_own_node(net, dnode)) { >> + tipc_sk_rcv(net, list); >> + return 0; >> + } >> >> - __skb_queue_head_init(&xmitq); >> n = tipc_node_find(net, dnode); >> - if (likely(n)) { >> - tipc_node_read_lock(n); >> - bearer_id = n->active_links[selector & 1]; >> - if (bearer_id >= 0) { >> - le = &n->links[bearer_id]; >> - spin_lock_bh(&le->lock); >> - rc = tipc_link_xmit(le->link, list, &xmitq); >> - spin_unlock_bh(&le->lock); >> - } >> + if (unlikely(!n)) { >> + skb_queue_purge(list); >> + return -EHOSTUNREACH; >> + } >> + >> + tipc_node_read_lock(n); >> + bearer_id = n->active_links[selector & 1]; >> + if (unlikely(bearer_id == INVALID_BEARER_ID)) { >> tipc_node_read_unlock(n); >> - if (likely(!rc)) >> - tipc_bearer_xmit(net, bearer_id, &xmitq, &le- >>> maddr); >> - else if (rc == -ENOBUFS) >> - tipc_node_link_down(n, bearer_id, false); >> tipc_node_put(n); >> - return rc; >> + skb_queue_purge(list); >> + return -EHOSTUNREACH; >> } >> >> - if (likely(in_own_node(net, dnode))) { >> - tipc_sk_rcv(net, list); >> - return 0; >> - } >> + __skb_queue_head_init(&xmitq); >> + le = &n->links[bearer_id]; >> + spin_lock_bh(&le->lock); >> + rc = tipc_link_xmit(le->link, list, &xmitq); >> + spin_unlock_bh(&le->lock); >> + tipc_node_read_unlock(n); >> + >> + if (likely(rc == 0)) >> + tipc_bearer_xmit(net, bearer_id, &xmitq, &le->maddr); >> + else if (rc == -ENOBUFS) >> + tipc_node_link_down(n, bearer_id, false); >> + >> + tipc_node_put(n); >> + >> return rc; >> } >> >> -- >> 2.1.4 > ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140 _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
