On Wed, Apr 29, 2015 at 02:50:24PM +0000, Nithin Raju wrote: > > On Apr 29, 2015, at 7:45 AM, Ben Pfaff <b...@nicira.com> wrote: > > > > On Tue, Apr 28, 2015 at 02:35:37PM -0700, Nithin Raju wrote: > >> In this patch, we make changes to usersapce as well as > >> kernel datapath on hyperv to make it more netlink socket > >> like. Previously, the kernel datapath did not distinguish > >> between "transport errors" and other errors. Netlink > >> semantics dictate that netlink functions should only > >> return an error only in the case of a "transport error" > >> which is generally something fatal. Eg. failure to > >> communicate with the OVS module, or an invalid command > >> altogether. Other errors such as an unsupported action, > >> or an invalid flow key is not considered a "transport > >> error", and in such cases, netlink functions are to return > >> success with a 'struct nlmsgerr' populated in the output > >> buffer. > >> > >> This patch implements these semantics. > >> > >> Signed-off-by: Nithin Raju <nit...@vmware.com> > >> Acked-by: Sorin Vinturis <svintu...@cloudbasesolutions.com> > >> Reported-at: > >> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_openvswitch_ovs-2Dissues_issues_72&d=AwIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=v_AjBISRY1zQ2PaRDRBCQCB_0QaDXkNlGCVxB_HMiI8&s=E2N_DYBJ186eGsp82T5N3pns4iqbsu214tqGDF2bpqE&e= > >> > >> --- > >> v2: addressed Sorin's comments > >> v3: rebase to HEAD > > > > I applied this, thanks! > > > > This assert in nl_sock_transact_multiple__() will fire (killing the > > process) whenever the "if" condition is entered. Is it really > > desirable? > > > > 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; > > } > > As you know, the Windows kernel is synchronous in terms of netlink > messages. Transaction semantics are implemented in one call that > includes both the “request” and the “reply” in one shot. So, if > there’s a mismatch, it implies the kernel bungled the ‘nlmsg_seq’. So, > the assert was added to catch this. As the code matures, we can nuke > it perhaps, but we have not seen this fire so far. > > Pls. let me know if I’m missing anything.
In a case like that I'd usually just put a bare ovs_assert, instead of a whole error-handling block, since the ovs_assert by itself should cover it. At any rate, it's harmless. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
