On 7/11/22 19:55, Numan Siddique wrote: > On Mon, Jul 11, 2022 at 6:16 AM Ales Musil <[email protected]> wrote: >> >> Hi Igor, >> >> thank you for the patch. Yeah I was not quick enough to reply on the PR :) >> >> Acked-by: Ales Musil <[email protected]> > > 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 <[email protected]> wrote: >> >>> From: Igor Zhukov <[email protected]> >>> >>> 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 <[email protected]> >>> Acked-by: Dumitru Ceara <[email protected]> >>> 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 >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> >> -- >> >> Ales Musil >> >> Senior Software Engineer - OVN Core >> >> Red Hat EMEA <https://www.redhat.com> >> >> [email protected] IM: amusil >> <https://red.ht/sig> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
