Re: [ovs-dev] [PATCH] test-ovsdb: Fix false-positive leaks from LeakSanitizer.

2022-08-02 Thread Dumitru Ceara 
On 8/2/22 12:31, Dumitru Ceara wrote:
> On 7/29/22 19:52, Ilya Maximets wrote:
>> On 7/29/22 08:05, Ales Musil wrote:
>>>
>>> On Thu, Jul 28, 2022 at 5:43 PM Ilya Maximets >> > wrote:
>>>
>>> LeakSanitizer for some reason reports these json objects as leaked,
>>> even though we do have references to them at the moment ovs_fatal()
>>> called from check_ovsdb_error().
>>>
>>> Previously it complained only with -O2, but with newer versions of
>>> clang/llvm it started complaining even with -O1.  For example, negative
>>> ovsdb parsing tests are failing on ubuntu 22.04 with clang 14 if built
>>> with ASan and detect_leaks=1.
>>>
>>> Fix that by destroying the json object before aborting the process.
>>> And we may also build with default -O2 in CI with that change.
>>>
>>> Alternative implementation might be to just pass the json to destroy
>>> to every check_ovsdb_error() call, but indirect registering of the
>>> pointer seems a bit less invasive.
>>>
>>> Signed-off-by: Ilya Maximets >> >
>>> ---
>>>  .ci/linux-build.sh |  6 ++---
>>>  tests/test-ovsdb.c | 61 ++
>>>  2 files changed, 63 insertions(+), 4 deletions(-)
>>>
>>>
>>> Hi Ilya,
>>>
>>> I actually saw that couple of times even when running the OVN tests.
>>>
>>> Acked-by: Ales Musil mailto:amu...@redhat.com>>
>>
>> Thanks!  Applied and backported down to 2.17 to avoid possible issues
>> on a future LTS branch.
>>
> 
> Hi, Ales, Ilya,
> 
> Do you guys plan to send a similar patch for OVN too?
> 

Actually, after Ales reached out off-list, it turns out the failures I
remembered seeing in OVN CI are different [0] and we need to investigate
them separately.

Sorry for the noise.

Regards,
Dumitru

[0] https://github.com/ovsrobot/ovn/runs/7337939413?check_suite_focus=true


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

Re: [ovs-dev] [PATCH] test-ovsdb: Fix false-positive leaks from LeakSanitizer.

2022-08-02 Thread Dumitru Ceara 
On 7/29/22 19:52, Ilya Maximets wrote:
> On 7/29/22 08:05, Ales Musil wrote:
>>
>> On Thu, Jul 28, 2022 at 5:43 PM Ilya Maximets > > wrote:
>>
>> LeakSanitizer for some reason reports these json objects as leaked,
>> even though we do have references to them at the moment ovs_fatal()
>> called from check_ovsdb_error().
>>
>> Previously it complained only with -O2, but with newer versions of
>> clang/llvm it started complaining even with -O1.  For example, negative
>> ovsdb parsing tests are failing on ubuntu 22.04 with clang 14 if built
>> with ASan and detect_leaks=1.
>>
>> Fix that by destroying the json object before aborting the process.
>> And we may also build with default -O2 in CI with that change.
>>
>> Alternative implementation might be to just pass the json to destroy
>> to every check_ovsdb_error() call, but indirect registering of the
>> pointer seems a bit less invasive.
>>
>> Signed-off-by: Ilya Maximets > >
>> ---
>>  .ci/linux-build.sh |  6 ++---
>>  tests/test-ovsdb.c | 61 ++
>>  2 files changed, 63 insertions(+), 4 deletions(-)
>>
>>
>> Hi Ilya,
>>
>> I actually saw that couple of times even when running the OVN tests.
>>
>> Acked-by: Ales Musil mailto:amu...@redhat.com>>
> 
> Thanks!  Applied and backported down to 2.17 to avoid possible issues
> on a future LTS branch.
> 

Hi, Ales, Ilya,

Do you guys plan to send a similar patch for OVN too?

Thanks,
Dumitru

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

Re: [ovs-dev] [PATCH] test-ovsdb: Fix false-positive leaks from LeakSanitizer.

2022-07-29 Thread Ilya Maximets 
On 7/29/22 08:05, Ales Musil wrote:
> 
> On Thu, Jul 28, 2022 at 5:43 PM Ilya Maximets  > wrote:
> 
> LeakSanitizer for some reason reports these json objects as leaked,
> even though we do have references to them at the moment ovs_fatal()
> called from check_ovsdb_error().
> 
> Previously it complained only with -O2, but with newer versions of
> clang/llvm it started complaining even with -O1.  For example, negative
> ovsdb parsing tests are failing on ubuntu 22.04 with clang 14 if built
> with ASan and detect_leaks=1.
> 
> Fix that by destroying the json object before aborting the process.
> And we may also build with default -O2 in CI with that change.
> 
> Alternative implementation might be to just pass the json to destroy
> to every check_ovsdb_error() call, but indirect registering of the
> pointer seems a bit less invasive.
> 
> Signed-off-by: Ilya Maximets  >
> ---
>  .ci/linux-build.sh |  6 ++---
>  tests/test-ovsdb.c | 61 ++
>  2 files changed, 63 insertions(+), 4 deletions(-)
> 
> 
> Hi Ilya,
> 
> I actually saw that couple of times even when running the OVN tests.
> 
> Acked-by: Ales Musil mailto:amu...@redhat.com>>

Thanks!  Applied and backported down to 2.17 to avoid possible issues
on a future LTS branch.

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

Re: [ovs-dev] [PATCH] test-ovsdb: Fix false-positive leaks from LeakSanitizer.

2022-07-29 Thread Ales Musil 
On Thu, Jul 28, 2022 at 5:43 PM Ilya Maximets  wrote:

