After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 740775 - [review] dcbw/cli-down-wait-bgo740775: wait for connection deactivation
[review] dcbw/cli-down-wait-bgo740775: wait for connection deactivation
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: nmcli
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-11-26 19:23 UTC by Dan Williams
Modified: 2014-12-03 10:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Description Dan Williams 2014-11-26 19:23:49 UTC 
DeactivateConnection() returns before the device is completely deactivated (because that may take a while due to dispatcher scripts or some operations like DCB cleanup).  But "nmcli con down" should wait for the operation to succeed, or for a short timeout.  The user can obviously do "nmcli --wait 0 con down" to proceed immediately.
Comment 1 Dan Winship 2014-12-01 11:37:25 UTC 
looks good
Comment 2 Thomas Haller 2014-12-01 15:35:21 UTC 
Both down_active_connection_state_cb() and down_timeout_cb() free the user_data. Not a real issue, because we quit() the mainloop in both cases. But seems error prone.

Otherwise looks good.
Comment 3 Dan Williams 2014-12-01 16:13:12 UTC 
20566c76de0d18162af020852ae1e2055addcfff
cli: wait for "con down" to deactivate the connection (bgo #740775) (rh #1168383)
Comment 4 Dan Williams 2014-12-01 16:13:54 UTC 
(In reply to comment #2)
> Both down_active_connection_state_cb() and down_timeout_cb() free the
> user_data. Not a real issue, because we quit() the mainloop in both cases. But
> seems error prone.

I changed the patch to remove both the timeout (if not triggered) and the signal handler from the free function.  This should address both these issues.
Comment 5 Jiri Klimes 2014-12-02 11:25:10 UTC 
Reopening for a fix with multiple connections: jk/cli-con-down-bgo740775

Sorry for not reviewing earlier, but I was distracted by other things.
Comment 6 Dan Williams 2014-12-02 23:46:32 UTC 
(In reply to comment #5)
> Reopening for a fix with multiple connections: jk/cli-con-down-bgo740775
> 
> Sorry for not reviewing earlier, but I was distracted by other things.

Aha!  I explicitly tested this case, but I was not using nmcli commands correctly, and thus I assumed that it was broken and no longer useful.

But I don't think the 'counter' stuff is going to work like you expect, since that'll just keep incrementing up.  Yeah, I guess this is one-shot...

But how about the fixup I pushed instead?  That uses a single 'info' for all the requests and quits when there is nothing left in the queue.  I think that's a slightly more robust solution.
Comment 7 Jiri Klimes 2014-12-03 10:04:51 UTC 
Your fixup is fine. I have squashed and pushed to master as:
4a7c886 cli: fix deactivation for multiple connections (bgo #740775) (rh #1168383)