Re: [PATCH 2/5] Move bluetooth utils from hfp.c to bluetooth.c

[Gustavo F. Padovan <gustavo@padovan.org>]

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

Hi Denis,

* Denis Kenzior <denkenz@gmail.com> [2010-06-10 16:17:12 -0500]:

> Hi Gustavo,
> 
> Please break this patch up more.  You're moving functions but also modifying 
> them in between.  I'm getting lost.
> 
> > +static int send_method_call_with_reply(const char *dest, const char *path,
> > +				const char *interface, const char *method,
> > +				DBusPendingCallNotifyFunction cb,
> > +				void *user_data, DBusFreeFunction free_func,
> > +				int timeout, int type, ...)
> > +{
> > +	DBusMessage *msg;
> > +	DBusPendingCall *call;
> > +	va_list args;
> > +	int err;
> > +
> > +	msg = dbus_message_new_method_call(dest, path, interface, method);
> > +	if (!msg) {
> > +		ofono_error("Unable to allocate new D-Bus %s message", method);
> > +		err = -ENOMEM;
> > +		goto fail;
> > +	}
> > +
> > +	va_start(args, type);
> > +
> > +	if (!dbus_message_append_args_valist(msg, type, args)) {
> > +		va_end(args);
> > +		err = -EIO;
> > +		goto fail;
> > +	}
> > +
> > +	va_end(args);
> > +
> > +	if (timeout > 0)
> > +		timeout *= 1000;
> > +
> > +	if (!dbus_connection_send_with_reply(connection, msg, &call, timeout)) {
> > +		ofono_error("Sending %s failed", method);
> > +		err = -EIO;
> > +		goto fail;
> > +	}
> > +
> > +	dbus_pending_call_set_notify(call, cb, user_data, free_func);
> > +	dbus_pending_call_unref(call);
> > +	dbus_message_unref(msg);
> > +
> > +	return 0;
> > +
> > +fail:
> > +	if (free_func && user_data)
> > +		free_func(user_data);
> > +
> > +	if (msg)
> > +		dbus_message_unref(msg);
> > +
> > +	return err;
> > +}
> 
> So this should be in a separate patch...

If I put this in a separate patch, it won't compile because no one is
using send_method_call_with_reply.

> 
> > +
> > +typedef void (*PropertyHandler)(DBusMessageIter *iter, gpointer
> >  user_data); +
> > +struct property_handler {
> > +	const char *property;
> > +	PropertyHandler callback;
> > +	gpointer user_data;
> > +};
> > +
> > +static gint property_handler_compare(gconstpointer a, gconstpointer b)
> > +{
> > +	const struct property_handler *handler = a;
> > +	const char *property = b;
> > +
> > +	return strcmp(handler->property, property);
> > +}
> > +
> > +static void parse_properties_reply(DBusMessage *reply,
> > +					const char *property, ...)
> > +{
> > +	va_list args;
> > +	GSList *prop_handlers = NULL;
> > +	DBusMessageIter array, dict;
> > +
> > +	va_start(args, property);
> > +
> > +	while (property != NULL) {
> > +		struct property_handler *handler =
> > +					g_new0(struct property_handler, 1);
> > +
> > +		handler->property = property;
> > +		handler->callback = va_arg(args, PropertyHandler);
> > +		handler->user_data = va_arg(args, gpointer);
> > +
> > +		property = va_arg(args, const char *);
> > +
> > +		prop_handlers = g_slist_prepend(prop_handlers, handler);
> > +	}
> > +
> > +	va_end(args);
> > +
> > +	if (dbus_message_iter_init(reply, &array) == FALSE)
> > +		goto done;
> > +
> > +	if (dbus_message_iter_get_arg_type(&array) != DBUS_TYPE_ARRAY)
> > +		goto done;
> > +
> > +	dbus_message_iter_recurse(&array, &dict);
> > +
> > +	while (dbus_message_iter_get_arg_type(&dict) == DBUS_TYPE_DICT_ENTRY) {
> > +		DBusMessageIter entry, value;
> > +		const char *key;
> > +		GSList *l;
> > +
> > +		dbus_message_iter_recurse(&dict, &entry);
> > +
> > +		if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_STRING)
> > +			goto done;
> > +
> > +		dbus_message_iter_get_basic(&entry, &key);
> > +
> > +		dbus_message_iter_next(&entry);
> > +
> > +		if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_VARIANT)
> > +			goto done;
> > +
> > +		dbus_message_iter_recurse(&entry, &value);
> > +
> > +		l = g_slist_find_custom(prop_handlers, key,
> > +					property_handler_compare);
> > +
> > +		if (l) {
> > +			struct property_handler *handler = l->data;
> > +
> > +			handler->callback(&value, handler->user_data);
> > +		}
> > +
> > +		dbus_message_iter_next(&dict);
> > +	}
> > +
> > +done:
> > +	g_slist_foreach(prop_handlers, (GFunc)g_free, NULL);
> > +	g_slist_free(prop_handlers);
> > +}
> 
> And this one too...

Same here.

-- 
Gustavo F. Padovan
http://padovan.org