[libvirt] [PATCH 3/1] qemu: refactor file opening
Eric Blake
eblake at redhat.com
Tue Aug 30 21:04:48 UTC 2011
In a SELinux or root-squashing NFS environment, libvirt has to go
through some hoops to create a new file that qemu can then open()
by name. Snapshots are a case where we want to guarantee an empty
file that qemu can open; also, reopening a save file to convert it
from being marked partial to complete requires a reopen to avoid
O_DIRECT headaches. Refactor some existing code to make it easier
to reuse in later patches.
* src/qemu/qemu_migration.h (qemuMigrationToFile): Drop parameter.
* src/qemu/qemu_migration.c (qemuMigrationToFile): Let cgroup do
the stat, rather than asking caller to do it and pass info down.
* src/qemu/qemu_driver.c (qemuOpenFile): New function, pulled from...
(qemuDomainSaveInternal): ...here.
(doCoreDump, qemuDomainSaveImageOpen): Use it here as well.
---
Originally posted here:
https://www.redhat.com/archives/libvir-list/2011-August/msg01174.html
but just a bit more tweaking made it useful for other purposes.
src/qemu/qemu_driver.c | 248 ++++++++++++++++++++++----------------------
src/qemu/qemu_migration.c | 13 ++-
src/qemu/qemu_migration.h | 2 +-
3 files changed, 133 insertions(+), 130 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4e8c691..9cb034b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2191,6 +2191,113 @@ qemuCompressProgramName(int compress)
qemudSaveCompressionTypeToString(compress));
}
+/* Internal function to properly create or open existing files, with
+ * ownership affected by qemu driver setup. */
+static int
+qemuOpenFile(struct qemud_driver *driver, const char *path, int oflags,
+ bool *needUnlink, bool *bypassSecurityDriver)
+{
+ struct stat sb;
+ bool is_reg = true;
+ bool need_unlink = false;
+ bool bypass_security = false;
+ int fd = -1;
+ uid_t uid = getuid();
+ gid_t gid = getgid();
+
+ /* path might be a pre-existing block dev, in which case
+ * we need to skip the create step, and also avoid unlink
+ * in the failure case */
+ if (oflags & O_CREAT) {
+ need_unlink = true;
+ if (stat(path, &sb) == 0) {
+ is_reg = !!S_ISREG(sb.st_mode);
+ /* If the path is regular file which exists
+ * already and dynamic_ownership is off, we don't
+ * want to change it's ownership, just open it as-is */
+ if (is_reg && !driver->dynamicOwnership) {
+ uid = sb.st_uid;
+ gid = sb.st_gid;
+ }
+ }
+ }
+
+ /* First try creating the file as root */
+ if (!is_reg) {
+ fd = open(path, oflags & ~O_CREAT);
+ if (fd < 0) {
+ virReportSystemError(errno, _("unable to open %s"), path);
+ goto cleanup;
+ }
+ } else {
+ if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR,
+ uid, gid, 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
+ 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 */
+ if ((fd != -EACCES && fd != -EPERM) ||
+ driver->user == getuid()) {
+ virReportSystemError(-fd,
+ _("Failed to create file '%s'"),
+ path);
+ goto cleanup;
+ }
+
+ /* On Linux we can also verify the FS-type of the directory. */
+ switch (virStorageFileIsSharedFS(path)) {
+ case 1:
+ /* it was on a network share, so we'll continue
+ * as outlined above
+ */
+ break;
+
+ case -1:
+ virReportSystemError(errno,
+ _("Failed to create file "
+ "'%s': couldn't determine fs type"),
+ path);
+ goto cleanup;
+
+ case 0:
+ default:
+ /* local file - log the error returned by virFileOpenAs */
+ virReportSystemError(-fd,
+ _("Failed to create file '%s'"),
+ path);
+ goto cleanup;
+ }
+
+ /* Retry creating the file as driver->user */
+
+ if ((fd = virFileOpenAs(path, oflags,
+ 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);
+ goto cleanup;
+ }
+
+ /* Since we had to setuid to create the file, and the fstype
+ is NFS, we assume it's a root-squashing NFS share, and that
+ the security driver stuff would have failed anyway */
+
+ bypass_security = true;
+ }
+ }
+cleanup:
+ if (needUnlink)
+ *needUnlink = need_unlink;
+ if (bypassSecurityDriver)
+ *bypassSecurityDriver = bypass_security;
+
+ return fd;
+}
+
/* This internal function expects the driver lock to already be held on
* entry and the vm must be active + locked. Vm will be unlocked and
* potentially free'd after this returns (eg transient VMs are freed
@@ -2209,14 +2316,11 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom,
int rc;
virDomainEventPtr event = NULL;
qemuDomainObjPrivatePtr priv;
- struct stat sb;
- bool is_reg = false;
+ bool needUnlink = false;
size_t len;
unsigned long long offset;
unsigned long long pad;
int fd = -1;
- uid_t uid = getuid();
- gid_t gid = getgid();
int directFlag = 0;
virFileDirectFdPtr directFd = NULL;
@@ -2304,107 +2408,18 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom,
header.xml_len = len;
/* Obtain the file handle. */
- /* path might be a pre-existing block dev, in which case
- * we need to skip the create step, and also avoid unlink
- * in the failure case */
- if (stat(path, &sb) < 0) {
- /* Avoid throwing an error here, since it is possible
- * that with NFS we can't actually stat() the file.
- * The subsequent codepaths will still raise an error
- * if a truly fatal problem is hit */
- is_reg = true;
- } else {
- is_reg = !!S_ISREG(sb.st_mode);
- /* If the path is regular file which exists
- * already and dynamic_ownership is off, we don't
- * want to change it's ownership, just open it as-is */
- if (is_reg && !driver->dynamicOwnership) {
- uid=sb.st_uid;
- gid=sb.st_gid;
- }
- }
-
- /* First try creating the file as root */
if (bypass_cache) {
directFlag = virFileDirectFdFlag();
if (directFlag < 0) {
qemuReportError(VIR_ERR_OPERATION_FAILED, "%s",
_("bypass cache unsupported by this system"));
- goto endjob;
- }
- }
- if (!is_reg) {
- fd = open(path, O_WRONLY | O_TRUNC | directFlag);
- if (fd < 0) {
- virReportSystemError(errno, _("unable to open %s"), path);
- goto endjob;
- }
- } else {
- int oflags = O_CREAT | O_TRUNC | O_WRONLY | directFlag;
- if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR,
- uid, gid, 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
- 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'"),
- path);
- goto endjob;
- }
-
- /* On Linux we can also verify the FS-type of the directory. */
- switch (virStorageFileIsSharedFS(path)) {
- case 1:
- /* it was on a network share, so we'll continue
- * as outlined above
- */
- break;
-
- case -1:
- virReportSystemError(errno,
- _("Failed to create domain save file "
- "'%s': couldn't determine fs type"),
- path);
- goto endjob;
- break;
-
- case 0:
- default:
- /* local file - log the error returned by virFileOpenAs */
- virReportSystemError(-rc,
- _("Failed to create domain save file '%s'"),
- path);
- goto endjob;
- break;
-
- }
-
- /* Retry creating the file as driver->user */
-
- if ((fd = virFileOpenAs(path, oflags,
- 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);
- goto endjob;
- }
-
- /* Since we had to setuid to create the file, and the fstype
- is NFS, we assume it's a root-squashing NFS share, and that
- the security driver stuff would have failed anyway */
-
- bypassSecurityDriver = true;
+ goto cleanup;
}
}
-
+ fd = qemuOpenFile(driver, path, O_WRONLY | O_TRUNC | O_CREAT | directFlag,
+ &needUnlink, &bypassSecurityDriver);
+ if (fd < 0)
+ goto endjob;
if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL)
goto endjob;
@@ -2417,7 +2432,7 @@ qemuDomainSaveInternal(struct qemud_driver *driver, virDomainPtr dom,
/* Perform the migration */
if (qemuMigrationToFile(driver, vm, fd, offset, path,
qemuCompressProgramName(compressed),
- is_reg, bypassSecurityDriver,
+ bypassSecurityDriver,
QEMU_ASYNC_JOB_SAVE) < 0)
goto endjob;
if (VIR_CLOSE(fd) < 0) {
@@ -2461,7 +2476,7 @@ cleanup:
VIR_FORCE_CLOSE(fd);
virFileDirectFdFree(directFd);
VIR_FREE(xml);
- if (ret != 0 && is_reg)
+ if (ret != 0 && needUnlink)
unlink(path);
if (event)
qemuDomainEventQueue(driver, event);
@@ -2705,18 +2720,19 @@ doCoreDump(struct qemud_driver *driver,
goto cleanup;
}
}
- if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | directFlag,
- S_IRUSR | S_IWUSR)) < 0) {
- qemuReportError(VIR_ERR_OPERATION_FAILED,
- _("failed to create '%s'"), path);
+ /* Core dumps usually imply last-ditch analysis efforts are
+ * desired, so we intentionally do not unlink even if a file was
+ * created. */
+ if ((fd = qemuOpenFile(driver, path,
+ O_CREAT | O_TRUNC | O_WRONLY | directFlag,
+ NULL, NULL)) < 0)
goto cleanup;
- }
if (bypass_cache && (directFd = virFileDirectFdNew(&fd, path)) == NULL)
goto cleanup;
if (qemuMigrationToFile(driver, vm, fd, 0, path,
- qemuCompressProgramName(compress), true, false,
+ qemuCompressProgramName(compress), false,
QEMU_ASYNC_JOB_DUMP) < 0)
goto cleanup;
@@ -3778,25 +3794,9 @@ qemuDomainSaveImageOpen(struct qemud_driver *driver,
}
oflags |= directFlag;
}
- if ((fd = virFileOpenAs(path, oflags, 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. */
- if ((fd = virFileOpenAs(path, oflags, 0,
- driver->user, driver->group,
- VIR_FILE_OPEN_AS_UID)) < 0) {
- qemuReportError(VIR_ERR_OPERATION_FAILED,
- "%s", _("cannot read domain image"));
- goto error;
- }
- }
+ if ((fd = qemuOpenFile(driver, path, oflags, NULL, NULL)) < 0)
+ goto error;
if (bypass_cache && (*directFd = virFileDirectFdNew(&fd, path)) == NULL)
goto error;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 38b05a9..d239cc8 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2690,7 +2690,7 @@ int
qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm,
int fd, off_t offset, const char *path,
const char *compressor,
- bool is_reg, bool bypassSecurityDriver,
+ bool bypassSecurityDriver,
enum qemuDomainAsyncJob asyncJob)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
@@ -2713,11 +2713,11 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm,
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
+ * has to popen() the file by name, and block devices have to be
+ * given cgroup ACL permission. 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,
+ if (qemuCgroupControllerActive(driver,
VIR_CGROUP_CONTROLLER_DEVICES)) {
if (virCgroupForDomain(driver->cgroup, vm->def->name,
&cgroup, 0) != 0) {
@@ -2729,7 +2729,10 @@ qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm,
rc = virCgroupAllowDevicePath(cgroup, path,
VIR_CGROUP_DEVICE_RW);
virDomainAuditCgroupPath(vm, cgroup, "allow", path, "rw", rc);
- if (rc < 0) {
+ if (rc == 1) {
+ /* path was not a device, no further need for cgroup */
+ virCgroupFree(&cgroup);
+ } else if (rc < 0) {
virReportSystemError(-rc,
_("Unable to allow device %s for %s"),
path, vm->def->name);
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index 5c6921d..ace411d 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
@@ -143,7 +143,7 @@ int qemuMigrationConfirm(struct qemud_driver *driver,
int qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm,
int fd, off_t offset, const char *path,
const char *compressor,
- bool is_reg, bool bypassSecurityDriver,
+ bool bypassSecurityDriver,
enum qemuDomainAsyncJob asyncJob)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5)
ATTRIBUTE_RETURN_CHECK;
--
1.7.4.4
More information about the libvir-list
mailing list