[ovs-dev] [netlink v4 33/52] dpif: Eliminate "struct odp_flow" from client-visible interface.

[Justin Pettit]

On Jan 11, 2011, at 9:49 PM, Ben Pfaff wrote:

> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 2218c11..072f0ef 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> ...
> +    /* Adds or modifies a flow in 'dpif'.  The flow is specified by the Netlink
> +     * attributes with types ODPKT_* in the 'key_len' bytes starting at 'key'.
> +     * The associated actions are specified by the Netlink attributes with
> +     * types ODPAT_* in the 'actions_len' bytes starting at 'actions'.
> ...
> +    int (*flow_put)(struct dpif *, int flags,
> +                    const struct nlattr *key, size_t key_len,
> +                    const struct nlattr *actions, size_t actions_len,
> +                    struct odp_flow_stats *stats);

It would be nice to add "dpif" to the prototype, since it's reference in the comment.

> +    /* Deletes a flow from 'dpif' and returns 0, or returns ENOENT if 'dpif'
> +     * does not contain such a flow.  The flow is specified by the Netlink
> +     * attributes with types ODPKT_* in the 'key_len' bytes starting at 'key'.
> ...
> +    int (*flow_del)(struct dpif *,
> +                    const struct nlattr *key, size_t key_len,
> +                    struct odp_flow_stats *stats);

Same here.

>     /* Attempts to retrieve another flow from 'dpif' for 'state', which was
>      * initialized by a successful call to the 'flow_dump_start' function for.
> +     * 'dpif'.  On success, updates the output parameters as described below
> +     * and returns 0.  Returns EOF if the end of the flow table has been
> +     * reached, or a positive errno value on error.  This function will not be
> +     * called again once it returns nonzero once for a given iteration (but the

I think you can drop the second "once" in the description of "flow_dump_next".

> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> +/* Adds or modifies a flow in 'dpif'.  The flow is specified by the Netlink
> + * attributes with types ODPKT_* in the 'key_len' bytes starting at 'key'.  The
> + * associated actions are specified by the Netlink attributes with types
> + * ODPAT_* in the 'actions_len' bytes starting at 'actions'.
>  *
> + * - If the flow's key does not exist in 'dpif', then the flow will be added if
> + *   'flags' includes ODPPF_CREATE.  Otherwise the operation will fail with
>  *   ENOENT.
>  *
> + *   If the operation succeeds, then 'stats', if nonnull, must be zeroed.

Should that read "will be zeroed"?

> + *   If the operation succeeds, then 'stats', if nonnull, must be set to the
> + *   flow's statistics before the update.

"will be set"?

> int
> +dpif_flow_put(struct dpif *dpif, int flags,
> +              const struct nlattr *key, size_t key_len,
> +              const struct nlattr *actions, size_t actions_len,
> +              struct odp_flow_stats *stats)

It seems like those last two highlighted comments were taken from "dpif-provider.h", which describes how an implementation should behave as opposed to what the user should expect to happen in "dpif.c".

> @@ -802,47 +851,66 @@ dpif_flow_dump_start(struct dpif_flow_dump *dump, const struct dpif *dpif)
> }
> 
> /* Attempts to retrieve another flow from 'dump', which must have been
> + * initialized with dpif_flow_dump_start().  On success, updates the output
> + * parameters as described below and returns true.  Otherwise, returns false.
> + * Failure might indicate an actual error or merely the end of the flow table.
> + * An error status for the entire dump operation is provided when it is
> + * completed by calling dpif_flow_dump_done().
> + *
> + * On success, if 'key' and 'key_len' are nonnull then '*key' and '*key_len'
> + * must be set to Netlink attributes with types ODPKT_* representing the dumped

"will be set"?


> + * flow's key.  If 'actions' and 'actions_len' are nonnull then they are set to
> + * Netlink attributes with types ODPAT_* representing the dumped flow's
> + * actions.  If 'statsp' is nonnull then it will be set to the dumped flow's
> + * statistics.

I believe that should be 'stats' instead of 'statsp'.  I noticed that 'statsp' is also used for the parallel definitions in "dpif-provider.h", "dpif-linux.c", dpif-netdev.c", but not anyplace else.  Should it just be 'stats' everywhere to be consistent?

Otherwise, looks good.

Thanks,

--Justin