[BlueZ v3 0/7] Fix bluetoothctl --help hanging if daemon isn't running

[BlueZ v3 0/7] Fix bluetoothctl --help hanging if daemon isn't running

[Bastien Nocera]

Changes from v2:
- rebased against latest upstream

Bastien Nocera (7):
  configure.ac: Update requirement to glib 2.34
  client: Use g_clear_pointer() to clean up menus
  client: Split installing submenu and doing I/O
  client: Install submenus before contacting bluez daemon
  shell: Document "mode" variable
  shared/shell: Add function to handle early help calls
  client: Fix --help hanging if bluetoothd is not running

 acinclude.m4       |  4 ++--
 client/admin.c     |  8 +++++---
 client/admin.h     |  1 +
 client/assistant.c |  8 +++++---
 client/assistant.h |  1 +
 client/main.c      | 14 ++++++++++----
 client/mgmt.c      | 11 +++++++----
 client/mgmt.h      |  3 ++-
 client/player.c    |  7 +++++--
 client/player.h    |  1 +
 configure.ac       |  2 +-
 src/shared/shell.c | 12 +++++++++++-
 src/shared/shell.h |  2 ++
 tools/btmgmt.c     |  3 ++-
 14 files changed, 55 insertions(+), 22 deletions(-)

-- 
2.47.0

[BlueZ v3 1/7] configure.ac: Update requirement to glib 2.34

[Bastien Nocera]

Update build requirements from 2011's glib 2.28 all the way up to 2012's
glib 2.34. This will allow us to use g_clear_pointer().
---
 acinclude.m4 | 4 ++--
 configure.ac | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 4b73a5bfc38f..d0bfe6ccb2fa 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -62,8 +62,8 @@ AC_DEFUN([COMPILER_FLAGS], [
 		with_cflags="$with_cflags -Wswitch-enum"
 		with_cflags="$with_cflags -Wformat -Wformat-security"
 		with_cflags="$with_cflags -DG_DISABLE_DEPRECATED"
-		with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
-		with_cflags="$with_cflags -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
+		with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_34"
+		with_cflags="$with_cflags -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_34"
 	fi
 	AC_SUBST([WARNING_CFLAGS], $with_cflags)
 ])
diff --git a/configure.ac b/configure.ac
index 7093e41ff714..14bd15293918 100644
--- a/configure.ac
+++ b/configure.ac
@@ -81,7 +81,7 @@ AC_CHECK_DECLS([basename], [],
 				 ])
 
 
-PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.28)
+PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.34)
 
 if (test "${enable_threads}" = "yes"); then
 	AC_DEFINE(NEED_THREADS, 1, [Define if threading support is required])
-- 
2.47.0

RE: Fix bluetoothctl --help hanging if daemon isn't running

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 3008 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=901846

---Test result---

Test Summary:
CheckPatch                    FAIL      3.54 seconds
GitLint                       PASS      2.41 seconds
BuildEll                      PASS      24.73 seconds
BluezMake                     PASS      1737.88 seconds
MakeCheck                     PASS      13.46 seconds
MakeDistcheck                 PASS      180.52 seconds
CheckValgrind                 PASS      254.65 seconds
CheckSmatch                   WARNING   358.75 seconds
bluezmakeextell               PASS      121.22 seconds
IncrementalBuild              PASS      11429.59 seconds
ScanBuild                     PASS      1028.62 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ,v3,2/7] client: Use g_clear_pointer() to clean up menus
ERROR:INITIALISED_STATIC: do not initialise statics to NULL
#66: FILE: client/admin.c:194:
+static GDBusClient *client = NULL;

ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)
#66: FILE: client/admin.c:194:
+static GDBusClient *client = NULL;
                    ^

ERROR:INITIALISED_STATIC: do not initialise statics to NULL
#87: FILE: client/assistant.c:393:
+static GDBusClient * client = NULL;

ERROR:POINTER_LOCATION: "foo * bar" should be "foo *bar"
#87: FILE: client/assistant.c:393:
+static GDBusClient * client = NULL;

ERROR:INITIALISED_STATIC: do not initialise statics to NULL
#109: FILE: client/player.c:5697:
+static GDBusClient *client = NULL;

/github/workspace/src/src/13845731.patch total: 5 errors, 0 warnings, 46 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13845731.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):


---
Regards,
Linux Bluetooth

[BlueZ v3 2/7] client: Use g_clear_pointer() to clean up menus

