[PATCH BlueZ 0/2] test-vcp: Fix test failing in some environments

[PATCH BlueZ 0/2] test-vcp: Fix test failing in some environments

[Kirill Samburskiy]

This patch fixes test-vcp failing on some environments. It is done by
manually shutting down tester IO as a part of test teardown, as well as
freeing server-side bt_vcp instances.

---
After updating our bluez source code from v5.72 to v5.83 we noticed
that one of the tests (test-vcp) no longer passes on all environments
(specifically when building for x86_64 or arm64 architectures). This
patch resolves the problem, enabling all tests to pass. Here is a
short extract from test logs:

```
AICS/SR/CP/BV-01-C - init
AICS/SR/CP/BV-01-C - setup
AICS/SR/CP/BV-01-C - setup complete
AICS/SR/CP/BV-01-C - run
gatt_notify_cb: Failed to send notification
ERROR:src/shared/tester.c:981:test_io_recv: assertion failed (len == iov->iov_len): (5 == 6)
```

The reason this test was failing is incomplete test teardown.
Specifically, bt_vcp instances created by vcp_get_session function
are not freed and, more importantly, not removed from sessions queue
(both function and queue are found in shared/vcp.c file).

When a new test case is started, vcp_get_session function may be called
at some point. This function looks up session object using current
bt_att object as key. Each test case creates its own bt_att instance,
however in our case bt_att is always allocated at the same memory
address. This leads to vcp_get_session function looking up session
object belonging to the previous test case instead of creating a new
bt_vcp instance (since both current and previous test cases allocated
memory for bt_att object at the same address).

Getting the wrong session object leads to using wrong gatt_db, which
then uses the wrong user_data for CCC callbacks, ultimately leading
to gatt_notify_cb function from test-vcp.c getting incorrect test_data
pointer. Finally gatt_notify_cb attempts to send a notification using
an already freed bt_gatt_server instance, which unsurprisingly fails,
causing expected data to not be written into tester IO channel.

This patch fixes the issue by doing two things. First, it shuts down
tester IO as a part of test teardown, triggering disconnection
callbacks in bt_att object. One of these callbacks is registered in
vcp_get_session function, specifically vcp_disconnected function.
This function detaches bt_vcp instance (removes from sessions queue)
and triggers *_remote_client_detached callbacks. The second part of the
fix is registering vcp remote client callbacks using bt_vcp_register
function, with vcp_client_detached function responsible for unrefing
(and freeing) the detached bt_vcp instance. Since the instance is now
removed from sessions queue, vcp_get_session function can no longer
look up a wrong object during the test, allowing it to pass.

The test teardown is now split in two functions. The reason for that is
that IO disonnection callbacks are executed by main loop, thus after
io_shutdown functions were executed, we need to return to main loop
to let them execute before proceeding with teardown and freeing bt_att.
If bt_att is freed too early, its disconnection callbacks are not going
to be executed.

Kirill Samburskiy (2):
  shared/tester: add ability to shutdown tester IO
  test-vcp: free server-side bt_vcp on test teardown

 src/shared/tester.c |  6 ++++++
 src/shared/tester.h |  1 +
 unit/test-vcp.c     | 26 ++++++++++++++++++++++++--
 3 files changed, 31 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH BlueZ 1/2] shared/tester: add ability to shutdown tester IO

[Kirill Samburskiy]

Some tests may require the ability to shutdown IO for proper teardown.
Add function tester_shutdown_io to accomplish that.
---
 src/shared/tester.c | 6 ++++++
 src/shared/tester.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/src/shared/tester.c b/src/shared/tester.c
index 371ccaced..230e9ef75 100644
--- a/src/shared/tester.c
+++ b/src/shared/tester.c
@@ -1047,6 +1047,12 @@ struct io *tester_setup_io(const struct iovec *iov, int iovcnt)
 	return ios[0];
 }
 
