[ovs-dev] [no-slow 4/6] ofproto: Add 'ofproto_uuid' and 'ofp_in_port' to user action cookie.

[Ben Pfaff]

On Thu, Dec 21, 2017 at 02:25:13PM -0800, Justin Pettit wrote:
> Previously, the ofproto instance and OpenFlow port have been derived
> based on the datapath port number.  This change explicitly declares them
> both, which will be helpful in future commits that no longer can depend
> on having a unique datapath port (e.g., a source port that represents
> the controller).
> 
> Signed-off-by: Justin Pettit <jpettit at ovn.org>

Some checkpatch warnings look legit:

WARNING: Line length is >79-characters long
#34 FILE: lib/odp-util.c:457:
                      && cookie.header.type == USER_ACTION_COOKIE_FLOW_SAMPLE) {

WARNING: Line length is >79-characters long
#259 FILE: ofproto/ofproto-dpif-upcall.c:1101:
             = ofproto_dpif_lookup_by_uuid(&upcall->cookie.header.ofproto_uuid);

WARNING: Line length is >79-characters long
#307 FILE: ofproto/ofproto-dpif-upcall.c:2072:
                          ofp_in_port, odp_actions, smid, cmid, &ofproto->uuid);

There's a stray " in this line in odp-util.h:
+    enum user_action_cookie_type type; /* One of USER_ACTION_COOKIE_*" */

Maybe you like the way you did it, which is fine, but an alternative way
to arrange user_action_cookie would be to make it a struct with the
standard header followed by embedding further structs inside an
anonymous union, like below.  Then you could omit lots of ".header."
stuff although you'd need to s/union/struct/ in other places.

/* user_action_cookie is passed as argument to OVS_ACTION_ATTR_USERSPACE. */
struct user_action_cookie {
    enum user_action_cookie_type type; /* One of USER_ACTION_COOKIE_*" */
    ofp_port_t ofp_in_port;            /* OpenFlow in port, or OFPP_NONE. */
    struct uuid ofproto_uuid;          /* UUID of ofproto-dpif. */

    union {
        struct {
            /* USER_ACTION_COOKIE_SFLOW. */
            ovs_be16 vlan_tci;      /* Destination VLAN TCI. */
            uint32_t output;        /* SFL_FLOW_SAMPLE_TYPE 'output' value. */
        } sflow;

        struct {
            /* USER_ACTION_COOKIE_SLOW_PATH. */
            uint32_t reason;        /* enum slow_path_reason. */
        } slow_path;

        struct {
            /* USER_ACTION_COOKIE_FLOW_SAMPLE. */
            uint16_t probability;   /* Sampling probability. */
            uint32_t collector_set_id; /* ID of IPFIX collector set. */
            uint32_t obs_domain_id; /* Observation Domain ID. */
            uint32_t obs_point_id;  /* Observation Point ID. */
            odp_port_t output_odp_port; /* The output odp port. */
            enum nx_action_sample_direction direction;
        } flow_sample;

        struct {
            /* USER_ACTION_COOKIE_IPFIX. */
            odp_port_t output_odp_port; /* The output odp port. */
        } ipfix;
    };
};

It looks like the shortest user_action_cookie variation is 24 bytes, the
longest is 48.  It may not be worth it to treat user_action_cookie as
variable-length now.  I think that it was done this way to better
tolerate old datapaths that had a short, fixed maximum length for
userdata.  It would be simpler to just always ship sizeof(union
user_action_cookie) bytes to the datapath and always require
sizeof(union user_action_cookie) back.  You could then skip a lot of
userdata_len checks and updates for each type of cookie.

Acked-by: Ben Pfaff <blp at ovn.org>