[libvirt] [PATCHv3 00/26] Work In Progress: Refactor handling of disk image metadata

In my quest to fix various issues with image chain handling I've set out to fix labelling of the image files. While I'm not far enough yet, the series has grown rather large and contains a few fixes of other stuff. I'm posting it incomplete to star review on the trivial parts. Peter Krempa (26): utils: s/virStorageSourceClearBackingStore/virStorageSourceBackingStoreClear security: Rename virSecurityManagerRestoreImageLabel to *Disk* security: manager: Avoid forward decl of virSecurityManagerDispose security: manager: Unify function header format security: manager: Document behavior of disk label manipulation funcs security: Fix header formatting of a few functions security: nop: Avoid very long lines storage: Move readonly and shared flags to disk source from disk def util: storagesource: Add helper to copy and free storage source seclabels util: storagefile: Add deep copy for struct virStorageSource 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: Add helper to determine whether storage is local 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: blockcopy: Don't remove existing disk mirror info src/conf/domain_conf.c | 18 +- src/conf/domain_conf.h | 2 - src/libvirt_private.syms | 10 +- 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 | 110 +++++++------ src/qemu/qemu_cgroup.h | 3 + src/qemu/qemu_command.c | 14 +- src/qemu/qemu_conf.c | 4 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 65 +++----- src/qemu/qemu_hotplug.c | 24 +-- src/qemu/qemu_migration.c | 16 +- src/security/security_apparmor.c | 52 ++++-- src/security/security_dac.c | 115 ++++++------- src/security/security_driver.h | 22 ++- src/security/security_manager.c | 347 +++++++++++++++++++++++++++------------ src/security/security_manager.h | 22 ++- src/security/security_nop.c | 166 ++++++++++++------- src/security/security_selinux.c | 154 +++++++++-------- src/security/security_stack.c | 50 +++++- src/security/virt-aa-helper.c | 2 +- src/util/vircgroup.c | 62 +++++-- src/util/vircgroup.h | 2 + src/util/virstoragefile.c | 239 +++++++++++++++++++++++---- src/util/virstoragefile.h | 17 +- src/vbox/vbox_tmpl.c | 30 ++-- src/xenxs/xen_sxpr.c | 10 +- src/xenxs/xen_xm.c | 10 +- tests/virstoragetest.c | 86 +++++----- 34 files changed, 1140 insertions(+), 595 deletions(-) -- 1.9.3

Rename them to comply with the naming policy. --- src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/util/virstoragefile.c | 6 +++--- src/util/virstoragefile.h | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a793b4c..b3f73f9 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1904,8 +1904,8 @@ virStorageNetHostTransportTypeFromString; virStorageNetHostTransportTypeToString; virStorageNetProtocolTypeToString; virStorageSourceAuthClear; +virStorageSourceBackingStoreClear; virStorageSourceClear; -virStorageSourceClearBackingStore; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2e55c99..f3f42be 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2437,7 +2437,7 @@ qemuDomainDetermineDiskChain(virQEMUDriverPtr driver, if (disk->src->backingStore) { if (force) - virStorageSourceClearBackingStore(disk->src); + virStorageSourceBackingStoreClear(disk->src); else goto cleanup; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 22699c1..41d7bb6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12868,7 +12868,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, * 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. */ - virStorageSourceClearBackingStore(disk->src); + virStorageSourceBackingStoreClear(disk->src); if (virStorageFileInit(snap->src) < 0) goto cleanup; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 6a57327..0c50de1 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1551,14 +1551,14 @@ virStorageSourceGetActualType(virStorageSourcePtr def) /** - * virStorageSourceClearBackingStore: + * virStorageSourceBackingStoreClear: * * @src: disk source to clear * * Clears information about backing store of the current storage file. */ void -virStorageSourceClearBackingStore(virStorageSourcePtr def) +virStorageSourceBackingStoreClear(virStorageSourcePtr def) { if (!def) return; @@ -1599,7 +1599,7 @@ virStorageSourceClear(virStorageSourcePtr def) virStorageNetHostDefFree(def->nhosts, def->hosts); virStorageSourceAuthClear(def); - virStorageSourceClearBackingStore(def); + virStorageSourceBackingStoreClear(def); } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index f98a763..48c7e02 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -321,7 +321,7 @@ void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(virStorageSourcePtr def); void virStorageSourceFree(virStorageSourcePtr def); -void virStorageSourceClearBackingStore(virStorageSourcePtr def); +void virStorageSourceBackingStoreClear(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); typedef int -- 1.9.3

On 06/25/2014 10:54 AM, Peter Krempa wrote:
Rename them to comply with the naming policy. --- src/libvirt_private.syms | 2 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 2 +- src/util/virstoragefile.c | 6 +++--- src/util/virstoragefile.h | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

I'm going to add functions that will deal with individual image files rather than whole disks. Rename the security function to make room for the new one. --- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_hotplug.c | 24 ++++++++++++------------ src/security/security_apparmor.c | 8 ++++---- src/security/security_dac.c | 8 ++++---- src/security/security_driver.h | 8 ++++---- src/security/security_manager.c | 10 +++++----- src/security/security_manager.h | 6 +++--- src/security/security_nop.c | 8 ++++---- src/security/security_selinux.c | 8 ++++---- src/security/security_stack.c | 10 +++++----- 11 files changed, 48 insertions(+), 48 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b3f73f9..1e1dd84 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -911,8 +911,8 @@ virSecurityManagerPreFork; virSecurityManagerReleaseLabel; virSecurityManagerReserveLabel; virSecurityManagerRestoreAllLabel; +virSecurityManagerRestoreDiskLabel; virSecurityManagerRestoreHostdevLabel; -virSecurityManagerRestoreImageLabel; virSecurityManagerRestoreSavedStateLabel; virSecurityManagerSetAllLabel; virSecurityManagerSetChildProcessLabel; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 41d7bb6..ce57542 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12087,8 +12087,8 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, disk->readonly = mode == VIR_DISK_CHAIN_READ_ONLY; if (mode == VIR_DISK_CHAIN_NO_ACCESS) { - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, disk) < 0) + 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", diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 9cd6a3e..5e8aa4e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -161,8 +161,8 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, if (ret < 0) goto error; - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, origdisk) < 0) + if (virSecurityManagerRestoreDiskLabel(driver->securityManager, + vm->def, origdisk) < 0) VIR_WARN("Unable to restore security label on ejected image %s", virDomainDiskGetSource(origdisk)); @@ -182,8 +182,8 @@ int qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, return ret; error: - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, disk) < 0) + if (virSecurityManagerRestoreDiskLabel(driver->securityManager, + vm->def, disk) < 0) VIR_WARN("Unable to restore security label on new media %s", src); if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) @@ -347,8 +347,8 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (releaseaddr) qemuDomainReleaseDeviceAddress(vm, &disk->info, src); - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, disk) < 0) + if (virSecurityManagerRestoreDiskLabel(driver->securityManager, + vm->def, disk) < 0) VIR_WARN("Unable to restore security label on %s", src); if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) @@ -597,8 +597,8 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, return ret; error: - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, disk) < 0) + if (virSecurityManagerRestoreDiskLabel(driver->securityManager, + vm->def, disk) < 0) VIR_WARN("Unable to restore security label on %s", src); if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) @@ -691,8 +691,8 @@ qemuDomainAttachUSBMassstorageDevice(virConnectPtr conn, return ret; error: - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, disk) < 0) + if (virSecurityManagerRestoreDiskLabel(driver->securityManager, + vm->def, disk) < 0) VIR_WARN("Unable to restore security label on %s", src); if (virDomainLockDiskDetach(driver->lockManager, vm, disk) < 0) @@ -2504,8 +2504,8 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, qemuDomainReleaseDeviceAddress(vm, &disk->info, src); - if (virSecurityManagerRestoreImageLabel(driver->securityManager, - vm->def, disk) < 0) + if (virSecurityManagerRestoreDiskLabel(driver->securityManager, + vm->def, disk) < 0) VIR_WARN("Unable to restore security label on %s", src); if (qemuTeardownDiskCgroup(vm, disk) < 0) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index c27ab47..b4cbc61 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -684,9 +684,9 @@ AppArmorClearSecuritySocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, /* Called when hotplugging */ static int -AppArmorRestoreSecurityImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) +AppArmorRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk) { if (virDomainDiskGetType(disk) == VIR_STORAGE_TYPE_NETWORK) return 0; @@ -973,7 +973,7 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSecurityVerify = AppArmorSecurityVerify, .domainSetSecurityDiskLabel = AppArmorSetSecurityDiskLabel, - .domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel, + .domainRestoreSecurityDiskLabel = AppArmorRestoreSecurityDiskLabel, .domainSetSecurityDaemonSocketLabel = AppArmorSetSecurityDaemonSocketLabel, .domainSetSecuritySocketLabel = AppArmorSetSecuritySocketLabel, diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 9760e6f..639f9b0 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -410,9 +410,9 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, static int -virSecurityDACRestoreSecurityImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) +virSecurityDACRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk) { return virSecurityDACRestoreSecurityImageLabelInt(mgr, def, disk, false); } @@ -1274,7 +1274,7 @@ virSecurityDriver virSecurityDriverDAC = { .domainSecurityVerify = virSecurityDACVerify, .domainSetSecurityDiskLabel = virSecurityDACSetSecurityDiskLabel, - .domainRestoreSecurityImageLabel = virSecurityDACRestoreSecurityImageLabel, + .domainRestoreSecurityDiskLabel = virSecurityDACRestoreSecurityDiskLabel, .domainSetSecurityDaemonSocketLabel = virSecurityDACSetDaemonSocketLabel, .domainSetSecuritySocketLabel = virSecurityDACSetSocketLabel, diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 6a17a8e..05d612a 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -51,9 +51,9 @@ typedef const char *(*virSecurityDriverGetBaseLabel) (virSecurityManagerPtr mgr, typedef int (*virSecurityDriverPreFork) (virSecurityManagerPtr mgr); -typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk); +typedef int (*virSecurityDomainRestoreDiskLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk); typedef int (*virSecurityDomainSetDaemonSocketLabel)(virSecurityManagerPtr mgr, virDomainDefPtr vm); typedef int (*virSecurityDomainSetSocketLabel) (virSecurityManagerPtr mgr, @@ -128,7 +128,7 @@ struct _virSecurityDriver { virSecurityDomainSecurityVerify domainSecurityVerify; virSecurityDomainSetDiskLabel domainSetSecurityDiskLabel; - virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; + virSecurityDomainRestoreDiskLabel domainRestoreSecurityDiskLabel; virSecurityDomainSetDaemonSocketLabel domainSetSecurityDaemonSocketLabel; virSecurityDomainSetSocketLabel domainSetSecuritySocketLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f0e3ee1..d57bab9 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -306,14 +306,14 @@ bool virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr) return mgr->requireConfined; } -int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainDiskDefPtr disk) +int virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainDiskDefPtr disk) { - if (mgr->drv->domainRestoreSecurityImageLabel) { + if (mgr->drv->domainRestoreSecurityDiskLabel) { int ret; virObjectLock(mgr); - ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, disk); + ret = mgr->drv->domainRestoreSecurityDiskLabel(mgr, vm, disk); virObjectUnlock(mgr); return ret; } diff --git a/src/security/security_manager.h b/src/security/security_manager.h index f083b3a..307e1c2 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -61,9 +61,9 @@ bool virSecurityManagerGetAllowDiskFormatProbing(virSecurityManagerPtr mgr); bool virSecurityManagerGetDefaultConfined(virSecurityManagerPtr mgr); bool virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr); -int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk); +int virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk); int virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm); int virSecurityManagerSetSocketLabel(virSecurityManagerPtr mgr, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index 7feeda6..ac9ceae 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -50,9 +50,9 @@ static const char * virSecurityDriverGetDOINop(virSecurityManagerPtr mgr ATTRIBU return "0"; } -static int virSecurityDomainRestoreImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) +static int virSecurityDomainRestoreDiskLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) { return 0; } @@ -207,7 +207,7 @@ virSecurityDriver virSecurityDriverNop = { .domainSecurityVerify = virSecurityDomainVerifyNop, .domainSetSecurityDiskLabel = virSecurityDomainSetDiskLabelNop, - .domainRestoreSecurityImageLabel = virSecurityDomainRestoreImageLabelNop, + .domainRestoreSecurityDiskLabel = virSecurityDomainRestoreDiskLabelNop, .domainSetSecurityDaemonSocketLabel = virSecurityDomainSetDaemonSocketLabelNop, .domainSetSecuritySocketLabel = virSecurityDomainSetSocketLabelNop, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index a4c13a1..572f8a1 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1182,9 +1182,9 @@ virSecuritySELinuxRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, static int -virSecuritySELinuxRestoreSecurityImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr def, - virDomainDiskDefPtr disk) +virSecuritySELinuxRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virDomainDiskDefPtr disk) { return virSecuritySELinuxRestoreSecurityImageLabelInt(mgr, def, disk, false); } @@ -2427,7 +2427,7 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSecurityVerify = virSecuritySELinuxSecurityVerify, .domainSetSecurityDiskLabel = virSecuritySELinuxSetSecurityDiskLabel, - .domainRestoreSecurityImageLabel = virSecuritySELinuxRestoreSecurityImageLabel, + .domainRestoreSecurityDiskLabel = virSecuritySELinuxRestoreSecurityDiskLabel, .domainSetSecurityDaemonSocketLabel = virSecuritySELinuxSetSecurityDaemonSocketLabel, .domainSetSecuritySocketLabel = virSecuritySELinuxSetSecuritySocketLabel, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 63b2720..7f210b2 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -240,16 +240,16 @@ virSecurityStackSetSecurityDiskLabel(virSecurityManagerPtr mgr, static int -virSecurityStackRestoreSecurityImageLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainDiskDefPtr disk) +virSecurityStackRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainDiskDefPtr disk) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; int rc = 0; for (; item; item = item->next) { - if (virSecurityManagerRestoreImageLabel(item->securityManager, vm, disk) < 0) + if (virSecurityManagerRestoreDiskLabel(item->securityManager, vm, disk) < 0) rc = -1; } @@ -579,7 +579,7 @@ virSecurityDriver virSecurityDriverStack = { .domainSecurityVerify = virSecurityStackVerify, .domainSetSecurityDiskLabel = virSecurityStackSetSecurityDiskLabel, - .domainRestoreSecurityImageLabel = virSecurityStackRestoreSecurityImageLabel, + .domainRestoreSecurityDiskLabel = virSecurityStackRestoreSecurityDiskLabel, .domainSetSecurityDaemonSocketLabel = virSecurityStackSetDaemonSocketLabel, .domainSetSecuritySocketLabel = virSecurityStackSetSocketLabel, -- 1.9.3

On 06/25/2014 10:54 AM, Peter Krempa wrote:
I'm going to add functions that will deal with individual image files rather than whole disks. Rename the security function to make room for the new one. --- src/libvirt_private.syms | 2 +- src/qemu/qemu_driver.c | 4 ++-- src/qemu/qemu_hotplug.c | 24 ++++++++++++------------ src/security/security_apparmor.c | 8 ++++---- src/security/security_dac.c | 8 ++++---- src/security/security_driver.h | 8 ++++---- src/security/security_manager.c | 10 +++++----- src/security/security_manager.h | 6 +++--- src/security/security_nop.c | 8 ++++---- src/security/security_selinux.c | 8 ++++---- src/security/security_stack.c | 10 +++++----- 11 files changed, 48 insertions(+), 48 deletions(-)
ACK. Mechanical. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

--- src/security/security_manager.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index d57bab9..84d90ac 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -48,7 +48,17 @@ struct _virSecurityManager { static virClassPtr virSecurityManagerClass; -static void virSecurityManagerDispose(void *obj); + +static +void virSecurityManagerDispose(void *obj) +{ + virSecurityManagerPtr mgr = obj; + + if (mgr->drv->close) + mgr->drv->close(mgr); + VIR_FREE(mgr->privateData); +} + static int virSecurityManagerOnceInit(void) { @@ -231,15 +241,6 @@ void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr) } -static void virSecurityManagerDispose(void *obj) -{ - virSecurityManagerPtr mgr = obj; - - if (mgr->drv->close) - mgr->drv->close(mgr); - VIR_FREE(mgr->privateData); -} - const char * virSecurityManagerGetDriver(virSecurityManagerPtr mgr) { -- 1.9.3

On 06/25/2014 10:54 AM, Peter Krempa wrote:
--- src/security/security_manager.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

--- src/security/security_manager.c | 248 +++++++++++++++++++++++++--------------- 1 file changed, 157 insertions(+), 91 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 84d90ac..bb12e8e 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -19,10 +19,8 @@ * * Author: Daniel P. Berrange <berrange@redhat.com> */ - #include <config.h> - #include "security_driver.h" #include "security_stack.h" #include "security_dac.h" @@ -60,7 +58,8 @@ void virSecurityManagerDispose(void *obj) } -static int virSecurityManagerOnceInit(void) +static int +virSecurityManagerOnceInit(void) { if (!(virSecurityManagerClass = virClassNew(virClassForObjectLockable(), "virSecurityManagerClass", @@ -73,11 +72,13 @@ static int virSecurityManagerOnceInit(void) VIR_ONCE_GLOBAL_INIT(virSecurityManager); -static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr drv, - const char *virtDriver, - bool allowDiskFormatProbing, - bool defaultConfined, - bool requireConfined) + +static virSecurityManagerPtr +virSecurityManagerNewDriver(virSecurityDriverPtr drv, + const char *virtDriver, + bool allowDiskFormatProbing, + bool defaultConfined, + bool requireConfined) { virSecurityManagerPtr mgr; char *privateData; @@ -114,7 +115,9 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr return mgr; } -virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary) + +virSecurityManagerPtr +virSecurityManagerNewStack(virSecurityManagerPtr primary) { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverStack, @@ -131,21 +134,25 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary) return mgr; } -int virSecurityManagerStackAddNested(virSecurityManagerPtr stack, - virSecurityManagerPtr nested) + +int +virSecurityManagerStackAddNested(virSecurityManagerPtr stack, + virSecurityManagerPtr nested) { if (!STREQ("stack", stack->drv->name)) return -1; return virSecurityStackAddNested(stack, nested); } -virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, - uid_t user, - gid_t group, - bool allowDiskFormatProbing, - bool defaultConfined, - bool requireConfined, - bool dynamicOwnership) + +virSecurityManagerPtr +virSecurityManagerNewDAC(const char *virtDriver, + uid_t user, + gid_t group, + bool allowDiskFormatProbing, + bool defaultConfined, + bool requireConfined, + bool dynamicOwnership) { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, @@ -161,16 +168,19 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, virSecurityManagerDispose(mgr); return NULL; } + virSecurityDACSetDynamicOwnership(mgr, dynamicOwnership); return mgr; } -virSecurityManagerPtr virSecurityManagerNew(const char *name, - const char *virtDriver, - bool allowDiskFormatProbing, - bool defaultConfined, - bool requireConfined) + +virSecurityManagerPtr +virSecurityManagerNew(const char *name, + const char *virtDriver, + bool allowDiskFormatProbing, + bool defaultConfined, + bool requireConfined) { virSecurityDriverPtr drv = virSecurityDriverLookup(name, virtDriver); if (!drv) @@ -211,7 +221,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, * followed by a call to virSecurityManagerPostFork() in both * parent and child. */ -int virSecurityManagerPreFork(virSecurityManagerPtr mgr) +int +virSecurityManagerPreFork(virSecurityManagerPtr mgr) { int ret = 0; @@ -230,12 +241,14 @@ int virSecurityManagerPreFork(virSecurityManagerPtr mgr) * Must be called after fork()'ing in both parent and child * to ensure mutex state is sane for the child to use */ -void virSecurityManagerPostFork(virSecurityManagerPtr mgr) +void +virSecurityManagerPostFork(virSecurityManagerPtr mgr) { virObjectUnlock(mgr); } -void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr) +void * +virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr) { return mgr->privateData; } @@ -247,6 +260,7 @@ virSecurityManagerGetDriver(virSecurityManagerPtr mgr) return mgr->virtDriver; } + const char * virSecurityManagerGetDOI(virSecurityManagerPtr mgr) { @@ -262,6 +276,7 @@ virSecurityManagerGetDOI(virSecurityManagerPtr mgr) return NULL; } + const char * virSecurityManagerGetModel(virSecurityManagerPtr mgr) { @@ -277,9 +292,11 @@ virSecurityManagerGetModel(virSecurityManagerPtr mgr) return NULL; } + /* return NULL if a base label is not present */ const char * -virSecurityManagerGetBaseLabel(virSecurityManagerPtr mgr, int virtType) +virSecurityManagerGetBaseLabel(virSecurityManagerPtr mgr, + int virtType) { if (mgr->drv->getBaseLabel) { const char *ret; @@ -292,24 +309,32 @@ virSecurityManagerGetBaseLabel(virSecurityManagerPtr mgr, int virtType) return NULL; } -bool virSecurityManagerGetAllowDiskFormatProbing(virSecurityManagerPtr mgr) + +bool +virSecurityManagerGetAllowDiskFormatProbing(virSecurityManagerPtr mgr) { return mgr->allowDiskFormatProbing; } -bool virSecurityManagerGetDefaultConfined(virSecurityManagerPtr mgr) + +bool +virSecurityManagerGetDefaultConfined(virSecurityManagerPtr mgr) { return mgr->defaultConfined; } -bool virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr) + +bool +virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr) { return mgr->requireConfined; } -int virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainDiskDefPtr disk) + +int +virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainDiskDefPtr disk) { if (mgr->drv->domainRestoreSecurityDiskLabel) { int ret; @@ -323,8 +348,10 @@ int virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm) + +int +virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm) { if (mgr->drv->domainSetSecurityDaemonSocketLabel) { int ret; @@ -338,8 +365,10 @@ int virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetSocketLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm) + +int +virSecurityManagerSetSocketLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm) { if (mgr->drv->domainSetSecuritySocketLabel) { int ret; @@ -353,8 +382,10 @@ int virSecurityManagerSetSocketLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm) + +int +virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm) { if (mgr->drv->domainClearSecuritySocketLabel) { int ret; @@ -368,9 +399,11 @@ int virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainDiskDefPtr disk) + +int +virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainDiskDefPtr disk) { if (mgr->drv->domainSetSecurityDiskLabel) { int ret; @@ -384,10 +417,12 @@ int virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerRestoreHostdevLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainHostdevDefPtr dev, - const char *vroot) + +int +virSecurityManagerRestoreHostdevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainHostdevDefPtr dev, + const char *vroot) { if (mgr->drv->domainRestoreSecurityHostdevLabel) { int ret; @@ -401,10 +436,12 @@ int virSecurityManagerRestoreHostdevLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetHostdevLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virDomainHostdevDefPtr dev, - const char *vroot) + +int +virSecurityManagerSetHostdevLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virDomainHostdevDefPtr dev, + const char *vroot) { if (mgr->drv->domainSetSecurityHostdevLabel) { int ret; @@ -418,9 +455,11 @@ int virSecurityManagerSetHostdevLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *savefile) + +int +virSecurityManagerSetSavedStateLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + const char *savefile) { if (mgr->drv->domainSetSavedStateLabel) { int ret; @@ -434,9 +473,10 @@ int virSecurityManagerSetSavedStateLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *savefile) +int +virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + const char *savefile) { if (mgr->drv->domainRestoreSavedStateLabel) { int ret; @@ -450,8 +490,10 @@ int virSecurityManagerRestoreSavedStateLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm) + +int +virSecurityManagerGenLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm) { int ret = -1; size_t i, j; @@ -545,9 +587,11 @@ int virSecurityManagerGenLabel(virSecurityManagerPtr mgr, return ret; } -int virSecurityManagerReserveLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - pid_t pid) + +int +virSecurityManagerReserveLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + pid_t pid) { if (mgr->drv->domainReserveSecurityLabel) { int ret; @@ -561,8 +605,10 @@ int virSecurityManagerReserveLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerReleaseLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm) + +int +virSecurityManagerReleaseLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm) { if (mgr->drv->domainReleaseSecurityLabel) { int ret; @@ -576,9 +622,11 @@ int virSecurityManagerReleaseLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *stdin_path) + +int +virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + const char *stdin_path) { if (mgr->drv->domainSetSecurityAllLabel) { int ret; @@ -592,9 +640,11 @@ int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - bool migrated) + +int +virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + bool migrated) { if (mgr->drv->domainRestoreSecurityAllLabel) { int ret; @@ -608,10 +658,11 @@ int virSecurityManagerRestoreAllLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - pid_t pid, - virSecurityLabelPtr sec) +int +virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + pid_t pid, + virSecurityLabelPtr sec) { if (mgr->drv->domainGetSecurityProcessLabel) { int ret; @@ -625,8 +676,10 @@ int virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetProcessLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm) + +int +virSecurityManagerSetProcessLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm) { if (mgr->drv->domainSetSecurityProcessLabel) { int ret; @@ -640,9 +693,11 @@ int virSecurityManagerSetProcessLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetChildProcessLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - virCommandPtr cmd) + +int +virSecurityManagerSetChildProcessLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virCommandPtr cmd) { if (mgr->drv->domainSetSecurityChildProcessLabel) return mgr->drv->domainSetSecurityChildProcessLabel(mgr, vm, cmd); @@ -651,8 +706,10 @@ int virSecurityManagerSetChildProcessLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerVerify(virSecurityManagerPtr mgr, - virDomainDefPtr def) + +int +virSecurityManagerVerify(virSecurityManagerPtr mgr, + virDomainDefPtr def) { virSecurityLabelDefPtr secdef; @@ -679,9 +736,11 @@ int virSecurityManagerVerify(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - int fd) + +int +virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + int fd) { if (mgr->drv->domainSetSecurityImageFDLabel) { int ret; @@ -695,9 +754,11 @@ int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, return -1; } -int virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - int fd) + +int +virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + int fd) { if (mgr->drv->domainSetSecurityTapFDLabel) { int ret; @@ -711,8 +772,10 @@ int virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr, return -1; } -char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr, - virDomainDefPtr vm) + +char * +virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr, + virDomainDefPtr vm) { if (mgr->drv->domainGetSecurityMountOptions) { char *ret; @@ -726,6 +789,7 @@ char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr, return NULL; } + virSecurityManagerPtr* virSecurityManagerGetNested(virSecurityManagerPtr mgr) { @@ -743,9 +807,11 @@ virSecurityManagerGetNested(virSecurityManagerPtr mgr) return list; } -int virSecurityManagerSetHugepages(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *path) + +int +virSecurityManagerSetHugepages(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + const char *path) { if (mgr->drv->domainSetSecurityHugepages) { int ret; -- 1.9.3

On 06/25/2014 10:54 AM, Peter Krempa wrote:
--- src/security/security_manager.c | 248 +++++++++++++++++++++++++--------------- 1 file changed, 157 insertions(+), 91 deletions(-)
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

virSecurityManagerSetDiskLabel and virSecurityManagerRestoreDiskLabel don't have complementary semantics. Document the semantics to avoid possible problems. --- src/security/security_manager.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index bb12e8e..06e5123 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -331,6 +331,17 @@ virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr) } +/** + * virSecurityManagerRestoreDiskLabel: + * @mgr: security manager object + * @vm: domain definition object + * @disk: disk definition to operate on + * + * Removes security label from the source image of the disk. Note that this + * function doesn't restore labels on backing chain elements of @disk. + * + * Returns: 0 on success, -1 on error. + */ int virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, @@ -400,6 +411,17 @@ virSecurityManagerClearSocketLabel(virSecurityManagerPtr mgr, } +/** + * virSecurityManagerSetDiskLabel: + * @mgr: security manager object + * @vm: domain definition object + * @disk: disk definition to operate on + * + * Labels the disk image and all images in the backing chain with the configured + * security label. + * + * Returns: 0 on success, -1 on error. + */ int virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, -- 1.9.3

On 06/25/2014 10:54 AM, Peter Krempa wrote:
virSecurityManagerSetDiskLabel and virSecurityManagerRestoreDiskLabel don't have complementary semantics. Document the semantics to avoid possible problems. --- src/security/security_manager.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index bb12e8e..06e5123 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -331,6 +331,17 @@ virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr) }
+/** + * virSecurityManagerRestoreDiskLabel: + * @mgr: security manager object + * @vm: domain definition object + * @disk: disk definition to operate on + * + * Removes security label from the source image of the disk. Note that this + * function doesn't restore labels on backing chain elements of @disk.
which probably ought to be considered a bug, and something that we might change in the future - but accurate documentation of what it does now. Restoring labels on backing chains is tricky - we need to start keeping a reference count of all places that are using a backing file (as it can be in use by more than one chain, even by more than one domain), and really the label restore ought to be part of releasing the last use of a storage volume after all domains are done sharing the same backing file. The disk lease manager may be helpful, as backing files are shared (readonly) leases. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Some of the functions in the storage driver had their headers formatted incorrectly. --- src/security/security_driver.h | 6 +++--- src/security/security_manager.h | 9 ++++----- src/security/security_stack.c | 4 ++-- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 05d612a..062dc8f 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -108,10 +108,10 @@ typedef int (*virSecurityDomainSetTapFDLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def, int fd); typedef char *(*virSecurityDomainGetMountOptions) (virSecurityManagerPtr mgr, - virDomainDefPtr def); + virDomainDefPtr def); typedef int (*virSecurityDomainSetHugepages) (virSecurityManagerPtr mgr, - virDomainDefPtr def, - const char *path); + virDomainDefPtr def, + const char *path); struct _virSecurityDriver { size_t privateDataLen; diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 307e1c2..8a5fcfb 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -118,11 +118,10 @@ int virSecurityManagerSetTapFDLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, int fd); char *virSecurityManagerGetMountOptions(virSecurityManagerPtr mgr, - virDomainDefPtr vm); -virSecurityManagerPtr* -virSecurityManagerGetNested(virSecurityManagerPtr mgr); + virDomainDefPtr vm); +virSecurityManagerPtr* virSecurityManagerGetNested(virSecurityManagerPtr mgr); int virSecurityManagerSetHugepages(virSecurityManagerPtr mgr, - virDomainDefPtr sec, - const char *hugepages_path); + virDomainDefPtr sec, + const char *hugepages_path); #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 7f210b2..e3e9c85 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -513,8 +513,8 @@ virSecurityStackSetTapFDLabel(virSecurityManagerPtr mgr, static int virSecurityStackSetHugepages(virSecurityManagerPtr mgr, - virDomainDefPtr vm, - const char *path) + virDomainDefPtr vm, + const char *path) { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; -- 1.9.3

On 06/25/2014 10:54 AM, Peter Krempa wrote:
Some of the functions in the storage driver had their headers formatted incorrectly. --- src/security/security_driver.h | 6 +++--- src/security/security_manager.h | 9 ++++----- src/security/security_stack.c | 4 ++-- 3 files changed, 9 insertions(+), 10 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

The function headers contain type on the same line as the name. When combined with usage of ATTRIBUTE_UNUSED, the function headers were very long. Shorten them by breaking the line after the type. --- src/security/security_nop.c | 147 ++++++++++++++++++++++++++------------------ 1 file changed, 87 insertions(+), 60 deletions(-) diff --git a/src/security/security_nop.c b/src/security/security_nop.c index ac9ceae..b57bf05 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -25,160 +25,187 @@ #define VIR_FROM_THIS VIR_FROM_SECURITY -static virSecurityDriverStatus virSecurityDriverProbeNop(const char *virtDriver ATTRIBUTE_UNUSED) +static virSecurityDriverStatus +virSecurityDriverProbeNop(const char *virtDriver ATTRIBUTE_UNUSED) { return SECURITY_DRIVER_ENABLE; } -static int virSecurityDriverOpenNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +static int +virSecurityDriverOpenNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDriverCloseNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +static int +virSecurityDriverCloseNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) { return 0; } -static const char * virSecurityDriverGetModelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +static const char * +virSecurityDriverGetModelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) { return "none"; } -static const char * virSecurityDriverGetDOINop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) +static const char * +virSecurityDriverGetDOINop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED) { return "0"; } -static int virSecurityDomainRestoreDiskLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) +static int +virSecurityDomainRestoreDiskLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainSetDaemonSocketLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED) +static int +virSecurityDomainSetDaemonSocketLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainSetSocketLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED) +static int +virSecurityDomainSetSocketLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainClearSocketLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED) +static int +virSecurityDomainClearSocketLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainSetDiskLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) +static int +virSecurityDomainSetDiskLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + virDomainDiskDefPtr disk ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainRestoreHostdevLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED, - const char *vroot ATTRIBUTE_UNUSED) +static int +virSecurityDomainRestoreHostdevLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED, + const char *vroot ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainSetHostdevLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED, - const char *vroot ATTRIBUTE_UNUSED) +static int +virSecurityDomainSetHostdevLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + virDomainHostdevDefPtr dev ATTRIBUTE_UNUSED, + const char *vroot ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainSetSavedStateLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - const char *savefile ATTRIBUTE_UNUSED) +static int +virSecurityDomainSetSavedStateLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + const char *savefile ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainRestoreSavedStateLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - const char *savefile ATTRIBUTE_UNUSED) + +static int +virSecurityDomainRestoreSavedStateLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + const char *savefile ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainGenLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr sec ATTRIBUTE_UNUSED) +static int +virSecurityDomainGenLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr sec ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainReserveLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr sec ATTRIBUTE_UNUSED, - pid_t pid ATTRIBUTE_UNUSED) +static int +virSecurityDomainReserveLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr sec ATTRIBUTE_UNUSED, + pid_t pid ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainReleaseLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr sec ATTRIBUTE_UNUSED) +static int +virSecurityDomainReleaseLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr sec ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr sec ATTRIBUTE_UNUSED, - const char *stdin_path ATTRIBUTE_UNUSED) +static int +virSecurityDomainSetAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr sec ATTRIBUTE_UNUSED, + const char *stdin_path ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainRestoreAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - bool migrated ATTRIBUTE_UNUSED) +static int +virSecurityDomainRestoreAllLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + bool migrated ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainGetProcessLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - pid_t pid ATTRIBUTE_UNUSED, - virSecurityLabelPtr sec ATTRIBUTE_UNUSED) + +static int +virSecurityDomainGetProcessLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + pid_t pid ATTRIBUTE_UNUSED, + virSecurityLabelPtr sec ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainSetProcessLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED) +static int +virSecurityDomainSetProcessLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainSetChildProcessLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED, - virCommandPtr cmd ATTRIBUTE_UNUSED) +static int +virSecurityDomainSetChildProcessLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + virCommandPtr cmd ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainVerifyNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr def ATTRIBUTE_UNUSED) +static int +virSecurityDomainVerifyNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED) { return 0; } -static int virSecurityDomainSetFDLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr sec ATTRIBUTE_UNUSED, - int fd ATTRIBUTE_UNUSED) +static int +virSecurityDomainSetFDLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr sec ATTRIBUTE_UNUSED, + int fd ATTRIBUTE_UNUSED) { return 0; } -static char *virSecurityDomainGetMountOptionsNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, - virDomainDefPtr vm ATTRIBUTE_UNUSED) +static char * +virSecurityDomainGetMountOptionsNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED) { char *opts; -- 1.9.3

On 06/25/2014 10:54 AM, Peter Krempa wrote:
The function headers contain type on the same line as the name. When combined with usage of ATTRIBUTE_UNUSED, the function headers were very long. Shorten them by breaking the line after the type. --- src/security/security_nop.c | 147 ++++++++++++++++++++++++++------------------ 1 file changed, 87 insertions(+), 60 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 02c394f..44903b0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5549,9 +5549,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) && @@ -5678,7 +5678,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) && @@ -5700,7 +5700,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, snapshot); goto error; } - } else if (def->readonly) { + } else if (def->src->readonly) { def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE; } @@ -13390,7 +13390,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; @@ -15114,7 +15115,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"); @@ -15270,9 +15272,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 1122eb2..bd85514 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 4b6b5c0..ea2b21b 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 38acdff..2866869 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 3875bf3..257303d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4068,7 +4068,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 93d303e..58ba214 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3316,7 +3316,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; @@ -3384,7 +3384,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)) virBufferAddLit(&opt, ",readonly=on"); if (disk->transient) { @@ -3443,7 +3443,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"); } @@ -8019,7 +8019,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; @@ -9649,7 +9649,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")) { @@ -9705,7 +9705,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, @@ -10873,7 +10873,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 8a3bdef..c9ca17c 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 ce57542..552e595 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12069,7 +12069,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; @@ -12084,7 +12084,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, @@ -12107,7 +12107,7 @@ qemuDomainPrepareDiskChainElement(virQEMUDriverPtr driver, cleanup: memcpy(disk->src, &origdisk, sizeof(origdisk)); - disk->readonly = origreadonly; + disk->src->readonly = origreadonly; virObjectUnref(cfg); return ret; } @@ -12759,7 +12759,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 7684aec..8a20bf8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1159,7 +1159,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); @@ -1265,7 +1266,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); @@ -1351,7 +1353,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); @@ -1414,7 +1417,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); @@ -1528,8 +1532,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 48c7e02..fe17b0b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -240,6 +240,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 4ba9ad7..91e9482 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 aacf74c..08a10e7 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); @@ -1818,9 +1818,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 2cd6d4c..6349d17 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 06/25/2014 10:54 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. ---
My immediate reaction is that all backing files are generally readonly, so when would we ever label them differently? Then again, we temporarily mark files readwrite during commit. For shared, this move makes total sense. (Shared is a host-only concept - the file is read-only but must not be relabeled by libvirt because it may be shared by other domains). And for how we are using readonly (label the host image differently than if it were read-write), it also seems to make sense. If we implemented reference-counted storage source objects, the difference between shared and readonly is whether a second reference can be obtained on a file already in use. One thing is sitting a little uneasy on my mind - do we have (or need, or want) a way to affect guest ABI by the readonly designation? That is, does it ever make sense to advertise to the guest that a disk is readonly (maybe if presenting the guest a virtual DVD drive, the guest will act differently if it is emulated as a DVD-ROM vs. if it is emulated as a DVD-RW that can be burned)? And if so, I could see a case where we might want an image to be marked readonly to the guest perspective, regardless of whether the host files are labeled for readonly use. But I've spend some time thinking about it, and can't come up with any cases where having a readonly disk (guest point of view) would still require a readwrite image from the host; and that tracking whether the guest disk is readonly by deferring to whether the host source is readonly seems to be reliable. I also don't know if we will ever want to update our <backingChain> live xml to expose whether backing chain elements are temporarily using a read-write label, even though they default to readonly; or even letting the user choose between <readonly/> vs. <shared/> for backing chain elements. This patch opens up some possibilities to think about for future changes. Okay, for all my ramblings above, I still can't articulate a firm reason why this might be a bad idea, so I can live with it going in.
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(-)
+++ b/src/conf/domain_conf.c @@ -5549,9 +5549,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;
Note that transient remains a per-guest disk item, not a per-host image item.
@@ -13390,7 +13390,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"));
You know, I think this ABI check is overly strict - a guest can't tell the difference between whether a host image is <shared/> or <readonly/> (the only difference between those two exclusive flags is whether other domains may use the file at the same time). But if we relax it, it should be a separate patch. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

They will be reused to transfer disk labels from snapshotted disks to the new disk definitions. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 45 ++++++++++++++++++++++++++++++++++++++------- src/util/virstoragefile.h | 3 +++ 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1e1dd84..792754f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1913,6 +1913,7 @@ virStorageSourceNewFromBacking; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; virStorageSourcePoolModeTypeToString; +virStorageSourceSeclabelsCopy; virStorageTypeFromString; virStorageTypeToString; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 0c50de1..c52206c 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1515,6 +1515,31 @@ virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, } +int +virStorageSourceSeclabelsCopy(virStorageSourcePtr to, + const virStorageSource *from) +{ + size_t i; + + virStorageSourceSeclabelsClear(to); + + 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; +} + + void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def) { @@ -1573,10 +1598,21 @@ virStorageSourceBackingStoreClear(virStorageSourcePtr def) void -virStorageSourceClear(virStorageSourcePtr def) +virStorageSourceSeclabelsClear(virStorageSourcePtr def) { size_t i; + if (def->seclabels) { + for (i = 0; i < def->nseclabels; i++) + virSecurityDeviceLabelDefFree(def->seclabels[i]); + VIR_FREE(def->seclabels); + } +} + + +void +virStorageSourceClear(virStorageSourcePtr def) +{ if (!def) return; @@ -1587,12 +1623,7 @@ virStorageSourceClear(virStorageSourcePtr def) virBitmapFree(def->features); VIR_FREE(def->compat); virStorageEncryptionFree(def->encryption); - - if (def->seclabels) { - for (i = 0; i < def->nseclabels; i++) - virSecurityDeviceLabelDefFree(def->seclabels[i]); - VIR_FREE(def->seclabels); - } + virStorageSourceSeclabelsClear(def); virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index fe17b0b..176661e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -322,6 +322,9 @@ void virStorageNetHostDefFree(size_t nhosts, virStorageNetHostDefPtr hosts); virStorageNetHostDefPtr virStorageNetHostDefCopy(size_t nhosts, virStorageNetHostDefPtr hosts); +void virStorageSourceSeclabelsClear(virStorageSourcePtr def); +int virStorageSourceSeclabelsCopy(virStorageSourcePtr to, + const virStorageSource *from); void virStorageSourceAuthClear(virStorageSourcePtr def); void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); -- 1.9.3

On 06/25/2014 10:54 AM, Peter Krempa wrote:
They will be reused to transfer disk labels from snapshotted disks to the new disk definitions. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 45 ++++++++++++++++++++++++++++++++++++++------- src/util/virstoragefile.h | 3 +++ 3 files changed, 42 insertions(+), 7 deletions(-)
+++ b/src/util/virstoragefile.c @@ -1515,6 +1515,31 @@ virStorageSourceGetSecurityLabelDef(virStorageSourcePtr src, }
+int +virStorageSourceSeclabelsCopy(virStorageSourcePtr to, + const virStorageSource *from)
Worth a doc comment, particularly mentioning that this version replaces any existing labels in 'to' (there are other feasible semantics, such as erroring out if there are existing labels in 'to', so adding docs makes it obvious what semantics you chose without making me have to read the code). ACK once you add that. -- 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 | 143 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 3 + 3 files changed, 147 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 792754f..bf3a45b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1906,6 +1906,7 @@ virStorageNetProtocolTypeToString; virStorageSourceAuthClear; virStorageSourceBackingStoreClear; virStorageSourceClear; +virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c52206c..5f8e02d 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1540,6 +1540,149 @@ virStorageSourceSeclabelsCopy(virStorageSourcePtr to, } +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 +virStorageSourceCopy(const virStorageSource *src, + bool backingChain) +{ + virStorageSourcePtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->type = src->type; + ret->protocol = src->protocol; + ret->format = src->format; + ret->allocation = src->allocation; + ret->capacity = src->capacity; + ret->readonly = src->readonly; + ret->shared = src->shared; + + /* storage driver metadata are not copied */ + ret->drv = NULL; + + if (VIR_STRDUP(ret->path, src->path) < 0 || + VIR_STRDUP(ret->volume, src->volume) < 0 || + VIR_STRDUP(ret->driverName, src->driverName) < 0 || + VIR_STRDUP(ret->relPath, src->relPath) < 0 || + VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 || + VIR_STRDUP(ret->compat, src->compat) < 0 || + VIR_STRDUP(ret->auth.username, src->auth.username) < 0) + goto error; + + if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) + goto error; + ret->nhosts = src->nhosts; + + if (!(ret->srcpool = virStorageSourcePoolDefCopy(src->srcpool))) + goto error; + + if (!(ret->features = virBitmapNewCopy(src->features))) + goto error; + + if (!(ret->encryption = virStorageEncryptionCopy(src->encryption))) + goto error; + + if (!(ret->perms = virStoragePermsCopy(src->perms))) + goto error; + + if (!(ret->timestamps = virStorageTimestampsCopy(src->timestamps))) + goto error; + + if (virStorageSourceSeclabelsCopy(ret, src) < 0) + goto error; + + ret->auth.secretType = src->auth.secretType; + switch ((virStorageSecretType) src->auth.secretType) { + case VIR_STORAGE_SECRET_TYPE_NONE: + case VIR_STORAGE_SECRET_TYPE_LAST: + break; + + case VIR_STORAGE_SECRET_TYPE_UUID: + memcpy(ret->auth.secret.uuid, src->auth.secret.uuid, VIR_UUID_BUFLEN); + break; + + case VIR_STORAGE_SECRET_TYPE_USAGE: + if (VIR_STRDUP(ret->auth.secret.usage, src->auth.secret.usage) < 0) + goto error; + break; + } + + if (backingChain && src->backingStore) { + if (!(ret->backingStore = virStorageSourceCopy(src->backingStore, + true))) + goto error; + } + + return ret; + + error: + virStorageSourceFree(ret); + return NULL; +} + + void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 176661e..3e13071 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -332,6 +332,9 @@ int virStorageSourceGetActualType(virStorageSourcePtr def); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceBackingStoreClear(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); +virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, + bool backingChain) + ATTRIBUTE_NONNULL(1); typedef int (*virStorageFileSimplifyPathReadlinkCallback)(const char *path, -- 1.9.3

On 06/25/2014 10:54 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 | 143 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 3 + 3 files changed, 147 insertions(+)
+ + +virStorageSourcePtr +virStorageSourceCopy(const virStorageSource *src, + bool backingChain)
Might be worth a doc comment for this function as well, mentioning that driver metadata is not copied, and backingChain controls whether the copy is shallow or deep with regards to backing files. 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 bf3a45b..99e9d52 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1049,6 +1049,7 @@ virCgroupGetCpuCfsQuota; virCgroupGetCpusetCpus; virCgroupGetCpusetMems; virCgroupGetCpuShares; +virCgroupGetDevicePermsString; virCgroupGetDomainTotalCpuStats; virCgroupGetFreezerState; virCgroupGetMemoryHardLimit; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index c578bd0..2eaf265 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2624,6 +2624,44 @@ virCgroupDenyAllDevices(virCgroupPtr group) /** + * virCgroupGetDevicePermsString: + * + * @perms: Bitwise or of VIR_CGROUP_DEVICE permission bits + * + * Returns string corresponding to the appropriate bits set. + */ +const char * +virCgroupGetDevicePermsString(int perms) +{ + if (perms & VIR_CGROUP_DEVICE_READ) { + if (perms & VIR_CGROUP_DEVICE_WRITE) { + if (perms & VIR_CGROUP_DEVICE_MKNOD) + return "rwm"; + else + return "rw"; + } else { + if (perms & VIR_CGROUP_DEVICE_MKNOD) + return "rm"; + else + return "r"; + } + } else { + if (perms & VIR_CGROUP_DEVICE_WRITE) { + if (perms & VIR_CGROUP_DEVICE_MKNOD) + return "wm"; + else + return "w"; + } else { + if (perms & VIR_CGROUP_DEVICE_MKNOD) + return "m"; + else + return ""; + } + } +} + + +/** * virCgroupAllowDevice: * * @group: The cgroup to allow a device for @@ -2641,10 +2679,8 @@ virCgroupAllowDevice(virCgroupPtr group, char type, int major, int minor, int ret = -1; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:%i %s%s%s", type, major, minor, - perms & VIR_CGROUP_DEVICE_READ ? "r" : "", - perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", - perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") < 0) + if (virAsprintf(&devstr, "%c %i:%i %s", type, major, minor, + virCgroupGetDevicePermsString(perms)) < 0) goto cleanup; if (virCgroupSetValueStr(group, @@ -2678,10 +2714,8 @@ virCgroupAllowDeviceMajor(virCgroupPtr group, char type, int major, int ret = -1; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:* %s%s%s", type, major, - perms & VIR_CGROUP_DEVICE_READ ? "r" : "", - perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", - perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") < 0) + if (virAsprintf(&devstr, "%c %i:* %s", type, major, + virCgroupGetDevicePermsString(perms)) < 0) goto cleanup; if (virCgroupSetValueStr(group, @@ -2752,10 +2786,8 @@ virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor, int ret = -1; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:%i %s%s%s", type, major, minor, - perms & VIR_CGROUP_DEVICE_READ ? "r" : "", - perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", - perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") < 0) + if (virAsprintf(&devstr, "%c %i:%i %s", type, major, minor, + virCgroupGetDevicePermsString(perms)) < 0) goto cleanup; if (virCgroupSetValueStr(group, @@ -2789,10 +2821,8 @@ virCgroupDenyDeviceMajor(virCgroupPtr group, char type, int major, int ret = -1; char *devstr = NULL; - if (virAsprintf(&devstr, "%c %i:* %s%s%s", type, major, - perms & VIR_CGROUP_DEVICE_READ ? "r" : "", - perms & VIR_CGROUP_DEVICE_WRITE ? "w" : "", - perms & VIR_CGROUP_DEVICE_MKNOD ? "m" : "") < 0) + if (virAsprintf(&devstr, "%c %i:* %s", type, major, + virCgroupGetDevicePermsString(perms)) < 0) goto cleanup; if (virCgroupSetValueStr(group, diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 7bb46bf..3ab9f1c 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -173,6 +173,8 @@ enum { VIR_CGROUP_DEVICE_RWM = VIR_CGROUP_DEVICE_RW | VIR_CGROUP_DEVICE_MKNOD, }; +const char *virCgroupGetDevicePermsString(int perms); + int virCgroupDenyAllDevices(virCgroupPtr group); int virCgroupAllowDevice(virCgroupPtr group, -- 1.9.3

On 06/25/2014 10:54 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

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 chagne. --- src/qemu/qemu_cgroup.c | 104 ++++++++++++++++++++++++------------------------- src/qemu/qemu_cgroup.h | 3 ++ 2 files changed, 55 insertions(+), 52 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 3394c68..1deafb7 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -49,30 +49,56 @@ 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 || + virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK) { + 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 +107,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 +122,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 06/25/2014 10:54 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 chagne.
s/chagne/change/
--- src/qemu/qemu_cgroup.c | 104 ++++++++++++++++++++++++------------------------- src/qemu/qemu_cgroup.h | 3 ++ 2 files changed, 55 insertions(+), 52 deletions(-)
+int +qemuSetImageCgroup(virDomainObjPtr vm, + virStorageSourcePtr src, + bool deny)
Bikeshedding: I would have named it 'bool allow' and flipped the logic (that is, passing true to turn on the cgroup, false to turn it back off). What you have works, though, so I won't insist. 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 1deafb7..97229c4 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; @@ -76,7 +77,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", @@ -104,14 +105,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 06/25/2014 10:54 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 99e9d52..78d6e3c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -853,6 +853,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 06/25/2014 10:54 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(-)
+static int virDomainLockManagerAddImage(virLockManagerPtr lock, + virStorageSourcePtr src)
- 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;
Don't you also need to ensure that backing files in the chain are marked readonly regardless of the setting on the active layer? Oh, I see - on the lock manager, we are currently only ever locking the active image. Hmm, I wonder if this is actually correct in the presence of snapshots - are we leaving a file locked when it becomes the backing element of a new file due to an external snapshot? We may have pre-existing bugs (while I know I got SELinux and cgroup issues sorted out in my testing, I haven't personally played much with the lease manager at the same time as playing with snapshots). But as far as I can tell, this patch makes no semantic difference, so it is no worse than what we already had. Therefore, 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 78d6e3c..98595e1 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -915,6 +915,7 @@ virSecurityManagerReserveLabel; virSecurityManagerRestoreAllLabel; virSecurityManagerRestoreDiskLabel; virSecurityManagerRestoreHostdevLabel; +virSecurityManagerRestoreImageLabel; virSecurityManagerRestoreSavedStateLabel; virSecurityManagerSetAllLabel; virSecurityManagerSetChildProcessLabel; @@ -923,6 +924,7 @@ virSecurityManagerSetDiskLabel; virSecurityManagerSetHostdevLabel; virSecurityManagerSetHugepages; virSecurityManagerSetImageFDLabel; +virSecurityManagerSetImageLabel; virSecurityManagerSetProcessLabel; virSecurityManagerSetSavedStateLabel; virSecurityManagerSetSocketLabel; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 062dc8f..f0dca09 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -112,6 +112,13 @@ typedef char *(*virSecurityDomainGetMountOptions) (virSecurityManagerPtr mgr, typedef int (*virSecurityDomainSetHugepages) (virSecurityManagerPtr mgr, virDomainDefPtr def, const char *path); +typedef int (*virSecurityDomainSetImageLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src); +typedef int (*virSecurityDomainRestoreImageLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virStorageSourcePtr src); + struct _virSecurityDriver { size_t privateDataLen; @@ -130,6 +137,9 @@ struct _virSecurityDriver { virSecurityDomainSetDiskLabel domainSetSecurityDiskLabel; virSecurityDomainRestoreDiskLabel domainRestoreSecurityDiskLabel; + virSecurityDomainSetImageLabel domainSetSecurityImageLabel; + virSecurityDomainRestoreImageLabel domainRestoreSecurityImageLabel; + virSecurityDomainSetDaemonSocketLabel domainSetSecurityDaemonSocketLabel; virSecurityDomainSetSocketLabel domainSetSecuritySocketLabel; virSecurityDomainClearSocketLabel domainClearSecuritySocketLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 06e5123..16bec5c 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -360,6 +360,34 @@ virSecurityManagerRestoreDiskLabel(virSecurityManagerPtr mgr, } +/** + * virSecurityManagerRestoreImageLabel: + * @mgr: security manager object + * @vm: domain definition object + * @src: disk source definition to operate on + * + * Removes security label from a single storage image. + * + * Returns: 0 on success, -1 on error. + */ +int +virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src) +{ + if (mgr->drv->domainRestoreSecurityImageLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainRestoreSecurityImageLabel(mgr, vm, src); + virObjectUnlock(mgr); + return ret; + } + + virReportUnsupportedError(); + return -1; +} + + int virSecurityManagerSetDaemonSocketLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm) @@ -440,6 +468,34 @@ virSecurityManagerSetDiskLabel(virSecurityManagerPtr mgr, } +/** + * virSecurityManagerSetImageLabel: + * @mgr: security manager object + * @vm: domain definition object + * @src: disk source definition to operate on + * + * Labels a single storage image with the configured security label. + * + * Returns: 0 on success, -1 on error. + */ +int +virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src) +{ + if (mgr->drv->domainSetSecurityImageLabel) { + int ret; + virObjectLock(mgr); + ret = mgr->drv->domainSetSecurityImageLabel(mgr, vm, src); + virObjectUnlock(mgr); + return ret; + } + + virReportUnsupportedError(); + return -1; +} + + int virSecurityManagerRestoreHostdevLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 8a5fcfb..97b6a2e 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -124,4 +124,11 @@ int virSecurityManagerSetHugepages(virSecurityManagerPtr mgr, virDomainDefPtr sec, const char *hugepages_path); +int virSecurityManagerSetImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src); +int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src); + #endif /* VIR_SECURITY_MANAGER_H__ */ diff --git a/src/security/security_nop.c b/src/security/security_nop.c index b57bf05..951125d 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -220,6 +220,22 @@ virSecurityGetBaseLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return NULL; } +static int +virSecurityDomainRestoreImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED, + virStorageSourcePtr src ATTRIBUTE_UNUSED) +{ + return 0; +} + +static int +virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def ATTRIBUTE_UNUSED, + virStorageSourcePtr src ATTRIBUTE_UNUSED) +{ + return 0; +} + virSecurityDriver virSecurityDriverNop = { .privateDataLen = 0, @@ -236,6 +252,9 @@ virSecurityDriver virSecurityDriverNop = { .domainSetSecurityDiskLabel = virSecurityDomainSetDiskLabelNop, .domainRestoreSecurityDiskLabel = virSecurityDomainRestoreDiskLabelNop, + .domainSetSecurityImageLabel = virSecurityDomainSetImageLabelNop, + .domainRestoreSecurityImageLabel = virSecurityDomainRestoreImageLabelNop, + .domainSetSecurityDaemonSocketLabel = virSecurityDomainSetDaemonSocketLabelNop, .domainSetSecuritySocketLabel = virSecurityDomainSetSocketLabelNop, .domainClearSecuritySocketLabel = virSecurityDomainClearSocketLabelNop, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index e3e9c85..1ded57b 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -564,6 +564,41 @@ virSecurityStackGetBaseLabel(virSecurityManagerPtr mgr, int virtType) virtType); } +static int +virSecurityStackSetSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerSetImageLabel(item->securityManager, vm, src) < 0) + rc = -1; + } + + return rc; +} + +static int +virSecurityStackRestoreSecurityImageLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virStorageSourcePtr src) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerRestoreImageLabel(item->securityManager, + vm, src) < 0) + rc = -1; + } + + return rc; +} + virSecurityDriver virSecurityDriverStack = { .privateDataLen = sizeof(virSecurityStackData), .name = "stack", @@ -581,6 +616,9 @@ virSecurityDriver virSecurityDriverStack = { .domainSetSecurityDiskLabel = virSecurityStackSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = virSecurityStackRestoreSecurityDiskLabel, + .domainSetSecurityImageLabel = virSecurityStackSetSecurityImageLabel, + .domainRestoreSecurityImageLabel = virSecurityStackRestoreSecurityImageLabel, + .domainSetSecurityDaemonSocketLabel = virSecurityStackSetDaemonSocketLabel, .domainSetSecuritySocketLabel = virSecurityStackSetSocketLabel, .domainClearSecuritySocketLabel = virSecurityStackClearSocketLabel, -- 1.9.3

Refactor the existing code to allow re-using it for the per-image label restore too. --- src/security/security_selinux.c | 60 ++++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 25 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7740e69..7b534b2 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1123,18 +1123,21 @@ 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 || + virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK) + 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 +1147,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 +1184,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 +1875,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 +2437,8 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSetSecurityDiskLabel = virSecuritySELinuxSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = virSecuritySELinuxRestoreSecurityDiskLabel, + .domainRestoreSecurityImageLabel = virSecuritySELinuxRestoreSecurityImageLabel, + .domainSetSecurityDaemonSocketLabel = virSecuritySELinuxSetSecurityDaemonSocketLabel, .domainSetSecuritySocketLabel = virSecuritySELinuxSetSecuritySocketLabel, .domainClearSecuritySocketLabel = virSecuritySELinuxClearSecuritySocketLabel, -- 1.9.3

Refactor the code and reuse it to implement the functionality. --- src/security/security_selinux.c | 92 ++++++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 38 deletions(-) diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7b534b2..97f91f7 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" @@ -1199,40 +1191,50 @@ 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; + + if (!src->path || + virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK) + return 0; + + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (!secdef || secdef->norelabel) + return 0; - disk_seclabel = virStorageSourceGetSecurityLabelDef(disk->src, + 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. */ @@ -1240,35 +1242,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) { @@ -2437,6 +2452,7 @@ virSecurityDriver virSecurityDriverSELinux = { .domainSetSecurityDiskLabel = virSecuritySELinuxSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = virSecuritySELinuxRestoreSecurityDiskLabel, + .domainSetSecurityImageLabel = virSecuritySELinuxSetSecurityImageLabel, .domainRestoreSecurityImageLabel = virSecuritySELinuxRestoreSecurityImageLabel, .domainSetSecurityDaemonSocketLabel = virSecuritySELinuxSetSecurityDaemonSocketLabel, -- 1.9.3

Refactor the existing code to allow re-using it for the per-image label restore too. --- src/security/security_dac.c | 60 ++++++++++++++++++++++++--------------------- 1 file changed, 32 insertions(+), 28 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 38cb47f..69b51c1 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -350,62 +350,64 @@ 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 || + virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK) 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 +416,7 @@ virSecurityDACRestoreSecurityDiskLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, virDomainDiskDefPtr disk) { - return virSecurityDACRestoreSecurityImageLabelInt(mgr, def, disk, false); + return virSecurityDACRestoreSecurityImageLabelInt(mgr, def, disk->src, false); } @@ -902,7 +904,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 +1278,8 @@ virSecurityDriver virSecurityDriverDAC = { .domainSetSecurityDiskLabel = virSecurityDACSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = virSecurityDACRestoreSecurityDiskLabel, + .domainRestoreSecurityImageLabel = virSecurityDACRestoreSecurityImageLabel, + .domainSetSecurityDaemonSocketLabel = virSecurityDACSetDaemonSocketLabel, .domainSetSecuritySocketLabel = virSecurityDACSetSocketLabel, .domainClearSecuritySocketLabel = virSecurityDACClearSocketLabel, -- 1.9.3

Refactor the code and reuse it to implement the functionality. --- src/security/security_dac.c | 53 ++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 69b51c1..3ff7817 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -289,22 +289,30 @@ 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 || + virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK) + 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 +324,7 @@ virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk, return -1; } - return virSecurityDACSetOwnership(path, user, group); + return virSecurityDACSetOwnership(src->path, user, group); } @@ -326,24 +334,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; } @@ -1278,6 +1276,7 @@ virSecurityDriver virSecurityDriverDAC = { .domainSetSecurityDiskLabel = virSecurityDACSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = virSecurityDACRestoreSecurityDiskLabel, + .domainSetSecurityImageLabel = virSecurityDACSetSecurityImageLabel, .domainRestoreSecurityImageLabel = virSecurityDACRestoreSecurityImageLabel, .domainSetSecurityDaemonSocketLabel = virSecurityDACSetDaemonSocketLabel, -- 1.9.3

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..72d1e16 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 (virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK) 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

Refactor the code and reuse it to implement the functionality. --- src/security/security_apparmor.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 72d1e16..fb41c5a 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -704,41 +704,40 @@ 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 || + virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK) + 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 +755,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 +990,7 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainSetSecurityDiskLabel = AppArmorSetSecurityDiskLabel, .domainRestoreSecurityDiskLabel = AppArmorRestoreSecurityDiskLabel, + .domainSetSecurityImageLabel = AppArmorSetSecurityImageLabel, .domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel, .domainSetSecurityDaemonSocketLabel = AppArmorSetSecurityDaemonSocketLabel, -- 1.9.3

