This is a pure code movement. Both of the APIs are going to be needed later in several areas of qemu driver code. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_domain.c | 147 ++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 10 ++++ src/qemu/qemu_driver.c | 156 ------------------------------------------------- 3 files changed, 157 insertions(+), 156 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 6513c78..e500fb3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2810,3 +2810,150 @@ qemuDomainAgentAvailable(qemuDomainObjPrivatePtr priv, } return true; } + +/* Internal function to properly create or open existing files, with + * ownership affected by qemu driver setup and domain DAC label. */ +int +qemuOpenFile(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, int oflags, + bool *needUnlink, bool *bypassSecurityDriver) +{ + int ret = -1; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + uid_t user = cfg->user; + gid_t group = cfg->group; + bool dynamicOwnership = cfg->dynamicOwnership; + virSecurityLabelDefPtr seclabel; + + virObjectUnref(cfg); + + /* TODO: Take imagelabel into account? */ + if (vm && + (seclabel = virDomainDefGetSecurityLabelDef(vm->def, "dac")) != NULL && + seclabel->label != NULL && + (virParseOwnershipIds(seclabel->label, &user, &group) < 0)) + goto cleanup; + + ret = qemuOpenFileAs(user, group, dynamicOwnership, + path, oflags, needUnlink, bypassSecurityDriver); + + cleanup: + return ret; +} + + +int +qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, + bool dynamicOwnership, + const char *path, int oflags, + bool *needUnlink, bool *bypassSecurityDriver) +{ + struct stat sb; + bool is_reg = true; + bool need_unlink = false; + bool bypass_security = false; + unsigned int vfoflags = 0; + int fd = -1; + int path_shared = virFileIsSharedFS(path); + uid_t uid = geteuid(); + gid_t gid = getegid(); + + /* 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; + + /* Don't force chown on network-shared FS + * as it is likely to fail. */ + if (path_shared <= 0 || dynamicOwnership) + vfoflags |= VIR_FILE_OPEN_FORCE_OWNER; + + 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 && !dynamicOwnership) { + uid = sb.st_uid; + gid = sb.st_gid; + } + } + } + + /* First try creating the file as root */ + if (!is_reg) { + if ((fd = open(path, oflags & ~O_CREAT)) < 0) { + fd = -errno; + goto error; + } + } else { + if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid, + vfoflags | VIR_FILE_OPEN_NOFORK)) < 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 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) || fallback_uid == geteuid()) + goto error; + + /* On Linux we can also verify the FS-type of the directory. */ + switch (path_shared) { + case 1: + /* it was on a network share, so we'll continue + * as outlined above + */ + break; + + case -1: + virReportSystemError(-fd, oflags & O_CREAT + ? _("Failed to create file " + "'%s': couldn't determine fs type") + : _("Failed to open file " + "'%s': couldn't determine fs type"), + path); + goto cleanup; + + case 0: + default: + /* local file - log the error returned by virFileOpenAs */ + goto error; + } + + /* Retry creating the file as qemu user */ + + if ((fd = virFileOpenAs(path, oflags, + S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, + fallback_uid, fallback_gid, + vfoflags | VIR_FILE_OPEN_FORK)) < 0) { + virReportSystemError(-fd, oflags & O_CREAT + ? _("Error from child process creating '%s'") + : _("Error from child process opening '%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; + + error: + virReportSystemError(-fd, oflags & O_CREAT + ? _("Failed to create file '%s'") + : _("Failed to open file '%s'"), + path); + goto cleanup; +} diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index e4ea4ce..5f36892 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -414,4 +414,14 @@ int qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +int qemuOpenFile(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *path, int oflags, + bool *needUnlink, bool *bypassSecurityDriver); + +int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, + bool dynamicOwnership, + const char *path, int oflags, + bool *needUnlink, bool *bypassSecurityDriver); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 07da3e3..561fa6c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -160,16 +160,6 @@ static int qemuDomainGetMaxVcpus(virDomainPtr dom); static int qemuDomainManagedSaveLoad(virDomainObjPtr vm, void *opaque); -static int qemuOpenFile(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *path, int oflags, - bool *needUnlink, bool *bypassSecurityDriver); - -static int qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, - bool dynamicOwnership, - const char *path, int oflags, - bool *needUnlink, bool *bypassSecurityDriver); - virQEMUDriverPtr qemu_driver = NULL; @@ -2881,152 +2871,6 @@ qemuCompressGetCommand(virQEMUSaveFormat compression) return ret; } -/* Internal function to properly create or open existing files, with - * ownership affected by qemu driver setup and domain DAC label. */ -static int -qemuOpenFile(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *path, int oflags, - bool *needUnlink, bool *bypassSecurityDriver) -{ - int ret = -1; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - uid_t user = cfg->user; - gid_t group = cfg->group; - bool dynamicOwnership = cfg->dynamicOwnership; - virSecurityLabelDefPtr seclabel; - - virObjectUnref(cfg); - - /* TODO: Take imagelabel into account? */ - if (vm && - (seclabel = virDomainDefGetSecurityLabelDef(vm->def, "dac")) != NULL && - seclabel->label != NULL && - (virParseOwnershipIds(seclabel->label, &user, &group) < 0)) - goto cleanup; - - ret = qemuOpenFileAs(user, group, dynamicOwnership, - path, oflags, needUnlink, bypassSecurityDriver); - - cleanup: - return ret; -} - -static int -qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, - bool dynamicOwnership, - const char *path, int oflags, - bool *needUnlink, bool *bypassSecurityDriver) -{ - struct stat sb; - bool is_reg = true; - bool need_unlink = false; - bool bypass_security = false; - unsigned int vfoflags = 0; - int fd = -1; - int path_shared = virFileIsSharedFS(path); - uid_t uid = geteuid(); - gid_t gid = getegid(); - - /* 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; - - /* Don't force chown on network-shared FS - * as it is likely to fail. */ - if (path_shared <= 0 || dynamicOwnership) - vfoflags |= VIR_FILE_OPEN_FORCE_OWNER; - - 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 && !dynamicOwnership) { - uid = sb.st_uid; - gid = sb.st_gid; - } - } - } - - /* First try creating the file as root */ - if (!is_reg) { - if ((fd = open(path, oflags & ~O_CREAT)) < 0) { - fd = -errno; - goto error; - } - } else { - if ((fd = virFileOpenAs(path, oflags, S_IRUSR | S_IWUSR, uid, gid, - vfoflags | VIR_FILE_OPEN_NOFORK)) < 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 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) || fallback_uid == geteuid()) - goto error; - - /* On Linux we can also verify the FS-type of the directory. */ - switch (path_shared) { - case 1: - /* it was on a network share, so we'll continue - * as outlined above - */ - break; - - case -1: - virReportSystemError(-fd, oflags & O_CREAT - ? _("Failed to create file " - "'%s': couldn't determine fs type") - : _("Failed to open file " - "'%s': couldn't determine fs type"), - path); - goto cleanup; - - case 0: - default: - /* local file - log the error returned by virFileOpenAs */ - goto error; - } - - /* Retry creating the file as qemu user */ - - if ((fd = virFileOpenAs(path, oflags, - S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP, - fallback_uid, fallback_gid, - vfoflags | VIR_FILE_OPEN_FORK)) < 0) { - virReportSystemError(-fd, oflags & O_CREAT - ? _("Error from child process creating '%s'") - : _("Error from child process opening '%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; - - error: - virReportSystemError(-fd, oflags & O_CREAT - ? _("Failed to create file '%s'") - : _("Failed to open file '%s'"), - path); - goto cleanup; -} - /* Helper function to execute a migration to file with a correct save header * the caller needs to make sure that the processors are stopped and do all other * actions besides saving memory */ -- 2.0.4

While this could be exposed as a public API, it's not done yet as there's no demand for that yet. Anyway, this is just preparing the environment for easier volume creation on the destination. Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/storage/storage_driver.c | 36 ++++++++++++++++++++++++++++++++++++ src/storage/storage_driver.h | 4 ++++ 2 files changed, 40 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 23b63f5..a4f1030 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1466,6 +1466,42 @@ storageVolLookupByPath(virConnectPtr conn, return ret; } +virStoragePoolPtr +storagePoolLookupByPath(virConnectPtr conn, + const char *path) +{ + size_t i; + virStoragePoolPtr ret = NULL; + char *cleanpath; + + cleanpath = virFileSanitizePath(path); + if (!cleanpath) + return NULL; + + storageDriverLock(); + for (i = 0; i < driver->pools.count && !ret; i++) { + virStoragePoolObjPtr pool = driver->pools.objs[i]; + + virStoragePoolObjLock(pool); + + if (!virStoragePoolObjIsActive(pool)) { + virStoragePoolObjUnlock(pool); + continue; + } + + if (STREQ(path, pool->def->target.path)) { + ret = virGetStoragePool(conn, pool->def->name, pool->def->uuid, + NULL, NULL); + } + + virStoragePoolObjUnlock(pool); + } + + VIR_FREE(cleanpath); + storageDriverUnlock(); + return ret; +} + static int storageVolDeleteInternal(virStorageVolPtr obj, diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index b805ddd..b51760a 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -57,6 +57,10 @@ int virStorageFileGetMetadata(virStorageSourcePtr src, int virStorageTranslateDiskSourcePool(virConnectPtr conn, virDomainDiskDefPtr def); +virStoragePoolPtr +storagePoolLookupByPath(virConnectPtr conn, + const char *path); + int storageRegister(void); #endif /* __VIR_STORAGE_DRIVER_H__ */ -- 2.0.4

On 11/27/14 14:55, Michal Privoznik wrote: I think this already warrants usage of a struct to group them together. Allocation and capacity are two different stats of a disk volume. You should stick with capacity here to avoid confusion Since you are requesting this only for a live domain and the only stat you care about is capacity of the guest visible portion of the disk it would be better to use a monitor call to determine the data from the disk rather than use the weird function that parses the information from the file on the disk. Allocing an array of strutcts would look nicer. I think also networked disks don't make sense here. We also have a function (virStorageSourceIsEmpty or similar) to check whether the source doesn't denote an empty drive. One additional thing that might break is if some of the storage is on a shared storage system, while other is not. We report errors by default. No need to note it explicitly. Again ... capacity is a better word for this IMO. capacity ... Peter