On 23 Mar 2022, at 8:28, Adrian Moreno wrote:
> On 3/18/22 14:38, Eelco Chaudron wrote: >> >> >> On 9 Mar 2022, at 17:10, Adrian Moreno wrote: >> >>> Using the SHORT version of the *_SAFE loops makes the code cleaner >>> and less error-prone. So, use the SHORT version and remove the extra >>> variable when possible. >>> >>> In order to be able to use both long and short versions without changing >>> the name of the macro for all the clients, overload the existing name >>> and select the appropriate version depending on the number of arguments. >>> >>> Acked-by: Dumitru Ceara <[email protected]> >>> Signed-off-by: Adrian Moreno <[email protected]> >> >> Other than some small nits below, the patch looks good to me. >> >> >> Acked-by: Eelco Chaudron <[email protected]> >> >>> --- >>> include/openvswitch/list.h | 30 ++++++++++++++++++++++++++++-- >>> lib/conntrack.c | 4 ++-- >>> lib/fat-rwlock.c | 4 ++-- >>> lib/id-fpool.c | 3 +-- >>> lib/ipf.c | 22 +++++++++++----------- >>> lib/lldp/lldpd-structs.c | 7 +++---- >>> lib/lldp/lldpd.c | 8 ++++---- >>> lib/mcast-snooping.c | 12 ++++++------ >>> lib/netdev-afxdp.c | 4 ++-- >>> lib/netdev-dpdk.c | 4 ++-- >>> lib/ofp-msgs.c | 4 ++-- >>> lib/ovs-lldp.c | 12 ++++++------ >>> lib/ovsdb-idl.c | 30 +++++++++++++++--------------- >>> lib/seq.c | 4 ++-- >>> lib/tnl-ports.c | 16 ++++++++-------- >>> lib/unixctl.c | 8 ++++---- >>> lib/vconn.c | 4 ++-- >>> ofproto/connmgr.c | 8 ++++---- >>> ofproto/ofproto-dpif-ipfix.c | 4 ++-- >>> ofproto/ofproto-dpif-trace.c | 4 ++-- >>> ofproto/ofproto-dpif-xlate.c | 4 ++-- >>> ofproto/ofproto-dpif.c | 24 +++++++++++------------- >>> ovsdb/jsonrpc-server.c | 16 ++++++++-------- >>> ovsdb/monitor.c | 24 ++++++++++++------------ >>> ovsdb/ovsdb.c | 4 ++-- >>> ovsdb/raft.c | 15 +++++++-------- >>> ovsdb/transaction-forward.c | 6 +++--- >>> ovsdb/transaction.c | 28 ++++++++++++++-------------- >>> ovsdb/trigger.c | 4 ++-- >>> tests/test-list.c | 29 +++++++++++++++++++++++++++++ >>> utilities/ovs-ofctl.c | 4 ++-- >>> utilities/ovs-vsctl.c | 8 ++++---- >>> vswitchd/bridge.c | 16 ++++++++-------- >>> vtep/vtep-ctl.c | 12 ++++++------ >>> 34 files changed, 218 insertions(+), 168 deletions(-) >>> >>> diff --git a/include/openvswitch/list.h b/include/openvswitch/list.h >>> index bbd2edbd0..6a7d4b2ff 100644 >>> --- a/include/openvswitch/list.h >>> +++ b/include/openvswitch/list.h >>> @@ -92,7 +92,8 @@ static inline bool ovs_list_is_short(const struct >>> ovs_list *); >>> CONDITION_MULTIVAR(VAR, MEMBER, ITER_VAR(VAR) != (LIST)); >>> \ >>> UPDATE_MULTIVAR(VAR, ITER_VAR(VAR)->prev)) >>> >>> -#define LIST_FOR_EACH_REVERSE_SAFE(VAR, PREV, MEMBER, LIST) >>> \ >>> +/* LONG version of SAFE iterators */ >> >> Ending sentence with a dot. >> >>> +#define LIST_FOR_EACH_REVERSE_SAFE_LONG(VAR, PREV, MEMBER, LIST) >>> \ >>> for (INIT_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, (LIST)->prev, >>> \ >>> struct ovs_list); >>> \ >>> CONDITION_MULTIVAR_SAFE_LONG(VAR, PREV, MEMBER, >>> \ >>> @@ -101,7 +102,7 @@ static inline bool ovs_list_is_short(const struct >>> ovs_list *); >>> ITER_VAR(PREV) != (LIST)); >>> \ >>> UPDATE_MULTIVAR_SAFE_LONG(VAR, PREV)) >>> >>> -#define LIST_FOR_EACH_SAFE(VAR, NEXT, MEMBER, LIST) >>> \ >>> +#define LIST_FOR_EACH_SAFE_LONG(VAR, NEXT, MEMBER, LIST) >>> \ >>> for (INIT_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, (LIST)->next, >>> \ >>> struct ovs_list); >>> \ >>> CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER, >>> \ >>> @@ -110,6 +111,31 @@ static inline bool ovs_list_is_short(const struct >>> ovs_list *); >>> ITER_VAR(NEXT) != (LIST)); >>> \ >>> UPDATE_MULTIVAR_SAFE_LONG(VAR, NEXT)) >> >> Do we really want to keep this MACRO as it’s not being used anywhere? >> > > You mean the _LONG verions of the loop MACRO? > The consensus was to keep backwards compatibility, i.e: allow the LONG > version of all MACROs, in order to: > - avoid forcing all clients to move inmediately to the shorter version > (including OVN) > - allow cases where "next" iterator is actually used inside the loop > - even if some macros are not used in the _LONG version, keep consistency > between them (e.g. we this series also changes the sset iterator) Yes the LONG versions, but you could ignore my comments here, as I misread some of the code on Friday (got it on Monday that’s why I sent the follow up ;) <SNIP> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
