[libvirt] [PATCH 2/2] Implementation deficiency in virInitctlSetRunLevel v4
Use helper virProcessRunInMountNamespace in lxcDomainShutdownFlags and lxcDomainReboot. --- src/lxc/lxc_driver.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e5298d1..2385f5b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2694,12 +2694,21 @@ lxcConnectListAllDomains(virConnectPtr conn, static int +virDomainShutdownCallback(pid_t pid ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + int rc; + rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, NULL); + return rc; +} + + +static int lxcDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { virLXCDomainObjPrivatePtr priv; virDomainObjPtr vm; - char *vroot = NULL; int ret = -1; int rc; @@ -2726,14 +2735,12 @@ lxcDomainShutdownFlags(virDomainPtr dom, goto cleanup; } - if (virAsprintf(&vroot, "/proc/%llu/root", - (unsigned long long)priv->initpid) < 0) - goto cleanup; - if (flags == 0 || (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) { - if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, - vroot)) < 0) { + rc = virProcessRunInMountNamespace(priv->initpid, + virDomainShutdownCallback, + NULL); + if (rc < 0) { goto cleanup; } if (rc == 0 && flags != 0 && @@ -2761,7 +2768,6 @@ lxcDomainShutdownFlags(virDomainPtr dom, ret = 0; cleanup: - VIR_FREE(vroot); if (vm) virObjectUnlock(vm); return ret; @@ -2773,13 +2779,22 @@ lxcDomainShutdown(virDomainPtr dom) return lxcDomainShutdownFlags(dom, 0); } + +virDomainRebootCallback(pid_t pid ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + int rc; + rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, NULL); + return rc; +} + + static int lxcDomainReboot(virDomainPtr dom, unsigned int flags) { virLXCDomainObjPrivatePtr priv; virDomainObjPtr vm; - char *vroot = NULL; int ret = -1; int rc; @@ -2806,14 +2821,12 @@ lxcDomainReboot(virDomainPtr dom, goto cleanup; } - if (virAsprintf(&vroot, "/proc/%llu/root", - (unsigned long long)priv->initpid) < 0) - goto cleanup; - if (flags == 0 || (flags & VIR_DOMAIN_REBOOT_INITCTL)) { - if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, - vroot)) < 0) { + rc = virProcessRunInMountNamespace(priv->initpid, + virDomainRebootCallback, + NULL); + if (rc < 0) { goto cleanup; } if (rc == 0 && flags != 0 && @@ -2841,7 +2854,6 @@ lxcDomainReboot(virDomainPtr dom, ret = 0; cleanup: - VIR_FREE(vroot); if (vm) virObjectUnlock(vm); return ret; -- 1.7.10.4
On Fri, Dec 20, 2013 at 08:24:52PM +0400, Reco wrote:
Use helper virProcessRunInMountNamespace in lxcDomainShutdownFlags and lxcDomainReboot.
--- src/lxc/lxc_driver.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index e5298d1..2385f5b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -2694,12 +2694,21 @@ lxcConnectListAllDomains(virConnectPtr conn,
static int +virDomainShutdownCallback(pid_t pid ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + int rc; + rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, NULL); + return rc; +} + + +static int lxcDomainShutdownFlags(virDomainPtr dom, unsigned int flags) { virLXCDomainObjPrivatePtr priv; virDomainObjPtr vm; - char *vroot = NULL; int ret = -1; int rc;
@@ -2726,14 +2735,12 @@ lxcDomainShutdownFlags(virDomainPtr dom, goto cleanup; }
- if (virAsprintf(&vroot, "/proc/%llu/root", - (unsigned long long)priv->initpid) < 0) - goto cleanup; - if (flags == 0 || (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) { - if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, - vroot)) < 0) { + rc = virProcessRunInMountNamespace(priv->initpid, + virDomainShutdownCallback, + NULL); + if (rc < 0) { goto cleanup; } if (rc == 0 && flags != 0 && @@ -2761,7 +2768,6 @@ lxcDomainShutdownFlags(virDomainPtr dom, ret = 0;
cleanup: - VIR_FREE(vroot); if (vm) virObjectUnlock(vm); return ret; @@ -2773,13 +2779,22 @@ lxcDomainShutdown(virDomainPtr dom) return lxcDomainShutdownFlags(dom, 0); }
+ +virDomainRebootCallback(pid_t pid ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + int rc; + rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, NULL); + return rc; +} + + static int lxcDomainReboot(virDomainPtr dom, unsigned int flags) { virLXCDomainObjPrivatePtr priv; virDomainObjPtr vm; - char *vroot = NULL; int ret = -1; int rc;
@@ -2806,14 +2821,12 @@ lxcDomainReboot(virDomainPtr dom, goto cleanup; }
- if (virAsprintf(&vroot, "/proc/%llu/root", - (unsigned long long)priv->initpid) < 0) - goto cleanup; - if (flags == 0 || (flags & VIR_DOMAIN_REBOOT_INITCTL)) { - if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, - vroot)) < 0) { + rc = virProcessRunInMountNamespace(priv->initpid, + virDomainRebootCallback, + NULL); + if (rc < 0) { goto cleanup; } if (rc == 0 && flags != 0 && @@ -2841,7 +2854,6 @@ lxcDomainReboot(virDomainPtr dom, ret = 0;
cleanup: - VIR_FREE(vroot); if (vm) virObjectUnlock(vm); return ret;
ACK 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 :|
On 12/20/2013 09:24 AM, Reco wrote:
Use helper virProcessRunInMountNamespace in lxcDomainShutdownFlags and lxcDomainReboot.
This fails to compile, due to [1] below: CCLD libvirt_lxc lxc/lxc_driver.c:2783:1: error: return type defaults to 'int' [-Werror] virDomainRebootCallback(pid_t pid ATTRIBUTE_UNUSED, ^ lxc/lxc_driver.c:2783:1: error: no previous prototype for 'virDomainRebootCallback' [-Werror=missing-prototypes] cc1: all warnings being treated as errors
--- src/lxc/lxc_driver.c | 44 ++++++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 16 deletions(-)
static int +virDomainShutdownCallback(pid_t pid ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + int rc; + rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, NULL); + return rc;
Hmm. The callback's return value being used directly as the _exit() value in patch 1 is not a good idea; either this function must convert -1 to EXIT_FAILURE, or we should fix the framework to turn our normal -1 conventions into the correct exit code (I'm opting for the latter). Also, you are now invoking code in between the fork/exec boundary off of a multithreaded parent, which means we must now audit that all calls in this code path are async-signal-safe (if not, the child will deadlock if the fork happened in the parent while some other thread held a lock that the child wants to obtain). Alas, virInitctlSetRunLevel() calls virAsprintf() calls malloc(), which is not async safe :( [we've gotten away with it in the past; LXC is a Linux-only technology, and glibc's malloc is relatively robust enough that I've never seen it fail multithreaded fork/exec setups, whereas I HAVE seen malloc deadlocks on other systems like Solaris]. It also calls virReportSystemError() on failure, but in a child process, this may not get logged the same was as an error message in the parent. More robust would be having the child be silent and just use distinct error codes, then let the parent convert error codes into actual error messages to be logged. But I think we can wait until an actual report of a deadlock before going to that effort. Aha - you are passing NULL for vroot, which means we could get rid of the malloc() issue. In fact, we could get rid of the vroot argument altogether, since all callers now pass NULL, and since we don't want to be tempted to use the unsafe construct.
if (flags == 0 || (flags & VIR_DOMAIN_SHUTDOWN_INITCTL)) { - if ((rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, - vroot)) < 0) { + rc = virProcessRunInMountNamespace(priv->initpid, + virDomainShutdownCallback, + NULL);
We could use the opaque argument to pass our desired runlevel constant... [2]
+ +virDomainRebootCallback(pid_t pid ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED)
[1] you missed the 'static int' line here.
+{ + int rc; + rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_REBOOT, NULL); + return rc; +}
[2] ... and have only one static callback helper instead of 2, since they differ only by the constant used. Good - I have enough cleanup to do that I can feel good about claiming authorship of the final version (thus bypassing the full name question) while still giving you credit for the find and initial attempts at the fix. v5 coming up soon! -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 12/20/2013 01:41 PM, Eric Blake wrote:
On 12/20/2013 09:24 AM, Reco wrote:
Use helper virProcessRunInMountNamespace in lxcDomainShutdownFlags and lxcDomainReboot.
static int +virDomainShutdownCallback(pid_t pid ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + int rc; + rc = virInitctlSetRunLevel(VIR_INITCTL_RUNLEVEL_POWEROFF, NULL); + return rc;
Hmm. The callback's return value being used directly as the _exit() value in patch 1 is not a good idea; either this function must convert -1 to EXIT_FAILURE, or we should fix the framework to turn our normal -1 conventions into the correct exit code (I'm opting for the latter).
Eww, it's even worse than that. virInitctlSetRunLevel() is documented as returning -1 on failure, 0 if initctl doesn't exist, and 1 on success; but virProcessRunInMountNamespace() was declaring failure on all but a return status of 0 (which only happens when initctl doesn't exist), which means failure and success both get lumped into the callback mechanism declaring failure. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Reco