+void tester_shutdown_io(void)
+{
+	io_shutdown(ios[0]);
+	io_shutdown(ios[1]);
+}
+
 void tester_io_send(void)
 {
 	struct test_case *test = tester_get_test();
diff --git a/src/shared/tester.h b/src/shared/tester.h
index 1f8138434..dfc1ca3a8 100644
--- a/src/shared/tester.h
+++ b/src/shared/tester.h
@@ -81,5 +81,6 @@ void tester_wait(unsigned int seconds, tester_wait_func_t func,
 							void *user_data);
 
 struct io *tester_setup_io(const struct iovec *iov, int iovcnt);
+void tester_shutdown_io(void);
 void tester_io_send(void);
 void tester_io_set_complete_func(tester_data_func_t func);
-- 
2.34.1

[PATCH BlueZ 2/2] test-vcp: free server-side bt_vcp on test teardown

[Kirill Samburskiy]

Shutdown tester IO before freeing bt_att instance to trigger its IO
disconnection callbacks. This detaches bt_vcp instance created by
vcp_get_session function, and executes detach callback registered
through bt_vcp_register that frees memory used by this bt_vcp instance.
This prevents the test from failing in some environments.
---
 unit/test-vcp.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/unit/test-vcp.c b/unit/test-vcp.c
index 6a61ea2c4..04254700b 100644
--- a/unit/test-vcp.c
+++ b/unit/test-vcp.c
@@ -38,6 +38,7 @@ struct test_data {
 	struct queue *ccc_states;
 	size_t iovcnt;
 	struct iovec *iov;
+	unsigned int vcp_id;
 };
 
 struct notify {
@@ -78,9 +79,9 @@ static void print_debug(const char *str, void *user_data)
 		tester_debug("%s%s", prefix, str);
 }
 
-static void test_teardown(const void *user_data)
+static gboolean test_teardown_finish(gpointer user_data)
 {
-	struct test_data *data = (void *)user_data;
+	struct test_data *data = user_data;
 
 	bt_vcp_unref(data->vcp);
 	bt_gatt_server_unref(data->server);
@@ -90,7 +91,16 @@ static void test_teardown(const void *user_data)
 
 	queue_destroy(data->ccc_states, free);
 
+	bt_vcp_unregister(data->vcp_id);
 	tester_teardown_complete();
+
+	return FALSE;
+}
+
+static void test_teardown(const void *user_data)
+{
+	tester_shutdown_io();
+	g_idle_add(test_teardown_finish, (void *) user_data);
 }
 
 static bool ccc_state_match(const void *a, const void *b)
@@ -163,6 +173,15 @@ done:
 							sizeof(value));
 }
 
+static void vcp_client_attached(struct bt_vcp *vcp, void *user_data)
+{
+}
+
+static void vcp_client_detached(struct bt_vcp *vcp, void *user_data)
+{
+	bt_vcp_unref(vcp);
+}
+
 static void test_server(const void *user_data)
 {
 	struct test_data *data = (void *)user_data;
@@ -188,6 +207,9 @@ static void test_server(const void *user_data)
 	data->vcp = bt_vcp_new(data->db, NULL);
 	g_assert(data->vcp);
 
+	data->vcp_id = bt_vcp_register(vcp_client_attached,
+						vcp_client_detached, NULL);
+
 	data->server = bt_gatt_server_new(data->db, att, 64, 0);
 	g_assert(data->server);
 
-- 
2.34.1

RE: test-vcp: Fix test failing in some environments

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=973896

---Test result---

Test Summary:
CheckPatch                    PENDING   0.21 seconds
GitLint                       PENDING   0.23 seconds
BuildEll                      PASS      19.84 seconds
BluezMake                     PASS      2607.48 seconds
MakeCheck                     PASS      20.36 seconds
MakeDistcheck                 PASS      195.75 seconds
CheckValgrind                 PASS      273.20 seconds
CheckSmatch                   PASS      300.41 seconds
bluezmakeextell               PASS      126.51 seconds
IncrementalBuild              PENDING   0.24 seconds
ScanBuild                     PASS      890.92 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH BlueZ 2/2] test-vcp: free server-side bt_vcp on test teardown

[Luiz Augusto von Dentz]

Hi Kirill,

On Thu, Jun 19, 2025 at 11:34 AM Kirill Samburskiy <k.samburskiy@omp.ru> wrote:
>
> Shutdown tester IO before freeing bt_att instance to trigger its IO
> disconnection callbacks. This detaches bt_vcp instance created by
> vcp_get_session function, and executes detach callback registered
> through bt_vcp_register that frees memory used by this bt_vcp instance.
> This prevents the test from failing in some environments.

I guess this should close the following issues:

https://github.com/bluez/bluez/issues/726
https://github.com/bluez/bluez/issues/683

> ---
>  unit/test-vcp.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/unit/test-vcp.c b/unit/test-vcp.c
> index 6a61ea2c4..04254700b 100644
> --- a/unit/test-vcp.c
> +++ b/unit/test-vcp.c
> @@ -38,6 +38,7 @@ struct test_data {
>         struct queue *ccc_states;
>         size_t iovcnt;
>         struct iovec *iov;
> +       unsigned int vcp_id;
>  };
>
>  struct notify {
> @@ -78,9 +79,9 @@ static void print_debug(const char *str, void *user_data)
>                 tester_debug("%s%s", prefix, str);
>  }
>
> -static void test_teardown(const void *user_data)
> +static gboolean test_teardown_finish(gpointer user_data)
>  {
> -       struct test_data *data = (void *)user_data;
> +       struct test_data *data = user_data;
>
>         bt_vcp_unref(data->vcp);
>         bt_gatt_server_unref(data->server);
> @@ -90,7 +91,16 @@ static void test_teardown(const void *user_data)
>
>         queue_destroy(data->ccc_states, free);
>
> +       bt_vcp_unregister(data->vcp_id);
>         tester_teardown_complete();
> +
> +       return FALSE;
> +}
> +
> +static void test_teardown(const void *user_data)
> +{
> +       tester_shutdown_io();
> +       g_idle_add(test_teardown_finish, (void *) user_data);
>  }
>
>  static bool ccc_state_match(const void *a, const void *b)
> @@ -163,6 +173,15 @@ done:
>                                                         sizeof(value));
>  }
>
> +static void vcp_client_attached(struct bt_vcp *vcp, void *user_data)
> +{
> +}
> +
> +static void vcp_client_detached(struct bt_vcp *vcp, void *user_data)
> +{
> +       bt_vcp_unref(vcp);

Hmm, are the multiple references to vcp that makes
test_teardown_finish not to free on bt_vcp_unref?

> +}
> +
>  static void test_server(const void *user_data)
>  {
>         struct test_data *data = (void *)user_data;
> @@ -188,6 +207,9 @@ static void test_server(const void *user_data)
>         data->vcp = bt_vcp_new(data->db, NULL);
>         g_assert(data->vcp);
>
> +       data->vcp_id = bt_vcp_register(vcp_client_attached,
> +                                               vcp_client_detached, NULL);
> +
>         data->server = bt_gatt_server_new(data->db, att, 64, 0);
>         g_assert(data->server);
>
> --
> 2.34.1
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 1/2] shared/tester: add ability to shutdown tester IO

[Luiz Augusto von Dentz]

Hi Kirill,

On Thu, Jun 19, 2025 at 11:34 AM Kirill Samburskiy <k.samburskiy@omp.ru> wrote:
>
> Some tests may require the ability to shutdown IO for proper teardown.
> Add function tester_shutdown_io to accomplish that.
> ---
>  src/shared/tester.c | 6 ++++++
>  src/shared/tester.h | 1 +
>  2 files changed, 7 insertions(+)
>
> diff --git a/src/shared/tester.c b/src/shared/tester.c
> index 371ccaced..230e9ef75 100644
> --- a/src/shared/tester.c
> +++ b/src/shared/tester.c
> @@ -1047,6 +1047,12 @@ struct io *tester_setup_io(const struct iovec *iov, int iovcnt)
>         return ios[0];
>  }
>
> +void tester_shutdown_io(void)
> +{
> +       io_shutdown(ios[0]);
> +       io_shutdown(ios[1]);
> +}

We might as well just done it in tester_result, something like this:

diff --git a/src/shared/tester.c b/src/shared/tester.c
index 230e9ef7579e..62a14d0732c8 100644
--- a/src/shared/tester.c
+++ b/src/shared/tester.c
@@ -654,6 +654,8 @@ static void test_result(enum test_result result)
                test->timeout_id = 0;
        }

+       tester_shutdown_io();
+
        if (test->result == TEST_RESULT_FAILED)
                result = TEST_RESULT_FAILED;

That said, it appears it uncovered some other issues after Ive done that.

>  void tester_io_send(void)
>  {
>         struct test_case *test = tester_get_test();
> diff --git a/src/shared/tester.h b/src/shared/tester.h
> index 1f8138434..dfc1ca3a8 100644
> --- a/src/shared/tester.h
> +++ b/src/shared/tester.h
> @@ -81,5 +81,6 @@ void tester_wait(unsigned int seconds, tester_wait_func_t func,
>                                                         void *user_data);
>
>  struct io *tester_setup_io(const struct iovec *iov, int iovcnt);
> +void tester_shutdown_io(void);
>  void tester_io_send(void);
>  void tester_io_set_complete_func(tester_data_func_t func);
> --
> 2.34.1
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 2/2] test-vcp: free server-side bt_vcp on test teardown

[Luiz Augusto von Dentz]

Hi Kirill,

On Thu, Jun 19, 2025 at 1:16 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Kirill,
>
> On Thu, Jun 19, 2025 at 11:34 AM Kirill Samburskiy <k.samburskiy@omp.ru> wrote:
> >
> > Shutdown tester IO before freeing bt_att instance to trigger its IO
> > disconnection callbacks. This detaches bt_vcp instance created by
> > vcp_get_session function, and executes detach callback registered
> > through bt_vcp_register that frees memory used by this bt_vcp instance.
> > This prevents the test from failing in some environments.
>
> I guess this should close the following issues:
>
> https://github.com/bluez/bluez/issues/726
> https://github.com/bluez/bluez/issues/683
>
> > ---
> >  unit/test-vcp.c | 26 ++++++++++++++++++++++++--
> >  1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/unit/test-vcp.c b/unit/test-vcp.c
> > index 6a61ea2c4..04254700b 100644
> > --- a/unit/test-vcp.c
> > +++ b/unit/test-vcp.c
> > @@ -38,6 +38,7 @@ struct test_data {
> >         struct queue *ccc_states;
> >         size_t iovcnt;
> >         struct iovec *iov;
> > +       unsigned int vcp_id;
> >  };
> >
> >  struct notify {
> > @@ -78,9 +79,9 @@ static void print_debug(const char *str, void *user_data)
> >                 tester_debug("%s%s", prefix, str);
> >  }
> >
> > -static void test_teardown(const void *user_data)
> > +static gboolean test_teardown_finish(gpointer user_data)
> >  {
> > -       struct test_data *data = (void *)user_data;
> > +       struct test_data *data = user_data;
> >
> >         bt_vcp_unref(data->vcp);
> >         bt_gatt_server_unref(data->server);
> > @@ -90,7 +91,16 @@ static void test_teardown(const void *user_data)
> >
> >         queue_destroy(data->ccc_states, free);
> >
> > +       bt_vcp_unregister(data->vcp_id);
> >         tester_teardown_complete();
> > +
> > +       return FALSE;
> > +}
> > +
> > +static void test_teardown(const void *user_data)
> > +{
> > +       tester_shutdown_io();
> > +       g_idle_add(test_teardown_finish, (void *) user_data);
> >  }
> >
> >  static bool ccc_state_match(const void *a, const void *b)
> > @@ -163,6 +173,15 @@ done:
> >                                                         sizeof(value));
> >  }
> >
> > +static void vcp_client_attached(struct bt_vcp *vcp, void *user_data)
> > +{
> > +}
> > +
> > +static void vcp_client_detached(struct bt_vcp *vcp, void *user_data)
> > +{
> > +       bt_vcp_unref(vcp);
>
> Hmm, are the multiple references to vcp that makes
> test_teardown_finish not to free on bt_vcp_unref?
>
> > +}
> > +
> >  static void test_server(const void *user_data)
> >  {
> >         struct test_data *data = (void *)user_data;
> > @@ -188,6 +207,9 @@ static void test_server(const void *user_data)
> >         data->vcp = bt_vcp_new(data->db, NULL);
> >         g_assert(data->vcp);

Looks like the above session is actually not used for anything, upon
connecting a new session will be created with corresponding bt_att and
that is why the attached callback is required, so the data->vcp
created here is useless and shall be removed, that said it does seem
like we have a similar problem in test-micp.c as well, also if we do
incorporate the tester_shutdown_io into test_result there is also a
crash on test-bap.c due to it not registering a detach callback, which
we should either ignore if that is registered as NULL or fail to
register. Anyway let me know if you need some help getting these
changes worked out, I can probably do it myself if you don't have time
to incorporate them.

> > +       data->vcp_id = bt_vcp_register(vcp_client_attached,
> > +                                               vcp_client_detached, NULL);
> > +
> >         data->server = bt_gatt_server_new(data->db, att, 64, 0);
> >         g_assert(data->server);
> >
> > --
> > 2.34.1
> >
> >
>
>
> --
> Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

RE: test-vcp: Fix test failing in some environments

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1864 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=974291

---Test result---

Test Summary:
CheckPatch                    PENDING   0.44 seconds
GitLint                       PENDING   0.40 seconds
BuildEll                      PASS      20.10 seconds
BluezMake                     PASS      2643.80 seconds
MakeCheck                     PASS      20.88 seconds
MakeDistcheck                 PASS      196.59 seconds
CheckValgrind                 PASS      274.06 seconds
CheckSmatch                   WARNING   306.77 seconds
bluezmakeextell               PASS      127.66 seconds
IncrementalBuild              PENDING   0.39 seconds
ScanBuild                     PASS      918.78 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/bap.c:317:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:317:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structuressrc/shared/bap.c:317:25: warning: array of flexible structuressrc/shared/bap.c: note: in included file:./src/shared/ascs.h:88:25: warning: array of flexible structures
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth