Bug 10577 (net.bluetooth)

Summary: Bluetooth Network Capability
Product: hal Reporter: Luiz Augusto von Dentz <luiz.dentz>
Component: haldAssignee: Luiz Augusto von Dentz <luiz.dentz>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: alexdeucher, marcel
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: net.bluetooth patch
Patch 1 of 5
Patch 2 of 5
Patch 3 of 5
Patch 4 of 5
Patch 5 of 5
Patch 1 of 2
Patch 2 of 2
Patch 1 of 2
Patch 2 of 2

Description Luiz Augusto von Dentz 2007-04-09 10:33:45 UTC
hald detects a bluetooth network interface as a wired interface.
Comment 1 Luiz Augusto von Dentz 2007-04-09 10:38:15 UTC
Created attachment 9529 [details] [review]
net.bluetooth patch

Make hald detect bluetooth network interface and gives net.bluetooth capability to it.
Comment 2 Luiz Augusto von Dentz 2007-04-11 14:25:52 UTC
Created attachment 9566 [details] [review]
Patch 1 of 5

Add a helper to recovery service record information from bluez.
Comment 3 Luiz Augusto von Dentz 2007-04-11 14:27:12 UTC
Created attachment 9567 [details] [review]
Patch 2 of 5

Changes to user the new helper.
Comment 4 Luiz Augusto von Dentz 2007-04-12 12:35:01 UTC
Created attachment 9585 [details] [review]
Patch 3 of 5

Use FindConnection to avoid dbus round trips.
Comment 5 Luiz Augusto von Dentz 2007-04-12 14:40:01 UTC
Created attachment 9586 [details] [review]
Patch 4 of 5

Removes net.bluetooth.description
Comment 6 Luiz Augusto von Dentz 2007-04-12 14:41:12 UTC
Created attachment 9587 [details] [review]
Patch 5 of 5

Updates hal spec to include net.bluetooth capability and related properties.
Comment 7 Luiz Augusto von Dentz 2007-04-12 15:31:33 UTC
Created attachment 9588 [details] [review]
Patch 1 of 2

Unified patch
Comment 8 Luiz Augusto von Dentz 2007-04-12 15:32:22 UTC
Created attachment 9589 [details] [review]
Patch 2 of 2

Unified patch
Comment 9 David Zeuthen (not reading bugmail) 2007-04-16 10:52:20 UTC
+              <entry>
+                <literal>net.bluetooth.name</literal> (string)
+              </entry>
+              <entry>example: Network Access Point Service</entry>
+              <entry>
+                Only if the <literal>net.bluetooth</literal> capability is set
+              </entry>
+              <entry>Displayable Name</entry>

Where does this come from? What does it mean?

+            </row>
+            <row>
+              <entry>
+                <literal>net.bluetooth.uuid</literal> (string)
+              </entry>
+              <entry>example: 00001116-0000-1000-8000-00805f9b34fb</entry>
+              <entry>
+                Only if the <literal>net.bluetooth</literal> capability is set
+              </entry>
+              <entry>Universal Unique IDentifier</entry>

What exactly is this identifier?

+			/* Leave those properties empty so helper may fill it up */
+			hal_device_property_set_string (d, "net.bluetooth.name", "");
+			hal_device_property_set_string (d, "net.bluetooth.uuid", "");

Probably no need to do this.

+	msg = dbus_message_new_method_call (BLUEZ_SERVICE, BLUEZ_PATH,
+										BLUEZ_MANAGER_IFACE,
+										"ActivateService");

Will this call have a side effect? What's the semantics of ActivateService? I'm
asking because this smells a bit like policy; we cannot make HAL do any policy;
it's just a mechanism.. and all settings/policy _need_ to originate from the
desktop session otherwise this will never work for multi-user (e.g. fast-user-
switching, multi-seat, hot-desking etc.)

