Re: [RFC PATCH 4/4] voicecall: add emergency call handling

[Andras Domokos <andras.domokos@nokia.com>]

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

Hi Denis,

On 11/23/2010 11:00 AM, ext Denis Kenzior wrote:
> Hi Andras,
>
> On 11/15/2010 10:58 AM, Andras Domokos wrote:
>    
>> ---
>>   src/voicecall.c |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 110 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/voicecall.c b/src/voicecall.c
>> index 3af614b..066cdb9 100644
>> --- a/src/voicecall.c
>> +++ b/src/voicecall.c
>> @@ -52,6 +52,7 @@ struct ofono_voicecall {
>>        struct ofono_sim *sim;
>>        unsigned int sim_watch;
>>        unsigned int sim_state_watch;
>> +     unsigned int modem_online_watch;
>>        const struct ofono_voicecall_driver *driver;
>>        void *driver_data;
>>        struct ofono_atom *atom;
>> @@ -133,6 +134,22 @@ static void add_to_en_list(GSList **l, const char **list)
>>                *l = g_slist_prepend(*l, g_strdup(list[i++]));
>>   }
>>
>> +static gint number_compare(gconstpointer a, gconstpointer b)
>> +{
>> +     const char *s1 = a, *s2 = b;
>> +     return strcmp(s1, s2);
>> +}
>> +
>> +static ofono_bool_t emergency_number(struct ofono_voicecall *vc,
>> +                                     const char *number)
>> +{
>> +     if (!number)
>>      
> Just nit picking here, but in general we really prefer this to be
> written like this:
>
> if (number == NULL)
>
> This is much easier to read when you don't know if number is a string or
> an integer.  Yes I know we're not always consistent about doing this,
> particularly in voicecall.c.
>
>    
>> +             return FALSE;
>> +
>> +     return g_slist_find_custom(vc->en_list,
>> +                             number, number_compare) ? TRUE : FALSE;
>> +}
>> +
>>   static const char *disconnect_reason_to_string(enum ofono_disconnect_reason r)
>>   {
>>        switch (r) {
>> @@ -1125,6 +1142,7 @@ static struct voicecall *dial_handle_result(struct ofono_voicecall *vc,
>>   static void manager_dial_callback(const struct ofono_error *error, void *data)
>>   {
>>        struct ofono_voicecall *vc = data;
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>>        DBusMessage *reply;
>>        const char *number;
>>        gboolean need_to_emit;
>> @@ -1143,8 +1161,12 @@ static void manager_dial_callback(const struct ofono_error *error, void *data)
>>
>>                dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH,&path,
>>                                                DBUS_TYPE_INVALID);
>> -     } else
>> +     } else {
>> +             if (emergency_number(vc, number))
>> +                     ofono_modem_dec_emergency_mode(modem);
>> +
>>                reply = __ofono_error_failed(vc->pending);
>> +     }
>>
>>        __ofono_dbus_pending_reply(&vc->pending, reply);
>>
>> @@ -1156,6 +1178,7 @@ static DBusMessage *manager_dial(DBusConnection *conn,
>>                                        DBusMessage *msg, void *data)
>>   {
>>        struct ofono_voicecall *vc = data;
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>>        const char *number;
>>        struct ofono_phone_number ph;
>>        const char *clirstr;
>> @@ -1195,6 +1218,15 @@ static DBusMessage *manager_dial(DBusConnection *conn,
>>
>>        string_to_phone_number(number,&ph);
>>
>> +     if (emergency_number(vc, number)) {
>> +             ofono_bool_t online = ofono_modem_get_online(modem);
>> +
>> +             ofono_modem_inc_emergency_mode(modem);
>> +
>> +             if (!online)
>>      
> Do me a favor and change this to:
> if (online == FALSE)
>
>    
>> +                     return NULL;
>> +     }
>> +
>>        vc->driver->dial(vc,&ph, clir, OFONO_CUG_OPTION_DEFAULT,
>>                                manager_dial_callback, vc);
>>
>> @@ -1748,6 +1780,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>>                                const struct ofono_error *error)
>>   {
>>        struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>> +     const char *number;
>>        GSList *l;
>>        struct voicecall *call;
>>        time_t ts;
>> @@ -1767,6 +1800,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>>        }
>>
>>        call = l->data;
>> +     number = phone_number_to_string(&call->call->phone_number);
>>
>>        ts = time(NULL);
>>        prev_status = call->call->status;
>> @@ -1805,6 +1839,9 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>>
>>        voicecalls_emit_call_removed(vc, call);
>>
>> +     if (emergency_number(vc, number))
>> +             ofono_modem_dec_emergency_mode(modem);
>> +
>>        voicecall_dbus_unregister(vc, call);
>>
>>        vc->call_list = g_slist_remove(vc->call_list, call);
>> @@ -2067,6 +2104,7 @@ static void voicecall_unregister(struct ofono_atom *atom)
>>   static void voicecall_remove(struct ofono_atom *atom)
>>   {
>>        struct ofono_voicecall *vc = __ofono_atom_get_data(atom);
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(atom);
>>
>>        DBG("atom: %p", atom);
>>
>> @@ -2108,6 +2146,12 @@ static void voicecall_remove(struct ofono_atom *atom)
>>                g_queue_free(vc->toneq);
>>        }
>>
>> +     if (vc->modem_online_watch) {
>> +             __ofono_modem_remove_online_watch(modem,
>> +                                             vc->modem_online_watch);
>> +             vc->modem_online_watch = 0;
>> +     }
>> +
>>        g_free(vc);
>>   }
>>
>> @@ -2205,6 +2249,7 @@ static void sim_watch(struct ofono_atom *atom,
>>   static void dial_request_cb(const struct ofono_error *error, void *data)
>>   {
>>        struct ofono_voicecall *vc = data;
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>>        gboolean need_to_emit;
>>        struct voicecall *v;
>>
>> @@ -2214,6 +2259,9 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
>>
>>        if (v == NULL) {
>>                dial_request_finish(vc);
>>      
> Please add an empty line here based on item M1.
>
>    
>> +             if (emergency_number(vc,
>> +                             phone_number_to_string(&vc->dial_req->ph)))
>> +                     ofono_modem_dec_emergency_mode(modem);
>>                return;
>>        }
>>
>> @@ -2237,6 +2285,53 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
>>                voicecalls_emit_call_added(vc, v);
>>   }
>>
>> +static void modem_online_watch(ofono_bool_t online, void *data)
>> +{
>> +     struct ofono_voicecall *vc = data;
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>> +     DBusMessage *reply;
>> +     const char *number;
>> +     struct ofono_phone_number ph;
>> +     const char *clirstr;
>> +     enum ofono_clir_option clir;
>> +
>> +     if (ofono_modem_get_emergency_mode(modem) != TRUE)
>> +             return;
>> +
>> +     if (vc->dial_req)
>> +             vc->driver->dial(vc,&vc->dial_req->ph,
>> +                                     OFONO_CLIR_OPTION_DEFAULT,
>> +                                     OFONO_CUG_OPTION_DEFAULT,
>> +                                     dial_request_cb, vc);
>> +
>> +     if (!vc->pending)
>>      
> if (vc->pending == NULL) here please
>
>    
>> +             return;
>> +
>> +     if (strcmp(dbus_message_get_member(vc->pending), "Dial"))
>> +             return;
>> +
>> +     if (dbus_message_get_args(vc->pending, NULL, DBUS_TYPE_STRING,&number,
>> +                                     DBUS_TYPE_STRING,&clirstr,
>> +                                     DBUS_TYPE_INVALID) == FALSE)
>> +             return;
>> +
>> +     if (!emergency_number(vc, number))
>>      
> Please do emergency_number() == FALSE here
>
>    
>> +             return;
>> +
>> +     if (!online) {
>>      
> And online == FALSE here
>
>    
>> +             reply = __ofono_error_failed(vc->pending);
>> +             __ofono_dbus_pending_reply(&vc->pending, reply);
>> +             ofono_modem_dec_emergency_mode(modem);
>> +             return;
>> +     }
>> +
>> +     clir_string_to_clir(clirstr,&clir);
>> +     string_to_phone_number(number,&ph);
>> +
>> +     vc->driver->dial(vc,&ph, clir, OFONO_CUG_OPTION_DEFAULT,
>> +                             manager_dial_callback, vc);
>> +}
>> +
>>   void ofono_voicecall_register(struct ofono_voicecall *vc)
>>   {
>>        DBusConnection *conn = ofono_dbus_get_connection();
>> @@ -2255,6 +2350,9 @@ void ofono_voicecall_register(struct ofono_voicecall *vc)
>>        }
>>
>>        ofono_modem_add_interface(modem, OFONO_VOICECALL_MANAGER_INTERFACE);
>> +     vc->modem_online_watch = __ofono_modem_add_online_watch(modem,
>> +                                                     modem_online_watch,
>> +                                                     vc, NULL);
>>
>>        /*
>>         * Start out with the 22.101 mandated numbers, if we have a SIM and
>> @@ -2331,6 +2429,17 @@ ofono_bool_t __ofono_voicecall_is_busy(struct ofono_voicecall *vc,
>>
>>   static void dial_request(struct ofono_voicecall *vc)
>>   {
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>> +
>> +     if (emergency_number(vc, phone_number_to_string(&vc->dial_req->ph))) {
>> +             ofono_bool_t online = ofono_modem_get_online(modem);
>> +
>> +             ofono_modem_inc_emergency_mode(modem);
>> +
>> +             if (!online)
>>      
> And online == FALSE here
>
>    
>> +                     return;
>> +     }
>> +
>>        vc->driver->dial(vc,&vc->dial_req->ph, OFONO_CLIR_OPTION_DEFAULT,
>>                                OFONO_CUG_OPTION_DEFAULT, dial_request_cb, vc);
>>   }
>>      
> Otherwise, looks good to me.
>    
Thanks for the comments, I am going to resend the patch with
the changes I made based on your comments.
> Have you figured out how to support the E911 call-back requirements?
>    
This is something that still needs to be figured out and can
be added in a separate patch.

> Regards,
> -Denis
>    
Regards,
Andras