Re: [PATCH] Add STE modem support for GPRS PDP Contexts

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Sjur,

> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> ---
>  Makefile.am                     |    3 +-
>  drivers/stemodem/gprs-context.c |  610
>  +++++++++++++++++++++++++++++++++++++++ drivers/stemodem/stemodem.c     | 
>    3 +
>  drivers/stemodem/stemodem.h     |    4 +
>  plugins/ste.c                   |    2 +-
>  5 files changed, 620 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/stemodem/gprs-context.c

I have applied the patch with some major style fixes afterward.  A couple of 
comments:

I have changed AT*EIAAUW handling to be just like MBM modems.  See my commit 
message for details, but basically you were leaking memory here.  If my 
changes for some reason have broken something, please send an updated patch.  
I can't test it here for obvious reasons :)

> +static void ste_eppsd_up_cb(gboolean ok, GAtResult *result, gpointer
>  user_data) +{
> +	if (!caif_if_create(conn->interface, conn->channel_id)) {
> +		ofono_error("Failed to create caif interface %s.",
> +			conn->interface);
> +		CALLBACK_WITH_SUCCESS(cb, NULL, FALSE, rsp.ip_address,
> +				rsp.subnet_mask, rsp.default_gateway,
> +				dns, cbd->data);
> +	} else {
> +		CALLBACK_WITH_SUCCESS(cb, conn->interface,
> +				FALSE, rsp.ip_address, rsp.subnet_mask,
> +				rsp.default_gateway, dns, cbd->data);
> +	}
> +	return;

I really don't like returning SUCCESS with a null interface in case the 
interface creation failed.  Can we return an error here and generate a context 
deactivation command?

Regards,
-Denis