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