> 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 [[email protected]]
Sent: Thursday, September 25, 2014 9:51 PM
To: Samuel Ghinet
Cc: [email protected]; 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 <[email protected]>
On Sep 24, 2014, at 8:42 AM, Samuel Ghinet <[email protected]>
wrote:
> The transactional get vport command.
> This command uses the netlink transactional errors.
>
> Signed-off-by: Samuel Ghinet <[email protected]>
> ---
> 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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
