[libvirt] [PATCH] lxc: don't up the veth interfaces unless explicitly asked to
Upping an interface for no reason and not configuring it is a cardinal sin. With the default addrgenmode if eui64 it sticks a link-local address to the interface. That is not good, as NetworkManager would see an address configured, assume the interface is already configured and won't touch it iself and the interface might stay unconfigured until the end of the days. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1124721 --- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_native.c | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index cc20b6d..50e38e9 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -541,7 +541,7 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, VIR_FREE(ipStr); } - if (netDef->linkstate != VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) { + if (netDef->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP) { VIR_DEBUG("Enabling %s", newname); rc = virNetDevSetOnline(newname, true); if (rc < 0) diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index c15eb19..2297dbe 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -348,12 +348,10 @@ lxcCreateNetDef(const char *type, if (VIR_ALLOC(net) < 0) goto error; - if (flag) { - if (STREQ(flag, "up")) - net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; - else - net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN; - } + if (flag && STREQ(flag, "up")) + net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; + else + net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN; if (VIR_STRDUP(net->ifname_guest, name) < 0) goto error; -- 2.1.0
On Wed, Apr 22, 2015 at 05:02:49PM +0200, Lubomir Rintel wrote:
Upping an interface for no reason and not configuring it is a cardinal sin.
The interface may well be configured with both IPv4 & IPv6 addresses depending on the XML config for the guest....
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index cc20b6d..50e38e9 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -541,7 +541,7 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, VIR_FREE(ipStr); }
- if (netDef->linkstate != VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) { + if (netDef->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP) {
...so this is wrong because it doesn't take account of the fact that we may have configured addresses just a few lines earlier in this very same function I'm fine with not setting the link state to online, but *only* if we do not have any addresses configured in the XML for that NIC. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Upping an interface for no reason and not configuring it is a cardinal sin. With the default addrgenmode if eui64 it sticks a link-local address to the interface. That is not good, as NetworkManager would see an address configured, assume the interface is already configured and won't touch it iself and the interface might stay unconfigured until the end of the days. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1124721 --- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_native.c | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index cc20b6d..bd135c7 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -541,7 +541,7 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, VIR_FREE(ipStr); } - if (netDef->linkstate != VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) { + if (netDef->nips || netDef->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP) { VIR_DEBUG("Enabling %s", newname); rc = virNetDevSetOnline(newname, true); if (rc < 0) diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index c15eb19..2297dbe 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -348,12 +348,10 @@ lxcCreateNetDef(const char *type, if (VIR_ALLOC(net) < 0) goto error; - if (flag) { - if (STREQ(flag, "up")) - net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; - else - net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN; - } + if (flag && STREQ(flag, "up")) + net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; + else + net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN; if (VIR_STRDUP(net->ifname_guest, name) < 0) goto error; -- 2.3.5
On 24.04.2015 15:52, Lubomir Rintel wrote:
Upping an interface for no reason and not configuring it is a cardinal sin.
With the default addrgenmode if eui64 it sticks a link-local address to the interface. That is not good, as NetworkManager would see an address configured, assume the interface is already configured and won't touch it iself and the interface might stay unconfigured until the end of the days.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1124721 --- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_native.c | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index cc20b6d..bd135c7 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -541,7 +541,7 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef, VIR_FREE(ipStr); }
- if (netDef->linkstate != VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) { + if (netDef->nips || netDef->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP) {
I'd split this long line into two.
VIR_DEBUG("Enabling %s", newname); rc = virNetDevSetOnline(newname, true); if (rc < 0) diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index c15eb19..2297dbe 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -348,12 +348,10 @@ lxcCreateNetDef(const char *type, if (VIR_ALLOC(net) < 0) goto error;
- if (flag) { - if (STREQ(flag, "up")) - net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; - else - net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN; - } + if (flag && STREQ(flag, "up"))
or STREQ_NULLABLE().
+ net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP; + else + net->linkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN;
if (VIR_STRDUP(net->ifname_guest, name) < 0) goto error;
I've fixed both nits, ACKed and pushed. Michal
participants (3)
-
Daniel P. Berrange -
Lubomir Rintel -
Michal Privoznik