Hi Nithin, Thanks for your review. Please see my answers inline.
--Sorin -----Original Message----- From: Nithin Raju [mailto:nit...@vmware.com] Sent: Thursday, 2 April, 2015 04:27 To: Sorin Vinturis Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] datapath-windows: Make GET_PID a separate IOCTL hi Sorin, Thanks for the patch. It is great that you are addressing many issues that have been pending for sometime. I don’t know if there’s a whole lot of technical advantage to using an separate ioctl v/s using a netlink command, but I can see that it makes the get_sock_pid_from_kernel() functions cleaner. So, I’ll buy it for that. On thing though is that, we may not be able to make such changes in the future once we hit release milestones (such as 2.4) in the interest of backwards compatibility. I had the following comments about the patch itself: > diff --git a/datapath-windows/include/OvsDpInterfaceExt.h > b/datapath-windows/include/OvsDpInterfaceExt.h > index 7e09caf..620ebfc 100644 > --- a/datapath-windows/include/OvsDpInterfaceExt.h > +++ b/datapath-windows/include/OvsDpInterfaceExt.h > @@ -46,6 +46,9 @@ > #define OVS_IOCTL_TRANSACT \ > CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x4, > METHOD_OUT_DIRECT,\ > FILE_WRITE_ACCESS) > +#define OVS_IOCTL_GET_PID \ > + CTL_CODE (OVS_IOCTL_DEVICE_TYPE, OVS_IOCTL_START + 0x5, METHOD_BUFFERED,\ > + FILE_WRITE_ACCESS) Pls. remove inclusion of odp-netlink.h (if it is not required elsewhere). I had added it earlier to be able to use the netlink message format. SV: There is no other mention of the 'odp-netlink.h' header other than in 'netlink-socket.c'. I’d prefer to keep this ioctl at OVS_IOCTL_START + 0x0 based on the order in which the ioctls are called, and push down the others. SV: OK, will do that. If we are dedicating a separate ioctl, we should also get rid of OVS_CTRL_CMD_WIN_GET_PID netlink command. Also, pls. update the documentation in OvsDpInterfaceExt.h to say that some of the ioctls are netlink message based, and some or not. SV: I have removed the 'OVS_CTRL_CMD_WIN_GET_PID' Netlink command and documented the IOCTLs in 'OvsDpInterfaceExt.h'. > /* > * On platforms that support netlink natively, the operating system > assigns a diff --git a/datapath-windows/ovsext/Datapath.c > b/datapath-windows/ovsext/Datapath.c > index 888c6ef..5a171b0 100644 > --- a/datapath-windows/ovsext/Datapath.c > +++ b/datapath-windows/ovsext/Datapath.c > @@ -87,8 +87,7 @@ typedef struct _NETLINK_FAMILY { } NETLINK_FAMILY, > *PNETLINK_FAMILY; > > /* Handlers for the various netlink commands. */ -static > NetlinkCmdHandler OvsGetPidCmdHandler, > - OvsPendEventCmdHandler, > +static NetlinkCmdHandler OvsPendEventCmdHandler, > OvsPendPacketCmdHandler, > OvsSubscribeEventCmdHandler, > OvsSubscribePacketCmdHandler, @@ -110,6 > +109,8 @@ static NTSTATUS HandleGetDpDump(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > UINT32 *replyLen); static NTSTATUS > HandleDpTransactionCommon( > POVS_USER_PARAMS_CONTEXT usrParamsCtx, UINT32 > *replyLen); > +static NTSTATUS OvsGetPidCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, > + UINT32 *replyLen); OvsGetPidCmdHandler() is no longer a netlink command handler. We could rename it to OvsGetPidIoctlHandler()/OvsGetPidHandler() SV: Renamed 'OvsGetPidCmdHandler()' to 'OvsGetPidHandler()'. > /* > * The various netlink families, along with the supported commands. > Most of @@ -120,11 +121,6 @@ static NTSTATUS > HandleDpTransactionCommon( > > /* Netlink control family: this is a Windows specific family. */ > NETLINK_CMD nlControlFamilyCmdOps[] = { > - { .cmd = OVS_CTRL_CMD_WIN_GET_PID, > - .handler = OvsGetPidCmdHandler, > - .supportedDevOp = OVS_TRANSACTION_DEV_OP, > - .validateDpIndex = FALSE, > - }, > { .cmd = OVS_CTRL_CMD_WIN_PEND_REQ, > .handler = OvsPendEventCmdHandler, > .supportedDevOp = OVS_WRITE_DEV_OP, @@ -736,6 +732,21 @@ > OvsDeviceControl(PDEVICE_OBJECT deviceObject, > * operation. > */ > switch (code) { > + case OVS_IOCTL_GET_PID: > + /* Both input buffer and output buffer use the same location. */ > + outputBuffer = irp->AssociatedIrp.SystemBuffer; > + > + if (outputBuffer) { > + InitUserParamsCtx(irp, instance, 0, NULL, > + inputBuffer, inputBufferLen, > + outputBuffer, outputBufferLen, > + &usrParamsCtx); > + > + status = OvsGetPidCmdHandler(&usrParamsCtx, &replyLen); > + } If ‘outputBuffer’ is NULL, I think we should return STATUS_NDIS_INVALID_LENGTH. I see that you are checking for the required length in OvsGetPidCmdHandler(). SV: I'll addressed this in the v2 patch. > + > + goto done; > + > case OVS_IOCTL_TRANSACT: > /* Both input buffer and output buffer are mandatory. */ > if (outputBufferLen != 0) { > @@ -992,38 +1003,34 @@ > InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, > > /* > * > ---------------------------------------------------------------------- > ---- > - * Command Handler for 'OVS_CTRL_CMD_WIN_GET_PID'. > + * Handler for 'OVS_IOCTL_GET_PID'. > * > * Each handle on the device is assigned a unique PID when the handle > is > + * created. This function passes the PID to userspace using > + METHOD_BUFFERED > + * method. > * > ---------------------------------------------------------------------- > ---- > */ > static NTSTATUS > OvsGetPidCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, > UINT32 *replyLen) > { > + NTSTATUS status = STATUS_SUCCESS; > + PUINT32 msgOut = (PUINT32)usrParamsCtx->outputBuffer; > > if (usrParamsCtx->outputLength >= sizeof *msgOut) { > POVS_OPEN_INSTANCE instance = > (POVS_OPEN_INSTANCE)usrParamsCtx->ovsInstance; > > RtlZeroMemory(msgOut, sizeof *msgOut); > + RtlCopyMemory(msgOut, &instance->pid, sizeof(*msgOut)); > *replyLen = sizeof *msgOut; > /* XXX: We might need to return the DP index as well. */ Nuke the XXX comment, since we have seen so far that DP index is not needed. SV: Will do. > } else { > + *replyLen = sizeof *msgOut; > + status = STATUS_NDIS_INVALID_LENGTH; > } > > + return status; > } > > /* > diff --git a/datapath-windows/ovsext/Flow.c > b/datapath-windows/ovsext/Flow.c index 97220b4..f25fe9a 100644 > --- a/datapath-windows/ovsext/Flow.c > +++ b/datapath-windows/ovsext/Flow.c > @@ -319,7 +319,7 @@ OvsFlowNlCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, > rc = OvsPutFlowIoctl(&mappedFlow, sizeof (struct OvsFlowPut), > &stats); > if (rc != STATUS_SUCCESS) { > - OVS_LOG_ERROR("OvsFlowPut failed."); > + OVS_LOG_ERROR("OvsPutFlowIoctl failed."); > goto done; > } > > diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c index > df889d3..a12bb22 100644 > --- a/lib/netlink-socket.c > +++ b/lib/netlink-socket.c > @@ -273,63 +273,27 @@ nl_sock_destroy(struct nl_sock *sock) > > #ifdef _WIN32 > /* Reads the pid for 'sock' generated in the kernel datapath. The > function > - * follows a transaction semantic. Eventually this function should > call into > - * nl_transact. */ > + * does not follow a transaction semantic to avoid the unnecessary > + overhead. */ You can rephrase the comment to make it more informative as: “The function uses a separate ioctl instead of the transaction semantic to unnecessary message overhead”. SV: OK. > static int > get_sock_pid_from_kernel(struct nl_sock *sock) { > + uint32_t size = 0; Nuke the size stack variable. SV: Good catch. I wanted to use that variable to get the return size, then I changed the code and forgot to remove it. > + uint32_t pid = 0; > + int retval = 0; > + DWORD bytes = 0; > > + if (!DeviceIoControl(sock->handle, OVS_IOCTL_GET_PID, > + NULL, 0, &pid, sizeof(size), Shouldn’t argument #6 be, sizeof(pid)? SV: Yes, it should. > &bytes, NULL)) { > retval = EINVAL; > } else { > + if (bytes < sizeof(size)) { sizeof(size) => sizeof(pid). SV: OK. thanks, -- Nithin _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
