Hi Nithin, I would chose option #1, because I would expect some feedback message when sending any command. For me, an error message is very helpful to get an idea about what is going on. Otherwise a user could not understand the reason for not getting the expected output.
Thanks, Sorin -----Original Message----- From: Nithin Raju [mailto:nit...@vmware.com] Sent: Tuesday, 18 November, 2014 17:18 To: Sorin Vinturis Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] datapath-windows: Removed all duplicate checking of hi Sorin, Pls. see my explanations inline. >> /* Concurrent netlink operations are not supported. */ >> if (InterlockedCompareExchange((LONG volatile *)&instance->inUse, 1, 0)) { >> status = STATUS_RESOURCE_IN_USE; @@ -891,7 +895,7 @@ >> ValidateNetlinkCmd(UINT32 devOp, >> /* Validate the DP for commands that require a DP. */ >> if (nlFamilyOps->cmds[i].validateDpIndex == TRUE) { >> OvsAcquireCtrlLock(); >> - if (!gOvsSwitchContext || ovsMsg->ovsHdr.dp_ifindex != >> + if (ovsMsg->ovsHdr.dp_ifindex != >> >> (INT)gOvsSwitchContext->dpNo) { > > This check is required. Without this check, if you run ovs-dpctl.exe show, > while OVS is not enabled on the Hyper-V switch, it results in a BSOD. > [Sorin]: I have removed the check of the 'gOvsSwitchContext' pointer from the > 'ValidateNetlinkCmd()' function, because this is done in the upper dispatch > routine. Thanks for the explanation Sorin. I missed out the new check added at the top of OvsDeviceControl(). Given that you've written some code here, what in your opinion is a better user experience? option #1: # ovs-dpctl.exe show 2014-11-18T14:54:48Z|00001|dpif|WARN|failed to enumerate system datapaths: Invalid argument" # ie. an error thrown OR option #2: # ovs-dpctl.exe show # ie. no output. What we had implemented earlier was option #2. Basically, sending down a netlink command to kernel is fully supported regardless of whether OVS is enabled on the Hyper-V switch or not. We had thought through this, and had come up with this behavior. I agree with you that the bulk of the command cannot really work if OVS is not enabled, which is exactly why we have a 'validateDpIndex' field in each netlink command handler. Our thinking was that "enabling OVS" implies "datapath exists", and as long as we gate the top level function that enumerates the datapaths, we'll provide the best user experience. If ovs-vswitchd cannot enumerate the datapaths (ie. number of datapaths is 0), then it cannot really send down any other commands such as flow dump. If you agree with this explanation, I'd request you to revert a part of the patch where you added check for 'gOvsSwitchContext' at top of OvsDeviceControl(), and bring back the code in the places I pointed out in previous review. I agree with your other changes in spirit where we were doing a lot of checks on 'gOvsSwitchContext'. That solution needs to be completed by adding some sort of ref counting, and making sure that throughout the execution of a netlink command, 'gOvsSwitchContext' indeed stays valid, and does not get deleted via a call to the NDIS DetachHandler. > >> status = STATUS_INVALID_PARAMETER; >> OvsReleaseCtrlLock(); @@ -1184,13 +1188,6 @@ >> HandleGetDpDump(POVS_USER_PARAMS_CONTEXT usrParamsCtx, >> usrParamsCtx->outputLength); >> >> OvsAcquireCtrlLock(); >> - if (!gOvsSwitchContext) { >> - /* Treat this as a dump done. */ >> - OvsReleaseCtrlLock(); >> - *replyLen = 0; >> - FreeUserDumpState(instance); >> - return STATUS_SUCCESS; >> - } >> status = OvsDpFillInfo(gOvsSwitchContext, msgIn, &nlBuf); >> OvsReleaseCtrlLock(); > > The reason for acquiring and releasing the control lock is to protect the > gOvsSwitchContext pointer. We should consider getting rid of the lock as well. > [Sorin] Here, the OvsDpFillInfo accesses the switch context to obtain > statistics from the datapath. The control lock is required here to protect > against modifications of the datapath. Got it. Generally, we don't protect stats so much. OK, we'll revisit this as part of the ref counting. > >> >> @@ -1280,8 +1277,7 @@ >> HandleDpTransactionCommon(POVS_USER_PARAMS_CONTEXT usrParamsCtx, >> >> OvsAcquireCtrlLock(); >> if (dpAttrs[OVS_DP_ATTR_NAME] != NULL) { >> - if (!gOvsSwitchContext && >> - !OvsCompareString(NlAttrGet(dpAttrs[OVS_DP_ATTR_NAME]), >> + if (!OvsCompareString(NlAttrGet(dpAttrs[OVS_DP_ATTR_NAME]), >> OVS_SYSTEM_DP_NAME)) { > > This check is required. Without this, running "ovs-dpctl.exe show" without > enabling OVS leads to a BSOD. > [Sorin]: I have tested the patch for this scenario and didn't got a BSOD. > In my tests, if I execute "ovs-dpctl.exe show" I am getting the following > warning message:" 2014-11-18T14:54:48Z|00001|dpif|WARN|failed to enumerate > system datapaths: Invalid argument". Please note that I have applied > "datapath-windows: Avoid BSOD when switch context is NULL" patch, which > solves the mentioned BSOD. Right, I didn't see the check to 'gOvsSwitchContext' at the top of OvsDeviceControl(). Please see explanation above. Thanks, -- Nithin _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
