9:52 p.m.
This addresses the comments raised during v4:
https://www.redhat.com/archives/libvir-list/2011-March/msg00421.html
More comments in individual patches.
It could still use a bit more testing with root-squash NFS, and I'm
also hitting a problem where if I run daemon/libvirtd myself, I
get a SELinux error:
error: unable to set security context
'system_u:object_r:svirt_image_t:s0:c80,c237' on fd 23: Permission denied
but if I run the system service libvirtd or SELinux permissive, things
work. Somehow, the attempt to set the fd SELinux label on a pipe is
not working when libvirt is started as an unconfined process (that is,
the fd has label
unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023) but when
started as a daemon, SELinux is happy to allow the transition. I
suspect that this is a bug in SELinux, since my understanding is that
it should always be possible to go from unconfined to something more
restrictive, but we already proved that SELinux fd labelling is
relatively unused and untested back when we first added it in commit
34a19dda.
If possible, I'd like to get this in before the 0.9.0 freeze, and we
can fix any fallout from testing during the freeze week.
Eric Blake (13):
util: allow clearing cloexec bit
qemu: fix restoring a compressed save image
qemu: allow simple domain save to use fd: protocol
util: use SCM_RIGHTS in virFileOperation when needed
qemu: simplify domain save fd handling
storage: simplify fd handling
util: rename virFileOperation to virFileOpenAs
util: adjust indentation in previous patch
qemu, storage: improve type safety
qemu: use common API for reading difficult files
qemu: consolidate migration to file code
qemu: skip granting access during fd migration
qemu: support fd: migration with compression
src/libvirt_private.syms | 3 +-
src/qemu/qemu_command.c | 16 ++
src/qemu/qemu_driver.c | 500 +++++++++--------------------------------
src/qemu/qemu_migration.c | 139 ++++++++++++
src/qemu/qemu_migration.h | 8 +
src/storage/storage_backend.c | 78 ++++---
src/util/util.c | 176 ++++++++++-----
src/util/util.h | 16 +-
tests/qemuxml2argvtest.c | 2 +-
9 files changed, 448 insertions(+), 490 deletions(-)
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 01/13] util: allow clearing cloexec bit
* src/util/util.h (virSetInherit): New prototype.
* src/util/util.c (virSetCloseExec): Move guts...
(virSetInherit): ...to new function, and allow clearing.
* src/libvirt_private.syms (util.h): Export it.
---
v5: new patch
src/libvirt_private.syms | 1 +
src/util/util.c | 14 +++++++++++---
src/util/util.h | 1 +
3 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5f58970..93504e5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -933,6 +933,7 @@ virRun;
virRunWithHook;
virSetBlocking;
virSetCloseExec;
+virSetInherit;
virSetNonBlock;
virSetUIDGID;
virSkipSpaces;
diff --git a/src/util/util.c b/src/util/util.c
index 4301b00..7c7da22 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -273,13 +273,21 @@ int virSetNonBlock(int fd) {
}
+int virSetCloseExec(int fd)
+{
+ return virSetInherit(fd, false);
+}
+
#ifndef WIN32
-int virSetCloseExec(int fd) {
+int virSetInherit(int fd, bool inherit) {
int flags;
if ((flags = fcntl(fd, F_GETFD)) < 0)
return -1;
- flags |= FD_CLOEXEC;
+ if (inherit)
+ flags &= ~FD_CLOEXEC;
+ else
+ flags |= FD_CLOEXEC;
if ((fcntl(fd, F_SETFD, flags)) < 0)
return -1;
return 0;
@@ -931,7 +939,7 @@ virRunWithHook(const char *const*argv,
#else /* WIN32 */
-int virSetCloseExec(int fd ATTRIBUTE_UNUSED)
+int virSetInherit(int fd ATTRIBUTE_UNUSED, bool inherit ATTRIBUTE_UNUSED)
{
return -1;
}
diff --git a/src/util/util.h b/src/util/util.h
index c313023..0f11f8f 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -51,6 +51,7 @@ enum {
int virSetBlocking(int fd, bool blocking) ATTRIBUTE_RETURN_CHECK;
int virSetNonBlock(int fd) ATTRIBUTE_RETURN_CHECK;
+int virSetInherit(int fd, bool inherit) ATTRIBUTE_RETURN_CHECK;
int virSetCloseExec(int fd) ATTRIBUTE_RETURN_CHECK;
/* This will execute in the context of the first child
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 02/13] qemu: fix restoring a compressed save image
Latent bug introduced in commit 2d6a581960 (Aug 2009), but not exposed
until commit 1859939a (Jan 2011). Basically, when virExec creates a
pipe, it always marks libvirt's side as cloexec. If libvirt then
wants to hand that pipe to another child process, things work great if
the fd is dup2()'d onto stdin or stdout (as with stdin: or exec:
migration), but if the pipe is instead used as-is (such as with fd:
migration) then qemu sees EBADF because the fd was closed at exec().
This is a minimal fix for the problem at hand; it is slightly racy,
but no more racy than the rest of libvirt fd handling, including the
case of uncompressed save images. A more invasive fix, but ultimately
safer at avoiding leaking unintended fds, would be to _always and
atomically_ open all fds as cloexec in libvirt (thanks to primitives
like open(O_CLOEXEC), pipe2(), accept4(), ...), then teach virExec to
clear that bit for all fds explicitly marked to be handed to the child
only after forking.
* src/qemu/qemu_command.c (qemuBuildCommandLine): Clear cloexec
flag.
* tests/qemuxml2argvtest.c (testCompareXMLToArgvFiles): Tweak test.
---
v5: new patch
src/qemu/qemu_command.c | 16 ++++++++++++++++
tests/qemuxml2argvtest.c | 2 +-
2 files changed, 17 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index c9b9850..3138943 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4329,6 +4329,14 @@ qemuBuildCommandLine(virConnectPtr conn,
} else if (STREQ(migrateFrom, "stdio")) {
if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) {
virCommandAddArgFormat(cmd, "fd:%d", migrateFd);
+ /* migrateFd might be cloexec, but qemu must inherit
+ * it if vmop indicates qemu will be executed */
+ if (vmop != VIR_VM_OP_NO_OP &&
+ virSetInherit(migrateFd, true) < 0) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to clear cloexec flag"));
+ goto error;
+ }
virCommandPreserveFD(cmd, migrateFd);
} else if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_EXEC)) {
virCommandAddArg(cmd, "exec:cat");
@@ -4358,6 +4366,14 @@ qemuBuildCommandLine(virConnectPtr conn,
goto error;
}
virCommandAddArg(cmd, migrateFrom);
+ /* migrateFd might be cloexec, but qemu must inherit
+ * it if vmop indicates qemu will be executed */
+ if (vmop != VIR_VM_OP_NO_OP &&
+ virSetInherit(migrateFd, true) < 0) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Failed to clear cloexec flag"));
+ goto error;
+ }
virCommandPreserveFD(cmd, migrateFd);
} else if (STRPREFIX(migrateFrom, "unix")) {
if (!qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_UNIX)) {
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index c1329fa..02de8de 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -122,7 +122,7 @@ static int testCompareXMLToArgvFiles(const char *xml,
if (!(cmd = qemuBuildCommandLine(conn, &driver,
vmdef, &monitor_chr, false, extraFlags,
migrateFrom, migrateFd, NULL,
- VIR_VM_OP_CREATE)))
+ VIR_VM_OP_NO_OP)))
goto fail;
if (!!virGetLastError() != expectError) {
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 03/13] qemu: allow simple domain save to use fd: protocol
This allows direct saves (no compression, no root-squash NFS) to use
the more efficient fd: migration, which in turn avoids a race where
qemu exec: migration can sometimes fail because qemu does a generic
waitpid() that conflicts with the pclose() used by exec:. Further
patches will solve compression and root-squash NFS.
* src/qemu/qemu_driver.c (qemudDomainSaveFlag): Use new function
when there is no compression.
---
v5: adjust earlier in the series
Note that this opens the fd a second time, but gracefully falls back
to the older exec: migration for things like root-squash NFS.
src/qemu/qemu_driver.c | 26 +++++++++++++++++++++++---
1 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e0e86d3..8aa72f8 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1823,6 +1823,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
int is_reg = 0;
unsigned long long offset;
virCgroupPtr cgroup = NULL;
+ virBitmapPtr qemuCaps = NULL;
memset(&header, 0, sizeof(header));
memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic));
@@ -1853,6 +1854,11 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
}
}
+ if (qemuCapsExtractVersionInfo(vm->def->emulator, vm->def->os.arch,
+ NULL,
+ &qemuCaps) < 0)
+ goto endjob;
+
/* Get XML for the domain */
xml = virDomainDefFormat(vm->def, VIR_DOMAIN_XML_SECURE);
if (!xml) {
@@ -2013,10 +2019,23 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
if (header.compressed == QEMUD_SAVE_FORMAT_RAW) {
const char *args[] = { "cat", NULL };
+ /* XXX gross - why don't we reuse the fd already opened earlier */
+ int fd = -1;
+
+ if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) &&
+ priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX)
+ fd = open(path, O_WRONLY);
qemuDomainObjEnterMonitorWithDriver(driver, vm);
- rc = qemuMonitorMigrateToFile(priv->mon,
- QEMU_MONITOR_MIGRATE_BACKGROUND,
- args, path, offset);
+ if (fd >= 0 && lseek(fd, offset, SEEK_SET) == offset) {
+ rc = qemuMonitorMigrateToFd(priv->mon,
+ QEMU_MONITOR_MIGRATE_BACKGROUND,
+ fd);
+ } else {
+ rc = qemuMonitorMigrateToFile(priv->mon,
+ QEMU_MONITOR_MIGRATE_BACKGROUND,
+ args, path, offset);
+ }
+ VIR_FORCE_CLOSE(fd);
qemuDomainObjExitMonitorWithDriver(driver, vm);
} else {
const char *prog = qemudSaveCompressionTypeToString(header.compressed);
@@ -2099,6 +2118,7 @@ endjob:
}
cleanup:
+ qemuCapsFree(qemuCaps);
VIR_FREE(xml);
if (ret != 0 && is_reg)
unlink(path);
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 04/13] util: use SCM_RIGHTS in virFileOperation when needed
Currently, the hook function in virFileOperation is extremely limited:
it must be async-signal-safe, and cannot modify any memory in the
parent process. It is much handier to return a valid fd and operate
on it in the parent than to deal with hook restrictions.
* src/util/util.h (VIR_FILE_OP_RETURN_FD): New flag.
* src/util/util.c (virFileOperationNoFork, virFileOperation):
Honor new flag.
---
v5: incorporate comments, such as closing fds sooner and switching
to SOCK_STREAM to avoid hangs
src/util/util.c | 138 ++++++++++++++++++++++++++++++++++++++++++++++++------
src/util/util.h | 4 +-
2 files changed, 125 insertions(+), 17 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 7c7da22..a0a331d 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1428,14 +1428,15 @@ static int virFileOperationNoFork(const char *path, int openflags,
mode_t mode,
if ((hook) && ((ret = hook(fd, hookdata)) != 0)) {
goto error;
}
+ if (flags & VIR_FILE_OP_RETURN_FD)
+ return fd;
if (VIR_CLOSE(fd) < 0) {
ret = -errno;
virReportSystemError(errno, _("failed to close new file
'%s'"),
path);
- fd = -1;
goto error;
}
- fd = -1;
+
error:
VIR_FORCE_CLOSE(fd);
return ret;
@@ -1480,7 +1481,32 @@ error:
return ret;
}
-/* return -errno on failure, or 0 on success */
+/**
+ * virFileOperation:
+ * @path: file to open or create
+ * @openflags: flags to pass to open
+ * @mode: mode to use on creation or when forcing permissions
+ * @uid: uid that should own file on creation
+ * @gid: gid that should own file
+ * @hook: callback to run once file is open; see below for restrictions
+ * @hookdata: opaque data for @hook
+ * @flags: bit-wise or of VIR_FILE_OP_* flags
+ *
+ * Open @path, and execute an optional callback @hook on the open file
+ * description. @hook must return 0 on success, or -errno on failure.
+ * If @flags includes VIR_FILE_OP_AS_UID, then open the file while the
+ * effective user id is @uid (by using a child process); this allows
+ * one to bypass root-squashing NFS issues. If @flags includes
+ * VIR_FILE_OP_FORCE_PERMS, then ensure that @path has those
+ * permissions before the callback, even if the file already existed
+ * with different permissions. If @flags includes
+ * VIR_FILE_OP_RETURN_FD, then use SCM_RIGHTS to guarantee that any
+ * @hook is run in the parent, and the return value (if non-negative)
+ * is the file descriptor, left open. Otherwise, @hook might be run
+ * in a child process, so it must be async-signal-safe (ie. no
+ * malloc() or anything else that depends on modifying locks held in
+ * the parent), no file descriptor remains open, and 0 is returned on
+ * success. Return -errno on failure. */
int virFileOperation(const char *path, int openflags, mode_t mode,
uid_t uid, gid_t gid,
virFileOperationHook hook, void *hookdata,
@@ -1488,7 +1514,14 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
struct stat st;
pid_t pid;
int waitret, status, ret = 0;
- int fd;
+ int fd = -1;
+ int pair[2] = { -1, -1 };
+ struct msghdr msg;
+ struct cmsghdr *cmsg;
+ char buf[CMSG_SPACE(sizeof(fd))];
+ char dummy = 0;
+ struct iovec iov;
+ int forkRet;
if ((!(flags & VIR_FILE_OP_AS_UID))
|| (getuid() != 0)
@@ -1502,7 +1535,29 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
* following dance avoids problems caused by root-squashing
* NFS servers. */
- int forkRet = virFork(&pid);
+ if (flags & VIR_FILE_OP_RETURN_FD) {
+ if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) {
+ ret = -errno;
+ virReportSystemError(errno,
+ _("failed to create socket needed for
'%s'"),
+ path);
+ return ret;
+ }
+
+ memset(&msg, 0, sizeof(msg));
+ iov.iov_base = &dummy;
+ iov.iov_len = 1;
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
+ }
+
+ forkRet = virFork(&pid);
if (pid < 0) {
ret = -errno;
@@ -1510,6 +1565,31 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
}
if (pid) { /* parent */
+ if (flags & VIR_FILE_OP_RETURN_FD) {
+ VIR_FORCE_CLOSE(pair[1]);
+
+ do {
+ ret = recvmsg(pair[0], &msg, 0);
+ } while (ret < 0 && errno == EINTR);
+
+ if (ret < 0) {
+ ret = -errno;
+ VIR_FORCE_CLOSE(pair[0]);
+ while ((waitret = waitpid(pid, NULL, 0) == -1)
+ && (errno == EINTR));
+ goto parenterror;
+ }
+ VIR_FORCE_CLOSE(pair[0]);
+
+ /* See if fd was transferred. */
+ cmsg = CMSG_FIRSTHDR(&msg);
+ if (cmsg && cmsg->cmsg_len == CMSG_LEN(sizeof(fd)) &&
+ cmsg->cmsg_level == SOL_SOCKET &&
+ cmsg->cmsg_type == SCM_RIGHTS) {
+ memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd));
+ }
+ }
+
/* wait for child to complete, and retrieve its exit code */
while ((waitret = waitpid(pid, &status, 0) == -1)
&& (errno == EINTR));
@@ -1520,12 +1600,20 @@ int virFileOperation(const char *path, int openflags, mode_t
mode,
path);
goto parenterror;
}
- if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES) {
+ if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES ||
+ ((flags & VIR_FILE_OP_RETURN_FD) && fd == -1)) {
/* fall back to the simpler method, which works better in
* some cases */
return virFileOperationNoFork(path, openflags, mode, uid, gid,
hook, hookdata, flags);
}
+ if (!ret && (flags & VIR_FILE_OP_RETURN_FD)) {
+ if ((hook) && ((ret = hook(fd, hookdata)) != 0)) {
+ VIR_FORCE_CLOSE(fd);
+ goto parenterror;
+ }
+ ret = fd;
+ }
parenterror:
return ret;
}
@@ -1535,6 +1623,7 @@ parenterror:
if (forkRet < 0) {
/* error encountered and logged in virFork() after the fork. */
+ ret = -errno;
goto childerror;
}
@@ -1584,15 +1673,32 @@ parenterror:
path, mode);
goto childerror;
}
- if ((hook) && ((ret = hook(fd, hookdata)) != 0)) {
- goto childerror;
- }
- if (VIR_CLOSE(fd) < 0) {
- ret = -errno;
- virReportSystemError(errno, _("child failed to close new file
'%s'"),
- path);
- goto childerror;
+ if (flags & VIR_FILE_OP_RETURN_FD) {
+ VIR_FORCE_CLOSE(pair[0]);
+ memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
+
+ do {
+ ret = sendmsg(pair[1], &msg, 0);
+ } while (ret < 0 && errno == EINTR);
+
+ if (ret < 0) {
+ ret = -errno;
+ goto childerror;
+ }
+ ret = 0;
+ } else {
+ if ((hook) && ((ret = hook(fd, hookdata)) != 0))
+ goto childerror;
+ if (VIR_CLOSE(fd) < 0) {
+ ret = -errno;
+ virReportSystemError(errno,
+ _("child failed to close new file
'%s'"),
+ path);
+ goto childerror;
+ }
}
+
+ ret = 0;
childerror:
/* ret tracks -errno on failure, but exit value must be positive.
* If the child exits with EACCES, then the parent tries again. */
@@ -1719,7 +1825,7 @@ int virFileOperation(const char *path ATTRIBUTE_UNUSED,
virUtilError(VIR_ERR_INTERNAL_ERROR,
"%s", _("virFileOperation is not implemented for
WIN32"));
- return -1;
+ return -ENOSYS;
}
int virDirCreate(const char *path ATTRIBUTE_UNUSED,
@@ -1731,7 +1837,7 @@ int virDirCreate(const char *path ATTRIBUTE_UNUSED,
virUtilError(VIR_ERR_INTERNAL_ERROR,
"%s", _("virDirCreate is not implemented for
WIN32"));
- return -1;
+ return -ENOSYS;
}
#endif /* WIN32 */
diff --git a/src/util/util.h b/src/util/util.h
index 0f11f8f..b1ca871 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -130,12 +130,14 @@ enum {
VIR_FILE_OP_NONE = 0,
VIR_FILE_OP_AS_UID = (1 << 0),
VIR_FILE_OP_FORCE_PERMS = (1 << 1),
+ VIR_FILE_OP_RETURN_FD = (1 << 2),
};
typedef int (*virFileOperationHook)(int fd, void *data);
int virFileOperation(const char *path, int openflags, mode_t mode,
uid_t uid, gid_t gid,
virFileOperationHook hook, void *hookdata,
- unsigned int flags) ATTRIBUTE_RETURN_CHECK;
+ unsigned int flags)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
enum {
VIR_DIR_CREATE_NONE = 0,
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 05/13] qemu: simplify domain save fd handling
This makes root-squash NFS saves more efficient.
* src/qemu/qemu_driver.c (qemudDomainSaveFlag): Use new
virFileOperation flag to open fd only once.
---
v5: no real changes
src/qemu/qemu_driver.c | 75 +++++++++++++++++++++++++-----------------------
1 files changed, 39 insertions(+), 36 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8aa72f8..7a6e4ff 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1824,6 +1824,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
unsigned long long offset;
virCgroupPtr cgroup = NULL;
virBitmapPtr qemuCaps = NULL;
+ int fd = -1;
memset(&header, 0, sizeof(header));
memcpy(header.magic, QEMUD_SAVE_MAGIC, sizeof(header.magic));
@@ -1902,45 +1903,32 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
header.xml_len += pad;
}
- /* Setup hook data needed by virFileOperation hook function */
- hdata.dom = dom;
- hdata.path = path;
- hdata.xml = xml;
- hdata.header = &header;
-
- /* Write header to file, followed by XML */
+ /* Obtain the file handle. */
/* First try creating the file as root */
if (!is_reg) {
- int fd = open(path, O_WRONLY | O_TRUNC);
+ fd = open(path, O_WRONLY | O_TRUNC);
if (fd < 0) {
virReportSystemError(errno, _("unable to open %s"), path);
goto endjob;
}
- if (qemudDomainSaveFileOpHook(fd, &hdata) < 0) {
- VIR_FORCE_CLOSE(fd);
- goto endjob;
- }
- if (VIR_CLOSE(fd) < 0) {
- virReportSystemError(errno, _("unable to close %s"), path);
- goto endjob;
- }
} else {
- if ((rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
- S_IRUSR|S_IWUSR,
- getuid(), getgid(),
- qemudDomainSaveFileOpHook, &hdata,
- 0)) < 0) {
+ if ((fd = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
+ S_IRUSR|S_IWUSR,
+ getuid(), getgid(),
+ NULL, NULL,
+ VIR_FILE_OP_RETURN_FD)) < 0) {
/* If we failed as root, and the error was permission-denied
(EACCES or EPERM), assume it's on a network-connected share
where root access is restricted (eg, root-squashed NFS). If the
qemu user (driver->user) is non-root, just set a flag to
bypass security driver shenanigans, and retry the operation
after doing setuid to qemu user */
-
+ rc = fd;
if (((rc != -EACCES) && (rc != -EPERM)) ||
driver->user == getuid()) {
- virReportSystemError(-rc, _("Failed to create domain save file
'%s'"),
+ virReportSystemError(-rc,
+ _("Failed to create domain save file
'%s'"),
path);
goto endjob;
}
@@ -1965,7 +1953,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
default:
/* local file - log the error returned by virFileOperation */
virReportSystemError(-rc,
- _("Failed to create domain save file
'%s'"),
+ _("Failed to create domain save file
'%s'"),
path);
goto endjob;
break;
@@ -1974,13 +1962,15 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
/* Retry creating the file as driver->user */
- if ((rc = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
+ if ((fd = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
driver->user, driver->group,
- qemudDomainSaveFileOpHook, &hdata,
- VIR_FILE_OP_AS_UID)) < 0) {
- virReportSystemError(-rc, _("Error from child process creating
'%s'"),
- path);
+ NULL, NULL,
+ (VIR_FILE_OP_AS_UID |
+ VIR_FILE_OP_RETURN_FD))) < 0) {
+ virReportSystemError(-fd,
+ _("Error from child process creating
'%s'"),
+ path);
goto endjob;
}
@@ -1992,6 +1982,18 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
}
}
+ /* Write header to file, followed by XML */
+ hdata.dom = dom;
+ hdata.path = path;
+ hdata.xml = xml;
+ hdata.header = &header;
+
+ if (qemudDomainSaveFileOpHook(fd, &hdata) < 0) {
+ VIR_FORCE_CLOSE(fd);
+ goto endjob;
+ }
+
+ /* Allow qemu to access file */
if (!is_reg &&
qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
@@ -2019,14 +2021,10 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
if (header.compressed == QEMUD_SAVE_FORMAT_RAW) {
const char *args[] = { "cat", NULL };
- /* XXX gross - why don't we reuse the fd already opened earlier */
- int fd = -1;
- if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) &&
- priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX)
- fd = open(path, O_WRONLY);
qemuDomainObjEnterMonitorWithDriver(driver, vm);
- if (fd >= 0 && lseek(fd, offset, SEEK_SET) == offset) {
+ if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) &&
+ priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
rc = qemuMonitorMigrateToFd(priv->mon,
QEMU_MONITOR_MIGRATE_BACKGROUND,
fd);
@@ -2035,7 +2033,6 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
QEMU_MONITOR_MIGRATE_BACKGROUND,
args, path, offset);
}
- VIR_FORCE_CLOSE(fd);
qemuDomainObjExitMonitorWithDriver(driver, vm);
} else {
const char *prog = qemudSaveCompressionTypeToString(header.compressed);
@@ -2054,6 +2051,11 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
if (rc < 0)
goto endjob;
+ if (VIR_CLOSE(fd) < 0) {
+ virReportSystemError(errno, _("unable to close %s"), path);
+ goto endjob;
+ }
+
rc = qemuMigrationWaitForCompletion(driver, vm);
if (rc < 0)
@@ -2119,6 +2121,7 @@ endjob:
cleanup:
qemuCapsFree(qemuCaps);
+ VIR_FORCE_CLOSE(fd);
VIR_FREE(xml);
if (ret != 0 && is_reg)
unlink(path);
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 06/13] storage: simplify fd handling
* src/storage/storage_backend.c (virStorageBackendCreateRaw): Use
new virFileOperation flag.
---
v5: no real changes
src/storage/storage_backend.c | 46 +++++++++++++++++++++++++---------------
1 files changed, 29 insertions(+), 17 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index c6c16c8..b0a512c 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -370,11 +370,16 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
- unsigned int flags ATTRIBUTE_UNUSED)
+ unsigned int flags)
{
int ret = -1;
- int createstat;
+ int fd = -1;
struct createRawFileOpHookData hdata = { vol, inputvol };
+ uid_t uid;
+ gid_t gid;
+ int operation_flags;
+
+ virCheckFlags(0, -1);
if (vol->target.encryption != NULL) {
virStorageReportError(VIR_ERR_NO_SUPPORT,
@@ -383,24 +388,31 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
goto cleanup;
}
- uid_t uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid;
- gid_t gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;
+ uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid;
+ gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;
+ operation_flags = VIR_FILE_OP_FORCE_PERMS | VIR_FILE_OP_RETURN_FD;
+ if (pool->def->type == VIR_STORAGE_POOL_NETFS)
+ operation_flags |= VIR_FILE_OP_AS_UID;
- if ((createstat = virFileOperation(vol->target.path,
- O_RDWR | O_CREAT | O_EXCL,
- vol->target.perms.mode, uid, gid,
- createRawFileOpHook, &hdata,
- VIR_FILE_OP_FORCE_PERMS |
- (pool->def->type == VIR_STORAGE_POOL_NETFS
- ? VIR_FILE_OP_AS_UID : 0))) < 0) {
- virReportSystemError(-createstat,
- _("cannot create path '%s'"),
- vol->target.path);
- goto cleanup;
+ if ((fd = virFileOperation(vol->target.path,
+ O_RDWR | O_CREAT | O_EXCL,
+ vol->target.perms.mode, uid, gid,
+ NULL, NULL, operation_flags)) < 0) {
+ virReportSystemError(-fd,
+ _("cannot create path '%s'"),
+ vol->target.path);
+ goto cleanup;
+ }
+
+ if ((ret = createRawFileOpHook(fd, &hdata)) < 0) {
+ virReportSystemError(-fd,
+ _("cannot create path '%s'"),
+ vol->target.path);
+ ret = -1;
}
- ret = 0;
cleanup:
+ VIR_FORCE_CLOSE(fd);
return ret;
}
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 07/13] util: rename virFileOperation to virFileOpenAs
This patch intentionally doesn't change indentation, in order to
make it easier to review the real changes.
* src/util/util.h (VIR_FILE_OP_RETURN_FD, virFileOperationHook):
Delete.
(virFileOperation): Rename...
(virFileOpenAs): ...and reduce parameters.
* src/util/util.c (virFileOperationNoFork, virFileOperation):
Rename and simplify.
* src/qemu/qemu_driver.c (qemudDomainSaveFlag): Adjust caller.
* src/storage/storage_backend.c (virStorageBackendCreateRaw):
Likewise.
* src/libvirt_private.syms: Reflect rename.
---
v5: rebase to changes in patch 4/13
src/libvirt_private.syms | 2 +-
src/qemu/qemu_driver.c | 20 +++-----
src/storage/storage_backend.c | 12 ++--
src/util/util.c | 105 +++++++++++++---------------------------
src/util/util.h | 15 ++----
5 files changed, 55 insertions(+), 99 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 93504e5..fb95858 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -898,8 +898,8 @@ virFileIsExecutable;
virFileLinkPointsTo;
virFileMakePath;
virFileMatchesNameSuffix;
+virFileOpenAs;
virFileOpenTty;
-virFileOperation;
virFilePid;
virFileReadAll;
virFileReadLimFD;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7a6e4ff..8ee2a29 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1913,11 +1913,9 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
goto endjob;
}
} else {
- if ((fd = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
- S_IRUSR|S_IWUSR,
- getuid(), getgid(),
- NULL, NULL,
- VIR_FILE_OP_RETURN_FD)) < 0) {
+ if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY,
+ S_IRUSR|S_IWUSR,
+ getuid(), getgid(), 0)) < 0) {
/* If we failed as root, and the error was permission-denied
(EACCES or EPERM), assume it's on a network-connected share
where root access is restricted (eg, root-squashed NFS). If the
@@ -1951,7 +1949,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
case 0:
default:
- /* local file - log the error returned by virFileOperation */
+ /* local file - log the error returned by virFileOpenAs */
virReportSystemError(-rc,
_("Failed to create domain save file
'%s'"),
path);
@@ -1962,12 +1960,10 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
/* Retry creating the file as driver->user */
- if ((fd = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
- S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
- driver->user, driver->group,
- NULL, NULL,
- (VIR_FILE_OP_AS_UID |
- VIR_FILE_OP_RETURN_FD))) < 0) {
+ if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY,
+ S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
+ driver->user, driver->group,
+ VIR_FILE_OPEN_AS_UID)) < 0) {
virReportSystemError(-fd,
_("Error from child process creating
'%s'"),
path);
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index b0a512c..7015475 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -390,14 +390,14 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid;
gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;
- operation_flags = VIR_FILE_OP_FORCE_PERMS | VIR_FILE_OP_RETURN_FD;
+ operation_flags = VIR_FILE_OPEN_FORCE_PERMS;
if (pool->def->type == VIR_STORAGE_POOL_NETFS)
- operation_flags |= VIR_FILE_OP_AS_UID;
+ operation_flags |= VIR_FILE_OPEN_AS_UID;
- if ((fd = virFileOperation(vol->target.path,
- O_RDWR | O_CREAT | O_EXCL,
- vol->target.perms.mode, uid, gid,
- NULL, NULL, operation_flags)) < 0) {
+ if ((fd = virFileOpenAs(vol->target.path,
+ O_RDWR | O_CREAT | O_EXCL,
+ vol->target.perms.mode, uid, gid,
+ operation_flags)) < 0) {
virReportSystemError(-fd,
_("cannot create path '%s'"),
vol->target.path);
diff --git a/src/util/util.c b/src/util/util.c
index a0a331d..49a8bb6 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1391,10 +1391,10 @@ virFileIsExecutable(const char *file)
#ifndef WIN32
/* return -errno on failure, or 0 on success */
-static int virFileOperationNoFork(const char *path, int openflags, mode_t mode,
- uid_t uid, gid_t gid,
- virFileOperationHook hook, void *hookdata,
- unsigned int flags) {
+static int
+virFileOpenAsNoFork(const char *path, int openflags, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags)
+{
int fd = -1;
int ret = 0;
struct stat st;
@@ -1417,7 +1417,7 @@ static int virFileOperationNoFork(const char *path, int openflags,
mode_t mode,
path, (unsigned int) uid, (unsigned int) gid);
goto error;
}
- if ((flags & VIR_FILE_OP_FORCE_PERMS)
+ if ((flags & VIR_FILE_OPEN_FORCE_PERMS)
&& (fchmod(fd, mode) < 0)) {
ret = -errno;
virReportSystemError(errno,
@@ -1425,17 +1425,7 @@ static int virFileOperationNoFork(const char *path, int openflags,
mode_t mode,
path, mode);
goto error;
}
- if ((hook) && ((ret = hook(fd, hookdata)) != 0)) {
- goto error;
- }
- if (flags & VIR_FILE_OP_RETURN_FD)
- return fd;
- if (VIR_CLOSE(fd) < 0) {
- ret = -errno;
- virReportSystemError(errno, _("failed to close new file
'%s'"),
- path);
- goto error;
- }
+ return fd;
error:
VIR_FORCE_CLOSE(fd);
@@ -1482,35 +1472,27 @@ error:
}
/**
- * virFileOperation:
+ * virFileOpenAs:
* @path: file to open or create
* @openflags: flags to pass to open
* @mode: mode to use on creation or when forcing permissions
* @uid: uid that should own file on creation
* @gid: gid that should own file
- * @hook: callback to run once file is open; see below for restrictions
- * @hookdata: opaque data for @hook
- * @flags: bit-wise or of VIR_FILE_OP_* flags
+ * @flags: bit-wise or of VIR_FILE_OPEN_* flags
*
* Open @path, and execute an optional callback @hook on the open file
* description. @hook must return 0 on success, or -errno on failure.
- * If @flags includes VIR_FILE_OP_AS_UID, then open the file while the
+ * If @flags includes VIR_FILE_OPEN_AS_UID, then open the file while the
* effective user id is @uid (by using a child process); this allows
* one to bypass root-squashing NFS issues. If @flags includes
- * VIR_FILE_OP_FORCE_PERMS, then ensure that @path has those
+ * VIR_FILE_OPEN_FORCE_PERMS, then ensure that @path has those
* permissions before the callback, even if the file already existed
- * with different permissions. If @flags includes
- * VIR_FILE_OP_RETURN_FD, then use SCM_RIGHTS to guarantee that any
- * @hook is run in the parent, and the return value (if non-negative)
- * is the file descriptor, left open. Otherwise, @hook might be run
- * in a child process, so it must be async-signal-safe (ie. no
- * malloc() or anything else that depends on modifying locks held in
- * the parent), no file descriptor remains open, and 0 is returned on
- * success. Return -errno on failure. */
-int virFileOperation(const char *path, int openflags, mode_t mode,
- uid_t uid, gid_t gid,
- virFileOperationHook hook, void *hookdata,
- unsigned int flags) {
+ * with different permissions. The return value (if non-negative)
+ * is the file descriptor, left open. Return -errno on failure. */
+int
+virFileOpenAs(const char *path, int openflags, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags)
+{
struct stat st;
pid_t pid;
int waitret, status, ret = 0;
@@ -1523,11 +1505,10 @@ int virFileOperation(const char *path, int openflags, mode_t
mode,
struct iovec iov;
int forkRet;
- if ((!(flags & VIR_FILE_OP_AS_UID))
+ if ((!(flags & VIR_FILE_OPEN_AS_UID))
|| (getuid() != 0)
|| ((uid == 0) && (gid == 0))) {
- return virFileOperationNoFork(path, openflags, mode, uid, gid,
- hook, hookdata, flags);
+ return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
}
/* parent is running as root, but caller requested that the
@@ -1535,7 +1516,7 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
* following dance avoids problems caused by root-squashing
* NFS servers. */
- if (flags & VIR_FILE_OP_RETURN_FD) {
+ {
if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) {
ret = -errno;
virReportSystemError(errno,
@@ -1565,7 +1546,7 @@ int virFileOperation(const char *path, int openflags, mode_t mode,
}
if (pid) { /* parent */
- if (flags & VIR_FILE_OP_RETURN_FD) {
+ {
VIR_FORCE_CLOSE(pair[1]);
do {
@@ -1601,19 +1582,13 @@ int virFileOperation(const char *path, int openflags, mode_t
mode,
goto parenterror;
}
if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES ||
- ((flags & VIR_FILE_OP_RETURN_FD) && fd == -1)) {
+ fd == -1) {
/* fall back to the simpler method, which works better in
* some cases */
- return virFileOperationNoFork(path, openflags, mode, uid, gid,
- hook, hookdata, flags);
+ return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
}
- if (!ret && (flags & VIR_FILE_OP_RETURN_FD)) {
- if ((hook) && ((ret = hook(fd, hookdata)) != 0)) {
- VIR_FORCE_CLOSE(fd);
- goto parenterror;
- }
+ if (!ret)
ret = fd;
- }
parenterror:
return ret;
}
@@ -1626,6 +1601,7 @@ parenterror:
ret = -errno;
goto childerror;
}
+ VIR_FORCE_CLOSE(pair[0]);
/* set desired uid/gid, then attempt to create the file */
@@ -1665,7 +1641,7 @@ parenterror:
path, (unsigned int) uid, (unsigned int) gid);
goto childerror;
}
- if ((flags & VIR_FILE_OP_FORCE_PERMS)
+ if ((flags & VIR_FILE_OPEN_FORCE_PERMS)
&& (fchmod(fd, mode) < 0)) {
ret = -errno;
virReportSystemError(errno,
@@ -1673,8 +1649,7 @@ parenterror:
path, mode);
goto childerror;
}
- if (flags & VIR_FILE_OP_RETURN_FD) {
- VIR_FORCE_CLOSE(pair[0]);
+ {
memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
do {
@@ -1685,17 +1660,6 @@ parenterror:
ret = -errno;
goto childerror;
}
- ret = 0;
- } else {
- if ((hook) && ((ret = hook(fd, hookdata)) != 0))
- goto childerror;
- if (VIR_CLOSE(fd) < 0) {
- ret = -errno;
- virReportSystemError(errno,
- _("child failed to close new file
'%s'"),
- path);
- goto childerror;
- }
}
ret = 0;
@@ -1704,6 +1668,7 @@ childerror:
* If the child exits with EACCES, then the parent tries again. */
/* XXX This makes assumptions about errno being < 255, which is
* not true on Hurd. */
+ VIR_FORCE_CLOSE(pair[1]);
ret = -ret;
if ((ret & 0xff) != ret) {
VIR_WARN("unable to pass desired return value %d", ret);
@@ -1813,17 +1778,15 @@ childerror:
#else /* WIN32 */
/* return -errno on failure, or 0 on success */
-int virFileOperation(const char *path ATTRIBUTE_UNUSED,
- int openflags ATTRIBUTE_UNUSED,
- mode_t mode ATTRIBUTE_UNUSED,
- uid_t uid ATTRIBUTE_UNUSED,
- gid_t gid ATTRIBUTE_UNUSED,
- virFileOperationHook hook ATTRIBUTE_UNUSED,
- void *hookdata ATTRIBUTE_UNUSED,
- unsigned int flags ATTRIBUTE_UNUSED)
+int virFileOpenAs(const char *path ATTRIBUTE_UNUSED,
+ int openflags ATTRIBUTE_UNUSED,
+ mode_t mode ATTRIBUTE_UNUSED,
+ uid_t uid ATTRIBUTE_UNUSED,
+ gid_t gid ATTRIBUTE_UNUSED,
+ unsigned int flags ATTRIBUTE_UNUSED)
{
virUtilError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("virFileOperation is not implemented for
WIN32"));
+ "%s", _("virFileOpenAs is not implemented for
WIN32"));
return -ENOSYS;
}
diff --git a/src/util/util.h b/src/util/util.h
index b1ca871..7b7722a 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -127,16 +127,13 @@ bool virFileIsExecutable(const char *file) ATTRIBUTE_NONNULL(1);
char *virFileSanitizePath(const char *path);
enum {
- VIR_FILE_OP_NONE = 0,
- VIR_FILE_OP_AS_UID = (1 << 0),
- VIR_FILE_OP_FORCE_PERMS = (1 << 1),
- VIR_FILE_OP_RETURN_FD = (1 << 2),
+ VIR_FILE_OPEN_NONE = 0,
+ VIR_FILE_OPEN_AS_UID = (1 << 0),
+ VIR_FILE_OPEN_FORCE_PERMS = (1 << 1),
};
-typedef int (*virFileOperationHook)(int fd, void *data);
-int virFileOperation(const char *path, int openflags, mode_t mode,
- uid_t uid, gid_t gid,
- virFileOperationHook hook, void *hookdata,
- unsigned int flags)
+int virFileOpenAs(const char *path, int openflags, mode_t mode,
+ uid_t uid, gid_t gid,
+ unsigned int flags)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
enum {
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 08/13] util: adjust indentation in previous patch
Separating the indentation from the real patch made review easier.
* src/util/util.c (virFileOpenAs): Whitespace changes.
---
v5: rebase to changes in 4/13 and 7/13
src/util/util.c | 97 +++++++++++++++++++++++++-----------------------------
1 files changed, 45 insertions(+), 52 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 49a8bb6..035036b 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1439,8 +1439,7 @@ static int virDirCreateNoFork(const char *path, mode_t mode, uid_t
uid, gid_t gi
struct stat st;
if ((mkdir(path, mode) < 0)
- && !((errno == EEXIST) && (flags &
VIR_DIR_CREATE_ALLOW_EXIST)))
- {
+ && !((errno == EEXIST) && (flags &
VIR_DIR_CREATE_ALLOW_EXIST))) {
ret = -errno;
virReportSystemError(errno, _("failed to create directory
'%s'"),
path);
@@ -1516,28 +1515,26 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
* following dance avoids problems caused by root-squashing
* NFS servers. */
- {
- if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) {
- ret = -errno;
- virReportSystemError(errno,
- _("failed to create socket needed for
'%s'"),
- path);
- return ret;
- }
-
- memset(&msg, 0, sizeof(msg));
- iov.iov_base = &dummy;
- iov.iov_len = 1;
- msg.msg_iov = &iov;
- msg.msg_iovlen = 1;
- msg.msg_control = buf;
- msg.msg_controllen = sizeof(buf);
- cmsg = CMSG_FIRSTHDR(&msg);
- cmsg->cmsg_level = SOL_SOCKET;
- cmsg->cmsg_type = SCM_RIGHTS;
- cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
+ if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) {
+ ret = -errno;
+ virReportSystemError(errno,
+ _("failed to create socket needed for
'%s'"),
+ path);
+ return ret;
}
+ memset(&msg, 0, sizeof(msg));
+ iov.iov_base = &dummy;
+ iov.iov_len = 1;
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+ msg.msg_control = buf;
+ msg.msg_controllen = sizeof(buf);
+ cmsg = CMSG_FIRSTHDR(&msg);
+ cmsg->cmsg_level = SOL_SOCKET;
+ cmsg->cmsg_type = SCM_RIGHTS;
+ cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
+
forkRet = virFork(&pid);
if (pid < 0) {
@@ -1546,29 +1543,27 @@ virFileOpenAs(const char *path, int openflags, mode_t mode,
}
if (pid) { /* parent */
- {
- VIR_FORCE_CLOSE(pair[1]);
-
- do {
- ret = recvmsg(pair[0], &msg, 0);
- } while (ret < 0 && errno == EINTR);
-
- if (ret < 0) {
- ret = -errno;
- VIR_FORCE_CLOSE(pair[0]);
- while ((waitret = waitpid(pid, NULL, 0) == -1)
- && (errno == EINTR));
- goto parenterror;
- }
+ VIR_FORCE_CLOSE(pair[1]);
+
+ do {
+ ret = recvmsg(pair[0], &msg, 0);
+ } while (ret < 0 && errno == EINTR);
+
+ if (ret < 0) {
+ ret = -errno;
VIR_FORCE_CLOSE(pair[0]);
+ while ((waitret = waitpid(pid, NULL, 0) == -1)
+ && (errno == EINTR));
+ goto parenterror;
+ }
+ VIR_FORCE_CLOSE(pair[0]);
- /* See if fd was transferred. */
- cmsg = CMSG_FIRSTHDR(&msg);
- if (cmsg && cmsg->cmsg_len == CMSG_LEN(sizeof(fd)) &&
- cmsg->cmsg_level == SOL_SOCKET &&
- cmsg->cmsg_type == SCM_RIGHTS) {
- memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd));
- }
+ /* See if fd was transferred. */
+ cmsg = CMSG_FIRSTHDR(&msg);
+ if (cmsg && cmsg->cmsg_len == CMSG_LEN(sizeof(fd)) &&
+ cmsg->cmsg_level == SOL_SOCKET &&
+ cmsg->cmsg_type == SCM_RIGHTS) {
+ memcpy(&fd, CMSG_DATA(cmsg), sizeof(fd));
}
/* wait for child to complete, and retrieve its exit code */
@@ -1649,17 +1644,15 @@ parenterror:
path, mode);
goto childerror;
}
- {
- memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
+ memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
- do {
- ret = sendmsg(pair[1], &msg, 0);
- } while (ret < 0 && errno == EINTR);
+ do {
+ ret = sendmsg(pair[1], &msg, 0);
+ } while (ret < 0 && errno == EINTR);
- if (ret < 0) {
- ret = -errno;
- goto childerror;
- }
+ if (ret < 0) {
+ ret = -errno;
+ goto childerror;
}
ret = 0;
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 09/13] qemu, storage: improve type safety
* src/storage/storage_backend.c (createRawFileOpHook): Change
signature.
(struct createRawFileOpHookData): Delete unused struct.
(virStorageBackendCreateRaw): Adjust caller.
* src/qemu/qemu_driver.c (struct fileOpHookData): Delete unused
struct.
(qemudDomainSaveFileOpHook): Rename...
(qemuDomainSaveFileOpHook): ...and change signature.
(qemudDomainSaveFlag): Adjust caller.
---
v5: no real changes
src/qemu/qemu_driver.c | 29 +++++++++--------------------
src/storage/storage_backend.c | 34 ++++++++++++++--------------------
2 files changed, 23 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8ee2a29..6ecfbd6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1774,30 +1774,25 @@ struct qemud_save_header {
int unused[15];
};
-struct fileOpHookData {
- virDomainPtr dom;
- const char *path;
- char *xml;
- struct qemud_save_header *header;
-};
-
/* return -errno on failure, or 0 on success */
-static int qemudDomainSaveFileOpHook(int fd, void *data) {
- struct fileOpHookData *hdata = data;
+static int
+qemuDomainSaveHeader(int fd, const char *path, char *xml,
+ struct qemud_save_header *header)
+{
int ret = 0;
- if (safewrite(fd, hdata->header, sizeof(*hdata->header)) !=
sizeof(*hdata->header)) {
+ if (safewrite(fd, header, sizeof(*header)) != sizeof(*header)) {
ret = -errno;
qemuReportError(VIR_ERR_OPERATION_FAILED,
_("failed to write header to domain save file
'%s'"),
- hdata->path);
+ path);
goto endjob;
}
- if (safewrite(fd, hdata->xml, hdata->header->xml_len) !=
hdata->header->xml_len) {
+ if (safewrite(fd, xml, header->xml_len) != header->xml_len) {
ret = -errno;
qemuReportError(VIR_ERR_OPERATION_FAILED,
- _("failed to write xml to '%s'"),
hdata->path);
+ _("failed to write xml to '%s'"), path);
goto endjob;
}
endjob:
@@ -1813,7 +1808,6 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
{
char *xml = NULL;
struct qemud_save_header header;
- struct fileOpHookData hdata;
int bypassSecurityDriver = 0;
int ret = -1;
int rc;
@@ -1979,12 +1973,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
}
/* Write header to file, followed by XML */
- hdata.dom = dom;
- hdata.path = path;
- hdata.xml = xml;
- hdata.header = &header;
-
- if (qemudDomainSaveFileOpHook(fd, &hdata) < 0) {
+ if (qemuDomainSaveHeader(fd, path, xml, &header) < 0) {
VIR_FORCE_CLOSE(fd);
goto endjob;
}
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 7015475..9b7bcc4 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -292,31 +292,27 @@ cleanup:
return ret;
}
-struct createRawFileOpHookData {
- virStorageVolDefPtr vol;
- virStorageVolDefPtr inputvol;
-};
-
-static int createRawFileOpHook(int fd, void *data) {
- struct createRawFileOpHookData *hdata = data;
+static int
+createRawFile(int fd, virStorageVolDefPtr vol,
+ virStorageVolDefPtr inputvol)
+{
int ret = 0;
unsigned long long remain;
/* Seek to the final size, so the capacity is available upfront
* for progress reporting */
- if (ftruncate(fd, hdata->vol->capacity) < 0) {
+ if (ftruncate(fd, vol->capacity) < 0) {
ret = -errno;
virReportSystemError(errno,
_("cannot extend file '%s'"),
- hdata->vol->target.path);
+ vol->target.path);
goto cleanup;
}
- remain = hdata->vol->allocation;
+ remain = vol->allocation;
- if (hdata->inputvol) {
- ret = virStorageBackendCopyToFD(hdata->vol, hdata->inputvol,
- fd, &remain, 1);
+ if (inputvol) {
+ ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain, 1);
if (ret < 0) {
goto cleanup;
}
@@ -334,11 +330,10 @@ static int createRawFileOpHook(int fd, void *data) {
if (bytes > remain)
bytes = remain;
- if (safezero(fd, 0, hdata->vol->allocation - remain,
- bytes) != 0) {
+ if (safezero(fd, 0, vol->allocation - remain, bytes) != 0) {
ret = -errno;
virReportSystemError(errno, _("cannot fill file
'%s'"),
- hdata->vol->target.path);
+ vol->target.path);
goto cleanup;
}
remain -= bytes;
@@ -347,7 +342,7 @@ static int createRawFileOpHook(int fd, void *data) {
if (safezero(fd, 0, 0, remain) != 0) {
ret = -errno;
virReportSystemError(errno, _("cannot fill file
'%s'"),
- hdata->vol->target.path);
+ vol->target.path);
goto cleanup;
}
}
@@ -357,7 +352,7 @@ static int createRawFileOpHook(int fd, void *data) {
if (fsync(fd) < 0) {
ret = -errno;
virReportSystemError(errno, _("cannot sync data to file
'%s'"),
- hdata->vol->target.path);
+ vol->target.path);
goto cleanup;
}
@@ -374,7 +369,6 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
{
int ret = -1;
int fd = -1;
- struct createRawFileOpHookData hdata = { vol, inputvol };
uid_t uid;
gid_t gid;
int operation_flags;
@@ -404,7 +398,7 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
goto cleanup;
}
- if ((ret = createRawFileOpHook(fd, &hdata)) < 0) {
+ if ((ret = createRawFile(fd, vol, inputvol)) < 0) {
virReportSystemError(-fd,
_("cannot create path '%s'"),
vol->target.path);
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 10/13] qemu: use common API for reading difficult files
Direct access to an open file is so much simpler than passing
everything through a pipe!
* src/qemu/qemu_driver.c (qemudOpenAsUID)
(qemudDomainSaveImageClose): Delete.
(qemudDomainSaveImageOpen): Rename...
(qemuDomainSaveImageOpen): ...and drop read_pid argument. Use
virFileOpenAs instead of qemudOpenAsUID.
(qemudDomainSaveImageStartVM, qemudDomainRestore)
(qemudDomainObjRestore): Rename...
(qemuDomainSaveImageStartVM, qemuDomainRestore)
(qemDomainObjRestore): ...and simplify accordingly.
(qemudDomainObjStart, qemuDriver): Update callers.
---
v5: no major changes
src/qemu/qemu_driver.c | 265 ++++++++---------------------------------------
1 files changed, 45 insertions(+), 220 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6ecfbd6..2c852c0 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3048,183 +3048,32 @@ cleanup:
return ret;
}
-/* qemudOpenAsUID() - pipe/fork/setuid/open a file, and return the
- pipe fd to caller, so that it can read from the file. Also return
- the pid of the child process, so the caller can wait for it to exit
- after it's finished reading (to avoid a zombie, if nothing
- else). */
-
-static int
-qemudOpenAsUID(const char *path, uid_t uid, gid_t gid, pid_t *child_pid)
-{
- int pipefd[2];
- int fd = -1;
-
- *child_pid = -1;
-
- if (pipe(pipefd) < 0) {
- virReportSystemError(errno,
- _("failed to create pipe to read '%s'"),
- path);
- pipefd[0] = pipefd[1] = -1;
- goto parent_cleanup;
- }
-
- int forkRet = virFork(child_pid);
-
- if (*child_pid < 0) {
- virReportSystemError(errno,
- _("failed to fork child to read '%s'"),
- path);
- goto parent_cleanup;
- }
-
- if (*child_pid > 0) {
-
- /* parent */
-
- /* parent doesn't need the write side of the pipe */
- VIR_FORCE_CLOSE(pipefd[1]);
-
- if (forkRet < 0) {
- virReportSystemError(errno,
- _("failed in parent after forking child to read
'%s'"),
- path);
- goto parent_cleanup;
- }
- /* caller gets the read side of the pipe */
- fd = pipefd[0];
- pipefd[0] = -1;
-parent_cleanup:
- VIR_FORCE_CLOSE(pipefd[0]);
- VIR_FORCE_CLOSE(pipefd[1]);
- if ((fd < 0) && (*child_pid > 0)) {
- /* a child process was started and subsequently an error
- occurred in the parent, so we need to wait for it to
- exit, but its status is inconsequential. */
- while ((waitpid(*child_pid, NULL, 0) == -1)
- && (errno == EINTR)) {
- /* empty */
- }
- *child_pid = -1;
- }
- return fd;
- }
-
- /* child */
-
- /* setuid to the qemu user, then open the file, read it,
- and stuff it into the pipe for the parent process to
- read */
- int exit_code;
- char *buf = NULL;
- size_t bufsize = 1024 * 1024;
- int bytesread;
-
- /* child doesn't need the read side of the pipe */
- VIR_FORCE_CLOSE(pipefd[0]);
-
- if (forkRet < 0) {
- exit_code = errno;
- virReportSystemError(errno,
- _("failed in child after forking to read
'%s'"),
- path);
- goto child_cleanup;
- }
-
- if (virSetUIDGID(uid, gid) < 0) {
- exit_code = errno;
- goto child_cleanup;
- }
-
- if ((fd = open(path, O_RDONLY)) < 0) {
- exit_code = errno;
- virReportSystemError(errno,
- _("cannot open '%s' as uid %d"),
- path, uid);
- goto child_cleanup;
- }
-
- if (VIR_ALLOC_N(buf, bufsize) < 0) {
- exit_code = ENOMEM;
- virReportOOMError();
- goto child_cleanup;
- }
-
- /* read from fd and write to pipefd[1] until EOF */
- do {
- if ((bytesread = saferead(fd, buf, bufsize)) < 0) {
- exit_code = errno;
- virReportSystemError(errno,
- _("child failed reading from '%s'"),
- path);
- goto child_cleanup;
- }
- if (safewrite(pipefd[1], buf, bytesread) != bytesread) {
- exit_code = errno;
- virReportSystemError(errno, "%s",
- _("child failed writing to pipe"));
- goto child_cleanup;
- }
- } while (bytesread > 0);
- exit_code = 0;
-
-child_cleanup:
- VIR_FREE(buf);
- VIR_FORCE_CLOSE(fd);
- VIR_FORCE_CLOSE(pipefd[1]);
- _exit(exit_code);
-}
-
-static int qemudDomainSaveImageClose(int fd, pid_t read_pid, int *status)
-{
- int ret = 0;
-
- if (VIR_CLOSE(fd) < 0) {
- virReportSystemError(errno, "%s",
- _("cannot close file"));
- }
-
- if (read_pid != -1) {
- /* reap the process that read the file */
- while ((ret = waitpid(read_pid, status, 0)) == -1
- && errno == EINTR) {
- /* empty */
- }
- } else if (status) {
- *status = 0;
- }
-
- return ret;
-}
-
-static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5)
-qemudDomainSaveImageOpen(struct qemud_driver *driver,
- const char *path,
- virDomainDefPtr *ret_def,
- struct qemud_save_header *ret_header,
- pid_t *ret_read_pid)
+static int ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4)
+qemuDomainSaveImageOpen(struct qemud_driver *driver,
+ const char *path,
+ virDomainDefPtr *ret_def,
+ struct qemud_save_header *ret_header)
{
int fd;
- pid_t read_pid = -1;
struct qemud_save_header header;
char *xml = NULL;
virDomainDefPtr def = NULL;
- if ((fd = open(path, O_RDONLY)) < 0) {
- if ((driver->user == 0) || (getuid() != 0)) {
+ if ((fd = virFileOpenAs(path, O_RDONLY, 0, getuid(), getgid(), 0)) < 0) {
+ if ((fd != -EACCES && fd != -EPERM) ||
+ driver->user == getuid()) {
qemuReportError(VIR_ERR_OPERATION_FAILED,
"%s", _("cannot read domain image"));
goto error;
}
/* Opening as root failed, but qemu runs as a different user
- that might have better luck. Create a pipe, then fork a
- child process to run as the qemu user, which will hopefully
- have the necessary authority to read the file. */
- if ((fd = qemudOpenAsUID(path,
- driver->user, driver->group, &read_pid)) <
0) {
- /* error already reported */
+ * that might have better luck. */
+ if ((fd = virFileOpenAs(path, O_RDONLY, 0,
+ driver->user, driver->group,
+ VIR_FILE_OPEN_AS_UID)) < 0) {
+ qemuReportError(VIR_ERR_OPERATION_FAILED,
+ "%s", _("cannot read domain image"));
goto error;
}
}
@@ -3274,34 +3123,30 @@ qemudDomainSaveImageOpen(struct qemud_driver *driver,
*ret_def = def;
*ret_header = header;
- *ret_read_pid = read_pid;
return fd;
error:
virDomainDefFree(def);
VIR_FREE(xml);
- qemudDomainSaveImageClose(fd, read_pid, NULL);
+ VIR_FORCE_CLOSE(fd);
return -1;
}
-static int ATTRIBUTE_NONNULL(6)
-qemudDomainSaveImageStartVM(virConnectPtr conn,
- struct qemud_driver *driver,
- virDomainObjPtr vm,
- int *fd,
- pid_t *read_pid,
- const struct qemud_save_header *header,
- const char *path)
+static int ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_NONNULL(6)
+qemuDomainSaveImageStartVM(virConnectPtr conn,
+ struct qemud_driver *driver,
+ virDomainObjPtr vm,
+ int *fd,
+ const struct qemud_save_header *header,
+ const char *path)
{
int ret = -1;
virDomainEventPtr event;
int intermediatefd = -1;
pid_t intermediate_pid = -1;
int childstat;
- int wait_ret;
- int status;
if (header->version == 2) {
const char *intermediate_argv[3] = { NULL, "-dc", NULL };
@@ -3334,8 +3179,9 @@ qemudDomainSaveImageStartVM(virConnectPtr conn,
if (intermediate_pid != -1) {
if (ret < 0) {
- /* if there was an error setting up qemu, the intermediate process will
- * wait forever to write to stdout, so we must manually kill it.
+ /* if there was an error setting up qemu, the intermediate
+ * process will wait forever to write to stdout, so we
+ * must manually kill it.
*/
VIR_FORCE_CLOSE(intermediatefd);
VIR_FORCE_CLOSE(*fd);
@@ -3350,30 +3196,10 @@ qemudDomainSaveImageStartVM(virConnectPtr conn,
}
VIR_FORCE_CLOSE(intermediatefd);
- wait_ret = qemudDomainSaveImageClose(*fd, *read_pid, &status);
- *fd = -1;
- if (*read_pid != -1) {
- if (wait_ret == -1) {
- virReportSystemError(errno,
- _("failed to wait for process reading
'%s'"),
- path);
- ret = -1;
- } else if (!WIFEXITED(status)) {
- qemuReportError(VIR_ERR_OPERATION_FAILED,
- _("child process exited abnormally reading
'%s'"),
- path);
- ret = -1;
- } else {
- int exit_status = WEXITSTATUS(status);
- if (exit_status != 0) {
- virReportSystemError(exit_status,
- _("child process returned error reading
'%s'"),
- path);
- ret = -1;
- }
- }
+ if (VIR_CLOSE(*fd) < 0) {
+ virReportSystemError(errno, _("cannot close file: %s"), path);
+ ret = -1;
}
- *read_pid = -1;
if (ret < 0) {
qemuAuditDomainStart(vm, "restored", false);
@@ -3412,19 +3238,20 @@ out:
return ret;
}
-static int qemudDomainRestore(virConnectPtr conn,
- const char *path) {
+static int
+qemuDomainRestore(virConnectPtr conn,
+ const char *path)
+{
struct qemud_driver *driver = conn->privateData;
virDomainDefPtr def = NULL;
virDomainObjPtr vm = NULL;
int fd = -1;
- pid_t read_pid = -1;
int ret = -1;
struct qemud_save_header header;
qemuDriverLock(driver);
- fd = qemudDomainSaveImageOpen(driver, path, &def, &header, &read_pid);
+ fd = qemuDomainSaveImageOpen(driver, path, &def, &header);
if (fd < 0)
goto cleanup;
@@ -3442,8 +3269,7 @@ static int qemudDomainRestore(virConnectPtr conn,
if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
goto cleanup;
- ret = qemudDomainSaveImageStartVM(conn, driver, vm, &fd,
- &read_pid, &header, path);
+ ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path);
if (qemuDomainObjEndJob(vm) == 0)
vm = NULL;
@@ -3454,25 +3280,25 @@ static int qemudDomainRestore(virConnectPtr conn,
cleanup:
virDomainDefFree(def);
- qemudDomainSaveImageClose(fd, read_pid, NULL);
+ VIR_FORCE_CLOSE(fd);
if (vm)
virDomainObjUnlock(vm);
qemuDriverUnlock(driver);
return ret;
}
-static int qemudDomainObjRestore(virConnectPtr conn,
- struct qemud_driver *driver,
- virDomainObjPtr vm,
- const char *path)
+static int
+qemuDomainObjRestore(virConnectPtr conn,
+ struct qemud_driver *driver,
+ virDomainObjPtr vm,
+ const char *path)
{
virDomainDefPtr def = NULL;
int fd = -1;
- pid_t read_pid = -1;
int ret = -1;
struct qemud_save_header header;
- fd = qemudDomainSaveImageOpen(driver, path, &def, &header, &read_pid);
+ fd = qemuDomainSaveImageOpen(driver, path, &def, &header);
if (fd < 0)
goto cleanup;
@@ -3493,12 +3319,11 @@ static int qemudDomainObjRestore(virConnectPtr conn,
virDomainObjAssignDef(vm, def, true);
def = NULL;
- ret = qemudDomainSaveImageStartVM(conn, driver, vm, &fd,
- &read_pid, &header, path);
+ ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, &header, path);
cleanup:
virDomainDefFree(def);
- qemudDomainSaveImageClose(fd, read_pid, NULL);
+ VIR_FORCE_CLOSE(fd);
return ret;
}
@@ -3709,7 +3534,7 @@ static int qemudDomainObjStart(virConnectPtr conn,
*/
managed_save = qemuDomainManagedSavePath(driver, vm);
if ((managed_save) && (virFileExists(managed_save))) {
- ret = qemudDomainObjRestore(conn, driver, vm, managed_save);
+ ret = qemuDomainObjRestore(conn, driver, vm, managed_save);
if (unlink(managed_save) < 0) {
VIR_WARN("Failed to remove the managed state %s", managed_save);
@@ -7126,7 +6951,7 @@ static virDriver qemuDriver = {
qemuDomainGetBlkioParameters, /* domainGetBlkioParameters */
qemudDomainGetInfo, /* domainGetInfo */
qemudDomainSave, /* domainSave */
- qemudDomainRestore, /* domainRestore */
+ qemuDomainRestore, /* domainRestore */
qemudDomainCoreDump, /* domainCoreDump */
qemudDomainSetVcpus, /* domainSetVcpus */
qemudDomainSetVcpusFlags, /* domainSetVcpusFlags */
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 11/13] qemu: consolidate migration to file code
This points out that core dumps (still) don't work for root-squash
NFS, since the fd is not opened correctly. This patch should not
introduce any functionality change, it is just a refactoring to
avoid duplicated code.
* src/qemu/qemu_migration.h (qemuMigrationToFile): New prototype.
* src/qemu/qemu_migration.c (qemuMigrationToFile): New function.
* src/qemu/qemu_driver.c (qemudDomainSaveFlag, doCoreDump): Use
it.
---
v5: move qemuMigrationToFile to qemu_migration, which required
adding qemuCompressProgramName
src/qemu/qemu_driver.c | 165 +++++++--------------------------------------
src/qemu/qemu_migration.c | 98 +++++++++++++++++++++++++++
src/qemu/qemu_migration.h | 8 ++
3 files changed, 131 insertions(+), 140 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 2c852c0..182b0fd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1799,6 +1799,15 @@ endjob:
return ret;
}
+/* Given a enum qemud_save_formats compression level, return the name
+ * of the program to run, or NULL if no program is needed. */
+static const char *
+qemuCompressProgramName(int compress)
+{
+ return (compress == QEMUD_SAVE_FORMAT_RAW ? NULL :
+ qemudSaveCompressionTypeToString(compress));
+}
+
/* This internal function expects the driver lock to already be held on
* entry and the vm must be active.
*/
@@ -1808,15 +1817,14 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
{
char *xml = NULL;
struct qemud_save_header header;
- int bypassSecurityDriver = 0;
+ bool bypassSecurityDriver = false;
int ret = -1;
int rc;
virDomainEventPtr event = NULL;
qemuDomainObjPrivatePtr priv;
struct stat sb;
- int is_reg = 0;
+ bool is_reg = false;
unsigned long long offset;
- virCgroupPtr cgroup = NULL;
virBitmapPtr qemuCaps = NULL;
int fd = -1;
@@ -1871,9 +1879,9 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
* that with NFS we can't actually stat() the file.
* The subsequent codepaths will still raise an error
* if a truely fatal problem is hit */
- is_reg = 1;
+ is_reg = true;
} else {
- is_reg = S_ISREG(sb.st_mode);
+ is_reg = !!S_ISREG(sb.st_mode);
}
offset = sizeof(header) + header.xml_len;
@@ -1968,7 +1976,7 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
is NFS, we assume it's a root-squashing NFS share, and that
the security driver stuff would have failed anyway */
- bypassSecurityDriver = 1;
+ bypassSecurityDriver = true;
}
}
@@ -1978,87 +1986,14 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
goto endjob;
}
- /* Allow qemu to access file */
-
- if (!is_reg &&
- qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
- if (virCgroupForDomain(driver->cgroup, vm->def->name, &cgroup, 0) !=
0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to find cgroup for %s"),
- vm->def->name);
- goto endjob;
- }
- rc = virCgroupAllowDevicePath(cgroup, path,
- VIR_CGROUP_DEVICE_RW);
- qemuAuditCgroupPath(vm, cgroup, "allow", path, "rw", rc);
- if (rc < 0) {
- virReportSystemError(-rc,
- _("Unable to allow device %s for %s"),
- path, vm->def->name);
- goto endjob;
- }
- }
-
- if ((!bypassSecurityDriver) &&
- virSecurityManagerSetSavedStateLabel(driver->securityManager,
- vm, path) < 0)
- goto endjob;
-
- if (header.compressed == QEMUD_SAVE_FORMAT_RAW) {
- const char *args[] = { "cat", NULL };
-
- qemuDomainObjEnterMonitorWithDriver(driver, vm);
- if (qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) &&
- priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
- rc = qemuMonitorMigrateToFd(priv->mon,
- QEMU_MONITOR_MIGRATE_BACKGROUND,
- fd);
- } else {
- rc = qemuMonitorMigrateToFile(priv->mon,
- QEMU_MONITOR_MIGRATE_BACKGROUND,
- args, path, offset);
- }
- qemuDomainObjExitMonitorWithDriver(driver, vm);
- } else {
- const char *prog = qemudSaveCompressionTypeToString(header.compressed);
- const char *args[] = {
- prog,
- "-c",
- NULL
- };
- qemuDomainObjEnterMonitorWithDriver(driver, vm);
- rc = qemuMonitorMigrateToFile(priv->mon,
- QEMU_MONITOR_MIGRATE_BACKGROUND,
- args, path, offset);
- qemuDomainObjExitMonitorWithDriver(driver, vm);
- }
-
- if (rc < 0)
- goto endjob;
-
+ /* Perform the migration */
+ if (qemuMigrationToFile(driver, vm, qemuCaps, fd, offset, path,
+ qemuCompressProgramName(compressed),
+ is_reg, bypassSecurityDriver) < 0)
+ goto cleanup;
if (VIR_CLOSE(fd) < 0) {
virReportSystemError(errno, _("unable to close %s"), path);
- goto endjob;
- }
-
- rc = qemuMigrationWaitForCompletion(driver, vm);
-
- if (rc < 0)
- goto endjob;
-
- if ((!bypassSecurityDriver) &&
- virSecurityManagerRestoreSavedStateLabel(driver->securityManager,
- vm, path) < 0)
- VIR_WARN("failed to restore save state label on %s", path);
- bypassSecurityDriver = true;
-
- if (cgroup != NULL) {
- rc = virCgroupDenyDevicePath(cgroup, path,
- VIR_CGROUP_DEVICE_RWM);
- qemuAuditCgroupPath(vm, cgroup, "deny", path, "rwm", rc);
- if (rc < 0)
- VIR_WARN("Unable to deny device %s for %s %d",
- path, vm->def->name, rc);
+ goto cleanup;
}
ret = 0;
@@ -2084,22 +2019,7 @@ endjob:
if (rc < 0)
VIR_WARN0("Unable to resume guest CPUs after save
failure");
}
-
- if (cgroup != NULL) {
- rc = virCgroupDenyDevicePath(cgroup, path,
- VIR_CGROUP_DEVICE_RWM);
- qemuAuditCgroupPath(vm, cgroup, "deny", path, "rwm",
rc);
- if (rc < 0)
- VIR_WARN("Unable to deny device %s for %s: %d",
- path, vm->def->name, rc);
- }
-
- if ((!bypassSecurityDriver) &&
- virSecurityManagerRestoreSavedStateLabel(driver->securityManager,
- vm, path) < 0)
- VIR_WARN("failed to restore save state label on %s", path);
}
-
if (qemuDomainObjEndJob(vm) == 0)
vm = NULL;
}
@@ -2112,7 +2032,6 @@ cleanup:
unlink(path);
if (event)
qemuDomainEventQueue(driver, event);
- virCgroupFree(&cgroup);
return ret;
}
@@ -2316,9 +2235,6 @@ static int doCoreDump(struct qemud_driver *driver,
{
int fd = -1;
int ret = -1;
- qemuDomainObjPrivatePtr priv;
-
- priv = vm->privateData;
/* Create an empty file with appropriate ownership. */
if ((fd = open(path, O_CREAT|O_TRUNC|O_WRONLY, S_IRUSR|S_IWUSR)) < 0) {
@@ -2327,6 +2243,10 @@ static int doCoreDump(struct qemud_driver *driver,
goto cleanup;
}
+ if (qemuMigrationToFile(driver, vm, NULL, fd, 0, path,
+ qemuCompressProgramName(compress), true, false) < 0)
+ goto cleanup;
+
if (VIR_CLOSE(fd) < 0) {
virReportSystemError(errno,
_("unable to save file %s"),
@@ -2334,42 +2254,7 @@ static int doCoreDump(struct qemud_driver *driver,
goto cleanup;
}
- if (virSecurityManagerSetSavedStateLabel(driver->securityManager,
- vm, path) < 0)
- goto cleanup;
-
- qemuDomainObjEnterMonitorWithDriver(driver, vm);
- if (compress == QEMUD_SAVE_FORMAT_RAW) {
- const char *args[] = {
- "cat",
- NULL,
- };
- ret = qemuMonitorMigrateToFile(priv->mon,
- QEMU_MONITOR_MIGRATE_BACKGROUND,
- args, path, 0);
- } else {
- const char *prog = qemudSaveCompressionTypeToString(compress);
- const char *args[] = {
- prog,
- "-c",
- NULL,
- };
- ret = qemuMonitorMigrateToFile(priv->mon,
- QEMU_MONITOR_MIGRATE_BACKGROUND,
- args, path, 0);
- }
- qemuDomainObjExitMonitorWithDriver(driver, vm);
- if (ret < 0)
- goto cleanup;
-
- ret = qemuMigrationWaitForCompletion(driver, vm);
-
- if (ret < 0)
- goto cleanup;
-
- if (virSecurityManagerRestoreSavedStateLabel(driver->securityManager,
- vm, path) < 0)
- goto cleanup;
+ ret = 0;
cleanup:
if (ret != 0)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 826e1bf..592ffe2 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -29,6 +29,7 @@
#include "qemu_process.h"
#include "qemu_capabilities.h"
#include "qemu_audit.h"
+#include "qemu_cgroup.h"
#include "logging.h"
#include "virterror_internal.h"
@@ -1283,3 +1284,100 @@ cleanup:
qemuDomainEventQueue(driver, event);
return dom;
}
+
+/* Helper function called while driver lock is held and vm is active. */
+int
+qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm,
+ virBitmapPtr qemuCaps,
+ int fd, off_t offset, const char *path,
+ const char *compressor,
+ bool is_reg, bool bypassSecurityDriver)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+ virCgroupPtr cgroup = NULL;
+ int ret = -1;
+ int rc;
+ bool restoreLabel = false;
+
+ if (!is_reg &&
+ qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
+ if (virCgroupForDomain(driver->cgroup, vm->def->name,
+ &cgroup, 0) != 0) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to find cgroup for %s"),
+ vm->def->name);
+ goto cleanup;
+ }
+ rc = virCgroupAllowDevicePath(cgroup, path,
+ VIR_CGROUP_DEVICE_RW);
+ qemuAuditCgroupPath(vm, cgroup, "allow", path, "rw", rc);
+ if (rc < 0) {
+ virReportSystemError(-rc,
+ _("Unable to allow device %s for %s"),
+ path, vm->def->name);
+ goto cleanup;
+ }
+ }
+
+ if ((!bypassSecurityDriver) &&
+ virSecurityManagerSetSavedStateLabel(driver->securityManager,
+ vm, path) < 0)
+ goto cleanup;
+ restoreLabel = true;
+
+ if (!compressor) {
+ const char *args[] = { "cat", NULL };
+
+ qemuDomainObjEnterMonitorWithDriver(driver, vm);
+ if (qemuCaps && qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)
&&
+ priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
+ rc = qemuMonitorMigrateToFd(priv->mon,
+ QEMU_MONITOR_MIGRATE_BACKGROUND,
+ fd);
+ } else {
+ rc = qemuMonitorMigrateToFile(priv->mon,
+ QEMU_MONITOR_MIGRATE_BACKGROUND,
+ args, path, offset);
+ }
+ qemuDomainObjExitMonitorWithDriver(driver, vm);
+ } else {
+ const char *prog = compressor;
+ const char *args[] = {
+ prog,
+ "-c",
+ NULL
+ };
+ qemuDomainObjEnterMonitorWithDriver(driver, vm);
+ rc = qemuMonitorMigrateToFile(priv->mon,
+ QEMU_MONITOR_MIGRATE_BACKGROUND,
+ args, path, offset);
+ qemuDomainObjExitMonitorWithDriver(driver, vm);
+ }
+
+ if (rc < 0)
+ goto cleanup;
+
+ rc = qemuMigrationWaitForCompletion(driver, vm);
+
+ if (rc < 0)
+ goto cleanup;
+
+ ret = 0;
+
+cleanup:
+ if (restoreLabel && (!bypassSecurityDriver) &&
+ virSecurityManagerRestoreSavedStateLabel(driver->securityManager,
+ vm, path) < 0)
+ VIR_WARN("failed to restore save state label on %s", path);
+
+ if (cgroup != NULL) {
+ rc = virCgroupDenyDevicePath(cgroup, path,
+ VIR_CGROUP_DEVICE_RWM);
+ qemuAuditCgroupPath(vm, cgroup, "deny", path, "rwm", rc);
+ if (rc < 0)
+ VIR_WARN("Unable to deny device %s for %s %d",
+ path, vm->def->name, rc);
+ virCgroupFree(&cgroup);
+ }
+ return ret;
+}
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index 3cac617..db16ce4 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -60,4 +60,12 @@ virDomainPtr qemuMigrationFinish(struct qemud_driver *driver,
int retcode);
+int qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm,
+ virBitmapPtr qemuCaps,
+ int fd, off_t offset, const char *path,
+ const char *compressor,
+ bool is_reg, bool bypassSecurityDriver)
+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(6)
+ ATTRIBUTE_RETURN_CHECK;
+
#endif /* __QEMU_MIGRATION_H__ */
--
1.7.4
8:04 a.m.
New subject: [libvirt] [PATCH] qemu: fix regression that hangs on save failure
Regression introduced in commit 6034ddd55.
* src/qemu/qemu_driver.c (qemudDomainSaveFlag): Jump to correct
label.
---
Aargh - I introduced a regression that hangs libvirtd due to an
inadvertent change in label names (copied and pasted from
doCoreDump, where the correct label was indeed named cleanup).
- if (rc < 0)
- goto endjob;
-
+ /* Perform the migration */
+ if (qemuMigrationToFile(driver, vm, qemuCaps, fd, offset, path,
+ qemuCompressProgramName(compressed),
+ is_reg, bypassSecurityDriver) < 0)
+ goto cleanup;
By jumping to the wrong label, a failed save attempt never released
the job condition, locking out all future operations on that domain.
Pushing this under the trivial rule.
src/qemu/qemu_driver.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 182b0fd..e97c1d9 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1990,10 +1990,10 @@ static int qemudDomainSaveFlag(struct qemud_driver *driver,
virDomainPtr dom,
if (qemuMigrationToFile(driver, vm, qemuCaps, fd, offset, path,
qemuCompressProgramName(compressed),
is_reg, bypassSecurityDriver) < 0)
- goto cleanup;
+ goto endjob;
if (VIR_CLOSE(fd) < 0) {
virReportSystemError(errno, _("unable to close %s"), path);
- goto cleanup;
+ goto endjob;
}
ret = 0;
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 12/13] qemu: skip granting access during fd migration
SELinux labeling and cgroup ACLs aren't required if we hand a
pre-opened fd to qemu. All the more reason to love fd: migration.
* src/qemu/qemu_migration.c (qemuMigrationToFile): Skip steps
that are irrelevant in fd migration.
---
v5: rebase to changes in 11/13,
src/qemu/qemu_migration.c | 63 ++++++++++++++++++++++++++------------------
1 files changed, 37 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 592ffe2..2803de6 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1299,36 +1299,49 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr
vm,
int rc;
bool restoreLabel = false;
- if (!is_reg &&
- qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
- if (virCgroupForDomain(driver->cgroup, vm->def->name,
- &cgroup, 0) != 0) {
- qemuReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to find cgroup for %s"),
- vm->def->name);
- goto cleanup;
+ if (qemuCaps && qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) &&
+ !compressor) {
+ /* All right! We can use fd migration, which means that qemu
+ * doesn't have to open() the file, so we don't have to futz
+ * around with granting access or revoking it later. */
+ is_reg = true;
+ bypassSecurityDriver = true;
+ } else {
+ /* Phooey - we have to fall back on exec migration, where qemu
+ * has to popen() the file by name. We might also stumble on
+ * a race present in some qemu versions where it does a wait()
+ * that botches pclose. */
+ if (!is_reg &&
+ qemuCgroupControllerActive(driver,
+ VIR_CGROUP_CONTROLLER_DEVICES)) {
+ if (virCgroupForDomain(driver->cgroup, vm->def->name,
+ &cgroup, 0) != 0) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Unable to find cgroup for %s"),
+ vm->def->name);
+ goto cleanup;
+ }
+ rc = virCgroupAllowDevicePath(cgroup, path,
+ VIR_CGROUP_DEVICE_RW);
+ qemuAuditCgroupPath(vm, cgroup, "allow", path, "rw",
rc);
+ if (rc < 0) {
+ virReportSystemError(-rc,
+ _("Unable to allow device %s for %s"),
+ path, vm->def->name);
+ goto cleanup;
+ }
}
- rc = virCgroupAllowDevicePath(cgroup, path,
- VIR_CGROUP_DEVICE_RW);
- qemuAuditCgroupPath(vm, cgroup, "allow", path, "rw", rc);
- if (rc < 0) {
- virReportSystemError(-rc,
- _("Unable to allow device %s for %s"),
- path, vm->def->name);
+ if ((!bypassSecurityDriver) &&
+ virSecurityManagerSetSavedStateLabel(driver->securityManager,
+ vm, path) < 0)
goto cleanup;
- }
+ restoreLabel = true;
}
- if ((!bypassSecurityDriver) &&
- virSecurityManagerSetSavedStateLabel(driver->securityManager,
- vm, path) < 0)
- goto cleanup;
- restoreLabel = true;
-
+ qemuDomainObjEnterMonitorWithDriver(driver, vm);
if (!compressor) {
const char *args[] = { "cat", NULL };
- qemuDomainObjEnterMonitorWithDriver(driver, vm);
if (qemuCaps && qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)
&&
priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
rc = qemuMonitorMigrateToFd(priv->mon,
@@ -1339,7 +1352,6 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr
vm,
QEMU_MONITOR_MIGRATE_BACKGROUND,
args, path, offset);
}
- qemuDomainObjExitMonitorWithDriver(driver, vm);
} else {
const char *prog = compressor;
const char *args[] = {
@@ -1347,12 +1359,11 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr
vm,
"-c",
NULL
};
- qemuDomainObjEnterMonitorWithDriver(driver, vm);
rc = qemuMonitorMigrateToFile(priv->mon,
QEMU_MONITOR_MIGRATE_BACKGROUND,
args, path, offset);
- qemuDomainObjExitMonitorWithDriver(driver, vm);
}
+ qemuDomainObjExitMonitorWithDriver(driver, vm);
if (rc < 0)
goto cleanup;
--
1.7.4
9:52 p.m.
New subject: [libvirt] [PATCHv5 13/13] qemu: support fd: migration with compression
Spawn the compressor ourselves, instead of requiring the shell.
* src/qemu/qemu_migration.c (qemuMigrationToFile): Spawn
compression helper process when needed.
---
v5: rebase to changes in 11/13, add a cloexec, rely on recent
virCommand changes to actually clean up the intermediate compressor
process, actually test this time
It's a minor shame that virCommandSetOutputFD can create an output
pipe and return the fd, but virCommandSetInputFD cannot do likewise
(this patch has to create its own pipe). On the other hand, by
creating our own pipe, this patch didn't fall foul of the cloexec bug
that had to first be fixed on the restore path in 2/13.
src/qemu/qemu_migration.c | 38 ++++++++++++++++++++++++++++++++++----
1 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 2803de6..98b9d01 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1298,9 +1298,11 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr
vm,
int ret = -1;
int rc;
bool restoreLabel = false;
+ virCommandPtr cmd = NULL;
+ int pipeFD[2] = { -1, -1 };
if (qemuCaps && qemuCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) &&
- !compressor) {
+ (!compressor || pipe(pipeFD) == 0)) {
/* All right! We can use fd migration, which means that qemu
* doesn't have to open() the file, so we don't have to futz
* around with granting access or revoking it later. */
@@ -1359,9 +1361,31 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr
vm,
"-c",
NULL
};
- rc = qemuMonitorMigrateToFile(priv->mon,
- QEMU_MONITOR_MIGRATE_BACKGROUND,
- args, path, offset);
+ if (pipeFD[0] != -1) {
+ cmd = virCommandNewArgs(args);
+ virCommandSetInputFD(cmd, pipeFD[0]);
+ virCommandSetOutputFD(cmd, &fd);
+ if (virSetCloseExec(pipeFD[1]) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to set cloexec flag"));
+ qemuDomainObjExitMonitorWithDriver(driver, vm);
+ goto cleanup;
+ }
+ if (virCommandRunAsync(cmd, NULL) < 0) {
+ qemuDomainObjExitMonitorWithDriver(driver, vm);
+ goto cleanup;
+ }
+ rc = qemuMonitorMigrateToFd(priv->mon,
+ QEMU_MONITOR_MIGRATE_BACKGROUND,
+ pipeFD[1]);
+ if (VIR_CLOSE(pipeFD[0]) < 0 ||
+ VIR_CLOSE(pipeFD[1]) < 0)
+ VIR_WARN0("failed to close intermediate pipe");
+ } else {
+ rc = qemuMonitorMigrateToFile(priv->mon,
+ QEMU_MONITOR_MIGRATE_BACKGROUND,
+ args, path, offset);
+ }
}
qemuDomainObjExitMonitorWithDriver(driver, vm);
@@ -1373,9 +1397,15 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr
vm,
if (rc < 0)
goto cleanup;
+ if (cmd && virCommandWait(cmd, NULL) < 0)
+ goto cleanup;
+
ret = 0;
cleanup:
+ VIR_FORCE_CLOSE(pipeFD[0]);
+ VIR_FORCE_CLOSE(pipeFD[1]);
+ virCommandFree(cmd);
if (restoreLabel && (!bypassSecurityDriver) &&
virSecurityManagerRestoreSavedStateLabel(driver->securityManager,
vm, path) < 0)
--
1.7.4
10:01 p.m.
On Sat, Mar 26, 2011 at 06:52:29AM -0600, Eric Blake wrote:
This addresses the comments raised during v4:
https://www.redhat.com/archives/libvir-list/2011-March/msg00421.html
More comments in individual patches.
It could still use a bit more testing with root-squash NFS, and I'm
also hitting a problem where if I run daemon/libvirtd myself, I
get a SELinux error:
error: unable to set security context
'system_u:object_r:svirt_image_t:s0:c80,c237' on fd 23: Permission denied
but if I run the system service libvirtd or SELinux permissive, things
work. Somehow, the attempt to set the fd SELinux label on a pipe is
not working when libvirt is started as an unconfined process (that is,
the fd has label
unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023) but when
started as a daemon, SELinux is happy to allow the transition. I
suspect that this is a bug in SELinux, since my understanding is that
it should always be possible to go from unconfined to something more
restrictive, but we already proved that SELinux fd labelling is
relatively unused and untested back when we first added it in commit
34a19dda.
If possible, I'd like to get this in before the 0.9.0 freeze, and we
can fix any fallout from testing during the freeze week.
Okay, go ahead, 5 iterations is a lot already, and we will clean
things up as they go later. Reviewing giant patch series ain't fun
for anybody (wild guess on my part :-) , and reviewing the fixes
is preferable now,
ACK
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
1:59 a.m.
On 03/28/2011 07:01 AM, Daniel Veillard wrote:
On Sat, Mar 26, 2011 at 06:52:29AM -0600, Eric Blake wrote:
> This addresses the comments raised during v4:
> https://www.redhat.com/archives/libvir-list/2011-March/msg00421.html
> More comments in individual patches.
>
> It could still use a bit more testing with root-squash NFS, and I'm
> also hitting a problem where if I run daemon/libvirtd myself, I
> get a SELinux error:
>
> error: unable to set security context
'system_u:object_r:svirt_image_t:s0:c80,c237' on fd 23: Permission denied
>
> but if I run the system service libvirtd or SELinux permissive, things
> work. Somehow, the attempt to set the fd SELinux label on a pipe is
> not working when libvirt is started as an unconfined process (that is,
> the fd has label
> unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023) but when
> started as a daemon, SELinux is happy to allow the transition. I
> suspect that this is a bug in SELinux, since my understanding is that
> it should always be possible to go from unconfined to something more
> restrictive, but we already proved that SELinux fd labelling is
> relatively unused and untested back when we first added it in commit
> 34a19dda.
>
> If possible, I'd like to get this in before the 0.9.0 freeze, and we
> can fix any fallout from testing during the freeze week.
Okay, go ahead, 5 iterations is a lot already, and we will clean
things up as they go later. Reviewing giant patch series ain't fun
for anybody (wild guess on my part :-) , and reviewing the fixes
is preferable now,
ACK
Thanks. Series pushed, and I'm now trying to track down why I get that
SELinux failure when run from an unconfined shell but not when run as a
system service.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5121
days inactive
5123
days old
16 comments
2 participants
participants (2)
-
Daniel Veillard -
Eric Blake