>> core: Fall back to CLOCK_MONOTONIC if CLOCK_BOOTTIME unsupported

How about the fixup I pushed on top of your branch?

It saves the call to "clock_gettime (CLOCK_BOOTTIME, tp);" that is going to fail every time.

but more importantly, it decides once which clock to use and sticks to that.




The rest looks good (including "contrib/rpm: Run make check")

> tests: Don't run session-long dbus daemons for tests

Oh, huh. I had thought "dbus-launch --exit-with-session CMD" worked the same way as, eg, "ssh-agent CMD" or "gpg-agent --daemon CMD" (where it exits when CMD exits)...

>+    eval `dbus-launch`
>+    export DBUS_SESSION_BUS_ADDRESS

if you do

  eval `dbus-launch --sh-syntax`

it will include the export command for you.


> core: Fall back to CLOCK_MONOTONIC if CLOCK_BOOTTIME unsupported

I like Thomas's fix to only end up calling with the wrong value once.

(Also, maybe there should be a comment in the code pointing out that it's only needed for mock builds on RHEL-6, so we know when it's safe to delete it.)


> libnm: Ignore NoReply errors when NM has vanished from the bus

Someone else posted a patch for this to some other branch too, but I can't remember where. I think the other one was more complicated than yours, but yours looks like it should work fine.

ah, the other client-nm-running fix was in dcbw/libnm-fixes, bug 738588

Added fixes for the last two test failures I've seen:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=lr/rpm-make-check&id=b86c4f03a1e67b9e4a4ee3f34aaadd413bd6c4bb

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=lr/rpm-make-check&id=785144510710a32ab8b3779b0d81d201d3a69941

(In reply to comment #4)
> Added fixes for the last two test failures I've seen:
> 
> http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=lr/rpm-make-check&id=b86c4f03a1e67b9e4a4ee3f34aaadd413bd6c4bb

looks right.

 
> http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=lr/rpm-make-check&id=785144510710a32ab8b3779b0d81d201d3a69941

I think this should be different. Pushed a fixup.

(In reply to comment #5)
> (In reply to comment #4)
> > http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=lr/rpm-make-check&id=785144510710a32ab8b3779b0d81d201d3a69941
> 
> I think this should be different. Pushed a fixup.

Applied.

Note that I've last seen the test fail the last Thursday and could not make it fail afterwards therefore I'm having trouble verifying the fix. Maybe some other change fixed it?

What bug does the "Don't clean up the connections if they hadn't been set yet." change fix?

In particular, the reason it currently unsets priv->all_connections first, and then works from a local copy of it, is to guarantee that nothing that any CONNECTION_REMOVED signal handler does can cause priv->all_connections to be modified while we're iterating it. (In this case, I'm not sure there's anything a signal handler could do that would mess things up anyway, but it's good defensive coding anyway...)

(In reply to comment #7)
> What bug does the "Don't clean up the connections if they hadn't been set yet."
> change fix?

I've seen a connections == NULL here when running the nm-client-test repeatedly (unfortunately I've lost the screen capture):

g_ptr_array_unref (connections);

I've concluded that nm_running_changed() may be triggered very early if the daemon terminates before we've gotten the connection list.

(In reply to comment #8)
> I've seen a connections == NULL here when running the nm-client-test repeatedly
...
> I've concluded that nm_running_changed() may be triggered very early if the
> daemon terminates before we've gotten the connection list.

Hm... priv->all_connections is never NULL until after dispose(). So the only way connections could be NULL is if nm_running_changed() ran after dispose(), and your patch already fixes that case separately anyway, so I don't think the nm_running_changed() part of the patch is needed.

(In reply to comment #9)
> (In reply to comment #8)
> > I've seen a connections == NULL here when running the nm-client-test repeatedly
> ...
> > I've concluded that nm_running_changed() may be triggered very early if the
> > daemon terminates before we've gotten the connection list.
> 
> Hm... priv->all_connections is never NULL until after dispose(). So the only
> way connections could be NULL is if nm_running_changed() ran after dispose(),
> and your patch already fixes that case separately anyway, so I don't think the
> nm_running_changed() part of the patch is needed.


The call to cleanup_connection() seems still needed to me.

I agree, with your point on defensive. But then the best way seem to be:

         /* Clear connections */
         while (priv->all_connections->len) {
              guint idx = priv->all_connections->len - 1;
              NMRemoteConnection *connection = priv->all_connections->pdata[idx];

              g_ptr_array_remove_index (priv->all_connections, idx);
              cleanup_connection (self, connection);
              g_signal_emit (self, signals[CONNECTION_REMOVED], 0, connection);
         }

(In reply to comment #10)
> The call to cleanup_connection() seems still needed to me.

Though admittedly non-obvious, cleanup_connection() already gets called, from within connection_removed() when we emit the connection-removed signal

(In reply to comment #11)
> (In reply to comment #10)
> > The call to cleanup_connection() seems still needed to me.
> 
> Though admittedly non-obvious, cleanup_connection() already gets called, from
> within connection_removed() when we emit the connection-removed signal


ah indeed. That might be worth a comment.

It seems like this branch already got merged?  Can the bug be closed now?

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=48b4f6f8307addb821aea0db22c3a4801cf7a336

Merged