Add backends for this frontend function so that we can use it in the snapshot creation code. --- src/storage/storage_backend_fs.c | 17 +++++++++++++++++ src/storage/storage_backend_gluster.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index c93fc1e..bd5cab8 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1367,6 +1367,22 @@ virStorageFileBackendFileInit(virStorageSourcePtr src) static int +virStorageFileBackendFileCreate(virStorageSourcePtr src) +{ + int fd = -1; + + if ((fd = virFileOpenAs(src->path, O_WRONLY | O_TRUNC | O_CREAT, 0, + src->drv->uid, src->drv->gid, 0)) < 0) { + errno = -fd; + return -1; + } + + VIR_FORCE_CLOSE(fd); + return 0; +} + + +static int virStorageFileBackendFileUnlink(virStorageSourcePtr src) { return unlink(src->path); @@ -1441,6 +1457,7 @@ virStorageFileBackend virStorageFileBackendFile = { .backendInit = virStorageFileBackendFileInit, .backendDeinit = virStorageFileBackendFileDeinit, + .storageFileCreate = virStorageFileBackendFileCreate, .storageFileUnlink = virStorageFileBackendFileUnlink, .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index cab23b0..6c94d61 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -623,6 +623,33 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) static int +virStorageFileBackendGlusterCreate(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + glfs_fd_t *fd = NULL; + int save_errno; + int ret = -1; + + if (!(fd = glfs_open(priv->vol, src->path, O_CREAT | O_TRUNC | O_WRONLY))) + return -1; + + if (src->drv->uid != 0 || src->drv->gid != 0) { + if (glfs_fchown(fd, src->drv->uid, src->drv->gid) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + save_errno = errno; + ignore_value(glfs_close(fd)); + errno = save_errno; + + return ret; +} + + +static int virStorageFileBackendGlusterUnlink(virStorageSourcePtr src) { virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; @@ -797,6 +824,7 @@ virStorageFileBackend virStorageFileBackendGluster = { .backendInit = virStorageFileBackendGlusterInit, .backendDeinit = virStorageFileBackendGlusterDeinit, + .storageFileCreate = virStorageFileBackendGlusterCreate, .storageFileUnlink = virStorageFileBackendGlusterUnlink, .storageFileStat = virStorageFileBackendGlusterStat, .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, -- 1.9.3

Add a wrapper that determines the correct uid and gid for a certain storage file and domain. --- src/qemu/qemu_domain.c | 23 +++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 27 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 37b28ab..cecb7c4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2430,6 +2430,29 @@ qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, int +qemuDomainStorageFileInit(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src) +{ + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + uid_t uid; + gid_t gid; + int ret = -1; + + qemuDomainGetImageIds(cfg, vm, src, &uid, &gid); + + if (virStorageFileInitAs(src, uid, gid) < 0) + goto cleanup; + + ret = 0; + + cleanup: + virObjectUnref(cfg); + return ret; +} + + +int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 3bda446..67972b9 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -353,6 +353,10 @@ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, virDomainDiskDefPtr disk, bool force); +int qemuDomainStorageFileInit(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virStorageSourcePtr src); + int qemuDomainCleanupAdd(virDomainObjPtr vm, qemuDomainCleanupCallback cb); void qemuDomainCleanupRemove(virDomainObjPtr vm, -- 1.9.3

Some of the further changes will propagate seclabels from a disk source element into the backing store elements. This would change the XML output of the backing store as the seclabels would be formatted for each backing store element. Skip the seclabels formatting until we decide that it's necessary. --- src/conf/domain_conf.c | 59 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index b7aa4f5..da9dd04 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14871,14 +14871,15 @@ virDomainDiskBlockIoDefFormat(virBufferPtr buf, * possible seclabels. */ static void -virDomainSourceDefFormatSeclabel(virBufferPtr buf, - size_t nseclabels, - virSecurityDeviceLabelDefPtr *seclabels, - unsigned int flags) +virDomainDiskSourceDefFormatSeclabel(virBufferPtr buf, + size_t nseclabels, + virSecurityDeviceLabelDefPtr *seclabels, + unsigned int flags, + bool skipSeclables) { size_t n; - if (nseclabels) { + if (nseclabels && !skipSeclables) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); for (n = 0; n < nseclabels; n++) @@ -14890,11 +14891,21 @@ virDomainSourceDefFormatSeclabel(virBufferPtr buf, } } -int -virDomainDiskSourceFormat(virBufferPtr buf, - virStorageSourcePtr src, - int policy, - unsigned int flags) +static void +virDomainSourceDefFormatSeclabel(virBufferPtr buf, + size_t nseclabels, + virSecurityDeviceLabelDefPtr *seclabels, + unsigned int flags) +{ + virDomainDiskSourceDefFormatSeclabel(buf, nseclabels, seclabels, flags, false); +} + +static int +virDomainDiskSourceFormatInternal(virBufferPtr buf, + virStorageSourcePtr src, + int policy, + unsigned int flags, + bool skipSeclabels) { size_t n; char *path = NULL; @@ -14910,8 +14921,9 @@ virDomainDiskSourceFormat(virBufferPtr buf, virBufferEscapeString(buf, " file='%s'", src->path); virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy); - virDomainSourceDefFormatSeclabel(buf, src->nseclabels, - src->seclabels, flags); + virDomainDiskSourceDefFormatSeclabel(buf, src->nseclabels, + src->seclabels, flags, + skipSeclabels); break; case VIR_STORAGE_TYPE_BLOCK: @@ -14919,8 +14931,9 @@ virDomainDiskSourceFormat(virBufferPtr buf, virBufferEscapeString(buf, " dev='%s'", src->path); virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy); - virDomainSourceDefFormatSeclabel(buf, src->nseclabels, - src->seclabels, flags); + virDomainDiskSourceDefFormatSeclabel(buf, src->nseclabels, + src->seclabels, flags, + skipSeclabels); break; case VIR_STORAGE_TYPE_DIR: @@ -14983,8 +14996,9 @@ virDomainDiskSourceFormat(virBufferPtr buf, } virBufferEscapeString(buf, " startupPolicy='%s'", startupPolicy); - virDomainSourceDefFormatSeclabel(buf, src->nseclabels, - src->seclabels, flags); + virDomainDiskSourceDefFormatSeclabel(buf, src->nseclabels, + src->seclabels, flags, + skipSeclabels); break; case VIR_STORAGE_TYPE_NONE: @@ -14999,6 +15013,16 @@ virDomainDiskSourceFormat(virBufferPtr buf, } +int +virDomainDiskSourceFormat(virBufferPtr buf, + virStorageSourcePtr src, + int policy, + unsigned int flags) +{ + return virDomainDiskSourceFormatInternal(buf, src, policy, flags, false); +} + + static int virDomainDiskBackingStoreFormat(virBufferPtr buf, virStorageSourcePtr backingStore, @@ -15035,7 +15059,8 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<format type='%s'/>\n", format); - if (virDomainDiskSourceFormat(buf, backingStore, 0, 0) < 0 || + /* We currently don't output seclabels for backing chain element */ + if (virDomainDiskSourceFormatInternal(buf, backingStore, 0, 0, true) < 0 || virDomainDiskBackingStoreFormat(buf, backingStore->backingStore, backingStore->backingStoreRaw, -- 1.9.3

Now that we have pointers to store disk source information and thus can easily exchange the structs behind we need a function to copy all the data. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 207 ++++++++++++++++++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 3 + 3 files changed, 203 insertions(+), 8 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1e1dd84..1d1c5db 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1906,6 +1906,7 @@ virStorageNetProtocolTypeToString; virStorageSourceAuthClear; virStorageSourceBackingStoreClear; virStorageSourceClear; +virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 0c50de1..10bdcda 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1515,6 +1515,202 @@ virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, } +static void +virStorageSourceSeclabelsClear(virStorageSourcePtr def) +{ + size_t i; + + if (def->seclabels) { + for (i = 0; i < def->nseclabels; i++) + virSecurityDeviceLabelDefFree(def->seclabels[i]); + VIR_FREE(def->seclabels); + } +} + + +static int +virStorageSourceSeclabelsCopy(virStorageSourcePtr to, + const virStorageSource *from) +{ + size_t i; + + if (from->nseclabels == 0) + return 0; + + if (VIR_ALLOC_N(to->seclabels, from->nseclabels) < 0) + return -1; + to->nseclabels = from->nseclabels; + + for (i = 0; i < to->nseclabels; i++) { + if (!(to->seclabels[i] = virSecurityDeviceLabelDefCopy(from->seclabels[i]))) + goto error; + } + + return 0; + + error: + virStorageSourceSeclabelsClear(to); + return -1; +} + + +static virStorageTimestampsPtr +virStorageTimestampsCopy(const virStorageTimestamps *src) +{ + virStorageTimestampsPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + memcpy(ret, src, sizeof(*src)); + + return ret; +} + + +static virStoragePermsPtr +virStoragePermsCopy(const virStoragePerms *src) +{ + virStoragePermsPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->mode = src->mode; + ret->uid = src->uid; + ret->gid = src->gid; + + if (VIR_STRDUP(ret->label, src->label)) + goto error; + + return ret; + + error: + virStoragePermsFree(ret); + return NULL; +} + + +static virStorageSourcePoolDefPtr +virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) +{ + virStorageSourcePoolDefPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->voltype = src->voltype; + ret->pooltype = src->pooltype; + ret->actualtype = src->actualtype; + ret->mode = src->mode; + + if (VIR_STRDUP(ret->pool, src->pool) < 0 || + VIR_STRDUP(ret->volume, src->volume) < 0) + goto error; + + return ret; + + error: + virStorageSourcePoolDefFree(ret); + return NULL; +} + + +/** + * virStorageSourcePtr: + * + * Deep-copies a virStorageSource structure. If @backing chain is true + * then also copies the backing chain recursively, otherwise just + * the top element is copied. This function doesn't copy the + * storage driver access structure and thus the struct needs to be initialized + * separately. + */ +virStorageSourcePtr +virStorageSourceCopy(const virStorageSource *src, + bool backingChain) +{ + virStorageSourcePtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->type = src->type; + ret->protocol = src->protocol; + ret->format = src->format; + ret->allocation = src->allocation; + ret->capacity = src->capacity; + ret->readonly = src->readonly; + ret->shared = src->shared; + + /* storage driver metadata are not copied */ + ret->drv = NULL; + + if (VIR_STRDUP(ret->path, src->path) < 0 || + VIR_STRDUP(ret->volume, src->volume) < 0 || + VIR_STRDUP(ret->driverName, src->driverName) < 0 || + VIR_STRDUP(ret->relPath, src->relPath) < 0 || + VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 || + VIR_STRDUP(ret->compat, src->compat) < 0 || + VIR_STRDUP(ret->auth.username, src->auth.username) < 0) + goto error; + + if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) + goto error; + ret->nhosts = src->nhosts; + + if (src->srcpool && + !(ret->srcpool = virStorageSourcePoolDefCopy(src->srcpool))) + goto error; + + if (src->features && + !(ret->features = virBitmapNewCopy(src->features))) + goto error; + + if (src->encryption && + !(ret->encryption = virStorageEncryptionCopy(src->encryption))) + goto error; + + if (src->perms && + !(ret->perms = virStoragePermsCopy(src->perms))) + goto error; + + if (src->timestamps && + !(ret->timestamps = virStorageTimestampsCopy(src->timestamps))) + goto error; + + if (virStorageSourceSeclabelsCopy(ret, src) < 0) + goto error; + + ret->auth.secretType = src->auth.secretType; + switch ((virStorageSecretType) src->auth.secretType) { + case VIR_STORAGE_SECRET_TYPE_NONE: + case VIR_STORAGE_SECRET_TYPE_LAST: + break; + + case VIR_STORAGE_SECRET_TYPE_UUID: + memcpy(ret->auth.secret.uuid, src->auth.secret.uuid, VIR_UUID_BUFLEN); + break; + + case VIR_STORAGE_SECRET_TYPE_USAGE: + if (VIR_STRDUP(ret->auth.secret.usage, src->auth.secret.usage) < 0) + goto error; + break; + } + + if (backingChain && src->backingStore) { + if (!(ret->backingStore = virStorageSourceCopy(src->backingStore, + true))) + goto error; + } + + return ret; + + error: + virStorageSourceFree(ret); + return NULL; +} + + void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def) { @@ -1572,11 +1768,11 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) } + + void virStorageSourceClear(virStorageSourcePtr def) { - size_t i; - if (!def) return; @@ -1587,12 +1783,7 @@ virStorageSourceClear(virStorageSourcePtr def) virBitmapFree(def->features); VIR_FREE(def->compat); virStorageEncryptionFree(def->encryption); - - if (def->seclabels) { - for (i = 0; i < def->nseclabels; i++) - virSecurityDeviceLabelDefFree(def->seclabels[i]); - VIR_FREE(def->seclabels); - } + virStorageSourceSeclabelsClear(def); virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index fe17b0b..27d7c3d 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -329,6 +329,9 @@ int virStorageSourceGetActualType(virStorageSourcePtr def); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceBackingStoreClear(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); +virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, + bool backingChain) + ATTRIBUTE_NONNULL(1); typedef int (*virStorageFileSimplifyPathReadlinkCallback)(const char *path, -- 1.9.3

We are going to modify storage source chains in place. Add a helper that will copy relevant information such as security labels to the new element if that doesn't contain it. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 3 +++ 3 files changed, 44 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 122e72a..7844e2f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1910,6 +1910,7 @@ virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; +virStorageSourceInitChainElement; virStorageSourceIsLocalStorage; virStorageSourceNewFromBacking; virStorageSourcePoolDefFree; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 330ba27..e27384d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1711,6 +1711,46 @@ virStorageSourceCopy(const virStorageSource *src, } +/** + * virStorageSourceInitChainElement: + * @newelem: New backing chain element disk source + * @old: Existing top level disk source + * @force: Force-copy the information + * + * Transfers relevant information from the existing disk source to the new + * backing chain element if they weren't supplied so that labelling info + * and possibly other stuff is correct. + * + * If @force is true, user-supplied information for the new backing store + * element is overwritten from @old instead of keeping it. + * + * Returns 0 on success, -1 on error. + */ +int +virStorageSourceInitChainElement(virStorageSourcePtr newelem, + virStorageSourcePtr old, + bool force) +{ + int ret = -1; + + if (force) { + virStorageSourceSeclabelsClear(newelem); + } + + if (!newelem->seclabels && + virStorageSourceSeclabelsCopy(newelem, old) < 0) + goto cleanup; + + newelem->shared = old->shared; + newelem->readonly = old->readonly; + + ret = 0; + + cleanup: + return ret; +} + + void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index f042847..78646ba 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -322,6 +322,9 @@ void virStorageNetHostDefFree(size_t nhosts, virStorageNetHostDefPtr hosts); virStorageNetHostDefPtr virStorageNetHostDefCopy(size_t nhosts, virStorageNetHostDefPtr hosts); +int virStorageSourceInitChainElement(virStorageSourcePtr newelem, + virStorageSourcePtr old, + bool force); void virStorageSourceAuthClear(virStorageSourcePtr def); void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); -- 1.9.3

Cgroups code uses VIR_CGROUP_DEVICE_* flags to specify the mode but in the end it needs to be converted to a string. Add a helper to do it and use it in the cgroup code before introducing it into the rest of the code. --- src/libvirt_private.syms | 1 + src/util/vircgroup.c | 62 +++++++++++++++++++++++++++++++++++------------- src/util/vircgroup.h | 2 ++ 3 files changed, 49 insertions(+), 16 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7844e2f..e73376e 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1049,6 +1049,7 @@ virCgroupGetCpuCfsQuota; virCgroupGetCpusetCpus; virCgroupGetCpusetMems; virCgroupGetCpuShares; +virCgroupGetDevicePermsString; virCgroupGetDomainTotalCpuStats; virCgroupGetFreezerState; virCgroupGetMemoryHardLimit; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index c578bd0..2eaf265 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2624,6 +2624,44 @@ virCgroupDenyAllDevices(virCgroupPtr group) /** + * virCgroupGetDevicePermsString: + * + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits + * + * Returns string corresponding to the appropriate bits set. + */ +const char * +virCgroupGetDevicePermsString(int perms) +{ + if (perms & VIR_CGROUP_DEVICE_READ) { + if (perms & VIR_CGROUP_DEVICE_WRITE) { + if (perms & VIR_CGROUP_DEVICE_MKNOD) + return "rwm"; + else + return "rw"; + } else { + if (perms & VIR_CGROUP_DEVICE_MKNOD) + return "rm"; + else + return "r"; + } + } else { + if (perms & VIR_CGROUP_DEVICE_WRITE) { + if (perms & VIR_CGROUP_DEVICE_MKNOD) + return "wm"; + else + return "w"; + } else { + if (perms & VIR_CGROUP_DEVICE_MKNOD) + return "m"; + else + return ""; + } + } +} + + +/** * virCgroupAllowDevice: * * @group: The cgroup to allow a device for @@ -2641,10 +2679,8 @@ virCgroupAllowDevice(virCgroupPtr group, char type, int major, int minor, int ret = -1; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:%i %s%s%s", type, major, minor, - perms & VIR_CGROUP_DEVICE_READ ? "r" : "", - perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", - perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") < 0) + if (virAsprintf(&devstr, "%c %i:%i %s", type, major, minor, + virCgroupGetDevicePermsString(perms)) < 0) goto cleanup; if (virCgroupSetValueStr(group, @@ -2678,10 +2714,8 @@ virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major, int ret = -1; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:* %s%s%s", type, major, - perms & VIR_CGROUP_DEVICE_READ ? "r" : "", - perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", - perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") < 0) + if (virAsprintf(&devstr, "%c %i:* %s", type, major, + virCgroupGetDevicePermsString(perms)) < 0) goto cleanup; if (virCgroupSetValueStr(group, @@ -2752,10 +2786,8 @@ virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor, int ret = -1; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:%i %s%s%s", type, major, minor, - perms & VIR_CGROUP_DEVICE_READ ? "r" : "", - perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", - perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") < 0) + if (virAsprintf(&devstr, "%c %i:%i %s", type, major, minor, + virCgroupGetDevicePermsString(perms)) < 0) goto cleanup; if (virCgroupSetValueStr(group, @@ -2789,10 +2821,8 @@ virCgroupDenyDeviceMajor(virCgroupPtr group, char type, int major, int ret = -1; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:* %s%s%s", type, major, - perms & VIR_CGROUP_DEVICE_READ ? "r" : "", - perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", - perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") < 0) + if (virAsprintf(&devstr, "%c %i:* %s", type, major, + virCgroupGetDevicePermsString(perms)) < 0) goto cleanup; if (virCgroupSetValueStr(group, diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 7bb46bf..3ab9f1c 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -173,6 +173,8 @@ enum { VIR_CGROUP_DEVICE_RWM = VIR_CGROUP_DEVICE_RW | VIR_CGROUP_DEVICE_MKNOD, }; +const char *virCgroupGetDevicePermsString(int perms); + int virCgroupDenyAllDevices(virCgroupPtr group); int virCgroupAllowDevice(virCgroupPtr group, -- 1.9.3

Only the top level gets writes, so the rest of the backing chain requires only read-only access. --- src/qemu/qemu_cgroup.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index c84a251..00b405b 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -49,10 +49,11 @@ static const char *const defaultDeviceACL[] = { #define DEVICE_PTY_MAJOR 136 #define DEVICE_SND_MAJOR 116 -int -qemuSetImageCgroup(virDomainObjPtr vm, - virStorageSourcePtr src, - bool deny) +static int +qemuSetImageCgroupInternal(virDomainObjPtr vm, + virStorageSourcePtr src, + bool deny, + bool forceReadonly) { qemuDomainObjPrivatePtr priv = vm->privateData; int perms = VIR_CGROUP_DEVICE_READ; @@ -75,7 +76,7 @@ qemuSetImageCgroup(virDomainObjPtr vm, ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms); } else { - if (!src->readonly) + if (!src->readonly && !forceReadonly) perms |= VIR_CGROUP_DEVICE_WRITE; VIR_DEBUG("Allow path %s, perms: %s", @@ -103,14 +104,27 @@ qemuSetImageCgroup(virDomainObjPtr vm, int +qemuSetImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src, + bool deny) +{ + return qemuSetImageCgroupInternal(vm, src, deny, false); +} + + +int qemuSetupDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk) { virStorageSourcePtr next; + bool forceReadonly = false; for (next = disk->src; next; next = next->backingStore) { - if (qemuSetImageCgroup(vm, next, false) < 0) + if (qemuSetImageCgroupInternal(vm, next, false, forceReadonly) < 0) return -1; + + /* setup only the top level image for read-write */ + forceReadonly = true; } return 0; -- 1.9.3

Add security driver functions to label separate storage images using the virStorageSource definition. This will help to avoid the need to do ugly changes to the disk struct and use the source directly. --- src/libvirt_private.syms | 2 ++ src/security/security_driver.h | 10 ++++++++ src/security/security_manager.c | 56 +++++++++++++++++++++++++++++++++++++++++ src/security/security_manager.h | 7 ++++++ src/security/security_nop.c | 19 ++++++++++++++ src/security/security_stack.c | 38 ++++++++++++++++++++++++++++ 6 files changed, 132 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 61325b7..5afc601 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -915,6 +915,7 @@ virSecurityManagerReserveLabel; virSecurityManagerRestoreAllLabel; virSecurityManagerRestoreDiskLabel; virSecurityManagerRestoreHostdevLabel; +virSecurityManagerRestoreImageLabel; virSecurityManagerRestoreSavedStateLabel; virSecurityManagerSetAllLabel; virSecurityManagerSetChildProcessLabel; @@ -923,6 +924,7 @@ virSecurityManagerSetDiskLabel; virSecurityManagerSetHostdevLabel; virSecurityManagerSetHugepages; virSecurityManagerSetImageFDLabel; +virSecurityManagerSetImageLabel; virSecurityManagerSetProcessLabel; virSecurityManagerSetSavedStateLabel; virSecurityManagerSetSocketLabel; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 062dc8f..f0dca09 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -112,6 +112,13 @@ typedef char *(*virSecurityDomainGetMountOptions) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainSetHugepages) (virSecurityManagerPtr mgr, virDomainDefPtr def, const char *path); +typedef int (*virSecurityDomainSetImageLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src); +typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src); + struct _virSecurityDriver { size_t privateDataLen; @@ -130,6 +137,9 @@ struct _virSecurityDriver { virSecurityDomainSetDiskLabel domainSetSecurityDiskLabel; virSecurityDomainRestoreDiskLabel domainRestoreSecurityDiskLabel; + virSecurityDomainSetImageLabel domainSetSecurityImageLabel; + virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; + virSecurityDomainSetDaemonSocketLabel domainSetSecurityDaemonSocketLabel; virSecurityDomainSetSocketLabel domainSetSecuritySocketLabel; virSecurityDomainClearSocketLabel domainClearSecuritySocketLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 06e5123..16bec5c 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -360,6 +360,34 @@ virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, } +/** + * virSecurityManagerRestoreImageLabel: + * @mgr: security manager object + * @vm: domain definition object + * @src: disk source definition to operate on + * + * Removes security label from a single storage image. + * + * Returns: 0 on success, -1 on error. + */ +int +virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src) +{ + if (mgr->drv->domainRestoreSecurityImageLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src); + virObjectUnlock(mgr); + return ret; + } + + virReportUnsupportedError(); + return -1; +} + + int virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm) @@ -440,6 +468,34 @@ virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, } +/** + * virSecurityManagerSetImageLabel: + * @mgr: security manager object + * @vm: domain definition object + * @src: disk source definition to operate on + * + * Labels a single storage image with the configured security label. + * + * Returns: 0 on success, -1 on error. + */ +int +virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src) +{ + if (mgr->drv->domainSetSecurityImageLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, src); + virObjectUnlock(mgr); + return ret; + } + + virReportUnsupportedError(); + return -1; +} + + int virSecurityManagerRestoreHostdevLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 8a5fcfb..97b6a2e 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -124,4 +124,11 @@ int virSecurityManagerSetHugepages(virSecurityManagerPtr mgr, virDomainDefPtr sec, const char *hugepages_path); +int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src); +int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src); + #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_nop.c b/src/security/security_nop.c index b57bf05..951125d 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -220,6 +220,22 @@ virSecurityGetBaseLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return NULL; } +static int +virSecurityDomainRestoreImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED, + virStorageSourcePtr src ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int +virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED, + virStorageSourcePtr src ATTRIBUTE_UNUSED) +{ + return 0; +} + virSecurityDriver virSecurityDriverNop = { .privateDataLen = 0, @@ -236,6 +252,9 @@ virSecurityDriver virSecurityDriverNop = { .domainSetSecurityDiskLabel = virSecurityDomainSetDiskLabelNop, .domainRestoreSecurityDiskLabel = virSecurityDomainRestoreDiskLabelNop, + .domainSetSecurityImageLabel = virSecurityDomainSetImageLabelNop, + .domainRestoreSecurityImageLabel = virSecurityDomainRestoreImageLabelNop, + .domainSetSecurityDaemonSocketLabel = virSecurityDomainSetDaemonSocketLabelNop, .domainSetSecuritySocketLabel = virSecurityDomainSetSocketLabelNop, .domainClearSecuritySocketLabel = virSecurityDomainClearSocketLabelNop, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index e3e9c85..1ded57b 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -564,6 +564,41 @@ virSecurityStackGetBaseLabel(virSecurityManagerPtr mgr, int virtType) virtType); } +static int +virSecurityStackSetSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerSetImageLabel(item->securityManager, vm, src) < 0) + rc = -1; + } + + return rc; +} + +static int +virSecurityStackRestoreSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerRestoreImageLabel(item->securityManager, + vm, src) < 0) + rc = -1; + } + + return rc; +} + virSecurityDriver virSecurityDriverStack = { .privateDataLen = sizeof(virSecurityStackData), .name = "stack", @@ -581,6 +616,9 @@ virSecurityDriver virSecurityDriverStack = { .domainSetSecurityDiskLabel = virSecurityStackSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = virSecurityStackRestoreSecurityDiskLabel, + .domainSetSecurityImageLabel = virSecurityStackSetSecurityImageLabel, + .domainRestoreSecurityImageLabel = virSecurityStackRestoreSecurityImageLabel, + .domainSetSecurityDaemonSocketLabel = virSecurityStackSetDaemonSocketLabel, .domainSetSecuritySocketLabel = virSecurityStackSetSocketLabel, .domainClearSecuritySocketLabel = virSecurityStackClearSocketLabel, -- 1.9.3

Refactor the code and reuse it to implement the functionality. --- src/security/security_selinux.c | 91 ++++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 38 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 87077ac..4d2e1dc 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -56,9 +56,6 @@ VIR_LOG_INIT("security.security_selinux"); typedef struct _virSecuritySELinuxData virSecuritySELinuxData; typedef virSecuritySELinuxData *virSecuritySELinuxDataPtr; -typedef struct _virSecuritySELinuxCallbackData virSecuritySELinuxCallbackData; -typedef virSecuritySELinuxCallbackData *virSecuritySELinuxCallbackDataPtr; - struct _virSecuritySELinuxData { char *domain_context; char *alt_domain_context; @@ -71,11 +68,6 @@ struct _virSecuritySELinuxData { #endif }; -struct _virSecuritySELinuxCallbackData { - virSecurityManagerPtr manager; - virSecurityLabelDefPtr secdef; -}; - #define SECURITY_SELINUX_VOID_DOI "0" #define SECURITY_SELINUX_NAME "selinux" @@ -1198,40 +1190,49 @@ virSecuritySELinuxRestoreSecurityImageLabel(virSecurityManagerPtr mgr, static int -virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, - const char *path, - size_t depth, - void *opaque) +virSecuritySELinuxSetSecurityImageLabelInternal(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src, + bool first) { - int ret; + virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; virSecurityDeviceLabelDefPtr disk_seclabel; - virSecuritySELinuxCallbackDataPtr cbdata = opaque; - virSecurityLabelDefPtr secdef = cbdata->secdef; - virSecuritySELinuxDataPtr data = virSecurityManagerGetPrivateData(cbdata->manager); + int ret; - disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, + if (!src->path || !virStorageSourceIsLocalStorage(src)) + return 0; + + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (!secdef || secdef->norelabel) + return 0; + + disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); if (disk_seclabel && disk_seclabel->norelabel) return 0; - if (disk_seclabel && !disk_seclabel->norelabel && - disk_seclabel->label) { - ret = virSecuritySELinuxSetFilecon(path, disk_seclabel->label); - } else if (depth == 0) { - - if (disk->src->shared) { - ret = virSecuritySELinuxSetFileconOptional(path, data->file_context); - } else if (disk->src->readonly) { - ret = virSecuritySELinuxSetFileconOptional(path, data->content_context); + if (disk_seclabel && !disk_seclabel->norelabel && disk_seclabel->label) { + ret = virSecuritySELinuxSetFilecon(src->path, disk_seclabel->label); + } else if (first) { + if (src->shared) { + ret = virSecuritySELinuxSetFileconOptional(src->path, + data->file_context); + } else if (src->readonly) { + ret = virSecuritySELinuxSetFileconOptional(src->path, + data->content_context); } else if (secdef->imagelabel) { - ret = virSecuritySELinuxSetFileconOptional(path, secdef->imagelabel); + ret = virSecuritySELinuxSetFileconOptional(src->path, + secdef->imagelabel); } else { ret = 0; } } else { - ret = virSecuritySELinuxSetFileconOptional(path, data->content_context); + ret = virSecuritySELinuxSetFileconOptional(src->path, + data->content_context); } + if (ret == 1 && !disk_seclabel) { /* If we failed to set a label, but virt_use_nfs let us * proceed anyway, then we don't need to relabel later. */ @@ -1239,35 +1240,48 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, if (!disk_seclabel) return -1; disk_seclabel->labelskip = true; - if (VIR_APPEND_ELEMENT(disk->src->seclabels, disk->src->nseclabels, + if (VIR_APPEND_ELEMENT(src->seclabels, src->nseclabels, disk_seclabel) < 0) { virSecurityDeviceLabelDefFree(disk_seclabel); return -1; } ret = 0; } + return ret; } + +static int +virSecuritySELinuxSetSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src) +{ + return virSecuritySELinuxSetSecurityImageLabelInternal(mgr, def, src, true); +} + + static int virSecuritySELinuxSetSecurityDiskLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk) { - virSecuritySELinuxCallbackData cbdata; - cbdata.manager = mgr; - cbdata.secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + bool first = true; + virStorageSourcePtr next; - if (!cbdata.secdef || cbdata.secdef->norelabel) - return 0; + for (next = disk->src; next; next = next->backingStore) { + if (virSecuritySELinuxSetSecurityImageLabelInternal(mgr, def, next, + first) < 0) + return -1; - return virDomainDiskDefForeachPath(disk, - true, - virSecuritySELinuxSetSecurityFileLabel, - &cbdata); + first = false; + } + + return 0; } + static int virSecuritySELinuxSetSecurityHostdevLabelHelper(const char *file, void *opaque) { @@ -2436,6 +2450,7 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSetSecurityDiskLabel = virSecuritySELinuxSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = virSecuritySELinuxRestoreSecurityDiskLabel, + .domainSetSecurityImageLabel = virSecuritySELinuxSetSecurityImageLabel, .domainRestoreSecurityImageLabel = virSecuritySELinuxRestoreSecurityImageLabel, .domainSetSecurityDaemonSocketLabel = virSecuritySELinuxSetSecurityDaemonSocketLabel, -- 1.9.3

Refactor the code and reuse it to implement the functionality. --- src/security/security_dac.c | 52 ++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 27 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index f86d532..715f68b 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -289,22 +289,29 @@ virSecurityDACRestoreSecurityFileLabel(const char *path) static int -virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk, - const char *path, - size_t depth ATTRIBUTE_UNUSED, - void *opaque) +virSecurityDACSetSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src) { - virSecurityDACCallbackDataPtr cbdata = opaque; - virSecurityManagerPtr mgr = cbdata->manager; - virSecurityLabelDefPtr secdef = cbdata->secdef; - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityLabelDefPtr secdef; virSecurityDeviceLabelDefPtr disk_seclabel; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); uid_t user; gid_t group; - disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, - SECURITY_DAC_NAME); + if (!priv->dynamicOwnership) + return 0; + + /* XXX: Add support for gluster DAC permissions */ + if (!src->path || !virStorageSourceIsLocalStorage(src)) + return 0; + + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + if (secdef && secdef->norelabel) + return 0; + disk_seclabel = virStorageSourceGetSecurityLabelDef(src, + SECURITY_DAC_NAME); if (disk_seclabel && disk_seclabel->norelabel) return 0; @@ -316,7 +323,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk, return -1; } - return virSecurityDACSetOwnership(path, user, group); + return virSecurityDACSetOwnership(src->path, user, group); } @@ -326,24 +333,14 @@ virSecurityDACSetSecurityDiskLabel(virSecurityManagerPtr mgr, virDomainDiskDefPtr disk) { - virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); - virSecurityDACCallbackData cbdata; - virSecurityLabelDefPtr secdef; + virStorageSourcePtr next; - if (!priv->dynamicOwnership) - return 0; - - secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); - - if (secdef && secdef->norelabel) - return 0; + for (next = disk->src; next; next = next->backingStore) { + if (virSecurityDACSetSecurityImageLabel(mgr, def, next) < 0) + return -1; + } - cbdata.manager = mgr; - cbdata.secdef = secdef; - return virDomainDiskDefForeachPath(disk, - false, - virSecurityDACSetSecurityFileLabel, - &cbdata); + return 0; } @@ -1277,6 +1274,7 @@ virSecurityDriver virSecurityDriverDAC = { .domainSetSecurityDiskLabel = virSecurityDACSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = virSecurityDACRestoreSecurityDiskLabel, + .domainSetSecurityImageLabel = virSecurityDACSetSecurityImageLabel, .domainRestoreSecurityImageLabel = virSecurityDACRestoreSecurityImageLabel, .domainSetSecurityDaemonSocketLabel = virSecurityDACSetDaemonSocketLabel, -- 1.9.3

Refactor the code and reuse it to implement the functionality. --- src/security/security_apparmor.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 391bf60..1e2a38b 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -704,41 +704,39 @@ AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, /* Called when hotplugging */ static int -AppArmorSetSecurityDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, virDomainDiskDefPtr disk) +AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src) { int rc = -1; char *profile_name = NULL; - virSecurityLabelDefPtr secdef = - virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); + virSecurityLabelDefPtr secdef; - if (!secdef) + if (!src->path || !virStorageSourceIsLocalStorage(src)) + return 0; + + if (!(secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME))) return -1; if (secdef->norelabel) return 0; - if (!virDomainDiskGetSource(disk) || - virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_NETWORK) - return 0; - if (secdef->imagelabel) { /* if the device doesn't exist, error out */ - if (!virFileExists(virDomainDiskGetSource(disk))) { + if (!virFileExists(src->path)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("\'%s\' does not exist"), - virDomainDiskGetSource(disk)); - return rc; + src->path); + return -1; } if ((profile_name = get_profile_name(def)) == NULL) - return rc; + return -1; /* update the profile only if it is loaded */ if (profile_loaded(secdef->imagelabel) >= 0) { if (load_profile(mgr, secdef->imagelabel, def, - virDomainDiskGetSource(disk), - false) < 0) { + src->path, false) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot update AppArmor profile " "\'%s\'"), @@ -756,6 +754,14 @@ AppArmorSetSecurityDiskLabel(virSecurityManagerPtr mgr, } static int +AppArmorSetSecurityDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk) +{ + return AppArmorSetSecurityImageLabel(mgr, def, disk->src); +} + +static int AppArmorSecurityVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def) { @@ -983,6 +989,7 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetSecurityDiskLabel = AppArmorSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = AppArmorRestoreSecurityDiskLabel, + .domainSetSecurityImageLabel = AppArmorSetSecurityImageLabel, .domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel, .domainSetSecurityDaemonSocketLabel = AppArmorSetSecurityDaemonSocketLabel, -- 1.9.3

Instead of just returning the parent path, return the complete parent source structure. --- src/qemu/qemu_driver.c | 16 ++++----- src/util/virstoragefile.c | 17 ++++------ src/util/virstoragefile.h | 2 +- tests/virstoragetest.c | 86 ++++++++++++++++++++++------------------------- 4 files changed, 56 insertions(+), 65 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 66b287c..c092494 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15494,7 +15494,7 @@ qemuDomainBlockCommit(virDomainPtr dom, unsigned int topIndex = 0; virStorageSourcePtr baseSource; unsigned int baseIndex = 0; - const char *top_parent = NULL; + virStorageSourcePtr top_parent = NULL; bool clean_access = false; /* XXX Add support for COMMIT_ACTIVE, COMMIT_DELETE */ @@ -15602,10 +15602,9 @@ qemuDomainBlockCommit(virDomainPtr dom, clean_access = true; if (qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource, VIR_DISK_CHAIN_READ_WRITE) < 0 || - (top_parent && top_parent != disk->src->path && - qemuDomainPrepareDiskChainElementPath(driver, vm, disk, - top_parent, - VIR_DISK_CHAIN_READ_WRITE) < 0)) + (top_parent != disk->src && + qemuDomainPrepareDiskChainElement(driver, vm, disk, top_parent, + VIR_DISK_CHAIN_READ_WRITE) < 0)) goto endjob; /* Start the commit operation. Pass the user's original spelling, @@ -15625,10 +15624,9 @@ qemuDomainBlockCommit(virDomainPtr dom, /* Revert access to read-only, if possible. */ qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource, VIR_DISK_CHAIN_READ_ONLY); - if (top_parent && top_parent != disk->src->path) - qemuDomainPrepareDiskChainElementPath(driver, vm, disk, - top_parent, - VIR_DISK_CHAIN_READ_ONLY); + if (top_parent != disk->src) + qemuDomainPrepareDiskChainElement(driver, vm, disk, top_parent, + VIR_DISK_CHAIN_READ_ONLY); } if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b7ae570..e4058a5 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1337,17 +1337,16 @@ virStorageFileChainLookup(virStorageSourcePtr chain, virStorageSourcePtr startFrom, const char *name, unsigned int idx, - const char **parent) + virStorageSourcePtr *parent) { - virStorageSourcePtr prev = NULL; + virStorageSourcePtr prev; const char *start = chain->path; - const char *tmp; char *parentDir = NULL; bool nameIsFile = virStorageIsFile(name); size_t i = 0; if (!parent) - parent = &tmp; + parent = &prev; *parent = NULL; if (startFrom) { @@ -1355,7 +1354,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, chain = chain->backingStore; i++; } - *parent = startFrom->path; + *parent = startFrom; } while (chain) { @@ -1372,8 +1371,8 @@ virStorageFileChainLookup(virStorageSourcePtr chain, break; if (nameIsFile && virStorageSourceIsLocalStorage(chain)) { - if (prev && virStorageSourceIsLocalStorage(prev)) - parentDir = mdir_name(prev->path); + if (*parent && virStorageSourceIsLocalStorage(*parent)) + parentDir = mdir_name((*parent)->path); else ignore_value(VIR_STRDUP(parentDir, ".")); @@ -1382,7 +1381,6 @@ virStorageFileChainLookup(virStorageSourcePtr chain, goto error; } - int result = virFileRelLinkPointsTo(parentDir, name, chain->path); @@ -1395,8 +1393,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, break; } } - *parent = chain->path; - prev = chain; + *parent = chain; chain = chain->backingStore; i++; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 78646ba..7c9260b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -297,7 +297,7 @@ virStorageSourcePtr virStorageFileChainLookup(virStorageSourcePtr chain, virStorageSourcePtr startFrom, const char *name, unsigned int idx, - const char **parent) + virStorageSourcePtr *parent) ATTRIBUTE_NONNULL(1); int virStorageFileResize(const char *path, diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index fb2837f..e2ee3ff 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -415,7 +415,7 @@ struct testLookupData unsigned int expIndex; const char *expResult; virStorageSourcePtr expMeta; - const char *expParent; + virStorageSourcePtr expParent; }; static int @@ -424,7 +424,7 @@ testStorageLookup(const void *args) const struct testLookupData *data = args; int ret = 0; virStorageSourcePtr result; - const char *actualParent; + virStorageSourcePtr actualParent; unsigned int idx; if (virStorageFileParseChainIndex(data->target, data->name, &idx) < 0 && @@ -488,9 +488,10 @@ testStorageLookup(const void *args) data->expMeta, result); ret = -1; } - if (STRNEQ_NULLABLE(data->expParent, actualParent)) { + if (data->expParent != actualParent) { fprintf(stderr, "parent: expected %s, got %s\n", - NULLSTR(data->expParent), NULLSTR(actualParent)); + NULLSTR(data->expParent ? data->expParent->path : NULL), + NULLSTR(actualParent ? actualParent->path : NULL)); ret = -1; } @@ -974,25 +975,25 @@ mymain(void) TEST_LOOKUP(5, NULL, abswrap, chain->path, chain, NULL); TEST_LOOKUP(6, chain, abswrap, NULL, NULL, NULL); TEST_LOOKUP(7, chain2, abswrap, NULL, NULL, NULL); - TEST_LOOKUP(8, NULL, "qcow2", chain2->path, chain2, chain->path); - TEST_LOOKUP(9, chain, "qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(8, NULL, "qcow2", chain2->path, chain2, chain); + TEST_LOOKUP(9, chain, "qcow2", chain2->path, chain2, chain); TEST_LOOKUP(10, chain2, "qcow2", NULL, NULL, NULL); TEST_LOOKUP(11, chain3, "qcow2", NULL, NULL, NULL); - TEST_LOOKUP(12, NULL, absqcow2, chain2->path, chain2, chain->path); - TEST_LOOKUP(13, chain, absqcow2, chain2->path, chain2, chain->path); + TEST_LOOKUP(12, NULL, absqcow2, chain2->path, chain2, chain); + TEST_LOOKUP(13, chain, absqcow2, chain2->path, chain2, chain); TEST_LOOKUP(14, chain2, absqcow2, NULL, NULL, NULL); TEST_LOOKUP(15, chain3, absqcow2, NULL, NULL, NULL); - TEST_LOOKUP(16, NULL, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(17, chain, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(18, chain2, "raw", chain3->path, chain3, chain2->path); + TEST_LOOKUP(16, NULL, "raw", chain3->path, chain3, chain2); + TEST_LOOKUP(17, chain, "raw", chain3->path, chain3, chain2); + TEST_LOOKUP(18, chain2, "raw", chain3->path, chain3, chain2); TEST_LOOKUP(19, chain3, "raw", NULL, NULL, NULL); - TEST_LOOKUP(20, NULL, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(21, chain, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(22, chain2, absraw, chain3->path, chain3, chain2->path); + TEST_LOOKUP(20, NULL, absraw, chain3->path, chain3, chain2); + TEST_LOOKUP(21, chain, absraw, chain3->path, chain3, chain2); + TEST_LOOKUP(22, chain2, absraw, chain3->path, chain3, chain2); TEST_LOOKUP(23, chain3, absraw, NULL, NULL, NULL); - TEST_LOOKUP(24, NULL, NULL, chain3->path, chain3, chain2->path); - TEST_LOOKUP(25, chain, NULL, chain3->path, chain3, chain2->path); - TEST_LOOKUP(26, chain2, NULL, chain3->path, chain3, chain2->path); + TEST_LOOKUP(24, NULL, NULL, chain3->path, chain3, chain2); + TEST_LOOKUP(25, chain, NULL, chain3->path, chain3, chain2); + TEST_LOOKUP(26, chain2, NULL, chain3->path, chain3, chain2); TEST_LOOKUP(27, chain3, NULL, NULL, NULL, NULL); /* Rewrite wrap and qcow2 back to 3-deep chain, relative backing */ @@ -1027,25 +1028,25 @@ mymain(void) TEST_LOOKUP(33, NULL, abswrap, chain->path, chain, NULL); TEST_LOOKUP(34, chain, abswrap, NULL, NULL, NULL); TEST_LOOKUP(35, chain2, abswrap, NULL, NULL, NULL); - TEST_LOOKUP(36, NULL, "qcow2", chain2->path, chain2, chain->path); - TEST_LOOKUP(37, chain, "qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(36, NULL, "qcow2", chain2->path, chain2, chain); + TEST_LOOKUP(37, chain, "qcow2", chain2->path, chain2, chain); TEST_LOOKUP(38, chain2, "qcow2", NULL, NULL, NULL); TEST_LOOKUP(39, chain3, "qcow2", NULL, NULL, NULL); - TEST_LOOKUP(40, NULL, absqcow2, chain2->path, chain2, chain->path); - TEST_LOOKUP(41, chain, absqcow2, chain2->path, chain2, chain->path); + TEST_LOOKUP(40, NULL, absqcow2, chain2->path, chain2, chain); + TEST_LOOKUP(41, chain, absqcow2, chain2->path, chain2, chain); TEST_LOOKUP(42, chain2, absqcow2, NULL, NULL, NULL); TEST_LOOKUP(43, chain3, absqcow2, NULL, NULL, NULL); - TEST_LOOKUP(44, NULL, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(45, chain, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(46, chain2, "raw", chain3->path, chain3, chain2->path); + TEST_LOOKUP(44, NULL, "raw", chain3->path, chain3, chain2); + TEST_LOOKUP(45, chain, "raw", chain3->path, chain3, chain2); + TEST_LOOKUP(46, chain2, "raw", chain3->path, chain3, chain2); TEST_LOOKUP(47, chain3, "raw", NULL, NULL, NULL); - TEST_LOOKUP(48, NULL, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(49, chain, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(50, chain2, absraw, chain3->path, chain3, chain2->path); + TEST_LOOKUP(48, NULL, absraw, chain3->path, chain3, chain2); + TEST_LOOKUP(49, chain, absraw, chain3->path, chain3, chain2); + TEST_LOOKUP(50, chain2, absraw, chain3->path, chain3, chain2); TEST_LOOKUP(51, chain3, absraw, NULL, NULL, NULL); - TEST_LOOKUP(52, NULL, NULL, chain3->path, chain3, chain2->path); - TEST_LOOKUP(53, chain, NULL, chain3->path, chain3, chain2->path); - TEST_LOOKUP(54, chain2, NULL, chain3->path, chain3, chain2->path); + TEST_LOOKUP(52, NULL, NULL, chain3->path, chain3, chain2); + TEST_LOOKUP(53, chain, NULL, chain3->path, chain3, chain2); + TEST_LOOKUP(54, chain2, NULL, chain3->path, chain3, chain2); TEST_LOOKUP(55, chain3, NULL, NULL, NULL, NULL); /* Use link to wrap with cross-directory relative backing */ @@ -1070,12 +1071,12 @@ mymain(void) TEST_LOOKUP(57, NULL, "sub/link2", chain->path, chain, NULL); TEST_LOOKUP(58, NULL, "wrap", chain->path, chain, NULL); TEST_LOOKUP(59, NULL, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(60, NULL, "../qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(60, NULL, "../qcow2", chain2->path, chain2, chain); TEST_LOOKUP(61, NULL, "qcow2", NULL, NULL, NULL); - TEST_LOOKUP(62, NULL, absqcow2, chain2->path, chain2, chain->path); - TEST_LOOKUP(63, NULL, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(64, NULL, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(65, NULL, NULL, chain3->path, chain3, chain2->path); + TEST_LOOKUP(62, NULL, absqcow2, chain2->path, chain2, chain); + TEST_LOOKUP(63, NULL, "raw", chain3->path, chain3, chain2); + TEST_LOOKUP(64, NULL, absraw, chain3->path, chain3, chain2); + TEST_LOOKUP(65, NULL, NULL, chain3->path, chain3, chain2); TEST_LOOKUP_TARGET(66, "vda", NULL, "bogus[1]", 0, NULL, NULL, NULL); TEST_LOOKUP_TARGET(67, "vda", NULL, "vda[-1]", 0, NULL, NULL, NULL); @@ -1084,18 +1085,13 @@ mymain(void) TEST_LOOKUP_TARGET(70, "vda", chain, "wrap", 0, NULL, NULL, NULL); TEST_LOOKUP_TARGET(71, "vda", chain2, "wrap", 0, NULL, NULL, NULL); TEST_LOOKUP_TARGET(72, "vda", NULL, "vda[0]", 0, NULL, NULL, NULL); - TEST_LOOKUP_TARGET(73, "vda", NULL, "vda[1]", 1, chain2->path, chain2, - chain->path); - TEST_LOOKUP_TARGET(74, "vda", chain, "vda[1]", 1, chain2->path, chain2, - chain->path); + TEST_LOOKUP_TARGET(73, "vda", NULL, "vda[1]", 1, chain2->path, chain2, chain); + TEST_LOOKUP_TARGET(74, "vda", chain, "vda[1]", 1, chain2->path, chain2, chain); TEST_LOOKUP_TARGET(75, "vda", chain2, "vda[1]", 1, NULL, NULL, NULL); TEST_LOOKUP_TARGET(76, "vda", chain3, "vda[1]", 1, NULL, NULL, NULL); - TEST_LOOKUP_TARGET(77, "vda", NULL, "vda[2]", 2, chain3->path, chain3, - chain2->path); - TEST_LOOKUP_TARGET(78, "vda", chain, "vda[2]", 2, chain3->path, chain3, - chain2->path); - TEST_LOOKUP_TARGET(79, "vda", chain2, "vda[2]", 2, chain3->path, chain3, - chain2->path); + TEST_LOOKUP_TARGET(77, "vda", NULL, "vda[2]", 2, chain3->path, chain3, chain2); + TEST_LOOKUP_TARGET(78, "vda", chain, "vda[2]", 2, chain3->path, chain3, chain2); + TEST_LOOKUP_TARGET(79, "vda", chain2, "vda[2]", 2, chain3->path, chain3, chain2); TEST_LOOKUP_TARGET(80, "vda", chain3, "vda[2]", 2, NULL, NULL, NULL); TEST_LOOKUP_TARGET(81, "vda", NULL, "vda[3]", 3, NULL, NULL, NULL); -- 1.9.3

When pivotting to a new disk source after a block commit (and possibly after a soon-to-be-added active block commit) we changed just a few fields to the new target. In case we'd copy a network disk to a local file we'd not change the type properly. To avoid such problems, switch to tracking of the source via changing of the complete source struct to the one tracking the mirroring info. --- src/qemu/qemu_driver.c | 49 +++++++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 486b899..ce04205 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14859,9 +14859,7 @@ qemuDomainBlockPivot(virConnectPtr conn, virDomainBlockJobInfo info; const char *format = NULL; bool resume = false; - char *oldsrc = NULL; - int oldformat; - virStorageSourcePtr oldchain = NULL; + virStorageSourcePtr oldsrc = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); if (!disk->mirror) { @@ -14927,29 +14925,17 @@ qemuDomainBlockPivot(virConnectPtr conn, * label the entire chain. This action is safe even if the * backing chain has already been labeled; but only necessary when * we know for sure that there is a backing chain. */ - oldsrc = disk->src->path; - oldformat = disk->src->format; - oldchain = disk->src->backingStore; - disk->src->path = disk->mirror->path; - disk->src->format = disk->mirror->format; - disk->src->backingStore = NULL; - if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) { - disk->src->path = oldsrc; - disk->src->format = oldformat; - disk->src->backingStore = oldchain; + oldsrc = disk->src; + disk->src = disk->mirror; + + if (qemuDomainDetermineDiskChain(driver, vm, disk, false) < 0) goto cleanup; - } + if (disk->mirror->format && disk->mirror->format != VIR_STORAGE_FILE_RAW && - (virDomainLockDiskAttach(driver->lockManager, cfg->uri, - vm, disk) < 0 || + (virDomainLockDiskAttach(driver->lockManager, cfg->uri, vm, disk) < 0 || qemuSetupDiskCgroup(vm, disk) < 0 || - virSecurityManagerSetDiskLabel(driver->securityManager, vm->def, - disk) < 0)) { - disk->src->path = oldsrc; - disk->src->format = oldformat; - disk->src->backingStore = oldchain; + virSecurityManagerSetDiskLabel(driver->securityManager, vm->def, disk) < 0)) goto cleanup; - } /* Attempt the pivot. */ qemuDomainObjEnterMonitor(driver, vm); @@ -14964,9 +14950,8 @@ qemuDomainBlockPivot(virConnectPtr conn, * portion of the chain, and is made more difficult by the * fact that we aren't tracking the full chain ourselves; so * for now, we leak the access to the original. */ - VIR_FREE(oldsrc); - virStorageSourceFree(oldchain); - disk->mirror->path = NULL; + virStorageSourceFree(oldsrc); + oldsrc = NULL; } else { /* On failure, qemu abandons the mirror, and reverts back to * the source disk (RHEL 6.3 has a bug where the revert could @@ -14976,16 +14961,17 @@ qemuDomainBlockPivot(virConnectPtr conn, * 'query-block', to see what state we really got left in * before killing the mirroring job? And just as on the * success case, there's security labeling to worry about. */ - disk->src->path = oldsrc; - disk->src->format = oldformat; - virStorageSourceFree(disk->src->backingStore); - disk->src->backingStore = oldchain; + virStorageSourceFree(disk->mirror); } - virStorageSourceFree(disk->mirror); + disk->mirror = NULL; disk->mirroring = false; cleanup: + /* revert to original disk def on failure */ + if (oldsrc) + disk->src = oldsrc; + if (resume && virDomainObjIsActive(vm) && qemuProcessStartCPUs(driver, vm, conn, VIR_DOMAIN_RUNNING_UNPAUSED, @@ -15337,6 +15323,9 @@ qemuDomainBlockCopy(virDomainObjPtr vm, /* XXX Allow non-file mirror destinations */ mirror->type = VIR_STORAGE_TYPE_FILE; + if (virStorageSourceInitChainElement(disk->mirror, disk->src, false) < 0) + goto endjob; + if (!(flags & VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT)) { int fd = qemuOpenFile(driver, vm, dest, O_WRONLY | O_TRUNC | O_CREAT, &need_unlink, NULL); -- 1.9.3

Now that security, cgroup and locking APIs support working on individual images and we track the backing chain security info on a per-image basis we can finally kill swapping the disk source in virDomainDiskDef and use the virStorageSource directly. --- src/qemu/qemu_driver.c | 85 ++++++++++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 47 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fbab2f1..5203431 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12062,61 +12062,52 @@ typedef enum { VIR_DISK_CHAIN_READ_WRITE, } qemuDomainDiskChainMode; -/* Several operations end up adding or removing a single element of a - * disk backing file chain; this helper function ensures that the lock - * manager, cgroup device controller, and security manager labelling - * are all aware of each new file before it is added to a chain, and - * can revoke access to a file no longer needed in a chain. */ +/* Several operations end up adding or removing a single element of a disk + * backing file chain; this helper function ensures that the lock manager, + * cgroup device controller, and security manager labelling are all aware of + * each new file before it is added to a chain, and can revoke access to a file + * no longer needed in a chain. */ static int qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk, virStorageSourcePtr elem, qemuDomainDiskChainMode mode) { - /* The easiest way to label a single file with the same - * permissions it would have as if part of the disk chain is to - * temporarily modify the disk in place. */ - virStorageSource origdisk; - bool origreadonly = disk->src->readonly; + bool readonly = elem->readonly; virQEMUDriverConfigPtr cfg = NULL; int ret = -1; - /* XXX Labelling of non-local files isn't currently supported */ - if (virStorageSourceGetActualType(disk->src) == VIR_STORAGE_TYPE_NETWORK) - return 0; - cfg = virQEMUDriverGetConfig(driver); - /* XXX Need to refactor the security manager and lock daemon to - * operate directly on a virStorageSourcePtr plus tidbits rather - * than a full virDomainDiskDef. */ - memcpy(&origdisk, disk->src, sizeof(origdisk)); - memcpy(disk->src, elem, sizeof(*elem)); - disk->src->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; + elem->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; if (mode == VIR_DISK_CHAIN_NO_ACCESS) { - if (virSecurityManagerRestoreDiskLabel(driver->securityManager, - vm->def, disk) < 0) - VIR_WARN("Unable to restore security label on %s", disk->src->path); - if (qemuTeardownDiskCgroup(vm, disk) < 0) - VIR_WARN("Failed to teardown cgroup for disk path %s", - disk->src->path); - if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) - VIR_WARN("Unable to release lock on %s", disk->src->path); - } else if (virDomainLockDiskAttach(driver->lockManager, cfg->uri, - vm, disk) < 0 || - qemuSetupDiskCgroup(vm, disk) < 0 || - virSecurityManagerSetDiskLabel(driver->securityManager, - vm->def, disk) < 0) { - goto cleanup; + if (virSecurityManagerRestoreImageLabel(driver->securityManager, + vm->def, elem) < 0) + VIR_WARN("Unable to restore security label on %s", elem->path); + + if (qemuSetImageCgroup(vm, elem, true) < 0) + VIR_WARN("Failed to teardown cgroup for disk path %s", elem->path); + + if (virDomainLockImageDetach(driver->lockManager, vm, elem) < 0) + VIR_WARN("Unable to release lock on %s", elem->path); + } else { + if (virDomainLockImageAttach(driver->lockManager, cfg->uri, + vm, elem) < 0) + goto cleanup; + + if (qemuSetImageCgroup(vm, elem, false) < 0) + goto cleanup; + + if (virSecurityManagerSetImageLabel(driver->securityManager, + vm->def, elem) < 0) + goto cleanup; } ret = 0; cleanup: - memcpy(disk->src, &origdisk, sizeof(origdisk)); - disk->src->readonly = origreadonly; + elem->readonly = readonly; virObjectUnref(cfg); return ret; } @@ -12885,9 +12876,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, VIR_FORCE_CLOSE(fd); } - if (qemuDomainPrepareDiskChainElement(driver, vm, disk, snap->src, + if (qemuDomainPrepareDiskChainElement(driver, vm, snap->src, VIR_DISK_CHAIN_READ_WRITE) < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, disk, snap->src, + qemuDomainPrepareDiskChainElement(driver, vm, snap->src, VIR_DISK_CHAIN_NO_ACCESS); goto cleanup; } @@ -12982,7 +12973,7 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, ignore_value(virStorageFileInit(disk->src)); - qemuDomainPrepareDiskChainElement(driver, vm, disk, disk->src, + qemuDomainPrepareDiskChainElement(driver, vm, disk->src, VIR_DISK_CHAIN_NO_ACCESS); if (need_unlink && virStorageFileStat(disk->src, &st) == 0 && S_ISREG(st.st_mode) && @@ -15300,9 +15291,9 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if (VIR_STRDUP(mirror->path, dest) < 0) goto endjob; - if (qemuDomainPrepareDiskChainElement(driver, vm, disk, mirror, + if (qemuDomainPrepareDiskChainElement(driver, vm, mirror, VIR_DISK_CHAIN_READ_WRITE) < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, disk, mirror, + qemuDomainPrepareDiskChainElement(driver, vm, mirror, VIR_DISK_CHAIN_NO_ACCESS); goto endjob; } @@ -15314,7 +15305,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, virDomainAuditDisk(vm, NULL, dest, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, disk, mirror, + qemuDomainPrepareDiskChainElement(driver, vm, mirror, VIR_DISK_CHAIN_NO_ACCESS); goto endjob; } @@ -15516,10 +15507,10 @@ qemuDomainBlockCommit(virDomainPtr dom, * operation succeeds, but doing that requires tracking the * operation in XML across libvirtd restarts. */ clean_access = true; - if (qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource, + if (qemuDomainPrepareDiskChainElement(driver, vm, baseSource, VIR_DISK_CHAIN_READ_WRITE) < 0 || (top_parent != disk->src && - qemuDomainPrepareDiskChainElement(driver, vm, disk, top_parent, + qemuDomainPrepareDiskChainElement(driver, vm, top_parent, VIR_DISK_CHAIN_READ_WRITE) < 0)) goto endjob; @@ -15538,10 +15529,10 @@ qemuDomainBlockCommit(virDomainPtr dom, endjob: if (ret < 0 && clean_access) { /* Revert access to read-only, if possible. */ - qemuDomainPrepareDiskChainElement(driver, vm, disk, baseSource, + qemuDomainPrepareDiskChainElement(driver, vm, baseSource, VIR_DISK_CHAIN_READ_ONLY); if (top_parent != disk->src) - qemuDomainPrepareDiskChainElement(driver, vm, disk, top_parent, + qemuDomainPrepareDiskChainElement(driver, vm, top_parent, VIR_DISK_CHAIN_READ_ONLY); } if (!qemuDomainObjEndJob(driver, vm)) -- 1.9.3

Move the last operation done on local files to the storage driver API. --- src/qemu/qemu_driver.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 281c648..79970f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12847,7 +12847,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup; - if (virStorageFileInit(newDiskSrc) < 0) + if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) goto cleanup; if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) @@ -12863,15 +12863,13 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } /* pre-create the image file so that we can label it before handing it to qemu */ - /* XXX we should switch to storage driver based pre-creation of the image */ - if (virStorageSourceIsLocalStorage(newDiskSrc)) { - if (!reuse && newDiskSrc->type != VIR_STORAGE_TYPE_BLOCK) { - fd = qemuOpenFile(driver, vm, source, O_WRONLY | O_TRUNC | O_CREAT, - &need_unlink, NULL); - if (fd < 0) - goto cleanup; - VIR_FORCE_CLOSE(fd); + if (!reuse && newDiskSrc->type != VIR_STORAGE_TYPE_BLOCK) { + if (virStorageFileCreate(newDiskSrc) < 0) { + virReportSystemError(errno, _("failed to create image file '%s'"), + source); + goto cleanup; } + need_unlink = true; } /* set correct security, cgroup and locking options on the new image */ -- 1.9.3