On Fri, Jun 12, 2015 at 02:36:21PM -0700, Jarno Rajahalme wrote: > After all, there are some cases in which both the insertion version > and removal version of a rule need to be considered. This makes the > cls_match a bit bigger, but makes classifier versioning much simpler > to understand. > > Also, avoid using type larger than int in an enum, as it is not > portable C. > > Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
You could use this to avoid having to change CLS_MAX_VERSION and CLS_NOT_REMOVED_VERSION if you change cls_version_t: diff --git a/lib/classifier.h b/lib/classifier.h index 9aec8b2..a25764e 100644 --- a/lib/classifier.h +++ b/lib/classifier.h @@ -308,6 +308,7 @@ #include "meta-flow.h" #include "pvector.h" #include "rculist.h" +#include "type-props.h" #ifdef __cplusplus extern "C" { @@ -330,8 +331,8 @@ typedef uint64_t cls_version_t; #define CLS_NO_VERSION 0 #define CLS_MIN_VERSION 1 /* Default version number to use. */ -#define CLS_MAX_VERSION (UINT64_MAX - 1) -#define CLS_NOT_REMOVED_VERSION UINT64_MAX +#define CLS_MAX_VERSION (TYPE_MAXIMUM(cls_version_t) - 1) +#define CLS_NOT_REMOVED_VERSION TYPE_MAXIMUM(cls_version_t) enum { CLS_MAX_INDICES = 3, /* Maximum number of lookup indices per subtable. */ cls_match has an *almost*-invariant that add_version <= remove_version. If we did the following, it would be a full invariant, I believe, and we could remove CLS_NO_VERSION. Using add_version == remove_version implies to my mind that the rule was added and removed at the same time, which seems a lot like it was never there. Is it correct codewise too (it passes the unit tests at least)? diff --git a/lib/classifier.c b/lib/classifier.c index 6487426..d76b3fb 100644 --- a/lib/classifier.c +++ b/lib/classifier.c @@ -100,7 +100,7 @@ cls_match_alloc(const struct cls_rule *rule, *CONST_CAST(const struct cls_rule **, &cls_match->cls_rule) = rule; *CONST_CAST(int *, &cls_match->priority) = rule->priority; *CONST_CAST(cls_version_t *, &cls_match->add_version) = rule->version; - atomic_init(&cls_match->remove_version, CLS_NO_VERSION); /* Initially + atomic_init(&cls_match->remove_version, rule->version); /* Initially invisible. */ miniflow_clone_inline(CONST_CAST(struct miniflow *, &cls_match->flow), &rule->match.flow, count); The 'version' member of cls_rule seems to have only marginal utility. Maybe we can remove it. No need to do that now though. Acked-by: Ben Pfaff <b...@nicira.com> _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev