Skip to contentSkip to conversation

dbus.c cleanup

PATCH In review Johan Hedberg 2 participants 10 messages Nov 3, 2005 Bluetooth drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 dbus.c cleanup

Hi Marcel,

Here's a cleanup patch for dbus.c which fixes some issues with whitespace, coding-style, unecessary variables, etc.

I've also fixed PeriodicInquiry functionality of the python test script
(http://www.iki.fi/~jhedberg/bluez-python/pybt.py). It should now support all currently implemented D-BUS services in hcid. Do you think it would be worth including it in the bluez-utils cvs?

Johan

Index: hcid/dbus.c
===================================================================
RCS file: /cvsroot/bluez/utils/hcid/dbus.c,v
retrieving revision 1.44

dbus.c +22 -42
diff -u -r1.44 dbus.c
@@ -106,7 +106,7 @@
 typedef struct  {
 	uint32_t code;
 	const char *str;
-}bluez_error_t;
+} bluez_error_t;
 
 typedef struct {
 	char *str;
@@ -381,14 +381,15 @@
 static gboolean register_dbus_path(const char *path, uint16_t path_id, uint16_t dev_id,
 				const DBusObjectPathVTable *pvtable, gboolean fallback)
 {
-	struct hci_dbus_data *data;
+	gboolean ret = FALSE;
+	struct hci_dbus_data *data = NULL;
 
 	syslog(LOG_INFO,"Registering DBUS Path: %s", path);
 
 	data = malloc(sizeof(struct hci_dbus_data));
 	if (data == NULL) {
 		syslog(LOG_ERR,"Failed to alloc memory to DBUS path register data (%s)", path);
-		return FALSE;
+		goto out;
 	}
 
 	data->path_id = path_id;
@@ -396,19 +397,23 @@
 
 	if (fallback) {
 		if (!dbus_connection_register_fallback(connection, path, pvtable, data)) {
-			syslog(LOG_ERR,"DBUS failed to register %s object", path);
-			free(data);
-			return FALSE;
+			syslog(LOG_ERR,"DBUS failed to register %s fallback", path);
+			goto out;
 		}
 	} else {
 		if (!dbus_connection_register_object_path(connection, path, pvtable, data)) {
 			syslog(LOG_ERR,"DBUS failed to register %s object", path);
-			free(data);
-			return FALSE;
+			goto out;
 		}
 	}
 
-	return TRUE;
+	ret = TRUE;
+
+out:
+	if (!ret && data)
+		free(data);
+
+	return ret;
 }
 
 static gboolean unregister_dbus_path(const char *path)
@@ -468,8 +473,7 @@
 
 failed:
 	dbus_message_unref(message);
-	hci_send_cmd(dev, OGF_LINK_CTL,
-				OCF_PIN_CODE_NEG_REPLY, 6, &ci->bdaddr);
+	hci_send_cmd(dev, OGF_LINK_CTL, OCF_PIN_CODE_NEG_REPLY, 6, &ci->bdaddr);
 }
 
 void hcid_dbus_inquiry_start(bdaddr_t *local)
@@ -506,10 +510,7 @@
 
 failed:
 	dbus_message_unref(message);
-
 	bt_free(local_addr);
-
-	return;
 }
 
 void hcid_dbus_inquiry_complete(bdaddr_t *local)
@@ -546,10 +547,7 @@
 
 failed:
 	dbus_message_unref(message);
-
 	bt_free(local_addr);
-
-	return;
 }
 
 void hcid_dbus_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class, int8_t rssi)
@@ -598,8 +596,6 @@
 
 	bt_free(local_addr);
 	bt_free(peer_addr);
-
-	return;
 }
 
 void hcid_dbus_remote_name(bdaddr_t *local, bdaddr_t *peer, char *name)
@@ -645,8 +641,6 @@
 
 	bt_free(local_addr);
 	bt_free(peer_addr);
-
-	return;
 }
 
 void hcid_dbus_remote_name_failed(bdaddr_t *local, bdaddr_t *peer, uint8_t status)
