Re: [PATCH v2] Skip unsolicied CREG/CGREG correctly while checking GPRS attach status

[Denis Kenzior <denkenz@gmail.com>]

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

On 12/03/2010 09:20 AM, Tonny Tzeng wrote:
> This patch is to skip unsolicited CGREG while checking the GPRS attach
> status, especially for Huawei modem which sends lac and ci strings in
> unquoted format, and casues at_util_parse_reg() return lac value as
> status.
> 
> ---
>  drivers/atmodem/atutil.c |   12 ++++++++----
>  gatchat/gatresult.c      |    5 +++++
>  gatchat/gatresult.h      |    1 +

We like to see patches split on a top level directory boundary.  Please
see the 'Submitting patches' section in the HACKING document.

>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
> index b6f0d92..c190aa2 100644
> --- a/drivers/atmodem/atutil.c
> +++ b/drivers/atmodem/atutil.c
> @@ -243,13 +243,11 @@ gboolean at_util_parse_reg(GAtResult *result,
> const char *prefix,
>  		if (g_at_result_iter_next_number(&iter, &s) == FALSE)
>  			continue;
>  
> -		/* Some firmware will report bogus lac/ci when unregistered */
> -		if (s != 1 && s != 5)
> -			goto out;
> -

Skipping this check is still a bad idea.

>  		switch (vendor) {
>  		case OFONO_VENDOR_HUAWEI:
>  		case OFONO_VENDOR_NOVATEL:
> +			if (g_at_result_iter_eol(&iter))
> +				continue;

Coding style item M1.

>  			r = g_at_result_iter_next_unquoted_string(&iter, &str);
>  
>  			if (r == TRUE)
> @@ -257,6 +255,8 @@ gboolean at_util_parse_reg(GAtResult *result, const
> char *prefix,
>  			else
>  				goto out;
>  
> +			if (g_at_result_iter_eol(&iter))
> +				continue;

Coding style item M1.

>  			r = g_at_result_iter_next_unquoted_string(&iter, &str);
>  
>  			if (r == TRUE)
> @@ -266,11 +266,15 @@ gboolean at_util_parse_reg(GAtResult *result,
> const char *prefix,
>  
>  			break;
>  		default:
> +			if (g_at_result_iter_eol(&iter))
> +				continue;

Coding style item M1.  And continuing is not the right thing to do here

>  			if (g_at_result_iter_next_string(&iter, &str) == TRUE)
>  				l = strtol(str, NULL, 16);
>  			else
>  				goto out;
>  
> +			if (g_at_result_iter_eol(&iter))
> +				continue;

Ditto

>  			if (g_at_result_iter_next_string(&iter, &str) == TRUE)
>  				c = strtol(str, NULL, 16);
>  			else
> diff --git a/gatchat/gatresult.c b/gatchat/gatresult.c
> index 8a6dfae..c919305 100644
> --- a/gatchat/gatresult.c
> +++ b/gatchat/gatresult.c
> @@ -39,6 +39,11 @@ void g_at_result_iter_init(GAtResultIter *iter,
> GAtResult *result)
>  	iter->line_pos = 0;
>  }
>  
> +gboolean g_at_result_iter_eol(GAtResultIter *iter)
> +{
> +	return iter->line_pos >= strlen(iter->l->data);
> +}
> +

And I pretty much don't see the need for this.  It doesn't really help
you here.

>  gboolean g_at_result_iter_next(GAtResultIter *iter, const char *prefix)
>  {
>  	char *line;
> diff --git a/gatchat/gatresult.h b/gatchat/gatresult.h
> index a74741f..33c0b2c 100644
> --- a/gatchat/gatresult.h
> +++ b/gatchat/gatresult.h
> @@ -46,6 +46,7 @@ struct _GAtResultIter {
>  typedef struct _GAtResultIter GAtResultIter;
>  
>  void g_at_result_iter_init(GAtResultIter *iter, GAtResult *result);
> +gboolean g_at_result_iter_eol(GAtResultIter *iter);
>  
>  gboolean g_at_result_iter_next(GAtResultIter *iter, const char
> *prefix);
>  gboolean g_at_result_iter_open_list(GAtResultIter *iter);

Anyhow, can you please try the attached patch and see if it solves your
problems.

Regards,
-Denis

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-atutil-Fix-parsing-of-un-quoted-CREG-CGREG.patch --]
[-- Type: text/x-patch, Size: 1462 bytes --]

>From f5473c398a2e29a98c74ad4c98a427564e25d9d0 Mon Sep 17 00:00:00 2001
From: Denis Kenzior <denkenz@gmail.com>
Date: Mon, 6 Dec 2010 21:36:54 -0600
Subject: [PATCH] atutil: Fix parsing of un-quoted CREG / CGREG

On broken hardware like the Huawei, it is possible to receive both an
unsolicited and a solicited version of the CREG / CGREG within within
the same response set.  Skipping of the unsolicited version was not
handled correctly.  This attempts to fix this issue.
---
 drivers/atmodem/atutil.c |   19 +++++++++++++++++--
 1 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/atmodem/atutil.c b/drivers/atmodem/atutil.c
index b6f0d92..427b098 100644
--- a/drivers/atmodem/atutil.c
+++ b/drivers/atmodem/atutil.c
@@ -240,8 +240,23 @@ gboolean at_util_parse_reg(GAtResult *result, const char *prefix,
 		g_at_result_iter_next_number(&iter, &m);
 
 		/* Sometimes we get an unsolicited CREG/CGREG here, skip it */
-		if (g_at_result_iter_next_number(&iter, &s) == FALSE)
-			continue;
+		switch (vendor) {
+		case OFONO_VENDOR_HUAWEI:
+		case OFONO_VENDOR_NOVATEL:
+			r = g_at_result_iter_next_unquoted_string(&iter, &str);
+
+			if (r == FALSE || strlen(str) != 1)
+				continue;
+
+			s = strtol(str, NULL, 10);
+
+			break;
+		default:
+			if (g_at_result_iter_next_number(&iter, &s) == FALSE)
+				continue;
+
+			break;
+		}
 
 		/* Some firmware will report bogus lac/ci when unregistered */
 		if (s != 1 && s != 5)
-- 
1.7.0.4