> On Jan 2, 2018, at 10:13 AM, Ben Pfaff <b...@ovn.org> wrote: > > 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 <jpet...@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);
Thanks. I fixed those and ran checkpatch on the v2 of the series. > There's a stray " in this line in odp-util.h: > + enum user_action_cookie_type type; /* One of USER_ACTION_COOKIE_*" */ Good catch. > 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; > }; > }; Yes, I do think that is an improvement. > 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. I reworked the series in v2 to make it fixed length. I agree it makes the code cleaner. Thanks for the suggestion! --Justin _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev