Re: [Bluez-devel] [DBUS-PATCH] cleanup

Re: [Bluez-devel] [DBUS-PATCH] cleanup

[Claudio Takahasi]

[-- Attachment #1.1: Type: text/plain, Size: 2532 bytes --]

Minor changes:
- removed some logs
- added verification for dbus_connection_list_registered function. It can
return FALSE if there is no memory to allocate the child entries


Regards,
Claudio.

On 10/14/05, Claudio Takahasi <cktakahasi@gmail.com> wrote:
>
>
>
> On 10/14/05, Marcel Holtmann <marcel@holtmann.org> wrote:
> >
> > Hi Claudio,
> >
> > > The attached patch implements the service for list the registered
> > > paths under the path /org/bluez/Manager.
> > >
> > > The message request signature can be:
> > > 1. ListPaths() - list all paths under /org/bluez/Manager
> > > 2. ListPaths(String), The string is the relative path. eg: "hci0",
> > > "default" - list all paths provided by one adapter
> >
> > do we really need this?
>
>
> [Claudio Takahasi]
> How clients will discover the active paths if we change the format?
> eg: "hci0" to "AA_BB_CC_DD_EE_FF"
>
> The developers can use the dbus.h constants to compose the path, but if we
> provide this service. It will be possible check if the path is active and/or
> check if it provide a specific profile(pan,rfcomm).
>
> When a client send a msg to a unregistered path, it will receive a reply
> with "Device path is not registered" message.
>
> I agree that at the moment this service is not much useful, the final
> decision is yours :)
>
>
> > Other minor changes:
> > > - removed some logs
> > > - added verification for dbus_connection_list_registered function. It
> > > can return FALSE if there is no memory
> > > to allocate the child entries
> >
> > Don't send combined patches. If you have cleanups then send them as a
> > separate patch.
>
>
> [Claudio Takahasi]
> Ok. I will send soon.
>
> Regards
> >
> > Marcel
> >
> >
> >
> >
> > -------------------------------------------------------
> > This SF.Net email is sponsored by:
> > Power Architecture Resource Center: Free content, downloads,
> > discussions,
> > and more. http://solutions.newsforge.com/ibmarch.tmpl
> > _______________________________________________
> > Bluez-devel mailing list
> > Bluez-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/bluez-devel
> >
>
>
>
> --
> ---------------------------------------------------------
> Claudio Takahasi
> Nokia's Institute of Technology - INdT
> claudio.takahasi@indt.org.br
>



--
---------------------------------------------------------
Claudio Takahasi
Nokia's Institute of Technology - INdT
claudio.takahasi@indt.org.br

[-- Attachment #1.2: Type: text/html, Size: 4356 bytes --]

[-- Attachment #2: cleanup_01.patch --]
[-- Type: application/octet-stream, Size: 5964 bytes --]

--- bluez-utils-cvs.orig/hcid/dbus.c	2005-10-14 08:13:43.000000000 -0300
+++ bluez-utils-cvs-hcid/hcid/dbus.c	2005-10-14 10:57:54.000000000 -0300
@@ -631,8 +631,6 @@
 		return FALSE;
 	}
 
-	syslog(LOG_INFO,"Registered %s object", DEVICE_PATH);
-
 	data = malloc(sizeof(struct hci_dbus_data));
 	if (data == NULL)
 		return FALSE;
@@ -645,8 +643,6 @@
 		return FALSE;
 	}
 
