dbus.c: cleanup/fix method parameter getting

Hi,

These patches have the following changes:

1. Return the right error if a matching handler is not found in the device message handler function. Eduardo said he would fix it in his next patch, but I think it's good to get the fix in as soon as possible.

2. Change a couple of places which use DBusMessageIter to use dbus_message_get_args instead (just one function call and removes the need to have the DBusMessageIter variable).

3. Use uint16 instead of uint8 for periodic inquiry period length parameters (since they are two octets each in the HCI spec).

4. Fix checking for the range of inquiry parameters (HCI spec says length 0x01-0x30 and num_resp 0x00-0xFF).

The range checking for periodic inquiry and role switch should probably also be fixed at some point. However, the controller should return an error if invalid values are passed to it, so I'm not even sure that it should be hcid's task to check them. What do you think?

Another thing with the inquiry and periodic inquiry methods is that the inquiry method allows the caller to specify the num_responses parameter while the periodic inquiry method has it hardcoded to 100 currently. Should the parameter also be added to periodic inquiry or should it be removed from normal inquiry?

The reason why I'm submitting the changes in two patches is that there's currently a bug[1] in the python dbus bindings which prevents uint16 values from being sent. The fix will probably go to the next dbus release but we may want to wait for it before applying the change at our end.

[1] http://lists.freedesktop.org/archives/dbus/2005-November/003733.html

Johan

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

