Re: [PATCH] atmodem: Add MBM/STE quirk for SIM record update

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Miia,

On 02/23/2011 02:09 AM, Miia Leinonen wrote:
> Fixed the <data> string to be set inside quotes for STE/MBM.
> 
> ---
> 
> Hi,
> 
> This fix has been tested with STE modem only due the lack of MBM HW.
> 
> Would it be possible that someone checks this with MBM modem? The problem is 
> likely to appear also there - therefore OFONO_VENDOR_MBM used. Could fix this 
> also to cover all modems, but that might cause some backward compatibility 
> issues.

This does seem to be required on MBM

> 
> BR,
> Miia
> 
> 
>  drivers/atmodem/sim.c |   59 ++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 46 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
> index d9c0d8d..7a0100a 100644
> --- a/drivers/atmodem/sim.c
> +++ b/drivers/atmodem/sim.c
> @@ -332,17 +332,34 @@ static void at_sim_update_record(struct ofono_sim *sim, int fileid,
>  {
>  	struct sim_data *sd = ofono_sim_get_data(sim);
>  	struct cb_data *cbd = cb_data_new(cb, data);
> -	char *buf = g_try_new(char, 36 + length * 2);
> +	char *buf;
>  	int len, ret;
>  
> -	if (buf == NULL)
> -		goto error;
> +	if (sd->vendor == OFONO_VENDOR_MBM) {
> +		buf = g_try_new(char, 36 + 2 + length * 2);
>  
> -	len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,", fileid,
> -			record, length);
> +		if (buf == NULL)
> +			goto error;
>  
> -	for (; length; length--)
> -		len += sprintf(buf + len, "%02hhX", *value++);
> +		len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,\"", fileid,
> +					record, length);
> +
> +		for (; length; length--)
> +			len += sprintf(buf + len, "%02hhX", *value++);
> +
> +		sprintf(buf + len, "\"");
> +	} else {
> +		buf = g_try_new(char, 36 + length * 2);
> +
> +		if (buf == NULL)
> +			goto error;
> +
> +		len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,", fileid,
> +				record, length);
> +
> +		for (; length; length--)
> +			len += sprintf(buf + len, "%02hhX", *value++);
> +	}
>  

Please don't duplicate code like this.  It is much better written
something like this:

int size = 36 + 2 + length * 2;

if (vendor == VENDOR_MBM)
	size += 2; /* Add quotes */

buf = g_try_new(char, size);

len = sprintf(buf, "AT+CRSM=220,%i,%i,4,%i,", fileid, record, length);

if (vendor == VENDOR_MBM)
	len += sprint(buf + len, "\"");

for (; length; length--)
	...

if (vendor == VENDOR_MBM)
	...

etc.

>  	ret = g_at_chat_send(sd->chat, buf, crsm_prefix,
>  				at_crsm_update_cb, cbd, g_free);
> @@ -364,16 +381,32 @@ static void at_sim_update_cyclic(struct ofono_sim *sim, int fileid,
>  {
>  	struct sim_data *sd = ofono_sim_get_data(sim);
>  	struct cb_data *cbd = cb_data_new(cb, data);
> -	char *buf = g_try_new(char, 36 + length * 2);
> +	char *buf;
>  	int len, ret;
>  
> -	if (buf == NULL)
> -		goto error;
> +	if (sd->vendor == OFONO_VENDOR_MBM) {
> +		buf = g_try_new(char, 36 + 2 + length * 2);
>  
> -	len = sprintf(buf, "AT+CRSM=220,%i,0,3,%i,", fileid, length);
> +		if (buf == NULL)
> +			goto error;
>  
> -	for (; length; length--)
> -		len += sprintf(buf + len, "%02hhX", *value++);
> +		len = sprintf(buf, "AT+CRSM=220,%i,0,3,%i,\"", fileid, length);
> +
> +		for (; length; length--)
> +			len += sprintf(buf + len, "%02hhX", *value++);
> +
> +		sprintf(buf + len, "\"");
> +	} else {
> +		buf = g_try_new(char, 36 + length * 2);
> +
> +		if (buf == NULL)
> +			goto error;
> +
> +		len = sprintf(buf, "AT+CRSM=220,%i,0,3,%i,", fileid, length);
> +
> +		for (; length; length--)
> +			len += sprintf(buf + len, "%02hhX", *value++);
> +	}
>  
>  	ret = g_at_chat_send(sd->chat, buf, crsm_prefix,
>  				at_crsm_update_cb, cbd, g_free);

Regards,
-Denis