+	dbus_message_get_args (reply, &error, DBUS_TYPE_STRING, &connection,
+							DBUS_TYPE_INVALID);
+	if (dbus_error_is_set (&error)) {
+		dbus_error_free (&error);
+		goto out;
+	}
+
+	name = get_property (conn, id, connection, "GetName");
+	if (name == NULL)
+		goto out;
+
+	libhal_device_set_property_string (ctx, udi, "net.bluetooth.name", name,
+										&error);
+
+	uuid = get_property (conn, id, connection, "GetUUID");
+	if (uuid == NULL)
+		goto out;

Hmm.. Probably the Bluez API should return both Name + UUID as part of
ActivateService so we don't need to do roundtrips; but this is fine for now.

Also, what happens if any of these calls fail? E.g. what if Bluez is not
available? I think it may be good enough to just make hald remove the
net.bluetooth capability if the helper returns with an exit code != 0. 
Thoughts?
Comment 10 Marcel Holtmann 2007-04-16 11:26:02 UTC
The name property is basically a friendly name of the network service. It is part of the SDP record and so not exported through the kernel sysfs integration.

The uuid property is the profile UUID of the used remote service. In this case it can be a NAP (network access point) or GN (group network). Using the UUID makes it really simple to integrate other profiles under the same capability. For example the old LAN access network or even ISDN.

The ActivateService() method will start the network service daemon if needed. Nothing more. The call is needed since it returns the unique bus name of the network service. Currently we don't use well known names for Bluetooth services. All the information are provided by the network service and not the Bluetooth core daemon.

We can add a dict based method to return all information at once to reduce the round trips to the network service. We have done this for the core daemon, but not yet for the service implementation. It will provide a GetInfo() method at some point.

If any of these calls fail, we still should add net.bluetooth as capability. So the helper needs to be fixed to exit without an error code in this case. Maybe some error text through syslog or so is the better approach.
Comment 11 Luiz Augusto von Dentz 2007-04-16 13:06:13 UTC
Currently the helper always return 0, so even if the interface was configured without bluez network service it will still be available and announced by hal.

 +                       /* Leave those properties empty so helper may fill it
up */
+                       hal_device_property_set_string (d,
"net.bluetooth.name", "");
+                       hal_device_property_set_string (d,
"net.bluetooth.uuid", "");

What is best here, empty values or missing properties?

I guess the rest marcel already explained
Comment 12 Luiz Augusto von Dentz 2007-04-17 10:36:09 UTC
Created attachment 9637 [details] [review]
Patch 1 of 2

Make helper to use GetInfo instead of properties one by one.
Comment 13 Luiz Augusto von Dentz 2007-04-17 10:38:33 UTC
Created attachment 9638 [details] [review]
Patch 2 of 2

Leave to helper create or not properties only available via bluez network service.
Comment 14 David Zeuthen (not reading bugmail) 2007-04-23 12:48:59 UTC
Committed, thanks. I also committed this on top

http://gitweb.freedesktop.org/?p=hal.git;a=commitdiff;h=bf29b2f07dc05cf8b95463ad76b0b2ebe4ed596a

since these properties are specific to Bluez.
Comment 15 Marcel Holtmann 2007-04-23 13:01:25 UTC
These information and attributes are not specific to BlueZ. They are general Bluetooth attributes. So adding a "bluez_" prefix makes no sense. The only BlueZ specific is the way how these information are retrieved. So please revert this patch.
Comment 16 Luiz Augusto von Dentz 2007-04-23 13:41:04 UTC
Those properties are present in the PAN profile, so I guess we could not consider them as a bluez specific. But the service record is not exposed in sysfs that why there is a need of a helper to try to recovery them from the bluez service. In case of the interface being configure without bluez we dont create any property present in the service record.
Comment 17 David Zeuthen (not reading bugmail) 2007-04-24 12:20:32 UTC
OK, fair enough; I misunderstood; reverted

http://gitweb.freedesktop.org/?p=hal.git;a=commitdiff;h=8e6daa796eec0bf639567ed94a784838c1fc5fdb

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.