[Bastien Nocera]

This would avoid warnings should the client be NULL.
---
 client/admin.c     | 5 ++---
 client/assistant.c | 5 ++---
 client/player.c    | 4 ++--
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/client/admin.c b/client/admin.c
index cd9af6f955da..9d48867bc1d7 100644
--- a/client/admin.c
+++ b/client/admin.c
@@ -191,7 +191,7 @@ static void proxy_removed(GDBusProxy *proxy, void *user_data)
 		admin_policy_status_removed(proxy);
 }
 
-static GDBusClient *client;
+static GDBusClient *client = NULL;
 
 static void disconnect_handler(DBusConnection *connection, void *user_data)
 {
@@ -215,6 +215,5 @@ void admin_add_submenu(void)
 
 void admin_remove_submenu(void)
 {
-	g_dbus_client_unref(client);
-	client = NULL;
+	g_clear_pointer(&client, g_dbus_client_unref);
 }
diff --git a/client/assistant.c b/client/assistant.c
index 16e94664a5c3..94052e26fd59 100644
--- a/client/assistant.c
+++ b/client/assistant.c
@@ -390,7 +390,7 @@ static const struct bt_shell_menu assistant_menu = {
 	{} },
 };
 
-static GDBusClient * client;
+static GDBusClient * client = NULL;
 
 void assistant_add_submenu(void)
 {
@@ -409,7 +409,6 @@ void assistant_add_submenu(void)
 
 void assistant_remove_submenu(void)
 {
-	g_dbus_client_unref(client);
-	client = NULL;
+	g_clear_pointer(&client, g_dbus_client_unref);
 }
 
diff --git a/client/player.c b/client/player.c
index 188378175486..dea5922d56db 100644
--- a/client/player.c
+++ b/client/player.c
@@ -5694,7 +5694,7 @@ static const struct bt_shell_menu transport_menu = {
 	{} },
 };
 
