On Mon, Jun 09, 2014 at 10:28:25PM +0100, Thomas Graf wrote:
> 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)
That is a better name. I made this change.
> Anyway, the code is obviously correct as-is, therefore:
>
> Acked-by: Thomas Graf <[email protected]>
Thanks!
By the way, please let me know if at any point you want me to repost
the not-yet-applied patches. I am keeping the "learn" branch at
https://github.com/blp/ovs-reviews.git up-to-date with all of the
changes as I go, but I know that review is sometimes easier based on
patches.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
