| Message ID | 20200828170305.43583-1-mark.d.gray@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v4,1/3] id-pool: Add interface to check if id has been allocated | expand |
Hi Mark, Sort of nitpicking but looks like making id_pool_find() public would be better because I don't know what is being checked in the new function. The new function is a simple wrapper around _find() and to me either _find() or _lookup() names translate better to what is being done. What do you think? fbl On Fri, Aug 28, 2020 at 06:03:03PM +0100, Mark Gray wrote: > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > --- > lib/id-pool.c | 10 ++++++++++ > lib/id-pool.h | 1 + > 2 files changed, 11 insertions(+) > > diff --git a/lib/id-pool.c b/lib/id-pool.c > index 69910ad08..2b085db64 100644 > --- a/lib/id-pool.c > +++ b/lib/id-pool.c > @@ -93,6 +93,16 @@ id_pool_find(struct id_pool *pool, uint32_t id) > return NULL; > } > > +bool > +id_pool_check(struct id_pool *pool, uint32_t id) > +{ > + if (!id_pool_find(pool, id)) { > + return false; > + } > + > + return true; > +} > + > void > id_pool_add(struct id_pool *pool, uint32_t id) > { > diff --git a/lib/id-pool.h b/lib/id-pool.h > index 8721f8793..6b642e4ff 100644 > --- a/lib/id-pool.h > +++ b/lib/id-pool.h > @@ -29,6 +29,7 @@ void id_pool_destroy(struct id_pool *); > bool id_pool_alloc_id(struct id_pool *, uint32_t *id); > void id_pool_free_id(struct id_pool *, uint32_t id); > void id_pool_add(struct id_pool *, uint32_t id); > +bool id_pool_check(struct id_pool *, uint32_t id); > > /* > * ID pool. > -- > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 01/09/2020 16:10, Flavio Leitner wrote: > > Hi Mark, > > Sort of nitpicking but looks like making id_pool_find() > public would be better because I don't know what is > being checked in the new function. > > The new function is a simple wrapper around _find() > and to me either _find() or _lookup() names translate > better to what is being done. > > What do you think? > > fbl > It's not nitpicking :D I did think of that but id_pool_find() returns a private object (id_node) so I'd have to expose that, breaking the encapsulation of id-pool which I didn't want to do. Perhaps I could change the public name to _lookup() or something like id_pool_is_id_avail()? Mark
On Fri, Sep 04, 2020 at 08:42:25AM +0100, Mark Gray wrote: > On 01/09/2020 16:10, Flavio Leitner wrote: > > > > Hi Mark, > > > > Sort of nitpicking but looks like making id_pool_find() > > public would be better because I don't know what is > > being checked in the new function. > > > > The new function is a simple wrapper around _find() > > and to me either _find() or _lookup() names translate > > better to what is being done. > > > > What do you think? > > > > fbl > > > > It's not nitpicking :D I did think of that but id_pool_find() returns a > private object (id_node) so I'd have to expose that, breaking the > encapsulation of id-pool which I didn't want to do. > > Perhaps I could change the public name to _lookup() or something like > id_pool_is_id_avail()? That works for me. OVS also uses _has_, so another suggestion would be id_pool_has_id(), which is shorter. ;) bool ofproto_has_snoops() bool connmgr_has_snoops() bool ofproto_rule_has_out_port() bool ofproto_rule_has_out_group() bool minimask_has_extra() bridge_has_bond_fake_iface() bool ovsdb_idl_has_lock() [...]
On 04/09/2020 13:59, Flavio Leitner wrote: > On Fri, Sep 04, 2020 at 08:42:25AM +0100, Mark Gray wrote: >> On 01/09/2020 16:10, Flavio Leitner wrote: >>> <snip> >> Perhaps I could change the public name to _lookup() or something like >> id_pool_is_id_avail()? > > That works for me. OVS also uses _has_, so another suggestion would > be id_pool_has_id(), which is shorter. ;) +1. I'll go for this. Thanks
diff --git a/lib/id-pool.c b/lib/id-pool.c index 69910ad08..2b085db64 100644 --- a/lib/id-pool.c +++ b/lib/id-pool.c @@ -93,6 +93,16 @@ id_pool_find(struct id_pool *pool, uint32_t id) return NULL; } +bool +id_pool_check(struct id_pool *pool, uint32_t id) +{ + if (!id_pool_find(pool, id)) { + return false; + } + + return true; +} + void id_pool_add(struct id_pool *pool, uint32_t id) { diff --git a/lib/id-pool.h b/lib/id-pool.h index 8721f8793..6b642e4ff 100644 --- a/lib/id-pool.h +++ b/lib/id-pool.h @@ -29,6 +29,7 @@ void id_pool_destroy(struct id_pool *); bool id_pool_alloc_id(struct id_pool *, uint32_t *id); void id_pool_free_id(struct id_pool *, uint32_t id); void id_pool_add(struct id_pool *, uint32_t id); +bool id_pool_check(struct id_pool *, uint32_t id); /* * ID pool.
Signed-off-by: Mark Gray <mark.d.gray@redhat.com> --- lib/id-pool.c | 10 ++++++++++ lib/id-pool.h | 1 + 2 files changed, 11 insertions(+)