Re: [PATCH 2/6 v2] Add an interactive mode to gatttool

[Johan Hedberg <johan.hedberg@gmail.com>]

On Mon, Jan 31, 2011, Sheldon Demario wrote:
> +static gboolean cmd_connect(gpointer cmd)
> +{
> +	const char **c = (const char **) cmd;
> +	if (conn_state != STATE_DISCONNECTED)

Missing line after the variable declaration and unnecessary explicit
type-cast (no need for void pointers). However why does this function
take a gpointer to begin with? I'd imagine the most convenient thing to
give the command handlers either a char* with the entire command line or
a pre-parsed argc + argv with the help of g_shell_parse_argv.

> +	if (c[1] != NULL) {

Now that's just ugly. Much better if you use g_shell_parse_argv and then
you can use the usual getopt to separate switches from mandatory
parameters, etc.

> +static struct {
> +	char *cmd;

I suppose this should be const char *

> +	gboolean (*func)(gpointer cmd);

As I mentioned, instead of gpointer do char * or then (what I'd prefer)
simply argc + argv.

> +	char *param;

Again, const

> +	char *desc;

Same here.

> +} commands[] = {
> +	{ "connect",	cmd_connect,	"<bdaddr>",	"Connect"},
> +	{ "disconnect",	cmd_disconnect,	NULL,		"Disconnect"},
> +	{ "characteristics",	characteristics,	NULL,
> +						"Characteristcs Discovery"},
> +	{ NULL, NULL, NULL, NULL}
> +};

Where's the "help" command? That's the first one I'd expect to be
implemented. Also, ctrl-d and "exit" should work from the start as ways
of exiting the program.

> +static void parse_line(char *line_read)
> +{
> +	char **command;
> +	int j;

Why j and not i?

> +static int do_interactive(void)

Didn't I tell you to do all the interactive stuff away from the main
gatttool.c file? There should only be the parsing of command line
parameters and a call to the interactive .c file in the case of
interactive mode.

Johan