[ovs-dev] [PATCH v3] ovs-hyperv: make kernel return values netlink socket like

Nithin Raju nithin at vmware.com
Wed Apr 29 14:50:24 UTC 2015


> On Apr 29, 2015, at 7:45 AM, Ben Pfaff <blp at 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 <nithin at vmware.com>
>> Acked-by: Sorin Vinturis <svinturis at 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;
>            }

Ben,
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.

thanks,
-- Nithin



More information about the dev mailing list