Re: [ovs-dev] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Ben Pfaff]

On Wed, May 08, 2019 at 11:41:16AM +0300, Ilya Maximets wrote:
> > On Thu, May 02, 2019 at 05:29:16PM -0700, Darrell Ball wrote:
> >> There are a few cases where structure copy can be replaced by
> >> memcpy(), for possible portability benefit.  This is because
> >> the structures involved have padding and elements of the
> >> structure are used to generate hashes.
> >> 
> >> Signed-off-by: Darrell Ball 
> > 
> > Thanks for the backports.  I applied them to branch-2.9.
> 
> Hi. These patches broke the clang build on branch-2.9 and it looks
> like not only the build:
> 
> lib/conntrack.c:2563:26: error: expecting mutex 'ctb->lock' to be held at 
> start
>   of each loop [-Werror,-Wthread-safety-analysis]
> for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {

Oops.  I always test that master builds with clang, but I only test
backports with GCC.  Maybe I should do both for backports too.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Ilya Maximets]

On 08.05.2019 12:20, Darrell Ball wrote:
> 
> 
> On Wed, May 8, 2019 at 1:41 AM Ilya Maximets  > wrote:
> 
> > On Thu, May 02, 2019 at 05:29:16PM -0700, Darrell Ball wrote:
> >> There are a few cases where structure copy can be replaced by
> >> memcpy(), for possible portability benefit.  This is because
> >> the structures involved have padding and elements of the
> >> structure are used to generate hashes.
> >>
> >> Signed-off-by: Darrell Ball http://gmail.com>>
> >
> > Thanks for the backports.  I applied them to branch-2.9.
> 
> Hi. These patches broke the clang build on branch-2.9 and it looks
> like not only the build:
> 
> 
> Thanks for the report Ilya
> 
> Besides the clang false positive, what else was broken ?

Nothing. Was an issue with -Wno-null-pointer-arithmetic on branch-2.9 that I
mistakenly treated as some other problem. We, probably, may want to backport
a7021b08b0d5 ("configure: Disable -Wnull-pointer-arithmetic Clang warning.")
because branches <= 2.9 fails to build with relatively new clang.

Thanks for the fix for false positives. I'll check it.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Darrell Ball]

On Wed, May 8, 2019 at 1:41 AM Ilya Maximets  wrote:

> > On Thu, May 02, 2019 at 05:29:16PM -0700, Darrell Ball wrote:
> >> There are a few cases where structure copy can be replaced by
> >> memcpy(), for possible portability benefit.  This is because
> >> the structures involved have padding and elements of the
> >> structure are used to generate hashes.
> >>
> >> Signed-off-by: Darrell Ball 
> >
> > Thanks for the backports.  I applied them to branch-2.9.
>
> Hi. These patches broke the clang build on branch-2.9 and it looks
> like not only the build:
>

Thanks for the report Ilya

Besides the clang false positive, what else was broken ?



>
> lib/conntrack.c:2563:26: error: expecting mutex 'ctb->lock' to be held at
> start
>   of each loop [-Werror,-Wthread-safety-analysis]
> for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {
>  ^
> lib/conntrack.c:2566:9: note: mutex acquired here
> ct_lock_lock(&ctb->lock);
> ^
> lib/conntrack.c:2575:9: error: releasing mutex '->buckets[i].lock' that
> was not
>   held [-Werror,-Wthread-safety-analysis]
> ct_lock_unlock(&ct->buckets[i].lock);
> ^
> 2 errors generated.
> make[2]: *** [lib/conntrack.lo] Error 1
>
> https://travis-ci.org/openvswitch/ovs/builds/528984734
>
>
> Best regards, Ilya Maximets.
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Ilya Maximets]

> On Thu, May 02, 2019 at 05:29:16PM -0700, Darrell Ball wrote:
>> There are a few cases where structure copy can be replaced by
>> memcpy(), for possible portability benefit.  This is because
>> the structures involved have padding and elements of the
>> structure are used to generate hashes.
>> 
>> Signed-off-by: Darrell Ball 
> 
> Thanks for the backports.  I applied them to branch-2.9.

Hi. These patches broke the clang build on branch-2.9 and it looks
like not only the build:

lib/conntrack.c:2563:26: error: expecting mutex 'ctb->lock' to be held at start
  of each loop [-Werror,-Wthread-safety-analysis]
for (unsigned i = 0; i < CONNTRACK_BUCKETS; i++) {
 ^
lib/conntrack.c:2566:9: note: mutex acquired here
ct_lock_lock(&ctb->lock);
^
lib/conntrack.c:2575:9: error: releasing mutex '->buckets[i].lock' that was not
  held [-Werror,-Wthread-safety-analysis]
ct_lock_unlock(&ct->buckets[i].lock);
^
2 errors generated.
make[2]: *** [lib/conntrack.lo] Error 1

https://travis-ci.org/openvswitch/ovs/builds/528984734


Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.9 3/3] conntrack: Replace structure copy by memcpy().

[Ben Pfaff]

On Thu, May 02, 2019 at 05:29:16PM -0700, Darrell Ball wrote:
> There are a few cases where structure copy can be replaced by
> memcpy(), for possible portability benefit.  This is because
> the structures involved have padding and elements of the
> structure are used to generate hashes.
> 
> Signed-off-by: Darrell Ball 

Thanks for the backports.  I applied them to branch-2.9.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev