[ovs-dev] [PATCH 10/16] ofproto: Move logic for collecting read-only rules into rule_criteria.

[Thomas Graf]

On 06/06/14 at 03:25pm, Ben Pfaff wrote:
> 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?

What confused me is that I interpreted the override_readonly = True
to overwrite the default policy as defined by _init(), i.e. one would
call rule_criteria_require_rw() to *not* include read-only flows.

After reading your explanation the code makes perfect sense.

How about:
  rule_criteria_require_rw([...] criteria, bool can_write_readonly)

Anyway, the code is obviously correct as-is, therefore:

Acked-by: Thomas Graf <tgraf at suug.ch>