[PATCH 0/4] Move common client code to bt_shell

[PATCH 0/4] Move common client code to bt_shell

[Marcin Kraglak]

This is first part of moving common code of client and meshctl to shared
directory. Next steps would be:
 - functions for switching between menus (needed for meshctl)
 - display functions
 - common support for history


Marcin Kraglak (4):
  shared/bt_shell: Add initial implementation
  client: Use bt_shell to process commands.
  shared/bt_shell: Add bt_shell_completion
  client: Use bt_shell_completion

 Makefile.tools        |   3 +-
 client/main.c         |  93 +++---------------------------
 src/shared/bt_shell.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/bt_shell.h |  43 ++++++++++++++
 4 files changed, 204 insertions(+), 87 deletions(-)
 create mode 100644 src/shared/bt_shell.c
 create mode 100644 src/shared/bt_shell.h

-- 
2.13.5

[PATCH 1/4] shared/bt_shell: Add initial implementation

[Marcin Kraglak]

This module will handle command line in applications like bluetoothctl and meshctl.
---
 src/shared/bt_shell.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/bt_shell.h | 42 +++++++++++++++++++++++
 2 files changed, 134 insertions(+)
 create mode 100644 src/shared/bt_shell.c
 create mode 100644 src/shared/bt_shell.h

diff --git a/src/shared/bt_shell.c b/src/shared/bt_shell.c
new file mode 100644
index 000000000..78d23c7ea
--- /dev/null
+++ b/src/shared/bt_shell.c
@@ -0,0 +1,92 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2017  Intel Corporation. All rights reserved.
+ *
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdio.h>
+#include "src/shared/util.h"
+#include "src/shared/queue.h"
+#include "src/shared/bt_shell.h"
+
+#define CMD_LENGTH	48
+
+static struct {
+	const struct bt_shell_menu_entry *current;
+} bt_shell_data;
+
+bool bt_shell_init(const struct bt_shell_menu_entry *menu)
+{
+	if (bt_shell_data.current || !menu)
+		return false;
+
+	bt_shell_data.current = menu;
+
+	return true;
+}
+
+void bt_shell_cleanup(void)
+{
+	bt_shell_data.current = NULL;
+}
+
+void bt_shell_process(const char *cmd, const char *arg)
+{
+	const struct bt_shell_menu_entry *entry;
+
+	if (!bt_shell_data.current || !cmd)
+		return;
+
+	for (entry = bt_shell_data.current; entry->cmd; entry++) {
+		if (strcmp(cmd, entry->cmd))
+			continue;
+
+		if (entry->func) {
+			entry->func(arg);
+			return;
+		}
+	}
+
+	if (strcmp(cmd, "help")) {
+		printf("Invalid command\n");
+		return;
+	}
+
+	bt_shell_print_menu();
+}
+
+void bt_shell_print_menu(void)
+{
+	const struct bt_shell_menu_entry *entry;
+
+	if (!bt_shell_data.current)
+		return;
+
+	printf("Available commands:\n");
+	for (entry = bt_shell_data.current; entry->cmd; entry++) {
+		printf("  %s %-*s %s\n", entry->cmd,
+					(int)(CMD_LENGTH - strlen(entry->cmd)),
+					entry->arg ? : "", entry->desc ? : "");
+	}
+}
diff --git a/src/shared/bt_shell.h b/src/shared/bt_shell.h
new file mode 100644
index 000000000..93d7ed771
--- /dev/null
+++ b/src/shared/bt_shell.h
@@ -0,0 +1,42 @@
+/*
+ *
+ *  BlueZ - Bluetooth protocol stack for Linux
+ *
+ *  Copyright (C) 2017  Intel Corporation. All rights reserved.
+ *
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+typedef void (*bt_shell_menu_cb_t)(const char *arg);
+typedef char * (*bt_shell_menu_gen_t)(const char *text, int state);
+typedef void (*bt_shell_menu_disp_t) (char **matches, int num_matches,
+							int max_length);
+
+struct bt_shell_menu_entry {
+	const char *cmd;
+	const char *arg;
+	bt_shell_menu_cb_t func;
+	const char *desc;
+	bt_shell_menu_gen_t gen;
+	bt_shell_menu_disp_t disp;
+};
+
+bool bt_shell_init(const struct bt_shell_menu_entry *menu);
+void bt_shell_cleanup(void);
+
+void bt_shell_process(const char *cmd, const char *arg);
+void bt_shell_print_menu(void);
-- 
2.13.5

[PATCH 2/4] client: Use bt_shell to process commands.

[Marcin Kraglak]

Use bt_shell to process and print available commands.
---
 Makefile.tools |  3 ++-
 client/main.c  | 44 ++++++--------------------------------------
 2 files changed, 8 insertions(+), 39 deletions(-)

diff --git a/Makefile.tools b/Makefile.tools
index 561302fa1..4893acd8a 100644
--- a/Makefile.tools
+++ b/Makefile.tools
@@ -8,7 +8,8 @@ client_bluetoothctl_SOURCES = client/main.c \
 					client/advertising.h \
 					client/advertising.c \
 					client/gatt.h client/gatt.c \
-					monitor/uuid.h monitor/uuid.c
+					monitor/uuid.h monitor/uuid.c \
+					src/shared/bt_shell.h src/shared/bt_shell.c
 client_bluetoothctl_LDADD = gdbus/libgdbus-internal.la src/libshared-glib.la \
 				@GLIB_LIBS@ @DBUS_LIBS@ -lreadline
 endif
diff --git a/client/main.c b/client/main.c
index 91b728a12..87019b463 100644
--- a/client/main.c
+++ b/client/main.c
@@ -38,6 +38,7 @@
 #include <readline/history.h>
 #include <glib.h>
 
+#include "src/shared/bt_shell.h"
 #include "src/shared/util.h"
 #include "gdbus/gdbus.h"
 #include "monitor/uuid.h"
@@ -2435,14 +2436,7 @@ static void cmd_set_advertise_appearance(const char *arg)
 	ad_advertise_local_appearance(dbus_conn, value);
 }
 
-static const struct {
-	const char *cmd;
-	const char *arg;
-	void (*func) (const char *arg);
-	const char *desc;
-	char * (*gen) (const char *text, int state);
-	void (*disp) (char **matches, int num_matches, int max_length);
-} cmd_table[] = {
+static const struct bt_shell_menu_entry cmd_table[] = {
 	{ "list",         NULL,       cmd_list, "List available controllers" },
 	{ "show",         "[ctrl]",   cmd_show, "Controller information",
 							ctrl_generator },
@@ -2630,7 +2624,6 @@ static char **cmd_completion(const char *text, int start, int end)
 static void rl_handler(char *input)
 {
 	char *cmd, *arg;
-	int i;
 
 	if (!input) {
 		rl_insert_text("quit");
@@ -2659,41 +2652,14 @@ static void rl_handler(char *input)
 			arg[len - 1] = '\0';
 	}
 
-	for (i = 0; cmd_table[i].cmd; i++) {
-		if (strcmp(cmd, cmd_table[i].cmd))
-			continue;
-
-		if (cmd_table[i].func) {
-			cmd_table[i].func(arg);
-			goto done;
-		}
-	}
-
-	printf("Invalid command\n");
+	bt_shell_process(cmd, arg);
 done:
 	free(input);
 }
 
 static void cmd_help(const char *arg)
 {
-	int i;
-
-	printf("Available commands:\n");
-
-	for (i = 0; cmd_table[i].cmd; i++) {
-		if ((int)strlen(cmd_table[i].arg? : "") <=
-					(int)(25 - strlen(cmd_table[i].cmd)))
-			printf("  %s %-*s %s\n", cmd_table[i].cmd,
-					(int)(25 - strlen(cmd_table[i].cmd)),
-					cmd_table[i].arg ? : "",
-					cmd_table[i].desc ? : "");
-		else
-			printf("  %s %-s\n" "  %s %-25s %s\n",
-					cmd_table[i].cmd,
-					cmd_table[i].arg ? : "",
-					"", "",
-					cmd_table[i].desc ? : "");
-	}
+	bt_shell_print_menu();
 }
 
 static gboolean signal_handler(GIOChannel *channel, GIOCondition condition,
@@ -2844,6 +2810,7 @@ int main(int argc, char *argv[])
 	dbus_conn = g_dbus_setup_bus(DBUS_BUS_SYSTEM, NULL, NULL);
 	g_dbus_attach_object_manager(dbus_conn);
 
+	bt_shell_init(cmd_table);
 	setlinebuf(stdout);
 	rl_attempted_completion_function = cmd_completion;
 
@@ -2874,6 +2841,7 @@ int main(int argc, char *argv[])
 
 	rl_message("");
 	rl_callback_handler_remove();
+	bt_shell_cleanup();
 
 	dbus_connection_unref(dbus_conn);
 	g_main_loop_unref(main_loop);
-- 
2.13.5

[PATCH 3/4] shared/bt_shell: Add bt_shell_completion

[Marcin Kraglak]

This function may be used as rl_attempted_completion_function
in applications using readline.
---
 src/shared/bt_shell.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/shared/bt_shell.h |  1 +
 2 files changed, 61 insertions(+)

diff --git a/src/shared/bt_shell.c b/src/shared/bt_shell.c
index 78d23c7ea..d27020b7f 100644
--- a/src/shared/bt_shell.c
+++ b/src/shared/bt_shell.c
@@ -26,6 +26,7 @@
 #endif
 
 #include <stdio.h>
+#include <readline/readline.h>
 #include "src/shared/util.h"
 #include "src/shared/queue.h"
 #include "src/shared/bt_shell.h"
@@ -90,3 +91,62 @@ void bt_shell_print_menu(void)
 					entry->arg ? : "", entry->desc ? : "");
 	}
 }
+
+static char *cmd_generator(const char *text, int state)
+{
+	const struct bt_shell_menu_entry *entry;
+	static int index, len;
+	const char *cmd;
+
+	entry = bt_shell_data.current;
+
+	if (!state) {
+		index = 0;
+		len = strlen(text);
+	}
+
+	while ((cmd = entry[index].cmd)) {
+		index++;
+
+		if (!strncmp(cmd, text, len))
+			return strdup(cmd);
+	}
+
+	return NULL;
+}
+
+char **bt_shell_completion(const char *text, int start, int end)
+{
+	char **matches = NULL;
+
+	if (!bt_shell_data.current)
+		return NULL;
+
+	if (start > 0) {
+		const struct bt_shell_menu_entry *entry;
+		char *input_cmd;
+
+		input_cmd = strndup(rl_line_buffer, start - 1);
+		for (entry = bt_shell_data.current; entry->cmd; entry++) {
+			if (strcmp(entry->cmd, input_cmd))
+				continue;
+
+			if (!entry->gen)
+				continue;
+
+			rl_completion_display_matches_hook = entry->disp;
+			matches = rl_completion_matches(text, entry->gen);
+			break;
+		}
+
+		free(input_cmd);
+	} else {
+		rl_completion_display_matches_hook = NULL;
+		matches = rl_completion_matches(text, cmd_generator);
+	}
+
+	if (!matches)
+		rl_attempted_completion_over = 1;
+
+	return matches;
+}
diff --git a/src/shared/bt_shell.h b/src/shared/bt_shell.h
index 93d7ed771..c30a3dcbd 100644
--- a/src/shared/bt_shell.h
+++ b/src/shared/bt_shell.h
@@ -39,4 +39,5 @@ bool bt_shell_init(const struct bt_shell_menu_entry *menu);
 void bt_shell_cleanup(void);
 
 void bt_shell_process(const char *cmd, const char *arg);
+char **bt_shell_completion(const char *text, int start, int end);
 void bt_shell_print_menu(void);
-- 
2.13.5

[PATCH 4/4] client: Use bt_shell_completion

[Marcin Kraglak]

---
 client/main.c | 49 +------------------------------------------------
 1 file changed, 1 insertion(+), 48 deletions(-)

diff --git a/client/main.c b/client/main.c
index 87019b463..86514f67e 100644
--- a/client/main.c
+++ b/client/main.c
@@ -2564,61 +2564,14 @@ static const struct bt_shell_menu_entry cmd_table[] = {
 	{ }
 };
 
-static char *cmd_generator(const char *text, int state)
-{
-	static int index, len;
-	const char *cmd;
-
-	if (!state) {
-		index = 0;
-		len = strlen(text);
-	}
-
-	while ((cmd = cmd_table[index].cmd)) {
-		index++;
-
-		if (!strncmp(cmd, text, len))
-			return strdup(cmd);
-	}
-
-	return NULL;
-}
-
 static char **cmd_completion(const char *text, int start, int end)
 {
-	char **matches = NULL;
-
 	if (agent_completion() == TRUE) {
 		rl_attempted_completion_over = 1;
 		return NULL;
 	}
 
-	if (start > 0) {
-		int i;
-		char *input_cmd;
-
-		input_cmd = g_strndup(rl_line_buffer, start -1);
-		for (i = 0; cmd_table[i].cmd; i++) {
-			if (strcmp(cmd_table[i].cmd, input_cmd))
-				continue;
-
-			if (!cmd_table[i].gen)
-				continue;
-
-			rl_completion_display_matches_hook = cmd_table[i].disp;
-			matches = rl_completion_matches(text, cmd_table[i].gen);
-			break;
-		}
-		g_free(input_cmd);
-	} else {
-		rl_completion_display_matches_hook = NULL;
-		matches = rl_completion_matches(text, cmd_generator);
-	}
-
-	if (!matches)
-		rl_attempted_completion_over = 1;
-
-	return matches;
+	return bt_shell_completion(text, start, end);
 }
 
 static void rl_handler(char *input)
-- 
2.13.5

Re: [PATCH 1/4] shared/bt_shell: Add initial implementation

[ERAMOTO Masaya]

Hi Marcin,

> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <stdio.h>
> +#include "src/shared/util.h"
> +#include "src/shared/queue.h"
> +#include "src/shared/bt_shell.h"
> +
> +#define CMD_LENGTH	48
> +
> +static struct {
> +	const struct bt_shell_menu_entry *current;
> +} bt_shell_data;
> +
> +bool bt_shell_init(const struct bt_shell_menu_entry *menu)
> +{
> +	if (bt_shell_data.current || !menu)
> +		return false;
> +
> +	bt_shell_data.current = menu;
> +
> +	return true;
> +}
> +
> +void bt_shell_cleanup(void)
> +{
> +	bt_shell_data.current = NULL;
> +}
> +
> +void bt_shell_process(const char *cmd, const char *arg)
> +{
> +	const struct bt_shell_menu_entry *entry;
> +
> +	if (!bt_shell_data.current || !cmd)
> +		return;
> +
> +	for (entry = bt_shell_data.current; entry->cmd; entry++) {
> +		if (strcmp(cmd, entry->cmd))
> +			continue;
> +
> +		if (entry->func) {
> +			entry->func(arg);
> +			return;
> +		}
> +	}
> +
> +	if (strcmp(cmd, "help")) {
> +		printf("Invalid command\n");
> +		return;
> +	}
> +
> +	bt_shell_print_menu();
> +}
> +
> +void bt_shell_print_menu(void)
> +{
> +	const struct bt_shell_menu_entry *entry;
> +
> +	if (!bt_shell_data.current)
> +		return;
> +
> +	printf("Available commands:\n");
> +	for (entry = bt_shell_data.current; entry->cmd; entry++) {
> +		printf("  %s %-*s %s\n", entry->cmd,
> +					(int)(CMD_LENGTH - strlen(entry->cmd)),
> +					entry->arg ? : "", entry->desc ? : "");


I think that it is better to
  - add some white-spaces
or
  - add a new line
between the argument string and the description string, because it is a little
difficult for the help of register-characteristic to read as below:

  [bluetooth]# help
  Available commands:
  ...
    unregister-service <UUID/object>                  Unregister application service
    register-characteristic <UUID> <Flags=read,write,notify...> Register application characteristic
    unregister-characteristic <UUID/object>           Unregister application characteristic
    register-descriptor <UUID> <Flags=read,write...>  Register application descriptor


Regards,
Eramoto

Re: [PATCH 1/4] shared/bt_shell: Add initial implementation

[Luiz Augusto von Dentz]

Hi Eramoto, Marcin,

On Fri, Sep 22, 2017 at 9:27 AM, ERAMOTO Masaya
<eramoto.masaya@jp.fujitsu.com> wrote:
> Hi Marcin,
>
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +
>> +#include <stdio.h>
>> +#include "src/shared/util.h"
>> +#include "src/shared/queue.h"
>> +#include "src/shared/bt_shell.h"
>> +
>> +#define CMD_LENGTH   48
>> +
>> +static struct {
>> +     const struct bt_shell_menu_entry *current;
>> +} bt_shell_data;
>> +
>> +bool bt_shell_init(const struct bt_shell_menu_entry *menu)
>> +{
>> +     if (bt_shell_data.current || !menu)
>> +             return false;
>> +
>> +     bt_shell_data.current = menu;
>> +
>> +     return true;
>> +}
>> +
>> +void bt_shell_cleanup(void)
>> +{
>> +     bt_shell_data.current = NULL;
>> +}
>> +
>> +void bt_shell_process(const char *cmd, const char *arg)
>> +{
>> +     const struct bt_shell_menu_entry *entry;
>> +
>> +     if (!bt_shell_data.current || !cmd)
>> +             return;
>> +
>> +     for (entry = bt_shell_data.current; entry->cmd; entry++) {
>> +             if (strcmp(cmd, entry->cmd))
>> +                     continue;
>> +
>> +             if (entry->func) {
>> +                     entry->func(arg);
>> +                     return;
>> +             }
>> +     }
>> +
>> +     if (strcmp(cmd, "help")) {
>> +             printf("Invalid command\n");
>> +             return;
>> +     }
>> +
>> +     bt_shell_print_menu();
>> +}
>> +
>> +void bt_shell_print_menu(void)
>> +{
>> +     const struct bt_shell_menu_entry *entry;
>> +
>> +     if (!bt_shell_data.current)
>> +             return;
>> +
>> +     printf("Available commands:\n");
>> +     for (entry = bt_shell_data.current; entry->cmd; entry++) {
>> +             printf("  %s %-*s %s\n", entry->cmd,
>> +                                     (int)(CMD_LENGTH - strlen(entry->cmd)),
>> +                                     entry->arg ? : "", entry->desc ? : "");
>
>
> I think that it is better to
>   - add some white-spaces
> or
>   - add a new line
> between the argument string and the description string, because it is a little
> difficult for the help of register-characteristic to read as below:
>
>   [bluetooth]# help
>   Available commands:
>   ...
>     unregister-service <UUID/object>                  Unregister application service
>     register-characteristic <UUID> <Flags=read,write,notify...> Register application characteristic
>     unregister-characteristic <UUID/object>           Unregister application characteristic
>     register-descriptor <UUID> <Flags=read,write...>  Register application descriptor

It should probably have the same formatting as bluetoothctl:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/main.c#n2677

-- 
Luiz Augusto von Dentz

Re: [PATCH 4/4] client: Use bt_shell_completion

[Luiz Augusto von Dentz]

Hi Marcin,

On Fri, Sep 22, 2017 at 8:00 AM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> ---
>  client/main.c | 49 +------------------------------------------------
>  1 file changed, 1 insertion(+), 48 deletions(-)
>
> diff --git a/client/main.c b/client/main.c
> index 87019b463..86514f67e 100644
> --- a/client/main.c
> +++ b/client/main.c
> @@ -2564,61 +2564,14 @@ static const struct bt_shell_menu_entry cmd_table[] = {
>         { }
>  };
>
> -static char *cmd_generator(const char *text, int state)
> -{
> -       static int index, len;
> -       const char *cmd;
> -
> -       if (!state) {
> -               index = 0;
> -               len = strlen(text);
> -       }
> -
> -       while ((cmd = cmd_table[index].cmd)) {
> -               index++;
> -
> -               if (!strncmp(cmd, text, len))
> -                       return strdup(cmd);
> -       }
> -
> -       return NULL;
> -}
> -
>  static char **cmd_completion(const char *text, int start, int end)
>  {
> -       char **matches = NULL;
> -
>         if (agent_completion() == TRUE) {
>                 rl_attempted_completion_over = 1;
>                 return NULL;
>         }
>
> -       if (start > 0) {
> -               int i;
> -               char *input_cmd;
> -
> -               input_cmd = g_strndup(rl_line_buffer, start -1);
> -               for (i = 0; cmd_table[i].cmd; i++) {
> -                       if (strcmp(cmd_table[i].cmd, input_cmd))
> -                               continue;
> -
> -                       if (!cmd_table[i].gen)
> -                               continue;
> -
> -                       rl_completion_display_matches_hook = cmd_table[i].disp;
> -                       matches = rl_completion_matches(text, cmd_table[i].gen);
> -                       break;
> -               }
> -               g_free(input_cmd);
> -       } else {
> -               rl_completion_display_matches_hook = NULL;
> -               matches = rl_completion_matches(text, cmd_generator);
> -       }
> -
> -       if (!matches)
> -               rl_attempted_completion_over = 1;
> -
> -       return matches;
> +       return bt_shell_completion(text, start, end);
>  }

While this is great as we reuse more code I think we better move the
whole input handling into the bt_shell so the application don't even
need to call bt_shell_completion, etc.

-- 
Luiz Augusto von Dentz

Re: [PATCH 1/4] shared/bt_shell: Add initial implementation

[Marcin Kraglak]

Hi Luiz Eramoto,

On 22 September 2017 at 03:42, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Eramoto, Marcin,
>
> On Fri, Sep 22, 2017 at 9:27 AM, ERAMOTO Masaya
> <eramoto.masaya@jp.fujitsu.com> wrote:
>> Hi Marcin,
>>
>>> +#ifdef HAVE_CONFIG_H
>>> +#include <config.h>
>>> +#endif
>>> +
>>> +#include <stdio.h>
>>> +#include "src/shared/util.h"
>>> +#include "src/shared/queue.h"
>>> +#include "src/shared/bt_shell.h"
>>> +
>>> +#define CMD_LENGTH   48
>>> +
>>> +static struct {
>>> +     const struct bt_shell_menu_entry *current;
>>> +} bt_shell_data;
>>> +
>>> +bool bt_shell_init(const struct bt_shell_menu_entry *menu)
>>> +{
>>> +     if (bt_shell_data.current || !menu)
>>> +             return false;
>>> +
>>> +     bt_shell_data.current = menu;
>>> +
>>> +     return true;
>>> +}
>>> +
>>> +void bt_shell_cleanup(void)
>>> +{
>>> +     bt_shell_data.current = NULL;
>>> +}
>>> +
>>> +void bt_shell_process(const char *cmd, const char *arg)
>>> +{
>>> +     const struct bt_shell_menu_entry *entry;
>>> +
>>> +     if (!bt_shell_data.current || !cmd)
>>> +             return;
>>> +
>>> +     for (entry = bt_shell_data.current; entry->cmd; entry++) {
>>> +             if (strcmp(cmd, entry->cmd))
>>> +                     continue;
>>> +
>>> +             if (entry->func) {
>>> +                     entry->func(arg);
>>> +                     return;
>>> +             }
>>> +     }
>>> +
>>> +     if (strcmp(cmd, "help")) {
>>> +             printf("Invalid command\n");
>>> +             return;
>>> +     }
>>> +
>>> +     bt_shell_print_menu();
>>> +}
>>> +
>>> +void bt_shell_print_menu(void)
>>> +{
>>> +     const struct bt_shell_menu_entry *entry;
>>> +
>>> +     if (!bt_shell_data.current)
>>> +             return;
>>> +
>>> +     printf("Available commands:\n");
>>> +     for (entry = bt_shell_data.current; entry->cmd; entry++) {
>>> +             printf("  %s %-*s %s\n", entry->cmd,
>>> +                                     (int)(CMD_LENGTH - strlen(entry->cmd)),
>>> +                                     entry->arg ? : "", entry->desc ? : "");
>>
>>
>> I think that it is better to
>>   - add some white-spaces
>> or
>>   - add a new line
>> between the argument string and the description string, because it is a little
>> difficult for the help of register-characteristic to read as below:
>>
>>   [bluetooth]# help
>>   Available commands:
>>   ...
>>     unregister-service <UUID/object>                  Unregister application service
>>     register-characteristic <UUID> <Flags=read,write,notify...> Register application characteristic
>>     unregister-characteristic <UUID/object>           Unregister application characteristic
>>     register-descriptor <UUID> <Flags=read,write...>  Register application descriptor
>
> It should probably have the same formatting as bluetoothctl:
>
> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/main.c#n2677
>
> --
> Luiz Augusto von Dentz

Yes, I'll correct it

-- 
BR
Marcin Kraglak

Re: [PATCH 4/4] client: Use bt_shell_completion

[Marcin Kraglak]

Hi Luiz,

On 22 September 2017 at 03:47, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Marcin,
>
> On Fri, Sep 22, 2017 at 8:00 AM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
>> ---
>>  client/main.c | 49 +------------------------------------------------
>>  1 file changed, 1 insertion(+), 48 deletions(-)
>>
>> diff --git a/client/main.c b/client/main.c
>> index 87019b463..86514f67e 100644
>> --- a/client/main.c
>> +++ b/client/main.c
>> @@ -2564,61 +2564,14 @@ static const struct bt_shell_menu_entry cmd_table[] = {
>>         { }
>>  };
>>
>> -static char *cmd_generator(const char *text, int state)
>> -{
>> -       static int index, len;
>> -       const char *cmd;
>> -
>> -       if (!state) {
>> -               index = 0;
>> -               len = strlen(text);
>> -       }
>> -
>> -       while ((cmd = cmd_table[index].cmd)) {
>> -               index++;
>> -
>> -               if (!strncmp(cmd, text, len))
>> -                       return strdup(cmd);
>> -       }
>> -
>> -       return NULL;
>> -}
>> -
>>  static char **cmd_completion(const char *text, int start, int end)
>>  {
>> -       char **matches = NULL;
>> -
>>         if (agent_completion() == TRUE) {
>>                 rl_attempted_completion_over = 1;
>>                 return NULL;
>>         }
>>
>> -       if (start > 0) {
>> -               int i;
>> -               char *input_cmd;
>> -
>> -               input_cmd = g_strndup(rl_line_buffer, start -1);
>> -               for (i = 0; cmd_table[i].cmd; i++) {
>> -                       if (strcmp(cmd_table[i].cmd, input_cmd))
>> -                               continue;
>> -
>> -                       if (!cmd_table[i].gen)
>> -                               continue;
>> -
>> -                       rl_completion_display_matches_hook = cmd_table[i].disp;
>> -                       matches = rl_completion_matches(text, cmd_table[i].gen);
>> -                       break;
>> -               }
>> -               g_free(input_cmd);
>> -       } else {
>> -               rl_completion_display_matches_hook = NULL;
>> -               matches = rl_completion_matches(text, cmd_generator);
>> -       }
>> -
>> -       if (!matches)
>> -               rl_attempted_completion_over = 1;
>> -
>> -       return matches;
>> +       return bt_shell_completion(text, start, end);
>>  }
>
> While this is great as we reuse more code I think we better move the
> whole input handling into the bt_shell so the application don't even
> need to call bt_shell_completion, etc.
>
> --
> Luiz Augusto von Dentz

You mean bt_shell should set rl_attempted_completion_function in
bt_shell_init() (then bt_shell_completion is not exposed)?



-- 
BR
Marcin Kraglak

Re: [PATCH 4/4] client: Use bt_shell_completion

[Luiz Augusto von Dentz]

Hi Marcin,

On Fri, Sep 22, 2017 at 11:00 AM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Hi Luiz,
>
> On 22 September 2017 at 03:47, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Marcin,
>>
>> On Fri, Sep 22, 2017 at 8:00 AM, Marcin Kraglak
>> <marcin.kraglak@tieto.com> wrote:
>>> ---
>>>  client/main.c | 49 +------------------------------------------------
>>>  1 file changed, 1 insertion(+), 48 deletions(-)
>>>
>>> diff --git a/client/main.c b/client/main.c
>>> index 87019b463..86514f67e 100644
>>> --- a/client/main.c
>>> +++ b/client/main.c
>>> @@ -2564,61 +2564,14 @@ static const struct bt_shell_menu_entry cmd_table[] = {
>>>         { }
>>>  };
>>>
>>> -static char *cmd_generator(const char *text, int state)
>>> -{
>>> -       static int index, len;
>>> -       const char *cmd;
>>> -
>>> -       if (!state) {
>>> -               index = 0;
>>> -               len = strlen(text);
>>> -       }
>>> -
>>> -       while ((cmd = cmd_table[index].cmd)) {
>>> -               index++;
>>> -
>>> -               if (!strncmp(cmd, text, len))
>>> -                       return strdup(cmd);
>>> -       }
>>> -
>>> -       return NULL;
>>> -}
>>> -
>>>  static char **cmd_completion(const char *text, int start, int end)
>>>  {
>>> -       char **matches = NULL;
>>> -
>>>         if (agent_completion() == TRUE) {
>>>                 rl_attempted_completion_over = 1;
>>>                 return NULL;
>>>         }
>>>
>>> -       if (start > 0) {
>>> -               int i;
>>> -               char *input_cmd;
>>> -
>>> -               input_cmd = g_strndup(rl_line_buffer, start -1);
>>> -               for (i = 0; cmd_table[i].cmd; i++) {
>>> -                       if (strcmp(cmd_table[i].cmd, input_cmd))
>>> -                               continue;
>>> -
>>> -                       if (!cmd_table[i].gen)
>>> -                               continue;
>>> -
>>> -                       rl_completion_display_matches_hook = cmd_table[i].disp;
>>> -                       matches = rl_completion_matches(text, cmd_table[i].gen);
>>> -                       break;
>>> -               }
>>> -               g_free(input_cmd);
>>> -       } else {
>>> -               rl_completion_display_matches_hook = NULL;
>>> -               matches = rl_completion_matches(text, cmd_generator);
>>> -       }
>>> -
>>> -       if (!matches)
>>> -               rl_attempted_completion_over = 1;
>>> -
>>> -       return matches;
>>> +       return bt_shell_completion(text, start, end);
>>>  }
>>
>> While this is great as we reuse more code I think we better move the
>> whole input handling into the bt_shell so the application don't even
>> need to call bt_shell_completion, etc.

Not only that but the entire readline setup including
rl_callback_handler_install which means the tools don't really need to
use readline functions just bt_shell.
>> --
>> Luiz Augusto von Dentz
>
> You mean bt_shell should set rl_attempted_completion_function in
> bt_shell_init() (then bt_shell_completion is not exposed)?
>
>
>
> --
> BR
> Marcin Kraglak



-- 
Luiz Augusto von Dentz

Re: [PATCH 1/4] shared/bt_shell: Add initial implementation

[Luiz Augusto von Dentz]

Hi Marcin,

On Fri, Sep 22, 2017 at 10:52 AM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> Hi Luiz Eramoto,
>
> On 22 September 2017 at 03:42, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi Eramoto, Marcin,
>>
>> On Fri, Sep 22, 2017 at 9:27 AM, ERAMOTO Masaya
>> <eramoto.masaya@jp.fujitsu.com> wrote:
>>> Hi Marcin,
>>>
>>>> +#ifdef HAVE_CONFIG_H
>>>> +#include <config.h>
>>>> +#endif
>>>> +
>>>> +#include <stdio.h>
>>>> +#include "src/shared/util.h"
>>>> +#include "src/shared/queue.h"
>>>> +#include "src/shared/bt_shell.h"
>>>> +
>>>> +#define CMD_LENGTH   48
>>>> +
>>>> +static struct {
>>>> +     const struct bt_shell_menu_entry *current;
>>>> +} bt_shell_data;
>>>> +
>>>> +bool bt_shell_init(const struct bt_shell_menu_entry *menu)
>>>> +{
>>>> +     if (bt_shell_data.current || !menu)
>>>> +             return false;
>>>> +
>>>> +     bt_shell_data.current = menu;
>>>> +
>>>> +     return true;
>>>> +}
>>>> +
>>>> +void bt_shell_cleanup(void)
>>>> +{
>>>> +     bt_shell_data.current = NULL;
>>>> +}
>>>> +
>>>> +void bt_shell_process(const char *cmd, const char *arg)
>>>> +{
>>>> +     const struct bt_shell_menu_entry *entry;
>>>> +
>>>> +     if (!bt_shell_data.current || !cmd)
>>>> +             return;
>>>> +
>>>> +     for (entry = bt_shell_data.current; entry->cmd; entry++) {
>>>> +             if (strcmp(cmd, entry->cmd))
>>>> +                     continue;
>>>> +
>>>> +             if (entry->func) {
>>>> +                     entry->func(arg);
>>>> +                     return;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     if (strcmp(cmd, "help")) {
>>>> +             printf("Invalid command\n");
>>>> +             return;
>>>> +     }
>>>> +
>>>> +     bt_shell_print_menu();
>>>> +}
>>>> +
>>>> +void bt_shell_print_menu(void)
>>>> +{
>>>> +     const struct bt_shell_menu_entry *entry;
>>>> +
>>>> +     if (!bt_shell_data.current)
>>>> +             return;
>>>> +
>>>> +     printf("Available commands:\n");
>>>> +     for (entry = bt_shell_data.current; entry->cmd; entry++) {
>>>> +             printf("  %s %-*s %s\n", entry->cmd,
>>>> +                                     (int)(CMD_LENGTH - strlen(entry->cmd)),
>>>> +                                     entry->arg ? : "", entry->desc ? : "");
>>>
>>>
>>> I think that it is better to
>>>   - add some white-spaces
>>> or
>>>   - add a new line
>>> between the argument string and the description string, because it is a little
>>> difficult for the help of register-characteristic to read as below:
>>>
>>>   [bluetooth]# help
>>>   Available commands:
>>>   ...
>>>     unregister-service <UUID/object>                  Unregister application service
>>>     register-characteristic <UUID> <Flags=read,write,notify...> Register application characteristic
>>>     unregister-characteristic <UUID/object>           Unregister application characteristic
>>>     register-descriptor <UUID> <Flags=read,write...>  Register application descriptor
>>
>> It should probably have the same formatting as bluetoothctl:
>>
>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/main.c#n2677
>>
>> --
>> Luiz Augusto von Dentz
>
> Yes, I'll correct it

Are you still working on this?

-- 
Luiz Augusto von Dentz

Re: [PATCH 0/4] Move common client code to bt_shell

[Luiz Augusto von Dentz]

Hi Marcin,

On Fri, Sep 22, 2017 at 7:59 AM, Marcin Kraglak
<marcin.kraglak@tieto.com> wrote:
> This is first part of moving common code of client and meshctl to shared
> directory. Next steps would be:
>  - functions for switching between menus (needed for meshctl)
>  - display functions
>  - common support for history
>
>
> Marcin Kraglak (4):
>   shared/bt_shell: Add initial implementation
>   client: Use bt_shell to process commands.
>   shared/bt_shell: Add bt_shell_completion
>   client: Use bt_shell_completion
>
>  Makefile.tools        |   3 +-
>  client/main.c         |  93 +++---------------------------
>  src/shared/bt_shell.c | 152 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/shared/bt_shell.h |  43 ++++++++++++++
>  4 files changed, 204 insertions(+), 87 deletions(-)
>  create mode 100644 src/shared/bt_shell.c
>  create mode 100644 src/shared/bt_shell.h
>
> --
> 2.13.5

Would you be able to continue working on this?

-- 
Luiz Augusto von Dentz

Re: [PATCH 1/4] shared/bt_shell: Add initial implementation

[Marcin Kraglak]

Hi Luiz,

On Mon, 2017-10-23 at 14:42 +0300, Luiz Augusto von Dentz wrote:
> Hi Marcin,
> 
> On Fri, Sep 22, 2017 at 10:52 AM, Marcin Kraglak
> <marcin.kraglak@tieto.com> wrote:
> > Hi Luiz Eramoto,
> > 
> > On 22 September 2017 at 03:42, Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > > Hi Eramoto, Marcin,
> > > 
> > > On Fri, Sep 22, 2017 at 9:27 AM, ERAMOTO Masaya
> > > <eramoto.masaya@jp.fujitsu.com> wrote:
> > > > Hi Marcin,
> > > > 
> > > > > +#ifdef HAVE_CONFIG_H
> > > > > +#include <config.h>
> > > > > +#endif
> > > > > +
> > > > > +#include <stdio.h>
> > > > > +#include "src/shared/util.h"
> > > > > +#include "src/shared/queue.h"
> > > > > +#include "src/shared/bt_shell.h"
> > > > > +
> > > > > +#define CMD_LENGTH   48
> > > > > +
> > > > > +static struct {
> > > > > +     const struct bt_shell_menu_entry *current;
> > > > > +} bt_shell_data;
> > > > > +
> > > > > +bool bt_shell_init(const struct bt_shell_menu_entry *menu)
> > > > > +{
> > > > > +     if (bt_shell_data.current || !menu)
> > > > > +             return false;
> > > > > +
> > > > > +     bt_shell_data.current = menu;
> > > > > +
> > > > > +     return true;
> > > > > +}
> > > > > +
> > > > > +void bt_shell_cleanup(void)
> > > > > +{
> > > > > +     bt_shell_data.current = NULL;
> > > > > +}
> > > > > +
> > > > > +void bt_shell_process(const char *cmd, const char *arg)
> > > > > +{
> > > > > +     const struct bt_shell_menu_entry *entry;
> > > > > +
> > > > > +     if (!bt_shell_data.current || !cmd)
> > > > > +             return;
> > > > > +
> > > > > +     for (entry = bt_shell_data.current; entry->cmd;
> > > > > entry++) {
> > > > > +             if (strcmp(cmd, entry->cmd))
> > > > > +                     continue;
> > > > > +
> > > > > +             if (entry->func) {
> > > > > +                     entry->func(arg);
> > > > > +                     return;
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     if (strcmp(cmd, "help")) {
> > > > > +             printf("Invalid command\n");
> > > > > +             return;
> > > > > +     }
> > > > > +
> > > > > +     bt_shell_print_menu();
> > > > > +}
> > > > > +
> > > > > +void bt_shell_print_menu(void)
> > > > > +{
> > > > > +     const struct bt_shell_menu_entry *entry;
> > > > > +
> > > > > +     if (!bt_shell_data.current)
> > > > > +             return;
> > > > > +
> > > > > +     printf("Available commands:\n");
> > > > > +     for (entry = bt_shell_data.current; entry->cmd;
> > > > > entry++) {
> > > > > +             printf("  %s %-*s %s\n", entry->cmd,
> > > > > +                                     (int)(CMD_LENGTH -
> > > > > strlen(entry->cmd)),
> > > > > +                                     entry->arg ? : "",
> > > > > entry->desc ? : "");
> > > > 
> > > > 
> > > > I think that it is better to
> > > >   - add some white-spaces
> > > > or
> > > >   - add a new line
> > > > between the argument string and the description string, because
> > > > it is a little
> > > > difficult for the help of register-characteristic to read as
> > > > below:
> > > > 
> > > >   [bluetooth]# help
> > > >   Available commands:
> > > >   ...
> > > >     unregister-service
> > > > <UUID/object>                  Unregister application service
> > > >     register-characteristic <UUID> <Flags=read,write,notify...>
> > > > Register application characteristic
> > > >     unregister-characteristic
> > > > <UUID/object>           Unregister application characteristic
> > > >     register-descriptor <UUID> <Flags=read,write...>  Register
> > > > application descriptor
> > > 
> > > It should probably have the same formatting as bluetoothctl:
> > > 
> > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/ma
> > > in.c#n2677
> > > 
> > > --
> > > Luiz Augusto von Dentz
> > 
> > Yes, I'll correct it
> 
> Are you still working on this?
> 
No, unfortunatelly I don't have enough time to finish it.

BR
Marcin

Re: [PATCH 1/4] shared/bt_shell: Add initial implementation

[Luiz Augusto von Dentz]

Hi Marcin,

On Mon, Nov 6, 2017 at 3:24 PM, Marcin Kraglak <marcin.kraglak@tieto.com> wrote:
> Hi Luiz,
>
> On Mon, 2017-10-23 at 14:42 +0300, Luiz Augusto von Dentz wrote:
>> Hi Marcin,
>>
>> On Fri, Sep 22, 2017 at 10:52 AM, Marcin Kraglak
>> <marcin.kraglak@tieto.com> wrote:
>> > Hi Luiz Eramoto,
>> >
>> > On 22 September 2017 at 03:42, Luiz Augusto von Dentz
>> > <luiz.dentz@gmail.com> wrote:
>> > > Hi Eramoto, Marcin,
>> > >
>> > > On Fri, Sep 22, 2017 at 9:27 AM, ERAMOTO Masaya
>> > > <eramoto.masaya@jp.fujitsu.com> wrote:
>> > > > Hi Marcin,
>> > > >
>> > > > > +#ifdef HAVE_CONFIG_H
>> > > > > +#include <config.h>
>> > > > > +#endif
>> > > > > +
>> > > > > +#include <stdio.h>
>> > > > > +#include "src/shared/util.h"
>> > > > > +#include "src/shared/queue.h"
>> > > > > +#include "src/shared/bt_shell.h"
>> > > > > +
>> > > > > +#define CMD_LENGTH   48
>> > > > > +
>> > > > > +static struct {
>> > > > > +     const struct bt_shell_menu_entry *current;
>> > > > > +} bt_shell_data;
>> > > > > +
>> > > > > +bool bt_shell_init(const struct bt_shell_menu_entry *menu)
>> > > > > +{
>> > > > > +     if (bt_shell_data.current || !menu)
>> > > > > +             return false;
>> > > > > +
>> > > > > +     bt_shell_data.current = menu;
>> > > > > +
>> > > > > +     return true;
>> > > > > +}
>> > > > > +
>> > > > > +void bt_shell_cleanup(void)
>> > > > > +{
>> > > > > +     bt_shell_data.current = NULL;
>> > > > > +}
>> > > > > +
>> > > > > +void bt_shell_process(const char *cmd, const char *arg)
>> > > > > +{
>> > > > > +     const struct bt_shell_menu_entry *entry;
>> > > > > +
>> > > > > +     if (!bt_shell_data.current || !cmd)
>> > > > > +             return;
>> > > > > +
>> > > > > +     for (entry = bt_shell_data.current; entry->cmd;
>> > > > > entry++) {
>> > > > > +             if (strcmp(cmd, entry->cmd))
>> > > > > +                     continue;
>> > > > > +
>> > > > > +             if (entry->func) {
>> > > > > +                     entry->func(arg);
>> > > > > +                     return;
>> > > > > +             }
>> > > > > +     }
>> > > > > +
>> > > > > +     if (strcmp(cmd, "help")) {
>> > > > > +             printf("Invalid command\n");
>> > > > > +             return;
>> > > > > +     }
>> > > > > +
>> > > > > +     bt_shell_print_menu();
>> > > > > +}
>> > > > > +
>> > > > > +void bt_shell_print_menu(void)
>> > > > > +{
>> > > > > +     const struct bt_shell_menu_entry *entry;
>> > > > > +
>> > > > > +     if (!bt_shell_data.current)
>> > > > > +             return;
>> > > > > +
>> > > > > +     printf("Available commands:\n");
>> > > > > +     for (entry = bt_shell_data.current; entry->cmd;
>> > > > > entry++) {
>> > > > > +             printf("  %s %-*s %s\n", entry->cmd,
>> > > > > +                                     (int)(CMD_LENGTH -
>> > > > > strlen(entry->cmd)),
>> > > > > +                                     entry->arg ? : "",
>> > > > > entry->desc ? : "");
>> > > >
>> > > >
>> > > > I think that it is better to
>> > > >   - add some white-spaces
>> > > > or
>> > > >   - add a new line
>> > > > between the argument string and the description string, because
>> > > > it is a little
>> > > > difficult for the help of register-characteristic to read as
>> > > > below:
>> > > >
>> > > >   [bluetooth]# help
>> > > >   Available commands:
>> > > >   ...
>> > > >     unregister-service
>> > > > <UUID/object>                  Unregister application service
>> > > >     register-characteristic <UUID> <Flags=read,write,notify...>
>> > > > Register application characteristic
>> > > >     unregister-characteristic
>> > > > <UUID/object>           Unregister application characteristic
>> > > >     register-descriptor <UUID> <Flags=read,write...>  Register
>> > > > application descriptor
>> > >
>> > > It should probably have the same formatting as bluetoothctl:
>> > >
>> > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/ma
>> > > in.c#n2677
>> > >
>> > > --
>> > > Luiz Augusto von Dentz
>> >
>> > Yes, I'll correct it
>>
>> Are you still working on this?
>>
> No, unfortunatelly I don't have enough time to finish it.

Ok, so I will start working on it myself.

-- 
Luiz Augusto von Dentz

Re: [PATCH 1/4] shared/bt_shell: Add initial implementation

[Marcel Holtmann]

Hi Luiz,

>>>>>>> +#ifdef HAVE_CONFIG_H
>>>>>>> +#include <config.h>
>>>>>>> +#endif
>>>>>>> +
>>>>>>> +#include <stdio.h>
>>>>>>> +#include "src/shared/util.h"
>>>>>>> +#include "src/shared/queue.h"
>>>>>>> +#include "src/shared/bt_shell.h"
>>>>>>> +
>>>>>>> +#define CMD_LENGTH   48
>>>>>>> +
>>>>>>> +static struct {
>>>>>>> +     const struct bt_shell_menu_entry *current;
>>>>>>> +} bt_shell_data;
>>>>>>> +
>>>>>>> +bool bt_shell_init(const struct bt_shell_menu_entry *menu)
>>>>>>> +{
>>>>>>> +     if (bt_shell_data.current || !menu)
>>>>>>> +             return false;
>>>>>>> +
>>>>>>> +     bt_shell_data.current = menu;
>>>>>>> +
>>>>>>> +     return true;
>>>>>>> +}
>>>>>>> +
>>>>>>> +void bt_shell_cleanup(void)
>>>>>>> +{
>>>>>>> +     bt_shell_data.current = NULL;
>>>>>>> +}
>>>>>>> +
>>>>>>> +void bt_shell_process(const char *cmd, const char *arg)
>>>>>>> +{
>>>>>>> +     const struct bt_shell_menu_entry *entry;
>>>>>>> +
>>>>>>> +     if (!bt_shell_data.current || !cmd)
>>>>>>> +             return;
>>>>>>> +
>>>>>>> +     for (entry = bt_shell_data.current; entry->cmd;
>>>>>>> entry++) {
>>>>>>> +             if (strcmp(cmd, entry->cmd))
>>>>>>> +                     continue;
>>>>>>> +
>>>>>>> +             if (entry->func) {
>>>>>>> +                     entry->func(arg);
>>>>>>> +                     return;
>>>>>>> +             }
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     if (strcmp(cmd, "help")) {
>>>>>>> +             printf("Invalid command\n");
>>>>>>> +             return;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     bt_shell_print_menu();
>>>>>>> +}
>>>>>>> +
>>>>>>> +void bt_shell_print_menu(void)
>>>>>>> +{
>>>>>>> +     const struct bt_shell_menu_entry *entry;
>>>>>>> +
>>>>>>> +     if (!bt_shell_data.current)
>>>>>>> +             return;
>>>>>>> +
>>>>>>> +     printf("Available commands:\n");
>>>>>>> +     for (entry = bt_shell_data.current; entry->cmd;
>>>>>>> entry++) {
>>>>>>> +             printf("  %s %-*s %s\n", entry->cmd,
>>>>>>> +                                     (int)(CMD_LENGTH -
>>>>>>> strlen(entry->cmd)),
>>>>>>> +                                     entry->arg ? : "",
>>>>>>> entry->desc ? : "");
>>>>>> 
>>>>>> 
>>>>>> I think that it is better to
>>>>>>  - add some white-spaces
>>>>>> or
>>>>>>  - add a new line
>>>>>> between the argument string and the description string, because
>>>>>> it is a little
>>>>>> difficult for the help of register-characteristic to read as
>>>>>> below:
>>>>>> 
>>>>>>  [bluetooth]# help
>>>>>>  Available commands:
>>>>>>  ...
>>>>>>    unregister-service
>>>>>> <UUID/object>                  Unregister application service
>>>>>>    register-characteristic <UUID> <Flags=read,write,notify...>
>>>>>> Register application characteristic
>>>>>>    unregister-characteristic
>>>>>> <UUID/object>           Unregister application characteristic
>>>>>>    register-descriptor <UUID> <Flags=read,write...>  Register
>>>>>> application descriptor
>>>>> 
>>>>> It should probably have the same formatting as bluetoothctl:
>>>>> 
>>>>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/ma
>>>>> in.c#n2677
>>>>> 
>>>>> --
>>>>> Luiz Augusto von Dentz
>>>> 
>>>> Yes, I'll correct it
>>> 
>>> Are you still working on this?
>>> 
>> No, unfortunatelly I don't have enough time to finish it.
> 
> Ok, so I will start working on it myself.

just a side note, lets do src/shared/shell.[ch] and not start with bt_ prefixes in file names.

Regards

Marcel

Re: [PATCH 1/4] shared/bt_shell: Add initial implementation

[Luiz Augusto von Dentz]

Hi Marcel,

On Mon, Nov 6, 2017 at 3:52 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luiz,
>
>>>>>>>> +#ifdef HAVE_CONFIG_H
>>>>>>>> +#include <config.h>
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>> +#include <stdio.h>
>>>>>>>> +#include "src/shared/util.h"
>>>>>>>> +#include "src/shared/queue.h"
>>>>>>>> +#include "src/shared/bt_shell.h"
>>>>>>>> +
>>>>>>>> +#define CMD_LENGTH   48
>>>>>>>> +
>>>>>>>> +static struct {
>>>>>>>> +     const struct bt_shell_menu_entry *current;
>>>>>>>> +} bt_shell_data;
>>>>>>>> +
>>>>>>>> +bool bt_shell_init(const struct bt_shell_menu_entry *menu)
>>>>>>>> +{
>>>>>>>> +     if (bt_shell_data.current || !menu)
>>>>>>>> +             return false;
>>>>>>>> +
>>>>>>>> +     bt_shell_data.current = menu;
>>>>>>>> +
>>>>>>>> +     return true;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void bt_shell_cleanup(void)
>>>>>>>> +{
>>>>>>>> +     bt_shell_data.current = NULL;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void bt_shell_process(const char *cmd, const char *arg)
>>>>>>>> +{
>>>>>>>> +     const struct bt_shell_menu_entry *entry;
>>>>>>>> +
>>>>>>>> +     if (!bt_shell_data.current || !cmd)
>>>>>>>> +             return;
>>>>>>>> +
>>>>>>>> +     for (entry = bt_shell_data.current; entry->cmd;
>>>>>>>> entry++) {
>>>>>>>> +             if (strcmp(cmd, entry->cmd))
>>>>>>>> +                     continue;
>>>>>>>> +
>>>>>>>> +             if (entry->func) {
>>>>>>>> +                     entry->func(arg);
>>>>>>>> +                     return;
>>>>>>>> +             }
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     if (strcmp(cmd, "help")) {
>>>>>>>> +             printf("Invalid command\n");
>>>>>>>> +             return;
>>>>>>>> +     }
>>>>>>>> +
>>>>>>>> +     bt_shell_print_menu();
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void bt_shell_print_menu(void)
>>>>>>>> +{
>>>>>>>> +     const struct bt_shell_menu_entry *entry;
>>>>>>>> +
>>>>>>>> +     if (!bt_shell_data.current)
>>>>>>>> +             return;
>>>>>>>> +
>>>>>>>> +     printf("Available commands:\n");
>>>>>>>> +     for (entry = bt_shell_data.current; entry->cmd;
>>>>>>>> entry++) {
>>>>>>>> +             printf("  %s %-*s %s\n", entry->cmd,
>>>>>>>> +                                     (int)(CMD_LENGTH -
>>>>>>>> strlen(entry->cmd)),
>>>>>>>> +                                     entry->arg ? : "",
>>>>>>>> entry->desc ? : "");
>>>>>>>
>>>>>>>
>>>>>>> I think that it is better to
>>>>>>>  - add some white-spaces
>>>>>>> or
>>>>>>>  - add a new line
>>>>>>> between the argument string and the description string, because
>>>>>>> it is a little
>>>>>>> difficult for the help of register-characteristic to read as
>>>>>>> below:
>>>>>>>
>>>>>>>  [bluetooth]# help
>>>>>>>  Available commands:
>>>>>>>  ...
>>>>>>>    unregister-service
>>>>>>> <UUID/object>                  Unregister application service
>>>>>>>    register-characteristic <UUID> <Flags=read,write,notify...>
>>>>>>> Register application characteristic
>>>>>>>    unregister-characteristic
>>>>>>> <UUID/object>           Unregister application characteristic
>>>>>>>    register-descriptor <UUID> <Flags=read,write...>  Register
>>>>>>> application descriptor
>>>>>>
>>>>>> It should probably have the same formatting as bluetoothctl:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/client/ma
>>>>>> in.c#n2677
>>>>>>
>>>>>> --
>>>>>> Luiz Augusto von Dentz
>>>>>
>>>>> Yes, I'll correct it
>>>>
>>>> Are you still working on this?
>>>>
>>> No, unfortunatelly I don't have enough time to finish it.
>>
>> Ok, so I will start working on it myself.
>
> just a side note, lets do src/shared/shell.[ch] and not start with bt_ prefixes in file names.

Yep, Ive just did that. Ive also started replacing GChannel usage to
struct io, etc, and moved the whole rl_handler into the shell so it
will automatically setup the readline internals so in the future all
we have to do to replace readline is to rework the internals of
bt_shell.

> Regards
>
> Marcel
>



-- 
Luiz Augusto von Dentz