It will help re-using the function. --- src/qemu/qemu_domain.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0e1c263..5ec5765 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2408,7 +2408,7 @@ qemuDomainCleanupRun(virQEMUDriverPtr driver, static void qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, virDomainObjPtr vm, - virDomainDiskDefPtr disk, + virStorageSourcePtr src, uid_t *uid, gid_t *gid) { virSecurityLabelDefPtr vmlabel; @@ -2431,7 +2431,7 @@ qemuDomainGetImageIds(virQEMUDriverConfigPtr cfg, vmlabel->label) virParseOwnershipIds(vmlabel->label, uid, gid); - if ((disklabel = virStorageSourceGetSecurityLabelDef(disk->src, "dac")) && + if ((disklabel = virStorageSourceGetSecurityLabelDef(src, "dac")) && disklabel->label) virParseOwnershipIds(disklabel->label, uid, gid); } @@ -2460,7 +2460,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, goto cleanup; } - qemuDomainGetImageIds(cfg, vm, disk, &uid, &gid); + qemuDomainGetImageIds(cfg, vm, disk->src, &uid, &gid); if (virStorageFileGetMetadata(disk->src, uid, gid, -- 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 5ec5765..4f63c88 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2438,6 +2438,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

Allow de-init of null storage sources. --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 8c0c5d6..ae86c69 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2540,7 +2540,7 @@ int storageRegister(void) static bool virStorageFileIsInitialized(virStorageSourcePtr src) { - return !!src->drv; + return src && src->drv; } -- 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 54925ba..ffdbf55 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14830,14 +14830,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++) @@ -14849,11 +14850,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; @@ -14869,8 +14880,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: @@ -14878,8 +14890,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: @@ -14942,8 +14955,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: @@ -14958,6 +14972,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, @@ -14994,7 +15018,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

In the future we might need to track state of individual images. Move the readonly and shared flags to the virStorageSource struct so that we can keep them in a per-image basis. --- src/conf/domain_conf.c | 18 ++++++++++-------- src/conf/domain_conf.h | 2 -- src/libxl/libxl_conf.c | 2 +- src/locking/domain_lock.c | 4 ++-- src/lxc/lxc_cgroup.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_cgroup.c | 4 ++-- src/qemu/qemu_command.c | 14 +++++++------- src/qemu/qemu_conf.c | 4 ++-- src/qemu/qemu_driver.c | 8 ++++---- src/qemu/qemu_migration.c | 16 ++++++++++------ src/security/security_dac.c | 2 +- src/security/security_selinux.c | 6 +++--- src/security/virt-aa-helper.c | 2 +- src/util/virstoragefile.h | 6 ++++++ src/vbox/vbox_tmpl.c | 30 +++++++++++++++--------------- src/xenxs/xen_sxpr.c | 10 +++++----- src/xenxs/xen_xm.c | 10 +++++----- 19 files changed, 77 insertions(+), 67 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ffdbf55..70f1103 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5495,9 +5495,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { - def->readonly = true; + def->src->readonly = true; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { - def->shared = true; + def->src->shared = true; } else if (xmlStrEqual(cur->name, BAD_CAST "transient")) { def->transient = true; } else if ((flags & VIR_DOMAIN_XML_INTERNAL_STATUS) && @@ -5624,7 +5624,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, /* Force CDROM to be listed as read only */ if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) - def->readonly = true; + def->src->readonly = true; if ((def->device == VIR_DOMAIN_DISK_DEVICE_DISK || def->device == VIR_DOMAIN_DISK_DEVICE_LUN) && @@ -5646,7 +5646,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, snapshot); goto error; } - } else if (def->readonly) { + } else if (def->src->readonly) { def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } @@ -13349,7 +13349,8 @@ virDomainDiskDefCheckABIStability(virDomainDiskDefPtr src, return false; } - if (src->readonly != dst->readonly || src->shared != dst->shared) { + if (src->src->readonly != dst->src->readonly || + src->src->shared != dst->src->shared) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Target disk access mode does not match source")); return false; @@ -15096,7 +15097,8 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " sgio='%s'", sgio); if (def->snapshot && - !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && def->readonly)) + !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE && + def->src->readonly)) virBufferAsprintf(buf, " snapshot='%s'", virDomainSnapshotLocationTypeToString(def->snapshot)); virBufferAddLit(buf, ">\n"); @@ -15235,9 +15237,9 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, "</iotune>\n"); } - if (def->readonly) + if (def->src->readonly) virBufferAddLit(buf, "<readonly/>\n"); - if (def->shared) + if (def->src->shared) virBufferAddLit(buf, "<shareable/>\n"); if (def->transient) virBufferAddLit(buf, "<transient/>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a00e30a..32674e0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -636,8 +636,6 @@ struct _virDomainDiskDef { int copy_on_read; /* enum virDomainDiskCopyOnRead */ int snapshot; /* virDomainSnapshotLocation, snapshot_conf.h */ int startupPolicy; /* enum virDomainStartupPolicy */ - bool readonly; - bool shared; bool transient; virDomainDeviceInfo info; bool rawio_specified; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 8eeaf82..a1ffdb2 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -827,7 +827,7 @@ libxlMakeDisk(virDomainDiskDefPtr l_disk, libxl_device_disk *x_disk) /* XXX is this right? */ x_disk->removable = 1; - x_disk->readwrite = !l_disk->readonly; + x_disk->readwrite = !l_disk->src->readonly; x_disk->is_cdrom = l_disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? 1 : 0; /* An empty CDROM must have the empty format, otherwise libxl fails. */ if (x_disk->is_cdrom && !x_disk->pdev_path) diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 4b3f4d4..78acaa6 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -83,9 +83,9 @@ static int virDomainLockManagerAddDisk(virLockManagerPtr lock, type == VIR_STORAGE_TYPE_DIR)) return 0; - if (disk->readonly) + if (disk->src->readonly) diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY; - if (disk->shared) + if (disk->src->shared) diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED; VIR_DEBUG("Add disk %s", src); diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 39e30ad..00ff807 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -380,7 +380,7 @@ static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def, if (virCgroupAllowDevicePath(cgroup, virDomainDiskGetSource(def->disks[i]), - (def->disks[i]->readonly ? + (def->disks[i]->src->readonly ? VIR_CGROUP_DEVICE_READ : VIR_CGROUP_DEVICE_RW) | VIR_CGROUP_DEVICE_MKNOD) < 0) diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index bc1b962..4aa7e02 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -455,7 +455,7 @@ static int virLXCControllerSetupNBDDeviceDisk(virDomainDiskDefPtr disk) if (virFileNBDDeviceAssociate(src, format, - disk->readonly, + disk->src->readonly, &dev) < 0) return -1; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index fce16f2..b1ec410 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4058,7 +4058,7 @@ lxcDomainAttachDeviceDiskLive(virLXCDriverPtr driver, goto cleanup; } - perms = (def->readonly ? + perms = (def->src->readonly ? VIR_CGROUP_DEVICE_READ : VIR_CGROUP_DEVICE_RW) | VIR_CGROUP_DEVICE_MKNOD; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index a31558f..3394c68 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -61,10 +61,10 @@ qemuSetupDiskPathAllow(virDomainDiskDefPtr disk, VIR_DEBUG("Process path %s for disk", path); ret = virCgroupAllowDevicePath(priv->cgroup, path, - (disk->readonly ? VIR_CGROUP_DEVICE_READ + (disk->src->readonly ? VIR_CGROUP_DEVICE_READ : VIR_CGROUP_DEVICE_RW)); virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, - disk->readonly ? "r" : "rw", ret == 0); + disk->src->readonly ? "r" : "rw", ret == 0); /* Get this for root squash NFS */ if (ret < 0 && diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index fb64cda..f4c0b12 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3344,7 +3344,7 @@ qemuBuildDriveStr(virConnectPtr conn, goto error; } - if (!disk->readonly) { + if (!disk->src->readonly) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot create virtual FAT disks in read-write mode")); goto error; @@ -3412,7 +3412,7 @@ qemuBuildDriveStr(virConnectPtr conn, disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) && disk->bus != VIR_DOMAIN_DISK_BUS_IDE) virBufferAddLit(&opt, ",boot=on"); - if (disk->readonly && + if (disk->src->readonly && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_READONLY)) { if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { @@ -3478,7 +3478,7 @@ qemuBuildDriveStr(virConnectPtr conn, } virBufferAsprintf(&opt, ",cache=%s", mode); - } else if (disk->shared && !disk->readonly) { + } else if (disk->src->shared && !disk->src->readonly) { virBufferAddLit(&opt, ",cache=off"); } @@ -8005,7 +8005,7 @@ qemuBuildCommandLine(virConnectPtr conn, virStorageFileFormatTypeToString(disk->src->format)); goto error; } - if (!disk->readonly) { + if (!disk->src->readonly) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot create virtual FAT disks in read-write mode")); goto error; @@ -9629,7 +9629,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, } else if (STREQ(keywords[i], "media")) { if (STREQ(values[i], "cdrom")) { def->device = VIR_DOMAIN_DISK_DEVICE_CDROM; - def->readonly = true; + def->src->readonly = true; } else if (STREQ(values[i], "floppy")) def->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; } else if (STREQ(keywords[i], "format")) { @@ -9685,7 +9685,7 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, } } else if (STREQ(keywords[i], "readonly")) { if ((values[i] == NULL) || STREQ(values[i], "on")) - def->readonly = true; + def->src->readonly = true; } else if (STREQ(keywords[i], "aio")) { if ((def->iomode = virDomainDiskIoTypeFromString(values[i])) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -10853,7 +10853,7 @@ qemuParseCommandLine(virCapsPtr qemuCaps, disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; if (VIR_STRDUP(disk->dst, "hdc") < 0) goto error; - disk->readonly = true; + disk->src->readonly = true; } else { if (STRPREFIX(arg, "-fd")) { disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index f92f831..e62bec0 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -906,7 +906,7 @@ qemuAddSharedDevice(virQEMUDriverPtr driver, if (dev->type == VIR_DOMAIN_DEVICE_DISK) { disk = dev->data.disk; - if (!disk->shared || !virDomainDiskSourceIsBlockType(disk)) + if (!disk->src->shared || !virDomainDiskSourceIsBlockType(disk)) return 0; } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { hostdev = dev->data.hostdev; @@ -1013,7 +1013,7 @@ qemuRemoveSharedDevice(virQEMUDriverPtr driver, if (dev->type == VIR_DOMAIN_DEVICE_DISK) { disk = dev->data.disk; - if (!disk->shared || !virDomainDiskSourceIsBlockType(disk)) + if (!disk->src->shared || !virDomainDiskSourceIsBlockType(disk)) return 0; } else if (dev->type == VIR_DOMAIN_DEVICE_HOSTDEV) { hostdev = dev->data.hostdev; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2d1aa9e..ae3cb91 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12056,7 +12056,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, * permissions it would have as if part of the disk chain is to * temporarily modify the disk in place. */ virStorageSource origdisk; - bool origreadonly = disk->readonly; + bool origreadonly = disk->src->readonly; virQEMUDriverConfigPtr cfg = NULL; int ret = -1; @@ -12071,7 +12071,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, * than a full virDomainDiskDef. */ memcpy(&origdisk, disk->src, sizeof(origdisk)); memcpy(disk->src, elem, sizeof(*elem)); - disk->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; + disk->src->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; if (mode == VIR_DISK_CHAIN_NO_ACCESS) { if (virSecurityManagerRestoreDiskLabel(driver->securityManager, @@ -12094,7 +12094,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, cleanup: memcpy(disk->src, &origdisk, sizeof(origdisk)); - disk->readonly = origreadonly; + disk->src->readonly = origreadonly; virObjectUnref(cfg); return ret; } @@ -12746,7 +12746,7 @@ qemuDomainSnapshotPrepare(virConnectPtr conn, case VIR_DOMAIN_SNAPSHOT_LOCATION_NONE: /* Remember seeing a disk that has snapshot disabled */ - if (!dom_disk->readonly) + if (!dom_disk->src->readonly) forbid_internal = true; break; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 7aa772a..767d840 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1156,7 +1156,8 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver, virDomainDiskDefPtr disk = vm->def->disks[i]; /* skip shared, RO and source-less disks */ - if (disk->shared || disk->readonly || !virDomainDiskGetSource(disk)) + if (disk->src->shared || disk->src->readonly || + !virDomainDiskGetSource(disk)) continue; VIR_FREE(diskAlias); @@ -1262,7 +1263,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, virDomainBlockJobInfo info; /* skip shared, RO and source-less disks */ - if (disk->shared || disk->readonly || !virDomainDiskGetSource(disk)) + if (disk->src->shared || disk->src->readonly || + !virDomainDiskGetSource(disk)) continue; VIR_FREE(diskAlias); @@ -1348,7 +1350,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, virDomainDiskDefPtr disk = vm->def->disks[--lastGood]; /* skip shared, RO disks */ - if (disk->shared || disk->readonly || !virDomainDiskGetSource(disk)) + if (disk->src->shared || disk->src->readonly || + !virDomainDiskGetSource(disk)) continue; VIR_FREE(diskAlias); @@ -1411,7 +1414,8 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig, virDomainDiskDefPtr disk = vm->def->disks[i]; /* skip shared, RO and source-less disks */ - if (disk->shared || disk->readonly || !virDomainDiskGetSource(disk)) + if (disk->src->shared || disk->src->readonly || + !virDomainDiskGetSource(disk)) continue; VIR_FREE(diskAlias); @@ -1540,8 +1544,8 @@ qemuMigrationIsSafe(virDomainDefPtr def) /* Our code elsewhere guarantees shared disks are either readonly (in * which case cache mode doesn't matter) or used with cache=none */ if (src && - !disk->shared && - !disk->readonly && + !disk->src->shared && + !disk->src->readonly && disk->cachemode != VIR_DOMAIN_DISK_CACHE_DISABLE) { int rc; diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 639f9b0..38cb47f 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -383,7 +383,7 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, * we can't see running VMs using the file on other nodes * Safest bet is thus to skip the restore step. */ - if (disk->readonly || disk->shared) + if (disk->src->readonly || disk->src->shared) return 0; if (!src) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 572f8a1..7740e69 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1155,7 +1155,7 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, * we can't see running VMs using the file on other nodes * Safest bet is thus to skip the restore step. */ - if (disk->readonly || disk->shared) + if (disk->src->readonly || disk->src->shared) return 0; if (!src || virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_NETWORK) @@ -1213,9 +1213,9 @@ virSecuritySELinuxSetSecurityFileLabel(virDomainDiskDefPtr disk, ret = virSecuritySELinuxSetFilecon(path, disk_seclabel->label); } else if (depth == 0) { - if (disk->shared) { + if (disk->src->shared) { ret = virSecuritySELinuxSetFileconOptional(path, data->file_context); - } else if (disk->readonly) { + } else if (disk->src->readonly) { ret = virSecuritySELinuxSetFileconOptional(path, data->content_context); } else if (secdef->imagelabel) { ret = virSecuritySELinuxSetFileconOptional(path, secdef->imagelabel); diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index e54f73f..b5f66f3 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -907,7 +907,7 @@ add_file_path(virDomainDiskDefPtr disk, int ret; if (depth == 0) { - if (disk->readonly) + if (disk->src->readonly) ret = vah_add_file(buf, path, "r"); else ret = vah_add_file(buf, path, "rw"); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 4f7357b..63950ec 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -255,6 +255,12 @@ struct _virStorageSource { size_t nseclabels; virSecurityDeviceLabelDefPtr *seclabels; + /* Don't ever write to the image */ + bool readonly; + + /* image is shared across hosts */ + bool shared; + /* backing chain of the storage source */ virStorageSourcePtr backingStore; diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index b27ab02..3825083 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2791,7 +2791,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { hardDiskPM->vtbl->GetType(hardDiskPM, &hddType); if (hddType == HardDiskType_Immutable) - def->disks[hddNum]->readonly = true; + def->disks[hddNum]->src->readonly = true; ignore_value(virDomainDiskSetSource(def->disks[hddNum], hddlocation)); ignore_value(VIR_STRDUP(def->disks[hddNum]->dst, "hda")); @@ -2813,7 +2813,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { hardDiskPS->vtbl->GetType(hardDiskPS, &hddType); if (hddType == HardDiskType_Immutable) - def->disks[hddNum]->readonly = true; + def->disks[hddNum]->src->readonly = true; ignore_value(virDomainDiskSetSource(def->disks[hddNum], hddlocation)); ignore_value(VIR_STRDUP(def->disks[hddNum]->dst, "hdb")); @@ -2835,7 +2835,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { hardDiskSS->vtbl->GetType(hardDiskSS, &hddType); if (hddType == HardDiskType_Immutable) - def->disks[hddNum]->readonly = true; + def->disks[hddNum]->src->readonly = true; ignore_value(virDomainDiskSetSource(def->disks[hddNum], hddlocation)); ignore_value(VIR_STRDUP(def->disks[hddNum]->dst, "hdd")); @@ -2977,7 +2977,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { medium->vtbl->GetReadOnly(medium, &readOnly); if (readOnly == PR_TRUE) - def->disks[diskCount]->readonly = true; + def->disks[diskCount]->src->readonly = true; virDomainDiskSetType(def->disks[diskCount], VIR_STORAGE_TYPE_FILE); @@ -3257,7 +3257,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { def->disks[def->ndisks - 1]->bus = VIR_DOMAIN_DISK_BUS_IDE; virDomainDiskSetType(def->disks[def->ndisks - 1], VIR_STORAGE_TYPE_FILE); - def->disks[def->ndisks - 1]->readonly = true; + def->disks[def->ndisks - 1]->src->readonly = true; ignore_value(virDomainDiskSetSource(def->disks[def->ndisks - 1], location)); ignore_value(VIR_STRDUP(def->disks[def->ndisks - 1]->dst, "hdc")); def->ndisks--; @@ -3304,7 +3304,7 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { def->disks[def->ndisks - 1]->bus = VIR_DOMAIN_DISK_BUS_FDC; virDomainDiskSetType(def->disks[def->ndisks - 1], VIR_STORAGE_TYPE_FILE); - def->disks[def->ndisks - 1]->readonly = false; + def->disks[def->ndisks - 1]->src->readonly = false; ignore_value(virDomainDiskSetSource(def->disks[def->ndisks - 1], location)); ignore_value(VIR_STRDUP(def->disks[def->ndisks - 1]->dst, "fda")); def->ndisks--; @@ -3910,9 +3910,9 @@ vboxAttachDrives(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) VIR_DEBUG("disk(%zu) driverType: %s", i, virStorageFileFormatTypeToString(format)); VIR_DEBUG("disk(%zu) cachemode: %d", i, def->disks[i]->cachemode); - VIR_DEBUG("disk(%zu) readonly: %s", i, (def->disks[i]->readonly + VIR_DEBUG("disk(%zu) readonly: %s", i, (def->disks[i]->src->readonly ? "True" : "False")); - VIR_DEBUG("disk(%zu) shared: %s", i, (def->disks[i]->shared + VIR_DEBUG("disk(%zu) shared: %s", i, (def->disks[i]->src->shared ? "True" : "False")); if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { @@ -4014,11 +4014,11 @@ vboxAttachDrives(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) "attached as harddisk: %s, rc=%08x"), src, (unsigned)rc); } else { - if (def->disks[i]->readonly) { + if (def->disks[i]->src->readonly) { hardDisk->vtbl->SetType(hardDisk, HardDiskType_Immutable); VIR_DEBUG("setting harddisk to readonly"); - } else if (!def->disks[i]->readonly) { + } else if (!def->disks[i]->src->readonly) { hardDisk->vtbl->SetType(hardDisk, HardDiskType_Normal); VIR_DEBUG("setting harddisk type to normal"); @@ -4193,9 +4193,9 @@ vboxAttachDrives(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) VIR_DEBUG("disk(%zu) driverType: %s", i, virStorageFileFormatTypeToString(format)); VIR_DEBUG("disk(%zu) cachemode: %d", i, def->disks[i]->cachemode); - VIR_DEBUG("disk(%zu) readonly: %s", i, (def->disks[i]->readonly + VIR_DEBUG("disk(%zu) readonly: %s", i, (def->disks[i]->src->readonly ? "True" : "False")); - VIR_DEBUG("disk(%zu) shared: %s", i, (def->disks[i]->shared + VIR_DEBUG("disk(%zu) shared: %s", i, (def->disks[i]->src->shared ? "True" : "False")); if (type == VIR_STORAGE_TYPE_FILE && src) { @@ -4317,10 +4317,10 @@ vboxAttachDrives(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine) } if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - if (def->disks[i]->readonly) { + if (def->disks[i]->src->readonly) { medium->vtbl->SetType(medium, MediumType_Immutable); VIR_DEBUG("setting harddisk to immutable"); - } else if (!def->disks[i]->readonly) { + } else if (!def->disks[i]->src->readonly) { medium->vtbl->SetType(medium, MediumType_Normal); VIR_DEBUG("setting harddisk type to normal"); } @@ -7500,7 +7500,7 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, goto cleanup; } if (readOnly == PR_TRUE) - def->dom->disks[diskCount]->readonly = true; + def->dom->disks[diskCount]->src->readonly = true; def->dom->disks[diskCount]->src->type = VIR_STORAGE_TYPE_FILE; def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, deviceInst, diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 38b8423..9e59804 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -495,10 +495,10 @@ xenParseSxprDisks(virDomainDefPtr def, if (mode && strchr(mode, 'r')) - disk->readonly = true; + disk->src->readonly = true; if (mode && strchr(mode, '!')) - disk->shared = true; + disk->src->shared = true; if (VIR_REALLOC_N(def->disks, def->ndisks+1) < 0) goto error; @@ -1321,7 +1321,7 @@ xenParseSxpr(const struct sexpr *root, goto error; } disk->bus = VIR_DOMAIN_DISK_BUS_IDE; - disk->readonly = true; + disk->src->readonly = true; if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) { virDomainDiskDefFree(disk); @@ -1816,9 +1816,9 @@ xenFormatSxprDisk(virDomainDiskDefPtr def, } } - if (def->readonly) + if (def->src->readonly) virBufferAddLit(buf, "(mode 'r')"); - else if (def->shared) + else if (def->src->shared) virBufferAddLit(buf, "(mode 'w!')"); else virBufferAddLit(buf, "(mode 'w')"); diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 2b28150..25a042d 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -625,10 +625,10 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (STREQ(head, "r") || STREQ(head, "ro")) - disk->readonly = true; + disk->src->readonly = true; else if ((STREQ(head, "w!")) || (STREQ(head, "!"))) - disk->shared = true; + disk->src->shared = true; /* Maintain list in sorted order according to target device name */ if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) @@ -656,7 +656,7 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (VIR_STRDUP(disk->dst, "hdc") < 0) goto cleanup; disk->bus = VIR_DOMAIN_DISK_BUS_IDE; - disk->readonly = true; + disk->src->readonly = true; if (VIR_APPEND_ELEMENT(def->disks, def->ndisks, disk) < 0) goto cleanup; @@ -1249,9 +1249,9 @@ xenFormatXMDisk(virConfValuePtr list, if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) virBufferAddLit(&buf, ":cdrom"); - if (disk->readonly) + if (disk->src->readonly) virBufferAddLit(&buf, ",r"); - else if (disk->shared) + else if (disk->src->shared) virBufferAddLit(&buf, ",!"); else virBufferAddLit(&buf, ",w"); -- 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 | 194 ++++++++++++++++++++++++++++++++++++++++++++-- src/util/virstoragefile.h | 3 + 3 files changed, 190 insertions(+), 8 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 29e9db9..918b6d2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1919,6 +1919,7 @@ virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStorageSourceBackingStoreClear; virStorageSourceClear; +virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 9208b77..3037435 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1719,6 +1719,189 @@ 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) + 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; + + if (src->auth && + !(ret->auth = virStorageAuthDefCopy(src->auth))) + goto error; + + if (backingChain && src->backingStore) { + if (!(ret->backingStore = virStorageSourceCopy(src->backingStore, + true))) + goto error; + } + + return ret; + + error: + virStorageSourceFree(ret); + return NULL; +} + + void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def) { @@ -1771,11 +1954,11 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) } + + void virStorageSourceClear(virStorageSourcePtr def) { - size_t i; - if (!def) return; @@ -1786,12 +1969,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 63950ec..5c0460b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -349,6 +349,9 @@ bool virStorageSourceIsLocalStorage(virStorageSourcePtr src); 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 918b6d2..9838dfa 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1923,6 +1923,7 @@ virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; +virStorageSourceInitChainElement; virStorageSourceIsLocalStorage; virStorageSourceNewFromBacking; virStorageSourcePoolDefFree; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 3037435..cdbe24b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1902,6 +1902,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 5c0460b..7fd0bd7 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -342,6 +342,9 @@ void virStorageNetHostDefFree(size_t nhosts, virStorageNetHostDefPtr hosts); virStorageNetHostDefPtr virStorageNetHostDefCopy(size_t nhosts, virStorageNetHostDefPtr hosts); +int virStorageSourceInitChainElement(virStorageSourcePtr newelem, + virStorageSourcePtr old, + bool force); void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(virStorageSourcePtr def); -- 1.9.3

When discovering a disk backing chain the parent disk's metadata need to be populated into the guest images so that each piece of the backing chain contains a copy of those. This will allow us to refactor the security driver so that it will not need to carry around the original disk definition. --- src/util/virstoragefile.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index cdbe24b..591c2fa 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2334,9 +2334,17 @@ virStorageSourceNewFromBacking(virStorageSourcePtr parent) } } } + + /* copy parent's labelling and other top level stuff */ + if (virStorageSourceInitChainElement(ret, parent, false) < 0) + goto error; } return ret; + + error: + virStorageSourceFree(ret); + return NULL; } -- 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 9838dfa..0bc4853 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1058,6 +1058,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

On 07/04/2014 05:29 AM, Peter Krempa wrote: Hmm. Pre-existing, and not changed here (so this patch is still good), but we probably ought to start making the lease manager reserve backing files as readonly, rather than limiting the reservation to just the top-level file. (Did I already mention this in an earlier review?) At any rate, I suspect that we are leaking reservations when we do snapshots and/or commits, although I haven't played enough with the lock manager to actually prove or disprove that. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 07/04/2014 05:29 AM, Peter Krempa wrote: Oh, we are SOOO close to getting labelskip to be a per-image attribute. :) While touching this, s/these/these./ s/nodes/nodes./ [actually, it's probably better to integrate this into the lease manager, or similar solution, where we also track WHAT label to restore rather than just slamming a default label on - but that's been a pervasive problem that you don't need to solve here] While touching this, s/doing migrated/are doing migration/ ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 07/04/2014 05:29 AM, Peter Krempa wrote: Oops. This doesn't quite work with active commit (where top_parent is NULL, but where qemuDomainPrepareDiskChainElement isn't too happy to get a NULL pointer). But then again, I'm rebasing my active commit on top of you, so your patch is not wrong if it goes in first. If you want, restore the condition to '(top_parent && top_parent != disk->src &&...)'. Aha - squash this hunk into an earlier commit. ACK, whether or not you fix the NULL top_parent for active commit. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Use the source struct and the corresponding function so that we can avoid using the path separately. Now that qemuDomainPrepareDiskChainElementPath isn't use anywhere, we can safely remove it. Additionally, the removal fixes a misaligned comment as the removed function was added under a comment for a different function. --- src/qemu/qemu_driver.c | 31 ++++++------------------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1bd82f0..ffc5f67 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12104,25 +12104,6 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, * is sent but failed, and number of frozen filesystems on success. If -2 is * returned, FSThaw should be called revert the quiesced status. */ static int -qemuDomainPrepareDiskChainElementPath(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - const char *file, - qemuDomainDiskChainMode mode) -{ - virStorageSource src; - - memset(&src, 0, sizeof(src)); - - src.type = VIR_STORAGE_TYPE_FILE; - src.format = VIR_STORAGE_FILE_RAW; - src.path = (char *) file; /* casting away const is safe here */ - - return qemuDomainPrepareDiskChainElement(driver, vm, disk, &src, mode); -} - - -static int qemuDomainSnapshotFSFreeze(virQEMUDriverPtr driver, virDomainObjPtr vm, const char **mountpoints, @@ -15367,10 +15348,10 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if (VIR_STRDUP(mirror->path, dest) < 0) goto endjob; - if (qemuDomainPrepareDiskChainElementPath(driver, vm, disk, dest, - VIR_DISK_CHAIN_READ_WRITE) < 0) { - qemuDomainPrepareDiskChainElementPath(driver, vm, disk, dest, - VIR_DISK_CHAIN_NO_ACCESS); + if (qemuDomainPrepareDiskChainElement(driver, vm, disk, mirror, + VIR_DISK_CHAIN_READ_WRITE) < 0) { + qemuDomainPrepareDiskChainElement(driver, vm, disk, mirror, + VIR_DISK_CHAIN_NO_ACCESS); goto endjob; } @@ -15381,8 +15362,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, virDomainAuditDisk(vm, NULL, mirror, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { - qemuDomainPrepareDiskChainElementPath(driver, vm, disk, dest, - VIR_DISK_CHAIN_NO_ACCESS); + qemuDomainPrepareDiskChainElement(driver, vm, disk, mirror, + VIR_DISK_CHAIN_NO_ACCESS); goto endjob; } -- 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 ffc5f67..15c5f3b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14837,9 +14837,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) { @@ -14905,29 +14903,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); @@ -14942,9 +14928,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 @@ -14954,16 +14939,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, @@ -15348,6 +15334,9 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if (VIR_STRDUP(mirror->path, dest) < 0) goto endjob; + if (virStorageSourceInitChainElement(disk->mirror, disk->src, false) < 0) + goto endjob; + if (qemuDomainPrepareDiskChainElement(driver, vm, disk, mirror, VIR_DISK_CHAIN_READ_WRITE) < 0) { qemuDomainPrepareDiskChainElement(driver, vm, disk, mirror, -- 1.9.3

Until now we were changing information about the disk source via multiple steps of copying data. Now that we changed to a pointer to store the disk source we might use it to change the approach to track the data. Additionally this will allow proper tracking of the backing chain. --- src/qemu/qemu_driver.c | 122 ++++++++++++++----------------------------------- 1 file changed, 34 insertions(+), 88 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 15c5f3b..e648108 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12810,14 +12810,11 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr newDiskSrc = NULL; + virStorageSourcePtr persistDiskSrc = NULL; char *device = NULL; char *source = NULL; - char *newsource = NULL; - virStorageNetHostDefPtr newhosts = NULL; - virStorageNetHostDefPtr persistHosts = NULL; - int format = snap->src->format; const char *formatStr = NULL; - char *persistSource = NULL; int ret = -1; int fd = -1; bool need_unlink = false; @@ -12831,26 +12828,27 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0) goto cleanup; - /* XXX Here, we know we are about to alter disk->src->backingStore if - * successful, so we nuke the existing chain so that future commands will - * recompute it. Better would be storing the chain ourselves rather than - * reprobing, but this requires modifying domain_conf and our XML to fully - * track the chain across libvirtd restarts. */ - virStorageSourceBackingStoreClear(disk->src); - if (virStorageFileInit(snap->src) < 0) goto cleanup; if (qemuGetDriveSourceString(snap->src, NULL, &source) < 0) goto cleanup; - if (VIR_STRDUP(newsource, snap->src->path) < 0) + if (!(newDiskSrc = virStorageSourceCopy(snap->src, false))) goto cleanup; - if (persistDisk && - VIR_STRDUP(persistSource, snap->src->path) < 0) + if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup; + if (persistDisk) { + if (!(persistDiskSrc = virStorageSourceCopy(snap->src, false))) + goto cleanup; + + if (virStorageSourceInitChainElement(persistDiskSrc, persistDisk->src, + false) < 0) + goto cleanup; + } + switch ((virStorageType)snap->src->type) { case VIR_STORAGE_TYPE_BLOCK: case VIR_STORAGE_TYPE_FILE: @@ -12876,15 +12874,6 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, case VIR_STORAGE_TYPE_NETWORK: switch (snap->src->protocol) { case VIR_STORAGE_NET_PROTOCOL_GLUSTER: - if (!(newhosts = virStorageNetHostDefCopy(snap->src->nhosts, - snap->src->hosts))) - goto cleanup; - - if (persistDisk && - !(persistHosts = virStorageNetHostDefCopy(snap->src->nhosts, - snap->src->hosts))) - goto cleanup; - break; default: @@ -12935,45 +12924,24 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, /* Update vm in place to match changes. */ need_unlink = false; - VIR_FREE(disk->src->path); - virStorageNetHostDefFree(disk->src->nhosts, disk->src->hosts); - - disk->src->path = newsource; - disk->src->format = format; - disk->src->type = snap->src->type; - disk->src->protocol = snap->src->protocol; - disk->src->nhosts = snap->src->nhosts; - disk->src->hosts = newhosts; - - newsource = NULL; - newhosts = NULL; + newDiskSrc->backingStore = disk->src; + disk->src = newDiskSrc; + newDiskSrc = NULL; if (persistDisk) { - VIR_FREE(persistDisk->src->path); - virStorageNetHostDefFree(persistDisk->src->nhosts, - persistDisk->src->hosts); - - persistDisk->src->path = persistSource; - persistDisk->src->format = format; - persistDisk->src->type = snap->src->type; - persistDisk->src->protocol = snap->src->protocol; - persistDisk->src->nhosts = snap->src->nhosts; - persistDisk->src->hosts = persistHosts; - - persistSource = NULL; - persistHosts = NULL; + persistDiskSrc->backingStore = persistDisk->src; + persistDisk->src = persistDiskSrc; + persistDiskSrc = NULL; } cleanup: if (need_unlink && virStorageFileUnlink(snap->src)) VIR_WARN("unable to unlink just-created %s", source); virStorageFileDeinit(snap->src); + virStorageSourceFree(newDiskSrc); + virStorageSourceFree(persistDiskSrc); VIR_FREE(device); VIR_FREE(source); - VIR_FREE(newsource); - VIR_FREE(persistSource); - virStorageNetHostDefFree(snap->src->nhosts, newhosts); - virStorageNetHostDefFree(snap->src->nhosts, persistHosts); return ret; } @@ -12983,21 +12951,15 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, static void qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr origdisk, virDomainDiskDefPtr disk, virDomainDiskDefPtr persistDisk, bool need_unlink) { - char *source = NULL; - char *persistSource = NULL; + virStorageSourcePtr tmp; struct stat st; ignore_value(virStorageFileInit(disk->src)); - if (VIR_STRDUP(source, origdisk->src->path) < 0 || - (persistDisk && VIR_STRDUP(persistSource, source) < 0)) - goto cleanup; - qemuDomainPrepareDiskChainElement(driver, vm, disk, disk->src, VIR_DISK_CHAIN_NO_ACCESS); if (need_unlink && @@ -13005,35 +12967,20 @@ qemuDomainSnapshotUndoSingleDiskActive(virQEMUDriverPtr driver, virStorageFileUnlink(disk->src) < 0) VIR_WARN("Unable to remove just-created %s", disk->src->path); - /* Update vm in place to match changes. */ - VIR_FREE(disk->src->path); - disk->src->path = source; - source = NULL; - disk->src->format = origdisk->src->format; - disk->src->type = origdisk->src->type; - disk->src->protocol = origdisk->src->protocol; - virStorageNetHostDefFree(disk->src->nhosts, disk->src->hosts); - disk->src->nhosts = origdisk->src->nhosts; - disk->src->hosts = virStorageNetHostDefCopy(origdisk->src->nhosts, - origdisk->src->hosts); + virStorageFileDeinit(disk->src); + + /* Update vm in place to match changes. */ + tmp = disk->src; + disk->src = tmp->backingStore; + tmp->backingStore = NULL; + virStorageSourceFree(tmp); + if (persistDisk) { - VIR_FREE(persistDisk->src->path); - persistDisk->src->path = persistSource; - persistSource = NULL; - persistDisk->src->format = origdisk->src->format; - persistDisk->src->type = origdisk->src->type; - persistDisk->src->protocol = origdisk->src->protocol; - virStorageNetHostDefFree(persistDisk->src->nhosts, - persistDisk->src->hosts); - persistDisk->src->nhosts = origdisk->src->nhosts; - persistDisk->src->hosts = virStorageNetHostDefCopy(origdisk->src->nhosts, - origdisk->src->hosts); + tmp = persistDisk->src; + persistDisk->src = tmp->backingStore; + tmp->backingStore = NULL; + virStorageSourceFree(tmp); } - - cleanup: - virStorageFileDeinit(disk->src); - VIR_FREE(source); - VIR_FREE(persistSource); } /* The domain is expected to be locked and active. */ @@ -13137,7 +13084,6 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver, } qemuDomainSnapshotUndoSingleDiskActive(driver, vm, - snap->def->dom->disks[i], vm->def->disks[i], persistDisk, need_unlink); -- 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 e648108..6721fc9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12040,61 +12040,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; } @@ -12863,9 +12854,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; } @@ -12960,7 +12951,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) && @@ -15283,9 +15274,9 @@ qemuDomainBlockCopy(virDomainObjPtr vm, if (virStorageSourceInitChainElement(disk->mirror, disk->src, false) < 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; } @@ -15297,7 +15288,7 @@ qemuDomainBlockCopy(virDomainObjPtr vm, virDomainAuditDisk(vm, NULL, mirror, "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; } @@ -15499,10 +15490,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; @@ -15522,10 +15513,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

Now that cgroups/security driver/locking driver support labelling of individual images and tolerate network storage we don't have to refrain from passing all image files to it. This allows to remove checking code as we already make sure that the snapshot function won't be called with unsupported options. --- src/qemu/qemu_driver.c | 62 +++++++++++++++----------------------------------- 1 file changed, 18 insertions(+), 44 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6721fc9..6623638 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12819,16 +12819,16 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (virAsprintf(&device, "drive-%s", disk->info.alias) < 0) goto cleanup; - if (virStorageFileInit(snap->src) < 0) + if (!(newDiskSrc = virStorageSourceCopy(snap->src, false))) goto cleanup; - if (qemuGetDriveSourceString(snap->src, NULL, &source) < 0) + if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup; - if (!(newDiskSrc = virStorageSourceCopy(snap->src, false))) + if (virStorageFileInit(newDiskSrc) < 0) goto cleanup; - if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) + if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) goto cleanup; if (persistDisk) { @@ -12840,55 +12840,29 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, goto cleanup; } - switch ((virStorageType)snap->src->type) { - case VIR_STORAGE_TYPE_BLOCK: - case VIR_STORAGE_TYPE_FILE: - - /* create the stub file and set selinux labels; manipulate disk in - * place, in a way that can be reverted on failure. */ - if (!reuse && snap->src->type != VIR_STORAGE_TYPE_BLOCK) { + /* 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 (qemuDomainPrepareDiskChainElement(driver, vm, snap->src, - VIR_DISK_CHAIN_READ_WRITE) < 0) { - qemuDomainPrepareDiskChainElement(driver, vm, snap->src, - VIR_DISK_CHAIN_NO_ACCESS); - goto cleanup; - } - break; - - case VIR_STORAGE_TYPE_NETWORK: - switch (snap->src->protocol) { - case VIR_STORAGE_NET_PROTOCOL_GLUSTER: - break; - - default: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("snapshots on volumes using '%s' protocol " - "are not supported"), - virStorageNetProtocolTypeToString(snap->src->protocol)); - goto cleanup; - } - break; - - case VIR_STORAGE_TYPE_DIR: - case VIR_STORAGE_TYPE_VOLUME: - case VIR_STORAGE_TYPE_NONE: - case VIR_STORAGE_TYPE_LAST: - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("snapshots are not supported on '%s' volumes"), - virStorageTypeToString(snap->src->type)); + /* set correct security, cgroup and locking options on the new image */ + if (qemuDomainPrepareDiskChainElement(driver, vm, newDiskSrc, + VIR_DISK_CHAIN_READ_WRITE) < 0) { + qemuDomainPrepareDiskChainElement(driver, vm, newDiskSrc, + VIR_DISK_CHAIN_NO_ACCESS); goto cleanup; } /* create the actual snapshot */ - if (snap->src->format) - formatStr = virStorageFileFormatTypeToString(snap->src->format); + if (newDiskSrc->format) + formatStr = virStorageFileFormatTypeToString(newDiskSrc->format); /* The monitor is only accessed if qemu doesn't support transactions. * Otherwise the following monitor command only constructs the command. @@ -12926,9 +12900,9 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } cleanup: - if (need_unlink && virStorageFileUnlink(snap->src)) + if (need_unlink && virStorageFileUnlink(newDiskSrc)) VIR_WARN("unable to unlink just-created %s", source); - virStorageFileDeinit(snap->src); + virStorageFileDeinit(newDiskSrc); virStorageSourceFree(newDiskSrc); virStorageSourceFree(persistDiskSrc); VIR_FREE(device); -- 1.9.3