There's a lot of places where we skip doing actions based on the locality of given storage type. The usual pattern is to skip it if: virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK Add a simple helper to simplify the pattern to virStorageSourceIsLocalStorage(src) --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 7 +++++++ src/util/virstoragefile.h | 1 + 3 files changed, 9 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 98595e1..889ae04 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1915,6 +1915,7 @@ virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceGetSecurityLabelDef; +virStorageSourceIsLocalStorage; virStorageSourceNewFromBacking; virStorageSourcePoolDefFree; virStorageSourcePoolModeTypeFromString; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 5f8e02d..965c3ea 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1718,6 +1718,13 @@ virStorageSourceGetActualType(virStorageSourcePtr def) } +bool +virStorageSourceIsLocalStorage(virStorageSourcePtr src) +{ + return virStorageSourceGetActualType(src) != VIR_STORAGE_TYPE_NETWORK; +} + + /** * virStorageSourceBackingStoreClear: * diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 3e13071..ee85ca5 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -329,6 +329,7 @@ void virStorageSourceAuthClear(virStorageSourcePtr def); void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def); void virStorageSourceClear(virStorageSourcePtr def); int virStorageSourceGetActualType(virStorageSourcePtr def); +bool virStorageSourceIsLocalStorage(virStorageSourcePtr src); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceBackingStoreClear(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); -- 1.9.3

