Re: [PATCH 3/3] gprs.c: add list contexts for emulator

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Olivier,

On 03/21/2011 08:45 AM, Olivier Guiter wrote:
> ---
>  src/gprs.c |   26 ++++++++++++++++++++++++--
>  1 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index 04432c3..06d52f3 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -2863,11 +2863,31 @@ static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
>  	__ofono_gprs_provision_free_settings(settings, count);
>  }
>  
> +static void ofono_gprs_list_contexts(struct ofono_emulator *em, void *userdata)
> +{
> +	struct ofono_gprs *gprs = userdata;
> +	GSList *l;
> +	char buf[256];
> +
> +	struct pri_context *ctx;
> +
> +	for (l = gprs->contexts; l; l = l->next) {
> +		ctx = l->data;
> +
> +		snprintf(buf, 255, "+CGDCONT: %d,\"%s\",\"%s\"",
> +				ctx->id, gprs_proto_to_string(ctx->context.proto),
> +				ctx->context.apn);

So you're mixing concepts here a bit.

gprs.c has two maps:

- pid_map
- cid_map

and two ids:
- pri_context::id
- ofono_gprs_primary_context::cid

Roughly speaking the cid_map lets us pick a context id that maps 1:1 to
the modem driver.  Hence the use of ofono_gprs_set_cid_range.  Basically
the driver tells us the valid range of context ids and oFono picks the
rest.  For an AT modem the driver can simply use the cid picked by oFono
in activate_primary callback.

Internally oFono has another idmap for tracking unique ids for contexts
and ensuring some sane limit on their number.  This is managed by
pid_map and pri_context::id.

The code you have right now would use the pid_map ids...

> +		ofono_emulator_send_info(em, buf, FALSE);
> +	}
> +}
> +
>  /* Process the usual AT+CGDCONT command
>   */
>  static void cgdcont_cb(struct ofono_emulator *em,
>  			struct ofono_emulator_request *req, void *userdata)
>  {
> +	struct ofono_gprs *gprs = userdata;
> +	struct idmap *cid = gprs->cid_map;
>  	struct ofono_error result;
>  	char buf[256];
>  
> @@ -2876,7 +2896,8 @@ static void cgdcont_cb(struct ofono_emulator *em,
>  	switch (ofono_emulator_request_get_type(req)) {
>  	case OFONO_EMULATOR_REQUEST_TYPE_SUPPORT:
>  		/* TODO: check additionnal parameters */
> -		snprintf(buf, 255, "+CGDCONT: (1-2),\"IP\",,,(0-2),(0,1,2,3,4)");
> +		snprintf(buf, 255, "+CGDCONT: (1-%d),\"IP\",,,(0-2),(0,1,2,3,4)",
> +					idmap_get_size(cid));

, but use the size of the cid_map in the CGDCONT=? implementation below.
 These sizes are highly unlikely to match.

However, this brings up a more fundamental question from me.  What do
you actually want to expose here?  Do you want the emulator to be able
to view / edit contexts exposed over D-Bus?

Or is it enough to simply fake all this and maintain the cgdcont list in
memory for each emulator instance?

>  		ofono_emulator_send_info(em, buf, FALSE);
>  		result.type = OFONO_ERROR_TYPE_NO_ERROR;
>  		ofono_emulator_send_final(em, &result);
> @@ -2888,7 +2909,8 @@ static void cgdcont_cb(struct ofono_emulator *em,
>  		break;
>  
>  	case OFONO_EMULATOR_REQUEST_TYPE_QUERY:
> -		result.type = OFONO_ERROR_TYPE_FAILURE;
> +		ofono_gprs_list_contexts(em, gprs);
> +		result.type = OFONO_ERROR_TYPE_NO_ERROR;
>  		ofono_emulator_send_final(em, &result);
>  		break;
>  	case OFONO_EMULATOR_REQUEST_TYPE_SET:

Regards,
-Denis