GNOME Bugzilla – Bug 762731
[review] lr/virtual-parent-bgo762731: activating vlan on virtual device fails with: failed to determine interface name: error determine name for vlan
Last modified: 2017-02-10 13:41:40 UTC
create a team-slave, a team, and a vlan on top of the team. nmcli connection delete t-slave t-team t-vlan DEV=enp0s25 nmcli connection add type team con-name t-team autoconnect no -- \ connection.interface-name t-i-team \ ipv6.method ignore \ ipv4.method manual \ ipv4.addresses 192.168.103.5/24 \ ipv4.gateway 192.168.103.1 nmcli connection add type ethernet con-name t-slave slave-type team \ ifname $DEV autoconnect no master t-team nmcli connection add type vlan con-name t-vlan autoconnect no id 42 \ dev t-i-team -- \ ipv6.method ignore \ ipv4.method manual \ ipv4.addresses 192.168.104.5/24 \ ipv4.gateway 192.168.104.1 nmcli connection up t-vlan # fails with: # Error: Connection activation failed: failed to determine interface # name: error determine name for vlan # but nmcli connection up t-slave nmcli connection up t-vlan # succeeds.
(In reply to Thomas Haller from comment #0) > nmcli connection up t-vlan > # fails with: > # Error: Connection activation failed: failed to determine interface > # name: error determine name for vlan should be: $ nmcli connection up t-vlan Error: Connection activation failed: t-i-team.42 failed to create resources: no support for VLANs on interface t-i-team of type Team
we have to do something similar to what we do with master/slave relation ship: delay activation until the master is realized. It's a bit more complicated.
(In reply to Thomas Haller from comment #2) > we have to do something similar to what we do with master/slave relation > ship: delay activation until the master is realized. It's a bit more > complicated. Yes, you're right. lr/virtual-parent-bgo762731
>> manager: split out best_connection_for_device() nm_device_get_available_connections() only has one single caller best_connection_for_device(). Still, it first needs to allocate a new GPtrArray which is thrown aways immediately afterwards. Instead, add a function nm_device_get_best_connection(). and remove nm_device_get_available_connections() as there are no more callers. >> active-connection: add parent active connection tracking >> manager: allow delaying the device activation when the parent is not real I dislike a bit that we don't own a reference to priv->parent, so it all depends on the CONNECTION_STATE >= ACTIVATED to really come. Maybe install a weak-ref too to detect that the parent is gone and to proceed with the activation (by still emiting PARENT_ACTIVE). Also, as we don't hold references to priv->parent, parent_state_cb() should first disconnect and then emit the PARENT_ACTIVE signal. The second commit is pretty tough to understand for me. Could you first do a simpler commit that only moves code to unmanaged_to_disconnected()? Overall, looks good!!
(In reply to Thomas Haller from comment #4) > >> manager: split out best_connection_for_device() > > nm_device_get_available_connections() only has one single caller > best_connection_for_device(). Still, it first needs to allocate a new > GPtrArray which is thrown aways immediately afterwards. > > Instead, add a function nm_device_get_best_connection(). and remove > nm_device_get_available_connections() as there are no more callers. A good catch; done. This indeed looks more straightforward. > >> active-connection: add parent active connection tracking > >> manager: allow delaying the device activation when the parent is not real > > I dislike a bit that we don't own a reference to priv->parent, so it all > depends on the CONNECTION_STATE >= ACTIVATED to really come. Maybe install a > weak-ref too to detect that the parent is gone and to proceed with the > activation (by still emiting PARENT_ACTIVE). > > Also, as we don't hold references to priv->parent, parent_state_cb() should > first disconnect and then emit the PARENT_ACTIVE signal. Agreed. Done. > The second commit is pretty tough to understand for me. Could you first do a > simpler commit that only moves code to unmanaged_to_disconnected()? Yes. Done too. > Overall, looks good!! Pushed a new version.
in active_connection_parent_active(), let's first disconnect active_connection_parent_active before doing anything else? Minor recurring nitpick: g_object_weak_ref (G_OBJECT (priv->parent), parent_destroyed, self); the G_OBJECT() cast does a runtime check (possibly printing a g_critical), only to pass NULL down to g_object_weak_ref(), which at first does: g_return_if_fail (G_IS_OBJECT (object)); so, you end up checking the type twice for a condition that is supposed to never happen. The G_OBJECT() cast is mostly useless, as it doesn't prevent g_object_weak_ref being called with a bad pointer. Regardless of this minor thing, the branch looks good to me.
Merged with fixes for the above.
https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=bbc941245dac37083f0b46e115191c59bfbf95eb