>
>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.
Ah yes, I seem to recall reading that some time back.
Apologies for the oversight on my part - consider this patch acked as-is.
Thanks,
Mark
>
>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
