[ovs-dev] [PATCH v7 1/3] ofproto-dpif: Implement group callbacks

[Ben Pfaff]

On Tue, Oct 15, 2013 at 05:17:48PM +0900, Simon Horman wrote:
> This is a first step towards implementing the dpif side of groups.
> 
> In order to be useful the action translation code needs
> to be taught about groups.
> 
> Signed-off-by: Simon Horman <horms at verge.net.au>

The code in group_get_stats() worries me.  First, what's this?  I
don't see why one would expect to have "struct bucket_counter"s
following the 'ogs' argument:
    struct bucket_counter *bc = (struct bucket_counter *)(ogs + 1);

Second, if the following comment in group_get_stats() is correct, then
I don't see how push_all_stats__() helps.  If the group that
group_get_stats() operates on can disappear then the right solution is
to take some lock or increment some refcount in the caller:
    /* push_all_stats() can handle flow misses which, when using the learn
     * action, can cause groups to be added and deleted.  This can corrupt our
     * caller's datastructures which assume that group_get_stats() doesn't have
     * an impact on the flow table. To be safe, we disable miss handling. */
    push_all_stats__(false);

Here are some tiny style fixes you can fold in:

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index b0c358a..470e455 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4827,7 +4827,7 @@ group_dealloc(struct ofgroup *group_)
 
 static void
 group_construct_stats(struct group_dpif *group)
-    OVS_REQUIRES(&group->stats_mutex)
+    OVS_REQUIRES(group->stats_mutex)
 {
     group->packet_count = 0;
     group->byte_count = 0;
@@ -4853,7 +4853,7 @@ group_construct(struct ofgroup *group_)
 
 static void
 group_destruct__(struct group_dpif *group)
-    OVS_REQUIRES(&group->stats_mutex)
+    OVS_REQUIRES(group->stats_mutex)
 {
     free(group->bucket_stats);
     group->bucket_stats = NULL;
@@ -4885,7 +4885,6 @@ group_modify(struct ofgroup *group_, struct ofgroup *victim_)
     return 0;
 }
 
-
 static enum ofperr
 group_get_stats(const struct ofgroup *group_, struct ofputil_group_stats *ogs)
 {