> On Jan 2, 2018, at 10:13 AM, Ben Pfaff <[email protected]> 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 <[email protected]>
>
> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
