GNOME Bugzilla – Bug 737659
[review] lr/unmanaged-slaves: Remove unmanaged slaves from master when they disappear
Last modified: 2015-01-12 15:51:03 UTC
Created attachment 287448 [details] [review] [PATCH] device: Don't enslave unmanaged devices They don't notify of their removal as they don't transition to UNMANAGED state in remove_device() and thus leave hanging in bridges despite the actual device is gone. It's probably a good idea to ignore them altogether. This happens when they are left in place: [lkundrak@belphegor ~]$ nmcli c (process:26000): libnm-WARNING **: Could not fetch property 'DeviceType' of interface 'org.freedesktop.NetworkManager.Device' on /org/freedesktop/NetworkManager/Devices/16: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn't exist (process:26000): libnm-WARNING **: Could not fetch property 'DeviceType' of interface 'org.freedesktop.NetworkManager.Device' on /org/freedesktop/NetworkManager/Devices/14: GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: Method "Get" with signature "ss" on interface "org.freedesktop.DBus.Properties" doesn't exist NAME UUID TYPE DEVICE vnet6 1c6cd3aa-6109-4809-a898-6690b5e58546 generic vnet6 ... [lkundrak@belphegor ~]$ :(
(In reply to comment #0) > Created an attachment (id=287448) [details] [review] > [PATCH] device: Don't enslave unmanaged devices > > They don't notify of their removal as they don't transition to UNMANAGED > state in remove_device() and thus leave hanging in bridges despite the > actual device is gone. It's probably a good idea to ignore them altogether. > A g_return_*() is an assert that should not actually fail. It should better not be used for actual program logic.
Created attachment 288048 [details] [PATCH] device: Don't enslave unmanaged devices (In reply to comment #1) > (In reply to comment #0) > > Created an attachment (id=287448) [details] [review] [details] [review] > > [PATCH] device: Don't enslave unmanaged devices > > > > They don't notify of their removal as they don't transition to UNMANAGED > > state in remove_device() and thus leave hanging in bridges despite the > > actual device is gone. It's probably a good idea to ignore them altogether. > > > > A g_return_*() is an assert that should not actually fail. It should better not > be used for actual program logic. Fixed. Thank you.
Created attachment 288050 [details] [PATCH] device: Don't enslave unmanaged devices Whoops...
Hm... we went out of our way to try to track master/slave changes on unmanaged devices. I think the fix here needs to be on the other end; make whatever changes are needed to clean things up when the slave goes away.
Created attachment 290793 [details] [PATCH] device: Remove unmanaged slaves from master when they disappear
Comment on attachment 290793 [details] [PATCH] device: Remove unmanaged slaves from master when they disappear >We ought to always emit the device removed signal if the device disappears, >watch for it being emmitted for slaves and react appropriately. typo "emmitted" >@@ -762,6 +762,7 @@ remove_device (NMManager *manager, > > g_signal_emit (manager, signals[DEVICE_REMOVED], 0, device); > g_object_notify (G_OBJECT (manager), NM_MANAGER_DEVICES); >+ g_signal_emit_by_name (device, NM_DEVICE_REMOVED); This seems stylistically backwards; NMManager should be calling a method on NMDevice, not forcing it to emit a signal which was really there for NMManager's benefit, not NMDevice's... If you added nm_device_removed(), then that could do the master/slave cleanup (without needing the SlaveInfo changes), and then emit the signal (so that the existing places that call g_signal_emit_by_name(device, NM_DEVICE_REMOVED) could call this function instead).
(In reply to comment #6) > If you added nm_device_removed(), then that could do the master/slave cleanup > (without needing the SlaveInfo changes), and then emit the signal (so that the > existing places that call g_signal_emit_by_name(device, NM_DEVICE_REMOVED) > could call this function instead). Done: http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/unmanaged-slaves I removed the signal emission from generic device code altogether, as I only needed it for this purpose (some device types trigger it as they need it).
LGTM
(In reply to comment #7) > I removed the signal emission from generic device code altogether, as I only > needed it for this purpose (some device types trigger it as they need it). Yeah, I was suggesting that those types could just call nm_device_removed() (which would then emit the signal) rather than emitting the signal themselves. But this way is fine too. LGTM
[master c83b40a] device: Remove unmanaged slaves from master when they disappear
reopening... this causes NM to remove all slaves from all master on exit, meaning, eg, that restarting NM breaks networking to all virtual machines (https://bugzilla.redhat.com/show_bug.cgi?id=1169936). one-line fix is at danw/slave-removed-rh1169936. I think that fixes this bug without unfixing the original bug?
(In reply to comment #11) > reopening... this causes NM to remove all slaves from all master on exit, > meaning, eg, that restarting NM breaks networking to all virtual machines > (https://bugzilla.redhat.com/show_bug.cgi?id=1169936). > > one-line fix is at danw/slave-removed-rh1169936. I think that fixes this bug > without unfixing the original bug? LGTM (oh, this problem is part of rhel7-beta)
committed