Yep, it does, and a lot.  It's going to be a long road.

*** Bug 653256 has been marked as a duplicate of this bug. ***

For 1.0 we at least want to completely hide the use of dbus-glib, even if we don't actually port to GDBus yet.

The two big issues:

  - Explicit use of DBusGConnection in some APIs

      None of these uses are actually necessary; they just allow you
      to connect to non-default buses. We can deprecate most of them,
      and for 1.0, change them to gpointers and make them stop working.

      (If we need to keep the non-default bus functionality, eg for
      testing, we can implement it some other way.)

  - Use of dbus-glib's extended type system for the types of
    GObject properties (particularly in NMSetting).
    (eg, dbus_g_type_get_collection ("GSList", G_TYPE_STRING))

      There is no good way to keep the exact GTypes without depending
      on dbus-glib. (There are bad ways, but they're bad.)

      However, the precise GTypes are only relevant to code that deals
      with the GObject property values directly (rather than using the
      get/set functions) and only to code that hardcodes the GTypes of
      the properties (rather than introspecting them at runtime). If we
      kept the structure of the data the same, but changed the GTypes
      (eg, using G_TYPE_SLIST for the list-of-strings above), then all
      existing javascript, python, and vala code should still work, and
      99% of C code as well. Probably...

      OTOH, maybe it would be better to switch to "libnm-util-2",
      and build the old and new libraries from the same sources, with
      ifdefs to control whether to use dbus-glib types or not. (And
      that would let us do some of the other stuff in bug 680675 too...)

*** Bug 729826 has been marked as a duplicate of this bug. ***

(In reply to comment #3)
>       OTOH, maybe it would be better to switch to "libnm-util-2",
>       and build the old and new libraries from the same sources, with
>       ifdefs to control whether to use dbus-glib types or not. (And
>       that would let us do some of the other stuff in bug 680675 too...)

I started working on this on Friday. Building both old and new libraries from the same sources turns out to not really be a good plan, because almost everything in the .c files would end up needing #ifdefs.

But, once we release the new library, the old libraries would be deprecated, and wouldn't necessarily need to be updated for new APIs, etc. So I think that we should just fork the existing libraries into a new one, and then port that one over (making whatever C API changes we want at the same time, subject to the constraint that we don't break D-Bus API compat). We'd have to be careful about merging changes between old and new for a little while, but eventually we'd just stop caring about the old libraries.

OK, pushed danw/gdbus-non-libnm, which starts things off by porting the non-libnm-using parts of NM from dbus-glib to gdbus. (That's nm-dhcp-helper, nm-pppd-plugin, and the dbus-glib-based parts of examples/C/glib/.)

I haven't been able to test the nm-pppd-plugin changes. (My pay-as-you-go SIM no longer connects and lets me get to the "you need to pay more" web page... I must have gone too long without actually paying anything :)

pushed a few fixup! commits. The rest looks good to me.

(In reply to comment #7)
> pushed a few fixup! commits. The rest looks good to me.

>-	                   g_variant_new ("(u)", ppp_status),
>+	                   g_variant_new ("(u)", (guint) ppp_status),

That shouldn't be necessary; even if NMPPPStatus was a [u]char, it would get promoted to [u]int by the rules of varargs. (And if ppp_status is negative, or greater than UINT_MAX, then the code is wrong with or without the cast.)

(In reply to comment #8)
> (In reply to comment #7)
> > pushed a few fixup! commits. The rest looks good to me.
> 
> >-	                   g_variant_new ("(u)", ppp_status),
> >+	                   g_variant_new ("(u)", (guint) ppp_status),
> 
> That shouldn't be necessary; even if NMPPPStatus was a [u]char, it would get
> promoted to [u]int by the rules of varargs. (And if ppp_status is negative, or
> greater than UINT_MAX, then the code is wrong with or without the cast.)

Right, most likely not necessary, but being explicit in this case seems right to me... anyway.

repushed with your fixups other than the ppp_status one squashed in

I also dropped the "require dbus-glib 0.100" patch; it wasn't really that much extra work to support pre-0.100 too.

dbus-glib 0.100 was released on 25-Jun-2012; so we can reasonably expect everyone to have it now.  But we're only using 0.100 as an indicator of whether to use the private socket or not.  Two interesting issues:

1) when NM is fully converted over to GDBus (well, except for libnm-util and libnm-glib), it would be really weird to have checks for dbus-glib > 100 in GDBus code, or have GDBus code caring about dbus-glib 100.

2) so what uses dbus-glib 100?

2a) libnm for private sockets; we don't support new libnm talking to old NetworkManager, but we do support old libnm talking to new NetworkManager.  This means that libnm can always assume private sockets exist, and doesn't need to care about dbus-glib >= 100.

2b) DHCP helper for private DHCP socket; we don't really support mismatched core components, so we don't really care about this here.  The only case that matters is if you upgrade from NM <= 0.9.8 to NM 0.9.10+ and don't restart NM, because the new DHCP client will be on-disk and try to use the private socket and talk to the old NetworkManager, and will fail.  But we don't even support that now...

So my suggestion with all of this would be:

1) require dbus-glib >= 0.100
2) assume that private sockets always exist in libnm-glib and libnm
3) for DHCP, assume that private sockets exist.  But, if you want to support the upgrade case, bonus points if you can detect that the private socket does *not* exist, and fall back to a shared bus name

Beyond all that, the patches look good.  I tested out PPP+IPv4 with a USB-tethered phone and that works correctly.

(In reply to comment #11)
> 2b) DHCP helper for private DHCP socket; we don't really support mismatched
> core components, so we don't really care about this here.

This wasn't about mismatched helper/daemon versions, it's about the days/weeks/months during which the tree contains a gdbus-based nm-dhcp-helper and a dbus-glib-based /sbin/NetworkManager; nm-dhcp-helper needs to know whether NetworkManager is expecting it to use a private socket or not, so it checks the dbus-glib version. (As mentioned in comment 10, the current version of the branch does not change the dbus-glib requirement. I just added the extra 20 lines of code to nm-dhcp-helper to deal with both cases.)

But that check can go away once the daemon is ported.

(pushed. next branch coming soon.)

ok, pushed dispatcher-gdbus (which probably should have been called "callouts-gdbus" since it ports nm-avahi-autoipd.action as well) and libnm-gdbus (which is built on top of dispatcher-gdbus).

libnm-gdbus can also be thought of as being in two pieces; everything except the last commit, and the last commit (which is the one that actually flips the switch). I tried to split as much out of it into the earlier commits as I could.

* danw/dispatcher-gdbus

> libnm-core: add GVariant-based versions of IP structure-manipulating utilities

nm_utils_ip4_dns_to_variant()/nm_utils_ip6_dns_to_variant() - error checking on the inet_pton()?  At least if somebody passes "q235235235" as an IP address, we shouldn't add '0' to the variant array.

nm_utils_ip4_addresses_from_variant() - maybe "if (length < 3)" instead of !=?  At least in libnm-util, the code intentionally does "< 3" as a feeble attempt at forward compatibility if we ever want to add more uints to each IP address...

nm_utils_ip4_routes_from_variant() - same thing here; libnm-util uses "< 4" for the validity check for lame forward compat.  I guess we could even update the settings spec docs to indicate that clients should just ignore any additional items in the route/address array too.


Everything else works for me.

* danw/libnm-gdbus

> libnm: let NMObject create all D-Bus proxies

Use DBUS_INTERFACE_PROPERTIES in init_dbus()

> libnm: move most of the subclass-type-deciding code into NMObject

_nm_object_register_type_func() - can we do some g_assert() or return-if-fail here since later code unconditionally accesses members of the struct that's being created?

Should we just copy DBUS_INTERFACE_PROPERTIES and friends from dbus headers into nm-dbus-helpers.h and use them in _nm_object_create() and _nm_object_create_async() and then later on in other patches?

> libnm: split nm-dbus-helpers utils into sync/async versions

Random thought with GAsyncResult; should we use "__builtin_frame_address(0)" instead of the current function pointer when creating them?  That way it's always constant and we don't need to copy & paste the function name, plus it might save us from copy & paste errors later on if we move code and forget to change the function pointer.  Could even "#define THIS_FUNCTION __builtin_frame_address(0)" to make it more explicit.  Just a thought.

> libnm-core: change connection hash tables to variants in API

Are we dropping the "if it's default it doesn't get serialized" thing now, eg in _nm_setting_to_dbus())?

> libnm-core: update tests to be fully variant-based

The nm-test-utils.h bits should probably go into "nmtst: add NMTST_VARIANT_EDITOR()".

> libnm: port to GDBus

I get this after 'make distclean' and re-autogen even; not sure what's going on.

make[3]: Entering directory `/home/dcbw/Development/fdo/NetworkManager/docs/libnm'
  DOC   Preparing build
  DOC   Scanning header files
  DOC   Introspecting gobjects
../../libnm/.libs/libnm-vpn.so: undefined reference to `_nm_dbus_register_error_domain'
../../libnm/.libs/libnm-vpn.so: undefined reference to `_nm_dbus_bind_methods'
../../libnm/.libs/libnm-vpn.so: undefined reference to `_nm_dbus_bind_properties'
collect2: error: ld returned 1 exit status
Linking of scanner failed: 
make[3]: *** [scan-build.stamp] Error 1

nm-device.c: stray unused DBUS_G_TYPE_UINT_STRUCT

_nm_dbus_register_proxy_type() - I don't think it's worth the memory to strdup the proxy interface name, given that we should be using #defines for all of them anyway...

nm-device-ethernet.c: this hunk should probably be part of "libnm: let NMObject create all D-Bus proxies"

nm-object.c: gratuitous whitespace removal in init_sync() after "Caller did not specify dbus path" and before "if (!priv->connetion)"?  Doesn't look intentional...

One random point; should we be disconnecting signals on all the priv->proxy cached proxies in libnm/nm-device-*?  Or don't we care because they are private and their lifetime is controlled by the NMObject superclass...

nm_vpn_plugin_set_connection() - maybe just do g_clear_object (&priv->connection); instead of following the pre-commit idiom.  Same kind of thing in init_sync() perhaps too?