On 06/25/2014 10:55 AM, Peter Krempa wrote:
There's a lot of places where we skip doing actions based on the locality of given storage type. The usual pattern is to skip it if:
virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK
Add a simple helper to simplify the pattern to virStorageSourceIsLocalStorage(src) --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 7 +++++++ src/util/virstoragefile.h | 1 + 3 files changed, 9 insertions(+)
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 965c3ea..e154f92 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1344,13 +1344,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; @@ -1371,24 +1370,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; } @@ -1401,6 +1403,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, if (!chain) goto error; + return chain; error: -- 1.9.3

Instead of just returning the parent path, return the complete parent source structure. --- src/qemu/qemu_driver.c | 16 ++++----- src/util/virstoragefile.c | 17 ++++------ src/util/virstoragefile.h | 2 +- tests/virstoragetest.c | 86 ++++++++++++++++++++++------------------------- 4 files changed, 56 insertions(+), 65 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 552e595..4546862 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15488,7 +15488,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 */ @@ -15596,10 +15596,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, @@ -15619,10 +15618,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 e154f92..9a7f997 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1337,17 +1337,16 @@ virStorageFileChainLookup(virStorageSourcePtr chain, virStorageSourcePtr startFrom, const char *name, unsigned int idx, - const char **parent) + virStorageSourcePtr *parent) { - virStorageSourcePtr prev = NULL; + virStorageSourcePtr prev; const char *start = chain->path; - const char *tmp; char *parentDir = NULL; bool nameIsFile = virStorageIsFile(name); size_t i = 0; if (!parent) - parent = &tmp; + parent = &prev; *parent = NULL; if (startFrom) { @@ -1355,7 +1354,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, chain = chain->backingStore; i++; } - *parent = startFrom->path; + *parent = startFrom; } while (chain) { @@ -1372,8 +1371,8 @@ virStorageFileChainLookup(virStorageSourcePtr chain, break; if (nameIsFile && virStorageSourceIsLocalStorage(chain)) { - if (prev && virStorageSourceIsLocalStorage(prev)) - parentDir = mdir_name(prev->path); + if (*parent && virStorageSourceIsLocalStorage(*parent)) + parentDir = mdir_name((*parent)->path); else ignore_value(VIR_STRDUP(parentDir, ".")); @@ -1382,7 +1381,6 @@ virStorageFileChainLookup(virStorageSourcePtr chain, goto error; } - int result = virFileRelLinkPointsTo(parentDir, name, chain->path); @@ -1395,8 +1393,7 @@ virStorageFileChainLookup(virStorageSourcePtr chain, break; } } - *parent = chain->path; - prev = chain; + *parent = chain; chain = chain->backingStore; i++; } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index ee85ca5..6cbcd6b 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -297,7 +297,7 @@ virStorageSourcePtr virStorageFileChainLookup(virStorageSourcePtr chain, virStorageSourcePtr startFrom, const char *name, unsigned int idx, - const char **parent) + virStorageSourcePtr *parent) ATTRIBUTE_NONNULL(1); int virStorageFileResize(const char *path, diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index fb2837f..e2ee3ff 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -415,7 +415,7 @@ struct testLookupData unsigned int expIndex; const char *expResult; virStorageSourcePtr expMeta; - const char *expParent; + virStorageSourcePtr expParent; }; static int @@ -424,7 +424,7 @@ testStorageLookup(const void *args) const struct testLookupData *data = args; int ret = 0; virStorageSourcePtr result; - const char *actualParent; + virStorageSourcePtr actualParent; unsigned int idx; if (virStorageFileParseChainIndex(data->target, data->name, &idx) < 0 && @@ -488,9 +488,10 @@ testStorageLookup(const void *args) data->expMeta, result); ret = -1; } - if (STRNEQ_NULLABLE(data->expParent, actualParent)) { + if (data->expParent != actualParent) { fprintf(stderr, "parent: expected %s, got %s\n", - NULLSTR(data->expParent), NULLSTR(actualParent)); + NULLSTR(data->expParent ? data->expParent->path : NULL), + NULLSTR(actualParent ? actualParent->path : NULL)); ret = -1; } @@ -974,25 +975,25 @@ mymain(void) TEST_LOOKUP(5, NULL, abswrap, chain->path, chain, NULL); TEST_LOOKUP(6, chain, abswrap, NULL, NULL, NULL); TEST_LOOKUP(7, chain2, abswrap, NULL, NULL, NULL); - TEST_LOOKUP(8, NULL, "qcow2", chain2->path, chain2, chain->path); - TEST_LOOKUP(9, chain, "qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(8, NULL, "qcow2", chain2->path, chain2, chain); + TEST_LOOKUP(9, chain, "qcow2", chain2->path, chain2, chain); TEST_LOOKUP(10, chain2, "qcow2", NULL, NULL, NULL); TEST_LOOKUP(11, chain3, "qcow2", NULL, NULL, NULL); - TEST_LOOKUP(12, NULL, absqcow2, chain2->path, chain2, chain->path); - TEST_LOOKUP(13, chain, absqcow2, chain2->path, chain2, chain->path); + TEST_LOOKUP(12, NULL, absqcow2, chain2->path, chain2, chain); + TEST_LOOKUP(13, chain, absqcow2, chain2->path, chain2, chain); TEST_LOOKUP(14, chain2, absqcow2, NULL, NULL, NULL); TEST_LOOKUP(15, chain3, absqcow2, NULL, NULL, NULL); - TEST_LOOKUP(16, NULL, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(17, chain, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(18, chain2, "raw", chain3->path, chain3, chain2->path); + TEST_LOOKUP(16, NULL, "raw", chain3->path, chain3, chain2); + TEST_LOOKUP(17, chain, "raw", chain3->path, chain3, chain2); + TEST_LOOKUP(18, chain2, "raw", chain3->path, chain3, chain2); TEST_LOOKUP(19, chain3, "raw", NULL, NULL, NULL); - TEST_LOOKUP(20, NULL, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(21, chain, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(22, chain2, absraw, chain3->path, chain3, chain2->path); + TEST_LOOKUP(20, NULL, absraw, chain3->path, chain3, chain2); + TEST_LOOKUP(21, chain, absraw, chain3->path, chain3, chain2); + TEST_LOOKUP(22, chain2, absraw, chain3->path, chain3, chain2); TEST_LOOKUP(23, chain3, absraw, NULL, NULL, NULL); - TEST_LOOKUP(24, NULL, NULL, chain3->path, chain3, chain2->path); - TEST_LOOKUP(25, chain, NULL, chain3->path, chain3, chain2->path); - TEST_LOOKUP(26, chain2, NULL, chain3->path, chain3, chain2->path); + TEST_LOOKUP(24, NULL, NULL, chain3->path, chain3, chain2); + TEST_LOOKUP(25, chain, NULL, chain3->path, chain3, chain2); + TEST_LOOKUP(26, chain2, NULL, chain3->path, chain3, chain2); TEST_LOOKUP(27, chain3, NULL, NULL, NULL, NULL); /* Rewrite wrap and qcow2 back to 3-deep chain, relative backing */ @@ -1027,25 +1028,25 @@ mymain(void) TEST_LOOKUP(33, NULL, abswrap, chain->path, chain, NULL); TEST_LOOKUP(34, chain, abswrap, NULL, NULL, NULL); TEST_LOOKUP(35, chain2, abswrap, NULL, NULL, NULL); - TEST_LOOKUP(36, NULL, "qcow2", chain2->path, chain2, chain->path); - TEST_LOOKUP(37, chain, "qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(36, NULL, "qcow2", chain2->path, chain2, chain); + TEST_LOOKUP(37, chain, "qcow2", chain2->path, chain2, chain); TEST_LOOKUP(38, chain2, "qcow2", NULL, NULL, NULL); TEST_LOOKUP(39, chain3, "qcow2", NULL, NULL, NULL); - TEST_LOOKUP(40, NULL, absqcow2, chain2->path, chain2, chain->path); - TEST_LOOKUP(41, chain, absqcow2, chain2->path, chain2, chain->path); + TEST_LOOKUP(40, NULL, absqcow2, chain2->path, chain2, chain); + TEST_LOOKUP(41, chain, absqcow2, chain2->path, chain2, chain); TEST_LOOKUP(42, chain2, absqcow2, NULL, NULL, NULL); TEST_LOOKUP(43, chain3, absqcow2, NULL, NULL, NULL); - TEST_LOOKUP(44, NULL, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(45, chain, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(46, chain2, "raw", chain3->path, chain3, chain2->path); + TEST_LOOKUP(44, NULL, "raw", chain3->path, chain3, chain2); + TEST_LOOKUP(45, chain, "raw", chain3->path, chain3, chain2); + TEST_LOOKUP(46, chain2, "raw", chain3->path, chain3, chain2); TEST_LOOKUP(47, chain3, "raw", NULL, NULL, NULL); - TEST_LOOKUP(48, NULL, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(49, chain, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(50, chain2, absraw, chain3->path, chain3, chain2->path); + TEST_LOOKUP(48, NULL, absraw, chain3->path, chain3, chain2); + TEST_LOOKUP(49, chain, absraw, chain3->path, chain3, chain2); + TEST_LOOKUP(50, chain2, absraw, chain3->path, chain3, chain2); TEST_LOOKUP(51, chain3, absraw, NULL, NULL, NULL); - TEST_LOOKUP(52, NULL, NULL, chain3->path, chain3, chain2->path); - TEST_LOOKUP(53, chain, NULL, chain3->path, chain3, chain2->path); - TEST_LOOKUP(54, chain2, NULL, chain3->path, chain3, chain2->path); + TEST_LOOKUP(52, NULL, NULL, chain3->path, chain3, chain2); + TEST_LOOKUP(53, chain, NULL, chain3->path, chain3, chain2); + TEST_LOOKUP(54, chain2, NULL, chain3->path, chain3, chain2); TEST_LOOKUP(55, chain3, NULL, NULL, NULL, NULL); /* Use link to wrap with cross-directory relative backing */ @@ -1070,12 +1071,12 @@ mymain(void) TEST_LOOKUP(57, NULL, "sub/link2", chain->path, chain, NULL); TEST_LOOKUP(58, NULL, "wrap", chain->path, chain, NULL); TEST_LOOKUP(59, NULL, abswrap, chain->path, chain, NULL); - TEST_LOOKUP(60, NULL, "../qcow2", chain2->path, chain2, chain->path); + TEST_LOOKUP(60, NULL, "../qcow2", chain2->path, chain2, chain); TEST_LOOKUP(61, NULL, "qcow2", NULL, NULL, NULL); - TEST_LOOKUP(62, NULL, absqcow2, chain2->path, chain2, chain->path); - TEST_LOOKUP(63, NULL, "raw", chain3->path, chain3, chain2->path); - TEST_LOOKUP(64, NULL, absraw, chain3->path, chain3, chain2->path); - TEST_LOOKUP(65, NULL, NULL, chain3->path, chain3, chain2->path); + TEST_LOOKUP(62, NULL, absqcow2, chain2->path, chain2, chain); + TEST_LOOKUP(63, NULL, "raw", chain3->path, chain3, chain2); + TEST_LOOKUP(64, NULL, absraw, chain3->path, chain3, chain2); + TEST_LOOKUP(65, NULL, NULL, chain3->path, chain3, chain2); TEST_LOOKUP_TARGET(66, "vda", NULL, "bogus[1]", 0, NULL, NULL, NULL); TEST_LOOKUP_TARGET(67, "vda", NULL, "vda[-1]", 0, NULL, NULL, NULL); @@ -1084,18 +1085,13 @@ mymain(void) TEST_LOOKUP_TARGET(70, "vda", chain, "wrap", 0, NULL, NULL, NULL); TEST_LOOKUP_TARGET(71, "vda", chain2, "wrap", 0, NULL, NULL, NULL); TEST_LOOKUP_TARGET(72, "vda", NULL, "vda[0]", 0, NULL, NULL, NULL); - TEST_LOOKUP_TARGET(73, "vda", NULL, "vda[1]", 1, chain2->path, chain2, - chain->path); - TEST_LOOKUP_TARGET(74, "vda", chain, "vda[1]", 1, chain2->path, chain2, - chain->path); + TEST_LOOKUP_TARGET(73, "vda", NULL, "vda[1]", 1, chain2->path, chain2, chain); + TEST_LOOKUP_TARGET(74, "vda", chain, "vda[1]", 1, chain2->path, chain2, chain); TEST_LOOKUP_TARGET(75, "vda", chain2, "vda[1]", 1, NULL, NULL, NULL); TEST_LOOKUP_TARGET(76, "vda", chain3, "vda[1]", 1, NULL, NULL, NULL); - TEST_LOOKUP_TARGET(77, "vda", NULL, "vda[2]", 2, chain3->path, chain3, - chain2->path); - TEST_LOOKUP_TARGET(78, "vda", chain, "vda[2]", 2, chain3->path, chain3, - chain2->path); - TEST_LOOKUP_TARGET(79, "vda", chain2, "vda[2]", 2, chain3->path, chain3, - chain2->path); + TEST_LOOKUP_TARGET(77, "vda", NULL, "vda[2]", 2, chain3->path, chain3, chain2); + TEST_LOOKUP_TARGET(78, "vda", chain, "vda[2]", 2, chain3->path, chain3, chain2); + TEST_LOOKUP_TARGET(79, "vda", chain2, "vda[2]", 2, chain3->path, chain3, chain2); TEST_LOOKUP_TARGET(80, "vda", chain3, "vda[2]", 2, NULL, NULL, NULL); TEST_LOOKUP_TARGET(81, "vda", NULL, "vda[3]", 3, NULL, NULL, NULL); -- 1.9.3

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 4546862..66752f1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12117,25 +12117,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, @@ -15374,10 +15355,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; } @@ -15388,8 +15369,8 @@ qemuDomainBlockCopy(virDomainObjPtr vm, virDomainAuditDisk(vm, NULL, dest, "mirror", ret >= 0); qemuDomainObjExitMonitor(driver, vm); if (ret < 0) { - qemuDomainPrepareDiskChainElementPath(driver, vm, disk, dest, - VIR_DISK_CHAIN_NO_ACCESS); + qemuDomainPrepareDiskChainElement(driver, vm, disk, mirror, + VIR_DISK_CHAIN_NO_ACCESS); goto endjob; } -- 1.9.3

When creating a new disk mirror the new struct is stored in a separate variable until everything went well. The removed hunk would actually remove existing mirror information for example when the api would be run if a mirror still exists. --- src/qemu/qemu_driver.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 66752f1..f6f5ace 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15382,10 +15382,6 @@ qemuDomainBlockCopy(virDomainObjPtr vm, endjob: if (need_unlink && unlink(dest)) VIR_WARN("unable to unlink just-created %s", dest); - if (ret < 0 && disk) { - virStorageSourceFree(disk->mirror); - disk->mirror = NULL; - } virStorageSourceFree(mirror); if (!qemuDomainObjEndJob(driver, vm)) vm = NULL; -- 1.9.3

On 06/25/2014 10:55 AM, Peter Krempa wrote:
When creating a new disk mirror the new struct is stored in a separate variable until everything went well. The removed hunk would actually remove existing mirror information for example when the api would be run if a mirror still exists. --- src/qemu/qemu_driver.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 66752f1..f6f5ace 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15382,10 +15382,6 @@ qemuDomainBlockCopy(virDomainObjPtr vm, endjob: if (need_unlink && unlink(dest)) VIR_WARN("unable to unlink just-created %s", dest); - if (ret < 0 && disk) { - virStorageSourceFree(disk->mirror); - disk->mirror = NULL; - }
Oh my. This was a regression latently introduced in commit ff5f30b, v1.2.1, then aggravated in commit 7b7bf001 (thankfully unreleased). Thanks for catching and fixing this. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 06/26/14 05:29, Eric Blake wrote:
On 06/25/2014 10:55 AM, Peter Krempa wrote:
When creating a new disk mirror the new struct is stored in a separate variable until everything went well. The removed hunk would actually remove existing mirror information for example when the api would be run if a mirror still exists. --- src/qemu/qemu_driver.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 66752f1..f6f5ace 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15382,10 +15382,6 @@ qemuDomainBlockCopy(virDomainObjPtr vm, endjob: if (need_unlink && unlink(dest)) VIR_WARN("unable to unlink just-created %s", dest); - if (ret < 0 && disk) { - virStorageSourceFree(disk->mirror); - disk->mirror = NULL; - }
Oh my. This was a regression latently introduced in commit ff5f30b, v1.2.1, then aggravated in commit 7b7bf001 (thankfully unreleased). Thanks for catching and fixing this.
ACK.
I've pushed this one and 1-7 of this series as they are trivial enough to get in during the freeze. I'm not sure though about the other changes. Peter
participants (2)
-
Eric Blake
-
Peter Krempa