-static GDBusClient *client;
+static GDBusClient *client = NULL;
 
 void player_add_submenu(void)
 {
@@ -5715,6 +5715,6 @@ void player_add_submenu(void)
 
 void player_remove_submenu(void)
 {
-	g_dbus_client_unref(client);
+	g_clear_pointer(&client, g_dbus_client_unref);
 	queue_destroy(ios, transport_free);
 }
-- 
2.47.0

[BlueZ v3 3/7] client: Split installing submenu and doing I/O

[Bastien Nocera]

Split off installing the command's submenu and contacting the management
socket or the bluez daemon.
---
 client/admin.c     |  3 +++
 client/admin.h     |  1 +
 client/assistant.c |  3 +++
 client/assistant.h |  1 +
 client/main.c      |  4 ++++
 client/mgmt.c      | 11 +++++++----
 client/mgmt.h      |  3 ++-
 client/player.c    |  3 +++
 client/player.h    |  1 +
 tools/btmgmt.c     |  3 ++-
 10 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/client/admin.c b/client/admin.c
index 9d48867bc1d7..15759ba0291a 100644
--- a/client/admin.c
+++ b/client/admin.c
@@ -202,7 +202,10 @@ static void disconnect_handler(DBusConnection *connection, void *user_data)
 void admin_add_submenu(void)
 {
 	bt_shell_add_submenu(&admin_menu);
+}
 
+void admin_enable_submenu(void)
+{
 	dbus_conn = bt_shell_get_env("DBUS_CONNECTION");
 	if (!dbus_conn || client)
 		return;
diff --git a/client/admin.h b/client/admin.h
index 4da83e4e36ba..68a497971f66 100644
--- a/client/admin.h
+++ b/client/admin.h
@@ -18,4 +18,5 @@
  */
 
 void admin_add_submenu(void);
+void admin_enable_submenu(void);
 void admin_remove_submenu(void);
diff --git a/client/assistant.c b/client/assistant.c
index 94052e26fd59..ceade046be68 100644
--- a/client/assistant.c
+++ b/client/assistant.c
@@ -395,7 +395,10 @@ static GDBusClient * client = NULL;
 void assistant_add_submenu(void)
 {
 	bt_shell_add_submenu(&assistant_menu);
+}
 
+void assistant_enable_submenu(void)
+{
 	dbus_conn = bt_shell_get_env("DBUS_CONNECTION");
 	if (!dbus_conn || client)
 		return;
diff --git a/client/assistant.h b/client/assistant.h
index 418b0b84031f..c304abea0515 100644
--- a/client/assistant.h
+++ b/client/assistant.h
@@ -9,5 +9,6 @@
  */
 
 void assistant_add_submenu(void);
+void assistant_enable_submenu(void);
 void assistant_remove_submenu(void);
 
diff --git a/client/main.c b/client/main.c
index 3f8143dde4b8..17af62000f6d 100644
--- a/client/main.c
+++ b/client/main.c
@@ -3204,9 +3204,13 @@ int main(int argc, char *argv[])
 					(void *)endpoint_option);
 
 	admin_add_submenu();
+	admin_enable_submenu();
 	player_add_submenu();
+	player_enable_submenu();
 	mgmt_add_submenu();
+	mgmt_enable_submenu();
 	assistant_add_submenu();
+	assistant_enable_submenu();
 
 	client = g_dbus_client_new(dbus_conn, "org.bluez", "/org/bluez");
 
diff --git a/client/mgmt.c b/client/mgmt.c
index 602b92228ab8..aea3d95597c9 100644
--- a/client/mgmt.c
+++ b/client/mgmt.c
@@ -6026,7 +6026,13 @@ static void mgmt_debug(const char *str, void *user_data)
 	print("%s%s", prefix, str);
 }
 
-bool mgmt_add_submenu(void)
+void mgmt_add_submenu(void)
+{
+	bt_shell_add_submenu(&mgmt_menu);
+	bt_shell_add_submenu(&monitor_menu);
+}
+
+bool mgmt_enable_submenu(void)
 {
 	mgmt = mgmt_new_default();
 	if (!mgmt) {
@@ -6034,9 +6040,6 @@ bool mgmt_add_submenu(void)
 		return false;
 	}
 
-	bt_shell_add_submenu(&mgmt_menu);
-	bt_shell_add_submenu(&monitor_menu);
-
 	if (getenv("MGMT_DEBUG"))
 		mgmt_set_debug(mgmt, mgmt_debug, "mgmt: ", NULL);
 
diff --git a/client/mgmt.h b/client/mgmt.h
index 5a2026eab6a2..b0f3cafd0777 100644
--- a/client/mgmt.h
+++ b/client/mgmt.h
@@ -8,6 +8,7 @@
  *
  */
 
-bool mgmt_add_submenu(void);
+void mgmt_add_submenu(void);
+bool mgmt_enable_submenu(void);
 void mgmt_remove_submenu(void);
 void mgmt_set_index(const char *arg);
diff --git a/client/player.c b/client/player.c
index dea5922d56db..3189e0575798 100644
--- a/client/player.c
+++ b/client/player.c
@@ -5701,7 +5701,10 @@ void player_add_submenu(void)
 	bt_shell_add_submenu(&player_menu);
 	bt_shell_add_submenu(&endpoint_menu);
 	bt_shell_add_submenu(&transport_menu);
+}
 
+void player_enable_submenu(void)
+{
 	dbus_conn = bt_shell_get_env("DBUS_CONNECTION");
 	if (!dbus_conn || client)
 		return;
diff --git a/client/player.h b/client/player.h
index e7778cb1efd9..c09ffa94cbff 100644
--- a/client/player.h
+++ b/client/player.h
@@ -9,4 +9,5 @@
  */
 
 void player_add_submenu(void);
+void player_enable_submenu(void);
 void player_remove_submenu(void);
diff --git a/tools/btmgmt.c b/tools/btmgmt.c
index 436c2bb21f10..0f6051d5befe 100644
--- a/tools/btmgmt.c
+++ b/tools/btmgmt.c
@@ -46,7 +46,8 @@ int main(int argc, char *argv[])
 
 	bt_shell_init(argc, argv, &opt);
 
-	if (!mgmt_add_submenu()) {
+	mgmt_add_submenu();
+	if (!mgmt_enable_submenu()) {
 		fprintf(stderr, "Unable to open mgmt_socket\n");
 		return EXIT_FAILURE;
 	}
-- 
2.47.0

[BlueZ v3 4/7] client: Install submenus before contacting bluez daemon

[Bastien Nocera]

So that the submenus are installed even if bluez isn't available.
---
 client/main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/client/main.c b/client/main.c
index 17af62000f6d..f60bef1a6d3a 100644
--- a/client/main.c
+++ b/client/main.c
@@ -3187,6 +3187,10 @@ int main(int argc, char *argv[])
 	bt_shell_add_submenu(&advertise_monitor_menu);
 	bt_shell_add_submenu(&scan_menu);
 	bt_shell_add_submenu(&gatt_menu);
+	admin_add_submenu();
+	player_add_submenu();
+	mgmt_add_submenu();
+	assistant_add_submenu();
 	bt_shell_set_prompt(PROMPT_OFF, NULL);
 
 	if (agent_option)
@@ -3203,13 +3207,9 @@ int main(int argc, char *argv[])
 		bt_shell_set_env("AUTO_REGISTER_ENDPOINT",
 					(void *)endpoint_option);
 
-	admin_add_submenu();
 	admin_enable_submenu();
-	player_add_submenu();
 	player_enable_submenu();
-	mgmt_add_submenu();
 	mgmt_enable_submenu();
-	assistant_add_submenu();
 	assistant_enable_submenu();
 
 	client = g_dbus_client_new(dbus_conn, "org.bluez", "/org/bluez");
-- 
2.47.0

[BlueZ v3 5/7] shell: Document "mode" variable

[Bastien Nocera]

It's unclear what it does, could probably do with being replaced with an
enum to make its possible values and meaning clearer.
---
 src/shared/shell.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/shared/shell.c b/src/shared/shell.c
index a8ad956c7948..2f83717b2d39 100644
--- a/src/shared/shell.c
+++ b/src/shared/shell.c
@@ -71,7 +71,7 @@ static struct {
 	char history[256];
 	int argc;
 	char **argv;
-	bool mode;
+	bool mode; /* whether non-interactive */
 	bool zsh;
 	bool monitor;
 	int timeout;
-- 
2.47.0

[BlueZ v3 6/7] shared/shell: Add function to handle early help calls

[Bastien Nocera]

Add a function that would allow tools to exit after handling --help, so
as to avoid the daemon waiting to communicate with a D-Bus service that
might not be running.
---
 src/shared/shell.c | 10 ++++++++++
 src/shared/shell.h |  2 ++
 2 files changed, 12 insertions(+)

diff --git a/src/shared/shell.c b/src/shared/shell.c
index 2f83717b2d39..88cc7df2d565 100644
--- a/src/shared/shell.c
+++ b/src/shared/shell.c
@@ -1697,3 +1697,13 @@ int bt_shell_get_timeout(void)
 {
 	return data.timeout;
 }
+
+void bt_shell_handle_non_interactive_help(void)
+{
+	if (!data.mode)
+		return;
+	if (data.argv[0] != cmplt)
+		return;
+	print_cmds();
+	exit(EXIT_SUCCESS);
+}
diff --git a/src/shared/shell.h b/src/shared/shell.h
index e431db9f5821..eebbc71faffb 100644
--- a/src/shared/shell.h
+++ b/src/shared/shell.h
@@ -60,6 +60,8 @@ int bt_shell_exec(const char *input);
 void bt_shell_quit(int status);
 void bt_shell_noninteractive_quit(int status);
 
+void bt_shell_handle_non_interactive_help(void);
+
 bool bt_shell_set_menu(const struct bt_shell_menu *menu);
 
 bool bt_shell_add_submenu(const struct bt_shell_menu *menu);
-- 
2.47.0

[BlueZ v3 7/7] client: Fix --help hanging if bluetoothd is not running

[Bastien Nocera]

Exit after printing all the main and submenu commands.
---
 client/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/client/main.c b/client/main.c
index f60bef1a6d3a..f5ed9f9f5297 100644
--- a/client/main.c
+++ b/client/main.c
@@ -3193,6 +3193,8 @@ int main(int argc, char *argv[])
 	assistant_add_submenu();
 	bt_shell_set_prompt(PROMPT_OFF, NULL);
 
+	bt_shell_handle_non_interactive_help();
+
 	if (agent_option)
 		auto_register_agent = g_strdup(agent_option);
 	else
-- 
2.47.0

Re: [BlueZ v3 7/7] client: Fix --help hanging if bluetoothd is not running

[Luiz Augusto von Dentz]

Hi Bastien,

On Tue, Oct 22, 2024 at 10:11 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> Exit after printing all the main and submenu commands.
> ---
>  client/main.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/client/main.c b/client/main.c
> index f60bef1a6d3a..f5ed9f9f5297 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -3193,6 +3193,8 @@ int main(int argc, char *argv[])
>         assistant_add_submenu();
>         bt_shell_set_prompt(PROMPT_OFF, NULL);
>
> +       bt_shell_handle_non_interactive_help();
> +
>         if (agent_option)
>                 auto_register_agent = g_strdup(agent_option);
>         else
> --
> 2.47.0
>

Having some thoughts about how to do this is more clean way, perhaps
we should do this as part of bt_shell_run and then introduce .run
callback to bt_shell_menu so it is called as part of bt_shell_run,
under the .run callback the menu can place e.g. DBUS connection setup,
etc, but before it reaches that we can check if it just a help
pending.

-- 
Luiz Augusto von Dentz

Re: [BlueZ v3 7/7] client: Fix --help hanging if bluetoothd is not running

[Bastien Nocera]

On Thu, 2024-10-24 at 12:08 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Tue, Oct 22, 2024 at 10:11 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > Exit after printing all the main and submenu commands.
> > ---
> >  client/main.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/client/main.c b/client/main.c
> > index f60bef1a6d3a..f5ed9f9f5297 100644
> > --- a/client/main.c
> > +++ b/client/main.c
> > @@ -3193,6 +3193,8 @@ int main(int argc, char *argv[])
> >         assistant_add_submenu();
> >         bt_shell_set_prompt(PROMPT_OFF, NULL);
> > 
> > +       bt_shell_handle_non_interactive_help();
> > +
> >         if (agent_option)
> >                 auto_register_agent = g_strdup(agent_option);
> >         else
> > --
> > 2.47.0
> > 
> 
> Having some thoughts about how to do this is more clean way, perhaps
> we should do this as part of bt_shell_run and then introduce .run
> callback to bt_shell_menu so it is called as part of bt_shell_run,
> under the .run callback the menu can place e.g. DBUS connection
> setup,
> etc, but before it reaches that we can check if it just a help
> pending.

We need to:
1) set the top menu
2) populate the submenus without any D-Bus IO
3) turn off the prompt
4) print the help and exit if that's what was requested
5) setup IO for submenus
6) run the mainloop

I guess I could do 2) with a callback from bt_shell_set_menu()
and then do 4) and 5) from a bt_shell_run() callback.

Is that what you expected?
Do you have preferred names for the callback functions?

I think that we can still use the function split from 3/7, did you have
a better name for the functions?

Cheers

Re: [BlueZ v3 7/7] client: Fix --help hanging if bluetoothd is not running

[Luiz Augusto von Dentz]

Hi Bastien,

On Mon, Oct 28, 2024 at 11:35 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Thu, 2024-10-24 at 12:08 -0400, Luiz Augusto von Dentz wrote:
> > Hi Bastien,
> >
> > On Tue, Oct 22, 2024 at 10:11 AM Bastien Nocera <hadess@hadess.net>
> > wrote:
> > >
> > > Exit after printing all the main and submenu commands.
> > > ---
> > >  client/main.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/client/main.c b/client/main.c
> > > index f60bef1a6d3a..f5ed9f9f5297 100644
> > > --- a/client/main.c
> > > +++ b/client/main.c
> > > @@ -3193,6 +3193,8 @@ int main(int argc, char *argv[])
> > >         assistant_add_submenu();
> > >         bt_shell_set_prompt(PROMPT_OFF, NULL);
> > >
> > > +       bt_shell_handle_non_interactive_help();
> > > +
> > >         if (agent_option)
> > >                 auto_register_agent = g_strdup(agent_option);
> > >         else
> > > --
> > > 2.47.0
> > >
> >
> > Having some thoughts about how to do this is more clean way, perhaps
> > we should do this as part of bt_shell_run and then introduce .run
> > callback to bt_shell_menu so it is called as part of bt_shell_run,
> > under the .run callback the menu can place e.g. DBUS connection
> > setup,
> > etc, but before it reaches that we can check if it just a help
> > pending.
>
> We need to:
> 1) set the top menu
> 2) populate the submenus without any D-Bus IO
> 3) turn off the prompt
> 4) print the help and exit if that's what was requested
> 5) setup IO for submenus
> 6) run the mainloop
>
> I guess I could do 2) with a callback from bt_shell_set_menu()
> and then do 4) and 5) from a bt_shell_run() callback.

Yep.

> Is that what you expected?
> Do you have preferred names for the callback functions?

Id call it .run since it should be the result of bt_shell_run

> I think that we can still use the function split from 3/7, did you have
> a better name for the functions?

Not sure I follow, there will be a split between adding submenus and
.run to achieve the 2 stages, or are you talking about some other
split?

> Cheers
>


-- 
Luiz Augusto von Dentz

Re: [BlueZ v3 7/7] client: Fix --help hanging if bluetoothd is not running

[Bastien Nocera]

On Mon, 2024-10-28 at 11:54 -0400, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Mon, Oct 28, 2024 at 11:35 AM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Thu, 2024-10-24 at 12:08 -0400, Luiz Augusto von Dentz wrote:
> > > Hi Bastien,
> > > 
> > > On Tue, Oct 22, 2024 at 10:11 AM Bastien Nocera
> > > <hadess@hadess.net>
> > > wrote:
> > > > 
> > > > Exit after printing all the main and submenu commands.
> > > > ---
> > > >  client/main.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/client/main.c b/client/main.c
> > > > index f60bef1a6d3a..f5ed9f9f5297 100644
> > > > --- a/client/main.c
> > > > +++ b/client/main.c
> > > > @@ -3193,6 +3193,8 @@ int main(int argc, char *argv[])
> > > >         assistant_add_submenu();
> > > >         bt_shell_set_prompt(PROMPT_OFF, NULL);
> > > > 
> > > > +       bt_shell_handle_non_interactive_help();
> > > > +
> > > >         if (agent_option)
> > > >                 auto_register_agent = g_strdup(agent_option);
> > > >         else
> > > > --
> > > > 2.47.0
> > > > 
> > > 
> > > Having some thoughts about how to do this is more clean way,
> > > perhaps
> > > we should do this as part of bt_shell_run and then introduce .run
> > > callback to bt_shell_menu so it is called as part of
> > > bt_shell_run,
> > > under the .run callback the menu can place e.g. DBUS connection
> > > setup,
> > > etc, but before it reaches that we can check if it just a help
> > > pending.
> > 
> > We need to:
> > 1) set the top menu
> > 2) populate the submenus without any D-Bus IO
> > 3) turn off the prompt
> > 4) print the help and exit if that's what was requested
> > 5) setup IO for submenus
> > 6) run the mainloop
> > 
> > I guess I could do 2) with a callback from bt_shell_set_menu()
> > and then do 4) and 5) from a bt_shell_run() callback.
> 
> Yep.
> 
> > Is that what you expected?
> > Do you have preferred names for the callback functions?
> 
> Id call it .run since it should be the result of bt_shell_run
> 
> > I think that we can still use the function split from 3/7, did you
> > have
> > a better name for the functions?
> 
> Not sure I follow, there will be a split between adding submenus and
> .run to achieve the 2 stages, or are you talking about some other
> split?