That's it!  We're halfway there, yay :)

OK, rebased and added fixups...

The rebase around the test-secret-agent changes required a bunch of rewriting in the gdbus port of nm-secret-agent.c and nm-dbus-helpers.c. All the other major changes since you last looked at it are in new commits.

(In reply to comment #16)
> Should we just copy DBUS_INTERFACE_PROPERTIES and friends from dbus headers
> into nm-dbus-helpers.h and use them in _nm_object_create() and
> _nm_object_create_async() and then later on in other patches?

Added a new commit

    libnm: don't hardcode things like "org.freedesktop.DBus.Properties"

and a fixup to the gdbus-port commit copying the defines into nm-dbus-helpers.h

> > libnm: split nm-dbus-helpers utils into sync/async versions
> 
> Random thought with GAsyncResult; should we use "__builtin_frame_address(0)"
> instead of the current function pointer when creating them?

Well, I'm not even bothering to check g_simple_async_result_is_valid() in the _finish functions, so we're not using the value we pass anyway...

No change made here.

> > libnm-core: change connection hash tables to variants in API
> 
> Are we dropping the "if it's default it doesn't get serialized" thing now, eg
> in _nm_setting_to_dbus())?

No, it just moves around a little; get_property_for_dbus() takes an ignore_default flag, which if TRUE, makes it return NULL if the property has its default value, in which case _nm_setting_to_dbus() will skip it.

> > libnm: port to GDBus
> 
> I get this after 'make distclean' and re-autogen even; not sure what's going
> on.