@@ -692,8 +686,6 @@
 
 	bt_free(local_addr);
 	bt_free(peer_addr);
-
-	return;
 }
 
 void hcid_dbus_conn_complete(bdaddr_t *local, bdaddr_t *peer)
@@ -910,7 +902,6 @@
 
 	unregister_dbus_path(DEVICE_PATH);
 	unregister_dbus_path(MANAGER_PATH);
-
 }
 
 gboolean hcid_dbus_register_device(uint16_t id) 
@@ -1046,7 +1037,7 @@
 
 	for (; ptr->id != INVALID_PATH_ID; ptr++) {
 		if (ptr->unreg_func(connection, id) < 0)
-			goto failed;
+			syslog(LOG_ERR, "Unregistering profile id %04X failed", ptr->id);
 	}
 
 	up_adapters--;
@@ -1161,8 +1152,6 @@
 	const struct service_data *handlers = NULL;
 	DBusMessage *reply = NULL;
 	struct hci_dbus_data *dbus_data = data;
-	int type;
-	const char *iface;
 	const char *method;
 	const char *signature;
 	const char *path;
@@ -1170,8 +1159,6 @@
 	DBusHandlerResult ret = DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 
 	path = dbus_message_get_path(msg);
-	type = dbus_message_get_type(msg);
-	iface = dbus_message_get_interface(msg);
 	method = dbus_message_get_member(msg);
 	signature = dbus_message_get_signature(msg);
 
@@ -1216,11 +1203,9 @@
 	if (error)
 		reply = bluez_new_failure_msg(msg, error);
 
-
 	if (reply) {
-		if (!dbus_connection_send (conn, reply, NULL)) {
+		if (!dbus_connection_send (conn, reply, NULL))
 			syslog(LOG_ERR, "Can't send reply message!");
-		}
 		dbus_message_unref (reply);
 	}
 
@@ -1240,7 +1225,7 @@
 
 	path = dbus_message_get_path(msg);
 	iface = dbus_message_get_interface(msg);
-	method = dbus_message_get_member (msg);
+	method = dbus_message_get_member(msg);
 	signature = dbus_message_get_signature(msg);
 
 	syslog (LOG_INFO, "%s - path:%s", __PRETTY_FUNCTION__, path);
@@ -1264,11 +1249,9 @@
 	if (error)
 		reply = bluez_new_failure_msg(msg, error);
 
-
 	if (reply) {
-		if (!dbus_connection_send (conn, reply, NULL)) {
+		if (!dbus_connection_send (conn, reply, NULL))
 			syslog(LOG_ERR, "Can't send reply message!");
-		}
 		dbus_message_unref (reply);
 	}
 
@@ -1505,13 +1488,10 @@
 	if (hci_switch_role(dd, &bdaddr, role, 10000) < 0) {
 		syslog(LOG_ERR, "Switch role request failed\n");
 		reply = bluez_new_failure_msg(msg, BLUEZ_ESYSTEM_OFFSET + errno);
-	} else {
-		uint8_t result = 0;
-		/* return TRUE to indicate that operation was completed */
-		reply = dbus_message_new_method_return(msg);
-		dbus_message_iter_init_append(reply, &iter);
-		dbus_message_iter_append_basic(&iter, DBUS_TYPE_BYTE, &result);
+		goto failed;
 	}
+	
+	reply = dbus_message_new_method_return(msg);
 
 failed:
 	return reply;
maintainer
#1 lore raw

Hi Johan,

Quoted 2 lines

Here's a cleanup patch for dbus.c which fixes some issues with whitespace, coding-style, unecessary variables, etc.

this is great, but I fixed some more issues. After "LOG_ERR," etc. there should be a whitespace and the syslog() call doesn't need a "\n" at the end ;)

Regards

Marcel

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

#2 lore raw

Hi Marcel,

Quoted 3 lines · Marcel Holtmann

this is great, but I fixed some more issues. After "LOG_ERR," etc. there should be a whitespace and the syslog() call doesn't need a "\n" at the end ;)

Yeah, I seem to have missed those. I guess nobody's perfect ;)

