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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html