[PATCH 6/9] Add SimToolkit and SimApplicationAgent interfaces.

[Andrzej Zaborowski]

Hi,

On 2 July 2010 19:21, Denis Kenzior <denkenz at gmail.com> wrote:
> So to summarize here:
>        1. If there's no agent, we start the command anyway
>        2. When the agent arrives, we send it the command to handle
>        3. If the agent goes away, we don't cancel the command
>        4. If the agent is unregistered, we send a cancel but don't cancel the
> command

That's about right.

>
> So it seems to me with the exception of Setup Menu and Idle Mode Text we
> should simply reject the Proactive Command outright if there is no Agent
> registered.
>
> This will prevent weird situations where a proactive command is pending
> for Timeout - 1 seconds, an agent is registered, we send the agent the
> command and 1 second later the User gets booted out.  It also seems to
> make the implementation easier...

As I mentioned in the other mail, I believe it's in line with the dbus
design that clients can connect and disconnect any time, and it
shouldn't affect the state.  If we choose to do it differently, I
think it ties the interface very much to dbus implementation and the
single type of users like a Home Screen app on a phone.  This use case
is not affected if we allow agents to connect and disconnect, so
absolutely there's no harm in doing it intelligently.

I agree with moving members related to the agent to a separate struct,
but I think the agent itself should be separate, so it can disconnect
and connect when it feels like it.

>> +
>> +static struct stk_app_agent *app_agent_create(struct ofono_stk *stk,
>> +                                             const char *path,
>> +                                             const char *sender)
>> +{
>> +     struct stk_app_agent *agent = g_try_new0(struct stk_app_agent, 1);
>> +     DBusConnection *conn = ofono_dbus_get_connection();
>> +
>> +     if (!agent)
>> +             return NULL;
>> +
>> +     agent->path = g_strdup(path);
>> +     agent->bus = g_strdup(sender);
>> +
>> +     agent->watch = g_dbus_add_disconnect_watch(conn, sender,
>> +                                                     app_agent_disconnect_cb,
>> +                                                     stk, NULL);
>> +
>> +     if (stk->app_agent_state != STK_AGENT_IDLE)
>> +             app_agent_request_send(stk, agent);
>> +
>
> This looks a bit dangerous, you're sending a request to the agent before
> it is registered.  Please be nice and perform the RegisterAgent method
> return first, and then send the request.

Good point, I'll move it.  (In my tests it always worked though)

>
>> +     return agent;
>> +}
>
> <snip>
>
>> +
>> +static DBusMessage *stk_set_property(DBusConnection *conn,
>> +                                     DBusMessage *msg, void *data)
>> +{
>> +     struct ofono_stk *stk = data;
>> +     const char *path = __ofono_atom_get_path(stk->atom);
>> +     DBusMessageIter iter;
>> +     DBusMessageIter var;
>> +     const char *property;
>> +     dbus_uint16_t timeout;
>> +
>> +     if (!dbus_message_iter_init(msg, &iter))
>> +             return __ofono_error_invalid_args(msg);
>> +
>> +     if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
>> +             return __ofono_error_invalid_args(msg);
>> +
>> +     dbus_message_iter_get_basic(&iter, &property);
>> +     if (!dbus_message_iter_next(&iter))
>> +             return __ofono_error_invalid_args(msg);
>> +
>> +     if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
>> +             return __ofono_error_invalid_args(msg);
>> +
>> +     dbus_message_iter_recurse(&iter, &var);
>> +
>> +     if (!strcmp(property, "Timeout")) {
>> +             if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_UINT16)
>> +                     return __ofono_error_invalid_args(msg);
>> +
>> +             dbus_message_iter_get_basic(&var, &timeout);
>> +             if (dbus_message_iter_next(&iter))
>> +                     return __ofono_error_invalid_args(msg);
>> +
>> +             stk->timeout = timeout;
>> +
>> +             if (stk->cmd_timeout && stk->custom_timeout == 0) {
>> +                     g_source_remove(stk->cmd_timeout);
>> +                     stk->cmd_timeout = g_timeout_add_seconds(stk->timeout,
>> +                                             app_agent_request_timeout, stk);
>> +             }
>
> This doesn't seem right to me, what if we have just 1 second left on our
> timeout and the Timeout property is set?  We end up resetting the
> timeout completely.  What do you think of simply using the new value for
> all subsequent requests?
>
> I actually question the need for this attribute, who's going to actually
> set it?  Let us default to some reasonable value and add a SetProperty
> for this later if required...

I was thinking in particular of things like writing an e-mail.

When you navigate the menus, 20 seconds seem like a good value for the
timeout, or when you're shown a message like "SMS deliver".  This is
about what my old Nokia dumb-phone menu timeout was.  But the SIM I
tested has a "write an e-mail" application, so it sends a Get Input
proactive command so you can type the contents of the e-mail, and 20
seconds seems very short.  So in my python test client I set the
timeout to 2 minutes for Get Input (should actually reset the timeout
everytime you stop typing).

>> +static DBusMessage *stk_unregister_agent(DBusConnection *conn,
>> +                                             DBusMessage *msg, void *data)
>> +{
>> +     struct ofono_stk *stk = data;
>> +     const char *agent_path;
>> +
>> +     if (dbus_message_get_args(msg, NULL,
>> +                                     DBUS_TYPE_OBJECT_PATH, &agent_path,
>> +                                     DBUS_TYPE_INVALID) == FALSE)
>> +             return __ofono_error_invalid_args(msg);
>> +
>> +     if (!stk->app_agent || strcmp(stk->app_agent->path, agent_path))
>> +             return __ofono_error_failed(msg);
>> +
>
> One other thing to keep in mind here is that only the sender who
> registered the agent should be able to unregister it.  This prevents
> some random app from messing with the system.  However, this can be
> added later if leaving this check out helps in testing in the short term.

I actually forgot the check, we just need to compare the
stk->app_agent->bus value.

Best regards,
Andrew