Re: [PATCH 08/12] handsfree: Expose RequestInput in D-Bus API

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Mikel,

On 09/15/2011 11:34 AM, Mikel Astiz wrote:
> ---
>  src/handsfree.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 64 insertions(+), 0 deletions(-)
> 
> diff --git a/src/handsfree.c b/src/handsfree.c
> index 2bf2284..3138474 100644
> --- a/src/handsfree.c
> +++ b/src/handsfree.c
> @@ -47,6 +47,7 @@ struct ofono_handsfree {
>  	const struct ofono_handsfree_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> +	DBusMessage *pending;
>  };
>  
>  void ofono_handsfree_set_inband_ringing(struct ofono_handsfree *hf,
> @@ -118,11 +119,71 @@ static DBusMessage *handsfree_set_property(DBusConnection *conn,
>  	return __ofono_error_invalid_args(msg);
>  }
>  
> +static void request_phone_number_cb(const struct ofono_error *error,
> +					const struct ofono_phone_number *number,
> +					void *data)
> +{
> +	struct ofono_handsfree *hf = data;
> +	DBusMessage *reply;
> +	const char *phone_number;
> +
> +	if (!hf->pending)
> +		return;

Is this check really necessary?

> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		DBG("Phone number request callback returned error: %s",
> +			telephony_error_to_str(error));
> +
> +		reply = __ofono_error_failed(hf->pending);
> +		__ofono_dbus_pending_reply(&hf->pending, reply);
> +		return;
> +	}
> +
> +	phone_number = phone_number_to_string(number);
> +	reply = dbus_message_new_method_return(hf->pending);
> +	dbus_message_append_args(reply, DBUS_TYPE_STRING, &phone_number,
> +					DBUS_TYPE_INVALID);
> +	__ofono_dbus_pending_reply(&hf->pending, reply);
> +}
> +
> +static DBusMessage *handsfree_request_input(DBusConnection *conn,
> +						DBusMessage *msg, void *data)
> +{
> +	struct ofono_handsfree *hf = data;
> +	DBusMessageIter iter;
> +	const char *name;
> +
> +	if (hf->pending)
> +		return __ofono_error_busy(msg);
> +
> +	if (dbus_message_iter_init(msg, &iter) == FALSE)
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_get_basic(&iter, &name);
> +
> +	if (g_str_equal(name, "PhoneNumber")) {
> +		if (!hf->driver->request_phone_number)
> +			return __ofono_error_not_supported(msg);
> +
> +		hf->pending = dbus_message_ref(msg);
> +		hf->driver->request_phone_number(hf, request_phone_number_cb,
> +							hf);
> +		return NULL;
> +	}

I'd rather not do it this way.  I know +BINP is supposed to be
'extendable', but it hasn't been in what 5-6 years now?  Using
RequestPhoneNumber or RequestNumberInput would probably be better.

> +
> +	return __ofono_error_invalid_args(msg);
> +}
> +
>  static GDBusMethodTable handsfree_methods[] = {
>  	{ "GetProperties",    "",    "a{sv}", handsfree_get_properties,
>  		G_DBUS_METHOD_FLAG_ASYNC },
>  	{ "SetProperty",      "sv",  "", handsfree_set_property,
>  		G_DBUS_METHOD_FLAG_ASYNC },
> +	{ "RequestInput",      "s",  "v", handsfree_request_input,
> +		G_DBUS_METHOD_FLAG_ASYNC },

You might want to submit a formal API proposal first (e.g. in doc/)
before trying to implement the API.  This would make discussing API
details easier.

>  	{ NULL, NULL, NULL, NULL }
>  };
>  
> @@ -143,6 +204,9 @@ static void handsfree_remove(struct ofono_atom *atom)
>  	if (hf->driver != NULL && hf->driver->remove != NULL)
>  		hf->driver->remove(hf);
>  
> +	if (hf->pending)
> +		dbus_message_unref(hf->pending);
> +

The rest of the code doesn't do this, though it probably should.
However, it probably belongs in _unregister, not _remove.  Using
ofono_dbus_pending_reply might be better as well.

>  	g_free(hf);
>  }
>  

Regards,
-Denis