GNOME Bugzilla – Bug 794294
Misc fixes
Last modified: 2018-03-27 08:41:35 UTC
.
Created attachment 369624 [details] [review] libnm-core/8021x: Add g_autoptr when GLib version supports it This means that systems with newer versions of GLib will know how to use g_autoptr with NMSetting8021x. Older systems won't see the change.
Created attachment 369625 [details] [review] build: Remove GLIB_GENMARSHAL check It's unused.
(In reply to Bastien Nocera from comment #1) > Created attachment 369624 [details] [review] [review] > libnm-core/8021x: Add g_autoptr when GLib version supports it > > This means that systems with newer versions of GLib will know how to use > g_autoptr with NMSetting8021x. Older systems won't see the change. Is this useful to only add it to one type? Why specifically NMSetting8021x? (In reply to Bastien Nocera from comment #2) > Created attachment 369625 [details] [review] [review] > build: Remove GLIB_GENMARSHAL check > > It's unused. $(GLIB_GENMARSHAL) seems to be used by Makefile.glib. Why do you think it's unused?
> Is this useful to only add it to one type? Why specifically NMSetting8021x? We could add a new header file that contains all these g_autoptr defines. Possibly stand-alone, so you can copy the header to your source-tree (in case you don't want to bump your libnm requirement yet, just to get the new defines).
(In reply to Thomas Haller from comment #3) > (In reply to Bastien Nocera from comment #1) > > Created attachment 369624 [details] [review] [review] [review] > > libnm-core/8021x: Add g_autoptr when GLib version supports it > > > > This means that systems with newer versions of GLib will know how to use > > g_autoptr with NMSetting8021x. Older systems won't see the change. > > Is this useful to only add it to one type? Why specifically NMSetting8021x? It's the only one that's used in connection-editor/wireless-security code in nm-connection-editor that's later copied in gnome-control-center. Right now, this code uses libgsystem's auto-dereference code (gs_unref_object, gs_free). In g-c-c, we don't need libgsystem, and instead use g_autoptr and g_autofree. See this patch to nm-connection-editor copy/paste: https://bug785099.bugzilla-attachments.gnome.org/attachment.cgi?id=369520 Ideally, this would be added to every single object NM exports. But I won't have time to work on that I'm afraid. > (In reply to Bastien Nocera from comment #2) > > Created attachment 369625 [details] [review] [review] [review] > > build: Remove GLIB_GENMARSHAL check > > > > It's unused. > > $(GLIB_GENMARSHAL) seems to be used by Makefile.glib. Why do you think it's > unused? Makefile.glib looks like copy/paste, so I didn't change it. And it's only used when there's a .list file, to generate marshal files, which there aren't any. (In reply to Thomas Haller from comment #4) > > Is this useful to only add it to one type? Why specifically NMSetting8021x? > > We could add a new header file that contains all these g_autoptr defines. > Possibly stand-alone, so you can copy the header to your source-tree (in > case you don't want to bump your libnm requirement yet, just to get the new > defines). That's a good plan, as long as we can guard those definitions with a specific NM version, to avoid later compilation failures (as some GNOME modules experienced when we added g_auto support in libgudev but some modules added their own definitions instead of upstreaming it).
Created attachment 369857 [details] [review] libnm: add nm-autoptr.h header "nm-autoptr.h" is done in a way that allows you to copy the header in your source tree to support older versions of libnm, that didn't contain the header yet. For example, we might want to use it in network-manager-applet, but we don't want to bump the libnm dependency to 1.11.2+ only to get this functionality. Note that G_DEFINE_AUTOPTR_CLEANUP_FUNC() was added in glib 2.43.4, and requires compiler support for the cleanup attribute. The compiler support is taken as given, because we rely on it already. However, NetworkManager and network-manager-applet still don't depend on a glib version recent enough to provide these macros. To actually use them (*inside*) NetworkManager/network-manager-applet, we either would have to bump the glib minimal dependency, or reimplement g_autoptr in /shared/nm-utils/nm-glib.h compat header.
Created attachment 369858 [details] [review] shared/nm-glib: add compat implementation for g_autofree Eventually, we should replace our uses of libgsystem's gsystem-local-alloc.h by glib's g_auto*. As a first tiny step, add a compat implementation for g_autofree, so that we could at least go ahead and use it instead of gs_free.
(In reply to Thomas Haller from comment #6) > libnm: add nm-autoptr.h header > shared/nm-glib: add compat implementation for g_autofree Both patches look good to me.
(In reply to Bastien Nocera from comment #2) > Created attachment 369625 [details] [review] [review] > build: Remove GLIB_GENMARSHAL check > > It's unused. LGTM
Comment on attachment 369625 [details] [review] build: Remove GLIB_GENMARSHAL check merged to master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=497a18aee9945348d0692c6c434e4d030d0b1a49
Comment on attachment 369857 [details] [review] libnm: add nm-autoptr.h header merged: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=ff8e56336574a168d427a993d666d9e038aacbaf
Comment on attachment 369858 [details] [review] shared/nm-glib: add compat implementation for g_autofree merged https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=b2edcdc939bbc0197dbbed8da044d1ecd76aae0b