On Tue, Dec 27, 2016 at 10:28:18PM -0800, Justin Pettit wrote:
>
> > 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.
OK, I made those changes.
> > 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.)
This was described pretty well in ovn-trace(8) in the second patch, so I
moved the appropriate bits of it to ovn-architecture(7) and changed
ovn-trace(8) to refer back to ovn-architecture(7).
> > 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.
I changed this to give an error if there are multiple matches.
> > 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.
OK, I added a --uuid option.
> Signed-off-by: Justin Pettit <jpet...@ovn.org>
Thanks, I converted this to an Acked-by.
I'll apply this in a few minutes.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
