[ovs-dev] [PATCH] datapath-windows: Make GET_PID a separate IOCTL
Nithin Raju
nithin at vmware.com
Thu Apr 2 01:26:33 UTC 2015
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.
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.
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.
> /*
> * 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()
> /*
> * 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().
> +
> + 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.
> } 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”.
> static int
> get_sock_pid_from_kernel(struct nl_sock *sock)
> {
> + uint32_t size = 0;
Nuke the size stack variable.
> + 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)?
> &bytes, NULL)) {
> retval = EINVAL;
> } else {
> + if (bytes < sizeof(size)) {
sizeof(size) => sizeof(pid).
thanks,
-- Nithin
More information about the dev
mailing list