* [PATCH v4 2/4] qmimodem: voicecall: Implement call answer
2024-04-13 21:53 [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing Adam Pigg
@ 2024-04-13 21:53 ` Adam Pigg
2024-04-16 18:19 ` Denis Kenzior
2024-04-13 21:53 ` [PATCH v4 3/4] qmimodem: voicecall: Implement active call hangup Adam Pigg
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Adam Pigg @ 2024-04-13 21:53 UTC (permalink / raw)
To: ofono; +Cc: Adam Pigg
The answer function set up the parameters for a call to the service
function QMI_VOICE_ANSWER_CALL. The only parameter is the call-id.
answer_cb will then be called which retrieves the call-id and checks
the status of the result.
---
Changed in V4
-Minor rework to the answer function
---
---
drivers/qmimodem/voice.h | 1 +
drivers/qmimodem/voicecall.c | 70 ++++++++++++++++++++++++++++++++++++
2 files changed, 71 insertions(+)
diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index 2344fd50..a524cf98 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -51,6 +51,7 @@ enum voice_commands {
QMI_VOICE_DIAL_CALL = 0x20,
QMI_VOICE_ALL_CALL_STATUS_IND = 0x2e,
QMI_VOICE_GET_ALL_CALL_INFO = 0x2f,
+ QMI_VOICE_ANSWER_CALL = 0x22,
QMI_VOICE_SUPS_NOTIFICATION_IND = 0x32,
QMI_VOICE_SET_SUPS_SERVICE = 0x33,
QMI_VOICE_GET_CALL_WAITING = 0x34,
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index 8c86eee7..4b87921e 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -93,6 +93,14 @@ static bool ofono_call_compare_by_id(const void *a, const void *b)
return (call->id == id);
}
+static bool ofono_call_compare_by_status(const void *a, const void *b)
+{
+ const struct ofono_call *call = a;
+ int status = L_PTR_TO_INT(b);
+
+ return status == call->status;
+}
+
static void ofono_call_list_dial_callback(struct ofono_voicecall *vc,
int call_id)
{
@@ -465,6 +473,67 @@ error:
l_free(param);
}
+static void answer_cb(struct qmi_result *result, void *user_data)
+{
+ struct cb_data *cbd = user_data;
+ ofono_voicecall_cb_t cb = cbd->cb;
+ uint16_t error;
+ uint8_t call_id;
+
+ static const uint8_t QMI_VOICE_ANSWER_RETURN_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_ANSWER_RETURN_CALL_ID, &call_id))
+ DBG("Received answer result with call id %d", call_id);
+
+ CALLBACK_WITH_SUCCESS(cb, cbd->data);
+}
+
+static void answer(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb, void *data)
+{
+ struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+ struct cb_data *cbd;
+ struct ofono_call *call;
+ struct qmi_param *param = NULL;
+
+ static const uint8_t QMI_VOICE_ANSWER_CALL_ID = 0x01;
+
+ DBG("");
+
+ call = l_queue_find(vd->call_list,
+ ofono_call_compare_by_status,
+ L_UINT_TO_PTR(CALL_STATUS_INCOMING));
+
+ if (call == NULL) {
+ DBG("Can not find a call to pick up");
+ return;
+ }
+
+ param = qmi_param_new();
+ cbd = cb_data_new(cb, data);
+ cbd->user = vc;
+
+ if (!qmi_param_append_uint8(param, QMI_VOICE_ANSWER_CALL_ID,
+ call->id))
+ goto error;
+
+ if (qmi_service_send(vd->voice, QMI_VOICE_ANSWER_CALL, param,
+ answer_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;
@@ -531,6 +600,7 @@ static const struct ofono_voicecall_driver driver = {
.probe = qmi_voicecall_probe,
.remove = qmi_voicecall_remove,
.dial = dial,
+ .answer = answer,
};
OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
--
2.44.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/4] qmimodem: voicecall: Implement call answer
2024-04-13 21:53 ` [PATCH v4 2/4] qmimodem: voicecall: Implement call answer Adam Pigg
@ 2024-04-16 18:19 ` Denis Kenzior
0 siblings, 0 replies; 15+ messages in thread
From: Denis Kenzior @ 2024-04-16 18:19 UTC (permalink / raw)
To: Adam Pigg, ofono
Hi Adam,
On 4/13/24 16:53, Adam Pigg wrote:
> The answer function set up the parameters for a call to the service
> function QMI_VOICE_ANSWER_CALL. The only parameter is the call-id.
> answer_cb will then be called which retrieves the call-id and checks
> the status of the result.
>
> ---
> Changed in V4
> -Minor rework to the answer function
> ---
> ---
> drivers/qmimodem/voice.h | 1 +
> drivers/qmimodem/voicecall.c | 70 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 71 insertions(+)
>
<snip>
> diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
> index 8c86eee7..4b87921e 100644
> --- a/drivers/qmimodem/voicecall.c
> +++ b/drivers/qmimodem/voicecall.c
> @@ -93,6 +93,14 @@ static bool ofono_call_compare_by_id(const void *a, const void *b)
> return (call->id == id);
> }
>
> +static bool ofono_call_compare_by_status(const void *a, const void *b)
Nit, similarly to my comment in patch 1, lets use 'match' in the name. Perhaps
ofono_call_match_by_status?
> +{
> + const struct ofono_call *call = a;
> + int status = L_PTR_TO_INT(b);
> +
> + return status == call->status;
> +}
> +
> static void ofono_call_list_dial_callback(struct ofono_voicecall *vc,
> int call_id)
> {
> @@ -465,6 +473,67 @@ error:
> l_free(param);
> }
>
> +static void answer_cb(struct qmi_result *result, void *user_data)
> +{
> + struct cb_data *cbd = user_data;
> + ofono_voicecall_cb_t cb = cbd->cb;
> + uint16_t error;
> + uint8_t call_id;
> +
> + static const uint8_t QMI_VOICE_ANSWER_RETURN_CALL_ID = 0x10;
RESULT_CALL_ID
> +
> + 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_ANSWER_RETURN_CALL_ID, &call_id))
> + DBG("Received answer result with call id %d", call_id);
> +
> + CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +}
> +
> +static void answer(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb, void *data)
> +{
> + struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> + struct cb_data *cbd;
> + struct ofono_call *call;
> + struct qmi_param *param = NULL;
> +
> + static const uint8_t QMI_VOICE_ANSWER_CALL_ID = 0x01;
PARAM_CALL_ID
> +
> + DBG("");
> +
> + call = l_queue_find(vd->call_list,
> + ofono_call_compare_by_status,
> + L_UINT_TO_PTR(CALL_STATUS_INCOMING));
> +
> + if (call == NULL) {
> + DBG("Can not find a call to pick up");
> + return;
Hmm, CALLBACK_WITH_FAILURE? And probably warrants an ofono_error()
> + }
> +
> + param = qmi_param_new();
> + cbd = cb_data_new(cb, data);
> + cbd->user = vc;
> +
> + if (!qmi_param_append_uint8(param, QMI_VOICE_ANSWER_CALL_ID,
> + call->id))
> + goto error;
> +
> + if (qmi_service_send(vd->voice, QMI_VOICE_ANSWER_CALL, param,
> + answer_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;
Regards,
-Denis
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 3/4] qmimodem: voicecall: Implement active call hangup
2024-04-13 21:53 [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing Adam Pigg
2024-04-13 21:53 ` [PATCH v4 2/4] qmimodem: voicecall: Implement call answer Adam Pigg
@ 2024-04-13 21:53 ` Adam Pigg
2024-04-16 18:21 ` Denis Kenzior
2024-04-13 21:53 ` [PATCH v4 4/4] qmimodem: voicecall: Implement DTMF tones Adam Pigg
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Adam Pigg @ 2024-04-13 21:53 UTC (permalink / raw)
To: ofono; +Cc: Adam Pigg
hangup_active iterates the current list of calls, looking for the first
active call and then calls release_specific. This then sets up the
parameters for a call to QMI_VOICE_END_CALL, with the only parameters
being the call-id.
end_call_cb will then be called and will parse out the call-id and check
for success.
---
Changes in V4
-Minor rework to hangup_active
-Removed unnescessary changes
---
---
drivers/qmimodem/voice.h | 1 +
drivers/qmimodem/voicecall.c | 82 ++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+)
diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index a524cf98..caedb079 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -51,6 +51,7 @@ enum voice_commands {
QMI_VOICE_DIAL_CALL = 0x20,
QMI_VOICE_ALL_CALL_STATUS_IND = 0x2e,
QMI_VOICE_GET_ALL_CALL_INFO = 0x2f,
+ QMI_VOICE_END_CALL = 0x21,
QMI_VOICE_ANSWER_CALL = 0x22,
QMI_VOICE_SUPS_NOTIFICATION_IND = 0x32,
QMI_VOICE_SET_SUPS_SERVICE = 0x33,
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index 4b87921e..396732ad 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -534,6 +534,86 @@ error:
l_free(param);
}
+static void end_call_cb(struct qmi_result *result, void *user_data)
+{
+ struct cb_data *cbd = user_data;
+ ofono_voicecall_cb_t cb = cbd->cb;
+ uint16_t error;
+ uint8_t call_id;
+
+ static const uint8_t QMI_VOICE_END_RETURN_CALL_ID = 0x10;
+
+ 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_END_RETURN_CALL_ID, &call_id))
+ DBG("Received end call result with call id %d", call_id);
+
+ CALLBACK_WITH_SUCCESS(cb, cbd->data);
+}
+
+static void release_specific(struct ofono_voicecall *vc, int id,
+ ofono_voicecall_cb_t cb, void *data)
+{
+ struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+ struct cb_data *cbd;
+ struct qmi_param *param = NULL;
+
+ static const uint8_t QMI_VOICE_END_CALL_ID = 0x01;
+
+ DBG("");
+
+ param = qmi_param_new();
+ cbd = cb_data_new(cb, data);
+ cbd->user = vc;
+
+ if (!qmi_param_append_uint8(param, QMI_VOICE_END_CALL_ID, id))
+ goto error;
+
+ if (qmi_service_send(vd->voice, QMI_VOICE_END_CALL, param, end_call_cb,
+ cbd, l_free) > 0)
+ return;
+
+error:
+ CALLBACK_WITH_FAILURE(cb, data);
+ l_free(cbd);
+ l_free(param);
+}
+
+static void hangup_active(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
+ void *data)
+{
+ struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+ struct ofono_call *call;
+ enum call_status active[] = {
+ CALL_STATUS_ACTIVE,
+ CALL_STATUS_DIALING,
+ CALL_STATUS_ALERTING,
+ CALL_STATUS_INCOMING,
+ };
+
+ DBG("");
+
+ for (uint32_t i = 0; i < L_ARRAY_SIZE(active); i++) {
+ call = l_queue_find(vd->call_list, ofono_call_compare_by_status,
+ L_INT_TO_PTR(active[i]));
+
+ if (call)
+ break;
+ }
+
+ if (call == NULL) {
+ DBG("Can not find a call to hang up");
+ CALLBACK_WITH_FAILURE(cb, data);
+ return;
+ }
+
+ release_specific(vc, call->id, cb, data);
+}
+
static void create_voice_cb(struct qmi_service *service, void *user_data)
{
struct ofono_voicecall *vc = user_data;
@@ -601,6 +681,8 @@ static const struct ofono_voicecall_driver driver = {
.remove = qmi_voicecall_remove,
.dial = dial,
.answer = answer,
+ .hangup_active = hangup_active,
+ .release_specific = release_specific,
};
OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
--
2.44.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/4] qmimodem: voicecall: Implement active call hangup
2024-04-13 21:53 ` [PATCH v4 3/4] qmimodem: voicecall: Implement active call hangup Adam Pigg
@ 2024-04-16 18:21 ` Denis Kenzior
0 siblings, 0 replies; 15+ messages in thread
From: Denis Kenzior @ 2024-04-16 18:21 UTC (permalink / raw)
To: Adam Pigg, ofono
Hi Adam,
On 4/13/24 16:53, Adam Pigg wrote:
> hangup_active iterates the current list of calls, looking for the first
> active call and then calls release_specific. This then sets up the
> parameters for a call to QMI_VOICE_END_CALL, with the only parameters
> being the call-id.
>
> end_call_cb will then be called and will parse out the call-id and check
> for success.
>
> ---
> Changes in V4
> -Minor rework to hangup_active
> -Removed unnescessary changes
> ---
> ---
> drivers/qmimodem/voice.h | 1 +
> drivers/qmimodem/voicecall.c | 82 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 83 insertions(+)
>
<snip>
> @@ -534,6 +534,86 @@ error:
> l_free(param);
> }
>
> +static void end_call_cb(struct qmi_result *result, void *user_data)
> +{
> + struct cb_data *cbd = user_data;
> + ofono_voicecall_cb_t cb = cbd->cb;
> + uint16_t error;
> + uint8_t call_id;
> +
> + static const uint8_t QMI_VOICE_END_RETURN_CALL_ID = 0x10;
RESULT_CALL_ID
> +
> + 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_END_RETURN_CALL_ID, &call_id))
> + DBG("Received end call result with call id %d", call_id);
> +
> + CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +}
> +
> +static void release_specific(struct ofono_voicecall *vc, int id,
> + ofono_voicecall_cb_t cb, void *data)
> +{
> + struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> + struct cb_data *cbd;
> + struct qmi_param *param = NULL;
> +
> + static const uint8_t QMI_VOICE_END_CALL_ID = 0x01;
PARAM_CALL_ID
> +
> + DBG("");
> +
> + param = qmi_param_new();
> + cbd = cb_data_new(cb, data);
> + cbd->user = vc;
> +
> + if (!qmi_param_append_uint8(param, QMI_VOICE_END_CALL_ID, id))
> + goto error;
> +
> + if (qmi_service_send(vd->voice, QMI_VOICE_END_CALL, param, end_call_cb,
> + cbd, l_free) > 0)
> + return;
> +
> +error:
> + CALLBACK_WITH_FAILURE(cb, data);
> + l_free(cbd);
> + l_free(param);
> +}
> +
Rest of this patch LGTM.
Regards,
-Denis
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 4/4] qmimodem: voicecall: Implement DTMF tones
2024-04-13 21:53 [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing Adam Pigg
2024-04-13 21:53 ` [PATCH v4 2/4] qmimodem: voicecall: Implement call answer Adam Pigg
2024-04-13 21:53 ` [PATCH v4 3/4] qmimodem: voicecall: Implement active call hangup Adam Pigg
@ 2024-04-13 21:53 ` Adam Pigg
2024-04-16 18:56 ` Denis Kenzior
2024-04-13 22:07 ` [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing Adam Pigg
2024-04-16 18:16 ` Denis Kenzior
4 siblings, 1 reply; 15+ messages in thread
From: Adam Pigg @ 2024-04-13 21:53 UTC (permalink / raw)
To: ofono; +Cc: Adam Pigg
The send_dtmf function sets up a call to send_one_dtmf, which will call
the QMI_VOICE_START_CONTINUOUS_DTMF service function. The parameters to
this call are a hard coded call-id and the DTMF character to send.
start_cont_dtmf_cb will then be called which will set up a call to
QMI_VOICE_STOP_CONTINUOUS_DTMF to stop the tone. Finally,
stop_cont_dtmf_cb will check the final status.
---
Changes in V4
-Removed unused enum
-Minor formatting fixes
-Ensure data->full_dtmf is free'd
-Use cb_data_ref/unref between chains of dtmf calls
---
---
drivers/qmimodem/voice.h | 6 ++
drivers/qmimodem/voicecall.c | 131 +++++++++++++++++++++++++++++++++++
2 files changed, 137 insertions(+)
diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index caedb079..961fe19f 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -53,6 +53,8 @@ enum voice_commands {
QMI_VOICE_GET_ALL_CALL_INFO = 0x2f,
QMI_VOICE_END_CALL = 0x21,
QMI_VOICE_ANSWER_CALL = 0x22,
+ QMI_VOICE_START_CONTINUOUS_DTMF = 0x29,
+ QMI_VOICE_STOP_CONTINUOUS_DTMF = 0x2A,
QMI_VOICE_SUPS_NOTIFICATION_IND = 0x32,
QMI_VOICE_SET_SUPS_SERVICE = 0x33,
QMI_VOICE_GET_CALL_WAITING = 0x34,
@@ -85,6 +87,10 @@ enum qmi_voice_call_state {
QMI_VOICE_CALL_STATE_SETUP
};
+enum qmi_voice_call_dtmf_param {
+ QMI_VOICE_DTMF_DATA = 0x01,
+};
+
struct qmi_ussd_data {
uint8_t dcs;
uint8_t length;
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index 396732ad..f3111c9d 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -42,6 +42,8 @@ struct voicecall_data {
uint16_t minor;
struct l_queue *call_list;
struct ofono_phone_number dialed;
+ char *full_dtmf;
+ const char *next_dtmf;
};
struct qmi_voice_call_information_instance {
@@ -614,6 +616,133 @@ static void hangup_active(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
release_specific(vc, call->id, cb, data);
}
+static void stop_cont_dtmf_cb(struct qmi_result *result, void *user_data)
+{
+ struct cb_data *cbd = user_data;
+ ofono_voicecall_cb_t cb = cbd->cb;
+
+ uint16_t error;
+
+ DBG("");
+
+ if (qmi_result_set_error(result, &error)) {
+ DBG("QMI Error %d", error);
+ CALLBACK_WITH_FAILURE(cb, cbd->data);
+ return;
+ }
+
+ CALLBACK_WITH_SUCCESS(cb, cbd->data);
+ l_free(cbd);
+}
+
+static void start_cont_dtmf_cb(struct qmi_result *result, void *user_data)
+{
+ struct cb_data *cbd = user_data;
+ ofono_voicecall_cb_t cb = cbd->cb;
+ struct ofono_voicecall *vc = cbd->user;
+ struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+ uint16_t error;
+ struct qmi_param *param;
+
+ DBG("");
+
+ if (qmi_result_set_error(result, &error)) {
+ DBG("QMI Error %d", error);
+ CALLBACK_WITH_FAILURE(cb, cbd->data);
+ return;
+ }
+
+ param = qmi_param_new();
+
+ if (!qmi_param_append_uint8(param, QMI_VOICE_DTMF_DATA, 0xff))
+ goto error;
+
+ if (qmi_service_send(vd->voice, QMI_VOICE_STOP_CONTINUOUS_DTMF, param,
+ stop_cont_dtmf_cb, cbd, cb_data_unref) > 0) {
+ cb_data_ref(cbd);
+ return;
+ }
+
+error:
+ CALLBACK_WITH_FAILURE(cb, cbd->data);
+ l_free(cbd);
+ l_free(param);
+}
+
+static void send_one_dtmf(struct ofono_voicecall *vc, const char dtmf,
+ ofono_voicecall_cb_t cb, void *data)
+{
+ struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+ struct qmi_param *param = NULL;
+ uint8_t param_body[2];
+ struct cb_data *cbd = cb_data_new(cb, data);
+
+ DBG("");
+
+ cbd->user = vc;
+ cb_data_ref(cbd);
+
+ param = qmi_param_new();
+
+ param_body[0] = 0xff;
+ param_body[1] = (uint8_t)dtmf;
+
+ if (!qmi_param_append(param, QMI_VOICE_DTMF_DATA, sizeof(param_body),
+ param_body))
+ goto error;
+
+ if (qmi_service_send(vd->voice, QMI_VOICE_START_CONTINUOUS_DTMF, param,
+ start_cont_dtmf_cb, cbd, cb_data_unref) > 0)
+ return;
+
+error:
+ CALLBACK_WITH_FAILURE(cb, data);
+ l_free(cbd);
+ l_free(param);
+}
+
+static void send_one_dtmf_cb(const struct ofono_error *error, void *data)
+{
+ struct cb_data *cbd = data;
+ struct voicecall_data *vd = ofono_voicecall_get_data(cbd->user);
+ ofono_voicecall_cb_t cb = cbd->cb;
+
+ DBG("");
+
+ if (error->type != OFONO_ERROR_TYPE_NO_ERROR ||
+ *vd->next_dtmf == 0) {
+ if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
+ CALLBACK_WITH_SUCCESS(cb, cbd->data);
+ else
+ CALLBACK_WITH_FAILURE(cb, cbd->data);
+
+ l_free(vd->full_dtmf);
+ vd->full_dtmf = NULL;
+ l_free(cbd);
+ } else {
+ send_one_dtmf(cbd->user,
+ *(vd->next_dtmf++),
+ send_one_dtmf_cb, data);
+ }
+}
+
+static void send_dtmf(struct ofono_voicecall *vc, const char *dtmf,
+ 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);
+
+ DBG("");
+
+ l_free(vd->full_dtmf);
+ vd->full_dtmf = l_strdup(dtmf);
+ vd->next_dtmf = &vd->full_dtmf[1];
+
+ cbd->user = vc;
+
+ send_one_dtmf(vc, *dtmf, send_one_dtmf_cb, cbd);
+}
+
static void create_voice_cb(struct qmi_service *service, void *user_data)
{
struct ofono_voicecall *vc = user_data;
@@ -673,6 +802,7 @@ 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->full_dtmf);
l_free(data);
}
@@ -683,6 +813,7 @@ static const struct ofono_voicecall_driver driver = {
.answer = answer,
.hangup_active = hangup_active,
.release_specific = release_specific,
+ .send_tones = send_dtmf,
};
OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
--
2.44.0
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 4/4] qmimodem: voicecall: Implement DTMF tones
2024-04-13 21:53 ` [PATCH v4 4/4] qmimodem: voicecall: Implement DTMF tones Adam Pigg
@ 2024-04-16 18:56 ` Denis Kenzior
0 siblings, 0 replies; 15+ messages in thread
From: Denis Kenzior @ 2024-04-16 18:56 UTC (permalink / raw)
To: Adam Pigg, ofono
Hi Adam,
On 4/13/24 16:53, Adam Pigg wrote:
> The send_dtmf function sets up a call to send_one_dtmf, which will call
> the QMI_VOICE_START_CONTINUOUS_DTMF service function. The parameters to
> this call are a hard coded call-id and the DTMF character to send.
> start_cont_dtmf_cb will then be called which will set up a call to
> QMI_VOICE_STOP_CONTINUOUS_DTMF to stop the tone. Finally,
> stop_cont_dtmf_cb will check the final status.
>
> ---
> Changes in V4
> -Removed unused enum
> -Minor formatting fixes
> -Ensure data->full_dtmf is free'd
> -Use cb_data_ref/unref between chains of dtmf calls
> ---
> ---
> drivers/qmimodem/voice.h | 6 ++
> drivers/qmimodem/voicecall.c | 131 +++++++++++++++++++++++++++++++++++
> 2 files changed, 137 insertions(+)
>
<snip>
> @@ -614,6 +616,133 @@ static void hangup_active(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
> release_specific(vc, call->id, cb, data);
> }
>
> +static void stop_cont_dtmf_cb(struct qmi_result *result, void *user_data)
> +{
> + struct cb_data *cbd = user_data;
> + ofono_voicecall_cb_t cb = cbd->cb;
> +
> + uint16_t error;
> +
> + DBG("");
> +
> + if (qmi_result_set_error(result, &error)) {
> + DBG("QMI Error %d", error);
> + CALLBACK_WITH_FAILURE(cb, cbd->data);
> + return;
> + }
> +
> + CALLBACK_WITH_SUCCESS(cb, cbd->data);
> + l_free(cbd);
This looks wrong. When invoking QMI_VOICE_STOP_CONTINUOUS_DTMF, you use
cb_data_unref as the destroy function. The destroy invocation should take care
of cbd.
> +}
> +
> +static void start_cont_dtmf_cb(struct qmi_result *result, void *user_data)
> +{
> + struct cb_data *cbd = user_data;
> + ofono_voicecall_cb_t cb = cbd->cb;
> + struct ofono_voicecall *vc = cbd->user;
> + struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> + uint16_t error;
> + struct qmi_param *param;
> +
> + DBG("");
> +
> + if (qmi_result_set_error(result, &error)) {
> + DBG("QMI Error %d", error);
> + CALLBACK_WITH_FAILURE(cb, cbd->data);
> + return;
This is called with cbd, and hopefully in all cases cbd->ref == 1 at this point
with the destroy callback being cb_data_unref. Once this function returns,
destroy is called and cbd is freed correctly. This looks good.
> + }
> +
> + param = qmi_param_new();
> +
> + if (!qmi_param_append_uint8(param, QMI_VOICE_DTMF_DATA, 0xff))
> + goto error;
> +
> + if (qmi_service_send(vd->voice, QMI_VOICE_STOP_CONTINUOUS_DTMF, param,
> + stop_cont_dtmf_cb, cbd, cb_data_unref) > 0) {
> + cb_data_ref(cbd);
> + return;
On the success path, we take another ref to cbd (so cbd->ref == 2). Once we
return, destroy is called and cbd->ref is back down to 1.
> + }
> +
> +error:
> + CALLBACK_WITH_FAILURE(cb, cbd->data); > + l_free(cbd);
On the error path, this isn't necessary at all. cbd->ref should be 1, and once
this function returns, cb_data_unref should free cbd.
> + l_free(param);
> +}
> +
> +static void send_one_dtmf(struct ofono_voicecall *vc, const char dtmf,
> + ofono_voicecall_cb_t cb, void *data)
> +{
> + struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> + struct qmi_param *param = NULL;
> + uint8_t param_body[2];
> + struct cb_data *cbd = cb_data_new(cb, data);
> +
> + DBG("");
> +
> + cbd->user = vc;
> + cb_data_ref(cbd);
No need to take a ref here, cb_data_new creates cbd with ref==1.
> +
> + param = qmi_param_new();
> +
> + param_body[0] = 0xff;
> + param_body[1] = (uint8_t)dtmf;
> +
> + if (!qmi_param_append(param, QMI_VOICE_DTMF_DATA, sizeof(param_body),
> + param_body))
> + goto error;
> +
> + if (qmi_service_send(vd->voice, QMI_VOICE_START_CONTINUOUS_DTMF, param,
> + start_cont_dtmf_cb, cbd, cb_data_unref) > 0)
> + return;
> +
> +error:
> + CALLBACK_WITH_FAILURE(cb, data);
> + l_free(cbd);
> + l_free(param);
> +}
> +
So you have two types of cb_data objects:
1. The one you create in send_dtmf() that contains the callback to ofono core
2. The one you create here that contains the callback to send_one_dtmf_cb and
pointer to 1.
The 2nd cbd goes through the following callbacks:
send_one_dtmf -> ref: 1
start_cont_dtmf_cb -> take ref to 2 on success, in all cases unref is called
when this function returns
stop_cont_dtmf -> ref: 1, unref is called when this function returns
Note that you still might leak the object in 1 if the DTMF is interrupted
mid-execution. For example upon voicecall atom removal.
> +static void send_one_dtmf_cb(const struct ofono_error *error, void *data)
> +{
> + struct cb_data *cbd = data;
> + struct voicecall_data *vd = ofono_voicecall_get_data(cbd->user);
> + ofono_voicecall_cb_t cb = cbd->cb;
> +
> + DBG("");
> +
> + if (error->type != OFONO_ERROR_TYPE_NO_ERROR ||
> + *vd->next_dtmf == 0) {
> + if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
> + CALLBACK_WITH_SUCCESS(cb, cbd->data);
> + else
> + CALLBACK_WITH_FAILURE(cb, cbd->data);
> +
> + l_free(vd->full_dtmf);
> + vd->full_dtmf = NULL;
> + l_free(cbd);
> + } else {
> + send_one_dtmf(cbd->user,
> + *(vd->next_dtmf++),
> + send_one_dtmf_cb, data);
> + }
> +}
> +
> +static void send_dtmf(struct ofono_voicecall *vc, const char *dtmf,
> + 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);
Hmm, you alloc cb_data_new here which creates cbd with 1 ref
> +
> + DBG("");
> +
> + l_free(vd->full_dtmf);
> + vd->full_dtmf = l_strdup(dtmf);
> + vd->next_dtmf = &vd->full_dtmf[1];
> +
> + cbd->user = vc;
> +
> + send_one_dtmf(vc, *dtmf, send_one_dtmf_cb, cbd);
Then here, cbd is refed again. There's likely a leak?
You're much safer storing 'cb' and 'data' inside struct voicecall_data (as
send_dtmf_cb and send_dtmf_data).
> +}
> +
> static void create_voice_cb(struct qmi_service *service, void *user_data)
> {
> struct ofono_voicecall *vc = user_data;
Regards,
-Denis
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing
2024-04-13 21:53 [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing Adam Pigg
` (2 preceding siblings ...)
2024-04-13 21:53 ` [PATCH v4 4/4] qmimodem: voicecall: Implement DTMF tones Adam Pigg
@ 2024-04-13 22:07 ` Adam Pigg
2024-04-16 18:16 ` Denis Kenzior
4 siblings, 0 replies; 15+ messages in thread
From: Adam Pigg @ 2024-04-13 22:07 UTC (permalink / raw)
To: ofono
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)
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing
2024-04-13 21:53 [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing Adam Pigg
` (3 preceding siblings ...)
2024-04-13 22:07 ` [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing Adam Pigg
@ 2024-04-16 18:16 ` Denis Kenzior
2024-04-17 20:38 ` Adam Pigg
4 siblings, 1 reply; 15+ messages in thread
From: Denis Kenzior @ 2024-04-16 18:16 UTC (permalink / raw)
To: Adam Pigg, ofono
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing
2024-04-16 18:16 ` Denis Kenzior
@ 2024-04-17 20:38 ` Adam Pigg
2024-04-17 20:54 ` Denis Kenzior
0 siblings, 1 reply; 15+ messages in thread
From: Adam Pigg @ 2024-04-17 20:38 UTC (permalink / raw)
To: ofono, Denis Kenzior
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing
2024-04-17 20:38 ` Adam Pigg
@ 2024-04-17 20:54 ` Denis Kenzior
2024-04-18 6:58 ` Marcel Holtmann
0 siblings, 1 reply; 15+ messages in thread
From: Denis Kenzior @ 2024-04-17 20:54 UTC (permalink / raw)
To: Adam Pigg, ofono
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing
2024-04-17 20:54 ` Denis Kenzior
@ 2024-04-18 6:58 ` Marcel Holtmann
2024-04-18 14:41 ` Denis Kenzior
0 siblings, 1 reply; 15+ messages in thread
From: Marcel Holtmann @ 2024-04-18 6:58 UTC (permalink / raw)
To: Denis Kenzior; +Cc: Adam Pigg, ofono
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing
2024-04-18 6:58 ` Marcel Holtmann
@ 2024-04-18 14:41 ` Denis Kenzior
2024-04-18 14:52 ` Marcel Holtmann
0 siblings, 1 reply; 15+ messages in thread
From: Denis Kenzior @ 2024-04-18 14:41 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Adam Pigg, ofono
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing
2024-04-18 14:41 ` Denis Kenzior
@ 2024-04-18 14:52 ` Marcel Holtmann
2024-04-18 15:03 ` Denis Kenzior
0 siblings, 1 reply; 15+ messages in thread
From: Marcel Holtmann @ 2024-04-18 14:52 UTC (permalink / raw)
To: Denis Kenzior; +Cc: Adam Pigg, ofono
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/4] qmimodem: voicecall: Implement call dialing
2024-04-18 14:52 ` Marcel Holtmann
@ 2024-04-18 15:03 ` Denis Kenzior
0 siblings, 0 replies; 15+ messages in thread
From: Denis Kenzior @ 2024-04-18 15:03 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Adam Pigg, ofono
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
^ permalink raw reply [flat|nested] 15+ messages in thread