diff -u -r1.48 dbus.c
@@ -1222,15 +1222,19 @@
 	}
 
 	if (handlers) {
-		error = BLUEZ_EDBUS_WRONG_SIGNATURE;
 		for (; handlers->name != NULL; handlers++) {
 			if (strcmp(handlers->name, method) == 0) {
+				ret = DBUS_HANDLER_RESULT_HANDLED;
 				if (strcmp(handlers->signature, signature) == 0) {
 					reply = handlers->handler_func(msg, data);
 					error = 0;
-					ret = DBUS_HANDLER_RESULT_HANDLED;
 					break;
 				}
+				else
+					/* Set the error, but continue looping incase there is
+					 * another method with the same name but a different
+					 * signature */
+					error = BLUEZ_EDBUS_WRONG_SIGNATURE;
 
 			}
 		}

@@ -1392,22 +1396,20 @@
 
 static DBusMessage* handle_inq_req(DBusMessage *msg, void *data)
 {
-	DBusMessageIter iter;
 	DBusMessage *reply = NULL;
 	inquiry_cp cp;
 	evt_cmd_status rp;
 	struct hci_request rq;
 	struct hci_dbus_data *dbus_data = data;
 	int dd = -1;
-	int8_t length;
-	int8_t num_rsp;
+	uint8_t length, num_rsp;
 
-	dbus_message_iter_init(msg, &iter);
-	dbus_message_iter_get_basic(&iter, &length);
-	dbus_message_iter_next(&iter);
-	dbus_message_iter_get_basic(&iter, &num_rsp);
+	dbus_message_get_args(msg, NULL,
+			DBUS_TYPE_BYTE, &length,
+			DBUS_TYPE_BYTE, &num_rsp,
+			DBUS_TYPE_INVALID);
 
-	if ((length <= 0) || (num_rsp <= 0)) {
+	if (length < 0x01 || length > 0x30) {
 		reply = bluez_new_failure_msg(msg, BLUEZ_EDBUS_WRONG_PARAM);
 		goto failed;
 	}

@@ -1486,7 +1488,6 @@
 
 static DBusMessage* handle_role_switch_req(DBusMessage *msg, void *data)
 {
-	DBusMessageIter iter;
 	DBusMessage *reply = NULL;
 	char *str_bdaddr = NULL;
 	struct hci_dbus_data *dbus_data = data;

@@ -1494,10 +1495,10 @@
 	uint8_t role;
 	int dev_id = -1, dd = -1;
 
-	dbus_message_iter_init(msg, &iter);
-	dbus_message_iter_get_basic(&iter, &str_bdaddr);
-	dbus_message_iter_next(&iter);
-	dbus_message_iter_get_basic(&iter, &role);
+	dbus_message_get_args(msg, NULL,
+			DBUS_TYPE_STRING, &str_bdaddr,
+			DBUS_TYPE_BYTE, &role,
+			DBUS_TYPE_INVALID);
 
 	str2ba(str_bdaddr, &bdaddr);
 

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

@@ -1546,8 +1546,9 @@
 	remote_name_req_cp cp;
 	evt_cmd_status rp;
 
-	dbus_message_iter_init(msg, &iter);
-	dbus_message_iter_get_basic(&iter, &str_bdaddr);
+	dbus_message_get_args(msg, NULL,
+			DBUS_TYPE_STRING, &str_bdaddr,
+			DBUS_TYPE_INVALID);
 
 	str2ba(str_bdaddr, &bdaddr);
 

@@ -1662,7 +1663,6 @@
 	struct hci_request rq;
 	auth_requested_cp cp;
 	evt_cmd_status rp;
-	DBusMessageIter iter;
 	DBusMessage *reply = NULL;
 	char *str_bdaddr = NULL;
 	struct hci_dbus_data *dbus_data = data;

@@ -1671,8 +1671,10 @@
 	int dev_id = -1;
 	int dd = -1;
 
-	dbus_message_iter_init(msg, &iter);
-	dbus_message_iter_get_basic(&iter, &str_bdaddr);
+	dbus_message_get_args(msg, NULL,
+			DBUS_TYPE_STRING, &str_bdaddr,
+			DBUS_TYPE_INVALID);
+
 	str2ba(str_bdaddr, &bdaddr);
 
 	dev_id = hci_for_each_dev(HCI_UP, find_conn, (long) &bdaddr);

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

diff -u -r1.2 dbus-test
@@ -222,7 +222,7 @@
                 length, min, max = self.cmd_args
             self.listen = True
             try:
-                self.ctl.PeriodicInquiry(dbus.Byte(length), dbus.Byte(min), dbus.Byte(max))
+                self.ctl.PeriodicInquiry(dbus.Byte(length), dbus.UInt16(min), dbus.UInt16(max))
             except dbus.DBusException, e:
                 print 'Sending %s failed: %s' % (self.cmd, e)
                 sys.exit(1)

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

diff -u -r1.48 dbus.c
@@ -1299,12 +1299,11 @@
 {
 	write_inquiry_mode_cp inq_mode;
 	periodic_inquiry_cp inq_param;
-	DBusMessageIter iter;
 	DBusMessage *reply = NULL;
 	struct hci_dbus_data *dbus_data = data;
 	uint8_t length;
-	uint8_t max_period;
-	uint8_t min_period;
+	uint16_t max_period;
+	uint16_t min_period;
 	int dd = -1;
 
 	dd = hci_open_dev(dbus_data->dev_id);

@@ -1314,12 +1313,13 @@
 		goto failed;
 	}
 
-	dbus_message_iter_init(msg, &iter);
-	dbus_message_iter_get_basic(&iter, &length);
-	dbus_message_iter_next(&iter);
-	dbus_message_iter_get_basic(&iter, &min_period);
-	dbus_message_iter_next(&iter);
-	dbus_message_iter_get_basic(&iter, &max_period);
+	dbus_message_get_args(msg, NULL,
+			DBUS_TYPE_BYTE, &length,
+			DBUS_TYPE_UINT16, &min_period,
+			DBUS_TYPE_UINT16, &max_period,
+			DBUS_TYPE_INVALID);
+
+	syslog(LOG_DEBUG, "%02X %04X %04X", length, min_period, max_period);
 
 	if (length >= min_period || min_period >= max_period) {
 		reply = bluez_new_failure_msg(msg, BLUEZ_EDBUS_WRONG_PARAM);

@@ -1329,8 +1329,8 @@
 	inq_param.num_rsp = 100;
 	inq_param.length  = length;
 
-	inq_param.max_period = max_period;
-	inq_param.min_period = min_period;
+	inq_param.max_period = htobs(max_period);
+	inq_param.min_period = htobs(min_period);
 
 	/* General/Unlimited Inquiry Access Code (GIAC) */
 	inq_param.lap[0] = 0x33;

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

diff -u -r1.14 dbus.h
@@ -166,8 +166,8 @@
 
 
 #define HCI_PERIODIC_INQ_SIGNATURE			DBUS_TYPE_BYTE_AS_STRING \
-							DBUS_TYPE_BYTE_AS_STRING \
-							DBUS_TYPE_BYTE_AS_STRING \
+							DBUS_TYPE_UINT16_AS_STRING \
+							DBUS_TYPE_UINT16_AS_STRING \
 							__END_SIG__
 
 #define HCI_CANCEL_PERIODIC_INQ_SIGNATURE		__END_SIG__

Hi Johan,

I decided to take both patches and hopefully the D-Bus Python support will be fixed soon. However I applied another cleanup after your patches that fixes some weird coding that ends up in unnecessary deep indented lines. Please use this style for any loops and make use of labels for the exit path in functions.

What do you think about using also tabs in 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

Hi Marcel,

Will do.

I have nothing fundamental against it. The primary reason why it uses four spaces for indentation currently is that it's the setting I've always had in my .vimrc for python code.

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,

the tabs makes it easy for people to change the visualization of them from four spaces to eight spaces (or others). This is why I prefer of having tabs, but since I am not really good in Python, I better not talk about that coding style ;)

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

we should do our own range check, because the intention is to design an API and not to simply map the Bluetooth HCI to D-Bus. If we have weird parameters that doesn't matter in real or are only for internal inside hcid or the kernel then we leave them out. If we wanna combine to HCI commands into one D-Bus method then we do this to. So please don't try to stick to the HCI specification. Try to design some logical API for Bluetooth.

See my comment above. The question here is if we need this parameter for any method call or should we make it a configurable option. So we have a sane default for num_responses and if somebody wants to use a different one that a second method to change it is needed.

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

Sure. However it would be nice to hear your opinions about e.g. the level of abstraction we should strive for and if there are areas (e.g. SDP) which should be abstracted more than others. At least for RFCOMM I was thinking of doing a similar interface that we have in maemo [1], where you could directly use identifiers such as "DUN" and "FTP" to connect to a service without the need to do SDP separately. Of course there should also be the possibility to use an exact UUID for less common services.

[1] http://www.maemo.org/platform/docs/osso-gwconnect.html

Is there a reason to have a limit at all, i.e. why not always use the value 0 (= unlimited number of responses)? Of course we can add a method for setting another default, but can you give and example of a situation where that would be needed?

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,

I like that idea and since one of my advanced goals is to implement a full SDP client into bluetoothd, this would work out just fine. Actually the D-Bus interface should not use any kind of RFCOMM channel or L2CAP PSM numbers. I consider it more as an API to the Bluetooth profiles and not the protocols. So in the case of RFCOMM <-> TTY assignments we may only wanna use bdaddr + uuid as parameters. The UUID then can be a clear text value like "dun" or "ftp" or a hex value like "0x1111". For the BD_ADDR it would also make sense if this can be an alias or the remote name. Maybe changing the parameter name to device would be better.

Since the current SDP library is blocking, we would need some SDP client code that integrates nicely into an event loop.

The num_responses and the IAC should be separate setting. It would be great if this setting is specific to the D-Bus session of the caller. In this case every D-Bus user can have its own settings. Do you think this is possible?

For the time (length) of the inquiry I am not sure at the moment.

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

Sure. We could either maintain a table of settings for each D-BUS peer using the unique name that the bus assigns for each connection, or we could also do the settings based on the unix user id of the client application (using the dbus_bus_get_unix_user function).

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,

I prefer the per each D-Bus peer thing, because this gives us the most flexibility for the applications. A general per user settings would be nice too, but maybe a little bit too much at the moment. Can you try to implement it and send me a patch.

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

I was trying understand all discussed things in this email... Sorry if I am writing irrrelevant thing. :)

More than one client is not a common scenario, but we need handle it! IMHO, control a per D-Bus client connection will be a little bit hard, why not support Inquiry, PeriodicInquiry with more than one signature? In the current implementation is easy make this change.

Use a per client connection identification will be nice if we have a peer responses instead of using signals. This will be nice when the kernel cache can be retrieved.

I didn't understand how the client id works. Could you explain a little bit more? Every time that a client establish a connection with the d-bus daemon the connection id changes.
Therefore control client id is useful only for clients that is always connected with the daemon.

Regards,
Claudio.

In

is

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

The id I was talking about is in practice what you get when you call dbus_message_get_sender for a received message. The D-BUS spec. calls it "Unique name of the sending connection".

True, and that was also one thing I was worried about when it comes to the practicality of that solution (the settings wouldn't be persistent enough). So, I think using the user id might be better in this respect.

However, let me now introduce a third option which we just discussed on IRC with Claudio: optional D-BUS method parameters. It would be quite easy to implement the methods so that the client could leave out most parameters in which case hcid (or bluetoothd) would use some set of default values. You could e.g. call the Inquiry method without any parameters and it would just work (kind of the same way that dbus-test allows doing "./dbus-test Inquiry" in which case it uses length=10 & num_resp=100 as the default values). If the client insists on using its own values for the method it would simply include them in the method call.

In case this variable parameters solution doesn't work out for all scenarios it could probably happily co-exist with the per-user id settings solution.

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,

I don't think so, because this options might be used not very often and so having them per unix user doesn't help. Two different applications running as them same user might have different needs. One wants to do inquiry with another IAC, the other with the general IAC. This tends to be a race condition.

Let's do this proposal first.

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

Ok, I see your point. I didn't quite understand the use-case you were thinking of earlier. One extra requirement that the dbus-id settings has is that we will have to monitor the lifetime of a D-BUS client (by listening NameOwnerChanged D-BUS signals) and remove the corresponding entry from the settings table when an id gets removed from the bus.

Ok, I'll begin by doing a patch for Inquiry and PeriodicInquiry (probably during tomorrow).

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,

this might be too much work for the current interface. Or do you think it can be easily implemented?

This would be great. For the next release I like to have the inquiry (or better name it device discovery) working. This will include some nice support for the extended inquiry. So you will get the remote name for free with a discovery. So is it possible to have different signatures for signals too?

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

It will require a moderate amount of coding, so it wont be easy at least in that respect. However, I think that implementing D-BUS client lifetime monitoring is a feature that we will inevitably have to implement at some point anyway. It may e.g. be needed for noticing if a program which requested an RFCOMM connection suddenly died (in which case hcid/bluetoothd should disconnect that connection). Also, I've already done it once (for dbus 0.23.4 though) for the D-BUS API we have in maemo, so there shouldn't come any "supprises" when implementing it.

Sure, and if the new signature differs only by having extra parameters at the end it should even be backwards compatible (i.e. dbus_message_get_args call for the old signature works also for the new signature).

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,

then lets do it. Or to be more precise, you do it ;)

Then I think that I will include the device name in the inquiry response signal and leave further extensions to a different signature.

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