-	syslog(LOG_INFO, "Registered %s object", MANAGER_PATH);
-
 	if (!dbus_connection_add_filter(connection, hci_signal_filter, NULL, NULL)) {
 		syslog(LOG_ERR, "Can't add new HCI filter");
 		return FALSE;
@@ -673,7 +669,7 @@
 		return;
 
 	if (dbus_connection_get_object_path_data(connection,
-						DEVICE_PATH, &data)) {
+				DEVICE_PATH, &data)) {
 		if (data) {
 			free(data);
 			data = NULL;
@@ -682,11 +678,9 @@
 
 	if (!dbus_connection_unregister_object_path(connection, DEVICE_PATH))
 		syslog(LOG_ERR, "Can't unregister %s object", DEVICE_PATH);
-	else
-		syslog(LOG_INFO, "Unregistered %s object", DEVICE_PATH);
 
 	if (dbus_connection_get_object_path_data(connection,
-						MANAGER_PATH, &data)) {
+				MANAGER_PATH, &data)) {
 		if (data) {
 			free(data);
 			data = NULL;
@@ -695,60 +689,56 @@
 
 	if (!dbus_connection_unregister_object_path(connection, MANAGER_PATH))
 		syslog(LOG_ERR, "Can't unregister %s object", MANAGER_PATH);
-	else
-		syslog(LOG_INFO, "Unregistered %s object", MANAGER_PATH);
 
-	dbus_connection_list_registered(connection, fst_parent, &fst_level);
+	if (dbus_connection_list_registered(connection, fst_parent, &fst_level)) {
 
-	for (; *fst_level; fst_level++) {
-		ptr1 = *fst_level;
-		sprintf(snd_parent, "%s/%s", fst_parent, ptr1);
+		for (; *fst_level; fst_level++) {
+			ptr1 = *fst_level;
+			sprintf(snd_parent, "%s/%s", fst_parent, ptr1);
 
-		dbus_connection_list_registered(connection, snd_parent, &snd_level);
+			if (dbus_connection_list_registered(connection, snd_parent, &snd_level)) {
 
-		if (!(*snd_level)) {
-			sprintf(path, "%s/%s", MANAGER_PATH, ptr1);
+				if (!(*snd_level)) {
+					sprintf(path, "%s/%s", MANAGER_PATH, ptr1);
 
-			syslog(LOG_INFO, "Unregistered %s object", path);
-
-			if (dbus_connection_get_object_path_data(connection,
+					if (dbus_connection_get_object_path_data(connection,
 								path, &data)) {
-				if (data) {
-					free(data);
-					data = NULL;
-				}
-			}
+						if (data) {
+							free(data);
+							data = NULL;
+						}
+					}
 
-			if (!dbus_connection_unregister_object_path(connection, path))
-				syslog(LOG_ERR, "Can't unregister %s object", path);
+					if (!dbus_connection_unregister_object_path(connection, path))
+						syslog(LOG_ERR, "Can't unregister %s object", path);
 
-			continue;
-		}
-
-		for (; *snd_level; snd_level++) {
-			ptr2 = *snd_level;
-			sprintf(path, "%s/%s/%s", MANAGER_PATH, ptr1, ptr2);
+					continue;
+				}
 
-			syslog(LOG_INFO, "Unregistered %s object", path);
+				for (; *snd_level; snd_level++) {
+					ptr2 = *snd_level;
+					sprintf(path, "%s/%s/%s", MANAGER_PATH, ptr1, ptr2);
 
-			if (dbus_connection_get_object_path_data(connection,
+					if (dbus_connection_get_object_path_data(connection,
 								path, &data)) {
-				if (data) {
-					free(data);
-					data = NULL;
+						if (data) {
+							free(data);
+							data = NULL;
+						}
+					}
+
+					if (!dbus_connection_unregister_object_path(connection, path))
+						syslog(LOG_ERR, "Can't unregister %s object", path);
 				}
-			}
 
-			if (!dbus_connection_unregister_object_path(connection, path))
-				syslog(LOG_ERR, "Can't unregister %s object", path);
+				if (*snd_level)
+					dbus_free_string_array(snd_level);
+			}
 		}
 
-		if (*snd_level)
-			dbus_free_string_array(snd_level);
+		if (*fst_level)
+			dbus_free_string_array(fst_level);
 	}
-
-	if (*fst_level)
-		dbus_free_string_array(fst_level);
 }
 
 gboolean hcid_dbus_register_device(uint16_t id)
@@ -806,9 +796,8 @@
 
 	/* register the default path*/
 	if (!dft_reg) {
-		sprintf(path, "%s/%s/%s", MANAGER_PATH, HCI_DEFAULT_DEVICE_NAME, BLUEZ_HCI);
 
-		syslog(LOG_INFO, "registering dft path:%s - id:%d", path, DEFAULT_DEVICE_PATH_ID);
+		sprintf(path, "%s/%s/%s", MANAGER_PATH, HCI_DEFAULT_DEVICE_NAME, BLUEZ_HCI);
 
 		data = malloc(sizeof(struct hci_dbus_data));
 		if (data == NULL)
@@ -831,8 +820,6 @@
 	/* register the default path*/
 	sprintf(path, "%s/%s%d/%s", MANAGER_PATH, HCI_DEVICE_NAME, id, BLUEZ_HCI);
 
-	syslog(LOG_INFO, "registering  -  path:%s - id:%d",path, id);
-
 	if (!dbus_connection_register_object_path(conn, path, &obj_vtable, data)) {
 		syslog(LOG_ERR,"DBUS failed to register %s object", path);
 		/* ignore, the path was already registered */
@@ -859,8 +846,9 @@
 	void *data = NULL;
 
 	if (unreg_dft) {
+
 		sprintf(dft_path, "%s/%s/%s", MANAGER_PATH, HCI_DEFAULT_DEVICE_NAME, BLUEZ_HCI);
-		syslog(LOG_INFO, "%s - unregistering dft:%s", __PRETTY_FUNCTION__, dft_path);
+
 		if (!dbus_connection_unregister_object_path (connection, dft_path)) {
 			syslog(LOG_ERR,"DBUS failed to unregister %s object", dft_path);
 			ret = -1;
@@ -875,7 +863,7 @@
 	}
 
 	sprintf(path, "%s/%s%d/%s", MANAGER_PATH, HCI_DEVICE_NAME, id, BLUEZ_HCI);
-	syslog(LOG_INFO, "%s - unregistering spec:%s", __PRETTY_FUNCTION__, path);
+
 	if (!dbus_connection_unregister_object_path (connection, path)) {
 		syslog(LOG_ERR,"DBUS failed to unregister %s object", path);
 		ret = -1;
@@ -1315,7 +1303,6 @@
 			reply = dbus_message_new_method_return(msg);
 			dbus_message_iter_init_append(reply, &iter);
 			dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &pname);
-			syslog(LOG_INFO, "Remote Name: %s", pname);
 		} else {
 			reply = bluez_new_failure_msg(msg, BLUEZ_ESYSTEM_OFFSET + errno);
 		}
@@ -1410,7 +1397,7 @@
 
 /*****************************************************************
  *  
- *  Section reserved to Device D-Bus message handlers
+ *  Section reserved to Manager D-Bus message handlers
  *  
  *****************************************************************/
 

Re: [Bluez-devel] [DBUS-PATCH] cleanup

[Marcel Holtmann]

Hi Claudio,

> Minor changes:
>  - removed some logs
>  - added verification for dbus_connection_list_registered function. It
> can return FALSE if there is no memory to allocate the child entries

the patch is now in the CVS. The whitespace thing is getting better and
I hope you use an editor that is visualizing them for you.

There are some programming styles in the code that we need to change,
because this code is getting ugly otherwise. Here is a bad example:

void func(void)
{
	if (test) {
		do_something();
	}
}

This will end up in nesting statements and you have to indent to deep to
keep it readable somehow. So the preferred way is:

void func(void)
{
	if (!test)
		return;

	do_something();
}

If a function is some lines longer this is not a big problem for the
understanding of the code, but if it is indented to deep then the code
becomes hard to read.

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [DBUS-PATCH] cleanup

[Marcel Holtmann]

Hi Claudio,

> There are some programming styles in the code that we need to change,
> because this code is getting ugly otherwise.

here are some more coding style things I like to see. This is the bad
example:

	for (i = 0; i < 10; i++) {
		if (test_something()) {
			do_something();
		}
	}

This will end up in some very nested loops. So the better way to write
it is this:

	for (i = 0; i < 10; i++) {
		if (!test_something())
			continue;

		do_something();
	}

Please keep these things in mind when coding. It will make it a lot more
readable and understandable for outsiders.

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel