[PATCH BlueZ 0/6] Make commands return exit status

[PATCH BlueZ 0/6] Make commands return exit status

[ERAMOTO Masaya]

Commands always return 0 as exit status in non-interactive mode. This
patch set makes them return the exit_status variable as exit status.

Note:
  This patch set does not include the following fix for meshctl, due to
  conflict with Inga's patch ("mesh/meshctl: Exit cleanly if start up
  fails")[1]. I will send a rebased fix after merging Inga's patch to
  BlueZ master branch.

  [1]: https://www.spinics.net/lists/linux-bluetooth/msg75020.html


ERAMOTO Masaya (6):
  shared/shell: Return exit status to caller
  client: Return exit status for non-interactive
  tools/bluetooth-player: Return exit status for non-interactive
  tools/obexctl: Return exit status for non-interactive
  tools/btmgmt: Return exit status for non-interactive
  shared/mainloop: Fix overwriting exit status

 client/main.c            | 5 +++--
 src/shared/mainloop.c    | 4 +---
 src/shared/shell.c       | 7 +++++--
 src/shared/shell.h       | 2 +-
 tools/bluetooth-player.c | 5 +++--
 tools/btmgmt.c           | 2 +-
 tools/obexctl.c          | 5 +++--
 7 files changed, 17 insertions(+), 13 deletions(-)


---
 mesh/main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mesh/main.c b/mesh/main.c
index d991c9f8c..c79c44bd8 100644
--- a/mesh/main.c
+++ b/mesh/main.c
@@ -1904,6 +1904,7 @@ static void client_ready(GDBusClient *client, void *user_data)
 int main(int argc, char *argv[])
 {
 	GDBusClient *client;
+	int status;
 	int len;
 	int extra;
 
@@ -1989,7 +1990,7 @@ int main(int argc, char *argv[])
 	if (!onoff_client_init(PRIMARY_ELEMENT_IDX))
 		g_printerr("Failed to initialize mesh generic On/Off client\n");
 
-	bt_shell_run();
+	status = bt_shell_run();
 
 	g_dbus_client_unref(client);
 
@@ -2001,5 +2002,5 @@ int main(int argc, char *argv[])
 	g_list_free(service_list);
 	g_list_free_full(ctrl_list, proxy_leak);
 
-	return 0;
+	return status;
 }
--
2.14.1

[PATCH BlueZ 1/6] shared/shell: Return exit status to caller

[ERAMOTO Masaya]

---
 src/shared/shell.c | 7 +++++--
 src/shared/shell.h | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/shared/shell.c b/src/shared/shell.c
index 33bc0d980..b503c6628 100644
--- a/src/shared/shell.c
+++ b/src/shared/shell.c
@@ -993,17 +993,20 @@ static void env_destroy(void *data)
 	free(env);
 }
 
-void bt_shell_run(void)
+int bt_shell_run(void)
 {
 	struct io *signal;
+	int status;
 
 	signal = setup_signalfd();
 
-	mainloop_run();
+	status = mainloop_run();
 
 	io_destroy(signal);
 
 	bt_shell_cleanup();
+
+	return status;
 }
 
 void bt_shell_cleanup(void)