> LeakSanitizer for some reason reports these json objects as leaked,
> even though we do have references to them at the moment ovs_fatal()
> called from check_ovsdb_error().
>
> Previously it complained only with -O2, but with newer versions of
> clang/llvm it started complaining even with -O1.  For example, negative
> ovsdb parsing tests are failing on ubuntu 22.04 with clang 14 if built
> with ASan and detect_leaks=1.
>
> Fix that by destroying the json object before aborting the process.
> And we may also build with default -O2 in CI with that change.
>
> Alternative implementation might be to just pass the json to destroy
> to every check_ovsdb_error() call, but indirect registering of the
> pointer seems a bit less invasive.
>
> Signed-off-by: Ilya Maximets 
> ---
>  .ci/linux-build.sh |  6 ++---
>  tests/test-ovsdb.c | 61 ++
>  2 files changed, 63 insertions(+), 4 deletions(-)
>
> diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
> index c396ec1e8..22e138df3 100755
> --- a/.ci/linux-build.sh
> +++ b/.ci/linux-build.sh
> @@ -245,15 +245,13 @@ fi
>  if [ "$ASAN" ]; then
>  # This will override default option configured in tests/atlocal.in.
>  export ASAN_OPTIONS='detect_leaks=1'
> -# -O2 generates few false-positive memory leak reports in test-ovsdb
> -# application, so lowering optimizations to -O1 here.
> -CFLAGS_ASAN="-O1 -fno-omit-frame-pointer -fno-common
> -fsanitize=address"
> +CFLAGS_ASAN="-fno-omit-frame-pointer -fno-common -fsanitize=address"
>  CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_ASAN}"
>  fi
>
>  if [ "$UBSAN" ]; then
>  # Use the default options configured in tests/atlocal.in, in
> UBSAN_OPTIONS.
> -CFLAGS_UBSAN="-O1 -fno-omit-frame-pointer -fno-common
> -fsanitize=undefined"
> +CFLAGS_UBSAN="-fno-omit-frame-pointer -fno-common
> -fsanitize=undefined"
>  CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_UBSAN}"
>  fi
>
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index 343833151..965b0f913 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -300,11 +300,24 @@ print_and_free_ovsdb_error(struct ovsdb_error *error)
>  free(string);
>  }
>
> +static struct json **json_to_destroy;
> +
> +static void
> +destroy_on_ovsdb_error(struct json **json)
> +{
> +json_to_destroy = json;
> +}
> +
>  static void
>  check_ovsdb_error(struct ovsdb_error *error)
>  {
>  if (error) {
>  char *s = ovsdb_error_to_string_free(error);
> +
> +if (json_to_destroy) {
> +json_destroy(*json_to_destroy);
> +json_to_destroy = NULL;
> +}
>  ovs_fatal(0, "%s", s);
>  }
>  }
> @@ -487,6 +500,8 @@ do_diff_data(struct ovs_cmdl_context *ctx)
>  struct json *json;
>  struct ovsdb_datum new, old, diff, reincarnation;
>
> +destroy_on_ovsdb_error();
> +
>  json = unbox_json(parse_json(ctx->argv[1]));
>  check_ovsdb_error(ovsdb_type_from_json(, json));
>  json_destroy(json);
> @@ -562,6 +577,8 @@ do_parse_atomic_type(struct ovs_cmdl_context *ctx)
>  enum ovsdb_atomic_type type;
>  struct json *json;
>
> +destroy_on_ovsdb_error();
> +
>  json = unbox_json(parse_json(ctx->argv[1]));
>  check_ovsdb_error(ovsdb_atomic_type_from_json(, json));
>  json_destroy(json);
> @@ -574,6 +591,8 @@ do_parse_base_type(struct ovs_cmdl_context *ctx)
>  struct ovsdb_base_type base;
>  struct json *json;
>
> +destroy_on_ovsdb_error();
> +
>  json = unbox_json(parse_json(ctx->argv[1]));
>  check_ovsdb_error(ovsdb_base_type_from_json(, json));
>  json_destroy(json);
> @@ -587,6 +606,8 @@ do_parse_type(struct ovs_cmdl_context *ctx)
>  struct ovsdb_type type;
>  struct json *json;
>
> +destroy_on_ovsdb_error();
> +
>  json = unbox_json(parse_json(ctx->argv[1]));
>  check_ovsdb_error(ovsdb_type_from_json(, json));
>  json_destroy(json);
> @@ -601,6 +622,8 @@ do_parse_atoms(struct ovs_cmdl_context *ctx)
>  struct json *json;
>  int i;
>
> +destroy_on_ovsdb_error();
> +
>  json = unbox_json(parse_json(ctx->argv[1]));
>  check_ovsdb_error(ovsdb_base_type_from_json(, json));
>  json_destroy(json);
> @@ -630,6 +653,8 @@ do_parse_atom_strings(struct ovs_cmdl_context *ctx)
>  struct json *json;
>  int i;
>
> +destroy_on_ovsdb_error();
> +
>  json = unbox_json(parse_json(ctx->argv[1]));
>  check_ovsdb_error(ovsdb_base_type_from_json(, json));
>  json_destroy(json);
> @@ -675,6 +700,8 @@ do_parse_data__(int argc, char *argv[],
>  struct json *json;
>  int i;
>
> +destroy_on_ovsdb_error();
> +
>  json = unbox_json(parse_json(argv[1]));
>  check_ovsdb_error(ovsdb_type_from_json(, json));
>  json_destroy(json);
> @@ -706,6 +733,8 @@ do_parse_data_strings(struct ovs_cmdl_context *ctx)
>  struct json *json;
>  int i;
>
> +destroy_on_ovsdb_error();
> +
>

[ovs-dev] [PATCH] test-ovsdb: Fix false-positive leaks from LeakSanitizer.

2022-07-28 Thread Ilya Maximets 
LeakSanitizer for some reason reports these json objects as leaked,
even though we do have references to them at the moment ovs_fatal()
called from check_ovsdb_error().

Previously it complained only with -O2, but with newer versions of
clang/llvm it started complaining even with -O1.  For example, negative
ovsdb parsing tests are failing on ubuntu 22.04 with clang 14 if built
with ASan and detect_leaks=1.

Fix that by destroying the json object before aborting the process.
And we may also build with default -O2 in CI with that change.

Alternative implementation might be to just pass the json to destroy
to every check_ovsdb_error() call, but indirect registering of the
pointer seems a bit less invasive.

Signed-off-by: Ilya Maximets 
---
 .ci/linux-build.sh |  6 ++---
 tests/test-ovsdb.c | 61 ++
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index c396ec1e8..22e138df3 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -245,15 +245,13 @@ fi
 if [ "$ASAN" ]; then
 # This will override default option configured in tests/atlocal.in.
 export ASAN_OPTIONS='detect_leaks=1'
-# -O2 generates few false-positive memory leak reports in test-ovsdb
-# application, so lowering optimizations to -O1 here.
-CFLAGS_ASAN="-O1 -fno-omit-frame-pointer -fno-common -fsanitize=address"
+CFLAGS_ASAN="-fno-omit-frame-pointer -fno-common -fsanitize=address"
 CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_ASAN}"
 fi
 
 if [ "$UBSAN" ]; then
 # Use the default options configured in tests/atlocal.in, in UBSAN_OPTIONS.
