Re: [PATCH 7/7] Fix: isimodem handling of call barring services.

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Pekka,

> From: Pekka Pessi <Pekka.Pessi@nokia.com>
> 
> 
> Signed-off-by: Pekka Pessi <Pekka.Pessi@nokia.com>
> ---
>  drivers/isimodem/call-barring.c |   64
>  ++++++++++++++++++++------------------ drivers/isimodem/ss.h           |  
>  28 +++++++++--------
>  2 files changed, 49 insertions(+), 43 deletions(-)
> @@ -105,7 +104,9 @@ static void isi_set(struct ofono_call_barring *barr,
>  const char *lock, DBG("lock code %s enable %d class %d password %s\n",
>  		lock, enable, cls, passwd);
> 
> -	if (!cbd || !passwd || strlen(passwd) > 4 || cls != 7)
> +	if (!cbd || !lock)
> +		goto error;
> +	if (!passwd || strlen(passwd) != 4 || strspn(passwd, "0123456789") != 4)
>  		goto error;

I suggest not doing this checking in the driver code, if the core is sending 
you something you don't expect, let us fix the core.

> 
>  	if (strcmp(lock, "AO") == 0)
> @@ -120,23 +121,23 @@ static void isi_set(struct ofono_call_barring *barr,
>  const char *lock, ss_code = SS_GSM_BARR_ALL_IN_ROAM;
>  	else if (strcmp(lock, "AB") == 0)
>  		ss_code = SS_GSM_ALL_BARRINGS;
> +	else if (enable)
> +		/* Only Deactivate supports AG (333) and AC (353) */
> +		goto error;

Same here, to my knowledge the core never actually allows this.

> -	if (!cbd || strlen(old_passwd) > 4 || strlen(new_passwd) > 4)
> +	if (!cbd)
> +		goto error;
> +	if (strlen(old_passwd) != 4 || strspn(old_passwd, "0123456789") != 4)
> +		goto error;
> +	if (strlen(new_passwd) != 4 || strspn(new_passwd, "0123456789") != 4)
>  		goto error;

Again, let us fix the core so you guys don't have to do this checking.

Regards,
-Denis