[libvirt] [PATCHv5 00/28] Refactor changing of domain disk source
v4 wasn't reviewed, but recent patches caused pretty serious conflicts thus I'm posting a rebased version. Note that one of the patches was already pushed as a part of a different series. This series is also a preparation for fixing issue with libvirt not setting correct permissions on gluster volumes. Peter Krempa (28): storage: Implement virStorageFileCreate for local and gluster files qemu: Don't propagate whole disk definition into qemuDomainGetImageIds qemu: Add helper to initialize storage file backend with correct uid/gid storage: file: Tolerate NULL src when uninitializing the backend conf: Don't output seclabels for backingStore elements storage: Move readonly and shared flags to disk source from disk def util: storagefile: Add deep copy for struct virStorageSource util: storage: Add function to transfer config parts to new chain element util: storage: Copy parent's disk metadata to backing chain elements util: cgroup: Add helper to convert device mode to string qemu: cgroup: Add functions to set cgroup image stuff on individual imgs qemu: cgroup: Setup only the top level disk image for read-write access locking: Add APIs to lock individual image files security: Introduce APIs to label single images security: selinux: Implement per-image seclabel restore security: selinux: Implement per-image seclabel set security: DAC: Implement per-image seclabel restore security: DAC: Implement per-image seclabel set security: AppArmor: Implement per-image seclabel restore security: AppArmor: Implement per-image seclabel set util: storage: Make virStorageFileChainLookup more network storage aware util: storage: Return complete parent info from virStorageFileChainLookup qemu: blockcopy: Use the mirror disk source to label the files qemu: block: Properly track disk source while pivotting to new image qemu: snapshot: Improve approach to deal with snapshot metadata qemu: Refactor qemuDomainPrepareDiskChainElement qemu: snapshot: Refactor image labelling of new snapshot files qemu: snapshot: Use storage driver to pre-create snapshot file src/conf/domain_conf.c | 77 +++++--- src/conf/domain_conf.h | 2 - src/libvirt_private.syms | 7 + src/libxl/libxl_conf.c | 2 +- src/locking/domain_lock.c | 65 ++++--- src/locking/domain_lock.h | 8 + src/lxc/lxc_cgroup.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 2 +- src/qemu/qemu_cgroup.c | 109 ++++++----- src/qemu/qemu_cgroup.h | 3 + src/qemu/qemu_command.c | 14 +- src/qemu/qemu_conf.c | 4 +- src/qemu/qemu_domain.c | 29 ++- src/qemu/qemu_domain.h | 4 + src/qemu/qemu_driver.c | 350 +++++++++++----------------------- src/qemu/qemu_migration.c | 16 +- src/security/security_apparmor.c | 55 ++++-- src/security/security_dac.c | 111 +++++------ 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_selinux.c | 150 +++++++++------ src/security/security_stack.c | 38 ++++ src/security/virt-aa-helper.c | 2 +- src/storage/storage_backend_fs.c | 17 ++ src/storage/storage_backend_gluster.c | 28 +++ src/storage/storage_driver.c | 2 +- src/util/vircgroup.c | 62 ++++-- src/util/vircgroup.h | 2 + src/util/virstoragefile.c | 280 ++++++++++++++++++++++++--- src/util/virstoragefile.h | 14 +- src/vbox/vbox_tmpl.c | 30 +-- src/xenxs/xen_sxpr.c | 10 +- src/xenxs/xen_xm.c | 10 +- tests/virstoragetest.c | 86 ++++----- 37 files changed, 1075 insertions(+), 610 deletions(-) -- 1.9.3
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 1a2b4ec..ae23999 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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(+)
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) {
Isn't our sentinel value -1 for default? I think that comparison against 0 is likely to be wrong for running qemu:///session. ACK with this change: if (src->drv->uid != (uid_t)-1 || src->drv->gid != (gid_t)-1) { -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/08/14 00:55, Eric Blake wrote:
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(+)
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) {
Isn't our sentinel value -1 for default? I think that comparison against 0 is likely to be wrong for running qemu:///session.
ACK with this change:
if (src->drv->uid != (uid_t)-1 || src->drv->gid != (gid_t)-1) {
Actually the init code changes the sentinel value -1 to the actual uid, so what I wanted for this condition is: if (src->drv->uid != getuid() || .. .. or possibly geteuid, I'll have to look up whether gluster uses the effective uid, or the actual uid. Additionally as a non-root cannot give up ownership of a file, we should probably make it a little stricter and not call it if get(e)uid() isn't root. Peter
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
It will help re-using the function. --- src/qemu/qemu_domain.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(+)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
Allow de-init of null storage sources. --- src/storage/storage_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
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; }
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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.
I agree - it's easier to be conservative now and turn it on later than it is to turn it on now and be stuck with it for back-compat.
--- src/conf/domain_conf.c | 59 +++++++++++++++++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 17 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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. ---
Mostly mechanical. ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(-)
@@ -1771,11 +1954,11 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) }
+ + void virStorageSourceClear(virStorageSourcePtr def) {
Extra newline here. ACK with that fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(+)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(+)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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:
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(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/08/14 06:03, Eric Blake wrote:
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(-)
ACK
Thanks, I've pushed 2 through to 10. Peter
Add functions that will allow to set all the required cgroup stuff on individual images taking a virStorageSourcePtr. Also convert functions designed to setup whole backing chain to take advantage of the change. --- src/qemu/qemu_cgroup.c | 103 ++++++++++++++++++++++++------------------------- src/qemu/qemu_cgroup.h | 3 ++ 2 files changed, 54 insertions(+), 52 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3394c68..c84a251 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -49,30 +49,55 @@ static const char *const defaultDeviceACL[] = { #define DEVICE_PTY_MAJOR 136 #define DEVICE_SND_MAJOR 116 -static int -qemuSetupDiskPathAllow(virDomainDiskDefPtr disk, - const char *path, - size_t depth ATTRIBUTE_UNUSED, - void *opaque) +int +qemuSetImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src, + bool deny) { - virDomainObjPtr vm = opaque; qemuDomainObjPrivatePtr priv = vm->privateData; + int perms = VIR_CGROUP_DEVICE_READ; int ret; - VIR_DEBUG("Process path %s for disk", path); - ret = virCgroupAllowDevicePath(priv->cgroup, path, - (disk->src->readonly ? VIR_CGROUP_DEVICE_READ - : VIR_CGROUP_DEVICE_RW)); - virDomainAuditCgroupPath(vm, priv->cgroup, "allow", path, - disk->src->readonly ? "r" : "rw", ret == 0); + if (!virCgroupHasController(priv->cgroup, + VIR_CGROUP_CONTROLLER_DEVICES)) + return 0; + + if (!src->path || !virStorageSourceIsLocalStorage(src)) { + VIR_DEBUG("Not updating cgroups for disk path '%s', type: %s", + NULLSTR(src->path), virStorageTypeToString(src->type)); + return 0; + } + + if (deny) { + perms |= VIR_CGROUP_DEVICE_WRITE | VIR_CGROUP_DEVICE_MKNOD; + + VIR_DEBUG("Deny path %s", src->path); + + ret = virCgroupDenyDevicePath(priv->cgroup, src->path, perms); + } else { + if (!src->readonly) + perms |= VIR_CGROUP_DEVICE_WRITE; + + VIR_DEBUG("Allow path %s, perms: %s", + src->path, virCgroupGetDevicePermsString(perms)); + + ret = virCgroupAllowDevicePath(priv->cgroup, src->path, perms); + } + + virDomainAuditCgroupPath(vm, priv->cgroup, + deny ? "deny" : "allow", + src->path, + virCgroupGetDevicePermsString(perms), + ret == 0); /* Get this for root squash NFS */ if (ret < 0 && virLastErrorIsSystemErrno(EACCES)) { - VIR_DEBUG("Ignoring EACCES for %s", path); + VIR_DEBUG("Ignoring EACCES for %s", src->path); virResetLastError(); ret = 0; } + return ret; } @@ -81,39 +106,14 @@ int qemuSetupDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk) { - qemuDomainObjPrivatePtr priv = vm->privateData; - - if (!virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_DEVICES)) - return 0; - - return virDomainDiskDefForeachPath(disk, true, qemuSetupDiskPathAllow, vm); -} + virStorageSourcePtr next; - -static int -qemuTeardownDiskPathDeny(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, - const char *path, - size_t depth ATTRIBUTE_UNUSED, - void *opaque) -{ - virDomainObjPtr vm = opaque; - qemuDomainObjPrivatePtr priv = vm->privateData; - int ret; - - VIR_DEBUG("Process path %s for disk", path); - ret = virCgroupDenyDevicePath(priv->cgroup, path, - VIR_CGROUP_DEVICE_RWM); - virDomainAuditCgroupPath(vm, priv->cgroup, "deny", path, "rwm", ret == 0); - - /* Get this for root squash NFS */ - if (ret < 0 && - virLastErrorIsSystemErrno(EACCES)) { - VIR_DEBUG("Ignoring EACCES for %s", path); - virResetLastError(); - ret = 0; + for (next = disk->src; next; next = next->backingStore) { + if (qemuSetImageCgroup(vm, next, false) < 0) + return -1; } - return ret; + + return 0; } @@ -121,18 +121,17 @@ int qemuTeardownDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk) { - qemuDomainObjPrivatePtr priv = vm->privateData; + virStorageSourcePtr next; - if (!virCgroupHasController(priv->cgroup, - VIR_CGROUP_CONTROLLER_DEVICES)) - return 0; + for (next = disk->src; next; next = next->backingStore) { + if (qemuSetImageCgroup(vm, next, true) < 0) + return -1; + } - return virDomainDiskDefForeachPath(disk, - true, - qemuTeardownDiskPathDeny, - vm); + return 0; } + static int qemuSetupChrSourceCgroup(virDomainDefPtr def ATTRIBUTE_UNUSED, virDomainChrSourceDefPtr dev, diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 14404d1..732860e 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -29,6 +29,9 @@ # include "domain_conf.h" # include "qemu_conf.h" +int qemuSetImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src, + bool deny); int qemuSetupDiskCgroup(virDomainObjPtr vm, virDomainDiskDefPtr disk); int qemuTeardownDiskCgroup(virDomainObjPtr vm, -- 1.9.3
On 07/04/2014 05:29 AM, Peter Krempa wrote:
Add functions that will allow to set all the required cgroup stuff on individual images taking a virStorageSourcePtr. Also convert functions designed to setup whole backing chain to take advantage of the change. --- src/qemu/qemu_cgroup.c | 103 ++++++++++++++++++++++++------------------------- src/qemu/qemu_cgroup.h | 3 ++ 2 files changed, 54 insertions(+), 52 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add helper APIs to manage individual image files rather than disks. To simplify the addition some parts of the code were refactored in this patch. --- src/libvirt_private.syms | 2 ++ src/locking/domain_lock.c | 65 ++++++++++++++++++++++++++++++----------------- src/locking/domain_lock.h | 8 ++++++ 3 files changed, 52 insertions(+), 23 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 0bc4853..064db9f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -861,6 +861,8 @@ virRegisterStorageDriver; # locking/domain_lock.h virDomainLockDiskAttach; virDomainLockDiskDetach; +virDomainLockImageAttach; +virDomainLockImageDetach; virDomainLockLeaseAttach; virDomainLockLeaseDetach; virDomainLockProcessInquire; diff --git a/src/locking/domain_lock.c b/src/locking/domain_lock.c index 78acaa6..d7b681e 100644 --- a/src/locking/domain_lock.c +++ b/src/locking/domain_lock.c @@ -68,14 +68,13 @@ static int virDomainLockManagerAddLease(virLockManagerPtr lock, } -static int virDomainLockManagerAddDisk(virLockManagerPtr lock, - virDomainDiskDefPtr disk) +static int virDomainLockManagerAddImage(virLockManagerPtr lock, + virStorageSourcePtr src) { unsigned int diskFlags = 0; - const char *src = virDomainDiskGetSource(disk); - int type = virDomainDiskGetType(disk); + int type = virStorageSourceGetActualType(src); - if (!src) + if (!src->path) return 0; if (!(type == VIR_STORAGE_TYPE_BLOCK || @@ -83,24 +82,25 @@ static int virDomainLockManagerAddDisk(virLockManagerPtr lock, type == VIR_STORAGE_TYPE_DIR)) return 0; - if (disk->src->readonly) + if (src->readonly) diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY; - if (disk->src->shared) + if (src->shared) diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED; - VIR_DEBUG("Add disk %s", src); + VIR_DEBUG("Add disk %s", src->path); if (virLockManagerAddResource(lock, VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK, - src, + src->path, 0, NULL, diskFlags) < 0) { - VIR_DEBUG("Failed add disk %s", src); + VIR_DEBUG("Failed add disk %s", src->path); return -1; } return 0; } + static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, const char *uri, virDomainObjPtr dom, @@ -148,9 +148,12 @@ static virLockManagerPtr virDomainLockManagerNew(virLockManagerPluginPtr plugin, goto error; VIR_DEBUG("Adding disks"); - for (i = 0; i < dom->def->ndisks; i++) - if (virDomainLockManagerAddDisk(lock, dom->def->disks[i]) < 0) + for (i = 0; i < dom->def->ndisks; i++) { + virDomainDiskDefPtr disk = dom->def->disks[i]; + + if (virDomainLockManagerAddImage(lock, disk->src) < 0) goto error; + } } return lock; @@ -247,21 +250,20 @@ int virDomainLockProcessInquire(virLockManagerPluginPtr plugin, } -int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, - const char *uri, - virDomainObjPtr dom, - virDomainDiskDefPtr disk) +int virDomainLockImageAttach(virLockManagerPluginPtr plugin, + const char *uri, + virDomainObjPtr dom, + virStorageSourcePtr src) { virLockManagerPtr lock; int ret = -1; - VIR_DEBUG("plugin=%p dom=%p disk=%p", - plugin, dom, disk); + VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); if (!(lock = virDomainLockManagerNew(plugin, uri, dom, false))) return -1; - if (virDomainLockManagerAddDisk(lock, disk) < 0) + if (virDomainLockManagerAddImage(lock, src) < 0) goto cleanup; if (virLockManagerAcquire(lock, NULL, 0, @@ -276,20 +278,29 @@ int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, return ret; } -int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, + +int virDomainLockDiskAttach(virLockManagerPluginPtr plugin, + const char *uri, virDomainObjPtr dom, virDomainDiskDefPtr disk) { + return virDomainLockImageAttach(plugin, uri, dom, disk->src); +} + + +int virDomainLockImageDetach(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src) +{ virLockManagerPtr lock; int ret = -1; - VIR_DEBUG("plugin=%p dom=%p disk=%p", - plugin, dom, disk); + VIR_DEBUG("plugin=%p dom=%p src=%p", plugin, dom, src); if (!(lock = virDomainLockManagerNew(plugin, NULL, dom, false))) return -1; - if (virDomainLockManagerAddDisk(lock, disk) < 0) + if (virDomainLockManagerAddImage(lock, src) < 0) goto cleanup; if (virLockManagerRelease(lock, NULL, 0) < 0) @@ -304,6 +315,14 @@ int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, } +int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virDomainDiskDefPtr disk) +{ + return virDomainLockImageDetach(plugin, dom, disk->src); +} + + int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, const char *uri, virDomainObjPtr dom, diff --git a/src/locking/domain_lock.h b/src/locking/domain_lock.h index a9b19c8..fb49102 100644 --- a/src/locking/domain_lock.h +++ b/src/locking/domain_lock.h @@ -50,6 +50,14 @@ int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, virDomainObjPtr dom, virDomainDiskDefPtr disk); +int virDomainLockImageAttach(virLockManagerPluginPtr plugin, + const char *uri, + virDomainObjPtr dom, + virStorageSourcePtr src); +int virDomainLockImageDetach(virLockManagerPluginPtr plugin, + virDomainObjPtr dom, + virStorageSourcePtr src); + int virDomainLockLeaseAttach(virLockManagerPluginPtr plugin, const char *uri, virDomainObjPtr dom, -- 1.9.3
On 07/04/2014 05:29 AM, Peter Krempa wrote:
Add helper APIs to manage individual image files rather than disks. To simplify the addition some parts of the code were refactored in this patch. --- src/libvirt_private.syms | 2 ++ src/locking/domain_lock.c | 65 ++++++++++++++++++++++++++++++----------------- src/locking/domain_lock.h | 8 ++++++ 3 files changed, 52 insertions(+), 23 deletions(-)
@@ -83,24 +82,25 @@ static int virDomainLockManagerAddDisk(virLockManagerPtr lock, type == VIR_STORAGE_TYPE_DIR)) return 0;
- if (disk->src->readonly) + if (src->readonly) diskFlags |= VIR_LOCK_MANAGER_RESOURCE_READONLY; - if (disk->src->shared) + if (src->shared) diskFlags |= VIR_LOCK_MANAGER_RESOURCE_SHARED;
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
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 064db9f..f3efd27 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -923,6 +923,7 @@ virSecurityManagerReserveLabel; virSecurityManagerRestoreAllLabel; virSecurityManagerRestoreDiskLabel; virSecurityManagerRestoreHostdevLabel; +virSecurityManagerRestoreImageLabel; virSecurityManagerRestoreSavedStateLabel; virSecurityManagerSetAllLabel; virSecurityManagerSetChildProcessLabel; @@ -931,6 +932,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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(+)
+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;
Pre-existing with virSecurityStackSetSecurityDiskLabel(), but on failure later in the stack, should we attempt to undo the labels set earlier in the stack, rather than leaving things half-labeled? If so, that's a separate patch, so it doesn't affect my review here.
+} + +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; + } +
Cleanup, of course, is best effort. Also pre-existing, and not affecting my review here - should cleanup be done in reverse stack order, in case we ever hit a situation in the future where the label applied by the first stack layer affects the ability of the second stack layer to operate, and therefore must be undone in opposite order? ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Refactor the existing code to allow re-using it for the per-image label restore too. --- src/security/security_selinux.c | 59 ++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7740e69..87077ac 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1123,18 +1123,20 @@ virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr, static int virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, virDomainDefPtr def, - virDomainDiskDefPtr disk, + virStorageSourcePtr src, bool migrated) { virSecurityLabelDefPtr seclabel; virSecurityDeviceLabelDefPtr disk_seclabel; - const char *src = virDomainDiskGetSource(disk); + + if (!src->path || !virStorageSourceIsLocalStorage(src)) + return 0; seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); if (seclabel == NULL) return 0; - disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, + disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_SELINUX_NAME); if (seclabel->norelabel || (disk_seclabel && disk_seclabel->norelabel)) return 0; @@ -1144,40 +1146,35 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, * be tracked in domain XML, at which point labelskip should be a * per-file attribute instead of a disk attribute. */ if (disk_seclabel && disk_seclabel->labelskip && - !disk->src->backingStore) + !src->backingStore) return 0; - /* Don't restore labels on readoly/shared disks, because - * other VMs may still be accessing these - * Alternatively we could iterate over all running - * domains and try to figure out if it is in use, but - * this would not work for clustered filesystems, since - * we can't see running VMs using the file on other nodes - * Safest bet is thus to skip the restore step. + /* Don't restore labels on readoly/shared disks, because other VMs may + * still be accessing these Alternatively we could iterate over all running + * domains and try to figure out if it is in use, but this would not work + * for clustered filesystems, since we can't see running VMs using the file + * on other nodes Safest bet is thus to skip the restore step. */ - if (disk->src->readonly || disk->src->shared) + if (src->readonly || src->shared) return 0; - if (!src || virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_NETWORK) - return 0; - /* If we have a shared FS & doing migrated, we must not - * change ownership, because that kills access on the - * destination host which is sub-optimal for the guest - * VM's I/O attempts :-) + /* If we have a shared FS & doing migrated, we must not change ownership, + * because that kills access on the destination host which is sub-optimal + * for the guest VM's I/O attempts :-) */ if (migrated) { - int rc = virFileIsSharedFS(src); + int rc = virFileIsSharedFS(src->path); if (rc < 0) return -1; if (rc == 1) { VIR_DEBUG("Skipping image label restore on %s because FS is shared", - src); + src->path); return 0; } } - return virSecuritySELinuxRestoreSecurityFileLabel(mgr, src); + return virSecuritySELinuxRestoreSecurityFileLabel(mgr, src->path); } @@ -1186,7 +1183,17 @@ virSecuritySELinuxRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk) { - return virSecuritySELinuxRestoreSecurityImageLabelInt(mgr, def, disk, false); + return virSecuritySELinuxRestoreSecurityImageLabelInt(mgr, def, disk->src, + false); +} + + +static int +virSecuritySELinuxRestoreSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src) +{ + return virSecuritySELinuxRestoreSecurityImageLabelInt(mgr, def, src, false); } @@ -1867,9 +1874,9 @@ virSecuritySELinuxRestoreSecurityAllLabel(virSecurityManagerPtr mgr, rc = -1; } for (i = 0; i < def->ndisks; i++) { - if (virSecuritySELinuxRestoreSecurityImageLabelInt(mgr, - def, - def->disks[i], + virDomainDiskDefPtr disk = def->disks[i]; + + if (virSecuritySELinuxRestoreSecurityImageLabelInt(mgr, def, disk->src, migrated) < 0) rc = -1; } @@ -2429,6 +2436,8 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSetSecurityDiskLabel = virSecuritySELinuxSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = virSecuritySELinuxRestoreSecurityDiskLabel, + .domainRestoreSecurityImageLabel = virSecuritySELinuxRestoreSecurityImageLabel, + .domainSetSecurityDaemonSocketLabel = virSecuritySELinuxSetSecurityDaemonSocketLabel, .domainSetSecuritySocketLabel = virSecuritySELinuxSetSecuritySocketLabel, .domainClearSecuritySocketLabel = virSecuritySELinuxClearSecuritySocketLabel, -- 1.9.3
On 07/04/2014 05:29 AM, Peter Krempa wrote:
Refactor the existing code to allow re-using it for the per-image label restore too. --- src/security/security_selinux.c | 59 ++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 25 deletions(-)
@@ -1144,40 +1146,35 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, * be tracked in domain XML, at which point labelskip should be a * per-file attribute instead of a disk attribute. */ if (disk_seclabel && disk_seclabel->labelskip &&
Oh, we are SOOO close to getting labelskip to be a per-image attribute. :)
+ /* Don't restore labels on readoly/shared disks, because other VMs may + * still be accessing these Alternatively we could iterate over all running
While touching this, s/these/these./
+ * domains and try to figure out if it is in use, but this would not work + * for clustered filesystems, since we can't see running VMs using the file + * on other nodes Safest bet is thus to skip the restore step.
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]
- /* If we have a shared FS & doing migrated, we must not - * change ownership, because that kills access on the - * destination host which is sub-optimal for the guest - * VM's I/O attempts :-) + /* If we have a shared FS & doing migrated, we must not change ownership,
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
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
Refactor the code and reuse it to implement the functionality. --- src/security/security_selinux.c | 91 ++++++++++++++++++++++++----------------- 1 file changed, 53 insertions(+), 38 deletions(-)
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;
Similar question to earlier patches about pre-existing design - if we fail partway through, should we undo our earlier labels on the exit path? But as before, that would be a separate patch (this one is focusing on the refactor with no semantic change, while that patch would be an intentional behavior change). ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Refactor the existing code to allow re-using it for the per-image label restore too. --- src/security/security_dac.c | 59 ++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 38cb47f..f86d532 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -350,62 +350,63 @@ virSecurityDACSetSecurityDiskLabel(virSecurityManagerPtr mgr, static int virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, virDomainDefPtr def, - virDomainDiskDefPtr disk, + virStorageSourcePtr src, bool migrated) { virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityLabelDefPtr secdef; virSecurityDeviceLabelDefPtr disk_seclabel; - const char *src = virDomainDiskGetSource(disk); if (!priv->dynamicOwnership) return 0; - if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_NETWORK) + if (!src->path || !virStorageSourceIsLocalStorage(src)) return 0; - secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + /* Don't restore labels on readoly/shared disks, because other VMs may + * still be accessing these Alternatively we could iterate over all running + * domains and try to figure out if it is in use, but this would not work + * for clustered filesystems, since we can't see running VMs using the file + * on other nodes Safest bet is thus to skip the restore step. + */ + if (src->readonly || src->shared) + return 0; + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); if (secdef && secdef->norelabel) return 0; - disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, + disk_seclabel = virStorageSourceGetSecurityLabelDef(src, SECURITY_DAC_NAME); - if (disk_seclabel && disk_seclabel->norelabel) return 0; - /* Don't restore labels on readoly/shared disks, because - * other VMs may still be accessing these - * Alternatively we could iterate over all running - * domains and try to figure out if it is in use, but - * this would not work for clustered filesystems, since - * we can't see running VMs using the file on other nodes - * Safest bet is thus to skip the restore step. - */ - if (disk->src->readonly || disk->src->shared) - return 0; - - if (!src) - return 0; - /* If we have a shared FS & doing migrated, we must not - * change ownership, because that kills access on the - * destination host which is sub-optimal for the guest - * VM's I/O attempts :-) + /* If we have a shared FS & doing migrated, we must not change ownership, + * because that kills access on the destination host which is sub-optimal + * for the guest VM's I/O attempts :-) */ if (migrated) { - int rc = virFileIsSharedFS(src); + int rc = virFileIsSharedFS(src->path); if (rc < 0) return -1; if (rc == 1) { VIR_DEBUG("Skipping image label restore on %s because FS is shared", - src); + src->path); return 0; } } - return virSecurityDACRestoreSecurityFileLabel(src); + return virSecurityDACRestoreSecurityFileLabel(src->path); +} + + +static int +virSecurityDACRestoreSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src) +{ + return virSecurityDACRestoreSecurityImageLabelInt(mgr, def, src, false); } @@ -414,7 +415,7 @@ virSecurityDACRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk) { - return virSecurityDACRestoreSecurityImageLabelInt(mgr, def, disk, false); + return virSecurityDACRestoreSecurityImageLabelInt(mgr, def, disk->src, false); } @@ -902,7 +903,7 @@ virSecurityDACRestoreSecurityAllLabel(virSecurityManagerPtr mgr, for (i = 0; i < def->ndisks; i++) { if (virSecurityDACRestoreSecurityImageLabelInt(mgr, def, - def->disks[i], + def->disks[i]->src, migrated) < 0) rc = -1; } @@ -1276,6 +1277,8 @@ virSecurityDriver virSecurityDriverDAC = { .domainSetSecurityDiskLabel = virSecurityDACSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = virSecurityDACRestoreSecurityDiskLabel, + .domainRestoreSecurityImageLabel = virSecurityDACRestoreSecurityImageLabel, + .domainSetSecurityDaemonSocketLabel = virSecurityDACSetDaemonSocketLabel, .domainSetSecuritySocketLabel = virSecurityDACSetSocketLabel, .domainClearSecuritySocketLabel = virSecurityDACClearSocketLabel, -- 1.9.3
On 07/04/2014 05:29 AM, Peter Krempa wrote:
Refactor the existing code to allow re-using it for the per-image label restore too. --- src/security/security_dac.c | 59 ++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 28 deletions(-)
- secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME); + /* Don't restore labels on readoly/shared disks, because other VMs may + * still be accessing these Alternatively we could iterate over all running
Pre-existing typo, but s/these/these./
+ * domains and try to figure out if it is in use, but this would not work + * for clustered filesystems, since we can't see running VMs using the file + * on other nodes Safest bet is thus to skip the restore step.
s/nodes/nodes./
- /* If we have a shared FS & doing migrated, we must not - * change ownership, because that kills access on the - * destination host which is sub-optimal for the guest - * VM's I/O attempts :-) + /* If we have a shared FS & doing migrated, we must not change ownership,
s/doing migrated/are doing migration/
+ * because that kills access on the destination host which is sub-optimal + * for the guest VM's I/O attempts :-)
Technically, changing SELinux labels kills guests (that takes effect immediately); but per POSIX, calling chmod() should not (as chmod only affects future open() calls, but not already-open fds). But then you throw in non-POSIX NFS, and there we have a case where chmod() can kill access to open fds (naughty NFS). So this comment is true, but only because of botched file systems :) ACK with comment cleanups. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
Refactor the code and reuse it to implement the functionality. --- src/security/security_dac.c | 52 ++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 27 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Refactor the existing code to allow re-using it for the per-image label restore too. --- src/security/security_apparmor.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index b4cbc61..391bf60 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -684,16 +684,24 @@ AppArmorClearSecuritySocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, /* Called when hotplugging */ static int -AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) +AppArmorRestoreSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src) { - if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_NETWORK) + if (!virStorageSourceIsLocalStorage(src)) return 0; return reload_profile(mgr, def, NULL, false); } +static int +AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk) +{ + return AppArmorRestoreSecurityImageLabel(mgr, def, disk->src); +} + /* Called when hotplugging */ static int AppArmorSetSecurityDiskLabel(virSecurityManagerPtr mgr, @@ -975,6 +983,8 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetSecurityDiskLabel = AppArmorSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = AppArmorRestoreSecurityDiskLabel, + .domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel, + .domainSetSecurityDaemonSocketLabel = AppArmorSetSecurityDaemonSocketLabel, .domainSetSecuritySocketLabel = AppArmorSetSecuritySocketLabel, .domainClearSecuritySocketLabel = AppArmorClearSecuritySocketLabel, -- 1.9.3
On 07/04/2014 05:29 AM, Peter Krempa wrote:
Refactor the existing code to allow re-using it for the per-image label restore too. --- src/security/security_apparmor.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
Refactor the code and reuse it to implement the functionality. --- src/security/security_apparmor.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Add a few checks and avoid resolving relative links on networked storage. --- src/util/virstoragefile.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 591c2fa..066973f 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1350,13 +1350,12 @@ virStorageFileChainLookup(virStorageSourcePtr chain, const char *tmp; char *parentDir = NULL; bool nameIsFile = virStorageIsFile(name); - size_t i; + size_t i = 0; if (!parent) parent = &tmp; *parent = NULL; - i = 0; if (startFrom) { while (chain && chain != startFrom->backingStore) { chain = chain->backingStore; @@ -1377,24 +1376,27 @@ virStorageFileChainLookup(virStorageSourcePtr chain, if (STREQ_NULLABLE(name, chain->relPath) || STREQ(name, chain->path)) break; - if (nameIsFile && (chain->type == VIR_STORAGE_TYPE_FILE || - chain->type == VIR_STORAGE_TYPE_BLOCK)) { - if (prev) { - if (!(parentDir = mdir_name(prev->path))) { - virReportOOMError(); - goto error; - } - } else { - if (VIR_STRDUP(parentDir, ".") < 0) - goto error; + + if (nameIsFile && virStorageSourceIsLocalStorage(chain)) { + if (prev && virStorageSourceIsLocalStorage(prev)) + parentDir = mdir_name(prev->path); + else + ignore_value(VIR_STRDUP(parentDir, ".")); + + if (!parentDir) { + virReportOOMError(); + goto error; } + int result = virFileRelLinkPointsTo(parentDir, name, chain->path); VIR_FREE(parentDir); + if (result < 0) goto error; + if (result > 0) break; } @@ -1407,6 +1409,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, if (!chain) goto error; + return chain; error: -- 1.9.3
On 07/04/2014 05:29 AM, Peter Krempa wrote:
Add a few checks and avoid resolving relative links on networked storage. --- src/util/virstoragefile.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-)
+ + if (nameIsFile && virStorageSourceIsLocalStorage(chain)) { + if (prev && virStorageSourceIsLocalStorage(prev)) + parentDir = mdir_name(prev->path); + else + ignore_value(VIR_STRDUP(parentDir, "."));
This reports OOM...
+ + if (!parentDir) { + virReportOOMError();
...and this does it again. If you are going to ignore_value, you probably want VIR_STRDUP_QUIET on the earlier instance. Otherwise, the pre-patch code had better control over when to report OOM without doing it twice.
+ goto error; }
+ int result = virFileRelLinkPointsTo(parentDir, name,
Spurious newline addition? ACK with OOM reporting cleaned up. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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 ae3cb91..1bd82f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15477,7 +15477,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 */ @@ -15585,10 +15585,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, @@ -15609,10 +15608,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 066973f..ae91063 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1343,17 +1343,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) { @@ -1361,7 +1360,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, chain = chain->backingStore; i++; } - *parent = startFrom->path; + *parent = startFrom; } while (chain) { @@ -1378,8 +1377,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, ".")); @@ -1388,7 +1387,6 @@ virStorageFileChainLookup(virStorageSourcePtr chain, goto error; } - int result = virFileRelLinkPointsTo(parentDir, name, chain->path); @@ -1401,8 +1399,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 7fd0bd7..6933f80 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -312,7 +312,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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(-)
@@ -15585,10 +15585,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))
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 &&...)'.
@@ -1388,7 +1387,6 @@ virStorageFileChainLookup(virStorageSourcePtr chain, goto error; }
- int result = virFileRelLinkPointsTo(parentDir, name, chain->path);
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
On 07/09/14 01:28, Eric Blake wrote:
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(-)
@@ -15585,10 +15585,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))
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 &&...)'.
I went with the active_commit tolerant approach as the code was actually there before this patch. Peter
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
When pivotting to a new disk source after a block commit (and possibly
s/pivotting/pivoting/ here and in subject
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(-)
@@ -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. */
We're closer to nuking that fixme comment. :)
@@ -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;
It took me a couple reads to track this, but I'm fairly sure that you got the aliasing correct (no double frees, no leaked memory, and the correct end results on both success and failure). ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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
s/to remove/removing the/
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(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 07/09/14 03:59, Eric Blake wrote:
On 07/04/2014 05:29 AM, Peter Krempa wrote:
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
s/to remove/removing the/
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(-)
ACK
Thanks for the reviews. I've fixed all the points in the individual reviews and pushed this series until here. I'll need to repost the two remaining patches as they'll require some changes. Peter
Move the last operation done on local files to the storage driver API. --- src/qemu/qemu_driver.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6623638..122cb6e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12807,7 +12807,6 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, char *source = NULL; const char *formatStr = NULL; int ret = -1; - int fd = -1; bool need_unlink = false; if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { @@ -12825,7 +12824,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) @@ -12841,15 +12840,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
participants (2)
-
Eric Blake -
Peter Krempa