Re: [PATCH v0 03/15] hfp_hf_bluez5: Add extracting version

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Claudio,

On 03/12/2013 07:42 AM, Claudio Takahasi wrote:
> This patch parses and reads the profile "Version" that comes in the fd
> dictionary of the NewConnection method. "Version" is input for Audio Card
> registration.
> ---
>   plugins/hfp_hf_bluez5.c | 37 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
>
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index 2020e63..a8d8277 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -343,6 +343,37 @@ static ofono_bool_t device_path_compare(struct ofono_modem *modem,
>   	return g_str_equal(path, value);
>   }
>
> +static int get_version(DBusMessageIter *iter, uint16_t *version)
> +{
> +	DBusMessageIter dict, entry, valiter;
> +	const char *key;
> +	uint16_t value;
> +
> +	dbus_message_iter_recurse(iter,&dict);
> +
> +	/* Dict entry key */
> +	dbus_message_iter_recurse(&dict,&entry);
> +	if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING)
> +		return -EINVAL;
> +
> +	dbus_message_iter_get_basic(&entry,&key);
> +	if (g_str_equal("Version", key) == FALSE)
> +		return -EINVAL;
> +
> +	/* Dict entry value */
> +	dbus_message_iter_next(&entry);
> +	if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_VARIANT)
> +		return -EINVAL;
> +
> +	dbus_message_iter_recurse(&entry,&valiter);
> +	dbus_message_iter_get_basic(&valiter,&value);
> +
> +	if (version)
> +		*version = value;
> +
> +	return 0;
> +}
> +
>   static DBusMessage *profile_new_connection(DBusConnection *conn,
>   					DBusMessage *msg, void *user_data)
>   {
> @@ -353,6 +384,7 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>   	DBusMessageIter entry;
>   	const char *device;
>   	char local[18], remote[18];
> +	uint16_t version;
>   	int fd, err;
>
>   	DBG("Profile handler NewConnection");
> @@ -373,6 +405,11 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>   	if (fd<  0)
>   		goto invalid;
>
> +	dbus_message_iter_next(&entry);
> +

I would not mind extra sanity checking here, e.g. argument type checking

> +	if (get_version(&entry,&version)<  0)
> +		goto invalid;
> +

I don't like this.  The fd_properties is a dictionary, you cannot assume 
that Version will be always the first one.

>   	modem = ofono_modem_find(device_path_compare, (void *) device);
>   	if (modem == NULL) {
>   		close(fd);

Regards,
-Denis