Sorin,
Thanks for this patch. Even though it is committed, I had the following
comments.
It is nice to get rid of the checks to gOvsSwitchContext in multiple places. On
thing though is that while a netlink command handler is being executed, we
could get the detach handler from NDIS, and that might end up setting
gOvsSwitchContext to NULL (after cleanup). After the check of gOvsSwitchContext
at some high level function, we should add some kind of ref count on
gOvsSwitchContext and release it at the end of the netlink command handler.
I also had a few comments.
On Nov 14, 2014, at 3:35 AM, Sorin Vinturis <svintu...@cloudbasesolutions.com>
wrote:
> Right now the gOvsSwitchContext pointer is checked against NULL
> in a lot of places of the OVS extension code. This check should
> be done only once to avoid wasteful checks. Thus I have added the
> check in the dispatch routine, before doing any processing, and
> removed all other checks from the rest of the code.
>
> Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com>
> ---
> datapath-windows/ovsext/Datapath.c | 66 ++++++--------------------------------
> datapath-windows/ovsext/Flow.c | 12 +++----
> datapath-windows/ovsext/User.c | 11 -------
> datapath-windows/ovsext/Vport.c | 4 ---
> 4 files changed, 13 insertions(+), 80 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Datapath.c
> b/datapath-windows/ovsext/Datapath.c
> index 1b504a2..6de011c 100644
> --- a/datapath-windows/ovsext/Datapath.c
> +++ b/datapath-windows/ovsext/Datapath.c
> @@ -470,8 +470,7 @@ OvsGetOpenInstance(PFILE_OBJECT fileObject,
> POVS_OPEN_INSTANCE instance = (POVS_OPEN_INSTANCE)fileObject->FsContext;
> ASSERT(instance);
> ASSERT(instance->fileObject == fileObject);
> - if (gOvsSwitchContext == NULL ||
> - gOvsSwitchContext->dpNo != dpNo) {
> + if (gOvsSwitchContext->dpNo != dpNo) {
> return NULL;
> }
> return instance;
> @@ -653,7 +652,6 @@ NTSTATUS
> OvsDeviceControl(PDEVICE_OBJECT deviceObject,
> PIRP irp)
> {
> -
> PIO_STACK_LOCATION irpSp;
> NTSTATUS status = STATUS_SUCCESS;
> PFILE_OBJECT fileObject;
> @@ -690,6 +688,12 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
> outputBufferLen = irpSp->Parameters.DeviceIoControl.OutputBufferLength;
> inputBuffer = irp->AssociatedIrp.SystemBuffer;
>
> + /* Check if the extension is enabled. */
> + if (NULL == gOvsSwitchContext) {
> + status = STATUS_DEVICE_NOT_READY;
> + goto done;
> + }
> +
> /* 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.
> 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.
>
> @@ -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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
