dbus.c cleanup

Hi,

Here's a cleanup patch for the hcid dbus code. It changes several sprintf calls to snprintf (better to risk string truncation than buffer overflow), as well as removes unecessary byte parameters from the replys of periodic inquiry methods.

Johan

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

diff -u -r1.36 dbus.c
@@ -886,12 +886,12 @@
 
 		for (; *fst_level; fst_level++) {
 			ptr1 = *fst_level;
-			sprintf(snd_parent, "%s/%s", fst_parent, ptr1);
+			snprintf(snd_parent, sizeof(snd_parent), "%s/%s", fst_parent, ptr1);
 
 			if (dbus_connection_list_registered(connection, snd_parent, &snd_level)) {
 
 				if (!(*snd_level)) {
-					sprintf(path, "%s/%s", MANAGER_PATH, ptr1);
+					snprintf(path, sizeof(path), "%s/%s", MANAGER_PATH, ptr1);
 
 					if (dbus_connection_get_object_path_data(connection,
 								path, &data)) {

@@ -909,7 +909,7 @@
 
 				for (; *snd_level; snd_level++) {
 					ptr2 = *snd_level;
-					sprintf(path, "%s/%s/%s", MANAGER_PATH, ptr1, ptr2);
+					snprintf(path, sizeof(path), "%s/%s/%s", MANAGER_PATH, ptr1, ptr2);
 
 					if (dbus_connection_get_object_path_data(connection,
 								path, &data)) {

@@ -935,12 +935,12 @@
 
 gboolean hcid_dbus_register_device(uint16_t id) 
 {
-	char path[MAX_PATH_LENGTH+1];
-	char dev[BLUETOOTH_DEVICE_NAME_LEN+1];
+	char path[MAX_PATH_LENGTH];
+	char dev[BLUETOOTH_DEVICE_NAME_LEN];
 	const char *pdev = dev;
 
-	snprintf(dev, BLUETOOTH_DEVICE_NAME_LEN, HCI_DEVICE_NAME "%d", id);
-	snprintf(path, MAX_PATH_LENGTH, "%s/%s", DEVICE_PATH, pdev);
+	snprintf(dev, sizeof(dev), HCI_DEVICE_NAME "%d", id);
+	snprintf(path, sizeof(path), "%s/%s", DEVICE_PATH, pdev);
 
 	/* register the default path*/
 	return register_dbus_path(path, id);

@@ -948,12 +948,12 @@
 
 gboolean hcid_dbus_unregister_device(uint16_t id)
 {
-	char dev[BLUETOOTH_DEVICE_NAME_LEN+1];
-	char path[MAX_PATH_LENGTH+1];
+	char dev[BLUETOOTH_DEVICE_NAME_LEN];
+	char path[MAX_PATH_LENGTH];
 	const char *pdev = dev;
 
-	snprintf(dev, BLUETOOTH_DEVICE_NAME_LEN, HCI_DEVICE_NAME "%d", id);
-	snprintf(path, MAX_PATH_LENGTH, "%s/%s", DEVICE_PATH, pdev);
+	snprintf(dev, sizeof(dev), HCI_DEVICE_NAME "%d", id);
+	snprintf(path, sizeof(path), "%s/%s", DEVICE_PATH, pdev);
 
 	return unregister_dbus_path(path);
 }

@@ -986,7 +986,7 @@
 		goto failed;
 	}
 
-	sprintf(dev, HCI_DEVICE_NAME "%d", id);
+	snprintf(dev, sizeof(dev), HCI_DEVICE_NAME "%d", id);
 
 	dbus_message_iter_init_append(message, &iter);
 	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING ,&pdev);

@@ -1036,7 +1036,7 @@
 		goto failed;
 	}
 
-	sprintf(dev, HCI_DEVICE_NAME "%d", id);
+	snprintf(dev, sizeof(dev), HCI_DEVICE_NAME "%d", id);
 
 	dbus_message_iter_init_append(message, &iter);
 	dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING ,&pdev);

@@ -1072,12 +1072,12 @@
 
 	/* register the default path*/
 	if (!dft_reg) {
-		sprintf(path, "%s/%s/%s", MANAGER_PATH, HCI_DEFAULT_DEVICE_NAME, BLUEZ_HCI);
+		snprintf(path, sizeof(path), "%s/%s/%s", MANAGER_PATH, HCI_DEFAULT_DEVICE_NAME, BLUEZ_HCI);
 		register_dbus_path(path, DEFAULT_DEVICE_PATH_ID);
 	}
 
 	/* register the default path*/
-	sprintf(path, "%s/%s%d/%s", MANAGER_PATH, HCI_DEVICE_NAME, id, BLUEZ_HCI);
+	snprintf(path, sizeof(path), "%s/%s%d/%s", MANAGER_PATH, HCI_DEVICE_NAME, id, BLUEZ_HCI);
 	register_dbus_path(path, id);
 
 	return 0;

@@ -1099,11 +1099,11 @@
 	char path[MAX_PATH_LENGTH];
 
 	if (unreg_dft) {
-		sprintf(path, "%s/%s/%s", MANAGER_PATH, HCI_DEFAULT_DEVICE_NAME, BLUEZ_HCI);
+		snprintf(path, sizeof(path), "%s/%s/%s", MANAGER_PATH, HCI_DEFAULT_DEVICE_NAME, BLUEZ_HCI);
 		unregister_dbus_path(path);
 	}
 
-	sprintf(path, "%s/%s%d/%s", MANAGER_PATH, HCI_DEVICE_NAME, id, BLUEZ_HCI);
+	snprintf(path, sizeof(path), "%s/%s%d/%s", MANAGER_PATH, HCI_DEVICE_NAME, id, BLUEZ_HCI);
 	unregister_dbus_path(path);
 
 	return ret;

@@ -1318,14 +1318,10 @@
 		syslog(LOG_ERR, "Can't send HCI commands:%s.", strerror(errno));
 		reply = bluez_new_failure_msg(msg, BLUEZ_ESYSTEM_OFFSET + errno);
 		goto failed;
-	} 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);
 	}
 
+	reply = dbus_message_new_method_return(msg);
+
 failed:
 	if (dd >= 0)
 		close(dd);

@@ -1335,7 +1331,6 @@
 
 static DBusMessage* handle_cancel_periodic_inq_req(DBusMessage *msg, void *data)
 {
-	DBusMessageIter iter;
 	DBusMessage *reply = NULL;
 	struct hci_dbus_data *dbus_data = data;
 	int dd = -1;

@@ -1361,14 +1356,11 @@
 	if (hci_send_cmd(dd, OGF_LINK_CTL, OCF_EXIT_PERIODIC_INQUIRY, 0 , NULL) < 0) {
 		syslog(LOG_ERR, "Send hci command failed.");
 		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:
 	if (dd >= 0)
 		close(dd);

Hi Johan,

the patch is in the CVS now. Are there more cleanups coming? I think it is time for the next release.

Regards

Marcel

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi Marcel,

At least not from me. However I'm not sure about the INdT guys.

Johan

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi Johan,

I still consider the D-Bus interface as experimental and that we might change it with further notification, but it will be good to have a new release out there.

So I think that I will do some pending cleanups in the CVS and then release a new version.

Regards

Marcel

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi,

I don't have cleanup patches to be sent. A new release can be good to receive the users/developers feedback.

Some cleanups/improvements suggestions for the next release:
- find_conn is a shared function: put in a shared directory. It's used by hcitool.c and dbus.c
- Disconnect signal: handle de disconnect signal sent when the dbus-daemon exits.
- split dbus.c file:
- use session bus to send the PinRequest message: using the session bus it's possible start the dbus pin helper application automatically

I think Eduardo is working on some services device configuration services. Maybe it should be better wait his patch. I will ask him and confirm soon.

Regards,
Claudio.

--
---------------------------------------------------------
Claudio Takahasi
Instituto Nokia de Tecnologia - INdT

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi Claudio,

I don't think that's the right thing or even a possible thing to do. This is because hcid is a system-wide daemon and cannot know about users session busses.

Johan

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi Johan,

so this is not working out. Then we still need a program that starts a session time. Does anyone work on a Bluetooth applet for Gnome that will also handle the PIN requests?

Regards

Marcel

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi Marcel,

I've implemented the DeviceList function from /org/bluez/Devices. I've also made some code clean up in dbus.c. Right now I'm hunting a segfault and as soon I have fixed this I'll send another patch.

BR,
Eduardo.

--
Eduardo Rocha
Instituto Nokia de Tecnologia - INdT

s

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi Claudio,

I have to do some other small fixes in other parts of bluez-utils, but the release is actually planned for today.

I know, but this has time. Maybe you wanna move it into lib.c inside the hcid directory first.

I have no idea what that means.

This sounds like a good idea. Is anybody working on it? However the bluez-pin package needs to updated to support the latest D-Bus version, because we don't support the D-Bus 0.23 anymore.

There is plenty of time after the release. I don't consider the current D-Bus API as stable.

Regards

Marcel

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi Marcel,

Regarding the dbus pin helper app, I am trying develop a app using python for test purpose, based on pygtk and pydbus. However, there is an problem with asynchronous request, I sent an email to dbus mailing list, but according with the received answer, I think will be not easy implement the functionality that I want.

[Claudio Takahasi]
Probably in the future when we start develop another features(rfcomm, pan, ...) we will find more shared functions. I will put the find_conn to a lib.c(inside de hcid) directory first according with your suggestion. In the future we can discuss it later again. Another file that I want create is the error.c

[Claudio Takahasi]
When the dbus-daemon exits, the hcid will receive a "Disconnect" signal. The hcid can start a task to re-establish the bus connection each X seconds or it can exit.
Currently, hcid catches the signal, but there is no action associated with this signal.

[Claudio Takahasi]
I updated the bluez-pin perform authentication tests. I will analize it and send
a patch to fix it.

--
---------------------------------------------------------
Claudio Takahasi
Instituto Nokia de Tecnologia - INdT

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi Claudio,

send me a separate patch for the move to the lib.c file.

What do you wanna put into the error.c file. Explain me your idea behind it and the benefits.

I see and it seems we need a patch to deal with the dbus-daemon exits in case of restarts or errors in it. Please come up with a patch for it.

Regards

Marcel

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi Marcel,

I will wait the D-Bus path changes before submit this changes.

Regarding the error.c file it's just organization, I would like move all error related function to other file.

Regards,
Claudio

--
---------------------------------------------------------
Claudio Takahasi
Instituto Nokia de Tecnologia - INdT

-------------------------------------------------------
This SF.Net email is sponsored by the JBoss Inc.
Get Certified Today * Register for a JBoss Training Course Free Certification Exam for All Training Attendees Through End of 2005 Visit http://www.jboss.com/services/certification for more information
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Hi,

here is patch to move the find_conn function to lib.c file.

Regarding the error related functions, can I move to a error.c file or it's better keep them in the dbus.c?

Another point is the Disconnect signal. Which approach you preffer exit or create a timer to re-connect each x seconds?

IMHO, I think it not necessary handle this scenario. Restart the dbus-daemon is not common.

Regards,
Claudio.

--
---------------------------------------------------------
Claudio Takahasi
Instituto Nokia de Tecnologia - INdT

@@ -23,10 +23,6 @@
  *
  */
 
-#include <syslog.h>
-#include <sys/types.h>
-#include <sys/ioctl.h>
-
 #include <bluetooth/bluetooth.h>
 #include <bluetooth/hci.h>
 

@@ -153,32 +149,3 @@
 int read_link_key(bdaddr_t *local, bdaddr_t *peer, unsigned char *key);
 int read_pin_code(bdaddr_t *local, bdaddr_t *peer, char *pin);
 
-static inline int find_conn(int dd, int dev_id, long arg)
-{
-	struct hci_conn_list_req *cl;
-	struct hci_conn_info *ci;
-	int i;
-
-	cl = malloc(10 * sizeof(*ci) + sizeof(*cl));
-	if (!cl) {
-		syslog(LOG_ERR, "Can't allocate memory");
-		return 0;
-	}
-
-	cl->dev_id = dev_id;
-	cl->conn_num = 10;
-	ci = cl->conn_info;
-
-	if (ioctl(dd, HCIGETCONNLIST, (void *) cl)) {
-		syslog(LOG_ERR, "Can't get connection list");
-		return 0;
-	}
-
-	for (i = 0; i < cl->conn_num; i++, ci++)
-		if (!bacmp((bdaddr_t *) arg, &ci->bdaddr))
-			return 1;
-
-	free(cl);
-
-	return 0;
-}

@@ -29,6 +29,7 @@
 #include <errno.h>
 #include <unistd.h>
 #include <string.h>
+#include <signal.h>
 #include <syslog.h>
 #include <sys/ioctl.h>
 #include <sys/socket.h>

@@ -42,6 +43,7 @@
 #include "glib-ectomy.h"
 
 #include "hcid.h"
+#include "lib.h"
 #include "dbus.h"
 
 static DBusConnection *connection;

@@ -25,6 +25,8 @@
 
 #include <errno.h>
 
+int find_conn(int dd, int dev_id, long arg);
+
 char *expand_name(char *dst, int size, char *str, int dev_id);
 
 char *get_host_name(void);

@@ -34,6 +34,8 @@
 #include <stdarg.h>
 #include <string.h>
 #include <signal.h>
+#include <sys/ioctl.h>
+#include <syslog.h>
 #include <sys/socket.h>
 
 #include <bluetooth/bluetooth.h>

@@ -45,6 +47,36 @@
 
 volatile sig_atomic_t __io_canceled;
 
+int find_conn(int dd, int dev_id, long arg)
+{
+	struct hci_conn_list_req *cl;
+	struct hci_conn_info *ci;
+	int i;
+
+	cl = malloc(10 * sizeof(*ci) + sizeof(*cl));
+	if (!cl) {
+		syslog(LOG_ERR, "Can't allocate memory");
+		return 0;
+	}
+
+	cl->dev_id = dev_id;
+	cl->conn_num = 10;
+	ci = cl->conn_info;
+
+	if (ioctl(dd, HCIGETCONNLIST, (void *) cl)) {
+		syslog(LOG_ERR, "Can't get connection list");
+		return 0;
+	}
+
+	for (i = 0; i < cl->conn_num; i++, ci++)
+		if (!bacmp((bdaddr_t *) arg, &ci->bdaddr))
+			return 1;
+
+	free(cl);
+
+	return 0;
+}
+
 /* 
  * Device name expansion 
  *   %d - device id

Hi Claudio,

I just realized that I already moved it out of dbus.c and so lets keep it in hcid as inline function for now.

At the moment, I think it is better to keep them there. I don't have any real opinion at the moment and so it might be better not to move to much code around.

We should try to re-connect. The security is still running and so we can't exit.

Every time you update your D-Bus installation. The Ubuntu guys even popup a message box that you should reboot your system ;)

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

At least in debian there's a /etc/dbus-1/event.d directory where scripts are automatically run when the dbus daemons starts or stops. E.g. hal installs its own script there for starting/stoping hald at the right moment.

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

Hi Johan,

as long as D-Bus support is optional, this is no option.

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

Hi,

Hal also add a timer to try re-initialize the connection and register the paths when the "Disconnect" signal is received.

Ok guys I will try implement a similar HAL approach :)

Regards,
Claudio.

oad

--
---------------------------------------------------------
Claudio Takahasi
Instituto Nokia de Tecnologia - INdT

-------------------------------------------------------
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