We still need to split the functions called in .run between the bits
that populate the submenu, and the bits that will make D-Bus I/O, so
patch number 3 still needs to be exist.

Is assistant_add_submenu() and assistant_enable_submenu() OK, or did
you want a different name?

[BlueZ v4 1/9] client: Split installing submenu and doing I/O

[Bastien Nocera]

Split off installing the command's submenu and contacting the management
socket or the bluez daemon.
---
 client/admin.c     |  3 +++
 client/admin.h     |  1 +
 client/assistant.c |  3 +++
 client/assistant.h |  1 +
 client/main.c      |  4 ++++
 client/mgmt.c      | 11 +++++++----
 client/mgmt.h      |  3 ++-
 client/player.c    |  3 +++
 client/player.h    |  1 +
 tools/btmgmt.c     |  3 ++-
 10 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/client/admin.c b/client/admin.c
index 4d645d9469bb..614a492f1f5f 100644
--- a/client/admin.c
+++ b/client/admin.c
@@ -193,7 +193,10 @@ static void disconnect_handler(DBusConnection *connection, void *user_data)
 void admin_add_submenu(void)
 {
 	bt_shell_add_submenu(&admin_menu);
+}
 
+void admin_enable_submenu(void)
+{
 	dbus_conn = bt_shell_get_env("DBUS_CONNECTION");
 	if (!dbus_conn || client)
 		return;
diff --git a/client/admin.h b/client/admin.h
index 0047770dc737..08cca585d466 100644
--- a/client/admin.h
+++ b/client/admin.h
@@ -9,4 +9,5 @@
  */
 
 void admin_add_submenu(void);
+void admin_enable_submenu(void);
 void admin_remove_submenu(void);
diff --git a/client/assistant.c b/client/assistant.c
index 16e94664a5c3..555ac6feb048 100644
--- a/client/assistant.c
+++ b/client/assistant.c
@@ -395,7 +395,10 @@ static GDBusClient * client;
 void assistant_add_submenu(void)
 {
 	bt_shell_add_submenu(&assistant_menu);
+}
 
+void assistant_enable_submenu(void)
+{
 	dbus_conn = bt_shell_get_env("DBUS_CONNECTION");
 	if (!dbus_conn || client)
 		return;
diff --git a/client/assistant.h b/client/assistant.h
index 418b0b84031f..c304abea0515 100644
--- a/client/assistant.h
+++ b/client/assistant.h
@@ -9,5 +9,6 @@
  */
 
 void assistant_add_submenu(void);
+void assistant_enable_submenu(void);
 void assistant_remove_submenu(void);
 
diff --git a/client/main.c b/client/main.c
index a2d9d88bff43..e35e89dd2362 100644
--- a/client/main.c
+++ b/client/main.c
@@ -3420,9 +3420,13 @@ int main(int argc, char *argv[])
 					(void *)endpoint_option);
 
 	admin_add_submenu();
+	admin_enable_submenu();
 	player_add_submenu();
+	player_enable_submenu();
 	mgmt_add_submenu();
+	mgmt_enable_submenu();
 	assistant_add_submenu();
+	assistant_enable_submenu();
 	hci_add_submenu();
 
 	client = g_dbus_client_new(dbus_conn, "org.bluez", "/org/bluez");
diff --git a/client/mgmt.c b/client/mgmt.c
index faa97a159e3c..6c6d62f4bba1 100644
--- a/client/mgmt.c
+++ b/client/mgmt.c
@@ -6169,7 +6169,13 @@ static void mgmt_debug(const char *str, void *user_data)
 	print("%s%s", prefix, str);
 }
 
