| Message ID | 20240413215318.12236-1-adam@piggz.co.uk (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v4,1/4] qmimodem: voicecall: Implement call dialing | expand |
Here is v4 of my patchset for qmimodem voicecall support. I think ive implemented all the changes from last time, but please double check! Ive compiled the code on sailfish gcc8 and openSuse tumbleweed gcc13 with no errors/warnings. ive also ran checkpatch, which highlighted 2 minor issues. Code has been tested on a PinephonePro with SailfishOS (requiring additional patches). The commit messages contains a brief summary of changes. Thanks Adam On Saturday 13 April 2024 22:53:10 BST you wrote: > Add voicecall dialling to the qmimodem driver > Includes required infratructure and setup of the QMI services > > Call State Handling > =================== > On initialisation, register the all_call_status_ind callback to be > called for QMI_VOICE_IND_ALL_STATUS. This will handle notificatiof of > call status to the rest of the system > > Dial Handling > ============= > The dial function sets up the parameters for the QMI_VOICE_DIAL_CALL > service. The parameters are the number to be called and the call type, > which is currently hard coded to be QMI_VOICE_CALL_TYPE_VOICE. The > dial_cb callback wi then be called and will receive the call-id. > > --- > Changes in V4 > -merged qmi_voice_call_status and all_call_status_ind > -several minor structure/formate changes > --- > --- > drivers/qmimodem/voice.h | 17 ++ > drivers/qmimodem/voicecall.c | 435 ++++++++++++++++++++++++++++++++++- > 2 files changed, 451 insertions(+), 1 deletion(-) > > diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h > index 917e72f7..2344fd50 100644 > --- a/drivers/qmimodem/voice.h > +++ b/drivers/qmimodem/voice.h > @@ -48,6 +48,9 @@ enum qmi_ussd_user_required { > > /* QMI service voice. Using an enum to prevent doublicated entries */ > enum voice_commands { > + QMI_VOICE_DIAL_CALL = 0x20, > + QMI_VOICE_ALL_CALL_STATUS_IND = 0x2e, > + QMI_VOICE_GET_ALL_CALL_INFO = 0x2f, > QMI_VOICE_SUPS_NOTIFICATION_IND = 0x32, > QMI_VOICE_SET_SUPS_SERVICE = 0x33, > QMI_VOICE_GET_CALL_WAITING = 0x34, > @@ -66,6 +69,20 @@ enum voice_commands { > QMI_VOICE_GET_CNAP = 0x4d > }; > > +enum qmi_voice_call_state { > + QMI_VOICE_CALL_STATE_IDLE = 0x0, > + QMI_VOICE_CALL_STATE_ORIG, > + QMI_VOICE_CALL_STATE_INCOMING, > + QMI_VOICE_CALL_STATE_CONV, > + QMI_VOICE_CALL_STATE_CC_IN_PROG, > + QMI_VOICE_CALL_STATE_ALERTING, > + QMI_VOICE_CALL_STATE_HOLD, > + QMI_VOICE_CALL_STATE_WAITING, > + QMI_VOICE_CALL_STATE_DISCONNECTING, > + QMI_VOICE_CALL_STATE_END, > + QMI_VOICE_CALL_STATE_SETUP > +}; > + > struct qmi_ussd_data { > uint8_t dcs; > uint8_t length; > diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c > index aa34fc25..8c86eee7 100644 > --- a/drivers/qmimodem/voicecall.c > +++ b/drivers/qmimodem/voicecall.c > @@ -3,6 +3,7 @@ > * oFono - Open Source Telephony > * > * Copyright (C) 2011-2012 Intel Corporation. All rights reserved. > + * Copyright (C) 2024 Adam Pigg <adam@piggz.co.uk> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License version 2 as > @@ -26,6 +27,10 @@ > #include <ofono/log.h> > #include <ofono/modem.h> > #include <ofono/voicecall.h> > +#include <src/common.h> > +#include <ell/ell.h> > + > +#include "voice.h" > > #include "qmi.h" > > @@ -35,8 +40,431 @@ struct voicecall_data { > struct qmi_service *voice; > uint16_t major; > uint16_t minor; > + struct l_queue *call_list; > + struct ofono_phone_number dialed; > }; > > +struct qmi_voice_call_information_instance { > + uint8_t id; > + uint8_t state; > + uint8_t type; > + uint8_t direction; > + uint8_t mode; > + uint8_t multipart_indicator; > + uint8_t als; > +} __attribute__((__packed__)); > + > +struct qmi_voice_call_information { > + uint8_t size; > + struct qmi_voice_call_information_instance instance[0]; > +} __attribute__((__packed__)); > + > +struct qmi_voice_remote_party_number_instance { > + uint8_t call_id; > + uint8_t presentation_indicator; > + uint8_t number_size; > + char number[0]; > +} __attribute__((__packed__)); > + > +struct qmi_voice_remote_party_number { > + uint8_t size; > + struct qmi_voice_remote_party_number_instance instance[0]; > +} __attribute__((__packed__)); > + > +static int ofono_call_compare(const void *a, const void *b, void *data) > +{ > + const struct ofono_call *ca = a; > + const struct ofono_call *cb = b; > + > + if (ca->id < cb->id) > + return -1; > + > + if (ca->id > cb->id) > + return 1; > + > + return 0; > +} > + > +static bool ofono_call_compare_by_id(const void *a, const void *b) > +{ > + const struct ofono_call *call = a; > + unsigned int id = L_PTR_TO_UINT(b); > + > + return (call->id == id); > +} > + > +static void ofono_call_list_dial_callback(struct ofono_voicecall *vc, > + int call_id) > +{ > + struct ofono_call *call; > + struct voicecall_data *vd = ofono_voicecall_get_data(vc); > + struct l_queue *call_list = vd->call_list; > + const struct ofono_phone_number *ph = &vd->dialed; > + > + /* check if call_id already present */ > + call = l_queue_find(call_list, ofono_call_compare_by_id, > + L_UINT_TO_PTR(call_id)); > + > + if (call) > + return; > + > + call = l_new(struct ofono_call, 1); > + call->id = call_id; > + > + memcpy(&call->called_number, ph, sizeof(*ph)); > + call->direction = CALL_DIRECTION_MOBILE_ORIGINATED; > + call->status = CALL_STATUS_DIALING; > + call->type = 0; /* voice */ > + > + l_queue_insert(call_list, call, ofono_call_compare, NULL); > + > + ofono_voicecall_notify(vc, call); > +} > + > +static void ofono_call_list_notify(struct ofono_voicecall *vc, > + struct l_queue *calls) > +{ > + struct voicecall_data *vd = ofono_voicecall_get_data(vc); > + struct l_queue *old_calls = vd->call_list; > + struct l_queue *new_calls = calls; > + struct ofono_call *new_call, *old_call; > + const struct l_queue_entry *old_entry, *new_entry; > + uint i; > + > + uint loop_length = > + MAX(l_queue_length(old_calls), l_queue_length(new_calls)); > + > + old_entry = l_queue_get_entries(old_calls); > + new_entry = l_queue_get_entries(new_calls); > + > + for (i = 0; i < loop_length; ++i) { > + old_call = old_entry ? old_entry->data : NULL; > + new_call = new_entry ? new_entry->data : NULL; > + > + if (new_call && new_call->status == CALL_STATUS_DISCONNECTED) { > + ofono_voicecall_disconnected( > + vc, new_call->id, > + OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL); > + > + l_queue_remove(calls, new_call); > + l_free(new_call); > + continue; > + } > + > + if (old_call && > + (new_call == NULL || (new_call->id > old_call->id))) > + ofono_voicecall_disconnected( > + vc, old_call->id, > + OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL); > + else if (new_call && > + (old_call == NULL || (new_call->id < old_call->id))) { > + DBG("Notify new call %d", new_call->id); > + /* new call, signal it */ > + if (new_call->type == 0) > + ofono_voicecall_notify(vc, new_call); > + } else if (memcmp(new_call, old_call, sizeof(*new_call)) && > + new_call->type == 0) > + ofono_voicecall_notify(vc, new_call); > + > + if (old_entry) > + old_entry = old_entry->next; > + if (new_entry) > + new_entry = new_entry->next; > + } > + > + l_queue_destroy(old_calls, l_free); > + vd->call_list = calls; > +} > + > +static const char *qmi_voice_call_state_name(enum qmi_voice_call_state > value) +{ > + switch (value) { > + case QMI_VOICE_CALL_STATE_IDLE: > + return "QMI_VOICE_CALL_STATE_IDLE"; > + case QMI_VOICE_CALL_STATE_ORIG: > + return "QMI_VOICE_CALL_STATE_ORIG"; > + case QMI_VOICE_CALL_STATE_INCOMING: > + return "QMI_VOICE_CALL_STATE_INCOMING"; > + case QMI_VOICE_CALL_STATE_CONV: > + return "QMI_VOICE_CALL_STATE_CONV"; > + case QMI_VOICE_CALL_STATE_CC_IN_PROG: > + return "QMI_VOICE_CALL_STATE_CC_IN_PROG"; > + case QMI_VOICE_CALL_STATE_ALERTING: > + return "QMI_VOICE_CALL_STATE_ALERTING"; > + case QMI_VOICE_CALL_STATE_HOLD: > + return "QMI_VOICE_CALL_STATE_HOLD"; > + case QMI_VOICE_CALL_STATE_WAITING: > + return "QMI_VOICE_CALL_STATE_WAITING"; > + case QMI_VOICE_CALL_STATE_DISCONNECTING: > + return "QMI_VOICE_CALL_STATE_DISCONNECTING"; > + case QMI_VOICE_CALL_STATE_END: > + return "QMI_VOICE_CALL_STATE_END"; > + case QMI_VOICE_CALL_STATE_SETUP: > + return "QMI_VOICE_CALL_STATE_SETUP"; > + } > + return "QMI_CALL_STATE_<UNKNOWN>"; > +} > + > +static bool qmi_to_ofono_status(uint8_t status, int *ret) > +{ > + int err = false; > + > + switch (status) { > + case QMI_VOICE_CALL_STATE_IDLE: > + case QMI_VOICE_CALL_STATE_END: > + case QMI_VOICE_CALL_STATE_DISCONNECTING: > + *ret = CALL_STATUS_DISCONNECTED; > + break; > + case QMI_VOICE_CALL_STATE_HOLD: > + *ret = CALL_STATUS_HELD; > + break; > + case QMI_VOICE_CALL_STATE_WAITING: > + *ret = CALL_STATUS_WAITING; > + break; > + case QMI_VOICE_CALL_STATE_ORIG: > + *ret = CALL_STATUS_DIALING; > + break; > + case QMI_VOICE_CALL_STATE_SETUP: > + case QMI_VOICE_CALL_STATE_INCOMING: > + *ret = CALL_STATUS_INCOMING; > + break; > + case QMI_VOICE_CALL_STATE_CONV: > + *ret = CALL_STATUS_ACTIVE; > + break; > + case QMI_VOICE_CALL_STATE_CC_IN_PROG: > + *ret = CALL_STATUS_DIALING; > + break; > + case QMI_VOICE_CALL_STATE_ALERTING: > + *ret = CALL_STATUS_ALERTING; > + break; > + default: > + err = true; > + } > + return err; > +} > + > +static enum call_direction qmi_to_ofono_direction(uint8_t qmi_direction) > +{ > + return qmi_direction - 1; > +} > + > +static void all_call_status_ind(struct qmi_result *result, void *user_data) > +{ > + struct ofono_voicecall *vc = user_data; > + > + int i; > + int offset; > + uint16_t len; > + bool status = true; > + int instance_size; > + const struct qmi_voice_call_information *call_information; > + const struct qmi_voice_remote_party_number *remote_party_number; > + const struct qmi_voice_remote_party_number_instance > *remote_party_number_inst[16]; + struct l_queue *calls = l_queue_new(); > + > + static const uint8_t QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 0x01; > + static const uint8_t QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 0x10; > + static const uint8_t QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 0x10; > + static const uint8_t QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11; > + > + DBG(""); > + > + /* mandatory */ > + call_information = qmi_result_get( > + result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len); > + > + if (!call_information) { > + call_information = qmi_result_get( > + result, QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION, > + &len); > + status = false; > + } > + > + if (!call_information || len < sizeof(call_information->size)) { > + DBG("Parsing of all call status indication failed"); > + goto error; > + } > + > + if (!call_information->size) { > + DBG("No call informations received!"); > + goto error; > + } > + > + if (len != call_information->size * > + sizeof(struct qmi_voice_call_information_instance) + > + sizeof(call_information->size)) { > + DBG("Call information size incorrect"); > + goto error; > + } > + > + > + /* mandatory */ > + remote_party_number = qmi_result_get( > + result, > + status ? QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER : > + QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER, > + &len); > + > + if (!remote_party_number) { > + DBG("Unable to retrieve remote numbers"); > + goto error; > + } > + > + /* verify the length */ > + if (len < sizeof(remote_party_number->size)) { > + DBG("Parsing of remote numbers failed"); > + goto error; > + } > + > + /* expect we have valid fields for every call */ > + if (call_information->size != remote_party_number->size) { > + DBG("Not all fields have the same size"); > + goto error; > + } > + > + /* pull the remote call info into a local array */ > + instance_size = sizeof(struct qmi_voice_remote_party_number_instance); > + > + for (i = 0, offset = sizeof(remote_party_number->size); > + offset <= len && i < 16 && i < remote_party_number->size; > + i++) { > + const struct qmi_voice_remote_party_number_instance *instance; > + > + if (offset == len) > + break; > + > + if (offset + instance_size > len) { > + DBG("Error parsing remote numbers"); > + goto error; > + } > + > + instance = (void *)remote_party_number + offset; > + if (offset + instance_size + instance->number_size > len) { > + DBG("Error parsing remote numbers"); > + goto error; > + } > + remote_party_number_inst[i] = instance; > + offset += > + sizeof(struct qmi_voice_remote_party_number_instance) + > + instance->number_size; > + } > + > + for (i = 0; i < call_information->size; i++) { > + struct ofono_call *call = l_new(struct ofono_call, 1); > + struct qmi_voice_call_information_instance call_info; > + const struct qmi_voice_remote_party_number_instance > + *remote_party = remote_party_number_inst[i]; > + int number_size; > + > + call_info = call_information->instance[i]; > + > + call->id = call_info.id; > + call->direction = qmi_to_ofono_direction(call_info.direction); > + call->type = 0; /* always voice */ > + > + number_size = remote_party->number_size + 1; > + if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH) > + number_size = OFONO_MAX_PHONE_NUMBER_LENGTH; > + > + l_strlcpy(call->phone_number.number, remote_party- >number, > + number_size); > + > + if (strlen(call->phone_number.number) > 0) > + call->clip_validity = 0; > + else > + call->clip_validity = 2; > + > + if (qmi_to_ofono_status(call_info.state, &call->status)) { > + DBG("Ignore call id %d, because can not convert QMI state 0x%x to > ofono.", + call_info.id, call_info.state); > + l_free(call); > + continue; > + } > + DBG("Call %d in state %s(%d)", call_info.id, > + qmi_voice_call_state_name(call_info.state), > + call_info.state); > + > + l_queue_push_tail(calls, call); > + } > + > + ofono_call_list_notify(vc, calls); > + > + return; > +error: > + l_queue_destroy(calls, l_free); > +} > + > +static void dial_cb(struct qmi_result *result, void *user_data) > +{ > + struct cb_data *cbd = user_data; > + struct ofono_voicecall *vc = cbd->user; > + ofono_voicecall_cb_t cb = cbd->cb; > + uint16_t error; > + uint8_t call_id; > + > + static const uint8_t QMI_VOICE_DIAL_RESULT_CALL_ID = 0x10; > + > + DBG(""); > + > + if (qmi_result_set_error(result, &error)) { > + DBG("QMI Error %d", error); > + CALLBACK_WITH_FAILURE(cb, cbd->data); > + return; > + } > + > + if (!qmi_result_get_uint8(result, QMI_VOICE_DIAL_RESULT_CALL_ID, > + &call_id)) { > + ofono_error("No call id in dial result"); > + CALLBACK_WITH_FAILURE(cb, cbd->data); > + return; > + } > + > + DBG("New call QMI id %d", call_id); > + ofono_call_list_dial_callback(vc, call_id); > + > + CALLBACK_WITH_SUCCESS(cb, cbd->data); > +} > + > +static void dial(struct ofono_voicecall *vc, > + const struct ofono_phone_number *ph, > + enum ofono_clir_option clir, ofono_voicecall_cb_t cb, > + void *data) > +{ > + struct voicecall_data *vd = ofono_voicecall_get_data(vc); > + struct cb_data *cbd = cb_data_new(cb, data); > + struct qmi_param *param; > + const char *calling_number = phone_number_to_string(ph); > + > + static const uint8_t QMI_VOICE_DIAL_CALL_NUMBER = 0x01; > + static const uint8_t QMI_VOICE_DIAL_CALL_TYPE = 0x10; > + static const uint8_t QMI_VOICE_CALL_TYPE_VOICE = 0x00; > + > + DBG(""); > + > + cbd->user = vc; > + memcpy(&vd->dialed, ph, sizeof(*ph)); > + > + param = qmi_param_new(); > + if (!param) > + goto error; > + > + if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER, > + strlen(calling_number), calling_number)) > + goto error; > + > + qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE, > + QMI_VOICE_CALL_TYPE_VOICE); > + > + if (qmi_service_send(vd->voice, QMI_VOICE_DIAL_CALL, param, dial_cb, > + cbd, l_free) > 0) > + return; > + > +error: > + CALLBACK_WITH_FAILURE(cb, data); > + l_free(cbd); > + l_free(param); > +} > + > static void create_voice_cb(struct qmi_service *service, void *user_data) > { > struct ofono_voicecall *vc = user_data; > @@ -58,6 +486,9 @@ static void create_voice_cb(struct qmi_service *service, > void *user_data) > > data->voice = qmi_service_ref(service); > > + qmi_service_register(data->voice, QMI_VOICE_ALL_CALL_STATUS_IND, > + all_call_status_ind, vc, NULL); > + > ofono_voicecall_register(vc); > } > > @@ -70,6 +501,7 @@ static int qmi_voicecall_probe(struct ofono_voicecall > *vc, DBG(""); > > data = l_new(struct voicecall_data, 1); > + data->call_list = l_queue_new(); > > ofono_voicecall_set_data(vc, data); > > @@ -77,7 +509,6 @@ static int qmi_voicecall_probe(struct ofono_voicecall > *vc, create_voice_cb, vc, NULL); > > return 0; > - > } > > static void qmi_voicecall_remove(struct ofono_voicecall *vc) > @@ -92,12 +523,14 @@ static void qmi_voicecall_remove(struct ofono_voicecall > *vc) > > qmi_service_unref(data->voice); > > + l_queue_destroy(data->call_list, l_free); > l_free(data); > } > > static const struct ofono_voicecall_driver driver = { > .probe = qmi_voicecall_probe, > .remove = qmi_voicecall_remove, > + .dial = dial, > }; > > OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
Hi Adam, So this is looking really close now, I think we're getting almost to the finish line :) On 4/13/24 16:53, Adam Pigg wrote: > Add voicecall dialling to the qmimodem driver "dialling -> dialing"? > Includes required infratructure and setup of the QMI services "infratructure" -> "infrastructure"? > > Call State Handling > =================== > On initialisation, register the all_call_status_ind callback to be > called for QMI_VOICE_IND_ALL_STATUS. This will handle notificatiof of notificatiof -> notification > call status to the rest of the system > > Dial Handling > ============= > The dial function sets up the parameters for the QMI_VOICE_DIAL_CALL > service. The parameters are the number to be called and the call type, > which is currently hard coded to be QMI_VOICE_CALL_TYPE_VOICE. The > dial_cb callback wi then be called and will receive the call-id. > "wi" -> will > --- > Changes in V4 > -merged qmi_voice_call_status and all_call_status_ind > -several minor structure/formate changes > --- > --- > drivers/qmimodem/voice.h | 17 ++ > drivers/qmimodem/voicecall.c | 435 ++++++++++++++++++++++++++++++++++- > 2 files changed, 451 insertions(+), 1 deletion(-) <snip> > +static int ofono_call_compare(const void *a, const void *b, void *data) > +{ > + const struct ofono_call *ca = a; > + const struct ofono_call *cb = b; > + > + if (ca->id < cb->id) > + return -1; > + > + if (ca->id > cb->id) > + return 1; > + > + return 0; > +} > + > +static bool ofono_call_compare_by_id(const void *a, const void *b) nit: it is a bit confusing to have two functions which both have 'ofono_call_compare' prefix. I like to name l_queue_match_func_t functions with 'match' in the name. Perhaps this one can be called ofono_call_id_matches or ofono_call_match_by_id? > +{ > + const struct ofono_call *call = a; > + unsigned int id = L_PTR_TO_UINT(b); > + > + return (call->id == id); > +} > + <snip> > +static void ofono_call_list_notify(struct ofono_voicecall *vc, > + struct l_queue *calls) > +{ > + struct voicecall_data *vd = ofono_voicecall_get_data(vc); > + struct l_queue *old_calls = vd->call_list; > + struct l_queue *new_calls = calls; > + struct ofono_call *new_call, *old_call; > + const struct l_queue_entry *old_entry, *new_entry; > + uint i; > + > + uint loop_length = > + MAX(l_queue_length(old_calls), l_queue_length(new_calls)); > + > + old_entry = l_queue_get_entries(old_calls); > + new_entry = l_queue_get_entries(new_calls); > + > + for (i = 0; i < loop_length; ++i) { > + old_call = old_entry ? old_entry->data : NULL; > + new_call = new_entry ? new_entry->data : NULL; > + > + if (new_call && new_call->status == CALL_STATUS_DISCONNECTED) { > + ofono_voicecall_disconnected( > + vc, new_call->id, > + OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL); > + You need to update new_entry here to point to the next entry, otherwise you'll have a use-after-free condition when there are multiple calls. Something like: new_entry = new_entry->next; > + l_queue_remove(calls, new_call); Remember, l_queue is just a singly linked list with a tail pointer. So l_queue_remove would be freeing new_entry underneath. > + l_free(new_call); > + continue; > + } > + > + if (old_call && > + (new_call == NULL || (new_call->id > old_call->id))) Looks like an indentation problem here still. Using !new_call would make it fit: if (old_call && (!new_call || (new_call->id > old_call->id))) > + ofono_voicecall_disconnected( > + vc, old_call->id, > + OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL); > + else if (new_call && > + (old_call == NULL || (new_call->id < old_call->id))) { indentation problem here too. > + DBG("Notify new call %d", new_call->id); > + /* new call, signal it */ > + if (new_call->type == 0) > + ofono_voicecall_notify(vc, new_call); > + } else if (memcmp(new_call, old_call, sizeof(*new_call)) && > + new_call->type == 0) > + ofono_voicecall_notify(vc, new_call); > + > + if (old_entry) > + old_entry = old_entry->next; > + if (new_entry) > + new_entry = new_entry->next; > + } > + > + l_queue_destroy(old_calls, l_free); > + vd->call_list = calls; > +} > + <snip> > + > +static void all_call_status_ind(struct qmi_result *result, void *user_data) > +{ > + struct ofono_voicecall *vc = user_data; > + > + int i; > + int offset; > + uint16_t len; > + bool status = true; > + int instance_size; > + const struct qmi_voice_call_information *call_information; > + const struct qmi_voice_remote_party_number *remote_party_number; > + const struct qmi_voice_remote_party_number_instance *remote_party_number_inst[16]; > + struct l_queue *calls = l_queue_new(); > + > + static const uint8_t QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 0x01; > + static const uint8_t QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 0x10; > + static const uint8_t QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 0x10; > + static const uint8_t QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11; Please change the name to RESULT_CALL_STATUS_* and RESULT_CALL_INFO_* > + > + DBG(""); > + > + /* mandatory */ > + call_information = qmi_result_get( > + result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len); > + > + if (!call_information) { > + call_information = qmi_result_get( > + result, QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION, > + &len); > + status = false; > + } > + > + if (!call_information || len < sizeof(call_information->size)) { > + DBG("Parsing of all call status indication failed"); > + goto error; > + } > + > + if (!call_information->size) { > + DBG("No call informations received!"); > + goto error; > + } > + > + if (len != call_information->size * > + sizeof(struct qmi_voice_call_information_instance) + > + sizeof(call_information->size)) { > + DBG("Call information size incorrect"); > + goto error; > + } You could choose to allocate the calls l_queue only after these basic checks are done and replace all the gotos above with return statements. > + > + Nit: No double-empty lines please > + /* mandatory */ > + remote_party_number = qmi_result_get( > + result, > + status ? QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER : > + QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER, > + &len); > + > + if (!remote_party_number) { > + DBG("Unable to retrieve remote numbers"); > + goto error; > + } > + > + /* verify the length */ > + if (len < sizeof(remote_party_number->size)) { > + DBG("Parsing of remote numbers failed"); > + goto error; > + } > + > + /* expect we have valid fields for every call */ > + if (call_information->size != remote_party_number->size) { > + DBG("Not all fields have the same size"); > + goto error; > + } Same here, if l_queue was allocated later, these could be return statements. > + > + /* pull the remote call info into a local array */ > + instance_size = sizeof(struct qmi_voice_remote_party_number_instance); > + > + for (i = 0, offset = sizeof(remote_party_number->size); > + offset <= len && i < 16 && i < remote_party_number->size; > + i++) { > + const struct qmi_voice_remote_party_number_instance *instance; > + > + if (offset == len) > + break; Hmm, would changing the condition in the for loop to 'offset < len' make this if statement redundant? > + > + if (offset + instance_size > len) { > + DBG("Error parsing remote numbers"); > + goto error; > + } > + > + instance = (void *)remote_party_number + offset; > + if (offset + instance_size + instance->number_size > len) { > + DBG("Error parsing remote numbers"); > + goto error; > + } Empty line here please, doc/coding-style.txt item M1 > + remote_party_number_inst[i] = instance; > + offset += > + sizeof(struct qmi_voice_remote_party_number_instance) + > + instance->number_size; > + } The goto statements in the above for {} block could also be switched to returns if the queue was allocated here. > + > + for (i = 0; i < call_information->size; i++) { In the for loop above you have an additional bound for i < 16, but here you don't have this. > + struct ofono_call *call = l_new(struct ofono_call, 1); > + struct qmi_voice_call_information_instance call_info; > + const struct qmi_voice_remote_party_number_instance > + *remote_party = remote_party_number_inst[i]; This could lead to an out-of-bounds access here. > + int number_size; > + > + call_info = call_information->instance[i]; > + > + call->id = call_info.id; > + call->direction = qmi_to_ofono_direction(call_info.direction); > + call->type = 0; /* always voice */ > + > + number_size = remote_party->number_size + 1; Why +1? We can't copy more than remote_party->number_size bytes from remote_party->number, otherwise we read out of bounds ? > + if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH) > + number_size = OFONO_MAX_PHONE_NUMBER_LENGTH; > + > + l_strlcpy(call->phone_number.number, remote_party->number, > + number_size); > + > + if (strlen(call->phone_number.number) > 0) > + call->clip_validity = 0; > + else > + call->clip_validity = 2; > + > + if (qmi_to_ofono_status(call_info.state, &call->status)) { > + DBG("Ignore call id %d, because can not convert QMI state 0x%x to ofono.", > + call_info.id, call_info.state); > + l_free(call); > + continue; > + } empty line please, item M1 > + DBG("Call %d in state %s(%d)", call_info.id, > + qmi_voice_call_state_name(call_info.state), > + call_info.state); > + > + l_queue_push_tail(calls, call); > + } > + > + ofono_call_list_notify(vc, calls); > + > + return; > +error: > + l_queue_destroy(calls, l_free); > +} > + > +static void dial_cb(struct qmi_result *result, void *user_data) > +{ > + struct cb_data *cbd = user_data; > + struct ofono_voicecall *vc = cbd->user; > + ofono_voicecall_cb_t cb = cbd->cb; > + uint16_t error; > + uint8_t call_id; > + > + static const uint8_t QMI_VOICE_DIAL_RESULT_CALL_ID = 0x10; Name this to RESULT_CALL_ID to be consistent with how this is done elsewhere > + > + DBG(""); > + > + if (qmi_result_set_error(result, &error)) { > + DBG("QMI Error %d", error); > + CALLBACK_WITH_FAILURE(cb, cbd->data); > + return; > + } > + > + if (!qmi_result_get_uint8(result, QMI_VOICE_DIAL_RESULT_CALL_ID, > + &call_id)) { > + ofono_error("No call id in dial result"); > + CALLBACK_WITH_FAILURE(cb, cbd->data); > + return; > + } > + > + DBG("New call QMI id %d", call_id); > + ofono_call_list_dial_callback(vc, call_id); > + > + CALLBACK_WITH_SUCCESS(cb, cbd->data); > +} > + > +static void dial(struct ofono_voicecall *vc, > + const struct ofono_phone_number *ph, > + enum ofono_clir_option clir, ofono_voicecall_cb_t cb, > + void *data) > +{ > + struct voicecall_data *vd = ofono_voicecall_get_data(vc); > + struct cb_data *cbd = cb_data_new(cb, data); > + struct qmi_param *param; > + const char *calling_number = phone_number_to_string(ph); > + > + static const uint8_t QMI_VOICE_DIAL_CALL_NUMBER = 0x01; > + static const uint8_t QMI_VOICE_DIAL_CALL_TYPE = 0x10; Name these two PARAM_CALL_NUMBER and PARAM_CALL_TYPE > + static const uint8_t QMI_VOICE_CALL_TYPE_VOICE = 0x00; > + > + DBG(""); > + > + cbd->user = vc; > + memcpy(&vd->dialed, ph, sizeof(*ph)); > + > + param = qmi_param_new(); > + if (!param) > + goto error; qmi_param_new can't fail, this if can be removed > + > + if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER, > + strlen(calling_number), calling_number)) > + goto error; > + > + qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE, > + QMI_VOICE_CALL_TYPE_VOICE); > + > + if (qmi_service_send(vd->voice, QMI_VOICE_DIAL_CALL, param, dial_cb, > + cbd, l_free) > 0) > + return; > + > +error: > + CALLBACK_WITH_FAILURE(cb, data); > + l_free(cbd); > + l_free(param); > +} > + > static void create_voice_cb(struct qmi_service *service, void *user_data) > { > struct ofono_voicecall *vc = user_data; <snip> Regards, -Denis
On Tuesday 16 April 2024 19:16:01 BST Denis Kenzior wrote: > Hi Adam, > > So this is looking really close now, I think we're getting almost to the > finish line :) > > On 4/13/24 16:53, Adam Pigg wrote: > > Add voicecall dialling to the qmimodem driver > > "dialling -> dialing"? > > > Includes required infratructure and setup of the QMI services > > "infratructure" -> "infrastructure"? > > > Call State Handling > > =================== > > On initialisation, register the all_call_status_ind callback to be > > called for QMI_VOICE_IND_ALL_STATUS. This will handle notificatiof of > > notificatiof -> notification > > > call status to the rest of the system > > > > Dial Handling > > ============= > > The dial function sets up the parameters for the QMI_VOICE_DIAL_CALL > > service. The parameters are the number to be called and the call type, > > which is currently hard coded to be QMI_VOICE_CALL_TYPE_VOICE. The > > dial_cb callback wi then be called and will receive the call-id. > > "wi" -> will > > > --- > > Changes in V4 > > -merged qmi_voice_call_status and all_call_status_ind > > -several minor structure/formate changes > > --- > > --- > > > > drivers/qmimodem/voice.h | 17 ++ > > drivers/qmimodem/voicecall.c | 435 ++++++++++++++++++++++++++++++++++- > > 2 files changed, 451 insertions(+), 1 deletion(-) > > <snip> > > > +static int ofono_call_compare(const void *a, const void *b, void *data) > > +{ > > + const struct ofono_call *ca = a; > > + const struct ofono_call *cb = b; > > + > > + if (ca->id < cb->id) > > + return -1; > > + > > + if (ca->id > cb->id) > > + return 1; > > + > > + return 0; > > +} > > + > > +static bool ofono_call_compare_by_id(const void *a, const void *b) > > nit: it is a bit confusing to have two functions which both have > 'ofono_call_compare' prefix. I like to name l_queue_match_func_t functions > with 'match' in the name. Perhaps this one can be called > ofono_call_id_matches or ofono_call_match_by_id? > > > +{ > > + const struct ofono_call *call = a; > > + unsigned int id = L_PTR_TO_UINT(b); > > + > > + return (call->id == id); > > +} > > + > > <snip> > > > +static void ofono_call_list_notify(struct ofono_voicecall *vc, > > + struct l_queue *calls) > > +{ > > + struct voicecall_data *vd = ofono_voicecall_get_data(vc); > > + struct l_queue *old_calls = vd->call_list; > > + struct l_queue *new_calls = calls; > > + struct ofono_call *new_call, *old_call; > > + const struct l_queue_entry *old_entry, *new_entry; > > + uint i; > > + > > + uint loop_length = > > + MAX(l_queue_length(old_calls), l_queue_length(new_calls)); > > + > > + old_entry = l_queue_get_entries(old_calls); > > + new_entry = l_queue_get_entries(new_calls); > > + > > + for (i = 0; i < loop_length; ++i) { > > + old_call = old_entry ? old_entry->data : NULL; > > + new_call = new_entry ? new_entry->data : NULL; > > + > > + if (new_call && new_call->status == CALL_STATUS_DISCONNECTED) { > > + ofono_voicecall_disconnected( > > + vc, new_call->id, > > + OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL); > > + > > You need to update new_entry here to point to the next entry, otherwise > you'll have a use-after-free condition when there are multiple calls. > Something like: new_entry = new_entry->next; > > > + l_queue_remove(calls, new_call); > > Remember, l_queue is just a singly linked list with a tail pointer. So > l_queue_remove would be freeing new_entry underneath. > > > + l_free(new_call); > > + continue; > > + } > > + > > + if (old_call && > > + (new_call == NULL || (new_call->id > old_call- >id))) > > Looks like an indentation problem here still. Using !new_call would make it > fit: if (old_call && (!new_call || (new_call->id > old_call->id))) > > + ofono_voicecall_disconnected( > > + vc, old_call->id, > > + OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL); > > + else if (new_call && > > + (old_call == NULL || (new_call->id < old_call->id))) { > > indentation problem here too. > > > + DBG("Notify new call %d", new_call->id); > > + /* new call, signal it */ > > + if (new_call->type == 0) > > + ofono_voicecall_notify(vc, new_call); > > + } else if (memcmp(new_call, old_call, sizeof(*new_call)) && > > + new_call->type == 0) > > + ofono_voicecall_notify(vc, new_call); > > + > > + if (old_entry) > > + old_entry = old_entry->next; > > + if (new_entry) > > + new_entry = new_entry->next; > > + } > > + > > + l_queue_destroy(old_calls, l_free); > > + vd->call_list = calls; > > +} > > + > > <snip> > > > + > > +static void all_call_status_ind(struct qmi_result *result, void > > *user_data) +{ > > + struct ofono_voicecall *vc = user_data; > > + > > + int i; > > + int offset; > > + uint16_t len; > > + bool status = true; > > + int instance_size; > > + const struct qmi_voice_call_information *call_information; > > + const struct qmi_voice_remote_party_number *remote_party_number; > > + const struct qmi_voice_remote_party_number_instance > > *remote_party_number_inst[16]; + struct l_queue *calls = l_queue_new(); > > + > > + static const uint8_t QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 0x01; > > + static const uint8_t QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 0x10; > > + static const uint8_t QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 0x10; > > + static const uint8_t QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11; > > Please change the name to RESULT_CALL_STATUS_* and RESULT_CALL_INFO_* > > > + > > + DBG(""); > > + > > + /* mandatory */ > > + call_information = qmi_result_get( > > + result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len); > > + > > + if (!call_information) { > > + call_information = qmi_result_get( > > + result, QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION, > > + &len); > > + status = false; > > + } > > + > > + if (!call_information || len < sizeof(call_information->size)) { > > + DBG("Parsing of all call status indication failed"); > > + goto error; > > + } > > + > > + if (!call_information->size) { > > + DBG("No call informations received!"); > > + goto error; > > + } > > + > > + if (len != call_information->size * > > + sizeof(struct qmi_voice_call_information_instance) + > > + sizeof(call_information->size)) { > > + DBG("Call information size incorrect"); > > + goto error; > > + } > > You could choose to allocate the calls l_queue only after these basic checks > are done and replace all the gotos above with return statements. > > > + > > + > > Nit: No double-empty lines please > > > + /* mandatory */ > > + remote_party_number = qmi_result_get( > > + result, > > + status ? QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER : > > + QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER, > > + &len); > > + > > + if (!remote_party_number) { > > + DBG("Unable to retrieve remote numbers"); > > + goto error; > > + } > > + > > + /* verify the length */ > > + if (len < sizeof(remote_party_number->size)) { > > + DBG("Parsing of remote numbers failed"); > > + goto error; > > + } > > + > > + /* expect we have valid fields for every call */ > > + if (call_information->size != remote_party_number->size) { > > + DBG("Not all fields have the same size"); > > + goto error; > > + } > > Same here, if l_queue was allocated later, these could be return statements. > > + > > + /* pull the remote call info into a local array */ > > + instance_size = sizeof(struct qmi_voice_remote_party_number_instance); > > + > > + for (i = 0, offset = sizeof(remote_party_number->size); > > + offset <= len && i < 16 && i < remote_party_number->size; > > + i++) { > > + const struct qmi_voice_remote_party_number_instance *instance; > > + > > + if (offset == len) > > + break; > > Hmm, would changing the condition in the for loop to 'offset < len' make > this if statement redundant? > > > + > > + if (offset + instance_size > len) { > > + DBG("Error parsing remote numbers"); > > + goto error; > > + } > > + > > + instance = (void *)remote_party_number + offset; > > + if (offset + instance_size + instance->number_size > len) { > > + DBG("Error parsing remote numbers"); > > + goto error; > > + } > > Empty line here please, doc/coding-style.txt item M1 > > > + remote_party_number_inst[i] = instance; > > + offset += > > + sizeof(struct qmi_voice_remote_party_number_instance) + > > + instance->number_size; > > + } > > The goto statements in the above for {} block could also be switched to > returns if the queue was allocated here. > > > + > > + for (i = 0; i < call_information->size; i++) { > > In the for loop above you have an additional bound for i < 16, but here you > don't have this. > > > + struct ofono_call *call = l_new(struct ofono_call, 1); > > + struct qmi_voice_call_information_instance call_info; > > + const struct qmi_voice_remote_party_number_instance > > + *remote_party = remote_party_number_inst[i]; > > This could lead to an out-of-bounds access here. > > > + int number_size; > > + > > + call_info = call_information->instance[i]; > > + > > + call->id = call_info.id; > > + call->direction = qmi_to_ofono_direction(call_info.direction); > > + call->type = 0; /* always voice */ > > + > > + number_size = remote_party->number_size + 1; > > Why +1? We can't copy more than remote_party->number_size bytes from > remote_party->number, otherwise we read out of bounds ? In my testing I found that the last digit of the number was being missed. This fixed it, though I suspected it may come back to haunt me! Any suggestions? > > > + if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH) > > + number_size = OFONO_MAX_PHONE_NUMBER_LENGTH; > > + > > + l_strlcpy(call->phone_number.number, remote_party- >number, > > + number_size); > > + > > + if (strlen(call->phone_number.number) > 0) > > + call->clip_validity = 0; > > + else > > + call->clip_validity = 2; > > + > > + if (qmi_to_ofono_status(call_info.state, &call- >status)) { > > + DBG("Ignore call id %d, because can not convert QMI state 0x%x to > > ofono.", + call_info.id, call_info.state); > > + l_free(call); > > + continue; > > + } > > empty line please, item M1 > > > + DBG("Call %d in state %s(%d)", call_info.id, > > + qmi_voice_call_state_name(call_info.state), > > + call_info.state); > > + > > + l_queue_push_tail(calls, call); > > + } > > + > > + ofono_call_list_notify(vc, calls); > > + > > + return; > > +error: > > + l_queue_destroy(calls, l_free); > > +} > > + > > +static void dial_cb(struct qmi_result *result, void *user_data) > > +{ > > + struct cb_data *cbd = user_data; > > + struct ofono_voicecall *vc = cbd->user; > > + ofono_voicecall_cb_t cb = cbd->cb; > > + uint16_t error; > > + uint8_t call_id; > > + > > + static const uint8_t QMI_VOICE_DIAL_RESULT_CALL_ID = 0x10; > > Name this to RESULT_CALL_ID to be consistent with how this is done elsewhere > > + > > + DBG(""); > > + > > + if (qmi_result_set_error(result, &error)) { > > + DBG("QMI Error %d", error); > > + CALLBACK_WITH_FAILURE(cb, cbd->data); > > + return; > > + } > > + > > + if (!qmi_result_get_uint8(result, QMI_VOICE_DIAL_RESULT_CALL_ID, > > + &call_id)) { > > + ofono_error("No call id in dial result"); > > + CALLBACK_WITH_FAILURE(cb, cbd->data); > > + return; > > + } > > + > > + DBG("New call QMI id %d", call_id); > > + ofono_call_list_dial_callback(vc, call_id); > > + > > + CALLBACK_WITH_SUCCESS(cb, cbd->data); > > +} > > + > > +static void dial(struct ofono_voicecall *vc, > > + const struct ofono_phone_number *ph, > > + enum ofono_clir_option clir, ofono_voicecall_cb_t cb, > > + void *data) > > +{ > > + struct voicecall_data *vd = ofono_voicecall_get_data(vc); > > + struct cb_data *cbd = cb_data_new(cb, data); > > + struct qmi_param *param; > > + const char *calling_number = phone_number_to_string(ph); > > + > > + static const uint8_t QMI_VOICE_DIAL_CALL_NUMBER = 0x01; > > + static const uint8_t QMI_VOICE_DIAL_CALL_TYPE = 0x10; > > Name these two PARAM_CALL_NUMBER and PARAM_CALL_TYPE > > > + static const uint8_t QMI_VOICE_CALL_TYPE_VOICE = 0x00; > > + > > + DBG(""); > > + > > + cbd->user = vc; > > + memcpy(&vd->dialed, ph, sizeof(*ph)); > > + > > + param = qmi_param_new(); > > + if (!param) > > + goto error; > > qmi_param_new can't fail, this if can be removed > > > + > > + if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER, > > + strlen(calling_number), calling_number)) > > + goto error; > > + > > + qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE, > > + QMI_VOICE_CALL_TYPE_VOICE); > > + > > + if (qmi_service_send(vd->voice, QMI_VOICE_DIAL_CALL, param, dial_cb, > > + cbd, l_free) > 0) > > + return; > > + > > +error: > > + CALLBACK_WITH_FAILURE(cb, data); > > + l_free(cbd); > > + l_free(param); > > +} > > + > > > > static void create_voice_cb(struct qmi_service *service, void > > *user_data) > > { > > > > struct ofono_voicecall *vc = user_data; > > <snip> > > Regards, > -Denis
Hi Adam, >>> + number_size = remote_party->number_size + 1; >> >> Why +1? We can't copy more than remote_party->number_size bytes from >> remote_party->number, otherwise we read out of bounds ? > In my testing I found that the last digit of the number was being missed. > This fixed it, though I suspected it may come back to haunt me! Any > suggestions? l_strlcpy takes 3 parameters: destination source size of the destination in bytes So strictly speaking, l_strlcpy should be invoked as: l_strlcpy(call->phone_number.number, remote_party->number, sizeof(call->phone_number.number)); l_strlcpy ensures that if strlen(src) is bigger than the destination buffer, the result is truncated, but the destination buffer is still '\0' terminated. But here's the problem, in QMI, strings are not NULL terminated, so you're just getting lucky that remote_party->number has a '\0' somewhere at the end. What you need to do is copy at most remote_party->size bytes into a buffer of sizeof(call->phone_number.number) and make sure the buffer is still '\0' terminated. You can do this using MAX(), memcpy and memset, or the far easier to understand: char *tmp = l_strndup(remove_party->number, remote_party->size); l_strlcpy(call->phone_number.number, tmp, sizeof(call->phone_number.number)); l_free(tmp); Regards, -Denis
Hi Denis, >>>> + number_size = remote_party->number_size + 1; >>> >>> Why +1? We can't copy more than remote_party->number_size bytes from >>> remote_party->number, otherwise we read out of bounds ? >> In my testing I found that the last digit of the number was being missed. >> This fixed it, though I suspected it may come back to haunt me! Any >> suggestions? > > l_strlcpy takes 3 parameters: > destination > source > size of the destination in bytes > > So strictly speaking, l_strlcpy should be invoked as: > l_strlcpy(call->phone_number.number, > remote_party->number, > sizeof(call->phone_number.number)); > > l_strlcpy ensures that if strlen(src) is bigger than the destination buffer, the result is truncated, but the destination buffer is still '\0' terminated. > > But here's the problem, in QMI, strings are not NULL terminated, so you're just getting lucky that remote_party->number has a '\0' somewhere at the end. What you need to do is copy at most remote_party->size bytes into a buffer of sizeof(call->phone_number.number) and make sure the buffer is still '\0' terminated. > > You can do this using MAX(), memcpy and memset, or the far easier to understand: > > char *tmp = l_strndup(remove_party->number, remote_party->size); > > l_strlcpy(call->phone_number.number, tmp, sizeof(call->phone_number.number)); > l_free(tmp); if phone_number.number is a fixed size buffer, wouldn’t you just use snprintf instead. Or just use strncpy/stpncpy with a buffer size - 1. Regards Marcel
Hi Marcel, > > if phone_number.number is a fixed size buffer, wouldn’t you just use snprintf > instead. Or just use strncpy/stpncpy with a buffer size - 1. No, because the source is not guaranteed to be null terminated. That is why it has a size parameter. Here's a good overview of all the most popular string copying methods and their advantages / disadvantages. https://lwn.net/Articles/905777/ What is really needed is a function that takes both src and dst buffer sizes into consideration. I'm not aware of one. Regards, -Denis
Hi Denis, >> if phone_number.number is a fixed size buffer, wouldn’t you just use snprintf >> instead. Or just use strncpy/stpncpy with a buffer size - 1. > > No, because the source is not guaranteed to be null terminated. That is why it has a size parameter. > > Here's a good overview of all the most popular string copying methods and their advantages / disadvantages. > > https://lwn.net/Articles/905777/ > > What is really needed is a function that takes both src and dst buffer sizes into consideration. I'm not aware of one. strncpy/stpncpy both set the dest to zero, but you are right, it will be still off by one. So best is to just zero out the dest buffer with memset first and then just copy dest size - 1 at max. Then you get your \0 in case of truncation. Regards Marcel
Hi Marcel, >> What is really needed is a function that takes both src and dst buffer sizes into consideration. I'm not aware of one. > > strncpy/stpncpy both set the dest to zero, but you are right, it will be > still off by one. So best is to just zero out the dest buffer with memset > first and then just copy dest size - 1 at max. Then you get your \0 in > case of truncation. Yes, strncpy can work. You'd need to take a min(dest size - 1, source size), strncpy and null terminate. Verbose and not super readable. Regards, -Denis
diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h index 917e72f7..2344fd50 100644 --- a/drivers/qmimodem/voice.h +++ b/drivers/qmimodem/voice.h @@ -48,6 +48,9 @@ enum qmi_ussd_user_required { /* QMI service voice. Using an enum to prevent doublicated entries */ enum voice_commands { + QMI_VOICE_DIAL_CALL = 0x20, + QMI_VOICE_ALL_CALL_STATUS_IND = 0x2e, + QMI_VOICE_GET_ALL_CALL_INFO = 0x2f, QMI_VOICE_SUPS_NOTIFICATION_IND = 0x32, QMI_VOICE_SET_SUPS_SERVICE = 0x33, QMI_VOICE_GET_CALL_WAITING = 0x34, @@ -66,6 +69,20 @@ enum voice_commands { QMI_VOICE_GET_CNAP = 0x4d }; +enum qmi_voice_call_state { + QMI_VOICE_CALL_STATE_IDLE = 0x0, + QMI_VOICE_CALL_STATE_ORIG, + QMI_VOICE_CALL_STATE_INCOMING, + QMI_VOICE_CALL_STATE_CONV, + QMI_VOICE_CALL_STATE_CC_IN_PROG, + QMI_VOICE_CALL_STATE_ALERTING, + QMI_VOICE_CALL_STATE_HOLD, + QMI_VOICE_CALL_STATE_WAITING, + QMI_VOICE_CALL_STATE_DISCONNECTING, + QMI_VOICE_CALL_STATE_END, + QMI_VOICE_CALL_STATE_SETUP +}; + struct qmi_ussd_data { uint8_t dcs; uint8_t length; diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c index aa34fc25..8c86eee7 100644 --- a/drivers/qmimodem/voicecall.c +++ b/drivers/qmimodem/voicecall.c @@ -3,6 +3,7 @@ * oFono - Open Source Telephony * * Copyright (C) 2011-2012 Intel Corporation. All rights reserved. + * Copyright (C) 2024 Adam Pigg <adam@piggz.co.uk> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -26,6 +27,10 @@ #include <ofono/log.h> #include <ofono/modem.h> #include <ofono/voicecall.h> +#include <src/common.h> +#include <ell/ell.h> + +#include "voice.h" #include "qmi.h" @@ -35,8 +40,431 @@ struct voicecall_data { struct qmi_service *voice; uint16_t major; uint16_t minor; + struct l_queue *call_list; + struct ofono_phone_number dialed; }; +struct qmi_voice_call_information_instance { + uint8_t id; + uint8_t state; + uint8_t type; + uint8_t direction; + uint8_t mode; + uint8_t multipart_indicator; + uint8_t als; +} __attribute__((__packed__)); + +struct qmi_voice_call_information { + uint8_t size; + struct qmi_voice_call_information_instance instance[0]; +} __attribute__((__packed__)); + +struct qmi_voice_remote_party_number_instance { + uint8_t call_id; + uint8_t presentation_indicator; + uint8_t number_size; + char number[0]; +} __attribute__((__packed__)); + +struct qmi_voice_remote_party_number { + uint8_t size; + struct qmi_voice_remote_party_number_instance instance[0]; +} __attribute__((__packed__)); + +static int ofono_call_compare(const void *a, const void *b, void *data) +{ + const struct ofono_call *ca = a; + const struct ofono_call *cb = b; + + if (ca->id < cb->id) + return -1; + + if (ca->id > cb->id) + return 1; + + return 0; +} + +static bool ofono_call_compare_by_id(const void *a, const void *b) +{ + const struct ofono_call *call = a; + unsigned int id = L_PTR_TO_UINT(b); + + return (call->id == id); +} + +static void ofono_call_list_dial_callback(struct ofono_voicecall *vc, + int call_id) +{ + struct ofono_call *call; + struct voicecall_data *vd = ofono_voicecall_get_data(vc); + struct l_queue *call_list = vd->call_list; + const struct ofono_phone_number *ph = &vd->dialed; + + /* check if call_id already present */ + call = l_queue_find(call_list, ofono_call_compare_by_id, + L_UINT_TO_PTR(call_id)); + + if (call) + return; + + call = l_new(struct ofono_call, 1); + call->id = call_id; + + memcpy(&call->called_number, ph, sizeof(*ph)); + call->direction = CALL_DIRECTION_MOBILE_ORIGINATED; + call->status = CALL_STATUS_DIALING; + call->type = 0; /* voice */ + + l_queue_insert(call_list, call, ofono_call_compare, NULL); + + ofono_voicecall_notify(vc, call); +} + +static void ofono_call_list_notify(struct ofono_voicecall *vc, + struct l_queue *calls) +{ + struct voicecall_data *vd = ofono_voicecall_get_data(vc); + struct l_queue *old_calls = vd->call_list; + struct l_queue *new_calls = calls; + struct ofono_call *new_call, *old_call; + const struct l_queue_entry *old_entry, *new_entry; + uint i; + + uint loop_length = + MAX(l_queue_length(old_calls), l_queue_length(new_calls)); + + old_entry = l_queue_get_entries(old_calls); + new_entry = l_queue_get_entries(new_calls); + + for (i = 0; i < loop_length; ++i) { + old_call = old_entry ? old_entry->data : NULL; + new_call = new_entry ? new_entry->data : NULL; + + if (new_call && new_call->status == CALL_STATUS_DISCONNECTED) { + ofono_voicecall_disconnected( + vc, new_call->id, + OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL); + + l_queue_remove(calls, new_call); + l_free(new_call); + continue; + } + + if (old_call && + (new_call == NULL || (new_call->id > old_call->id))) + ofono_voicecall_disconnected( + vc, old_call->id, + OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL); + else if (new_call && + (old_call == NULL || (new_call->id < old_call->id))) { + DBG("Notify new call %d", new_call->id); + /* new call, signal it */ + if (new_call->type == 0) + ofono_voicecall_notify(vc, new_call); + } else if (memcmp(new_call, old_call, sizeof(*new_call)) && + new_call->type == 0) + ofono_voicecall_notify(vc, new_call); + + if (old_entry) + old_entry = old_entry->next; + if (new_entry) + new_entry = new_entry->next; + } + + l_queue_destroy(old_calls, l_free); + vd->call_list = calls; +} + +static const char *qmi_voice_call_state_name(enum qmi_voice_call_state value) +{ + switch (value) { + case QMI_VOICE_CALL_STATE_IDLE: + return "QMI_VOICE_CALL_STATE_IDLE"; + case QMI_VOICE_CALL_STATE_ORIG: + return "QMI_VOICE_CALL_STATE_ORIG"; + case QMI_VOICE_CALL_STATE_INCOMING: + return "QMI_VOICE_CALL_STATE_INCOMING"; + case QMI_VOICE_CALL_STATE_CONV: + return "QMI_VOICE_CALL_STATE_CONV"; + case QMI_VOICE_CALL_STATE_CC_IN_PROG: + return "QMI_VOICE_CALL_STATE_CC_IN_PROG"; + case QMI_VOICE_CALL_STATE_ALERTING: + return "QMI_VOICE_CALL_STATE_ALERTING"; + case QMI_VOICE_CALL_STATE_HOLD: + return "QMI_VOICE_CALL_STATE_HOLD"; + case QMI_VOICE_CALL_STATE_WAITING: + return "QMI_VOICE_CALL_STATE_WAITING"; + case QMI_VOICE_CALL_STATE_DISCONNECTING: + return "QMI_VOICE_CALL_STATE_DISCONNECTING"; + case QMI_VOICE_CALL_STATE_END: + return "QMI_VOICE_CALL_STATE_END"; + case QMI_VOICE_CALL_STATE_SETUP: + return "QMI_VOICE_CALL_STATE_SETUP"; + } + return "QMI_CALL_STATE_<UNKNOWN>"; +} + +static bool qmi_to_ofono_status(uint8_t status, int *ret) +{ + int err = false; + + switch (status) { + case QMI_VOICE_CALL_STATE_IDLE: + case QMI_VOICE_CALL_STATE_END: + case QMI_VOICE_CALL_STATE_DISCONNECTING: + *ret = CALL_STATUS_DISCONNECTED; + break; + case QMI_VOICE_CALL_STATE_HOLD: + *ret = CALL_STATUS_HELD; + break; + case QMI_VOICE_CALL_STATE_WAITING: + *ret = CALL_STATUS_WAITING; + break; + case QMI_VOICE_CALL_STATE_ORIG: + *ret = CALL_STATUS_DIALING; + break; + case QMI_VOICE_CALL_STATE_SETUP: + case QMI_VOICE_CALL_STATE_INCOMING: + *ret = CALL_STATUS_INCOMING; + break; + case QMI_VOICE_CALL_STATE_CONV: + *ret = CALL_STATUS_ACTIVE; + break; + case QMI_VOICE_CALL_STATE_CC_IN_PROG: + *ret = CALL_STATUS_DIALING; + break; + case QMI_VOICE_CALL_STATE_ALERTING: + *ret = CALL_STATUS_ALERTING; + break; + default: + err = true; + } + return err; +} + +static enum call_direction qmi_to_ofono_direction(uint8_t qmi_direction) +{ + return qmi_direction - 1; +} + +static void all_call_status_ind(struct qmi_result *result, void *user_data) +{ + struct ofono_voicecall *vc = user_data; + + int i; + int offset; + uint16_t len; + bool status = true; + int instance_size; + const struct qmi_voice_call_information *call_information; + const struct qmi_voice_remote_party_number *remote_party_number; + const struct qmi_voice_remote_party_number_instance *remote_party_number_inst[16]; + struct l_queue *calls = l_queue_new(); + + static const uint8_t QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 0x01; + static const uint8_t QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 0x10; + static const uint8_t QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 0x10; + static const uint8_t QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11; + + DBG(""); + + /* mandatory */ + call_information = qmi_result_get( + result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len); + + if (!call_information) { + call_information = qmi_result_get( + result, QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION, + &len); + status = false; + } + + if (!call_information || len < sizeof(call_information->size)) { + DBG("Parsing of all call status indication failed"); + goto error; + } + + if (!call_information->size) { + DBG("No call informations received!"); + goto error; + } + + if (len != call_information->size * + sizeof(struct qmi_voice_call_information_instance) + + sizeof(call_information->size)) { + DBG("Call information size incorrect"); + goto error; + } + + + /* mandatory */ + remote_party_number = qmi_result_get( + result, + status ? QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER : + QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER, + &len); + + if (!remote_party_number) { + DBG("Unable to retrieve remote numbers"); + goto error; + } + + /* verify the length */ + if (len < sizeof(remote_party_number->size)) { + DBG("Parsing of remote numbers failed"); + goto error; + } + + /* expect we have valid fields for every call */ + if (call_information->size != remote_party_number->size) { + DBG("Not all fields have the same size"); + goto error; + } + + /* pull the remote call info into a local array */ + instance_size = sizeof(struct qmi_voice_remote_party_number_instance); + + for (i = 0, offset = sizeof(remote_party_number->size); + offset <= len && i < 16 && i < remote_party_number->size; + i++) { + const struct qmi_voice_remote_party_number_instance *instance; + + if (offset == len) + break; + + if (offset + instance_size > len) { + DBG("Error parsing remote numbers"); + goto error; + } + + instance = (void *)remote_party_number + offset; + if (offset + instance_size + instance->number_size > len) { + DBG("Error parsing remote numbers"); + goto error; + } + remote_party_number_inst[i] = instance; + offset += + sizeof(struct qmi_voice_remote_party_number_instance) + + instance->number_size; + } + + for (i = 0; i < call_information->size; i++) { + struct ofono_call *call = l_new(struct ofono_call, 1); + struct qmi_voice_call_information_instance call_info; + const struct qmi_voice_remote_party_number_instance + *remote_party = remote_party_number_inst[i]; + int number_size; + + call_info = call_information->instance[i]; + + call->id = call_info.id; + call->direction = qmi_to_ofono_direction(call_info.direction); + call->type = 0; /* always voice */ + + number_size = remote_party->number_size + 1; + if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH) + number_size = OFONO_MAX_PHONE_NUMBER_LENGTH; + + l_strlcpy(call->phone_number.number, remote_party->number, + number_size); + + if (strlen(call->phone_number.number) > 0) + call->clip_validity = 0; + else + call->clip_validity = 2; + + if (qmi_to_ofono_status(call_info.state, &call->status)) { + DBG("Ignore call id %d, because can not convert QMI state 0x%x to ofono.", + call_info.id, call_info.state); + l_free(call); + continue; + } + DBG("Call %d in state %s(%d)", call_info.id, + qmi_voice_call_state_name(call_info.state), + call_info.state); + + l_queue_push_tail(calls, call); + } + + ofono_call_list_notify(vc, calls); + + return; +error: + l_queue_destroy(calls, l_free); +} + +static void dial_cb(struct qmi_result *result, void *user_data) +{ + struct cb_data *cbd = user_data; + struct ofono_voicecall *vc = cbd->user; + ofono_voicecall_cb_t cb = cbd->cb; + uint16_t error; + uint8_t call_id; + + static const uint8_t QMI_VOICE_DIAL_RESULT_CALL_ID = 0x10; + + DBG(""); + + if (qmi_result_set_error(result, &error)) { + DBG("QMI Error %d", error); + CALLBACK_WITH_FAILURE(cb, cbd->data); + return; + } + + if (!qmi_result_get_uint8(result, QMI_VOICE_DIAL_RESULT_CALL_ID, + &call_id)) { + ofono_error("No call id in dial result"); + CALLBACK_WITH_FAILURE(cb, cbd->data); + return; + } + + DBG("New call QMI id %d", call_id); + ofono_call_list_dial_callback(vc, call_id); + + CALLBACK_WITH_SUCCESS(cb, cbd->data); +} + +static void dial(struct ofono_voicecall *vc, + const struct ofono_phone_number *ph, + enum ofono_clir_option clir, ofono_voicecall_cb_t cb, + void *data) +{ + struct voicecall_data *vd = ofono_voicecall_get_data(vc); + struct cb_data *cbd = cb_data_new(cb, data); + struct qmi_param *param; + const char *calling_number = phone_number_to_string(ph); + + static const uint8_t QMI_VOICE_DIAL_CALL_NUMBER = 0x01; + static const uint8_t QMI_VOICE_DIAL_CALL_TYPE = 0x10; + static const uint8_t QMI_VOICE_CALL_TYPE_VOICE = 0x00; + + DBG(""); + + cbd->user = vc; + memcpy(&vd->dialed, ph, sizeof(*ph)); + + param = qmi_param_new(); + if (!param) + goto error; + + if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER, + strlen(calling_number), calling_number)) + goto error; + + qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE, + QMI_VOICE_CALL_TYPE_VOICE); + + if (qmi_service_send(vd->voice, QMI_VOICE_DIAL_CALL, param, dial_cb, + cbd, l_free) > 0) + return; + +error: + CALLBACK_WITH_FAILURE(cb, data); + l_free(cbd); + l_free(param); +} + static void create_voice_cb(struct qmi_service *service, void *user_data) { struct ofono_voicecall *vc = user_data; @@ -58,6 +486,9 @@ static void create_voice_cb(struct qmi_service *service, void *user_data) data->voice = qmi_service_ref(service); + qmi_service_register(data->voice, QMI_VOICE_ALL_CALL_STATUS_IND, + all_call_status_ind, vc, NULL); + ofono_voicecall_register(vc); } @@ -70,6 +501,7 @@ static int qmi_voicecall_probe(struct ofono_voicecall *vc, DBG(""); data = l_new(struct voicecall_data, 1); + data->call_list = l_queue_new(); ofono_voicecall_set_data(vc, data); @@ -77,7 +509,6 @@ static int qmi_voicecall_probe(struct ofono_voicecall *vc, create_voice_cb, vc, NULL); return 0; - } static void qmi_voicecall_remove(struct ofono_voicecall *vc) @@ -92,12 +523,14 @@ static void qmi_voicecall_remove(struct ofono_voicecall *vc) qmi_service_unref(data->voice); + l_queue_destroy(data->call_list, l_free); l_free(data); } static const struct ofono_voicecall_driver driver = { .probe = qmi_voicecall_probe, .remove = qmi_voicecall_remove, + .dial = dial, }; OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)