[ovs-dev] [PATCH v2 06/26] ofproto: Add generic ofproto_collection.

[Jarno Rajahalme]

> 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