[ovs-dev] [PATCH v2 06/26] ofproto: Add generic ofproto_collection.
Jarno Rajahalme
jarno at ovn.org
Fri Jul 29 22:17:19 UTC 2016
> On Jul 29, 2016, at 12:46 PM, Ben Pfaff <blp at ovn.org> wrote:
>
> On Thu, Jul 28, 2016 at 05:55:58PM -0700, Jarno Rajahalme wrote:
>> Define rule_collection in terms of a new ofproto_collection. This
>> makes it easier to add other types of collections later.
>>
>> This patch makes no functional changes.
>>
>> Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
>
> Is there anything ofproto-specific about these collections? They might
> be appropriate as a generic data structure in lib/.
>
There is a small dependency for the _ref() and _unref() functions.
I added lib/object-collection.[ch] for the generic parts, and kept the ofproto dependent parts in ofproto-provider.h.
> At some point it becomes easier to implement this kind of thing as an
> #include file than as a long \-delimited macro. Then you also get the
> benefit that you can define macros in the #include file.
Elaborate a bit, I don't get what you mean.
>
> "sparse" warns:
> ../ofproto/ofproto.c:4363:5: warning: Using plain integer as NULL pointer
> ../ofproto/ofproto.c:4528:5: warning: Using plain integer as NULL pointer
> ../ofproto/ofproto.c:5091:9: warning: Using plain integer as NULL pointer
> ../ofproto/ofproto.c:5111:9: warning: Using plain integer as NULL pointer
> ../ofproto/ofproto.c:5176:9: warning: Using plain integer as NULL pointer
> ../ofproto/ofproto.c:5203:9: warning: Using plain integer as NULL pointer
> ../ofproto/ofproto.c:5255:5: warning: Using plain integer as NULL pointer
> ../ofproto/ofproto.c:5277:9: warning: Using plain integer as NULL pointer
> ../ofproto/ofproto.c:5349:5: warning: Using plain integer as NULL pointer
> ../ofproto/ofproto.c:5717:5: warning: Using plain integer as NULL pointer
>
> which is because C rules say that both sides of the ?: here get
> converted to a pointer type:
>
> #define RULE_COLLECTION_FOR_EACH(RULE, RULES) \
> for (size_t i__ = 0; \
> i__ < rule_collection_n(RULES) \
> ? (RULE = rule_collection_rules(RULES)[i__]) : false; \
> i__++)
>
> I suggest:
>
Thanks.
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index a82a398..9197813 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -540,14 +540,14 @@ DECL_COLLECTION(struct rule *, rule)
> #define RULE_COLLECTION_FOR_EACH(RULE, RULES) \
> for (size_t i__ = 0; \
> i__ < rule_collection_n(RULES) \
> - ? (RULE = rule_collection_rules(RULES)[i__]) : false; \
> + ? (RULE = rule_collection_rules(RULES)[i__]) != NULL: false; \
> i__++)
>
> #define RULE_COLLECTIONS_FOR_EACH(RULE1, RULE2, RULES1, RULES2) \
> for (size_t i__ = 0; \
> i__ < rule_collection_n(RULES1) \
> ? ((RULE1 = rule_collection_rules(RULES1)[i__]), \
> - (RULE2 = rule_collection_rules(RULES2)[i__])) \
> + (RULE2 = rule_collection_rules(RULES2)[i__]) != NULL) \
> : false; \
> i__++)
>
> RULE_COLLECTIONS_FOR_EACH could really use a comment describing what it
> does. I think that it pairwise traverses both collections, stopping
> when it runs out of rules in either collection.
>
It actually has a requirement that both collections are of the same size. I added a comment.
> Acked-by: Ben Pfaff <blp at ovn.org>
Thanks for the review!
Jarno
More information about the dev
mailing list