[libvirt] [PATCH 00/10] Misc LXC related fixes
The following is a list of random bug fixes I've identified while working on LXC
From: "Daniel P. Berrange" <berrange@redhat.com> Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_process.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 94a74dd..1bf4697 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1292,7 +1292,7 @@ virLXCProcessReconnectDomain(void *payload, const void *name ATTRIBUTE_UNUSED, v virLXCDomainObjPrivatePtr priv; virDomainObjLock(vm); - VIR_DEBUG("Reconnect %d %d %d\n", vm->def->id, vm->pid, vm->state.state); + VIR_DEBUG("Reconnect id=%d pid=%d state=%d", vm->def->id, vm->pid, vm->state.state); priv = vm->privateData; -- 1.7.11.2
From: "Daniel P. Berrange" <berrange@redhat.com> The initpid will be required long term to enable LXC to implement various hotplug operations. Thus it needs to be persisted in the domain status XML. LXC has not used the domain status XML before, so this introduces use of the helpers. --- src/lxc/lxc_domain.c | 32 ++++++++++++++++++++++++++++++++ src/lxc/lxc_domain.h | 2 ++ src/lxc/lxc_process.c | 7 +++++++ 3 files changed, 41 insertions(+) diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index 17f1d0c..bd80d9f 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -24,6 +24,10 @@ #include "lxc_domain.h" #include "memory.h" +#include "logging.h" +#include "virterror_internal.h" + +#define VIR_FROM_THIS VIR_FROM_LXC static void *virLXCDomainObjPrivateAlloc(void) { @@ -43,8 +47,36 @@ static void virLXCDomainObjPrivateFree(void *data) } +static int virLXCDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) +{ + virLXCDomainObjPrivatePtr priv = data; + + virBufferAsprintf(buf, " <init pid='%llu'/>\n", + (unsigned long long)priv->initpid); + + return 0; +} + +static int virLXCDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) +{ + virLXCDomainObjPrivatePtr priv = data; + unsigned long long thepid; + + if (virXPathULongLong("string(./init[1]/@pid)", ctxt, &thepid) < 0) { + virErrorPtr err = virGetLastError(); + VIR_WARN("Failed to load init pid from state %s", err ? err->message : "null"); + priv->initpid = 0; + } else { + priv->initpid = thepid; + } + + return 0; +} + void virLXCDomainSetPrivateDataHooks(virCapsPtr caps) { caps->privateDataAllocFunc = virLXCDomainObjPrivateAlloc; caps->privateDataFreeFunc = virLXCDomainObjPrivateFree; + caps->privateDataXMLFormat = virLXCDomainObjPrivateXMLFormat; + caps->privateDataXMLParse = virLXCDomainObjPrivateXMLParse; } diff --git a/src/lxc/lxc_domain.h b/src/lxc/lxc_domain.h index b1dd5d9..882f34a 100644 --- a/src/lxc/lxc_domain.h +++ b/src/lxc/lxc_domain.h @@ -34,6 +34,8 @@ struct _virLXCDomainObjPrivate { bool doneStopEvent; int stopReason; bool wantReboot; + + pid_t initpid; }; void virLXCDomainSetPrivateDataHooks(virCapsPtr caps); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 1bf4697..d489c04 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -637,11 +637,18 @@ static void virLXCProcessMonitorExitNotify(virLXCMonitorPtr mon ATTRIBUTE_UNUSED priv->stopReason, status); } +/* XXX a little evil */ +extern virLXCDriverPtr lxc_driver; static void virLXCProcessMonitorInitNotify(virLXCMonitorPtr mon ATTRIBUTE_UNUSED, pid_t initpid, virDomainObjPtr vm) { + virLXCDomainObjPrivatePtr priv = vm->privateData; + priv->initpid = initpid; virDomainAuditInit(vm, initpid); + + if (virDomainSaveStatus(lxc_driver->caps, lxc_driver->stateDir, vm) < 0) + VIR_WARN("Cannot update XML with PID for LXC %s", vm->def->name); } static virLXCMonitorCallbacks monitorCallbacks = { -- 1.7.11.2
The initpid will be required long term to enable LXC to implement various hotplug operations. Thus it needs to be persisted in the domain status XML. LXC has not used the domain status XML before, so this introduces use of the helpers. --- src/lxc/lxc_domain.c | 32 ++++++++++++++++++++++++++++++++ src/lxc/lxc_domain.h | 2 ++ src/lxc/lxc_process.c | 7 +++++++ 3 files changed, 41 insertions(+)
ACK.
From: "Daniel P. Berrange" <berrange@redhat.com> The code setting up LXC cgroups used an 'rc' variable both for capturing the return value of methods it calls, and its own return status. The result was that several failures in setting up cgroups would actually result in success being returned. Use a separate 'ret' for tracking return value as per normal code design in other parts of libvirt Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_cgroup.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index bdfaa54..ed86b43 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -224,7 +224,8 @@ int virLXCCgroupSetup(virDomainDefPtr def) { virCgroupPtr driver = NULL; virCgroupPtr cgroup = NULL; - int rc = -1; + int ret = -1; + int rc; rc = virCgroupForDriver("lxc", &driver, 1, 0); if (rc != 0) { @@ -234,7 +235,7 @@ int virLXCCgroupSetup(virDomainDefPtr def) virReportSystemError(-rc, "%s", _("Unable to get cgroup for driver")); - return rc; + goto cleanup; } rc = virCgroupForDomain(driver, def->name, &cgroup, 1); @@ -262,11 +263,14 @@ int virLXCCgroupSetup(virDomainDefPtr def) virReportSystemError(-rc, _("Unable to add task %d to cgroup for domain %s"), getpid(), def->name); + goto cleanup; } + ret = 0; + cleanup: virCgroupFree(&cgroup); virCgroupFree(&driver); - return rc; + return ret; } -- 1.7.11.2
From: "Daniel P. Berrange" <berrange@redhat.com> The LXC driver relies on use of cgroups to kill off LXC processes in shutdown. If cgroups aren't available, we're unable to kill off processes, so we must treat lack of cgroups as a fatal startup error. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_cgroup.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index ed86b43..912233f 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -229,10 +229,6 @@ int virLXCCgroupSetup(virDomainDefPtr def) rc = virCgroupForDriver("lxc", &driver, 1, 0); if (rc != 0) { - /* Skip all if no driver cgroup is configured */ - if (rc == -ENXIO || rc == -ENOENT) - return 0; - virReportSystemError(-rc, "%s", _("Unable to get cgroup for driver")); goto cleanup; -- 1.7.11.2
From: "Daniel P. Berrange" <berrange@redhat.com> When failing to create a macvlan interface, make sure the error message contains the name of the host interface Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virnetdevmacvlan.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index bd26ba9..d8e646a 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -176,8 +176,8 @@ virNetDevMacVLanCreate(const char *ifname, default: virReportSystemError(-err->error, - _("error creating %s type of interface"), - type); + _("error creating %s type of interface attach to %s"), + type, srcdev); goto cleanup; } break; -- 1.7.11.2
From: "Daniel P. Berrange" <berrange@redhat.com> If the <interface> device did not contain any <target> element, LXC would crash on a NULL pointer if starting the container failed Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index d489c04..954cb9e 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -254,7 +254,8 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, for (i = 0 ; i < vm->def->nnets ; i++) { virDomainNetDefPtr iface = vm->def->nets[i]; vport = virDomainNetGetActualVirtPortProfile(iface); - ignore_value(virNetDevSetOnline(iface->ifname, false)); + if (iface->ifname) + ignore_value(virNetDevSetOnline(iface->ifname, false)); if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) ignore_value(virNetDevOpenvswitchRemovePort( virDomainNetGetActualBridgeName(iface), -- 1.7.11.2
From: "Daniel P. Berrange" <berrange@redhat.com> A mistaken initialization of 'ret' caused failure to create macvtap devices to be ignored. The libvirt_lxc process would later fail to start due to missing devices Also make sure code checks '< 0' and not '!= 0' since only -1 is considered an error condition Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_process.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 954cb9e..6cfbb0d 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -360,7 +360,7 @@ static int virLXCProcessSetupInterfaceDirect(virConnectPtr conn, unsigned int *nveths, char ***veths) { - int ret = 0; + int ret = -1; char *res_ifname = NULL; virLXCDriverPtr driver = conn->privateData; virNetDevBandwidthPtr bw; @@ -539,10 +539,10 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, } } - ret= 0; + ret = 0; cleanup: - if (ret != 0) { + if (ret < 0) { for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr iface = def->nets[i]; virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(iface); @@ -1046,7 +1046,7 @@ int virLXCProcessStart(virConnectPtr conn, } } - if (virLXCProcessSetupInterfaces(conn, vm->def, &nveths, &veths) != 0) + if (virLXCProcessSetupInterfaces(conn, vm->def, &nveths, &veths) < 0) goto cleanup; /* Save the configuration for the controller */ -- 1.7.11.2
From: "Daniel P. Berrange" <berrange@redhat.com> When starting a container, newDef is initialized to a copy of 'def', but when startup fails newDef is never removed. This cause later attempts to use 'virDomainDefine' to loose the new data being defined. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_process.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 6cfbb0d..28eecec 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1199,6 +1199,10 @@ cleanup: VIR_FREE(veths[i]); } if (rc != 0) { + if (vm->newDef) { + virDomainDefFree(vm->newDef); + vm->newDef = NULL; + } if (priv->monitor) { virObjectUnref(priv->monitor); priv->monitor = NULL; -- 1.7.11.2
----- Original Message -----
From: "Daniel P. Berrange" <berrange@redhat.com>
When starting a container, newDef is initialized to a copy of 'def', but when startup fails newDef is never removed. This cause later attempts to use 'virDomainDefine' to loose the new data being defined.
s/loose/lose/
From: "Daniel P. Berrange" <berrange@redhat.com> In virNetDevVethDelete the virRun method will properly report errors, but when checking the exit status for non-zero exit code no error is reported Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- po/POTFILES.in | 1 + src/util/virnetdevveth.c | 19 +++++++++---------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 1b7640c..32e7525 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -159,6 +159,7 @@ src/util/virnetdevbridge.c src/util/virnetdevmacvlan.c src/util/virnetdevopenvswitch.c src/util/virnetdevtap.c +src/util/virnetdevveth.c src/util/virnetdevvportprofile.c src/util/virnetlink.c src/util/virnodesuspend.c diff --git a/src/util/virnetdevveth.c b/src/util/virnetdevveth.c index 7414a14..e6ccdd1 100644 --- a/src/util/virnetdevveth.c +++ b/src/util/virnetdevveth.c @@ -170,16 +170,15 @@ int virNetDevVethDelete(const char *veth) rc = virRun(argv, &cmdResult); - if (rc != 0 || - (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) { - /* - * Prevent overwriting an error log which may be set - * where an actual failure occurs. - */ - VIR_DEBUG("Failed to delete '%s' (%d)", - veth, WEXITSTATUS(cmdResult)); - rc = -1; + if (rc != 0) + return -1; + + if (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to delete '%s' (exit status %d)"), + veth, WEXITSTATUS(cmdResult)); + return -1; } - return rc; + return 0; } -- 1.7.11.2
----- Original Message -----
From: "Daniel P. Berrange" <berrange@redhat.com>
In virNetDevVethDelete the virRun method will properly report errors, but when checking the exit status for non-zero exit code no error is reported
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- po/POTFILES.in | 1 + src/util/virnetdevveth.c | 19 +++++++++---------- 2 files changed, 10 insertions(+), 10 deletions(-)
I think this is overkill.
rc = virRun(argv, &cmdResult);
- if (rc != 0 || - (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
The old code wants cmdResult to be exactly 0. But guess what - if you pass NULL to virRun, then virRun errors out unless the exit status is exactly 0. That is it is better to just do: rc = virRun(argv, NULL); and delete the rest of this attempt at warning about non-zero status, as virRun does a better job of it in the first place.
On Tue, Nov 27, 2012 at 12:46:44PM -0500, Eric Blake wrote:
----- Original Message -----
From: "Daniel P. Berrange" <berrange@redhat.com>
In virNetDevVethDelete the virRun method will properly report errors, but when checking the exit status for non-zero exit code no error is reported
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- po/POTFILES.in | 1 + src/util/virnetdevveth.c | 19 +++++++++---------- 2 files changed, 10 insertions(+), 10 deletions(-)
I think this is overkill.
rc = virRun(argv, &cmdResult);
- if (rc != 0 || - (WIFEXITED(cmdResult) && WEXITSTATUS(cmdResult) != 0)) {
The old code wants cmdResult to be exactly 0. But guess what - if you pass NULL to virRun, then virRun errors out unless the exit status is exactly 0.
That is it is better to just do:
rc = virRun(argv, NULL);
and delete the rest of this attempt at warning about non-zero status, as virRun does a better job of it in the first place.
Hmm, good point. I'll delete all this code 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 :|
From: "Daniel P. Berrange" <berrange@redhat.com> When starting an LXC guest with a virNetwork based NIC device, if the network was not active, the virNetworkPtr device would be leaked Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/lxc/lxc_process.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 28eecec..514ef81 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -468,7 +468,6 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn, virReportError(VIR_ERR_INTERNAL_ERROR, _("Network '%s' is not active."), def->nets[i]->data.network.name); - goto cleanup; } if (!fail) { -- 1.7.11.2
On Tue, Nov 27, 2012 at 12:44:43PM -0500, Eric Blake wrote:
----- Original Message -----
The following is a list of random bug fixes I've identified while working on LXC
ACK 1 through 8, and 10. Nit in the commit message for 8/10. See my proposal for an alternate fix for 9.
This series is pushed with the change you mention 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 :|
participants (2)
-
Daniel P. Berrange -
Eric Blake