[PATCH v2 1/4] shared/shell: Detect if the arguments are properly set

[PATCH v2 1/4] shared/shell: Detect if the arguments are properly set

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Parse arguments to detect if user has passed a valid number of
arguments.
---
v2: Just print the usage of the commands without auto-complete generator so it
does add the arguments into the input.

 src/shared/shell.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 92 insertions(+), 6 deletions(-)

diff --git a/src/shared/shell.c b/src/shared/shell.c
index c271a53ee..73aedaf79 100644
--- a/src/shared/shell.c
+++ b/src/shared/shell.c
@@ -207,6 +207,95 @@ static void shell_print_menu(void)
 	}
 }
 
+static int parse_args(char *arg, wordexp_t *w, char *del, int flags)
+{
+	char *str;
+
+	str = g_strdelimit(arg, del, '"');
+
+	if (wordexp(str, w, flags)) {
+		g_free(str);
+		return -EINVAL;
+	}
+
+	/* If argument ends with ,,, set we_offs bypass strict checks */
+	if (w->we_wordc && g_str_has_suffix(w->we_wordv[w->we_wordc -1], "..."))
+		w->we_offs = 1;
+
+	g_free(str);
+
+	return 0;
+}
+
+static int cmd_exec(const struct bt_shell_menu_entry *entry,
+					int argc, char *argv[])
+{
+	wordexp_t w;
+	size_t len;
+	char *man, *opt;
+	int flags = WRDE_NOCMD;
+
+	if (!entry->arg || entry->arg[0] == '\0') {
+		if (argc) {
+			print_text(COLOR_HIGHLIGHT, "Too many arguments");
+			return -EINVAL;
+		}
+		goto exec;
+	}
+
+	/* Find last mandatory arguments */
+	man = strrchr(entry->arg, '>');
+	if (!man) {
+		opt = strdup(entry->arg);
+		goto optional;
+	}
+
+	len = man - entry->arg;
+	man = strndup(entry->arg, len + 1);
+
+	if (parse_args(man, &w, "<>", flags) < 0) {
+		print_text(COLOR_HIGHLIGHT,
+				"Unable to parse mandatory command arguments");
+		return -EINVAL;
+	}
+
+	/* Check if there are enough arguments */
+	if ((unsigned) argc < w.we_wordc && !w.we_offs) {
+		print_text(COLOR_HIGHLIGHT, "Missing %s argument",
+						w.we_wordv[argc]);
+		goto fail;
+	}
+
+	flags |= WRDE_APPEND;
+	opt = strdup(entry->arg + len + 1);
+
+optional:
+	if (parse_args(opt, &w, "<>", flags) < 0) {
+		print_text(COLOR_HIGHLIGHT,
+				"Unable to parse mandatory command arguments");
+		return -EINVAL;
+	}
+
+	/* Check if there are too many arguments */
+	if ((unsigned) argc > w.we_wordc && !w.we_offs) {
+		print_text(COLOR_HIGHLIGHT, "Too many arguments: %d > %zu",
+					argc, w.we_wordc);
+		goto fail;
+	}
+
+	wordfree(&w);
+
+exec:
+	if (entry->func)
+		entry->func(argc, argv);
+
+	return 0;
+
+fail:
+	wordfree(&w);
+	return -EINVAL;
+}
+
 static int menu_exec(const struct bt_shell_menu_entry *entry,
 					int argc, char *argv[])
 {
@@ -222,10 +311,7 @@ static int menu_exec(const struct bt_shell_menu_entry *entry,
 		if (data.menu == data.main && !strcmp(entry->cmd, "back"))
 			continue;
 
-		if (entry->func) {
-			entry->func(argc - 1, ++argv);
-			return 0;
-		}
+		return cmd_exec(entry, --argc, ++argv);
 	}
 
 	return -ENOENT;
@@ -236,8 +322,8 @@ static void shell_exec(int argc, char *argv[])
 	if (!data.menu || !argv[0])
 		return;
 
-	if (menu_exec(default_menu, argc, argv) < 0) {
-		if (menu_exec(data.menu->entries, argc, argv) < 0)
+	if (menu_exec(default_menu, argc, argv) == -ENOENT) {
+		if (menu_exec(data.menu->entries, argc, argv) == -ENOENT)
 			print_text(COLOR_HIGHLIGHT, "Invalid command");
 	}
 }
-- 
2.13.6

[PATCH v2 2/4] client: Remove argument checks

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This is now done automatically by bt_shell with the use of the
arguments entered in the command table.
---
 client/main.c | 93 -----------------------------------------------------------
 1 file changed, 93 deletions(-)

diff --git a/client/main.c b/client/main.c
index a510851e3..38028c6c0 100644
--- a/client/main.c
+++ b/client/main.c
@@ -804,14 +804,6 @@ static gboolean parse_argument(int argc, char *argv[], const char **arg_table,
 {
 	const char **opt;
 
-	if (!argc || !strlen(argv[0])) {
-		if (msg)
-			bt_shell_printf("Missing on/off/%s argument\n", msg);
-		else
-			bt_shell_printf("Missing on/off argument\n");
-		return FALSE;
-	}
-
 	if (!strcmp(argv[0], "on") || !strcmp(argv[0], "yes")) {
 		*value = TRUE;
 		if (option)
@@ -889,11 +881,6 @@ static void cmd_select(int argc, char *argv[])
 {
 	struct adapter *adapter;
 
-	if (!argc || !strlen(argv[0])) {
-		bt_shell_printf("Missing controller address argument\n");
-		return;
-	}
-
 	adapter = find_ctrl_by_address(ctrl_list, argv[0]);
 	if (!adapter) {
 		bt_shell_printf("Controller %s not available\n", argv[0]);
@@ -959,11 +946,6 @@ static void cmd_system_alias(int argc, char *argv[])
 {
 	char *name;
 
-	if (!argc || !strlen(argv[0])) {
-		bt_shell_printf("Missing name argument\n");
-		return;
-	}
-
 	if (check_default_ctrl() == FALSE)
 		return;
 
@@ -1637,11 +1619,6 @@ static void cmd_remove(int argc, char *argv[])
 {
 	GDBusProxy *proxy;
 
-	if (!argc || !strlen(argv[0])) {
-		bt_shell_printf("Missing device address argument\n");
-		return;
-	}
-
 	if (check_default_ctrl() == FALSE)
 		return;
 
@@ -1688,11 +1665,6 @@ static void cmd_connect(int argc, char *argv[])
 {
 	GDBusProxy *proxy;
 
-	if (!argc || !strlen(argv[0])) {
-		bt_shell_printf("Missing device address argument\n");
-		return;
-	}
-
 	if (check_default_ctrl() == FALSE)
 		return;
 
@@ -1773,11 +1745,6 @@ static void cmd_set_alias(int argc, char *argv[])
 {
 	char *name;
 
-	if (!argc || !strlen(argv[0])) {
-		bt_shell_printf("Missing name argument\n");
-		return;
-	}
-
 	if (!default_dev) {
 		bt_shell_printf("No device connected\n");
 		return;
@@ -1797,11 +1764,6 @@ static void cmd_select_attribute(int argc, char *argv[])
 {
 	GDBusProxy *proxy;
 
-	if (!argc || !strlen(argv[0])) {
-		bt_shell_printf("Missing attribute argument\n");
-		return;
-	}
-
 	if (!default_dev) {
 		bt_shell_printf("No device connected\n");
 		return;
@@ -1889,11 +1851,6 @@ static void cmd_read(int argc, char *argv[])
 
 static void cmd_write(int argc, char *argv[])
 {
-	if (!argc || !strlen(argv[0])) {
-		bt_shell_printf("Missing data argument\n");
-		return;
-	}
-
 	if (!default_attr) {
 		bt_shell_printf("No attribute selected\n");
 		return;
@@ -1978,11 +1935,6 @@ static void cmd_register_service(int argc, char *argv[])
 	if (check_default_ctrl() == FALSE)
 		return;
 
-	if (!argc) {
-		bt_shell_printf("Missing argument\n");
-		return;
-	}
-
 	gatt_register_service(dbus_conn, default_ctrl->proxy, argc, argv);
 }
 
@@ -1991,11 +1943,6 @@ static void cmd_unregister_service(int argc, char *argv[])
 	if (check_default_ctrl() == FALSE)
 		return;
 
-	if (!argc) {
-		bt_shell_printf("Missing argument\n");
-		return;
-	}
-
 	gatt_unregister_service(dbus_conn, default_ctrl->proxy, argc, argv);
 }
 
@@ -2004,11 +1951,6 @@ static void cmd_register_characteristic(int argc, char *argv[])
 	if (check_default_ctrl() == FALSE)
 		return;
 
-	if (argc < 2) {
-		bt_shell_printf("Missing arguments\n");
-		return;
-	}
-
 	gatt_register_chrc(dbus_conn, default_ctrl->proxy, argc, argv);
 }
 
@@ -2017,11 +1959,6 @@ static void cmd_unregister_characteristic(int argc, char *argv[])
 	if (check_default_ctrl() == FALSE)
 		return;
 
-	if (argc < 1) {
-		bt_shell_printf("Missing arguments\n");
-		return;
-	}
-
 	gatt_unregister_chrc(dbus_conn, default_ctrl->proxy, argc, argv);
 }
 
@@ -2030,11 +1967,6 @@ static void cmd_register_descriptor(int argc, char *argv[])
 	if (check_default_ctrl() == FALSE)
 		return;
 
-	if (argc < 2) {
-		bt_shell_printf("Missing arguments\n");
-		return;
-	}
-
 	gatt_register_desc(dbus_conn, default_ctrl->proxy, argc, argv);
 }
 
@@ -2043,11 +1975,6 @@ static void cmd_unregister_descriptor(int argc, char *argv[])
 	if (check_default_ctrl() == FALSE)
 		return;
 
-	if (argc < 1) {
-		bt_shell_printf("Missing arguments\n");
-		return;
-	}
-
 	gatt_unregister_desc(dbus_conn, default_ctrl->proxy, argc, argv);
 }
 
@@ -2208,11 +2135,6 @@ static void cmd_set_advertise_tx_power(int argc, char *argv[])
 
 static void cmd_set_advertise_name(int argc, char *argv[])
 {
-	if (!argc || strlen(argv[0]) == 0) {
-		bt_shell_printf("Missing on/off argument\n");
-		return;
-	}
-
 	if (strcmp(argv[0], "on") == 0 || strcmp(argv[0], "yes") == 0) {
 		ad_advertise_name(dbus_conn, true);
 		return;
@@ -2231,11 +2153,6 @@ static void cmd_set_advertise_appearance(int argc, char *argv[])
 	long int value;
 	char *endptr = NULL;
 
-	if (!argc || strlen(argv[0]) == 0) {
-		bt_shell_printf("Missing value argument\n");
-		return;
-	}
-
 	if (strcmp(argv[0], "on") == 0 || strcmp(argv[0], "yes") == 0) {
 		ad_advertise_appearance(dbus_conn, true);
 		return;
@@ -2260,11 +2177,6 @@ static void cmd_set_advertise_duration(int argc, char *argv[])
 	long int value;
 	char *endptr = NULL;
 
-	if (!argc || strlen(argv[0]) == 0) {
-		bt_shell_printf("Missing value argument\n");
-		return;
-	}
-
 	value = strtol(argv[0], &endptr, 0);
 	if (!endptr || *endptr != '\0' || value > UINT16_MAX) {
 		bt_shell_printf("Invalid argument\n");
@@ -2279,11 +2191,6 @@ static void cmd_set_advertise_timeout(int argc, char *argv[])
 	long int value;
 	char *endptr = NULL;
 
-	if (!argc || strlen(argv[0]) == 0) {
-		bt_shell_printf("Missing value argument\n");
-		return;
-	}
-
 	value = strtol(argv[0], &endptr, 0);
 	if (!endptr || *endptr != '\0' || value > UINT16_MAX) {
 		bt_shell_printf("Invalid argument\n");
-- 
2.13.6

[PATCH v2 3/4] client: Fix format of arguments of set-service and set-manufacturer

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Optional arguments should contain only one set of '[' ']' otherwise
wordexp won't be able to parse it.
---
 client/main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/client/main.c b/client/main.c
index 38028c6c0..7661b33c0 100644
--- a/client/main.c
+++ b/client/main.c
@@ -2205,9 +2205,9 @@ static const struct bt_shell_menu advertise_menu = {
 	.entries = {
 	{ "set-uuids", "[uuid1 uuid2 ...]",
 			cmd_set_advertise_uuids, "Set advertise uuids" },
-	{ "set-service", "[uuid][data=[xx xx ...]", cmd_set_advertise_service,
+	{ "set-service", "[uuid] [data=xx xx ...]", cmd_set_advertise_service,
 			"Set advertise service data" },
-	{ "set-manufacturer", "[id][data=[xx xx ...]",
+	{ "set-manufacturer", "[id] [data=xx xx ...]",
 			cmd_set_advertise_manufacturer,
 			"Set advertise manufacturer data" },
 	{ "set-tx-power", "<on/off>", cmd_set_advertise_tx_power,
@@ -2254,7 +2254,7 @@ static const struct bt_shell_menu gatt_menu = {
 	{ "attribute-info", "[attribute/UUID]",  cmd_attribute_info,
 				"Select attribute", attribute_generator },
 	{ "read",         NULL,       cmd_read, "Read attribute value" },
-	{ "write",        "<data=[xx xx ...]>", cmd_write,
+	{ "write",        "<data=xx xx ...>", cmd_write,
 						"Write attribute value" },
 	{ "acquire-write", NULL, cmd_acquire_write,
 					"Acquire Write file descriptor" },
-- 
2.13.6

[PATCH v2 4/4] shared/shell: Print usage when no argument generator is set

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This prints the command and arguments if the command don't provide
a generator so one don't have to always resort to help for checking it.
---
 src/shared/shell.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/shared/shell.c b/src/shared/shell.c
index 73aedaf79..bd5cf18fc 100644
--- a/src/shared/shell.c
+++ b/src/shared/shell.c
@@ -527,8 +527,12 @@ static char **menu_completion(const struct bt_shell_menu_entry *entry,
 		if (strcmp(entry->cmd, input_cmd))
 			continue;
 
-		if (!entry->gen)
-			continue;
+		if (!entry->gen) {
+			if (text[0] == '\0')
+				bt_shell_printf("Usage: %s %s\n", entry->cmd,
+								entry->arg);
+			break;
+		}
 
 		rl_completion_display_matches_hook = entry->disp;
 		matches = rl_completion_matches(text, entry->gen);
-- 
2.13.6

Re: [PATCH v2 4/4] shared/shell: Print usage when no argument generator is set

[ERAMOTO Masaya]

Hi Luiz,

Thank you for dealing with my request.

On 11/28/2017 01:17 AM, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> This prints the command and arguments if the command don't provide
> a generator so one don't have to always resort to help for checking it.
> ---
>  src/shared/shell.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/shared/shell.c b/src/shared/shell.c
> index 73aedaf79..bd5cf18fc 100644
> --- a/src/shared/shell.c
> +++ b/src/shared/shell.c
> @@ -527,8 +527,12 @@ static char **menu_completion(const struct bt_shell_menu_entry *entry,
>  		if (strcmp(entry->cmd, input_cmd))
>  			continue;
>  
> -		if (!entry->gen)
> -			continue;
> +		if (!entry->gen) {
> +			if (text[0] == '\0')
> +				bt_shell_printf("Usage: %s %s\n", entry->cmd,
> +								entry->arg);

If entry->arg is NULL, it prints out "(null)" on my box.


Regards,
Eramoto

Re: [PATCH v2 1/4] shared/shell: Detect if the arguments are properly set

[Luiz Augusto von Dentz]

Hi,

On Mon, Nov 27, 2017 at 6:17 PM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Parse arguments to detect if user has passed a valid number of
> arguments.
> ---
> v2: Just print the usage of the commands without auto-complete generator so it
> does add the arguments into the input.
>
>  src/shared/shell.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 92 insertions(+), 6 deletions(-)
>
> diff --git a/src/shared/shell.c b/src/shared/shell.c
> index c271a53ee..73aedaf79 100644
> --- a/src/shared/shell.c
> +++ b/src/shared/shell.c
> @@ -207,6 +207,95 @@ static void shell_print_menu(void)
>         }
>  }
>
> +static int parse_args(char *arg, wordexp_t *w, char *del, int flags)
> +{
> +       char *str;
> +
> +       str = g_strdelimit(arg, del, '"');
> +
> +       if (wordexp(str, w, flags)) {
> +               g_free(str);
> +               return -EINVAL;
> +       }
> +
> +       /* If argument ends with ,,, set we_offs bypass strict checks */
> +       if (w->we_wordc && g_str_has_suffix(w->we_wordv[w->we_wordc -1], "..."))
> +               w->we_offs = 1;
> +
> +       g_free(str);
> +
> +       return 0;
> +}
> +
> +static int cmd_exec(const struct bt_shell_menu_entry *entry,
> +                                       int argc, char *argv[])
> +{
> +       wordexp_t w;
> +       size_t len;
> +       char *man, *opt;
> +       int flags = WRDE_NOCMD;
> +
> +       if (!entry->arg || entry->arg[0] == '\0') {
> +               if (argc) {
> +                       print_text(COLOR_HIGHLIGHT, "Too many arguments");
> +                       return -EINVAL;
> +               }
> +               goto exec;
> +       }
> +
> +       /* Find last mandatory arguments */
> +       man = strrchr(entry->arg, '>');
> +       if (!man) {
> +               opt = strdup(entry->arg);
> +               goto optional;
> +       }
> +
> +       len = man - entry->arg;
> +       man = strndup(entry->arg, len + 1);
> +
> +       if (parse_args(man, &w, "<>", flags) < 0) {
> +               print_text(COLOR_HIGHLIGHT,
> +                               "Unable to parse mandatory command arguments");
> +               return -EINVAL;
> +       }
> +
> +       /* Check if there are enough arguments */
> +       if ((unsigned) argc < w.we_wordc && !w.we_offs) {
> +               print_text(COLOR_HIGHLIGHT, "Missing %s argument",
> +                                               w.we_wordv[argc]);
> +               goto fail;
> +       }
> +
> +       flags |= WRDE_APPEND;
> +       opt = strdup(entry->arg + len + 1);
> +
> +optional:
> +       if (parse_args(opt, &w, "<>", flags) < 0) {
> +               print_text(COLOR_HIGHLIGHT,
> +                               "Unable to parse mandatory command arguments");
> +               return -EINVAL;
> +       }
> +
> +       /* Check if there are too many arguments */
> +       if ((unsigned) argc > w.we_wordc && !w.we_offs) {
> +               print_text(COLOR_HIGHLIGHT, "Too many arguments: %d > %zu",
> +                                       argc, w.we_wordc);
> +               goto fail;
> +       }
> +
> +       wordfree(&w);
> +
> +exec:
> +       if (entry->func)
> +               entry->func(argc, argv);
> +
> +       return 0;
> +
> +fail:
> +       wordfree(&w);
> +       return -EINVAL;
> +}
> +
>  static int menu_exec(const struct bt_shell_menu_entry *entry,
>                                         int argc, char *argv[])
>  {
> @@ -222,10 +311,7 @@ static int menu_exec(const struct bt_shell_menu_entry *entry,
>                 if (data.menu == data.main && !strcmp(entry->cmd, "back"))
>                         continue;
>
> -               if (entry->func) {
> -                       entry->func(argc - 1, ++argv);
> -                       return 0;
> -               }
> +               return cmd_exec(entry, --argc, ++argv);
>         }
>
>         return -ENOENT;
> @@ -236,8 +322,8 @@ static void shell_exec(int argc, char *argv[])
>         if (!data.menu || !argv[0])
>                 return;
>
> -       if (menu_exec(default_menu, argc, argv) < 0) {
> -               if (menu_exec(data.menu->entries, argc, argv) < 0)
> +       if (menu_exec(default_menu, argc, argv) == -ENOENT) {
> +               if (menu_exec(data.menu->entries, argc, argv) == -ENOENT)
>                         print_text(COLOR_HIGHLIGHT, "Invalid command");
>         }
>  }
> --
> 2.13.6

Applied after fixing the formatting of commands where .arg is NULL.


-- 
Luiz Augusto von Dentz