-CFLAGS_UBSAN="-O1 -fno-omit-frame-pointer -fno-common -fsanitize=undefined"
+CFLAGS_UBSAN="-fno-omit-frame-pointer -fno-common -fsanitize=undefined"
 CFLAGS_FOR_OVS="${CFLAGS_FOR_OVS} ${CFLAGS_UBSAN}"
 fi
 
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index 343833151..965b0f913 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -300,11 +300,24 @@ print_and_free_ovsdb_error(struct ovsdb_error *error)
 free(string);
 }
 
+static struct json **json_to_destroy;
+
+static void
+destroy_on_ovsdb_error(struct json **json)
+{
+json_to_destroy = json;
+}
+
 static void
 check_ovsdb_error(struct ovsdb_error *error)
 {
 if (error) {
 char *s = ovsdb_error_to_string_free(error);
+
+if (json_to_destroy) {
+json_destroy(*json_to_destroy);
+json_to_destroy = NULL;
+}
 ovs_fatal(0, "%s", s);
 }
 }
@@ -487,6 +500,8 @@ do_diff_data(struct ovs_cmdl_context *ctx)
 struct json *json;
 struct ovsdb_datum new, old, diff, reincarnation;
 
+destroy_on_ovsdb_error();
+
 json = unbox_json(parse_json(ctx->argv[1]));
 check_ovsdb_error(ovsdb_type_from_json(, json));
 json_destroy(json);
@@ -562,6 +577,8 @@ do_parse_atomic_type(struct ovs_cmdl_context *ctx)
 enum ovsdb_atomic_type type;
 struct json *json;
 
+destroy_on_ovsdb_error();
+
 json = unbox_json(parse_json(ctx->argv[1]));
 check_ovsdb_error(ovsdb_atomic_type_from_json(, json));
 json_destroy(json);
@@ -574,6 +591,8 @@ do_parse_base_type(struct ovs_cmdl_context *ctx)
 struct ovsdb_base_type base;
 struct json *json;
 
+destroy_on_ovsdb_error();
+
 json = unbox_json(parse_json(ctx->argv[1]));
 check_ovsdb_error(ovsdb_base_type_from_json(, json));
 json_destroy(json);
@@ -587,6 +606,8 @@ do_parse_type(struct ovs_cmdl_context *ctx)
 struct ovsdb_type type;
 struct json *json;
 
+destroy_on_ovsdb_error();
+
 json = unbox_json(parse_json(ctx->argv[1]));
 check_ovsdb_error(ovsdb_type_from_json(, json));
 json_destroy(json);
@@ -601,6 +622,8 @@ do_parse_atoms(struct ovs_cmdl_context *ctx)
 struct json *json;
 int i;
 
+destroy_on_ovsdb_error();
+
 json = unbox_json(parse_json(ctx->argv[1]));
 check_ovsdb_error(ovsdb_base_type_from_json(, json));
 json_destroy(json);
@@ -630,6 +653,8 @@ do_parse_atom_strings(struct ovs_cmdl_context *ctx)
 struct json *json;
 int i;
 
+destroy_on_ovsdb_error();
+
 json = unbox_json(parse_json(ctx->argv[1]));
 check_ovsdb_error(ovsdb_base_type_from_json(, json));
 json_destroy(json);
@@ -675,6 +700,8 @@ do_parse_data__(int argc, char *argv[],
 struct json *json;
 int i;
 
+destroy_on_ovsdb_error();
+
 json = unbox_json(parse_json(argv[1]));
 check_ovsdb_error(ovsdb_type_from_json(, json));
 json_destroy(json);
@@ -706,6 +733,8 @@ do_parse_data_strings(struct ovs_cmdl_context *ctx)
 struct json *json;
 int i;
 
+destroy_on_ovsdb_error();
+
 json = unbox_json(parse_json(ctx->argv[1]));
 check_ovsdb_error(ovsdb_type_from_json(, json));
 json_destroy(json);
@@ -746,6 +775,8 @@ do_sort_atoms(struct ovs_cmdl_context *ctx)
 size_t n_atoms;
 int i;
 
+destroy_on_ovsdb_error();
+
 json = unbox_json(parse_json(ctx->argv[1]));