-bool mgmt_add_submenu(void)
+void mgmt_add_submenu(void)
+{
+	bt_shell_add_submenu(&mgmt_menu);
+	bt_shell_add_submenu(&monitor_menu);
+}
+
+bool mgmt_enable_submenu(void)
 {
 	mgmt = mgmt_new_default();
 	if (!mgmt) {
@@ -6177,9 +6183,6 @@ bool mgmt_add_submenu(void)
 		return false;
 	}
 
-	bt_shell_add_submenu(&mgmt_menu);
-	bt_shell_add_submenu(&monitor_menu);
-
 	if (getenv("MGMT_DEBUG"))
 		mgmt_set_debug(mgmt, mgmt_debug, "mgmt: ", NULL);
 
diff --git a/client/mgmt.h b/client/mgmt.h
index 5a2026eab6a2..b0f3cafd0777 100644
--- a/client/mgmt.h
+++ b/client/mgmt.h
@@ -8,6 +8,7 @@
  *
  */
 
-bool mgmt_add_submenu(void);
+void mgmt_add_submenu(void);
+bool mgmt_enable_submenu(void);
 void mgmt_remove_submenu(void);
 void mgmt_set_index(const char *arg);
diff --git a/client/player.c b/client/player.c
index 647e9bed70e6..7aab4af6d861 100644
--- a/client/player.c
+++ b/client/player.c
@@ -5876,7 +5876,10 @@ void player_add_submenu(void)
 	bt_shell_add_submenu(&player_menu);
 	bt_shell_add_submenu(&endpoint_menu);
 	bt_shell_add_submenu(&transport_menu);
+}
 
