On Tue, Oct 08, 2013 at 12:26:18PM +0300, Alexandru Copot wrote:
> It contains only Set-Async-Config and Role status message definitions.
>
> Signed-off-by: Alexandru Copot <[email protected]>
> Cc: Daniel Baluta <[email protected]>
Thank you for the patch. I have some minor comments inline below:
> +/* Asynchronous message configuration. */
> +struct ofp14_async_config {
> + struct ofp_header header;
This seems a funny place for a comment that states the message type.
I'd move it up to the previous comment.
> + /* OFPT_GET_ASYNC_REPLY or OFPT_SET_ASYNC. */
> + /* Async config Property list - 0 or more */
> + struct ofp14_async_config_prop_header properties[0];
> +};
> +OFP_ASSERT(sizeof(struct ofp14_async_config) == 8);
> +
> +/* Async Config property types.
> +* Low order bit cleared indicates a property for the slave role.
> +* Low order bit set indicates a property for the master/equal role.
> +*/
> +enum ofp14_async_config_prop_type {
> + OFPACPT_PACKET_IN_SLAVE = 0, /* Packet-in mask for slave. */
> + OFPACPT_PACKET_IN_MASTER = 1, /* Packet-in mask for master. */
> + OFPACPT_PORT_STATUS_SLAVE = 2, /* Port-status mask for slave. */
> + OFPACPT_PORT_STATUS_MASTER = 3, /* Port-status mask for master. */
> + OFPACPT_FLOW_REMOVED_SLAVE = 4, /* Flow removed mask for slave. */
> + OFPACPT_FLOW_REMOVED_MASTER = 5, /* Flow removed mask for master. */
> + OFPACPT_ROLE_STATUS_SLAVE = 6, /* Role status mask for slave. */
> + OFPACPT_ROLE_STATUS_MASTER = 7, /* Role status mask for master. */
> + OFPACPT_TABLE_STATUS_SLAVE = 8, /* Table status mask for slave. */
> + OFPACPT_TABLE_STATUS_MASTER = 9, /* Table status mask for master. */
> + OFPACPT_REQUESTFORWARD_SLAVE = 10, /* RequestForward mask for slave. */
> + OFPACPT_REQUESTFORWARD_MASTER = 11, /* RequestForward mask for master. */
Why do the following start with OFPTFPT_ but the foregoing with
OFPACPT_?
> + OFPTFPT_EXPERIMENTER_SLAVE = 0xFFFE, /* Experimenter for slave. */
> + OFPTFPT_EXPERIMENTER_MASTER = 0xFFFF, /* Experimenter for master. */
> +};
We usually put only one blank line at a time:
> +
> +
> +/* Various reason based properties */
> +struct ofp14_async_config_prop_reasons {
This comment would be more readable if you moved it up above the
struct header ("'type' is one of ..."):
> + ovs_be16 type; /* One of OFPACPT_PACKET_IN_*,
> + OFPACPT_PORT_STATUS_*,
> + OFPACPT_FLOW_REMOVED_*,
> + OFPACPT_ROLE_STATUS_*,
> + OFPACPT_TABLE_STATUS_*,
> + OFPACPT_REQUESTFORWARD_*. */
> + ovs_be16 length; /* Length in bytes of this property. */
> + ovs_be32 mask; /* Bitmasks of reason values. */
> +};
> +OFP_ASSERT(sizeof(struct ofp14_async_config_prop_reasons) == 8);
> +
> +/* Experimenter async config property */
> +struct ofp14_async_config_prop_experimenter {
> + ovs_be16 type; /* One of OFPTFPT_EXPERIMENTER_SLAVE,
> + OFPTFPT_EXPERIMENTER_MASTER. */
> + ovs_be16 length; /* Length in bytes of this property. */
> + ovs_be32 experimenter; /* Experimenter ID which takes the same
> + form as in struct
> + ofp_experimenter_header. */
> + ovs_be32 exp_type; /* Experimenter defined. */
> + /* Followed by:
> + * - Exactly (length - 12) bytes containing the experimenter data, then
> + * - Exactly (length + 7)/8*8 - (length) (between 0 and 7)
> + * bytes of all-zero bytes */
I think that we should omit the following member. In my experience
these zero-length arrays do not make code easier to write:
> + ovs_be32 experimenter_data[0];
> +};
> +OFP_ASSERT(sizeof(struct ofp14_async_config_prop_experimenter) == 12);
We usually use only single blank lines:
> +
> +
> +/* Common header for all Role Properties */
> +struct ofp14_role_prop_header {
> + ovs_be16 type; /* One of OFPRPT_*. */
> + ovs_be16 length; /* Length in bytes of this property. */
> +};
> +OFP_ASSERT(sizeof(struct ofp14_role_prop_header) == 4);
> +
> +/* Role status event message. */
> +struct ofp14_role_status {
> + struct ofp_header header; /* Type OFPT_ROLE_REQUEST/OFPT_ROLE_REPLY. */
> + ovs_be32 role; /* One of OFPCR_ROLE_*. */
> + uint8_t reason; /* One of OFPCRR_*. */
> + uint8_t pad[3]; /* Align to 64 bits. */
> + ovs_be64 generation_id; /* Master Election Generation Id */
> +
I'd remove this member too, replacing it by a comment that explains
what follows:
> + /* Role Property list */
> + struct ofp14_role_prop_header properties[0];
> +};
> +OFP_ASSERT(sizeof(struct ofp14_role_status) == 24);
> +
> +/* What changed about the controller role */
> +enum ofp14_controller_role_reason {
> + OFPCRR_MASTER_REQUEST = 0, /* Another controller asked to be master. */
> + OFPCRR_CONFIG = 1, /* Configuration changed on the switch. */
> + OFPCRR_EXPERIMENTER = 2, /* Experimenter data changed. */
> +};
> +
> +/* Role property types.
> +*/
> +enum ofp14_role_prop_type {
> + OFPRPT_EXPERIMENTER = 0xFFFF, /* Experimenter property. */
> +};
> +
> +/* Experimenter role property */
> +struct ofp14_role_prop_experimenter {
> + ovs_be16 type; /* One of OFPRPT_EXPERIMENTER. */
> + ovs_be16 length; /* Length in bytes of this property. */
> + ovs_be32 experimenter; /* Experimenter ID which takes the same
> + form as in struct
> + ofp_experimenter_header. */
> + ovs_be32 exp_type; /* Experimenter defined. */
> + /* Followed by:
> + * - Exactly (length - 12) bytes containing the experimenter data, then
> + * - Exactly (length + 7)/8*8 - (length) (between 0 and 7)
> + * bytes of all-zero bytes */
I'd remove this member too:
> + ovs_be32 experimenter_data[0];
> +};
> +OFP_ASSERT(sizeof(struct ofp14_role_prop_experimenter) == 12);
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
