Re: [PATCH v2 7/8] voicecall: add +CHUP support for HFP emulator

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Frédéric,

On 04/07/2011 11:33 AM, Frédéric Danis wrote:
> ---
>  src/voicecall.c |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 108 insertions(+), 0 deletions(-)
> 
> diff --git a/src/voicecall.c b/src/voicecall.c
> index cf02bb6..8aec0a0 100644
> --- a/src/voicecall.c
> +++ b/src/voicecall.c
> @@ -60,6 +60,7 @@ struct ofono_voicecall {
>  	GQueue *toneq;
>  	guint tone_source;
>  	unsigned int hfp_watch;
> +	GSList *hfp_release_list;

I'd rather not do this, you're not really avoiding a race condition with
MPTY hangups or HangupAll here.  I'd rather we re-use release_queue but
add additional sanity checking.  Perhaps adding a callback parameter to
voicecalls_release_next might make this easy.  In general we need to
think some more about how to avoid clashes between the D-Bus API and the
emulator.

>  };
>  
>  struct voicecall {
> @@ -95,6 +96,11 @@ struct tone_queue_entry {
>  	int id;
>  };
>  
> +struct emulator_release {
> +	struct ofono_voicecall *vc;
> +	struct ofono_emulator *em;
> +};
> +
>  static const char *default_en_list[] = { "911", "112", NULL };
>  static const char *default_en_list_no_sim[] = { "119", "118", "999", "110",
>  						"08", "000", NULL };
> @@ -102,6 +108,7 @@ static const char *default_en_list_no_sim[] = { "119", "118", "999", "110",
>  static void generic_callback(const struct ofono_error *error, void *data);
>  static void multirelease_callback(const struct ofono_error *err, void *data);
>  static gboolean tone_request_run(gpointer user_data);
> +static void emulator_release_next(struct emulator_release *er);
>  
>  static gint call_compare_by_id(gconstpointer a, gconstpointer b)
>  {
> @@ -2344,6 +2351,10 @@ static void voicecall_unregister(struct ofono_atom *atom)
>  						OFONO_ATOM_TYPE_EMULATOR_HFP,
>  						emulator_remove_handler,
>  						"A");
> +	__ofono_modem_foreach_registered_atom(modem,
> +						OFONO_ATOM_TYPE_EMULATOR_HFP,
> +						emulator_remove_handler,
> +						"+CHUP");
>  
>  	__ofono_modem_remove_atom_watch(modem, vc->hfp_watch);
>  
> @@ -2558,6 +2569,102 @@ fail:
>  	};
>  }
>  
> +static void emulator_multirelease_callback(const struct ofono_error *error,
> +						void *data)
> +{
> +	struct emulator_release *er = data;
> +	struct ofono_voicecall *vc = er->vc;
> +
> +	if (vc->hfp_release_list != NULL) {
> +		emulator_release_next(er);
> +		return;
> +	}
> +
> +	emulator_generic_cb(error, er->em);
> +
> +	g_free(er);
> +}
> +
> +static void emulator_release_next(struct emulator_release *er)
> +{
> +	struct ofono_voicecall *vc;
> +	struct voicecall *call;
> +
> +	vc = er->vc;
> +
> +	call = vc->hfp_release_list->data;
> +
> +	vc->hfp_release_list = g_slist_remove(vc->hfp_release_list, call);
> +
> +	vc->driver->release_specific(vc, call->call->id,
> +				emulator_multirelease_callback, er);
> +}
> +
> +static void emulator_chup_cb(struct ofono_emulator *em,
> +			struct ofono_emulator_request *req, void *userdata)
> +{
> +	struct ofono_voicecall *vc = userdata;
> +	struct ofono_error result;
> +	struct emulator_release *er;
> +	GSList *l;
> +	struct voicecall *call;
> +
> +	result.error = 0;
> +
> +	switch (ofono_emulator_request_get_type(req)) {
> +	case OFONO_EMULATOR_REQUEST_TYPE_COMMAND_ONLY:
> +		if (vc->driver->release_specific == NULL &&
> +				vc->driver->hangup_active == NULL)
> +			goto fail;
> +
> +		if (vc->driver->hangup_active) {
> +			vc->driver->hangup_active(vc, emulator_generic_cb, em);
> +			goto done;
> +		}
> +
> +		/* if there is already a CHUP pending we return an error */
> +		if (vc->hfp_release_list)
> +			goto fail;

Why are you checking this here?  Shouldn't you check this before
potentially sending a hangup_active?

> +
> +		er = g_try_new0(struct emulator_release, 1);

For simple structures (e.g. under 1k or so) it is fine to simply use g_new0

> +		if (er == NULL) {
> +			ofono_error("Unable to allocate release structure");
> +			goto fail;
> +		}
> +
> +		er->vc = vc;
> +		er->em = em;
> +
> +		for (l = vc->call_list; l; l = l->next) {
> +			call = l->data;
> +
> +			if (call->call->status == CALL_STATUS_WAITING ||
> +					call->call->status == CALL_STATUS_HELD)
> +				continue;
> +
> +			vc->hfp_release_list = g_slist_prepend(
> +						vc->hfp_release_list,
> +						l->data);
> +		}
> +
> +		if (vc->hfp_release_list == NULL) {
> +			g_free(er);
> +			goto fail;
> +		}
> +
> +		emulator_release_next(er);
> +
> +done:
> +		dial_request_user_cancel(vc, NULL);
> +		break;
> +
> +	default:
> +fail:
> +		result.type = OFONO_ERROR_TYPE_FAILURE;
> +		ofono_emulator_send_final(em, &result);
> +	};
> +}
> +
>  static void emulator_hfp_watch(struct ofono_atom *atom,
>  				enum ofono_atom_watch_condition cond,
>  				void *data)
> @@ -2570,6 +2677,7 @@ static void emulator_hfp_watch(struct ofono_atom *atom,
>  	notify_emulator_call_status(data);
>  
>  	ofono_emulator_add_handler(em, "A", emulator_ata_cb, data, NULL);
> +	ofono_emulator_add_handler(em, "+CHUP", emulator_chup_cb, data, NULL);
>  }
>  
>  void ofono_voicecall_register(struct ofono_voicecall *vc)

Regards,
-Denis