GNOME Bugzilla – Bug 747465
[review] lr/veth-no-external-up: lift the external IFF_UP requirement for veths
Last modified: 2015-04-13 11:56:28 UTC
Created attachment 301082 [details] [review] types: make veth non-software device For all practical purposes, the virtual ethernet should be treated as ordinary ethernet. It should not wait until being upped externally inside a container. Outside the container the udev rule still prevents veths from being managed. --- src/nm-types.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
this affects what nm_platform_link_is_software() returns, which affects priv->is_software of a device. Also, it affects the exposed capabilities (NM_DEVICE_CAP_IS_SOFTWARE). It seems that a veth should still have the capability NM_DEVICE_CAP_IS_SOFTWARE. Which part of "veth being is_software" causes a problem that this patch fixes? I think the commit message should explain the actual consequences (beyond: a veth is no longer veth) and why we want that change.
a veth is no longer veth ^^^^ is_software
Reworked as discussed with Thomas in person. 9fed01e device: turn off "unmanaged unless IFF_UP externally" for veth d8ffe95 device: move the decision whether to wait for IFF_UP a virtual function
(In reply to Lubomir Rintel from comment #3) > Reworked as discussed with Thomas in person. > > 9fed01e device: turn off "unmanaged unless IFF_UP externally" for veth > d8ffe95 device: move the decision whether to wait for IFF_UP a virtual > function looks good, except that the name "manage_on_external_up" sounds to me like having the opposite meaning. How about "can_unmanaged_external_down()"? + * didn't create it. */ + return FALSE; +} ^^^^^ whitespace
lr/veth-no-external-up updated
- if (is_software_external (self)) { + if (NM_DEVICE_GET_CLASS (self)->can_unmanaged_external_down (self)) { Maybe can be replaced with: "klass->can_unmanaged_external_down (self))" ? Anyway, looks good to me.
(In reply to Beniamino Galvani from comment #6) > - if (is_software_external (self)) { > + if (NM_DEVICE_GET_CLASS (self)->can_unmanaged_external_down (self)) { > > Maybe can be replaced with: "klass->can_unmanaged_external_down (self))" ? No strong opinion here. However, I think that we usually assign the macro expansion to a variable when we use it multiple times which is not the case here; I'm leaving it as it is. > Anyway, looks good to me. Thank you. Merged: 0b9a4cd device: merge branch 'lr/veth-no-external-up' bcc79cc device: turn off "unmanaged unless IFF_UP externally" for veth adb6e9a device: move the decision whether to wait for IFF_UP a virtual function