On 3/3/22 07:43, Adrian Moreno wrote: > Thanks for the review Dumitru, > > A comment below. > > On 2/25/22 13:06, Dumitru Ceara wrote: >> On 2/16/22 15:27, Adrian Moreno wrote: >>> Multi-variable loop iterators avoid potential undefined behavior by >>> using an internal iterator variable to perform the iteration and only >>> referencing the containing object (via OBJECT_CONTAINING) if the >>> iterator has been validated via the second expression of the for >>> statement. >>> >>> That way, the user can easily implement a loop that never tries to >>> obtain the object containing NULL or stack-allocated non-contained >>> nodes. >>> >>> When the loop ends normally (not via "break;") the user-provided >>> variable is set to NULL. >>> >>> Signed-off-by: Adrian Moreno <amore...@redhat.com> >>> --- >> >> Hi Adrian, >> >> It feels a bit weird that these new macros are not used anywhere yet >> (until we apply the next patches in the series). On the other hand I >> don't think it's easy to split the series otherwise so, with a small nit >> below: >> >> Acked-by: Dumitru Ceara <dce...@redhat.com> >> >> Thanks, >> Dumitru >> >>> include/openvswitch/util.h | 44 ++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 44 insertions(+) >>> >>> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h >>> index 228b185c3..9c09e8aea 100644 >>> --- a/include/openvswitch/util.h >>> +++ b/include/openvswitch/util.h >>> @@ -145,6 +145,50 @@ OVS_NO_RETURN void ovs_assert_failure(const char >>> *, const char *, const char *); >>> #define INIT_CONTAINER(OBJECT, POINTER, MEMBER) \ >>> ((OBJECT) = NULL, ASSIGN_CONTAINER(OBJECT, POINTER, MEMBER)) >>> + >> >> Nit: We don't need this extra line. >> >>> +/* Multi-variable container iterators. >>> + * >>> + * The following macros facilitate safe iteration over data structures >>> + * contained in objects. It does so by using an internal iterator >>> variable of >>> + * the type of the member object pointer (i.e: pointer to the data >>> structure). >>> + */ >>> + >>> +/* Multi-variable iterator variable name. >>> + * Returns the name of the internal iterator variable. >>> + */ >>> +#define ITER_VAR(NAME) NAME ## __iterator__ >>> + >>> +/* Multi-variable initialization. Creates an internal iterator >>> variable that >>> + * points to the provided pointer. The type of the iterator variable is >>> + * ITER_TYPE*. It must be the same type as &VAR->MEMBER. >>> + * >>> + * The _EXP version evaluates the extra expressions once. >>> + */ >>> +#define INIT_MULTIVAR(VAR, MEMBER, POINTER, >>> ITER_TYPE) \ >>> + INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, (void) 0) >>> + >>> +#define INIT_MULTIVAR_EXP(VAR, MEMBER, POINTER, ITER_TYPE, >>> ...) \ >>> + ITER_TYPE *ITER_VAR(VAR) = ( __VA_ARGS__ , (ITER_TYPE *) POINTER) >>> + >>> +/* Multi-variable condition. >>> + * Evaluates the condition expression (that must be based on the >>> internal >>> + * iterator variable). Only if the result of expression is true, the >>> OBJECT is >>> + * set to the object containing the current value of the iterator >>> variable. >>> + * >>> + * It is up to the caller to make sure it is safe to run >>> OBJECT_CONTAINING on >>> + * the pointers that verify the condition. >>> + */ >>> +#define CONDITION_MULTIVAR(VAR, MEMBER, >>> EXPR) \ >>> + ((EXPR) >>> ? \ >>> + (((VAR) = OBJECT_CONTAINING(ITER_VAR(VAR), VAR, MEMBER)), 1) >>> : \ >>> + (((VAR) = NULL), 0)) >>> + >>> +/* Multi-variable update. >>> + * Evaluates the expresssion that is supposed to update the iterator >>> variable. >>> + */ >>> +#define UPDATE_MULTIVAR(VAR, >>> EXPR) \ >>> + ((EXPR), (VAR) = NULL) >>> + > > Setting VAR = NULL here may not really be needed. The CONDITION is > evaluated just after the UPDATE stage and it sets VAR appropriately. >
Good point. The same is true for the _SAFE_* versions in the next patch too, right? > If you agree, I'll remove the entire UPDATE macro alongside the extra > line and the extra "s" in "expression" in the next version. > Sounds good to me, thanks! _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
