GNOME Bugzilla – Bug 736289
[review] dcbw/internal-device-factories
Last modified: 2014-09-19 18:57:15 UTC
Convert all internal (eg, non-plugin) device subclasses to use device factories. We'll need this later when we add device factories for VPNs, and when we create device objects for every connection before the device is realized.
> build: ensure device source file constructors can be linked and called I had a comment here about this possibly causing problems with nm-settings.c referring to NMDeviceEthernet directly, but I see you addressed that in the next commit. Seems like those to commits should be swapped to avoid problems? OTOH, does changing NetworkManager_LDADD to -Wl,-whole-archive libNetworkManager.la -Wl,-no-whole-archive solve the problem as well? Might be simpler... > settings: create default wired connection from NMDeviceEthernet generally like. It seems like (later) there could be some synergy here with Thomas's patches for generated-vs-assumed connections, and maybe making default-wired and default-bluetooth-dun use more of the same infrastructure. Maybe stop referring to them as default *wired* connections in nm-settings.c? Also rename nm_settings_utils_get_default_wired_name(). (And maybe just absorb it into nm-device-ethernet.c?) > core: make device factory registration function more generic >+ g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_INTERNAL, >+ "multiple plugins for same type (using '%s' instead of '%s')", indent (Also, not sure the commit message really describes the commit?) > core: add support for internal device factories >+ * INTERNAL DEVICE FACTORY FUNCTIONS - devices provided by plugins should >+ * not use these functions. Maybe name them accordingly? > /*******************************************************************/ > >+/*******************************************************************/ >+ in nm-manager.c. bad merge or a leftover or something? >+ nm_log_info (LOGD_HW, "Loaded device plugin: %s", g_type_name (ftype)); Not nm_log_dbg()? I don't think we really need to log that every time... I just skimmed the remaining commits
(In reply to comment #1) > > build: ensure device source file constructors can be linked and called > > I had a comment here about this possibly causing problems with nm-settings.c > referring to NMDeviceEthernet directly, but I see you addressed that in the > next commit. Seems like those to commits should be swapped to avoid problems? Swapped. > OTOH, does changing NetworkManager_LDADD to > > -Wl,-whole-archive libNetworkManager.la -Wl,-no-whole-archive > > solve the problem as well? Might be simpler... Unfortunately, it seems libtool reorders flags and actually removes the library between -whole-archive/-no-whole-archive. I thought that might be due to libNetworkManager.la being in NetworkManager_LDADD (eg, libtool enforces it's own options for stuff it controls) but removing it from LDADD means it's no longer a dependency of NetworkManager and thus it's not built before NM is, and linkage fails with "libNetworkManager.a not found". Other references: https://github.com/aldebaran/urbi/blob/master/sdk-remote/sdk/umake-link.in https://www.mail-archive.com/libtool@gnu.org/msg08875.html And possibly most damning: http://osdir.com/ml/libtool-gnu/2011-06/msg00003.html "Yes, when convenience libraries are used to create a shared library, all the objects are included in the output, when the output is an application they are used like a normal archive library. Either use them to create a shared library or, if creating an application, don't use them, use the objects instead." So I think the original patch is the best option... > > settings: create default wired connection from NMDeviceEthernet > > generally like. It seems like (later) there could be some synergy here with > Thomas's patches for generated-vs-assumed connections, and maybe making > default-wired and default-bluetooth-dun use more of the same infrastructure. Yeah, that's the general idea. I'm hoping we can remove all the default wired junk from nm-settings once that lands. > Maybe stop referring to them as default *wired* connections in nm-settings.c? I think all that code will go away though actually, so I just left it as-is until we kill it. > Also rename nm_settings_utils_get_default_wired_name(). (And maybe just absorb > it into nm-device-ethernet.c?) Renamed; if we absorb it into nm-device-ethernet.c we can't unit test it easily. > > core: make device factory registration function more generic > > >+ g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_INTERNAL, > >+ "multiple plugins for same type (using '%s' instead of '%s')", > > indent Fixed. > (Also, not sure the commit message really describes the commit?) Fixed. > > core: add support for internal device factories > > >+ * INTERNAL DEVICE FACTORY FUNCTIONS - devices provided by plugins should > >+ * not use these functions. > > Maybe name them accordingly? Done. > > /*******************************************************************/ > > > >+/*******************************************************************/ > >+ > > in nm-manager.c. bad merge or a leftover or something? Yeah, fixed. > >+ nm_log_info (LOGD_HW, "Loaded device plugin: %s", g_type_name (ftype)); > > Not nm_log_dbg()? I don't think we really need to log that every time... Fixed.
looks good
Merged to master with a few commit message fixes.