| Message ID | 20220711110655.28169-1-fsb4000@yandex.ru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] Build tests with asan and ubsan together to reduce CI time. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Igor, thank you for the patch. Yeah I was not quick enough to reply on the PR :) Acked-by: Ales Musil <amusil@redhat.com> On Mon, Jul 11, 2022 at 1:10 PM Igor Zhukov <fsb4000@yandex.ru> wrote: > From: Igor Zhukov <ivzhukov@sbercloud.ru> > > Also fix typo in comments > > Also fix memory leak in utilities/ovn-dbctl.c > > Also add "-g" to CFLAGS to forbid autotools from adding "-g -O2" > (temporary solution) > > Signed-off-by: Igor Zhukov <ivzhukov@sbercloud.ru> > Acked-by: Dumitru Ceara <dceara@redhat.com> > Submitted-at: https://github.com/ovn-org/ovn/pull/139 > --- > .ci/linux-build.sh | 16 +++++----------- > .github/workflows/test.yml | 10 ++++------ > tests/atlocal.in | 4 ++-- > tests/automake.mk | 3 +-- > tests/ovs-macros.at | 11 +++-------- > utilities/ovn-dbctl.c | 4 ++++ > 6 files changed, 19 insertions(+), 29 deletions(-) > > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh > index 570ddbace..8c6d751dc 100755 > --- a/.ci/linux-build.sh > +++ b/.ci/linux-build.sh > @@ -28,20 +28,14 @@ function configure_ovn() > save_OPTS="${OPTS} $*" > OPTS="${EXTRA_OPTS} ${save_OPTS}" > > -# If AddressSanitizer or UdefinedBehaviorSanitizer is requested, enable > it, > +# If AddressSanitizer and UndefinedBehaviorSanitizer are requested, > enable them, > # but only for OVN, not for OVS. However, disable some optimizations for > # OVS, to make sanitizer reports user friendly. > -if [ "$ASAN" ]; then > - CFLAGS="-O1 -fno-omit-frame-pointer -fno-common" > - CFLAGS_ASAN="-fsanitize=address" > - OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_ASAN}" > -fi > - > -if [ "$UBSAN" ]; then > +if [ "$SANITIZERS" ]; then > # Use the default options configured in tests/atlocal.in, in > UBSAN_OPTIONS. > - CFLAGS="-O1 -fno-omit-frame-pointer -fno-common" > - CFLAGS_UBSAN="-fsanitize=undefined" > - OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_UBSAN}" > + CFLAGS="-O1 -fno-omit-frame-pointer -fno-common -g" > + CFLAGS_SANITIZERS="-fsanitize=address,undefined" > + OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_SANITIZERS}" > fi > > if [ "$CC" = "clang" ]; then > diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml > index b24bc7a3b..e0de7c60e 100644 > --- a/.github/workflows/test.yml > +++ b/.github/workflows/test.yml > @@ -20,8 +20,8 @@ jobs: > M32: ${{ matrix.m32 }} > OPTS: ${{ matrix.opts }} > TESTSUITE: ${{ matrix.testsuite }} > - ASAN: ${{ matrix.asan }} > - UBSAN: ${{ matrix.ubsan }} > + SANITIZERS: ${{ matrix.sanitizers }} > + CFLAGS: ${{ matrix.cflags }} > > name: linux ${{ join(matrix.*, ' ') }} > runs-on: ubuntu-20.04 > @@ -41,10 +41,8 @@ jobs: > testsuite: system-test > - compiler: clang > testsuite: test > - asan: asan > - - compiler: clang > - testsuite: test > - ubsan: ubsan > + sanitizers: sanitizers > + cflags: -g > > - compiler: gcc > testsuite: test > diff --git a/tests/atlocal.in b/tests/atlocal.in > index b3a011f41..0b9a31276 100644 > --- a/tests/atlocal.in > +++ b/tests/atlocal.in > @@ -211,10 +211,10 @@ export OVS_CTL_TIMEOUT > > # Add some default flags to make the tests run better under Address > # Sanitizer, if it was used for the build. > > -ASAN_OPTIONS=detect_leaks=1:abort_on_error=true:log_path=asan:$ASAN_OPTIONS > > +ASAN_OPTIONS=detect_leaks=1:abort_on_error=true:log_path=sanitizers:$ASAN_OPTIONS > export ASAN_OPTIONS > > # Add some default flags for UndefinedBehaviorSanitizer, if it was used > # for the build. > > -UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=ubsan:$UBSAN_OPTIONS > > +UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=sanitizers:$UBSAN_OPTIONS > export UBSAN_OPTIONS > diff --git a/tests/automake.mk b/tests/automake.mk > index 9733e8fa0..dce9c9108 100644 > --- a/tests/automake.mk > +++ b/tests/automake.mk > @@ -80,8 +80,7 @@ export ovs_srcdir > check-local: > set $(SHELL) '$(TESTSUITE)' -C tests > AUTOTEST_PATH=$(AUTOTEST_PATH); \ > "$$@" $(TESTSUITEFLAGS) || \ > - (test -z "$$(find $(TESTSUITE_DIR) -name 'asan.*')" && \ > - test -z "$$(find $(TESTSUITE_DIR) -name 'ubsan.*')" && \ > + (test -z "$$(find $(TESTSUITE_DIR) -name 'sanitizers.*')" && \ > test X'$(RECHECK)' = Xyes && "$$@" --recheck) > > # Python Coverage support. > diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at > index 1bb31f315..5a06fe956 100644 > --- a/tests/ovs-macros.at > +++ b/tests/ovs-macros.at > @@ -224,14 +224,9 @@ m4_divert_pop([PREPARE_TESTS]) > > OVS_START_SHELL_HELPERS > ovs_cleanup() { > - if test "$(echo asan.*)" != 'asan.*'; then > - echo "Address Sanitizer reported errors in:" asan.* > - cat asan.* > - AT_FAIL_IF([:]) > - fi > - if test "$(echo ubsan.*)" != 'ubsan.*'; then > - echo "Undefined Behavior Sanitizer reported errors in:" ubsan.* > - cat ubsan.* > + if test "$(echo sanitizers.*)" != 'sanitizers.*'; then > + echo "Undefined Behavior Sanitizer or Address Sanitizer reported > errors in:" sanitizers.* > + cat sanitizers.* > AT_FAIL_IF([:]) > fi > } > diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c > index c4cc8c9b2..7ee0b92b4 100644 > --- a/utilities/ovn-dbctl.c > +++ b/utilities/ovn-dbctl.c > @@ -237,6 +237,10 @@ cleanup: > } > free(commands); > if (error) { > + for (int i = 0; i < argc; i++) { > + free(argv_[i]); > + } > + free(argv_); > ctl_fatal("%s", error); > } > } > -- > 2.30.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
Bleep bloop. Greetings Igor Zhukov, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 80 characters long (recommended limit is 79) #34 FILE: .ci/linux-build.sh:31: # If AddressSanitizer and UndefinedBehaviorSanitizer are requested, enable them, Lines checked: 154, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Mon, Jul 11, 2022 at 6:16 AM Ales Musil <amusil@redhat.com> wrote: > > Hi Igor, > > thank you for the patch. Yeah I was not quick enough to reply on the PR :) > > Acked-by: Ales Musil <amusil@redhat.com> Thanks for the patch. Can you please split this patch into 2 patches. Patch 1 to fix the memory leak in utilities/ovn-dbctl.c and the patch 2 with the other changes. This way we can backport the memory fix patch to the other branches. Numan > > > On Mon, Jul 11, 2022 at 1:10 PM Igor Zhukov <fsb4000@yandex.ru> wrote: > > > From: Igor Zhukov <ivzhukov@sbercloud.ru> > > > > Also fix typo in comments > > > > Also fix memory leak in utilities/ovn-dbctl.c > > > > Also add "-g" to CFLAGS to forbid autotools from adding "-g -O2" > > (temporary solution) > > > > Signed-off-by: Igor Zhukov <ivzhukov@sbercloud.ru> > > Acked-by: Dumitru Ceara <dceara@redhat.com> > > Submitted-at: https://github.com/ovn-org/ovn/pull/139 > > --- > > .ci/linux-build.sh | 16 +++++----------- > > .github/workflows/test.yml | 10 ++++------ > > tests/atlocal.in | 4 ++-- > > tests/automake.mk | 3 +-- > > tests/ovs-macros.at | 11 +++-------- > > utilities/ovn-dbctl.c | 4 ++++ > > 6 files changed, 19 insertions(+), 29 deletions(-) > > > > diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh > > index 570ddbace..8c6d751dc 100755 > > --- a/.ci/linux-build.sh > > +++ b/.ci/linux-build.sh > > @@ -28,20 +28,14 @@ function configure_ovn() > > save_OPTS="${OPTS} $*" > > OPTS="${EXTRA_OPTS} ${save_OPTS}" > > > > -# If AddressSanitizer or UdefinedBehaviorSanitizer is requested, enable > > it, > > +# If AddressSanitizer and UndefinedBehaviorSanitizer are requested, > > enable them, > > # but only for OVN, not for OVS. However, disable some optimizations for > > # OVS, to make sanitizer reports user friendly. > > -if [ "$ASAN" ]; then > > - CFLAGS="-O1 -fno-omit-frame-pointer -fno-common" > > - CFLAGS_ASAN="-fsanitize=address" > > - OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_ASAN}" > > -fi > > - > > -if [ "$UBSAN" ]; then > > +if [ "$SANITIZERS" ]; then > > # Use the default options configured in tests/atlocal.in, in > > UBSAN_OPTIONS. > > - CFLAGS="-O1 -fno-omit-frame-pointer -fno-common" > > - CFLAGS_UBSAN="-fsanitize=undefined" > > - OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_UBSAN}" > > + CFLAGS="-O1 -fno-omit-frame-pointer -fno-common -g" > > + CFLAGS_SANITIZERS="-fsanitize=address,undefined" > > + OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_SANITIZERS}" > > fi > > > > if [ "$CC" = "clang" ]; then > > diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml > > index b24bc7a3b..e0de7c60e 100644 > > --- a/.github/workflows/test.yml > > +++ b/.github/workflows/test.yml > > @@ -20,8 +20,8 @@ jobs: > > M32: ${{ matrix.m32 }} > > OPTS: ${{ matrix.opts }} > > TESTSUITE: ${{ matrix.testsuite }} > > - ASAN: ${{ matrix.asan }} > > - UBSAN: ${{ matrix.ubsan }} > > + SANITIZERS: ${{ matrix.sanitizers }} > > + CFLAGS: ${{ matrix.cflags }} > > > > name: linux ${{ join(matrix.*, ' ') }} > > runs-on: ubuntu-20.04 > > @@ -41,10 +41,8 @@ jobs: > > testsuite: system-test > > - compiler: clang > > testsuite: test > > - asan: asan > > - - compiler: clang > > - testsuite: test > > - ubsan: ubsan > > + sanitizers: sanitizers > > + cflags: -g > > > > - compiler: gcc > > testsuite: test > > diff --git a/tests/atlocal.in b/tests/atlocal.in > > index b3a011f41..0b9a31276 100644 > > --- a/tests/atlocal.in > > +++ b/tests/atlocal.in > > @@ -211,10 +211,10 @@ export OVS_CTL_TIMEOUT > > > > # Add some default flags to make the tests run better under Address > > # Sanitizer, if it was used for the build. > > > > -ASAN_OPTIONS=detect_leaks=1:abort_on_error=true:log_path=asan:$ASAN_OPTIONS > > > > +ASAN_OPTIONS=detect_leaks=1:abort_on_error=true:log_path=sanitizers:$ASAN_OPTIONS > > export ASAN_OPTIONS > > > > # Add some default flags for UndefinedBehaviorSanitizer, if it was used > > # for the build. > > > > -UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=ubsan:$UBSAN_OPTIONS > > > > +UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=sanitizers:$UBSAN_OPTIONS > > export UBSAN_OPTIONS > > diff --git a/tests/automake.mk b/tests/automake.mk > > index 9733e8fa0..dce9c9108 100644 > > --- a/tests/automake.mk > > +++ b/tests/automake.mk > > @@ -80,8 +80,7 @@ export ovs_srcdir > > check-local: > > set $(SHELL) '$(TESTSUITE)' -C tests > > AUTOTEST_PATH=$(AUTOTEST_PATH); \ > > "$$@" $(TESTSUITEFLAGS) || \ > > - (test -z "$$(find $(TESTSUITE_DIR) -name 'asan.*')" && \ > > - test -z "$$(find $(TESTSUITE_DIR) -name 'ubsan.*')" && \ > > + (test -z "$$(find $(TESTSUITE_DIR) -name 'sanitizers.*')" && \ > > test X'$(RECHECK)' = Xyes && "$$@" --recheck) > > > > # Python Coverage support. > > diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at > > index 1bb31f315..5a06fe956 100644 > > --- a/tests/ovs-macros.at > > +++ b/tests/ovs-macros.at > > @@ -224,14 +224,9 @@ m4_divert_pop([PREPARE_TESTS]) > > > > OVS_START_SHELL_HELPERS > > ovs_cleanup() { > > - if test "$(echo asan.*)" != 'asan.*'; then > > - echo "Address Sanitizer reported errors in:" asan.* > > - cat asan.* > > - AT_FAIL_IF([:]) > > - fi > > - if test "$(echo ubsan.*)" != 'ubsan.*'; then > > - echo "Undefined Behavior Sanitizer reported errors in:" ubsan.* > > - cat ubsan.* > > + if test "$(echo sanitizers.*)" != 'sanitizers.*'; then > > + echo "Undefined Behavior Sanitizer or Address Sanitizer reported > > errors in:" sanitizers.* > > + cat sanitizers.* > > AT_FAIL_IF([:]) > > fi > > } > > diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c > > index c4cc8c9b2..7ee0b92b4 100644 > > --- a/utilities/ovn-dbctl.c > > +++ b/utilities/ovn-dbctl.c > > @@ -237,6 +237,10 @@ cleanup: > > } > > free(commands); > > if (error) { > > + for (int i = 0; i < argc; i++) { > > + free(argv_[i]); > > + } > > + free(argv_); > > ctl_fatal("%s", error); > > } > > } > > -- > > 2.30.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com IM: amusil > <https://red.ht/sig> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 7/11/22 19:55, Numan Siddique wrote: > On Mon, Jul 11, 2022 at 6:16 AM Ales Musil <amusil@redhat.com> wrote: >> >> Hi Igor, >> >> thank you for the patch. Yeah I was not quick enough to reply on the PR :) >> >> Acked-by: Ales Musil <amusil@redhat.com> > > Thanks for the patch. > > Can you please split this patch into 2 patches. > > Patch 1 to fix the memory leak in utilities/ovn-dbctl.c and the patch > 2 with the other changes. This way we can backport the memory fix > patch to the other branches. I'd normally agree with this but I don't really think that we need to backport the memleak fix. That's unless we want to backport the rest too. The "leak" is benign, we call ctl_fatal() just after so the memory is not really leaked. Splitting the patch in two is also OK, obviously. :) Regards, Dumitru > > Numan > >> >> >> On Mon, Jul 11, 2022 at 1:10 PM Igor Zhukov <fsb4000@yandex.ru> wrote: >> >>> From: Igor Zhukov <ivzhukov@sbercloud.ru> >>> >>> Also fix typo in comments >>> >>> Also fix memory leak in utilities/ovn-dbctl.c >>> >>> Also add "-g" to CFLAGS to forbid autotools from adding "-g -O2" >>> (temporary solution) >>> >>> Signed-off-by: Igor Zhukov <ivzhukov@sbercloud.ru> >>> Acked-by: Dumitru Ceara <dceara@redhat.com> >>> Submitted-at: https://github.com/ovn-org/ovn/pull/139 >>> --- >>> .ci/linux-build.sh | 16 +++++----------- >>> .github/workflows/test.yml | 10 ++++------ >>> tests/atlocal.in | 4 ++-- >>> tests/automake.mk | 3 +-- >>> tests/ovs-macros.at | 11 +++-------- >>> utilities/ovn-dbctl.c | 4 ++++ >>> 6 files changed, 19 insertions(+), 29 deletions(-) >>> >>> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh >>> index 570ddbace..8c6d751dc 100755 >>> --- a/.ci/linux-build.sh >>> +++ b/.ci/linux-build.sh >>> @@ -28,20 +28,14 @@ function configure_ovn() >>> save_OPTS="${OPTS} $*" >>> OPTS="${EXTRA_OPTS} ${save_OPTS}" >>> >>> -# If AddressSanitizer or UdefinedBehaviorSanitizer is requested, enable >>> it, >>> +# If AddressSanitizer and UndefinedBehaviorSanitizer are requested, >>> enable them, >>> # but only for OVN, not for OVS. However, disable some optimizations for >>> # OVS, to make sanitizer reports user friendly. >>> -if [ "$ASAN" ]; then >>> - CFLAGS="-O1 -fno-omit-frame-pointer -fno-common" >>> - CFLAGS_ASAN="-fsanitize=address" >>> - OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_ASAN}" >>> -fi >>> - >>> -if [ "$UBSAN" ]; then >>> +if [ "$SANITIZERS" ]; then >>> # Use the default options configured in tests/atlocal.in, in >>> UBSAN_OPTIONS. >>> - CFLAGS="-O1 -fno-omit-frame-pointer -fno-common" >>> - CFLAGS_UBSAN="-fsanitize=undefined" >>> - OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_UBSAN}" >>> + CFLAGS="-O1 -fno-omit-frame-pointer -fno-common -g" >>> + CFLAGS_SANITIZERS="-fsanitize=address,undefined" >>> + OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_SANITIZERS}" >>> fi >>> >>> if [ "$CC" = "clang" ]; then >>> diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml >>> index b24bc7a3b..e0de7c60e 100644 >>> --- a/.github/workflows/test.yml >>> +++ b/.github/workflows/test.yml >>> @@ -20,8 +20,8 @@ jobs: >>> M32: ${{ matrix.m32 }} >>> OPTS: ${{ matrix.opts }} >>> TESTSUITE: ${{ matrix.testsuite }} >>> - ASAN: ${{ matrix.asan }} >>> - UBSAN: ${{ matrix.ubsan }} >>> + SANITIZERS: ${{ matrix.sanitizers }} >>> + CFLAGS: ${{ matrix.cflags }} >>> >>> name: linux ${{ join(matrix.*, ' ') }} >>> runs-on: ubuntu-20.04 >>> @@ -41,10 +41,8 @@ jobs: >>> testsuite: system-test >>> - compiler: clang >>> testsuite: test >>> - asan: asan >>> - - compiler: clang >>> - testsuite: test >>> - ubsan: ubsan >>> + sanitizers: sanitizers >>> + cflags: -g >>> >>> - compiler: gcc >>> testsuite: test >>> diff --git a/tests/atlocal.in b/tests/atlocal.in >>> index b3a011f41..0b9a31276 100644 >>> --- a/tests/atlocal.in >>> +++ b/tests/atlocal.in >>> @@ -211,10 +211,10 @@ export OVS_CTL_TIMEOUT >>> >>> # Add some default flags to make the tests run better under Address >>> # Sanitizer, if it was used for the build. >>> >>> -ASAN_OPTIONS=detect_leaks=1:abort_on_error=true:log_path=asan:$ASAN_OPTIONS >>> >>> +ASAN_OPTIONS=detect_leaks=1:abort_on_error=true:log_path=sanitizers:$ASAN_OPTIONS >>> export ASAN_OPTIONS >>> >>> # Add some default flags for UndefinedBehaviorSanitizer, if it was used >>> # for the build. >>> >>> -UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=ubsan:$UBSAN_OPTIONS >>> >>> +UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=sanitizers:$UBSAN_OPTIONS >>> export UBSAN_OPTIONS >>> diff --git a/tests/automake.mk b/tests/automake.mk >>> index 9733e8fa0..dce9c9108 100644 >>> --- a/tests/automake.mk >>> +++ b/tests/automake.mk >>> @@ -80,8 +80,7 @@ export ovs_srcdir >>> check-local: >>> set $(SHELL) '$(TESTSUITE)' -C tests >>> AUTOTEST_PATH=$(AUTOTEST_PATH); \ >>> "$$@" $(TESTSUITEFLAGS) || \ >>> - (test -z "$$(find $(TESTSUITE_DIR) -name 'asan.*')" && \ >>> - test -z "$$(find $(TESTSUITE_DIR) -name 'ubsan.*')" && \ >>> + (test -z "$$(find $(TESTSUITE_DIR) -name 'sanitizers.*')" && \ >>> test X'$(RECHECK)' = Xyes && "$$@" --recheck) >>> >>> # Python Coverage support. >>> diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at >>> index 1bb31f315..5a06fe956 100644 >>> --- a/tests/ovs-macros.at >>> +++ b/tests/ovs-macros.at >>> @@ -224,14 +224,9 @@ m4_divert_pop([PREPARE_TESTS]) >>> >>> OVS_START_SHELL_HELPERS >>> ovs_cleanup() { >>> - if test "$(echo asan.*)" != 'asan.*'; then >>> - echo "Address Sanitizer reported errors in:" asan.* >>> - cat asan.* >>> - AT_FAIL_IF([:]) >>> - fi >>> - if test "$(echo ubsan.*)" != 'ubsan.*'; then >>> - echo "Undefined Behavior Sanitizer reported errors in:" ubsan.* >>> - cat ubsan.* >>> + if test "$(echo sanitizers.*)" != 'sanitizers.*'; then >>> + echo "Undefined Behavior Sanitizer or Address Sanitizer reported >>> errors in:" sanitizers.* >>> + cat sanitizers.* >>> AT_FAIL_IF([:]) >>> fi >>> } >>> diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c >>> index c4cc8c9b2..7ee0b92b4 100644 >>> --- a/utilities/ovn-dbctl.c >>> +++ b/utilities/ovn-dbctl.c >>> @@ -237,6 +237,10 @@ cleanup: >>> } >>> free(commands); >>> if (error) { >>> + for (int i = 0; i < argc; i++) { >>> + free(argv_[i]); >>> + } >>> + free(argv_); >>> ctl_fatal("%s", error); >>> } >>> } >>> -- >>> 2.30.2 >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> >> -- >> >> Ales Musil >> >> Senior Software Engineer - OVN Core >> >> Red Hat EMEA <https://www.redhat.com> >> >> amusil@redhat.com IM: amusil >> <https://red.ht/sig> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
On Mon, Jul 11, 2022 at 1:45 PM Dumitru Ceara <dceara@redhat.com> wrote: > > On 7/11/22 19:55, Numan Siddique wrote: > > On Mon, Jul 11, 2022 at 6:16 AM Ales Musil <amusil@redhat.com> wrote: > >> > >> Hi Igor, > >> > >> thank you for the patch. Yeah I was not quick enough to reply on the PR :) > >> > >> Acked-by: Ales Musil <amusil@redhat.com> > > > > Thanks for the patch. > > > > Can you please split this patch into 2 patches. > > > > Patch 1 to fix the memory leak in utilities/ovn-dbctl.c and the patch > > 2 with the other changes. This way we can backport the memory fix > > patch to the other branches. > > I'd normally agree with this but I don't really think that we need to > backport the memleak fix. That's unless we want to backport the rest > too. The "leak" is benign, we call ctl_fatal() just after so the memory > is not really leaked. > > Splitting the patch in two is also OK, obviously. :) Thanks Dumitru for the explanation. I don't think there is a need to split the patch. I applied this patch to the main branch. Numan > > Regards, > Dumitru > > > > > Numan > > > >> > >> > >> On Mon, Jul 11, 2022 at 1:10 PM Igor Zhukov <fsb4000@yandex.ru> wrote: > >> > >>> From: Igor Zhukov <ivzhukov@sbercloud.ru> > >>> > >>> Also fix typo in comments > >>> > >>> Also fix memory leak in utilities/ovn-dbctl.c > >>> > >>> Also add "-g" to CFLAGS to forbid autotools from adding "-g -O2" > >>> (temporary solution) > >>> > >>> Signed-off-by: Igor Zhukov <ivzhukov@sbercloud.ru> > >>> Acked-by: Dumitru Ceara <dceara@redhat.com> > >>> Submitted-at: https://github.com/ovn-org/ovn/pull/139 > >>> --- > >>> .ci/linux-build.sh | 16 +++++----------- > >>> .github/workflows/test.yml | 10 ++++------ > >>> tests/atlocal.in | 4 ++-- > >>> tests/automake.mk | 3 +-- > >>> tests/ovs-macros.at | 11 +++-------- > >>> utilities/ovn-dbctl.c | 4 ++++ > >>> 6 files changed, 19 insertions(+), 29 deletions(-) > >>> > >>> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh > >>> index 570ddbace..8c6d751dc 100755 > >>> --- a/.ci/linux-build.sh > >>> +++ b/.ci/linux-build.sh > >>> @@ -28,20 +28,14 @@ function configure_ovn() > >>> save_OPTS="${OPTS} $*" > >>> OPTS="${EXTRA_OPTS} ${save_OPTS}" > >>> > >>> -# If AddressSanitizer or UdefinedBehaviorSanitizer is requested, enable > >>> it, > >>> +# If AddressSanitizer and UndefinedBehaviorSanitizer are requested, > >>> enable them, > >>> # but only for OVN, not for OVS. However, disable some optimizations for > >>> # OVS, to make sanitizer reports user friendly. > >>> -if [ "$ASAN" ]; then > >>> - CFLAGS="-O1 -fno-omit-frame-pointer -fno-common" > >>> - CFLAGS_ASAN="-fsanitize=address" > >>> - OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_ASAN}" > >>> -fi > >>> - > >>> -if [ "$UBSAN" ]; then > >>> +if [ "$SANITIZERS" ]; then > >>> # Use the default options configured in tests/atlocal.in, in > >>> UBSAN_OPTIONS. > >>> - CFLAGS="-O1 -fno-omit-frame-pointer -fno-common" > >>> - CFLAGS_UBSAN="-fsanitize=undefined" > >>> - OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_UBSAN}" > >>> + CFLAGS="-O1 -fno-omit-frame-pointer -fno-common -g" > >>> + CFLAGS_SANITIZERS="-fsanitize=address,undefined" > >>> + OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_SANITIZERS}" > >>> fi > >>> > >>> if [ "$CC" = "clang" ]; then > >>> diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml > >>> index b24bc7a3b..e0de7c60e 100644 > >>> --- a/.github/workflows/test.yml > >>> +++ b/.github/workflows/test.yml > >>> @@ -20,8 +20,8 @@ jobs: > >>> M32: ${{ matrix.m32 }} > >>> OPTS: ${{ matrix.opts }} > >>> TESTSUITE: ${{ matrix.testsuite }} > >>> - ASAN: ${{ matrix.asan }} > >>> - UBSAN: ${{ matrix.ubsan }} > >>> + SANITIZERS: ${{ matrix.sanitizers }} > >>> + CFLAGS: ${{ matrix.cflags }} > >>> > >>> name: linux ${{ join(matrix.*, ' ') }} > >>> runs-on: ubuntu-20.04 > >>> @@ -41,10 +41,8 @@ jobs: > >>> testsuite: system-test > >>> - compiler: clang > >>> testsuite: test > >>> - asan: asan > >>> - - compiler: clang > >>> - testsuite: test > >>> - ubsan: ubsan > >>> + sanitizers: sanitizers > >>> + cflags: -g > >>> > >>> - compiler: gcc > >>> testsuite: test > >>> diff --git a/tests/atlocal.in b/tests/atlocal.in > >>> index b3a011f41..0b9a31276 100644 > >>> --- a/tests/atlocal.in > >>> +++ b/tests/atlocal.in > >>> @@ -211,10 +211,10 @@ export OVS_CTL_TIMEOUT > >>> > >>> # Add some default flags to make the tests run better under Address > >>> # Sanitizer, if it was used for the build. > >>> > >>> -ASAN_OPTIONS=detect_leaks=1:abort_on_error=true:log_path=asan:$ASAN_OPTIONS > >>> > >>> +ASAN_OPTIONS=detect_leaks=1:abort_on_error=true:log_path=sanitizers:$ASAN_OPTIONS > >>> export ASAN_OPTIONS > >>> > >>> # Add some default flags for UndefinedBehaviorSanitizer, if it was used > >>> # for the build. > >>> > >>> -UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=ubsan:$UBSAN_OPTIONS > >>> > >>> +UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=sanitizers:$UBSAN_OPTIONS > >>> export UBSAN_OPTIONS > >>> diff --git a/tests/automake.mk b/tests/automake.mk > >>> index 9733e8fa0..dce9c9108 100644 > >>> --- a/tests/automake.mk > >>> +++ b/tests/automake.mk > >>> @@ -80,8 +80,7 @@ export ovs_srcdir > >>> check-local: > >>> set $(SHELL) '$(TESTSUITE)' -C tests > >>> AUTOTEST_PATH=$(AUTOTEST_PATH); \ > >>> "$$@" $(TESTSUITEFLAGS) || \ > >>> - (test -z "$$(find $(TESTSUITE_DIR) -name 'asan.*')" && \ > >>> - test -z "$$(find $(TESTSUITE_DIR) -name 'ubsan.*')" && \ > >>> + (test -z "$$(find $(TESTSUITE_DIR) -name 'sanitizers.*')" && \ > >>> test X'$(RECHECK)' = Xyes && "$$@" --recheck) > >>> > >>> # Python Coverage support. > >>> diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at > >>> index 1bb31f315..5a06fe956 100644 > >>> --- a/tests/ovs-macros.at > >>> +++ b/tests/ovs-macros.at > >>> @@ -224,14 +224,9 @@ m4_divert_pop([PREPARE_TESTS]) > >>> > >>> OVS_START_SHELL_HELPERS > >>> ovs_cleanup() { > >>> - if test "$(echo asan.*)" != 'asan.*'; then > >>> - echo "Address Sanitizer reported errors in:" asan.* > >>> - cat asan.* > >>> - AT_FAIL_IF([:]) > >>> - fi > >>> - if test "$(echo ubsan.*)" != 'ubsan.*'; then > >>> - echo "Undefined Behavior Sanitizer reported errors in:" ubsan.* > >>> - cat ubsan.* > >>> + if test "$(echo sanitizers.*)" != 'sanitizers.*'; then > >>> + echo "Undefined Behavior Sanitizer or Address Sanitizer reported > >>> errors in:" sanitizers.* > >>> + cat sanitizers.* > >>> AT_FAIL_IF([:]) > >>> fi > >>> } > >>> diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c > >>> index c4cc8c9b2..7ee0b92b4 100644 > >>> --- a/utilities/ovn-dbctl.c > >>> +++ b/utilities/ovn-dbctl.c > >>> @@ -237,6 +237,10 @@ cleanup: > >>> } > >>> free(commands); > >>> if (error) { > >>> + for (int i = 0; i < argc; i++) { > >>> + free(argv_[i]); > >>> + } > >>> + free(argv_); > >>> ctl_fatal("%s", error); > >>> } > >>> } > >>> -- > >>> 2.30.2 > >>> > >>> _______________________________________________ > >>> dev mailing list > >>> dev@openvswitch.org > >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >>> > >>> > >> > >> -- > >> > >> Ales Musil > >> > >> Senior Software Engineer - OVN Core > >> > >> Red Hat EMEA <https://www.redhat.com> > >> > >> amusil@redhat.com IM: amusil > >> <https://red.ht/sig> > >> _______________________________________________ > >> dev mailing list > >> dev@openvswitch.org > >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >> > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh index 570ddbace..8c6d751dc 100755 --- a/.ci/linux-build.sh +++ b/.ci/linux-build.sh @@ -28,20 +28,14 @@ function configure_ovn() save_OPTS="${OPTS} $*" OPTS="${EXTRA_OPTS} ${save_OPTS}" -# If AddressSanitizer or UdefinedBehaviorSanitizer is requested, enable it, +# If AddressSanitizer and UndefinedBehaviorSanitizer are requested, enable them, # but only for OVN, not for OVS. However, disable some optimizations for # OVS, to make sanitizer reports user friendly. -if [ "$ASAN" ]; then - CFLAGS="-O1 -fno-omit-frame-pointer -fno-common" - CFLAGS_ASAN="-fsanitize=address" - OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_ASAN}" -fi - -if [ "$UBSAN" ]; then +if [ "$SANITIZERS" ]; then # Use the default options configured in tests/atlocal.in, in UBSAN_OPTIONS. - CFLAGS="-O1 -fno-omit-frame-pointer -fno-common" - CFLAGS_UBSAN="-fsanitize=undefined" - OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_UBSAN}" + CFLAGS="-O1 -fno-omit-frame-pointer -fno-common -g" + CFLAGS_SANITIZERS="-fsanitize=address,undefined" + OVN_CFLAGS="${OVN_CFLAGS} ${CFLAGS_SANITIZERS}" fi if [ "$CC" = "clang" ]; then diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b24bc7a3b..e0de7c60e 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -20,8 +20,8 @@ jobs: M32: ${{ matrix.m32 }} OPTS: ${{ matrix.opts }} TESTSUITE: ${{ matrix.testsuite }} - ASAN: ${{ matrix.asan }} - UBSAN: ${{ matrix.ubsan }} + SANITIZERS: ${{ matrix.sanitizers }} + CFLAGS: ${{ matrix.cflags }} name: linux ${{ join(matrix.*, ' ') }} runs-on: ubuntu-20.04 @@ -41,10 +41,8 @@ jobs: testsuite: system-test - compiler: clang testsuite: test - asan: asan - - compiler: clang - testsuite: test - ubsan: ubsan + sanitizers: sanitizers + cflags: -g - compiler: gcc testsuite: test diff --git a/tests/atlocal.in b/tests/atlocal.in index b3a011f41..0b9a31276 100644 --- a/tests/atlocal.in +++ b/tests/atlocal.in @@ -211,10 +211,10 @@ export OVS_CTL_TIMEOUT # Add some default flags to make the tests run better under Address # Sanitizer, if it was used for the build. -ASAN_OPTIONS=detect_leaks=1:abort_on_error=true:log_path=asan:$ASAN_OPTIONS +ASAN_OPTIONS=detect_leaks=1:abort_on_error=true:log_path=sanitizers:$ASAN_OPTIONS export ASAN_OPTIONS # Add some default flags for UndefinedBehaviorSanitizer, if it was used # for the build. -UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=ubsan:$UBSAN_OPTIONS +UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=true:log_path=sanitizers:$UBSAN_OPTIONS export UBSAN_OPTIONS diff --git a/tests/automake.mk b/tests/automake.mk index 9733e8fa0..dce9c9108 100644 --- a/tests/automake.mk +++ b/tests/automake.mk @@ -80,8 +80,7 @@ export ovs_srcdir check-local: set $(SHELL) '$(TESTSUITE)' -C tests AUTOTEST_PATH=$(AUTOTEST_PATH); \ "$$@" $(TESTSUITEFLAGS) || \ - (test -z "$$(find $(TESTSUITE_DIR) -name 'asan.*')" && \ - test -z "$$(find $(TESTSUITE_DIR) -name 'ubsan.*')" && \ + (test -z "$$(find $(TESTSUITE_DIR) -name 'sanitizers.*')" && \ test X'$(RECHECK)' = Xyes && "$$@" --recheck) # Python Coverage support. diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at index 1bb31f315..5a06fe956 100644 --- a/tests/ovs-macros.at +++ b/tests/ovs-macros.at @@ -224,14 +224,9 @@ m4_divert_pop([PREPARE_TESTS]) OVS_START_SHELL_HELPERS ovs_cleanup() { - if test "$(echo asan.*)" != 'asan.*'; then - echo "Address Sanitizer reported errors in:" asan.* - cat asan.* - AT_FAIL_IF([:]) - fi - if test "$(echo ubsan.*)" != 'ubsan.*'; then - echo "Undefined Behavior Sanitizer reported errors in:" ubsan.* - cat ubsan.* + if test "$(echo sanitizers.*)" != 'sanitizers.*'; then + echo "Undefined Behavior Sanitizer or Address Sanitizer reported errors in:" sanitizers.* + cat sanitizers.* AT_FAIL_IF([:]) fi } diff --git a/utilities/ovn-dbctl.c b/utilities/ovn-dbctl.c index c4cc8c9b2..7ee0b92b4 100644 --- a/utilities/ovn-dbctl.c +++ b/utilities/ovn-dbctl.c @@ -237,6 +237,10 @@ cleanup: } free(commands); if (error) { + for (int i = 0; i < argc; i++) { + free(argv_[i]); + } + free(argv_); ctl_fatal("%s", error); } }