> ../../libnm/.libs/libnm-vpn.so: undefined reference to
> `_nm_dbus_register_error_domain'
> ../../libnm/.libs/libnm-vpn.so: undefined reference to `_nm_dbus_bind_methods'
> ../../libnm/.libs/libnm-vpn.so: undefined reference to
> `_nm_dbus_bind_properties'

Oops, I guess I didn't try building with docs yet...

What's happening is that libnm-vpn is trying to use private libnm APIs, and failing, because they're private [duh].

I guess it's time for...

    libnm: merge libnm-vpn into libnm

> nm-object.c: gratuitous whitespace removal in init_sync() after "Caller did not
> specify dbus path" and before "if (!priv->connection)"?  Doesn't look
> intentional...

I don't see that there... I must have fixed it while rebasing.

> One random point; should we be disconnecting signals on all the priv->proxy
> cached proxies in libnm/nm-device-*?  Or don't we care because they are private
> and their lifetime is controlled by the NMObject superclass...

The latter

(All points not mentioned above should be fixed in the fixups.)

The fixups look good to me.  Maybe get jklimes to look at it and do some testing with nmcli before merging?

Pushed dispatcher-gdbus and libnm-gdbus to master.

Now just the daemon is left to port; I pushed my work-in-progress on that to danw/wip/gdbus since I'm not planning to finish it right away, but note that some of the stuff there (particularly involving how D-Bus interfaces are implemented) are based on an earlier approach than what I eventually ended up using in libnm (for NMSecretAgent and NMVpnPlugin), so that will all need to be revisted.

At least there's a number of discrete pieces in the core daemon which would be easier to convert, which are all private API:

1) VPN plugin
2) DHCP helper
3) avahi-autoipd helper
4) PPP helper
5) ifcfg-rh plugin (not private API but very simple)
6) Bluez4 code
7) wpa_supplicant
8) firewalld
9) dispatcher
10) upower

Then of course we have to core daemon, where we have to be very careful to ensure we don't break the D-Bus API.

(In reply to comment #20)
> 2) DHCP helper
> 3) avahi-autoipd helper
> 4) PPP helper
> 9) dispatcher

Those are all already ported. (Everything left to be done is either part of /sbin/NetworkManager, or a plugin loaded into /sbin/NetworkManager.)

> Then of course we have to core daemon, where we have to be very careful to
> ensure we don't break the D-Bus API.

I don't think accidentally breaking the D-Bus API is all that likely; we'll still be using code autogenerated from the same introspection data.

OK, pushed danw/pre-gdbus-bgo622927. This is the last set of changes before the big dbus-glib to gdbus flag day. It's somewhat tested and ready for review (but someone should try running our QE tests against it too).

I have also pushed danw/gdbus-bgo622927, which contains the actual final gdbus porting. It is less tested and perhaps not *quite* ready for review, although maybe running that against the QE tests would be useful too. (Assuming that pre-gdbus mostly passes.) It may be useful for understanding better why some changes were made in pre-gdbus though.

Note that danw/gdbus-bgo622927 is "weird" in that none of the commits in it other than the final one actually compile, because all of the changes are intertwined. But it just seemed gross to throw them all into a single commit, so they're split up functionally for ease of review. Note that .gitignore, src/NetworkManagerUtils.c and src/NetworkManagerUtils.h are the only files that get modified by more than one commit; everything else is isolated to a single commit. (Eg, the files in src/devices/ are only modified in "486f8918 devices, active-connection: port to gdbus".) I haven't decided if it would make more sense to keep it like this when committing, or to squash it all together (so as to not break "git bisect" with bad commits, etc).

(In reply to Dan Winship from comment #22)
> OK, pushed danw/pre-gdbus-bgo622927. This is the last set of changes before
> the big dbus-glib to gdbus flag day. It's somewhat tested and ready for
> review (but someone should try running our QE tests against it too).


>> libnm-core: Makefile whitespace fixes

I intentionally added those new files with spaces at the end. Why do we even have non-leading tabs? I use a tab-width of 4, and it is not aligned. Don't we have a tab-width of 4 for NM-project? (or does that only apply to C, not Makefile?)

I'd rather see new entries with spaces, or can we just reformat the whole file and replace the non-leading tabs with spaces?



>> include: add nm-dbus-compat.h

this file is mostly a copy of "dbus-shared.h". Maybe point that out more clearly in the commit message and maybe add a code comment in the file, saying where the file originally comes from?

Or maybe copy "dbus-shared.h" directly, and preserve the original name? The same we did for "gsystem-local-alloc.h".




>> core: remove some redundant includes

How about going one step further and replace our internal includes of <glib.h>, <glib-object.h>, etc. by "nm-glib-compat.h"? It's a constant source of error to forget including "nm-glib-compat.h" (which we usually don't spot immediately).


>> core: add missing g_dbus_error_strip_remote_error() calls

+    /* reply will be unreffed when function exits */
     return !!reply;

let's remove this comment? We make heave use of the cleanup attribute now, no need to add comments about it.


>> core: add an NMExportedObject base class



Maybe add one commit before this, where you just

  cp src/nm-properties-changed-signal.c src/nm-exported-object.c
  cp src/nm-properties-changed-signal.h src/nm-exported-object.h

This could make for a nicer history.
OTOH, git-log can detect the rename if you pass "-M30", so maybe not...




Maybe include "nm-exported-object.h" in "nm-types.h"? It seems that there are very few places where we include "nm-types.h", without also needing "nm-exported-object.h".


Or how about instead adding a NetworkManagerCore.h, which basically has:

  #include "nm-glib-compat.h"
  #include "nm-version.h"
  #include "nm-types.h"
  #include "nm-exported-object.h"
  #include "nm-logging.h"

and then only include "NetworkManagerCore.h", instead of having to include this minimal set of headers over and over.

Just asking if we agree on this. Doesn't have to happen on this branch.



nm_exported_object_class_add_interface():

I think if the gobject_name contains underscores then there is no need to hash the name with the underscores. Only the original glib-property-names should be hashed: those with the hyphens.

Basically:

- g_hash_table_insert (classinfo->properties, (char *) gobject_name, (ch

  if (strchr (gobject_name, '_')) {
      ...
+ } else
+     g_hash_table_insert (classinfo->properties, (char *) gobject_name,




Interestingly, the change to NMSettings means that we don't emit "properties-changed" immediately, but collect them and idle_emit_properties_changed(). This seems an improvement. Also the signature of the signal changes, but it seems correct too...



nm_exported_object_dispose():

it sets pending_notifies() to NULL. Maybe only remove_all() and delay clearing the hash until finalize(). Otherwise we might crash on a spurious notify signal during deconstruction.

Maybe use nm_clear_g_source() to clear notify_idle_id().



I don't see how the "properties-changed" signal wire up with the D-Bus events? How does that work?



>> core: move D-Bus export/unexport into NMExportedObject

Let's make nm_exported_object_export() and nm_exported_object_unexport() idempotent, so that the following simplifies:

+              if (success && !nm_exported_object_is_exported (NM_EXPORTED_OBJECT (new_config))) {
                    /* Export over D-Bus */
+                   nm_exported_object_export (NM_EXPORTED_OBJECT (new_config));
               }

(also, drop that comment. If nm_exported_object_export() is not self explaining enough, rename it to nm_exported_object_export_over_dbus()).



Or if you think that the assertion in export() is of any use, let's add an util:
 nm_exported_object_ensure_exported() that can be called without need to check whether the object is already exported.


nm_exported_object_export(): maybe it's better (simplicity & performance-wise) to store the uint counter just along NM_EXPORTED_OBJECT_GET_CLASS (self)->export_path instead of having the prefix_counters hash.


And how about being extra careful and making the guint count a guint64?



>> core: port NMDhcp4Config/NMDhcp6Config to GVariant
    
let's have nm_dhcp6_config_get_options() [and others] *not* return an additional ref?

NMDhcp6ConfigPriv->options [and others] should always be a non-floating reference. E.g. sink the ref in nm_dhcp6_config_init() and nm_dhcp6_config_set_options().
Otherwise I think there is a bug with reference-counting/ownership.



dhcp6_state_changed() misses:
   g_object_notify (G_OBJECT (self), NM_DEVICE_DHCP6_CONFIG);




move the following:

+    if (!device_dhcp4_props)
+         device_dhcp4_props = g_variant_ref_sink (g_variant_new_array (G_VARIANT_TYPE ("{sv}"), NULL, 0));
+    if (!device_dhcp6_props)
+         device_dhcp6_props = g_variant_ref_sink (g_variant_new_array (G_VARIANT_TYPE ("{sv}"), NULL, 0));

into fill_device_props().









pushed two minor fixup!.





Overall, looks good!!

(In reply to Dan Winship from comment #22)

> Note that danw/gdbus-bgo622927 is "weird" in that none of the commits in it
> other than the final one actually compile, because all of the changes are
> intertwined. But it just seemed gross to throw them all into a single
> commit, so they're split up functionally for ease of review. Note that
> .gitignore, src/NetworkManagerUtils.c and src/NetworkManagerUtils.h are the
> only files that get modified by more than one commit; everything else is
> isolated to a single commit. (Eg, the files in src/devices/ are only
> modified in "486f8918 devices, active-connection: port to gdbus".) I haven't
> decided if it would make more sense to keep it like this when committing, or
> to squash it all together (so as to not break "git bisect" with bad commits,
> etc).

I think there is value in having smaller individual commits, even if they are not complete as a whole.

We don't bisect that much, and if we do, you probably want to skip anything but the --first-parent path anyway. IMO, I'd vote for not squashing them.

(In reply to Thomas Haller from comment #23)
> >> libnm-core: Makefile whitespace fixes
> 
> I intentionally added those new files with spaces at the end. Why do we even
> have non-leading tabs? I use a tab-width of 4, and it is not aligned. Don't
> we have a tab-width of 4 for NM-project? (or does that only apply to C, not
> Makefile?)

I don't think we have any standard/consistency for anything besides C files.

> I'd rather see new entries with spaces, or can we just reformat the whole
> file and replace the non-leading tabs with spaces?

I don't care either way, I just didn't like the inconsistency.

Actually, I'm going to just drop this commit for now; it's there because originally I was adding an nm-dbus-something.h to libnm-core, which would land between nm-core-internal.h and nm-keyfile-internal.h. But that file ended up becoming include/nm-dbus-compat.h instead, so there's no reason to be changing Makefile.libnm-core in this branch any more.

> >> include: add nm-dbus-compat.h
> 
> this file is mostly a copy of "dbus-shared.h". Maybe point that out more
> clearly in the commit message and maybe add a code comment in the file,
> saying where the file originally comes from?

The commit message says "Add a file containing the defines like DBUS_INTERFACE_DBUS from dbus-shared.h", and the file contains the comment "/* Copied from <dbus/dbus-shared.h> */" just after the boilerplate...

> Or maybe copy "dbus-shared.h" directly, and preserve the original name? The
> same we did for "gsystem-local-alloc.h".

dbus-shared.h also contains some stuff which is specific to libdbus though, like DBusBusType.

> >> core: remove some redundant includes
> 
> How about going one step further and replace our internal includes of
> <glib.h>, <glib-object.h>, etc. by "nm-glib-compat.h"? It's a constant
> source of error to forget including "nm-glib-compat.h" (which we usually
> don't spot immediately).

Ooh, that's a good idea. Maybe have "nm-glib.h" which includes <gio/gio.h> and "nm-glib-compat.h" instead? It would seem weird to be including a "compat" header for normal usage...

> >> core: add an NMExportedObject base class

> Or how about instead adding a NetworkManagerCore.h, which basically has:
> 
>   #include "nm-glib-compat.h"
>   #include "nm-version.h"
>   #include "nm-types.h"
>   #include "nm-exported-object.h"
>   #include "nm-logging.h"
> 
> and then only include "NetworkManagerCore.h", instead of having to include
> this minimal set of headers over and over.
> 
> Just asking if we agree on this. Doesn't have to happen on this branch.

That might make sense. And then you could drop the mass of #includes from nm-core-internal.h too probably.

> nm_exported_object_class_add_interface():
> 
> I think if the gobject_name contains underscores then there is no need to
> hash the name with the underscores. Only the original glib-property-names
> should be hashed: those with the hyphens.

That's copied straight from nm-properties-changed-signal.c, but you're right, there doesn't seem to be any reason to keep the underscore name.

> Interestingly, the change to NMSettings means that we don't emit
> "properties-changed" immediately, but collect them and
> idle_emit_properties_changed(). This seems an improvement. Also the
> signature of the signal changes, but it seems correct too...

Hm? It has a single DBUS_TYPE_G_MAP_OF_VARIANT argument and a G_TYPE_NONE return type in both the old nm-settings.c signal and the new nm-exported-object.c signal.

> I don't see how the "properties-changed" signal wire up with the D-Bus
> events? How does that work?

dbus-glib does that behind the scenes. (It knows about the signal from the DBusGObjectInfo.)

> >> core: port NMDhcp4Config/NMDhcp6Config to GVariant
>     
> let's have nm_dhcp6_config_get_options() [and others] *not* return an
> additional ref?

Hm... yes, not sure why I wrote it that way.

(In reply to Thomas Haller from comment #23)
> >> core: remove some redundant includes

as discussed on IRC, I replaced this with

    core: rename nm-glib-compat.h to nm-glib.h, use everywhere

However, one problem with this is that this is a private header, so we can't refer to it from public header files, so most parts of libnm*/ still need to manually include nm-glib.h if they need compat. (This is something that could be improved with the NetworkManagerCore.h plan though; you could have a "master" header file for each library.)

> >> core: move D-Bus export/unexport into NMExportedObject
> 
> Let's make nm_exported_object_export() and nm_exported_object_unexport()
> idempotent, so that the following simplifies:
> 
> +              if (success && !nm_exported_object_is_exported
> (NM_EXPORTED_OBJECT (new_config))) {
>                     /* Export over D-Bus */
> +                   nm_exported_object_export (NM_EXPORTED_OBJECT
> (new_config));
>                }

That is the only place that does "if !exported then export()". Everywhere else just unconditionally calls export() (because it knows that it's only going to be called once; eg, it calls it from constructed()). Given that, I don't think there's need for idempotence.

> nm_exported_object_export(): maybe it's better (simplicity &
> performance-wise) to store the uint counter just along
> NM_EXPORTED_OBJECT_GET_CLASS (self)->export_path instead of having the
> prefix_counters hash.

That won't work though, because, eg, NMKeyfileConnectionClass and NMIfcfgConnectionClass would then each have their own ->counter member, but they need to share the /org/freedesktop/NetworkManager/Settings/ namespace.

> >> core: port NMDhcp4Config/NMDhcp6Config to GVariant
>     
> let's have nm_dhcp6_config_get_options() [and others] *not* return an
> additional ref?

Oh, right, so the reason for adding a ref was that nm-dispatcher.c always needs to have *some* GVariant to pass to the D-Bus call, so in the case where there is no DHCP config, it has to create a new empty vardict (the code quoted below). So if nm_dhcp6_config_get_options() didn't call ref(), then we'd have to just ref() its return value from nm-dispatcher.c instead. Having NMDhcp6Config do it seemed more future-proof.

> move the following:
> 
> +    if (!device_dhcp4_props)
> +         device_dhcp4_props = g_variant_ref_sink (g_variant_new_array
> (G_VARIANT_TYPE ("{sv}"), NULL, 0));
> +    if (!device_dhcp6_props)
> +         device_dhcp6_props = g_variant_ref_sink (g_variant_new_array
> (G_VARIANT_TYPE ("{sv}"), NULL, 0));
> 
> into fill_device_props().

In the DISPATCHER_ACTION_HOSTNAME case, fill_device_props() never gets called, so this needs to stay in _dispatcher_call().


re-pushed. (but gdbus is not yet rebased on top of the new pre-gdbus.)

(In reply to Dan Winship from comment #26)
> (In reply to Thomas Haller from comment #23)
> > >> core: remove some redundant includes
> 
> as discussed on IRC, I replaced this with
> 
>     core: rename nm-glib-compat.h to nm-glib.h, use everywhere
> 
> However, one problem with this is that this is a private header, so we can't
> refer to it from public header files, so most parts of libnm*/ still need to
> manually include nm-glib.h if they need compat. (This is something that
> could be improved with the NetworkManagerCore.h plan though; you could have
> a "master" header file for each library.)

Great.

I would only include the file with quotes and not <>. Also, I would include it together with our internal headers, not with the system-headers.


>> settings: reorganize some NMSettingConnection code
    
It drops @remove_non_secrets argument, I think this is important. It was added by commit 978724da96b9750e2e91387e4fc4ed0aeb0c235d .



Otherwise, LGTM

(In reply to Thomas Haller from comment #27)
> I would only include the file with quotes and not <>. Also, I would include
> it together with our internal headers, not with the system-headers.

ok, done

> >> settings: reorganize some NMSettingConnection code
>     
> It drops @remove_non_secrets argument, I think this is important. It was
> added by commit 978724da96b9750e2e91387e4fc4ed0aeb0c235d .

In the new code, filter_secrets always removes all non-secrets, although it looks like, relative to the old code, secrets now don't get removed if (agent_dbus_owner && !agent_had_system), so that should be fixed. (Note that even in the old code, non-secrets didn't get removed if (!agent_dbus_owner && !(flags == NM_SECRET_AGENT_GET_SECRETS_FLAG_NONE)). Is that a bug?)

(In reply to Dan Winship from comment #28)
> (In reply to Thomas Haller from comment #27)
> > I would only include the file with quotes and not <>. Also, I would include
> > it together with our internal headers, not with the system-headers.
> 
> ok, done
> 
> > >> settings: reorganize some NMSettingConnection code
> >     
> > It drops @remove_non_secrets argument, I think this is important. It was
> > added by commit 978724da96b9750e2e91387e4fc4ed0aeb0c235d .
> 
> In the new code, filter_secrets always removes all non-secrets, although it
> looks like, relative to the old code, secrets now don't get removed if
> (agent_dbus_owner && !agent_had_system), so that should be fixed. (Note that
> even in the old code, non-secrets didn't get removed if (!agent_dbus_owner
> && !(flags == NM_SECRET_AGENT_GET_SECRETS_FLAG_NONE)). Is that a bug?)

Oh right. The old code seems also faulty in this regard.



was this change in semantics intentional? Looks like it fixes a preexisting bug?

-    if (flags != NM_SETTING_SECRET_FLAG_AGENT_OWNED)
-         g_hash_table_iter_remove (iter);
-    return TRUE;
+    return !!(flags & NM_SETTING_SECRET_FLAG_AGENT_OWNED);



pushed one fixup for this patch. Feel free to reject it.

Maybe move include/nm-dbus-glib-types.h into libnm-util now?  Then libnm-glib can just #include it from there.  But I don't think it really needs to be in include/ anymore.

settings/nm-settings-connection.c: In function 'agent_secrets_done_cb':
settings/nm-settings-connection.c:946:7: error: 'agent_had_system' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    if (agent_had_system) {
       ^
settings/nm-settings-connection.c:920:20: error: 'filtered_secrets' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   filtered_secrets = filter_secrets (NM_CONNECTION (self), secrets, clear_unsaved_secrets, NULL);
                    ^

Fixed that one up and pushed a few more fixes.

Another fixup; it passes distcheck now.

Also pushed a WIP commit that starts adding support for the ObjectManager interface, which is what we should eventually move libnm to use instead of all our internal code for getting objects one-by-one.

Moving to the ObjectManager will fix the problems we've run into that 2c299ba65c51e9c407090dc83929d692c74ee3f2 papers over.  Note that kdbus also has a hardcoded limit of 128 max replies per connection, which the fix in 2c299ba won't help.  We need to reduce the number of in-flight calls we make when starting up libnm-based applications so that we don't hit that limit, and GetManagedObjects() can help us do that in one call instead of a couple hundred.

Re-pushed pre-gdbus-bgo622927. I ended up totally redoing the nm-settings-connection patch; I didn't like having the secrets-walking logic duplicated in two different functions, so I made find_secret() just be a wrapper.

Also, relative to the last version, nm-glib is now included as "nm-glib.h" rather than <nm-glib.h>. Oh, and I pulled part of one of dcbw's fixups into "introspection: trivial fixes" (to include nmdbus-device-team in libnmdbus).

I think pre-gdbus is ready to go? Although, we still haven't gotten a working beaker test run. (Thomas's second attempt last week also seems to have failed for infrastructure-related reasons.)

I've re-pushed/rebased danw/gdbus-bgo622927 with some more fixups for the ObjectManager stuff.  Now there are couple issues; C&P from the commit log:

    Previously exported objects were not referenced by the BusManager,
    but instead removed from the exports hash via weak references.  The
    GDBusObjectManagerServer instead references exported objects, which
    can make them live much longer than they did before.
    
    This unfortunately causes issues with our singletons, which can now
    be re-created when NM quits.  Objects that can cause this are
    NMDHCPListener (which re-creates NMBusManager on dispose to clear
    its private server watch) and NMDevice (which re-creates FirewallManager
    on dispose to remove the interface from the zone).  We either need
    to revert to ref-ing the singletons in objects that use them, or
    somehow order our calls to _singleton_destructor to ensure core
    singletons live longer?

Haven't looked into that issue and I'm not sure I have the time...  but anyway, I'm happy to move the GDBusObjectManagerServer patches to a different bug so they don't block danw/gdbus-bgo622927 if people think that's a good idea.

Recreating a singleton is a bug, I think we even assert against it, don't we?


I think an object making use of any singleton object must either:

 a) be sure that the object doesn't use the singleton after end of main().
    An easy way to ensure that is if the object is destructed before the end of 
    main().

 b) own a reference to the singleton instance to keep it alive.


Especially for singleton objects using other singletons, a) is usually not easily true, so b) is better.


b) is actually a way to "order our calls to _singleton_destructor to ensure core singletons live longer"

Making people have to keep refs on the singletons again would be annoying.

dcbw and I were talking about this yesterday, and I suggested that instead of having each singleton register a destructor, we could just have each one prepend itself to a global singleton list when it is created, and then main() or some destructor could unref the singletons in order when exiting. That should ensure that they get destroyed in a reasonable order, as long as the singletons don't add themselves to the list until they are fully initialized (so that any singletons they make use of get added to the list before they do).

Created attachment 308094 [details] [review]
core: order destruction of singleton instances

Let singleton instances register their destruction via
nm_singleton_instance_register_destruction().

Objects that are registered later, will be destructed earlier. IOW,
they will be destroyed in reverse order of construction.

Previously, their order was undefined.


=====================

How about this? Untested, and applies on current master
(8bca864111a248ac4965f4f5887580c9829624da).

Created attachment 308095 [details] [review]
core: order destruction of singleton instances

Let singleton instances register their destruction via
nm_singleton_instance_register_destruction().

Objects that are registered later, will be destructed earlier. IOW,
they will be destroyed in reverse order of construction.

Previously, their order was undefined.



=====

v2

pushed pre-gdbus

Comment on attachment 308095 [details] [review]
core: order destruction of singleton instances

Moved modified version of patch to bug 752857.

Please review th/nm-default-bgo752857

ok, updated and re-pushed danw/gdbus-bgo622927, with all of dcbw's fixups merged in, and some other changes, and then I made sure it still distchecks this time.

Notable changes from before:

  - There's now an additional commit at the end:

        build: make libnm-util/libnm-glib optional

    which adds --without-libnm-glib, and you don't need libdbus/dbus-glib
    if you build with that option.

  - In addition to dcbw's patch to move nm-dbus-glib-types.h into libnm-util,
    I also moved nm-gvaluearray-compat.h there. (These are both in the
    second-to-last commit, "core: final gdbus porting".)

  - The for_each_secret() stuff in src/settings/nm-settings-connection.c has
    been redone, to match how it eventually ended up in the pre-gdbus branch.
    In addition, it fixes a bug that snuck in in the last revision of that
    code in pre-gdbus, which caused VPN secrets to never be found (because
    we were calling nm_setting_get_secret_flags() on secret_name rather
    than vpn_secret_name).

Basic testing passes. I can connect to wifi and VPN...

re-pushed with three new commits at the start:

  e409a76 core: fix NMManager in private-bus-only case
  3c136d5 core: better order the code at startup

The first is just a cleanup/drive-by. The second fixes a problem noticed by dcbw where on restart, clients were reconnecting to NM and getting back, eg, "No such interface 'org.freedesktop.NetworkManager.AgentManager' on object at path /org/freedesktop/NetworkManager/AgentManager". The problem is that the code was doing:

  - claim D-Bus name
  - export objects
  - start main loop
     - dbus-glib starts accepting client connections

but with gdbus, that becomes

  - claim D-Bus name
     - gdbus starts accepting client connections in another thread
     - gdbus rejects method calls to unknown interfaces
  - export objects
  - start main loop

so we have to make sure to export all toplevel objects (NMManager, NMSettings, NMAgentManager) before claiming our name (while still making sure to not do time-consuming things before claiming the D-Bus name, so as to not slow down startup).


The third commit:

  3f77738 core: change the way destruction of singletons works

fixes the crash-on-shutdown caused by destroying the singletons in a bad order.

(In reply to Dan Winship from comment #44)
> re-pushed with three new commits at the start:
> 
>   e409a76 core: fix NMManager in private-bus-only case
>   3c136d5 core: better order the code at startup

We could rename nm_manager_new() to nm_manager_setup() to match the pattern of other singletons that need any setup. IMO a function named _new() should be callable multiple times (without asserting !singleton).

While at it, we could also rename @singleton to @singleton_instance -- because all other singleton instances are named like that.

I think, instead of main() calling nm_settings_new(), the @settings instance should be primarily owned by one of our singletons. Let's have nm_manager_new() call nm_settings_new() instead of accepting a @settings argument.
Also have nm_manager_start() call to nm_settings_start().


> The first is just a cleanup/drive-by. The second fixes a problem noticed by
> dcbw where on restart, clients were reconnecting to NM and getting back, eg,
> "No such interface 'org.freedesktop.NetworkManager.AgentManager' on object
> at path /org/freedesktop/NetworkManager/AgentManager". The problem is that
> the code was doing:
> 
>   - claim D-Bus name
>   - export objects
>   - start main loop
>      - dbus-glib starts accepting client connections
> 
> but with gdbus, that becomes
> 
>   - claim D-Bus name
>      - gdbus starts accepting client connections in another thread
>      - gdbus rejects method calls to unknown interfaces
>   - export objects
>   - start main loop
> 
> so we have to make sure to export all toplevel objects (NMManager,
> NMSettings, NMAgentManager) before claiming our name (while still making
> sure to not do time-consuming things before claiming the D-Bus name, so as
> to not slow down startup).
> 
> 
> The third commit:
> 
>   3f77738 core: change the way destruction of singletons works

This is similar to 01bece28929f6c9ae665ae2b9bc608a75396f762 from th/nm-default-bgo752857 branch, as mentioned in comment 42. Was there a problem with that?


> fixes the crash-on-shutdown caused by destroying the singletons in a bad
> order.

Pushed a patch to have the Manager create the Settings singleton.

The rest of the patches look OK to me...

(In reply to Thomas Haller from comment #45)
> We could rename nm_manager_new() to nm_manager_setup()...
> 
> While at it, we could also rename @singleton to @singleton_instance...
> 
> I think, instead of main() calling nm_settings_new(), the @settings instance
> should be primarily owned by one of our singletons. Let's have
> nm_manager_new() call nm_settings_new() instead of accepting a @settings
> argument.
> Also have nm_manager_start() call to nm_settings_start().

New commit "d5b7d5c core: make NMManager singleton more like others" does all that.

> >   3f77738 core: change the way destruction of singletons works
> 
> This is similar to 01bece28929f6c9ae665ae2b9bc608a75396f762 from
> th/nm-default-bgo752857 branch, as mentioned in comment 42. Was there a
> problem with that?

No, I just forgot you'd done that. I'll remove this commit if that's going to be committed first.

(In reply to Dan Williams from comment #46)
> Pushed a patch to have the Manager create the Settings singleton.

oops, which I push --force'd over with a different patch

Branch looks OK to me now, so fine to be merged once the singleton stuff gets ironed out.  We'll figure out the rest of the details later.

pushed!