Please review: http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/bgo709412-bluez45


dcbw already looked at a first version (I incorporated those remarks).

Since then, I changed that dbus calls are now done asynchronously.

That would be great!(In reply to comment #1)
> Please review:
> http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/bgo709412-bluez45
> 
> 
> dcbw already looked at a first version (I incorporated those remarks).
> 
> Since then, I changed that dbus calls are now done asynchronously.

Great work! Not enough time for detailed review, though.

> blue: support BlueZ 4 and 5 together in nm-bluez-device.c

(missing "z" in "bluez" at the start)

There's no inherent reason why BlueZ 4 needs to use dbus-glib rather than GDBus. At this point it seems like it would be cleaner to port the BlueZ 4 codepaths (at least in this file) to use GDBus as well. (You would probably then only need a single copy of the properties code, for instance.)

+         (   (priv->bluez_version == 4) ||
+             (priv->bluez_version == 5 && priv->adapter && priv->adapter_powered) ) &&

If you're putting the "||" at the end of the first line rather than the beginning of the second, then there's no reason to have the extra spaces after the first "(".



> bluez: enable both BlueZ4 and 5 and select it dynamically at runtime

Since the bluez code isn't conditional any more, you can just move it back into the main nm_sources declaration rather than having a separate "nm_sources += ..." section.

>+bluez4_manager_bdaddr_added_cb (NMBluez4Manager *bluez_mgr,
>+bluez5_manager_bdaddr_added_cb (NMBluez5Manager *bluez_mgr,

You don't really need separate callbacks here... just change the first arg to be GObject or gpointer, and use a single callback for both 4 and 5. (Same with removed_cb)

>+	nm_log_info (LOGD_BT, "detecting the BlueZ version failed");
>+	nm_log_dbg (LOGD_BT, "bluez cannot introspect version: %s", error->message);
>+	nm_log_dbg (LOGD_BT, "bluez error creating dbus proxy: %s", error->message);

Seems like these should be nm_log_warn()

>+	/* might not be the best approach to detect the version, but it's good enough in practice. */
>+	if (strstr (xml_data, "org.freedesktop.DBus.ObjectManager"))

Ugh. Isn't there some BlueZ 5-specific interface you can look for?

>+	GCancellable *introspect_proxy_cancellable;
>+	GCancellable *call_cancellable;

I don't think you need both of these. A single cancellable should work fine. Also, in both check_bluez_and_try_setup_do_introspect() and check_bluez_and_try_setup_on_new_proxy(), you should check g_cancellable_is_cancelled(), and bail out early in that case. (And in check_bluez_and_try_setup_final_step(), I guess you don't want to nm_log_warn() in this case.)

(In reply to comment #3)

I updated the branch th/bgo709412-bluez45. The previous version is available for comparison as th/bgo709412-bluez45-0 .

I implemented all mentioned points except the two points below. Also, the last commit is something new and fixes an existing issue in bluez4-adapter.


* I did not increase the log level, because IMO this does not qualify as a warning. There are only two places left in nm-bluez-manager.c where something gets logged.


* I did also not (yet?) change the way how to detect the version. First I am not sure how to do it better/properly, and moreover, I think the current version works well enough (in practice).

(In reply to comment #4)
> * I did also not (yet?) change the way how to detect the version. First I am
> not sure how to do it better/properly, and moreover, I think the current
> version works well enough (in practice).

Yeah, I guess to do it properly you'd have to introspect on /org/bluez rather than / for Bluez 5.

Eh, I guess it's ok. Maybe put the bluez4 check first, since it's more specific than the bluez5 check?

> +#define VARIANT_IS_OF_TYPE_STRING(v)  ((v) != NULL && ( g_variant_is_of_type ((v), G_VARIANT_TYPE_STRING) || g_variant_is_of_type ((v), G_VARIANT_TYPE_OBJECT_PATH) || g_variant_is_of_type ((v), G_VARIANT_TYPE_SIGNATURE) ))

Any given property should only be one of those types, so I think you should have separate VARIANT_IS_OF_TYPE_STRING() and VARIANT_IS_OF_TYPE_OBJECT_PATH(). (Or just the former if none of the properties are object-path-values.)

(In reply to comment #5)

> Eh, I guess it's ok. Maybe put the bluez4 check first, since it's more specific
> than the bluez5 check?

I tested it, and it never failed to detect the version. I think, this introspect data should hardly change, so it should work well (enough).


> Any given property should only be one of those types, so I think you should
> have separate VARIANT_IS_OF_TYPE_STRING() and VARIANT_IS_OF_TYPE_OBJECT_PATH().
> (Or just the former if none of the properties are object-path-values.)

I agree.



I pushed a new version th/bgo709412-bluez45, th/bgo709412-bluez45-1 is there for reference from comment #4.

Changes:

- I broke bluez4 in the patch before. This was a major rework.
- new patch "bluez: remove created NAP connection together with NMBluezDevice" to cleanup created NMConnection.
- new patch "core: make callback argument in nm_settings_connection_commit_changes/_delete optional"



Also, this time I really tested both bluez4 and bluez5. It should work fine now, I did not meet any issues.

> bluez: use dbus-glib instead of GDBus in nm-bluez-device.c

The commit shortlog here is inverted: you converted the code from dbus-glib -> GDBus.  But the shortlog says you converted GDBus -> dbus-glib.  So:

"bluez: use GDBus instead of dbus-glib in nm-bluez-device.c"

> bluez: fix calling of bdaddr added/removed signals in nm-bluez4-adapter

I think this can be simpler.  With the current code, NMBluezDevice guarantees that it will always toggle the "usable" signal.  It also always starts as usable=FALSE.  So, this means that in the adapter code, device_usable() is the only function that will ever handle the change to usable=TRUE.  That means we can move the emit[DEVICE_ADDED] code into that function.

Second, we don't need the check_emit_usable() call in device_created().  Device creation is always asynchronous and after calling nm_bluez_device_create(), usable is always FALSE, so emit[DEVICE_ADDED] will never happen here.  So we can simply remove that call because it does nothing.

So, check_emit_usable() is now only called from device_do_remove().  Which means that force_unusable will always be TRUE.  When device_do_remove() is called, the device will be either usable=TRUE|FALSE.  If the device is usable=TRUE, then we must emit[DEVICE_REMOVED] to alert the manager that it is gone.  If the device is usable=FALSE, then we don't  need to emit anything at all.  In both cases (usable=TRUE|FALSE) the adapter still needs to disconnect signal handlers.

Next, device_initialized() should never emit any signals, because if initialization failed, the device cannot have ever been usable.  So calling device_do_remove() here is wrong, because that will emit[DEVICE_REMOVED] for a device that was never exposed to the BluezManager.  All we need to do here is remove it from the adapter from the hash table and disconnect signals.

If all this is correct, I think we can get rid of check_emit_usable() and simplify things.  The only place emit[DEVICE_ADDED] should be called is device_usable(), and the emit[DEVICE_REMOVED] can be put into its own function called from device_initalized(), and device_do_remove().

Proposed fixup on top of your patch:  http://bigw.org/~dan/bluez4-adapter.patch

What do you think?

> bluez: remove created NAP connection together with NMBluezDevice

NMSettings (and by extension the ConnectionProvider interface) return an object that's owned by the ConnectionProvider, and thus the caller must reference it should the caller wish to continue using that object.  Yes, technically the new connection will be alive from creation until the ConnectionProvider's connection-deleted signal is sent, but I think we should make the lifetime clearer by referencing the 'added' connection.  That allows us to clean up the removal code too.

How about:  http://bigw.org/~dan/bluez-connection-remove.patch ?

(In reply to comment #7)

I repushed a new version with your two patches (as separate commits).

> > bluez: fix calling of bdaddr added/removed signals in nm-bluez4-adapter
> 
> I think this can be simpler.  With the current code, NMBluezDevice guarantees
> that it will always toggle the "usable" signal.  It also always starts as
> usable=FALSE.  So, this means that in the adapter code, device_usable() is the
> only function that will ever handle the change to usable=TRUE.  That means we
> can move the emit[DEVICE_ADDED] code into that function.
> 
> Second, we don't need the check_emit_usable() call in device_created().  Device
> creation is always asynchronous and after calling nm_bluez_device_create(),
> usable is always FALSE, so emit[DEVICE_ADDED] will never happen here.  So we
> can simply remove that call because it does nothing.
> 
> So, check_emit_usable() is now only called from device_do_remove().  Which
> means that force_unusable will always be TRUE.  When device_do_remove() is
> called, the device will be either usable=TRUE|FALSE.  If the device is
> usable=TRUE, then we must emit[DEVICE_REMOVED] to alert the manager that it is
> gone.  If the device is usable=FALSE, then we don't  need to emit anything at
> all.  In both cases (usable=TRUE|FALSE) the adapter still needs to disconnect
> signal handlers.
> 
> Next, device_initialized() should never emit any signals, because if
> initialization failed, the device cannot have ever been usable.  So calling
> device_do_remove() here is wrong, because that will emit[DEVICE_REMOVED] for a
> device that was never exposed to the BluezManager.  All we need to do here is
> remove it from the adapter from the hash table and disconnect signals.

It's not actually wrong behaviour, because the removed signal gets only emitted if device_was_usable_lastly (which was not the case).

> 
> If all this is correct, I think we can get rid of check_emit_usable() and
> simplify things.  The only place emit[DEVICE_ADDED] should be called is
> device_usable(), and the emit[DEVICE_REMOVED] can be put into its own function
> called from device_initalized(), and device_do_remove().
> 
> Proposed fixup on top of your patch:  http://bigw.org/~dan/bluez4-adapter.patch
> 
> What do you think?
> 

Your change seems correct to. It makes some of assumptions of code in the other module, e.g.
- a NMBluezDevice is not usable, unless initialized
- a newly created device is never usable and gets initialized asynchronously.
- nm_bluez_device_get_usable returns consistent results, never missing to raise a notify::usable signal (in a strictly toggling way).

Yes, these assumptions hold now, and probably in the future, so your patch is better (because shorter and more concise).

Usually, I try to minimize my assumptions and not to assume, that repeatedly calling a function (or a property getter) from another class/module yields the same result. IOW, I don't trust nm_bluez_device_get_usable, that's why I cached device_was_usable_lastly. But yeah, simplicity wins :)


I would squash your "<PROPOSED>" patch over mine as is.



> > bluez: remove created NAP connection together with NMBluezDevice
> 
> NMSettings (and by extension the ConnectionProvider interface) return an object
> that's owned by the ConnectionProvider, and thus the caller must reference it
> should the caller wish to continue using that object.  Yes, technically the new
> connection will be alive from creation until the ConnectionProvider's
> connection-deleted signal is sent, but I think we should make the lifetime
> clearer by referencing the 'added' connection.  That allows us to clean up the
> removal code too.

This was not a bug, because I paid attention, that "priv->pan_connection" is always ref'ed from inside the "priv->connections" list, so NMBluezDevice already keeps a reference to this connection on its own.
Yes, these are assumptions too, but it's within the same file, and I can reason about that easily. I took parts of your rework in the dispose method though.

After that, I would discard the remaining parts of your "<PROPOSED>" patch.

If you want to put more assertions into nm-bluez-device.c to enforce certain beahvior, that's fine.  I think we should both (a) assert that things we expect to be true are true, and (b) document those assertions and expectations in the code.  Which might include some code comments for nm-bluez-device.c and the usable stuff.

For nm_bluez_device_get_usable(), it's got checks for "if (new_usable != priv->usable) return" though, so I can't see how it would ever emit the same value twice for 'usable'.  Since the GObject propert is only G_PARAM_READABLE, the property notification will never get fired via g_object_set() either.  So I think we can trust it...

But in general, I think careful use of assertions is a great thing.
---

The fixups in the branch now look good, and I tested it lightly with DUN and NAP on bluez4 and bluez5.  I think it's good to merge.  Thanks!

No, I think your patch was actually better (because shorter).

With asserts and code to handle unexpected conditions, one might always argue, that they are not needed -- because they cannot possibly fail (to current knowledge). So, it's often up to a matter of taste.

--

I pushed the following commits to upstream master:

72da550f68c88d7c3a7836673de950dd61e05dab bluez: remove created NAP connection together with NMBluezDevice
d5bc5401509595cc8473efe50bcc90b4ad95f642 bluez: merge rework of BlueZ to detect BlueZ 4 vs. 5 at runtime (bgo #709412)
4ba86e2cc8d5d10a42f02beb71ed796afcec3e01 bluez: fix calling of bdaddr added/removed signals in nm-bluez4-adapter
28a6f11b2c288357559b6475c2d000a5ff88cb13 bluez: use GDBus instead of dbus-glib in nm-bluez-device.c
bf5a6ad4431c50b19ca954071900c4235c0f44a3 bluez: enable both BlueZ4 and 5 and select it dynamically at runtime
3344ce9ff6213585d641e568a96bf063c2cce24f bluez: copy bluez-manager file for version 4 and 5
ef8501096fd7a458f14c0fb36fbdf3c592b6c957 bluez: rename BlueZ 4 adapter to make the BlueZ version explicit
e8e8031676e724a6ccc0ecbdc96b6d59bd5c92f2 bluez: support BlueZ 4 and 5 together in nm-bluez-device.c
b3ec1587d353666b93bc51f036f19dcf9209ffcd bluez: rename variables in nm-bluez-common.h for BlueZ 4 vs. 5
e46722b72b7d0ac03ab0be24f20c6a86d7300c13 core: make callback argument in nm_settings_connection_commit_changes/_delete optional

(In reply to comment #10)
> With asserts and code to handle unexpected conditions, one might always argue,
> that they are not needed -- because they cannot possibly fail (to current
> knowledge). So, it's often up to a matter of taste.

You could also argue that *if* they can fail, they constitute a bug (assertions were always meant to be optionally left out by preprocessor).