[libvirt] [PATCH 1/2] Implementation deficiency in virInitctlSetRunLevel v3
Implement virProcessRunInMountNamespace, which runs callback of type virProcessNamespaceCallback in a container namespace. --- src/libvirt_private.syms | 1 + src/util/virprocess.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virprocess.h | 6 +++++ 3 files changed, 70 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2dbb8f8..3f4b350 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1646,6 +1646,7 @@ virProcessGetNamespaces; virProcessGetStartTime; virProcessKill; virProcessKillPainfully; +virProcessRunInMountNamespace virProcessSetAffinity; virProcessSetMaxFiles; virProcessSetMaxMemLock; diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 9fc3207..2e8535e 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -31,6 +31,7 @@ # include <sys/resource.h> #endif #include <sched.h> +#include <stdlib.h> #ifdef __FreeBSD__ # include <sys/param.h> @@ -847,3 +848,65 @@ int virProcessGetStartTime(pid_t pid, return 0; } #endif + +#ifdef HAVE_SETNS +int virProcessRunInMountNamespace(pid_t pid, + virProcessNamespaceCallback cb, + void *opaque) +{ + char* path = NULL; + int ret = -1; + int cpid = -1; + int status = -1; + int fd = -1; + + if (virAsprintf(&path, "/proc/%llu/ns/mnt", + (unsigned long long)pid) < 0) { + goto cleanup; + } + + if ((fd = open(path, O_RDONLY)) < 0) { + virReportSystemError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Kernel does not provide mount namespace")); + goto cleanup; + } + + switch (cpid = fork()) { + case 0: + if (setns(fd, 0) == -1) { + exit(-1); + } + + ret = cb(pid, opaque); + exit(ret); + break; + case -1: + virReportSystemError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Fork failed")); + goto cleanup; + default: + if (waitpid(cpid, &status, 0) < 0 || status < 0) { + virReportSystemError(errno, + _("Callback failed with status %i"), + status); + ret = 1; + } else { + ret = 0; + } + } + +cleanup: + VIR_FREE(path); + VIR_FORCE_CLOSE(fd); + return ret; +} +#else +int virProcessRunInMountNamespace(pid_t pid ATTRIBUTE_UNUSED, + virProcessNamespaceCallback cb ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Mount namespaces are not available on this platform")); + return -1; +} +#endif diff --git a/src/util/virprocess.h b/src/util/virprocess.h index 9f77bc5..205abf7 100644 --- a/src/util/virprocess.h +++ b/src/util/virprocess.h @@ -60,4 +60,10 @@ int virProcessSetNamespaces(size_t nfdlist, int virProcessSetMaxMemLock(pid_t pid, unsigned long long bytes); int virProcessSetMaxProcesses(pid_t pid, unsigned int procs); int virProcessSetMaxFiles(pid_t pid, unsigned int files); + +typedef int (*virProcessNamespaceCallback)(pid_t pid, void *opaque); + +int virProcessRunInMountNamespace(pid_t pid, + virProcessNamespaceCallback cb, + void *opaque); #endif /* __VIR_PROCESS_H__ */ -- 1.7.10.4
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..7f4acbe 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, + (virProcessNamespaceCallback) 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, + (virProcessNamespaceCallback) 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 07:42:28PM +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..7f4acbe 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)
Nitpick - the '*' should associate with 'opaque' rather than 'void'
+{ + 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, + (virProcessNamespaceCallback) virDomainShutdownCallback,
I think you can leave out the (virProcessNamespaceCallback) cast here, since the function prototype already matches what is required.
+ 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)
Same nitpick
+{ + 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, + (virProcessNamespaceCallback) virDomainRebootCallback,
And here
+ NULL); + if (rc < 0) { goto cleanup; } if (rc == 0 && flags != 0 &&
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 :|
On Fri, Dec 20, 2013 at 07:39:21PM +0400, Reco wrote:
diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 9fc3207..2e8535e 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -31,6 +31,7 @@ # include <sys/resource.h> #endif #include <sched.h> +#include <stdlib.h>
#ifdef __FreeBSD__ # include <sys/param.h> @@ -847,3 +848,65 @@ int virProcessGetStartTime(pid_t pid, return 0; } #endif + +#ifdef HAVE_SETNS +int virProcessRunInMountNamespace(pid_t pid, + virProcessNamespaceCallback cb, + void *opaque) +{ + char* path = NULL; + int ret = -1; + int cpid = -1; + int status = -1; + int fd = -1; + + if (virAsprintf(&path, "/proc/%llu/ns/mnt", + (unsigned long long)pid) < 0) { + goto cleanup; + } + + if ((fd = open(path, O_RDONLY)) < 0) { + virReportSystemError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
The virReportSystemError method wants to be given 'errno' value here.
+ _("Kernel does not provide mount namespace")); + goto cleanup; + } + + switch (cpid = fork()) { + case 0: + if (setns(fd, 0) == -1) { + exit(-1);
I think we need _exit instead of exit
+ } + + ret = cb(pid, opaque); + exit(ret);
And again here.
+ break; + case -1: + virReportSystemError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Fork failed")); + goto cleanup; + default: + if (waitpid(cpid, &status, 0) < 0 || status < 0) { + virReportSystemError(errno, + _("Callback failed with status %i"), + status); + ret = 1; + } else { + ret = 0; + }
You'll want to use virProcessWait(cpiud, &status) here instead since to avoid tedious waitpid calling conventions
+ } + +cleanup: + VIR_FREE(path); + VIR_FORCE_CLOSE(fd); + return ret; +} +#else +int virProcessRunInMountNamespace(pid_t pid ATTRIBUTE_UNUSED, + virProcessNamespaceCallback cb ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + virReportSystemError(ENOSYS, "%s", + _("Mount namespaces are not available on this platform")); + return -1; +} +#endif
Looks good aside from those small tweaks. BTW, thanks very much for taking the time to write these patches for us, as well as reporting the issue in the first place ! 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 :|
participants (2)
-
Daniel P. Berrange -
Reco