On 11/21/22 16:54, Adrian Moreno wrote:
> 
> 
> On 11/4/22 15:25, Ilya Maximets wrote:
>> Some macros for rculist have no users and there are no unit tests
>> specific to that library as well, so broken code wasn't spotted
>> while updating to multi-variable iterators.
>>
>> Fixing multiple problems like missing commas, parenthesis, incorrect
>> variable and macro names.
>>
>> Fixes: d293965d7b06 ("rculist: use multi-variable helpers for loop macros.")
>> Reported-by: Subrata Nath <subrata.n...@nokia.com>
>> Co-authored-by: Dumitru Ceara <dce...@redhat.com>
>> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
>> ---
>>   lib/rculist.h | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/rculist.h b/lib/rculist.h
>> index c0d77acf9..9bb8cbf3e 100644
>> --- a/lib/rculist.h
>> +++ b/lib/rculist.h
>> @@ -380,18 +380,18 @@ rculist_is_singleton_protected(const struct rculist 
>> *list)
>>   #define RCULIST_FOR_EACH_REVERSE_PROTECTED(ITER, MEMBER, RCULIST)          
>>    \
>>       for (INIT_MULTIVAR(ITER, MEMBER, (RCULIST)->prev, struct rculist);     
>>    \
>>            CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST));    
>>    \
>> -         UPDATE_MULTIVAR(ITER, ITER_VAR(VAR).prev))
>> +         UPDATE_MULTIVAR(ITER, ITER_VAR(ITER)->prev))
>>     #define RCULIST_FOR_EACH_REVERSE_PROTECTED_CONTINUE(ITER, MEMBER, 
>> RCULIST)    \
>>       for (INIT_MULTIVAR(ITER, MEMBER, (ITER)->MEMBER.prev, struct rculist); 
>>    \
>>            CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST));    
>>    \
>> -         UPDATE_MULTIVAR(ITER, ITER_VAR(VAR).prev))
>> +         UPDATE_MULTIVAR(ITER, ITER_VAR(ITER)->prev))
>>
> 
> There's another hidden problem with the REVERSE iterators that has not popped 
> up yet: They access 'prev' member directly which will not compile when using 
> clang's thread-safety macros because of a fake mutex specifically added to 
> avoid it.
> Since the macros are PROTECTED it should be OK to use 
> rculist_back_protected() instead.

Hmm, interesting.

> 
> I have written a unit test for rculist that I was planning to post soon. If 
> you prefer I can fix this at the same time.

If you can fix that in your patch that would be easier, I think.
I'll try to merge the current fix somewhere soon (Just got back
from PTO today).

Best regards, Ilya Maximets.

> 
> 
>>   #define RCULIST_FOR_EACH_PROTECTED(ITER, MEMBER, RCULIST)                  
>>    \
>>       for (INIT_MULTIVAR(ITER, MEMBER, rculist_next_protected(RCULIST),      
>>    \
>>                          struct rculist);                                    
>>    \
>>            CONDITION_MULTIVAR(ITER, MEMBER, ITER_VAR(ITER) != (RCULIST));    
>>    \
>> -         UPDATE_MULTIVAR(ITER, rculist_next_protected(ITER_VAR(ITER)))      
>>   \
>> +         UPDATE_MULTIVAR(ITER, rculist_next_protected(ITER_VAR(ITER))))     
>>   \
>>     #define RCULIST_FOR_EACH_SAFE_SHORT_PROTECTED(ITER, MEMBER, RCULIST)     
>>      \
>>       for (INIT_MULTIVAR_SAFE_SHORT(ITER, MEMBER,                            
>>    \
>> @@ -399,18 +399,18 @@ rculist_is_singleton_protected(const struct rculist 
>> *list)
>>                                     struct rculist);                         
>>    \
>>            CONDITION_MULTIVAR_SAFE_SHORT(ITER, MEMBER,                       
>>    \
>>                                          ITER_VAR(ITER) != (RCULIST),        
>>    \
>> -             ITER_NEXT_VAR(ITER) = rculist_next_protected(ITER_VAR(VAR)));  
>>   \
>> -        UPDATE_MULTIVAR_SHORT(ITER))
>> +             ITER_NEXT_VAR(ITER) = rculist_next_protected(ITER_VAR(ITER))); 
>>   \
>> +        UPDATE_MULTIVAR_SAFE_SHORT(ITER))
>>     #define RCULIST_FOR_EACH_SAFE_LONG_PROTECTED(ITER, NEXT, MEMBER, 
>> RCULIST)     \
>>       for (INIT_MULTIVAR_SAFE_LONG(ITER, NEXT, MEMBER,                       
>>    \
>> -                                 rculist_next_protected(RCULIST)            
>>   \
>> +                                 rculist_next_protected(RCULIST),           
>>   \
>>                                    struct rculist);                          
>>    \
>> -         CONDITION_MULTIVAR_SAFE_LONG(VAR, NEXT, MEMBER                     
>>   \
>> +         CONDITION_MULTIVAR_SAFE_LONG(ITER, NEXT, MEMBER,                   
>>   \
>>                                         ITER_VAR(ITER) != (RCULIST),         
>>    \
>> -             ITER_VAR(NEXT) = rculist_next_protected(ITER_VAR(VAR)),        
>>   \
>> +             ITER_VAR(NEXT) = rculist_next_protected(ITER_VAR(ITER)),       
>>   \
>>                                         ITER_VAR(NEXT) != (RCULIST));        
>>    \
>> -        UPDATE_MULTIVAR_LONG(ITER))
>> +        UPDATE_MULTIVAR_SAFE_LONG(ITER, NEXT))
>>     #define RCULIST_FOR_EACH_SAFE_PROTECTED(...)                             
>>      \
>>       OVERLOAD_SAFE_MACRO(RCULIST_FOR_EACH_SAFE_LONG_PROTECTED,              
>>    \
> 
> Thanks

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to