[ovs-dev] [PATCH 2/3] datapath-windows: Add Netlink vport command get

[Samuel Ghinet]

> All of the vport commands are implemented in Vport.c. Pls. move this to that file. Perhaps even the Vport dump. You can move all of them at once in a subsequent commit.
I know that. I was not sure of the approach I should take.

The linux ovs driver does all the netlink communication part in datapath.c. Except for the flows, which is in flow_netlink.c as I remember.
I had considered the same approach here. It may changed later the way you guys think it would look best.

> Are all the attributes mandatory? I'd think that one of OVS_VPORT_ATTR_PORT_NO or OVS_VPORT_ATTR_NAME are mandatory. Rest are optional.
Oops, my bad. I think I had tested this function upon not last commit, where the "optional" field did not exist yet. I fixed it.

> I am not sure if 'STATUS_DATA_NOT_ACCEPTED' is the correct error code to be returned here. The documentation says:
> "{Data Not Accepted} The TDI client could not handle the data received during an indication."
I have STATUS_INVALID_PARAMETER for now. May not be the best value though.

> Returning a transactional error with ENODEV would be appropriate I think, or you could return STATUS_INVALID_PARAMETER.
No. ENODEV will mean that the vport does not exist. Reason why I cannot use that error code here.
Later on we might need to consider a proper error value, either as transactional error or as error returned by DeviceIoControl, and apply this in one place for all netlink functions (all require the switch context).
Hmmm, in our implementation we replied with NL_ERROR_PERM for this case. Perhaps it might be a good fit.

Anyway, this approach we have:
[CODE]
    OvsAcquireCtrlLock();
    if (!gOvsSwitchContext) {
        OvsReleaseCtrlLock();
        return STATUS_INVALID_PARAMETER;
    }
    OvsReleaseCtrlLock();
[/CODE]
Is not proper either, because we cannot guarantee that after OvsReleaseCtrlLock, the extension will not detach or that gOvsSwitchContext will not become NULL or the memory it points to, deallocated.
We should normally handle the case where netlink commands are requested while the extension is detached, so that the detaching waits for all current netlink commands to finish, and that after the extension is detached, all OvsDeviceControl operations will fail.
But this will be another issue.

Regards,
Sam
________________________________________
From: Nithin Raju [nithin at vmware.com]
Sent: Thursday, September 25, 2014 9:51 PM
To: Samuel Ghinet
Cc: dev at openvswitch.org; Alin Serdean; Eitan Eliahu; Ankur Sharma; Saurabh Shah
Subject: Re: [PATCH 2/3] datapath-windows: Add Netlink vport command get

hi Samuel,
I had some minor comments. Looks good otherwise.

Acked-by: Nithin Raju <nithin at vmware.com>

On Sep 24, 2014, at 8:42 AM, Samuel Ghinet <sghinet at cloudbasesolutions.com> wrote:

> The transactional get vport command.
> This command uses the netlink transactional errors.
>
> Signed-off-by: Samuel Ghinet <sghinet at cloudbasesolutions.com>
> ---
> datapath-windows/ovsext/Datapath.c | 83 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 83 insertions(+)
>
> diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
> index 05c06b6..8a8c542 100644
> --- a/datapath-windows/ovsext/Datapath.c
> +++ b/datapath-windows/ovsext/Datapath.c
> @@ -1425,6 +1425,86 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
>     return STATUS_SUCCESS;
> }
>
> +static NTSTATUS
> +OvsGetVport(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> +            UINT32 *replyLen)

All of the vport commands are implemented in Vport.c. Pls. move this to that file. Perhaps even the Vport dump. You can move all of them at once in a subsequent commit.

> +{
> +    NDIS_STATUS status = STATUS_SUCCESS;

NDIS_STATUS => NTSTATUS.

> +    LOCK_STATE_EX lockState;
> +
> +    POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer;
> +    POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
> +    POVS_VPORT_ENTRY vport = NULL;
> +    NL_ERROR nlError = NL_ERROR_SUCCESS;
> +
> +    static const NL_POLICY ovsVportPolicy[] = {
> +        [OVS_VPORT_ATTR_PORT_NO] = { .type = NL_A_U32 },
> +        [OVS_VPORT_ATTR_TYPE] = { .type = NL_A_U32 },
> +        [OVS_VPORT_ATTR_NAME] = { .type = NL_A_STRING, .maxLen = IFNAMSIZ },
> +        [OVS_VPORT_ATTR_UPCALL_PID] = { .type = NL_A_UNSPEC },
> +        [OVS_VPORT_ATTR_STATS] = { .type = NL_A_UNSPEC,
> +                                   .minLen = sizeof(OVS_VPORT_FULL_STATS),
> +                                   .maxLen = sizeof(OVS_VPORT_FULL_STATS)
> +        },
> +        [OVS_VPORT_ATTR_OPTIONS] = { .type = NL_A_NESTED, .optional = TRUE },

Are all the attributes mandatory? I'd think that one of OVS_VPORT_ATTR_PORT_NO or OVS_VPORT_ATTR_NAME are mandatory. Rest are optional.

You can see an example here:
dpif_netlink_port_query_by_number()
-> dpif_netlink_port_query__()
   -> dpif_netlink_vport_transact()

 825     dpif_netlink_vport_init(&request);
 826     request.cmd = OVS_VPORT_CMD_GET;
 827     request.dp_ifindex = dpif->dp_ifindex;
 828     request.port_no = port_no;
 829     request.name = port_name;
 830
 831     error = dpif_netlink_vport_transact(&request, &reply, &buf);


> +    };
> +    PNL_ATTR vportAttrs[ARRAY_SIZE(ovsVportPolicy)];
> +
> +    /* input buffer has been validated while validating write dev op. */
> +    ASSERT(usrParamsCtx->inputBuffer != NULL);
> +
> +    if (!NlAttrParse((PNL_MSG_HDR)msgIn,
> +        NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN,
> +        ovsVportPolicy, vportAttrs, ARRAY_SIZE(vportAttrs))) {
> +        return STATUS_INVALID_PARAMETER;
> +    }
> +
> +    if (msgOut == NULL || usrParamsCtx->outputLength < sizeof *msgOut) {
> +        return STATUS_NDIS_INVALID_LENGTH;
> +    }
> +
> +    OvsAcquireCtrlLock();
> +    if (!gOvsSwitchContext) {
> +        OvsReleaseCtrlLock();
> +        *replyLen = 0;
> +        return STATUS_DATA_NOT_ACCEPTED

I am not sure if 'STATUS_DATA_NOT_ACCEPTED' is the correct error code to be returned here. The documentation says:
"{Data Not Accepted} The TDI client could not handle the data received during an indication."

Returning a transactional error with ENODEV would be appropriate I think, or you could return STATUS_INVALID_PARAMETER.