hi Samuel,
I had some minor comments. Looks good otherwise.
Acked-by: Nithin Raju <nit...@vmware.com>
On Sep 24, 2014, at 8:42 AM, Samuel Ghinet <sghi...@cloudbasesolutions.com>
wrote:
> The transactional get vport command.
> This command uses the netlink transactional errors.
>
> Signed-off-by: Samuel Ghinet <sghi...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