+void player_enable_submenu(void)
+{
 	dbus_conn = bt_shell_get_env("DBUS_CONNECTION");
 	if (!dbus_conn || client)
 		return;
diff --git a/client/player.h b/client/player.h
index e7778cb1efd9..c09ffa94cbff 100644
--- a/client/player.h
+++ b/client/player.h
@@ -9,4 +9,5 @@
  */
 
 void player_add_submenu(void);
+void player_enable_submenu(void);
 void player_remove_submenu(void);
diff --git a/tools/btmgmt.c b/tools/btmgmt.c
index 436c2bb21f10..0f6051d5befe 100644
--- a/tools/btmgmt.c
+++ b/tools/btmgmt.c
@@ -46,7 +46,8 @@ int main(int argc, char *argv[])
 
 	bt_shell_init(argc, argv, &opt);
 
-	if (!mgmt_add_submenu()) {
+	mgmt_add_submenu();
+	if (!mgmt_enable_submenu()) {
 		fprintf(stderr, "Unable to open mgmt_socket\n");
 		return EXIT_FAILURE;
 	}
-- 
2.49.0

RE: Fix bluetoothctl --help hanging if daemon isn't running

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1879 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=962345

---Test result---

Test Summary:
CheckPatch                    PENDING   0.29 seconds
GitLint                       PENDING   0.30 seconds
BuildEll                      PASS      20.52 seconds
BluezMake                     PASS      2734.32 seconds
MakeCheck                     PASS      19.90 seconds
MakeDistcheck                 PASS      199.61 seconds
CheckValgrind                 PASS      277.78 seconds
CheckSmatch                   WARNING   305.92 seconds
bluezmakeextell               PASS      129.21 seconds
IncrementalBuild              PENDING   0.33 seconds
ScanBuild                     PASS      913.02 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/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):src/shared/shell.c: note: in included file (through /usr/include/readline/readline.h):
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

[BlueZ v2 1/7] configure.ac: Update requirement to glib 2.34

[Bastien Nocera]

Update build requirements from 2011's glib 2.28 all the way up to 2012's
glib 2.34. This will allow us to use g_clear_pointer().
---
 acinclude.m4 | 4 ++--
 configure.ac | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index 4b73a5bfc38f..d0bfe6ccb2fa 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -62,8 +62,8 @@ AC_DEFUN([COMPILER_FLAGS], [
 		with_cflags="$with_cflags -Wswitch-enum"
 		with_cflags="$with_cflags -Wformat -Wformat-security"
 		with_cflags="$with_cflags -DG_DISABLE_DEPRECATED"
-		with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_28"
-		with_cflags="$with_cflags -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_32"
+		with_cflags="$with_cflags -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_34"
+		with_cflags="$with_cflags -DGLIB_VERSION_MAX_ALLOWED=GLIB_VERSION_2_34"
 	fi
 	AC_SUBST([WARNING_CFLAGS], $with_cflags)
 ])
diff --git a/configure.ac b/configure.ac
index 7093e41ff714..14bd15293918 100644
--- a/configure.ac
+++ b/configure.ac
@@ -81,7 +81,7 @@ AC_CHECK_DECLS([basename], [],
 				 ])
 
 
-PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.28)
+PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.34)
 
 if (test "${enable_threads}" = "yes"); then
 	AC_DEFINE(NEED_THREADS, 1, [Define if threading support is required])
-- 
2.47.0

RE: Fix bluetoothctl --help hanging if daemon isn't running

[bluez.test.bot]

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

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: client/main.c:3187
error: client/main.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

Re: Fix bluetoothctl --help hanging if daemon isn't running

[Bastien Nocera]

On Tue, 2024-10-22 at 05:31 -0700, bluez.test.bot@gmail.com wrote:
> This is an automated email and please do not reply to this email.
> 
> Dear Submitter,
> 
> Thank you for submitting the patches to the linux bluetooth mailing
> list.
> While preparing the CI tests, the patches you submitted couldn't be
> applied to the current HEAD of the repository.
> 
> ----- Output -----
> 
> error: patch failed: client/main.c:3187
> error: client/main.c: patch does not apply
> hint: Use 'git am --show-current-patch' to see the failed patch
> 
> Please resolve the issue and submit the patches again.

Which patch is that for? The error message doesn't say.

> 
> 
> ---
> Regards,
> Linux Bluetooth
> 

Re: Fix bluetoothctl --help hanging if daemon isn't running

[Bastien Nocera]

On Tue, 2024-10-22 at 14:40 +0200, Bastien Nocera wrote:
> On Tue, 2024-10-22 at 05:31 -0700, bluez.test.bot@gmail.com wrote:
> > This is an automated email and please do not reply to this email.
> > 
> > Dear Submitter,
> > 
> > Thank you for submitting the patches to the linux bluetooth mailing
> > list.
> > While preparing the CI tests, the patches you submitted couldn't be
> > applied to the current HEAD of the repository.
> > 
> > ----- Output -----
> > 
> > error: patch failed: client/main.c:3187
> > error: client/main.c: patch does not apply
> > hint: Use 'git am --show-current-patch' to see the failed patch
> > 
> > Please resolve the issue and submit the patches again.
> 
> Which patch is that for? The error message doesn't say.

Figured it out. Happy to send a patch to the bot if somebody tells me
where its code lives.