Hi Mark, On 14/04/2016 05:36, "Kavanagh, Mark B" <mark.b.kavan...@intel.com> wrote:
>Hi Daniele, > >One minor comment inline. > >Cheers, >Mark > >> >>The interface will be more similar to the cmap. >> >>Signed-off-by: Daniele Di Proietto <diproiet...@vmware.com> >>--- >> lib/hmap.c | 26 ++++++++++++-------------- >> lib/hmap.h | 7 ++++++- >> lib/sset.c | 12 +++++------- >> lib/sset.h | 7 ++++++- >> ofproto/ofproto-dpif.c | 8 +++----- >> 5 files changed, 32 insertions(+), 28 deletions(-) >> >>diff --git a/lib/hmap.c b/lib/hmap.c >>index b70ce51..9462c5e 100644 >>--- a/lib/hmap.c >>+++ b/lib/hmap.c >>@@ -236,24 +236,22 @@ hmap_random_node(const struct hmap *hmap) >> } >> >> /* Returns the next node in 'hmap' in hash order, or NULL if no nodes >>remain in >>- * 'hmap'. Uses '*bucketp' and '*offsetp' to determine where to begin >>- * iteration, and stores new values to pass on the next iteration into >>them >>- * before returning. >>+ * 'hmap'. Uses '*pos' to determine where to begin iteration, and >>updates >>+ * '*pos' to pass on the next iteration into them before returning. >> * >> * It's better to use plain HMAP_FOR_EACH and related functions, since >>they are >> * faster and better at dealing with hmaps that change during iteration. >> * >>- * Before beginning iteration, store 0 into '*bucketp' and '*offsetp'. >>- */ >>+ * Before beginning iteration, set '*pos' to all zeros. */ >> struct hmap_node * >> hmap_at_position(const struct hmap *hmap, >>- uint32_t *bucketp, uint32_t *offsetp) >>+ struct hmap_position *pos) >> { >> size_t offset; >> size_t b_idx; >> >>- offset = *offsetp; >>- for (b_idx = *bucketp; b_idx <= hmap->mask; b_idx++) { >>+ offset = pos->offset; >>+ for (b_idx = pos->bucket; b_idx <= hmap->mask; b_idx++) { >> struct hmap_node *node; >> size_t n_idx; >> >>@@ -261,11 +259,11 @@ hmap_at_position(const struct hmap *hmap, >> n_idx++, node = node->next) { >> if (n_idx == offset) { >> if (node->next) { >>- *bucketp = node->hash & hmap->mask; >>- *offsetp = offset + 1; >>+ pos->bucket = node->hash & hmap->mask; >>+ pos->offset = offset + 1; >> } else { >>- *bucketp = (node->hash & hmap->mask) + 1; >>- *offsetp = 0; >>+ pos->bucket = (node->hash & hmap->mask) + 1; >>+ pos->offset = 0; >> } >> return node; >> } >>@@ -273,8 +271,8 @@ hmap_at_position(const struct hmap *hmap, >> offset = 0; >> } >> >>- *bucketp = 0; >>- *offsetp = 0; >>+ pos->bucket = 0; >>+ pos->offset = 0; >> return NULL; >> } >> >>diff --git a/lib/hmap.h b/lib/hmap.h >>index 08c4719..9a96c5f 100644 >>--- a/lib/hmap.h >>+++ b/lib/hmap.h >>@@ -201,8 +201,13 @@ static inline struct hmap_node *hmap_first(const >>struct hmap *); >> static inline struct hmap_node *hmap_next(const struct hmap *, >> const struct hmap_node *); >> >>+struct hmap_position { >>+ unsigned int bucket; >>+ unsigned int offset; >>+}; >>+ >> struct hmap_node *hmap_at_position(const struct hmap *, >>- uint32_t *bucket, uint32_t *offset); >>+ struct hmap_position *); >> >> /* Returns the number of nodes currently in 'hmap'. */ >> static inline size_t >>diff --git a/lib/sset.c b/lib/sset.c >>index f9d4fc0..4fd3fae 100644 >>--- a/lib/sset.c >>+++ b/lib/sset.c >>@@ -251,21 +251,19 @@ sset_equals(const struct sset *a, const struct >>sset *b) >> } >> >> /* Returns the next node in 'set' in hash order, or NULL if no nodes >>remain in >>- * 'set'. Uses '*bucketp' and '*offsetp' to determine where to begin >>- * iteration, and stores new values to pass on the next iteration into >>them >>- * before returning. >>+ * 'set'. Uses '*pos' to determine where to begin iteration, and >>updates >>+ * '*pos' to pass on the next iteration into them before returning. >> * >> * It's better to use plain SSET_FOR_EACH and related functions, since >>they are >> * faster and better at dealing with ssets that change during iteration. >> * >>- * Before beginning iteration, store 0 into '*bucketp' and '*offsetp'. >>- */ >>+ * Before beginning iteration, set '*pos' to all zeros. */ >> struct sset_node * >>-sset_at_position(const struct sset *set, uint32_t *bucketp, uint32_t >>*offsetp) >>+sset_at_position(const struct sset *set, struct sset_position *pos) >> { >> struct hmap_node *hmap_node; >> >>- hmap_node = hmap_at_position(&set->map, bucketp, offsetp); >>+ hmap_node = hmap_at_position(&set->map, &pos->pos); >> return SSET_NODE_FROM_HMAP_NODE(hmap_node); >> } >> >>diff --git a/lib/sset.h b/lib/sset.h >>index 7d1d496..9c2f703 100644 >>--- a/lib/sset.h >>+++ b/lib/sset.h >>@@ -64,8 +64,13 @@ char *sset_pop(struct sset *); >> struct sset_node *sset_find(const struct sset *, const char *); >> bool sset_contains(const struct sset *, const char *); >> bool sset_equals(const struct sset *, const struct sset *); >>+ >>+struct sset_position { >>+ struct hmap_position pos; >>+}; > >[MK] Would a typedef be more appropriate here, as it would avoid the >additional layer of dereference? CodingStyle.md says: "Use typedefs sparingly. Code is clearer if the actual type is visible at the point of declaration. Do not, in general, declare a typedef for a struct, union, or enum." and I agree :-), so IMHO it'd be better to keep it as it is. What do you think? Thanks >>+ >> struct sset_node *sset_at_position(const struct sset *, >>- uint32_t *bucketp, uint32_t >>*offsetp); >>+ struct sset_position *); >> >> /* Set operations. */ >> void sset_intersect(struct sset *, const struct sset *); >>diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>index 530c49a..aceb11f 100644 >>--- a/ofproto/ofproto-dpif.c >>+++ b/ofproto/ofproto-dpif.c >>@@ -3558,8 +3558,7 @@ port_get_lacp_stats(const struct ofport *ofport_, >>struct >>lacp_slave_stats *stats >> } >> >> struct port_dump_state { >>- uint32_t bucket; >>- uint32_t offset; >>+ struct sset_position pos; >> bool ghost; >> >> struct ofproto_port port; >>@@ -3587,7 +3586,7 @@ port_dump_next(const struct ofproto *ofproto_, >>void *state_, >> state->has_port = false; >> } >> sset = state->ghost ? &ofproto->ghost_ports : &ofproto->ports; >>- while ((node = sset_at_position(sset, &state->bucket, >>&state->offset))) { >>+ while ((node = sset_at_position(sset, &state->pos))) { >> int error; >> >> error = port_query_by_name(ofproto_, node->name, &state->port); >>@@ -3602,8 +3601,7 @@ port_dump_next(const struct ofproto *ofproto_, >>void *state_, >> >> if (!state->ghost) { >> state->ghost = true; >>- state->bucket = 0; >>- state->offset = 0; >>+ memset(&state->pos, 0, sizeof state->pos); >> return port_dump_next(ofproto_, state_, port); >> } >> >>-- >>2.1.4 > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev