10:24 a.m.
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
10:35 a.m.
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 :|
2:41 p.m.
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
2:59 p.m.
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
3962
days inactive
3962
days old
3 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Reco