diff --git a/src/shared/shell.h b/src/shared/shell.h
index 33c31f35b..8b7cb7f30 100644
--- a/src/shared/shell.h
+++ b/src/shared/shell.h
@@ -66,7 +66,7 @@ struct bt_shell_opt {
 
 void bt_shell_init(int argc, char **argv, const struct bt_shell_opt *opt);
 
-void bt_shell_run(void);
+int bt_shell_run(void);
 
 void bt_shell_quit(int status);
 void bt_shell_noninteractive_quit(int status);
-- 
2.14.1

[PATCH BlueZ 2/6] client: Return exit status for non-interactive

[ERAMOTO Masaya]

---
 client/main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/client/main.c b/client/main.c
index 584217a82..7a100b552 100644
--- a/client/main.c
+++ b/client/main.c
@@ -2532,6 +2532,7 @@ static void client_ready(GDBusClient *client, void *user_data)
 int main(int argc, char *argv[])
 {
 	GDBusClient *client;
+	int status;
 
 	bt_shell_init(argc, argv, &opt);
 	bt_shell_set_menu(&main_menu);
@@ -2559,7 +2560,7 @@ int main(int argc, char *argv[])
 
 	g_dbus_client_set_ready_watch(client, client_ready, NULL);
 
-	bt_shell_run();
+	status = bt_shell_run();
 
 	g_dbus_client_unref(client);
 
@@ -2569,5 +2570,5 @@ int main(int argc, char *argv[])
 
 	g_free(auto_register_agent);
 
-	return 0;
+	return status;
 }
-- 
2.14.1

[PATCH BlueZ 3/6] tools/bluetooth-player: Return exit status for non-interactive

[ERAMOTO Masaya]

---
 tools/bluetooth-player.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/bluetooth-player.c b/tools/bluetooth-player.c
index 441a934cc..ccb0b5ee5 100644
--- a/tools/bluetooth-player.c
+++ b/tools/bluetooth-player.c
@@ -1119,6 +1119,7 @@ static void property_changed(GDBusProxy *proxy, const char *name,
 int main(int argc, char *argv[])
 {
 	GDBusClient *client;
+	int status;
 
 	bt_shell_init(argc, argv, NULL);
 	bt_shell_set_menu(&main_menu);
@@ -1134,11 +1135,11 @@ int main(int argc, char *argv[])
 	g_dbus_client_set_proxy_handlers(client, proxy_added, proxy_removed,
 							property_changed, NULL);
 
-	bt_shell_run();
+	status = bt_shell_run();
 
 	g_dbus_client_unref(client);
 
 	dbus_connection_unref(dbus_conn);
 
-	return 0;
+	return status;
 }
-- 
2.14.1

[PATCH BlueZ 4/6] tools/obexctl: Return exit status for non-interactive

[ERAMOTO Masaya]

---
 tools/obexctl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/obexctl.c b/tools/obexctl.c
index 3360972bd..f839ab0bd 100644
--- a/tools/obexctl.c
+++ b/tools/obexctl.c
@@ -2145,6 +2145,7 @@ static void property_changed(GDBusProxy *proxy, const char *name,
 int main(int argc, char *argv[])
 {
 	GDBusClient *client;
+	int status;
 
 	bt_shell_init(argc, argv, NULL);
 	bt_shell_set_menu(&main_menu);
@@ -2161,11 +2162,11 @@ int main(int argc, char *argv[])
 	g_dbus_client_set_proxy_handlers(client, proxy_added, proxy_removed,
 							property_changed, NULL);
 
-	bt_shell_run();
+	status = bt_shell_run();
 
 	g_dbus_client_unref(client);
 
 	dbus_connection_unref(dbus_conn);
 
-	return 0;
+	return status;
 }
-- 
2.14.1

[PATCH BlueZ 5/6] tools/btmgmt: Return exit status for non-interactive

[ERAMOTO Masaya]

---
 tools/btmgmt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/btmgmt.c b/tools/btmgmt.c
index 122c46d0d..f60bb8b53 100644
--- a/tools/btmgmt.c
+++ b/tools/btmgmt.c
@@ -4416,7 +4416,7 @@ int main(int argc, char *argv[])
 
 	bt_shell_attach(fileno(stdin));
 	update_prompt(mgmt_index);
-	bt_shell_run();
+	status = bt_shell_run();
 
 	mgmt_cancel_all(mgmt);
 	mgmt_unregister_all(mgmt);
-- 
2.14.1

[PATCH BlueZ 6/6] shared/mainloop: Fix overwriting exit status

[ERAMOTO Masaya]

Since mainloop_run() overwrites the exit_status variable even if having
called mainloop_{quit,exit_*}(), such as in case executing btmgmt with
an invalid command, it is initialized in declaration.
---
 src/shared/mainloop.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/shared/mainloop.c b/src/shared/mainloop.c
index 09c46a79a..e6ab9c43d 100644
--- a/src/shared/mainloop.c
+++ b/src/shared/mainloop.c
@@ -42,7 +42,7 @@
 
 static int epoll_fd;
 static int epoll_terminate;
-static int exit_status;
+static int exit_status = EXIT_SUCCESS;
 
 struct mainloop_data {
 	int fd;
@@ -141,8 +141,6 @@ int mainloop_run(void)
 		}
 	}
 
-	exit_status = EXIT_SUCCESS;
-
 	while (!epoll_terminate) {
 		struct epoll_event events[MAX_EPOLL_EVENTS];
 		int n, nfds;
-- 
2.14.1

Re: [PATCH BlueZ 0/6] Make commands return exit status

[Luiz Augusto von Dentz]

Hi Eramoto,

On Mon, Mar 26, 2018 at 11:25 AM, ERAMOTO Masaya
<eramoto.masaya@jp.fujitsu.com> wrote:
> Commands always return 0 as exit status in non-interactive mode. This
> patch set makes them return the exit_status variable as exit status.
>
> Note:
>   This patch set does not include the following fix for meshctl, due to
>   conflict with Inga's patch ("mesh/meshctl: Exit cleanly if start up
>   fails")[1]. I will send a rebased fix after merging Inga's patch to
>   BlueZ master branch.
>
>   [1]: https://www.spinics.net/lists/linux-bluetooth/msg75020.html

So here is the concern the exit status or the memory leaks, because
the latter is not really a problem if the process will be terminated
all the resources will be freed anyway so calling bt_shell_cleanup
seems pointless in this regard. The status seems alright as I can see
it being used with scripts that may parse the exit status.

>
> ERAMOTO Masaya (6):
>   shared/shell: Return exit status to caller
>   client: Return exit status for non-interactive
>   tools/bluetooth-player: Return exit status for non-interactive
>   tools/obexctl: Return exit status for non-interactive
>   tools/btmgmt: Return exit status for non-interactive
>   shared/mainloop: Fix overwriting exit status
>
>  client/main.c            | 5 +++--
>  src/shared/mainloop.c    | 4 +---
>  src/shared/shell.c       | 7 +++++--
>  src/shared/shell.h       | 2 +-
>  tools/bluetooth-player.c | 5 +++--
>  tools/btmgmt.c           | 2 +-
>  tools/obexctl.c          | 5 +++--
>  7 files changed, 17 insertions(+), 13 deletions(-)
>
>
> ---
>  mesh/main.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/mesh/main.c b/mesh/main.c
> index d991c9f8c..c79c44bd8 100644
> --- a/mesh/main.c
> +++ b/mesh/main.c
> @@ -1904,6 +1904,7 @@ static void client_ready(GDBusClient *client, void *user_data)
>  int main(int argc, char *argv[])
>  {
>         GDBusClient *client;
> +       int status;
>         int len;
>         int extra;
>
> @@ -1989,7 +1990,7 @@ int main(int argc, char *argv[])
>         if (!onoff_client_init(PRIMARY_ELEMENT_IDX))
>                 g_printerr("Failed to initialize mesh generic On/Off client\n");
>
> -       bt_shell_run();
> +       status = bt_shell_run();
>
>         g_dbus_client_unref(client);
>
> @@ -2001,5 +2002,5 @@ int main(int argc, char *argv[])
>         g_list_free(service_list);
>         g_list_free_full(ctrl_list, proxy_leak);
>
> -       return 0;
> +       return status;
>  }
> --
> 2.14.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ 0/6] Make commands return exit status

[ERAMOTO Masaya]

Hi Luiz,

On 03/26/2018 10:26 PM, Luiz Augusto von Dentz wrote:
> Hi Eramoto,
> 
> On Mon, Mar 26, 2018 at 11:25 AM, ERAMOTO Masaya
> <eramoto.masaya@jp.fujitsu.com> wrote:
>> Commands always return 0 as exit status in non-interactive mode. This
>> patch set makes them return the exit_status variable as exit status.
>>
>> Note:
>>   This patch set does not include the following fix for meshctl, due to
>>   conflict with Inga's patch ("mesh/meshctl: Exit cleanly if start up
>>   fails")[1]. I will send a rebased fix after merging Inga's patch to
>>   BlueZ master branch.
>>
>>   [1]: https://www.spinics.net/lists/linux-bluetooth/msg75020.html
> 
> So here is the concern the exit status or the memory leaks, because
> the latter is not really a problem if the process will be terminated
> all the resources will be freed anyway so calling bt_shell_cleanup
> seems pointless in this regard. The status seems alright as I can see
> it being used with scripts that may parse the exit status.

bluetoothctl etc. always return 0 except calling exit(), since
bt_shell_run() does not return value even if a subcommand fails.
Because it is hard for scripts to handle them whose exit status is 0
if they hit any unexpected problem, they are not convenient on
non-interactive mode.

This patch set makes bt_shell_run() return EXIT_SUCCESS/EXIT_FAILURE or
error code, and bluetoothctl etc. be able to pass the status to scripts.


Regards,
Eramoto


> 
>>
>> ERAMOTO Masaya (6):
>>   shared/shell: Return exit status to caller
>>   client: Return exit status for non-interactive
>>   tools/bluetooth-player: Return exit status for non-interactive
>>   tools/obexctl: Return exit status for non-interactive
>>   tools/btmgmt: Return exit status for non-interactive
>>   shared/mainloop: Fix overwriting exit status
>>
>>  client/main.c            | 5 +++--
>>  src/shared/mainloop.c    | 4 +---
>>  src/shared/shell.c       | 7 +++++--
>>  src/shared/shell.h       | 2 +-
>>  tools/bluetooth-player.c | 5 +++--
>>  tools/btmgmt.c           | 2 +-
>>  tools/obexctl.c          | 5 +++--
>>  7 files changed, 17 insertions(+), 13 deletions(-)
>>
>>
>> ---
>>  mesh/main.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/mesh/main.c b/mesh/main.c
>> index d991c9f8c..c79c44bd8 100644
>> --- a/mesh/main.c
>> +++ b/mesh/main.c
>> @@ -1904,6 +1904,7 @@ static void client_ready(GDBusClient *client, void *user_data)
>>  int main(int argc, char *argv[])
>>  {
>>         GDBusClient *client;
>> +       int status;
>>         int len;
>>         int extra;
>>
>> @@ -1989,7 +1990,7 @@ int main(int argc, char *argv[])
>>         if (!onoff_client_init(PRIMARY_ELEMENT_IDX))
>>                 g_printerr("Failed to initialize mesh generic On/Off client\n");
>>
>> -       bt_shell_run();
>> +       status = bt_shell_run();
>>
>>         g_dbus_client_unref(client);
>>
>> @@ -2001,5 +2002,5 @@ int main(int argc, char *argv[])
>>         g_list_free(service_list);
>>         g_list_free_full(ctrl_list, proxy_leak);
>>
>> -       return 0;
>> +       return status;
>>  }
>> --
>> 2.14.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 

Re: [PATCH BlueZ 0/6] Make commands return exit status

[Luiz Augusto von Dentz]

Hi Eramoto,

On Tue, Mar 27, 2018 at 8:38 AM, ERAMOTO Masaya
<eramoto.masaya@jp.fujitsu.com> wrote:
> Hi Luiz,
>
> On 03/26/2018 10:26 PM, Luiz Augusto von Dentz wrote:
>> Hi Eramoto,
>>
>> On Mon, Mar 26, 2018 at 11:25 AM, ERAMOTO Masaya
>> <eramoto.masaya@jp.fujitsu.com> wrote:
>>> Commands always return 0 as exit status in non-interactive mode. This
>>> patch set makes them return the exit_status variable as exit status.
>>>
>>> Note:
>>>   This patch set does not include the following fix for meshctl, due to
>>>   conflict with Inga's patch ("mesh/meshctl: Exit cleanly if start up
>>>   fails")[1]. I will send a rebased fix after merging Inga's patch to
>>>   BlueZ master branch.
>>>
>>>   [1]: https://www.spinics.net/lists/linux-bluetooth/msg75020.html
>>
>> So here is the concern the exit status or the memory leaks, because
>> the latter is not really a problem if the process will be terminated
>> all the resources will be freed anyway so calling bt_shell_cleanup
>> seems pointless in this regard. The status seems alright as I can see
>> it being used with scripts that may parse the exit status.
>
> bluetoothctl etc. always return 0 except calling exit(), since
> bt_shell_run() does not return value even if a subcommand fails.
> Because it is hard for scripts to handle them whose exit status is 0
> if they hit any unexpected problem, they are not convenient on
> non-interactive mode.
>
> This patch set makes bt_shell_run() return EXIT_SUCCESS/EXIT_FAILURE or
> error code, and bluetoothctl etc. be able to pass the status to scripts.

Applied, thanks.