Hi Denis, On 06/01/2011 08:18 AM, Denis Kenzior wrote: > Hi Philippe, > > On 05/20/2011 11:26 AM, Philippe Nunes wrote: >> --- >> src/gprs.c | 239 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> src/ofono.h | 24 ++++++ >> 2 files changed, 263 insertions(+), 0 deletions(-) >> >> diff --git a/src/gprs.c b/src/gprs.c >> index 9657a3e..86d95bc 100644 >> --- a/src/gprs.c >> +++ b/src/gprs.c >> @@ -147,6 +147,8 @@ struct pri_context { >> struct ofono_gprs_primary_context context; >> struct ofono_gprs_context *context_driver; >> struct ofono_gprs *gprs; >> + void *notify; >> + void *notify_data; >> }; >> > > Modifying pri_context for this does not seem like a good idea, I suggest > you make your own structure since pretty much all the members of this > structure are not relevant to what you're trying to accomplish. > > As a general philosophy we prefer clearer code (even if there's more of > it) than trying to complicate the logic of code designed for a > completely different purpose. > Ok, I can remove this callback pointer since this callback is only needed for the private primary context created by the STK atom. >> static void gprs_netreg_update(struct ofono_gprs *gprs); >> @@ -356,6 +358,35 @@ static struct pri_context *gprs_context_by_path(struct ofono_gprs *gprs, >> return NULL; >> } >> >> +static struct pri_context *gprs_context_by_id(struct ofono_gprs *gprs, >> + unsigned int id) >> +{ >> + GSList *l; >> + >> + for (l = gprs->contexts; l; l = l->next) { >> + struct pri_context *ctx = l->data; >> + >> + if (ctx->id == id) >> + return ctx; >> + } >> + >> + return NULL; >> +} >> + >> +static struct pri_context *gprs_get_default_context(struct ofono_gprs *gprs) >> +{ >> + GSList *l; >> + >> + for (l = gprs->contexts; l; l = l->next) { >> + struct pri_context *ctx = l->data; >> + >> + if (ctx->type == OFONO_GPRS_CONTEXT_TYPE_INTERNET) >> + return ctx; >> + } >> + >> + return NULL; >> +} >> + >> static void context_settings_free(struct context_settings *settings) >> { >> if (settings->ipv4) { >> @@ -537,6 +568,9 @@ static void signal_settings(struct pri_context *ctx, const char *prop, >> DBusMessageIter iter; >> struct context_settings *settings; >> >> + if (path == NULL) >> + return; >> + >> signal = dbus_message_new_signal(path, >> OFONO_CONNECTION_CONTEXT_INTERFACE, >> "PropertyChanged"); >> @@ -2968,3 +3002,208 @@ void *ofono_gprs_get_data(struct ofono_gprs *gprs) >> { >> return gprs->driver_data; >> } >> + >> +unsigned int __ofono_gprs_add_pdp_context(struct ofono_gprs *gprs, >> + enum ofono_gprs_context_type context_type, >> + enum ofono_gprs_proto proto, >> + const char *apn, >> + const char *username, >> + const char *password, >> + const char *host) > > What is the purpose of the host parameter? Doesn't STK use IP addresses > for the destination? > Here, the purpose of the host parameter is to set the interface with a special host route (id the IP stream with this host/destination address has to go through this interface). Indeed, I need to bind the UDP/TCP socket to the interface created precisely for STK purpose. But, I can remove this parameter since you suggested to make pri_setproxy (gprs.c) into a utility function and call it from stk.c >> +{ > > Returning int here would be better: > >> + unsigned int id; >> + struct pri_context *context; >> + struct pri_context *default_ctx = NULL; >> + const char *name; >> + >> + /* Sanity check */ >> + if (apn&& strlen(apn)> OFONO_GPRS_MAX_APN_LENGTH) >> + return 0; > > Then you can return meaningful error here, e.g. -EINVAL > >> + >> + if (username&& strlen(username)> OFONO_GPRS_MAX_USERNAME_LENGTH) >> + return 0; >> + >> + if (password&& strlen(password)> OFONO_GPRS_MAX_PASSWORD_LENGTH) >> + return 0; >> + >> + if (apn == NULL || (username == NULL&& password == NULL)) { >> + /* take the default primary internet context */ >> + default_ctx = gprs_get_default_context(gprs); >> + >> + if (default_ctx == NULL&& apn == NULL) >> + return 0; > > Maybe ENOENT here > >> + } >> + >> + if (apn&& is_valid_apn(apn) == FALSE) >> + return 0; >> + > > etc > > This will allow you to return meaningful terminal responses in case > things fail. > >> + name = gprs_context_default_name(context_type); >> + if (name == NULL) >> + return 0; >> + >> + if (gprs->last_context_id) >> + id = idmap_alloc_next(gprs->pid_map, gprs->last_context_id); >> + else >> + id = idmap_alloc(gprs->pid_map); >> + >> + if (id> idmap_get_max(gprs->pid_map)) >> + return 0; >> + >> + context = pri_context_create(gprs, name, context_type); >> + if (context == NULL) { >> + idmap_put(gprs->pid_map, id); >> + return 0; >> + } >> + > > As mentioned previously, I don't see the point of creating pri_context > structure for this. So you can rip much of this stuff out. I don't understand why you think we don't need to create a pri_context. Could you clarify please? Even if no parameters are provided by the UICC (id use default parameters), I still need to setup a dedicated primary context with defaults parameters since we can't double-activate an existing primary context. > >> + context->id = id; >> + >> + if (username != NULL) >> + strcpy(context->context.username, username); >> + >> + if (password != NULL) >> + strcpy(context->context.password, password); >> + >> + if (username == NULL&& password == NULL&& default_ctx) { >> + if (default_ctx->context.username) >> + strcpy(context->context.username, >> + default_ctx->context.username); >> + if (default_ctx->context.password) >> + strcpy(context->context.password, >> + default_ctx->context.password); >> + } >> + >> + if (apn) >> + strcpy(context->context.apn, apn); >> + else if (default_ctx) { >> + strcpy(context->context.apn, default_ctx->context.apn); >> + if (default_ctx->context.username) >> + strcpy(context->context.username, >> + default_ctx->context.username); >> + if (default_ctx->context.password) >> + strcpy(context->context.password, >> + default_ctx->context.password); >> + } >> + >> + context->context.proto = proto; >> + >> + gprs->last_context_id = id; >> + gprs->contexts = g_slist_append(gprs->contexts, context); >> + >> + return id; >> +} >> + >> +static void activate_request_callback(const struct ofono_error *error, >> + void *data) >> +{ >> + struct pri_context *ctx = data; >> + struct ofono_gprs_context *gc = ctx->context_driver; >> + >> + DBG("%p", ctx); >> + >> + if (error->type != OFONO_ERROR_TYPE_NO_ERROR) { >> + DBG("Activating context failed with error: %s", >> + telephony_error_to_str(error)); >> + context_settings_free(ctx->context_driver->settings); >> + release_context(ctx); >> + >> + if (ctx->notify) >> + ((__ofono_gprs_add_pdp_context_cb_t) ctx->notify) >> + (-ENOSYS, NULL, NULL, ctx->notify_data); >> + return; >> + } >> + >> + ctx->active = TRUE; >> + >> + if (gc->settings->interface != NULL) { >> + pri_ifupdown(gc->settings->interface, TRUE); >> + >> + if (ctx->type == OFONO_GPRS_CONTEXT_TYPE_STK&& >> + gc->settings->ipv4) { >> + pri_set_ipv4_addr(gc->settings->interface, >> + gc->settings->ipv4->ip); >> + } >> + } >> + >> + if (ctx->notify&& gc->settings->ipv4) >> + ((__ofono_gprs_add_pdp_context_cb_t) ctx->notify) >> + (0, gc->settings->interface, >> + gc->settings->ipv4->ip, >> + ctx->notify_data); >> +} >> + >> +int __ofono_gprs_activate_pdp_context(struct ofono_gprs *gprs, unsigned int id, >> + __ofono_gprs_add_pdp_context_cb_t cb, >> + void *data) >> +{ >> + struct pri_context *ctx; >> + struct ofono_gprs_context *gc; >> + >> + ctx = gprs_context_by_id(gprs, id); >> + if (ctx == NULL) >> + return -EINVAL; >> + >> + if (ctx->active == TRUE) >> + return 0; >> + >> + if (assign_context(ctx) == FALSE) >> + return -ENOSYS; >> + >> + gc = ctx->context_driver; >> + >> + ctx->notify = cb; >> + ctx->notify_data = data; >> + >> + gc->driver->activate_primary(gc,&ctx->context, >> + activate_request_callback, ctx); >> + return 0; >> +} >> + >> +static void remove_request_callback(const struct ofono_error *error, >> + void *data) >> +{ >> + struct pri_context *ctx = data; >> + struct ofono_gprs *gprs = ctx->gprs; >> + int err = 0; >> + >> + if (error->type != OFONO_ERROR_TYPE_NO_ERROR) { >> + DBG("Removing context failed with error: %s", >> + telephony_error_to_str(error)); >> + err = -ENOSYS; >> + } >> + >> + pri_reset_context_settings(ctx); >> + release_context(ctx); >> + idmap_put(gprs->pid_map, ctx->id); >> + gprs->contexts = g_slist_remove(gprs->contexts, ctx); >> + >> + if (ctx->notify) >> + ((__ofono_gprs_remove_pdp_context_cb_t) ctx->notify) >> + (err, ctx->notify_data); >> +} >> + >> +int __ofono_gprs_remove_pdp_context(struct ofono_gprs *gprs, unsigned int id, >> + __ofono_gprs_remove_pdp_context_cb_t cb, >> + void *data) >> +{ >> + struct pri_context *ctx; >> + >> + ctx = gprs_context_by_id(gprs, id); >> + if (ctx == NULL) >> + return -EINVAL; >> + >> + if (ctx->active) { >> + struct ofono_gprs_context *gc = ctx->context_driver; >> + >> + ctx->notify = cb; >> + ctx->notify_data = data; >> + >> + gc->driver->deactivate_primary(gc, ctx->context.cid, >> + remove_request_callback, ctx); >> + return -EINPROGRESS; >> + } >> + >> + idmap_put(gprs->pid_map, ctx->id); >> + gprs->contexts = g_slist_remove(gprs->contexts, ctx); >> + >> + return 0; >> +} >> diff --git a/src/ofono.h b/src/ofono.h >> index 82d7e34..ee5864d 100644 >> --- a/src/ofono.h >> +++ b/src/ofono.h >> @@ -239,6 +239,30 @@ gboolean __ofono_call_settings_is_busy(struct ofono_call_settings *cs); >> #include >> #include >> #include >> + >> +typedef void (*__ofono_gprs_add_pdp_context_cb_t)(int error, >> + const char *interface, >> + const char *ip, >> + void *data); >> + >> +typedef void (*__ofono_gprs_remove_pdp_context_cb_t)(int error, void *data); >> + >> +unsigned int __ofono_gprs_add_pdp_context(struct ofono_gprs *gprs, >> + enum ofono_gprs_context_type type, >> + enum ofono_gprs_proto proto, >> + const char *apn, >> + const char *username, >> + const char *password, >> + const char *host); >> + > > I don't think this is needed at all. All STK needs to do is just say > 'please activate a context with these params'. You can pretty much get > away with __ofono_gprs_activate_context and __ofono_gprs_deactivate_context. > When "on link demand" mode is requested, we just need to setup the PDP context and return the PDP identifier. The PDP context activation is done only once we receive a proactive command SEND DATA with the qualifier "Send data immediately". So, I don't see how to differentiate this mode without the API "__ofono_gprs_add_pdp_context". >> +int __ofono_gprs_remove_pdp_context(struct ofono_gprs *gprs, unsigned int id, >> + __ofono_gprs_remove_pdp_context_cb_t cb, >> + void *data); >> + > > Are you sure you really need the callback function here? The spec is a > little fuzzy on when the terminal response is actually sent. At first sight, yes, that was also my opinion but then I saw the examples in annex I (TS 102 223). For Close channel, we can see the following sequence: UICC Terminal SGSN |---------------------> | | | Close Channel | | | | --------------------------> | | | Deactivate PDP context request| | | <-----------------------------| | | Deactivate PDP context accept | |<---------------------- Terminal response That's why, I introduced the callback to trigger the Terminal response. Regards, Philippe. > >> +int __ofono_gprs_activate_pdp_context(struct ofono_gprs *gprs, unsigned int id, >> + __ofono_gprs_add_pdp_context_cb_t cb, >> + void *data); >> + >> #include >> #include >> #include > > Regards, > -Denis >