Could you comment on my question about the possibility of including the python test script in the cvs? It would simplify keeping it in sync with the daemon when the API changes.

Johan

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

maintainer
#3 lore raw

Hi Johan,

Quoted 5 lines
Quoted 3 lines

this is great, but I fixed some more issues. After "LOG_ERR," etc. there should be a whitespace and the syslog() call doesn't need a "\n" at the end ;)

Yeah, I seem to have missed those. I guess nobody's perfect ;)

nobody is, but every cleanup is a step in the right direction. So keep them coming.

Quoted 3 lines

Could you comment on my question about the possibility of including the python test script in the cvs? It would simplify keeping it in sync with the daemon when the API changes.

I haven't looked at it, but I have no problem with putting it in the CVS and also including it in the source package. However this will be then a script that will never be installed. Is this ok for you?

Regards

Marcel

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

#4 lore raw
Quoted 3 lines · Marcel Holtmann

I haven't looked at it, but I have no problem with putting it in the CVS and also including it in the source package. However this will be then a script that will never be installed. Is this ok for you?

Yes, that's ok.

Johan

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

maintainer
#5 lore raw

Hi Johan,

Quoted 5 lines
Quoted 3 lines

I haven't looked at it, but I have no problem with putting it in the CVS and also including it in the source package. However this will be then a script that will never be installed. Is this ok for you?

Yes, that's ok.

please include a list of available commands in the usage and I think we should rename it to dbus-test.

Regards

Marcel

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

#6 lore raw
Quoted 2 lines · Marcel Holtmann

please include a list of available commands in the usage and I think we should rename it to dbus-test.

done:
http://www.iki.fi/~jhedberg/bluez-python/dbus-test

Johan

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

maintainer
#7 lore raw

Hi Johan,

Quoted 5 lines
Quoted 2 lines

please include a list of available commands in the usage and I think we should rename it to dbus-test.

done:
http://www.iki.fi/~jhedberg/bluez-python/dbus-test

it is in the CVS now and will also come with next source package, but it will never be installed.

Regards

Marcel

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

#8 lore raw
Quoted 2 lines · Marcel Holtmann

it is in the CVS now and will also come with next source package, but it will never be installed.

Thanks, and here's the first patch for it :-)

Johan

Index: dbus-test
===================================================================
RCS file: /cvsroot/bluez/utils/hcid/dbus-test,v
retrieving revision 1.1

dbus-test +5 -4
diff -u -r1.1 dbus-test
@@ -8,8 +8,9 @@
 from signal import *
 
 mgr_cmds = [ "DeviceList", "DefaultDevice" ]
-dev_cmds = [ "Inquiry", "CancelInquiry", "PeriodicInquiry", "CancelPeriodic",
-             "RemoteName", "Connections", "Authenticate", "RoleSwitch" ]
+dev_cmds = [ "Up", "Down", "Inquiry", "CancelInquiry", "PeriodicInquiry",
+             "CancelPeriodic", "RemoteName", "Connections", "Authenticate",
+             "RoleSwitch" ]
 
 class Tester:
     exit_events = []
@@ -100,8 +101,8 @@
         self.manager.connect_to_signal('DeviceRemoved', self.device_removed)
 
     def usage(self):
-        print 'Usage: %s [-i] [-l] [-h] <cmd> [arg1..]' % self.name
-        print '  -i <dev>   Specify device (e.g. "hci0" or "/dev/bluez/Device/hci0")'
+        print 'Usage: %s [-i <dev>] [-l] [-h] <cmd> [arg1..]' % self.name
+        print '  -i <dev>   Specify device (e.g. "hci0" or "/org/bluez/Device/hci0")'
         print '  -l         Listen for events (no command required)'
         print '  -h         Show this help'
         print 'Manager commands:'
maintainer
#9 lore raw

Hi Johan,

Quoted 4 lines
Quoted 2 lines

it is in the CVS now and will also come with next source package, but it will never be installed.

Thanks, and here's the first patch for it :-)

patch is in the CVS now.

Regards

Marcel

-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download it for free - -and be entered to win a 42" plasma tv or your very own Sony(tm)PSP. Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel