[libvirt] [PATCH v3 0/4]Add startupPolicy attribute support for hard disks
v2 to v3: not only check disk source, startupPolicy should work if any backing file is missing. The commit 039a3283 break the limition of contiguous device boot orders, so I remove my previous patch about it. v1 to v2: added relax schema for disk of block and dir type removed original patch 3/5. The set of patches is trying to add 'startupPolicy' attribute support to the source element of hard disks. Policy levels are using the mandatory, requisite, optional levels as originally documented. For the 'optional' policy, there is a little difference from CDROM and Floppy which only drop its source path, for disks, if missing, the checking function will drop their definitions, because qemu doesn't allow missing source path for hard disk. Guannan Ren [PATCH v3 1/4] conf: add startupPolicy attribute for harddisk [PATCH v3 2/4] storage: report error rather than warning if backing files doesn't exist [PATCH v3 3/4] qemu: drop disk with 'optional' startupPolicy if it is not present [PATCH v3 4/4] security: restore DAC for every disk file in disk chain docs/schemas/domaincommon.rng | 6 +++++ include/libvirt/libvirt.h.in | 1 + src/conf/domain_conf.c | 20 ++++++++++------ src/qemu/qemu_domain.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------ src/qemu/qemu_process.c | 7 ------ src/security/security_dac.c | 15 +++++++++++- src/util/virstoragefile.c | 23 +++++++++++------- tests/virstoragetest.c | 16 ++++++------- 8 files changed, 130 insertions(+), 77 deletions(-)
Add startupPolicy attribute policy for harddisk with type "file", "block" and "dir". The "network" type disk is still not supported. --- docs/schemas/domaincommon.rng | 6 ++++++ src/conf/domain_conf.c | 20 +++++++++++++------- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3cace35..fb3f980 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1095,6 +1095,9 @@ <ref name="absFilePath"/> </attribute> <optional> + <ref name="startupPolicy"/> + </optional> + <optional> <ref name='devSeclabel'/> </optional> </element> @@ -1112,6 +1115,9 @@ <attribute name="dir"> <ref name="absFilePath"/> </attribute> + <optional> + <ref name="startupPolicy"/> + </optional> <empty/> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3e81013..4485756 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4737,7 +4737,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, switch (def->type) { case VIR_DOMAIN_DISK_TYPE_FILE: source = virXMLPropString(cur, "file"); - startupPolicy = virXMLPropString(cur, "startupPolicy"); break; case VIR_DOMAIN_DISK_TYPE_BLOCK: source = virXMLPropString(cur, "dev"); @@ -4826,7 +4825,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, case VIR_DOMAIN_DISK_TYPE_VOLUME: if (virDomainDiskSourcePoolDefParse(cur, def) < 0) goto error; - startupPolicy = virXMLPropString(cur, "startupPolicy"); break; default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -4835,6 +4833,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } + startupPolicy = virXMLPropString(cur, "startupPolicy"); + /* People sometimes pass a bogus '' source path when they mean to omit the source element completely (e.g. CDROM without media). This is @@ -5400,11 +5400,10 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } - if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && - def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + if (def->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { virReportError(VIR_ERR_INVALID_ARG, - _("Setting disk %s is allowed only for " - "cdrom or floppy"), + _("Setting disk %s is not allowed for " + "disk of network type"), startupPolicy); goto error; } @@ -13805,6 +13804,9 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, case VIR_DOMAIN_DISK_TYPE_BLOCK: virBufferEscapeString(buf, " <source dev='%s'", def->src); + if (def->startupPolicy) + virBufferEscapeString(buf, " startupPolicy='%s'", + startupPolicy); if (def->nseclabels) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 8); @@ -13817,8 +13819,12 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, } break; case VIR_DOMAIN_DISK_TYPE_DIR: - virBufferEscapeString(buf, " <source dir='%s'/>\n", + virBufferEscapeString(buf, " <source dir='%s'", def->src); + if (def->startupPolicy) + virBufferEscapeString(buf, " startupPolicy='%s'", + startupPolicy); + virBufferAddLit(buf, "/>\n"); break; case VIR_DOMAIN_DISK_TYPE_NETWORK: virBufferAsprintf(buf, " <source protocol='%s'", -- 1.8.1.4
If one of backing files for disk source doesn't exist, the guest will not be able to find and use the disk even though the disk still exists in guest xml definition. So reporting an error make more sense. Adding virFileAccessibleAs() to check if the backing file described in disk meta exist in real path. If not, report error. the uid and gid arguments don't have so much meannings for F_OK, so give 0 for them. --- src/util/virstoragefile.c | 23 +++++++++++++++-------- tests/virstoragetest.c | 16 ++++++++-------- 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index b985df4..4781221 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -534,6 +534,13 @@ virFindBackingFile(const char *start, bool start_is_dir, const char *path, goto cleanup; } + if (virFileAccessibleAs(combined, F_OK, 0, 0) < 0) { + virReportSystemError(errno, + _("Backing file '%s' does not exist"), + combined); + goto cleanup; + } + if (!(*canonical = canonicalize_file_name(combined))) { virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); @@ -784,14 +791,10 @@ virStorageFileGetMetadataInternal(const char *path, !!directory, backing, &meta->directory, &meta->backingStore) < 0) { - /* the backing file is (currently) unavailable, treat this - * file as standalone: - * backingStoreRaw is kept to mark broken image chains */ - meta->backingStoreIsFile = false; - backingFormat = VIR_STORAGE_FILE_NONE; - VIR_WARN("Backing file '%s' of image '%s' is missing.", - meta->backingStoreRaw, path); - + VIR_ERROR(_("Backing file '%s' of image '%s' is missing."), + meta->backingStoreRaw, path); + VIR_FREE(backing); + goto cleanup; } } VIR_FREE(backing); @@ -968,6 +971,10 @@ virStorageFileGetMetadataRecurse(const char *path, const char *directory, uid, gid, allow_probe, cycle); + if (!ret->backingMeta) { + virStorageFileFreeMetadata(ret); + ret = NULL; + } } return ret; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index fef4b37..fb5da5c 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -483,10 +483,10 @@ mymain(void) /* Qcow2 file with missing backing file but specified type */ const testFileData chain9[] = { qcow2_bogus }; TEST_CHAIN(9, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - chain9, EXP_WARN, - chain9, ALLOW_PROBE | EXP_WARN, - chain9, EXP_WARN, - chain9, ALLOW_PROBE | EXP_WARN); + chain9, EXP_FAIL, + chain9, ALLOW_PROBE | EXP_FAIL, + chain9, EXP_FAIL, + chain9, ALLOW_PROBE | EXP_FAIL); /* Rewrite qcow2 to a missing backing file, without backing type */ virCommandFree(cmd); @@ -498,10 +498,10 @@ mymain(void) /* Qcow2 file with missing backing file and no specified type */ const testFileData chain10[] = { qcow2_bogus }; TEST_CHAIN(10, "qcow2", absqcow2, VIR_STORAGE_FILE_QCOW2, - chain10, EXP_WARN, - chain10, ALLOW_PROBE | EXP_WARN, - chain10, EXP_WARN, - chain10, ALLOW_PROBE | EXP_WARN); + chain10, EXP_FAIL, + chain10, ALLOW_PROBE | EXP_FAIL, + chain10, EXP_FAIL, + chain10, ALLOW_PROBE | EXP_FAIL); /* Rewrite qcow2 to use an nbd: protocol as backend */ virCommandFree(cmd); -- 1.8.1.4
Go through disks of guest, if one disk doesn't exist or its backing chain is broken, with 'optional' startupPolicy, for CDROM and Floppy we only discard its source path definition in xml, for disks we drop it from disk list and free it. Originally, for these disks without startupPolicy attribute, libvirt ignore the presence checking, this patch make startupPolicy 'mandatory' as default. --- include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_domain.c | 119 ++++++++++++++++++++++++++----------------- src/qemu/qemu_process.c | 7 --- 3 files changed, 74 insertions(+), 53 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index e057be1..14d5363 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4610,6 +4610,7 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, */ typedef enum { VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0, /* oldSrcPath is set */ + VIR_DOMAIN_EVENT_HARDDISK_DROP_MISSING_ON_START = 1, #ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_DISK_CHANGE_LAST diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index fdcf7bc..62a54ef 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1977,75 +1977,102 @@ cleanup: virObjectUnref(cfg); } +static int +qemuDomainCheckRemoveOptionalDisk(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk) +{ + char uuid[VIR_UUID_STRING_BUFLEN]; + virDomainEventPtr event = NULL; + virDomainDiskDefPtr del_disk = NULL; + + virUUIDFormat(vm->def->uuid, uuid); + + VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " + "due to inaccessible source '%s'", + disk->dst, vm->def->name, uuid, disk->src); + + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || + disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + + event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, + disk->info.alias, + VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START); + + VIR_FREE(disk->src); + } else { + event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, + disk->info.alias, + VIR_DOMAIN_EVENT_HARDDISK_DROP_MISSING_ON_START); + + if (!(del_disk = virDomainDiskRemoveByName(vm->def, disk->src))) { + virReportError(VIR_ERR_INVALID_ARG, + _("no source device %s"), disk->src); + return -1; + } + virDomainDiskDefFree(del_disk); + } + + if (event) + qemuDomainEventQueue(driver, event); + + return 0; +} + int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, bool cold_boot) { int ret = -1; - int i; + size_t i; virDomainDiskDefPtr disk; - char uuid[VIR_UUID_STRING_BUFLEN]; - virDomainEventPtr event = NULL; - virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - - virUUIDFormat(vm->def->uuid, uuid); - - for (i = 0; i < vm->def->ndisks; i++) { - disk = vm->def->disks[i]; + size_t count = vm->def->ndisks; + size_t nextdisk = 0; - if (!disk->startupPolicy || !disk->src) - continue; + for (i = 0; i < count; i++) { + disk = vm->def->disks[nextdisk]; - if (virFileAccessibleAs(disk->src, F_OK, - cfg->user, - cfg->group) >= 0) { - /* disk accessible */ + if (qemuDomainDetermineDiskChain(driver, disk, false) == 0) { + nextdisk++; continue; } switch ((enum virDomainStartupPolicy) disk->startupPolicy) { - case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL: - break; + case VIR_DOMAIN_STARTUP_POLICY_OPTIONAL: + break; - case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); - goto cleanup; - break; - - case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: - if (cold_boot) { - virReportSystemError(errno, - _("cannot access file '%s'"), - disk->src); - goto cleanup; - } - break; + case VIR_DOMAIN_STARTUP_POLICY_REQUISITE: + if (cold_boot) { + goto error; + } + break; + + case VIR_DOMAIN_STARTUP_POLICY_MANDATORY: + case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: + goto error; - case VIR_DOMAIN_STARTUP_POLICY_DEFAULT: - case VIR_DOMAIN_STARTUP_POLICY_LAST: - /* this should never happen */ - break; + case VIR_DOMAIN_STARTUP_POLICY_LAST: + /* this should never happen */ + break; } - VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " - "due to inaccessible source '%s'", - disk->dst, vm->def->name, uuid, disk->src); + virResetLastError(); - event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, disk->info.alias, - VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START); - if (event) - qemuDomainEventQueue(driver, event); + /* For cdrom or floppy, we only remove its source definition, + * so, the nextdisk need to point to next disk in next loop. + */ + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || + disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) + nextdisk++; - VIR_FREE(disk->src); + if (qemuDomainCheckRemoveOptionalDisk(driver, vm, disk) < 0) + goto error; } ret = 0; -cleanup: - virObjectUnref(cfg); +error: return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c412ea2..7cbcbb3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3521,17 +3521,10 @@ int qemuProcessStart(virConnectPtr conn, if (qemuAssignDeviceAliases(vm->def, priv->qemuCaps) < 0) goto cleanup; - VIR_DEBUG("Checking for CDROM and floppy presence"); if (qemuDomainCheckDiskPresence(driver, vm, flags & VIR_QEMU_PROCESS_START_COLD) < 0) goto cleanup; - for (i = 0; i < vm->def->ndisks; i++) { - if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i], - false) < 0) - goto cleanup; - } - /* Get the advisory nodeset from numad if 'placement' of * either <vcpu> or <numatune> is 'auto'. */ -- 1.8.1.4
--- src/security/security_dac.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index b8d1a92..7a6f9c9 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -335,6 +335,16 @@ err: return rc; } +static int +virSecurityDACRestoreDiskSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, + const char *path, + size_t depth ATTRIBUTE_UNUSED, + void *opaque ATTRIBUTE_UNUSED) +{ + return virSecurityDACRestoreSecurityFileLabel(path); +} + + static int virSecurityDACSetSecurityFileLabel(virDomainDiskDefPtr disk ATTRIBUTE_UNUSED, @@ -424,7 +434,10 @@ virSecurityDACRestoreSecurityImageLabelInt(virSecurityManagerPtr mgr, } } - return virSecurityDACRestoreSecurityFileLabel(disk->src); + return virDomainDiskDefForeachPath(disk, + false, + virSecurityDACRestoreDiskSecurityFileLabel, + NULL); } -- 1.8.1.4
participants (1)
-
Guannan Ren