> On Dec 21, 2016, at 3:25 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index 1d8fbf3..7449293 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -57,6 +57,7 @@ struct ovn_flow {
> /* Data. */
> struct ofpact *ofpacts;
> size_t ofpacts_len;
> + uint64_t cookie;
> };
>
> static uint32_t ovn_flow_hash(const struct ovn_flow *);
> @@ -602,7 +603,8 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype
> type)
> void
> ofctrl_add_flow(struct hmap *desired_flows,
> uint8_t table_id, uint16_t priority,
> - const struct match *match, const struct ofpbuf *actions)
> + const struct match *match, uint64_t cookie,
> + const struct ofpbuf *actions)
> {
All the other arguments for ofctrl_add_flow() are documented in the comment
above it, so it would be nice to add 'cookie'.
This is really minor, but all the metadata for the flow is before 'match', so
it might be more consistent to put 'cookie' there.
> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> index 9d37410..a84671e 100644
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
>
> @@ -935,7 +937,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id
> mff_ovn_geneve,
>
> /* Resubmit to table 33. */
> put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
> - ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match,
> &ofpacts);
> + ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150,
> + &match, 0, &ofpacts);
It might be nice to document in ovn-architecture how cookies are used and
defined--particularly on when they are "0" versus having a value (and how the
value was defined). (Thinking more about it, maybe it belongs in the
ovn-controller man page, since it's really an implementation detail.)
> diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
> index 92ae3e5..2c9c4a1 100644
> --- a/ovn/utilities/ovn-sbctl.c
> +++ b/ovn/utilities/ovn-sbctl.c
> ...
> +static const struct sbrec_datapath_binding *
> +lookup_datapath(struct ovsdb_idl *idl, const char *s)
> +{
> + const struct sbrec_datapath_binding *datapath;
> +
> + struct uuid uuid;
> + if (uuid_from_string(&uuid, s)) {
> + datapath = sbrec_datapath_binding_get_for_uuid(idl, &uuid);
> + if (datapath) {
> + return datapath;
> + }
> + }
> +
> + SBREC_DATAPATH_BINDING_FOR_EACH (datapath, idl) {
> + const char *name = smap_get(&datapath->external_ids, "name");
> + if (name && !strcmp(name, s)) {
> + return datapath;
> + }
> + }
> +
> + return NULL;
> +}
I don't think there's any requirement that the datapath name be unique, so it
may be worth noting here and in the documentation that if there are multiple
datapaths with the same name, it will only choose one at random.
> static void
> cmd_lflow_list(struct ctl_context *ctx)
> {
I'd think it would be helpful to provide an option to print the first 32-bits
of the logical flow's UUID when calling "ovn-sbctl lflow-list". That way,
there's a pretty clear way to look for those matching flows from "ovs-ofctl
dump-flows" without have to go to lower level commands.
Signed-off-by: Justin Pettit <jpet...@ovn.org>
--Justin
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
