On Tue, Dec 15, 2015 at 05:59:53PM -0800, Justin Pettit wrote:
>
> > On Dec 8, 2015, at 5:08 PM, Ben Pfaff <[email protected]> wrote:
> >
> > @@ -31,10 +32,11 @@ struct action_params {
> > * expr_parse()). */
> > const struct shash *symtab;
> >
> > - /* 'ports' must be a map from strings (presumably names of ports) to
> > - * integers (as one would provide to expr_to_matches()). Strings
> > used in
> > - * the actions that are not in 'ports' are translated to zero. */
> > - const struct simap *ports;
> > + /* Looks up logical port 'name'. If found, stores its port number in
> > + * '*portp' and returns true; otherwise, returns false. */
> > + bool (*lookup_port)(const void *aux, const char *name,
> > + unsigned int *portp);
>
> lookup_port_cb() refers to the second argument as "port_name" instead
> of "name". It might be nice to use the same argument names. If you
> change the name to "port_name", there are quite a few other protoypes
> defined in "expr.c" and "expr.h" that use "name", too.
Thanks. 'port_name' is better. I changed all the prototypes to
consistently use that name.
> > @@ -2443,19 +2452,21 @@ add_cmp_flow(const struct expr *cmp, const struct
> > simap *ports,
> > * (This treatment of string fields might be too simplistic in general, but
> > it
> > * seems reasonable for now when string fields are used only for ports.) */
> > uint32_t
> > -expr_to_matches(const struct expr *expr, const struct simap *ports,
> > - struct hmap *matches)
> > +expr_to_matches(const struct expr *expr,
> > + bool (*lookup_port)(const void *aux, const char *name,
> > + unsigned int *portp),
> > + const void *aux, struct hmap *matches)
> > {
>
> I think the comment describing this function needs to be updated,
> since "ports" is no longer an argument.
Thanks, I updated the comment.
> Acked-by: Justin Pettit <[email protected]>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
