Re: [PATCH v3 1/4] qmimodem: voicecall: Implement call dialing

[piggz1@gmail.com]

On Thursday, 4 April 2024, Denis Kenzior wrote:
> Hi Adam,
> 
> On 4/2/24 16:25, Adam Pigg 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.
> > ---
> >   drivers/qmimodem/voice.h     |  17 ++
> >   drivers/qmimodem/voicecall.c | 479 ++++++++++++++++++++++++++++++++++-
> >   2 files changed, 495 insertions(+), 1 deletion(-)
> > 
> 
> <snip>
> 
> Couple of general comments:
> 
> - There's still an empty line @ EOF in this commit, please fix that.
> - After applying just this patch, compilation fails:
> 
> [denkenz@archdev ofono]$ make
> make --no-print-directory all-am
>    CC       drivers/qmimodem/voicecall.o
> drivers/qmimodem/voicecall.c: In function ‘all_call_status_ind’:
> drivers/qmimodem/voicecall.c:355:32: error: unused variable ‘vd’ 
> [-Werror=unused-variable]
>    355 |         struct voicecall_data *vd = ofono_voicecall_get_data(vc);
>        |                                ^~
> drivers/qmimodem/voicecall.c: At top level:
> drivers/qmimodem/voicecall.c:262:16: error: ‘ofono_to_qmi_direction’ defined but 
> not used [-Werror=unused-function]
>    262 | static uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
>        |                ^~~~~~~~~~~~~~~~~~~~~~
> drivers/qmimodem/voicecall.c:97:13: error: ‘ofono_call_compare_by_status’ 
> defined but not used [-Werror=unused-function]
>     97 | static bool ofono_call_compare_by_status(const void *a, const void *b)
>        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make[1]: *** [Makefile:4094: drivers/qmimodem/voicecall.o] Error 1
> make: *** [Makefile:2402: all] Error 2
> 
> Ideally, each patch should compile and run cleanly to help with 'git bisect'.
> 
> <snip>

Thanks for this, i think the issue might be compiler differences. It all builds ok for me, but sailfish is using gcc 8 atm, so i guess defaults have changed.


