Hi Nithin, Thanks for addressing the transactional error issue. Are we sure we want to make another copy for the Reply just for getting the transactional error on a different buffer? Can we use the stack buffer only if the caller does not provide enough space for the transactional error message? Please find a comment inline. Eitan
-----Original Message----- From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Nithin Raju Sent: Tuesday, October 07, 2014 3:09 PM To: dev@openvswitch.org Subject: [ovs-dev] [PATCH v2] lib/netlink-socket.c: always pass the output buffer in a transaction We need to pass down the output buffer so that the kernel can return transaction status - error or otherwise. Also, we were processing the output buffer only when when 'txn->reply != NULL' ie when the caller specified an ofpbuf for the reply. In this patch, the code has been updated to process the reply unconditionally, but making sure to copy the reply to the 'txn->reply' only when it is not NULL. The reason for the unconditional processing is we can pass up transactional errors in 'txn->error'. Otherwise, it results in an endless loop of calling nl_transact(). Signed-off-by: Nithin Raju <nit...@vmware.com> --- lib/netlink-socket.c | 80 +++++++++++++++++++++++--------------------------- 1 files changed, 37 insertions(+), 43 deletions(-) diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c index 1aa76ae..7e30ab1 100644 --- a/lib/netlink-socket.c +++ b/lib/netlink-socket.c @@ -758,74 +758,68 @@ nl_sock_transact_multiple__(struct nl_sock *sock, ofpbuf_uninit(&tmp_reply); #else error = 0; + uint8_t reply_buf[65536]; for (i = 0; i < n; i++) { DWORD reply_len; - uint8_t tail[65536]; struct nl_transaction *txn = transactions[i]; struct nlmsghdr *request_nlmsg, *reply_nlmsg; if (!DeviceIoControl(sock->handle, OVS_IOCTL_TRANSACT, ofpbuf_data(txn->request), ofpbuf_size(txn->request), - txn->reply ? tail : 0, - txn->reply ? sizeof tail : 0, + reply_buf, sizeof reply_buf, &reply_len, NULL)) { /* XXX: Map to a more appropriate error. */ error = EINVAL; break; } - if (txn->reply) { - if (reply_len < sizeof *reply_nlmsg) { - VLOG_DBG_RL(&rl, "insufficient length of reply %#"PRIu32, - reply_len); - break; - } + if (reply_len < sizeof *reply_nlmsg) { + nl_sock_record_errors__(transactions, n, 0); + VLOG_DBG_RL(&rl, "insufficient length of reply %#"PRIu32 + " for seq: %#"PRIx32, reply_len, request_nlmsg->nlmsg_seq); + break; + } - /* Validate the sequence number in the reply. */ - request_nlmsg = nl_msg_nlmsghdr(txn->request); - reply_nlmsg = (struct nlmsghdr *)tail; + /* Validate the sequence number in the reply. */ + request_nlmsg = nl_msg_nlmsghdr(txn->request); + reply_nlmsg = (struct nlmsghdr *)reply_buf; - if (request_nlmsg->nlmsg_seq != reply_nlmsg->nlmsg_seq) { - ovs_assert(request_nlmsg->nlmsg_seq == reply_nlmsg->nlmsg_seq); - VLOG_DBG_RL(&rl, "mismatched seq request %#"PRIx32 - ", reply %#"PRIx32, request_nlmsg->nlmsg_seq, - reply_nlmsg->nlmsg_seq); - break; - } [EITAN] This condition should never be true unless there is a bug in the kernel. Since the request and the reply for a transaction are synchronous we don't have to match the reply to the request. I would remove this condition but leave the assertion [EITAN] + if (request_nlmsg->nlmsg_seq != reply_nlmsg->nlmsg_seq) { + ovs_assert(request_nlmsg->nlmsg_seq == reply_nlmsg->nlmsg_seq); + VLOG_DBG_RL(&rl, "mismatched seq request %#"PRIx32 + ", reply %#"PRIx32, request_nlmsg->nlmsg_seq, + reply_nlmsg->nlmsg_seq); + break; + } - /* If reply was expected, verify if there was indeed a reply - * received. */ - if (reply_len == 0) { - nl_sock_record_errors__(transactions, n, 0); - VLOG_DBG_RL(&rl, "reply not seen when expected seq %#"PRIx32, - request_nlmsg->nlmsg_seq); - break; + /* Handle errors embedded within the netlink message. */ + ofpbuf_use_stub(&tmp_reply, reply_buf, sizeof reply_buf); + ofpbuf_set_size(&tmp_reply, sizeof reply_buf); + if (nl_msg_nlmsgerr(&tmp_reply, &txn->error)) { + if (txn->reply) { + ofpbuf_clear(txn->reply); } - - /* Copy the reply to the buffer specified by the caller. */ - if (reply_len > txn->reply->allocated) { - ofpbuf_reinit(txn->reply, reply_len); + if (txn->error) { + VLOG_DBG_RL(&rl, "received NAK error=%d (%s)", + error, ovs_strerror(txn->error)); } - memcpy(ofpbuf_data(txn->reply), tail, reply_len); - ofpbuf_set_size(txn->reply, reply_len); - - /* Handle errors embedded within the netlink message. */ - if (nl_msg_nlmsgerr(txn->reply, &txn->error)) { - if (txn->reply) { - ofpbuf_clear(txn->reply); - } - if (txn->error) { - VLOG_DBG_RL(&rl, "received NAK error=%d (%s)", - error, ovs_strerror(txn->error)); + } else { + txn->error = 0; + if (txn->reply) { + /* Copy the reply to the buffer specified by the caller. */ + if (reply_len > txn->reply->allocated) { + ofpbuf_reinit(txn->reply, reply_len); } - } else { - txn->error = 0; + memcpy(ofpbuf_data(txn->reply), reply_buf, reply_len); + ofpbuf_set_size(txn->reply, reply_len); } } + ofpbuf_uninit(&tmp_reply); /* Count the number of successful transactions. */ (*done)++; + } if (!error) { -- 1.7.4.1 _______________________________________________ dev mailing list dev@openvswitch.org https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=ywI6QX9u4uPfiqMz%2BPxOIp18z6FJyvOFLidQH3%2Fhvyg%3D%0A&s=71932ad9aa65ebef7a58a1c8c55181ce56140fbd7d925a6662383c076ee45c0f _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
