On Fri, Jun 06, 2014 at 11:11:57AM +0100, Thomas Graf wrote:
> On 06/05/14 at 10:02pm, Ben Pfaff wrote:
> > +/* By default, criteria initialized by rule_criteria_init() will match 
> > flows
> > + * that are read-only, on the assumption that the collected flows won't be
> > + * modified.  This function to match only flows that are be modifiable.
>                            ^^^^^^^^^^^^^^^^
> 
> > + *
> > + * Specify 'override_readonly' as false in ordinary circumstances, true if 
> > the
> > + * caller has special privileges that allow it to modify even "read-only"
> > + * flows. */
> > +static void
> > +rule_criteria_require_rw(struct rule_criteria *criteria,
> > +                         bool override_readonly)
> > +{
> > +    criteria->include_readonly = override_readonly;
> 
> I think this needs to be inverted as you pass True if only want
> RW flows.

I think that it is correct, but it is confusing, and I could use
advice on how to make it clear.

There are three cases:

        * Flows will not be modified, so read-only flows should be
          included.  rule_criteria_require_rw() isn't called at all in
          that case, and thus criteria->include_readonly is true.

        * Flows will be modified, so read-only flows should not be
          included.  rule_criteria_require_rw(false) sets
          criteria->include_readonly to false.

        * Flows will be modified, but the caller has special
          privileges, so read-only flows should be included anyway.
          rule_criteria_require_rw(true) sets
          criteria->include_readonly to true.

Can you (or anyone) suggest better naming or comment wording to
explain this clearly?

Thanks,

Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to