> 
> > +
> > +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;
> > +}
> > +
> 
> This function belongs in patch 2.  It isn't used in this commit and is causing a 
> compiler warning / error.
> 
> <snip>
> 
> > +static uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
> > +{
> > +	return ofono_direction + 1;
> > +}
> 
> This function isn't used, get rid of it
> 
> <snip>
> 
> > +static int qmi_voice_call_status(struct qmi_result *qmi_result,
> > +					struct qmi_voice_all_call_status_ind *result)
> 
> Right now you have this function producing an intermediate structure which 
> points to memory owned by struct qmi_result and is only valid as long as 
> qmi_result is valid.  all_call_status_ind() then converts this intermediate 
> structure into an l_queue with ofono_call objects.  The intermediate structure 
> is never needed after or used elsewhere.  No other caller uses this intermediate 
> structure.
> 
> Have you thought about having this function return an allocated l_queue with 
> ofono_call objects as contents instead and getting rid of struct 
> qmi_voice_all_call_status_ind completely?
> 
> > +{
> > +	int offset;
> > +	uint16_t len;
> > +	bool status = true;
> > +	const struct qmi_voice_remote_party_number *remote_party_number;
> > +	const struct qmi_voice_call_information *call_information;
> > +
> > +	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;
> > +
> > +	/* mandatory */
> > +	call_information = qmi_result_get(
> > +		qmi_result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len);
> > +
> > +	if (!call_information) {
> > +		call_information = qmi_result_get(
> > +			qmi_result, QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION,
> > +			&len);
> > +		status = false;
> > +	}
> > +
> > +	if (call_information) {
> > +		/* verify the length */
> > +		if (len < sizeof(call_information->size))
> > +			return -EMSGSIZE;
> > +
> > +		if (len !=
> > +			call_information->size *
> > +			sizeof(struct qmi_voice_call_information_instance) +
> > +			sizeof(call_information->size))
> > +			return -EMSGSIZE;
> 
> doc/coding-style.txt item M4
> 
> > +
> > +		result->call_information_set = 1;
> > +		result->call_information = call_information;
> > +	} else
> > +		return -EBADMSG;
> 
> Easier to bail early and not nest or need the if condition:
> 
> if (!call_information)
> 	return -EBADMSG;
> 
> if (len < sizeof(call_information->size))
> 	return -EMSGSIZE;
> 
> ...
> 
> > +
> > +	/* mandatory */
> > +	remote_party_number = qmi_result_get(
> > +		qmi_result,
> > +		status ? QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER :
> > +			 QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER,
> > +		&len);
> > +
> > +	if (remote_party_number) {
> > +		const struct qmi_voice_remote_party_number_instance *instance;
> > +		int instance_size =
> > +			sizeof(struct qmi_voice_remote_party_number_instance);
> > +		int i;
> > +
> > +		/* verify the length */
> > +		if (len < sizeof(remote_party_number->size))
> > +			return -EMSGSIZE;
> > +
> > +		for (i = 0, offset = sizeof(remote_party_number->size);
> > +			offset <= len && i < 16 && i < remote_party_number->size;
> > +			i++) {
> > +			if (offset == len)
> > +				break;
> > +			else if (offset + instance_size > len)
> > +				return -EMSGSIZE;
> > +
> > +			instance = (void *)remote_party_number + offset;
> > +			result->remote_party_number[i] = instance;
> > +			offset +=
> > +				sizeof(struct qmi_voice_remote_party_number_instance) +
> > +				instance->number_size;
> > +		}
> > +		result->remote_party_number_set = 1;
> > +		result->remote_party_number_size = remote_party_number->size;
> > +	} else
> > +		return -EBADMSG;
> 
> Same here:
> 
> if (!remote_party_number)
> 	return -EBADMSG;
> 
> if (len < ...)
> 
> for (...)
> 
> See doc/coding-style.txt item O2
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void all_call_status_ind(struct qmi_result *result, void *user_data)
> > +{
> > +	struct ofono_voicecall *vc = user_data;
> > +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> > +	struct l_queue *calls = l_queue_new();
> > +	int i;
> > +	int size = 0;
> > +	struct qmi_voice_all_call_status_ind status_ind;
> > +
> > +	DBG("");
> 
> doc/coding-style.txt Item M1
> 
> > +	if (qmi_voice_call_status(result, &status_ind) != 0) {
> 
> nit: Prefer < 0 check for functions that return -errno.
> 
> > +		DBG("Parsing of all call status indication failed");
> > +		goto error;
> > +	}
> > +
> > +	if (!status_ind.remote_party_number_set ||
> > +	    !status_ind.call_information_set) {
> 
> Incorrect indentation
> 
> > +		DBG("Some required fields are not set");
> > +		goto error;
> > +	}
> > +
> > +	size = status_ind.call_information->size;
> > +	if (!size) {
> > +		DBG("No call informations received!");
> > +		goto error;
> > +	}
> > +
> > +	/* expect we have valid fields for every call */
> > +	if (size != status_ind.remote_party_number_size) {
> > +		DBG("Not all fields have the same size");
> > +		goto error;
> > +	}
> > +
> > +	for (i = 0; i < size; i++) {
> > +		struct qmi_voice_call_information_instance call_info;
> > +		struct ofono_call *call;
> > +		const struct qmi_voice_remote_party_number_instance
> > +			*remote_party = status_ind.remote_party_number[i];
> > +		int number_size;
> > +
> > +		call_info = status_ind.call_information->instance[i];
> > +		call = l_new(struct ofono_call, 1);
> > +		call->id = call_info.id;
> > +		call->direction = qmi_to_ofono_direction(call_info.direction);
> > +
> > +		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);
> > +
> > +		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;
> > +
> > +		l_queue_push_tail(calls, call);
> > +		DBG("%d", l_queue_length(calls));
> > +	}
> > +
> > +	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;
> > +	bool call_id_set = false;
> > +
> > +	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))
> > +		call_id_set = true;
> > +	else {
> > +		DBG("Received invalid Result");
> > +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> > +		return;
> > +	}
> > +
> > +	if (!call_id_set) {
> > +		DBG("Didn't receive a call id");
> > +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> > +		return;
> > +	}
> 
> This if looks like a dead branch.  The only way call_id_set is false here is if 
> qmi_result_get_uint8 earlier failed.  In fact, the above section can be 
> rewritten as:
> 
> if (!qmi_result_get_uint8(...)) {
> 	ofono_error("No call id in dial result");
> 	CALLBACK_WITH_FAILURE(...);
> 	return;
> }
> 
> No need for call_id_set variable either.
> 
> > +
> > +	DBG("New call QMI id %d", call_id);
> > +	ofono_call_list_dial_callback(vc, call_id);
> > +
> > +	CALLBACK_WITH_SUCCESS(cb, cbd->data);
> > +}
> > +
> 
> <snip>
> 
> > +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;
> > +	}
> 
> No need for {}
> 
> > +
> > +	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>
> 
> > @@ -92,13 +566,16 @@ 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)
> >   
> > +
> 
> Empty line at EOF I mentioned earlier is here
> 
> Regards,
> -Denis
>

-- 
Sent from my Sailfish device