GNOME Bugzilla – Bug 724324
[review] dcbw/dev-plugins: convert BT, WWAN, and ADSL to device plugins
Last modified: 2014-03-03 22:20:06 UTC
This branch enhances the device plugin interface and converts the WiMAX, Bluetooth, WWAN, and ADSL plugins to use it. Total savings in the core NM binary are about 11% in stripped size. Overall there is an increase in code, but that is all in the plugins themselves over their previous built-in object size. The general idea here is that if you don't need these plugins at runtime, don't install them (eg, initrd).
> core: add nm_connection_provider_get() > This function references the provider, so callers must ensure they > unref it when they are disposed. lies! > mobile: convert to device removed signals NMDeviceBT doesn't seem to emit "removed" when its modem goes away >- g_signal_emit (self, signals[MODEM_REMOVED], 0, modem); >+ g_signal_emit_by_name (modem, NM_MODEM_REMOVED); It would be stylistically nicer to add nm_modem_emit_removed(), rather than having NMModemManager call g_signal_emit_by_name() on the NMModem directly. > devices: rework device plugin interface to be more flexible >+ * existing Bluetooth device may wish to claim. If no device claims the >+ * component, and the plugin is allowed to create a new #NMDevice instance >+ * for that component and emit the "device-added" signal. (component_added docs). The "and" in the second line doesn't belong there. >+nm_device_factory_emit_component_added (NMDeviceFactory *factory, GObject *component) >+ g_value_set_boolean (&retval, FALSE); >+ >+ /* Use g_signal_emitv() rather than g_signal_emit() to avoid the return >+ * value being changed if no handlers are connected */ "FALSE" is the default for a G_TYPE_BOOLEAN value, so that's what will be returned if no handlers are connected. So you can just use g_signal_emit(). >+ component_added_accumulator, NULL, NULL, you don't need component_added_accumulator, you can just use g_signal_accumulator_true_handled. >+factory_component_added_cb (NMDeviceFactory *factory, shouldn't this iterate across the devices? (ie, the code you add later on in the WWAN commit) >+ load_device_factories (singleton); why did you move this from _init() to _new()? > bluez: make Bluetooth a plugin This commit message doesn't have size-savings information like the next two do > enum { > PROP_0, >- PROP_PROVIDER, >+ PROP_DEVICE_TYPE, > > LAST_PROP > }; (nm-bluez-manager.c) fix up existing indentation > mobile: make WWAN support a plugin >+libnm_device_plugin_bt_la_LIBADD = \ >+ $(top_builddir)/src/devices/wwan/libnm-device-plugin-wwan.la \ Making a loadable module depend on another loadable module seems un-kosher... (among other things, it means there would be two nm_device_factory_create()s in libnm-device-plugin-bt...) I think you'd need to make modem-manager be a separate library, and have both plugins link to it. (Or push NM's modem-manager code into libmm-glib?) >+#if 0 >+ /* FIXME: this should be replaced by parent/child relationships between so does this break stuff? > atm: make ADSL support a plugin > enum { >- DEVICE_ADDED, >- DEVICE_REMOVED, >+ PROP_0, >+ PROP_DEVICE_TYPE, indentation again
(In reply to comment #1) > > core: add nm_connection_provider_get() > > > This function references the provider, so callers must ensure they > > unref it when they are disposed. > > lies! Fixed. > > mobile: convert to device removed signals > > NMDeviceBT doesn't seem to emit "removed" when its modem goes away > > >- g_signal_emit (self, signals[MODEM_REMOVED], 0, modem); > >+ g_signal_emit_by_name (modem, NM_MODEM_REMOVED); > > It would be stylistically nicer to add nm_modem_emit_removed(), rather than > having NMModemManager call g_signal_emit_by_name() on the NMModem directly. Fixed. > > devices: rework device plugin interface to be more flexible > > >+ * existing Bluetooth device may wish to claim. If no device claims the > >+ * component, and the plugin is allowed to create a new #NMDevice instance > >+ * for that component and emit the "device-added" signal. > > (component_added docs). The "and" in the second line doesn't belong there. Fixed. > >+nm_device_factory_emit_component_added (NMDeviceFactory *factory, GObject *component) > > >+ g_value_set_boolean (&retval, FALSE); > >+ > >+ /* Use g_signal_emitv() rather than g_signal_emit() to avoid the return > >+ * value being changed if no handlers are connected */ > > "FALSE" is the default for a G_TYPE_BOOLEAN value, so that's what will be > returned if no handlers are connected. So you can just use g_signal_emit(). > > >+ component_added_accumulator, NULL, NULL, > > you don't need component_added_accumulator, you can just use > g_signal_accumulator_true_handled. Fixed. > >+factory_component_added_cb (NMDeviceFactory *factory, > > shouldn't this iterate across the devices? (ie, the code you add later on in > the WWAN commit) Fixed; it was originally to keep the codepaths clearer. The BT DUN code is the only consumer, and WWAN will be the only provider, but WWAN is not yet a plugin by this commit and thus doesn't emit component-added. Instead that's handled explicitly in nm-manager.c::modem_added(), and I was attempting to keep the DUN code from seeing the modem twice. However, that's not actually possible, so I'll add the code to factory_component_added_cb() too. > >+ load_device_factories (singleton); > > why did you move this from _init() to _new()? nm_connection_provider_get() returns manager's priv->settings; and plugins like BT need the provider. But the manager's priv->settings is only set in _new(), so we need to ensure that the plugins load after that. > > bluez: make Bluetooth a plugin > > This commit message doesn't have size-savings information like the next two do Fixed. > > enum { > > PROP_0, > >- PROP_PROVIDER, > >+ PROP_DEVICE_TYPE, > > > > LAST_PROP > > }; > > (nm-bluez-manager.c) fix up existing indentation Fixed. > > mobile: make WWAN support a plugin > > >+libnm_device_plugin_bt_la_LIBADD = \ > >+ $(top_builddir)/src/devices/wwan/libnm-device-plugin-wwan.la \ > > Making a loadable module depend on another loadable module seems un-kosher... > (among other things, it means there would be two nm_device_factory_create()s in > libnm-device-plugin-bt...) I think you'd need to make modem-manager be a > separate library, and have both plugins link to it. (Or push NM's modem-manager > code into libmm-glib?) Valid point. I'll think about this, though it shouldn't change the general architecture of NMDeviceFactory, so I've done the rest of these cleanups first and pushed them. > >+#if 0 > >+ /* FIXME: this should be replaced by parent/child relationships between > > so does this break stuff? Fixed a different way; added nm_device_owns_iface() to push the decision out of the manager. It was #if 0-ed out because the manager shouldn't care about any nm_modem_*** functions, but the old code relied on them, so the stuff in the #if 0 wouldn't have compiled. > > atm: make ADSL support a plugin > > > enum { > >- DEVICE_ADDED, > >- DEVICE_REMOVED, > >+ PROP_0, > >+ PROP_DEVICE_TYPE, > > indentation again Fixed.
Just curious, are you going to support external plugins as well? If yes, then I would like to do a thorough cleanup of nm-device before it's published.
(In reply to comment #3) > Just curious, are you going to support external plugins as well? If yes, then I > would like to do a thorough cleanup of nm-device before it's published. Not at this point, no. But that could be a goal for the future, and you would certainly be correct that the internal APIs would need some cleanups.
How about now? libnm-wwan.so = NMModemManager, NMModem, NMModemOld, & NMModemBroadband (no device plugin/factory code at all) libnm-device-plugin-wwan.so = NMWwanFactory, NMDeviceModem (the WWAN device factory code) So now, the Bluetooth code links to libnm-wwan.so but *not* to libnm-device-plugin-wwan.so. The actual WWAN plugin (libnm-device-plugin-wwan.so) also links to libnm-wwan.so.
(In reply to comment #2) > > > mobile: convert to device removed signals > > > > NMDeviceBT doesn't seem to emit "removed" when its modem goes away That's still true, although I see now that it's correct. So I guess the problem is that the commit message only describes the changes to NMDeviceModem, even though there are equally big changes to NMDeviceBt. > This removes direct dependencies on the > ModemManager from the Manager. except it doesn't, because NMManager is still handling modem-added. > > > mobile: make WWAN support a plugin >- nm_log_warn (LOGD_MB, "(%s): unusable modem detected", >+ g_set_error (error, NM_MODEM_ERROR, NM_MODEM_ERROR_INITIALIZATION_FAILED, >+ "(%s): unusable modem detected", > mm_modem_get_primary_port (modem_iface)); more indentation funkiness in nm-modem-broadband.c code-wise everything looks great
How about now? Fixed up the commit message and the indentation. Also, not sure if I pushed it last time, but there's another patch (core: consolidate auto-activation recheck signals) that removes more device-specific stuff from the policy.
everything looks good
Merged to git master.