Re: [PATCH] Add basic command parsing

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Zhenhua,

> ---
>  gatchat/gatserver.c |  110
>  ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 
109
>  insertions(+), 1 deletions(-)
> 
> diff --git a/gatchat/gatserver.c b/gatchat/gatserver.c
> index 6579b38..c207bd8 100644
> --- a/gatchat/gatserver.c
> +++ b/gatchat/gatserver.c
> @@ -308,9 +308,117 @@ next:
>  	return i + 1;
>  }
> 
> +static gboolean get_basic_prefix(const char *buf, char *prefix)
> +{
> +	char c = *buf;
> +
> +	if (!g_ascii_isalpha(c) && c != '&')
> +		return FALSE;
> +
> +	if (g_ascii_isalpha(c)) {
> +		c = g_ascii_toupper(c);
> +		if (c == 'S') {
> +			int i = 0;
> +
> +			prefix[0] = 'S';
> +
> +			/* V.250 5.3.2 'S' command follows with
> +			 * a parameter number.
> +			 */
> +			while (g_ascii_isdigit(buf[++i]))
> +				prefix[i] = buf[i];

Is there some limit on the number?  Can we limit it to max int or something?

> +
> +			prefix[i] = '\0';
> +		} else {
> +			prefix[0] = c;
> +			prefix[1] = '\0';
> +		}

Use a return here or an else if clause, no sense checking the if twice.

> +	}
> +
> +	if (c == '&') {
> +		prefix[0] = '&';
> +		prefix[1] = g_ascii_toupper(buf[1]);
> +		prefix[2] = '\0';
> +	}
> +
> +	return TRUE;
> +}
> +
>  static unsigned int parse_basic_command(GAtServer *server, char *buf)
>  {
> -	return 0;
> +	char *command = NULL;

Why do you initialize this?  Doesn't seem to be a point.

> +	char prefix[3];
> +	unsigned int i;
> +	GAtServerRequestType type;
> +	gboolean seen_equals = FALSE;
> +
> +	if (!get_basic_prefix(buf, prefix))
> +		return 0;
> +
> +	i = strlen(prefix);
> +
> +	if (*prefix == 'D') {
> +		type = G_AT_SERVER_REQUEST_TYPE_SET;
> +
> +		/* All following characters are the part of the call */
> +		while (buf[i] != '\0')
> +			i += 1;

Quoting V.250:
All characters appearing on the same command line after the "D" are considered 
part of the call addressing information to be signalled to the network, or 
modifiers used to control the signalling process (collectively known as a "dial 
string"), up to a semicolon character (IA5 3/11) or the end of the command 
line.

So this logic seems incorrect.

> +
> +		goto done;
> +	}
> +
> +	if (buf[i] == '\0' || buf[i] == ';') {
> +		type = G_AT_SERVER_REQUEST_TYPE_COMMAND_ONLY;
> +		goto done;
> +	}
> +
> +	/* Additional commands may follow a command without any character
> +	 * required for separation.
> +	 */
> +	if (is_basic_command_prefix(&buf[i])) {
> +		type = G_AT_SERVER_REQUEST_TYPE_COMMAND_ONLY;
> +		goto done;
> +	}
> +
> +	/* Match '?', '=',  '=?' and '=xxx' */
> +	if (buf[i] == '=') {
> +		seen_equals = TRUE;
> +		i += 1;
> +	}
> +
> +	if (buf[i] == '?') {
> +		i += 1;
> +
> +		if (seen_equals)
> +			type = G_AT_SERVER_REQUEST_TYPE_SUPPORT;
> +		else
> +			type = G_AT_SERVER_REQUEST_TYPE_QUERY;
> +	} else {
> +		/* V.250 5.3.1 The subparameter (if any) are all digits */
> +		while (g_ascii_isdigit(buf[i]))
> +			i++;
> +
> +		type = G_AT_SERVER_REQUEST_TYPE_SET;
> +	}
> +
> +done:
> +	command = g_strndup(buf, i);
> +
> +	at_command_notify(server, command, prefix, type);
> +
> +	g_free(command);
> +
> +	/* Commands like ATA, ATD, ATZ cause the remainder line
> +	 * to be ignored.
> +	 */
> +	if (*prefix == 'A' || *prefix == 'D' || *prefix == 'Z')
> +		return 0;

Returning 0 means this command was not properly formatted.  Not something you 
want.  Also, as we saw before this is true of ATA and possibly ATZ, but not 
ATD.

> +
> +	/* Consumed the seperator ';' */
> +	if (buf[i] == ';')
> +		i += 1;
> +
> +	return i;
>  }
> 
>  static void server_parse_line(GAtServer *server, char *line)
> 

Regards,
-Denis