Re: [PATCH 02/11] Add framework of server parser

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2727 bytes --]

Hi Zhenhua,

> a. The parser fetch and parse one command per loop. The prefix is
> the command prefix without parameter. For example, the prefix of
> "AT+CLIP=1" is "+CLIP".
> 
> b. Search registered notification node in command_list. Invoke the
> callback if found.
> 
> c. Termiate the execution if the result is an error. Otherwise,
> parse next command.
> ---
>  gatchat/gatserver.c |   95
>  +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 
95
>  insertions(+), 0 deletions(-)
> 
> diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
> index 46fa423..fb82ad4 100644
> --- a/gatchat/gatserver.c
> +++ b/gatchat/gatserver.c
> @@ -173,8 +173,103 @@ static void g_at_server_send_final(GAtServer *server,
>  GAtServerResult result) send_common(server, buf, MIN(len, sizeof(buf)-1));
>  }
> 
> +static inline gboolean is_extended_command_prefix(const char c)
> +{
> +	switch (c) {
> +	case '+':
> +	case '*':
> +	case '!':
> +	case '%':
> +		return TRUE;
> +	default:
> +		return FALSE;
> +	}
> +}

So I don't see the point of a patch that removes the entire function, then 
puts the same function back in with the same name.  If you want to rename it, 
send a patch to do so.

>  static void server_parse_line(GAtServer *server, char *line)
>  {
> +	GAtServerResult res = G_AT_SERVER_RESULT_ERROR;
> +	char *buf = line;
> +	char *command = NULL;
> +	char prefix[20];
> +
> +	while (buf) {

So this part is wrong, buf will never be NULL.  I suggest you preserve the 
logic of the current implementation here.

> +		char c = *buf;
> +
> +		/* skip semicolon */
> +		if (c == ';')
> +			c = *(++buf);
> +
> +		if (c == '\0') {
> +			res = G_AT_SERVER_RESULT_OK;
> +			break;
> +		}
> +
> +		command = parse_next_command(server, buf, prefix);
> +		if (!command) {
> +			res = G_AT_SERVER_RESULT_ERROR;
> +			break;
> +		}

Why is a char * being allocated by parse_next_command?  What is the purpose?  
I suggest you get rid of this and perform notifications there.  It is too hard 
to keep track of who should free what and where otherwise.

> +
> +		res = at_command_notify(server, command, prefix);

I've mentioned this before, I don't like returning a result here.  We should 
simply store the last result given to g_at_server_send_final or 
g_at_server_send_ext_final (e.g. do not send it out right away).  If it is not 
OK then quit command processing and send the final result.

> +		if (res != G_AT_SERVER_RESULT_OK) {
> +			g_free(command);
> +			break;
> +		}
> +
> +		buf += strlen(command);
> +
> +		g_free(command);
> +	}
> +
> +	g_at_server_send_final(server, res);
> +
>  	g_free(line);
>  }
>