[libvirt] [PATCH v4 0/4] Add startupPolicy attribute support for hard disks
v3: https://www.redhat.com/archives/libvir-list/2013-June/thread.html v3 to v4: Rebase 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 cf82878..483f068 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1093,6 +1093,9 @@ <ref name="absFilePath"/> </attribute> <optional> + <ref name="startupPolicy"/> + </optional> + <optional> <ref name='devSeclabel'/> </optional> </element> @@ -1110,6 +1113,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 011de71..1040b40 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4739,7 +4739,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"); @@ -4828,7 +4827,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, @@ -4837,6 +4835,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 @@ -5410,11 +5410,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; } @@ -13872,6 +13871,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); @@ -13884,8 +13886,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
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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/src/conf/domain_conf.c b/src/conf/domain_conf.c index 011de71..1040b40 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c [...] @@ -5410,11 +5410,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,
Pre-existing, but shouldn't this be VIR_ERR_XML_ERROR ? Otherwise the patch looks ok, but is missing documentation. And in this case it is important to have it there, I think. The possibility of specifying startupPolicy should be also documented and it should be mentioned there that the whole disk will be dropped in case this attribute is specified and the disk (or any other in it's backing chain) is not found. I'm trying to stress this out because for cdrom and floppy disks we can safely drop the source only, it can be plugged in even with old QEMU, OSes are used to that. But with disks it changes what is seen from the guest. I believe that's also why startupPolicy can be specified only for cdrom, floppy and USB hostdevs for now. This particular functionality calls for potentional problems IMHO in case it is not properly documented. Also, 'requisite' for disks would mean that you have to hot-unplug the disk if it needs to be removed due to migration because otherwise qemu wouldn't start on the destination, or would it? The more I'm staring into the code, the more I feel like this is not a good idea and should be managed from upper application if this behavior is required for disks. Martin
On 07/15/2013 09:31 PM, Martin Kletzander wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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/src/conf/domain_conf.c b/src/conf/domain_conf.c index 011de71..1040b40 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c [...] @@ -5410,11 +5410,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, Pre-existing, but shouldn't this be VIR_ERR_XML_ERROR ?
Otherwise the patch looks ok, but is missing documentation. And in this case it is important to have it there, I think.
The possibility of specifying startupPolicy should be also documented and it should be mentioned there that the whole disk will be dropped in case this attribute is specified and the disk (or any other in it's backing chain) is not found. I'm trying to stress this out because for cdrom and floppy disks we can safely drop the source only, it can be plugged in even with old QEMU, OSes are used to that. But with disks it changes what is seen from the guest. I believe that's also why startupPolicy can be specified only for cdrom, floppy and USB hostdevs for now. This particular functionality calls for potentional problems IMHO in case it is not properly documented.
Also, 'requisite' for disks would mean that you have to hot-unplug the disk if it needs to be removed due to migration because otherwise qemu wouldn't start on the destination, or would it?
Yeah, the hot-unplugging disk is necessary in the case of 'requisite' if diskchains checking failed during migration.
The more I'm staring into the code, the more I feel like this is not a good idea and should be managed from upper application if this behavior is required for disks.
I am not sure it is upper application's work. libvirt collected diskchain data for each disk at guest bootup already so it is easier to remove or hot-unplug a disk if backingchain is broken from libvirt point of view. upper application know nothing about backingchain for guest disks.
On 07/15/2013 05:01 PM, Guannan Ren wrote:
On 07/15/2013 09:31 PM, Martin Kletzander wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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/src/conf/domain_conf.c b/src/conf/domain_conf.c index 011de71..1040b40 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c [...] @@ -5410,11 +5410,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, Pre-existing, but shouldn't this be VIR_ERR_XML_ERROR ?
Otherwise the patch looks ok, but is missing documentation. And in this case it is important to have it there, I think.
The possibility of specifying startupPolicy should be also documented and it should be mentioned there that the whole disk will be dropped in case this attribute is specified and the disk (or any other in it's backing chain) is not found. I'm trying to stress this out because for cdrom and floppy disks we can safely drop the source only, it can be plugged in even with old QEMU, OSes are used to that. But with disks it changes what is seen from the guest. I believe that's also why startupPolicy can be specified only for cdrom, floppy and USB hostdevs for now. This particular functionality calls for potentional problems IMHO in case it is not properly documented.
Also, 'requisite' for disks would mean that you have to hot-unplug the disk if it needs to be removed due to migration because otherwise qemu wouldn't start on the destination, or would it?
Yeah, the hot-unplugging disk is necessary in the case of 'requisite' if diskchains checking failed during migration.
The more I'm staring into the code, the more I feel like this is not a good idea and should be managed from upper application if this behavior is required for disks.
I am not sure it is upper application's work. libvirt collected diskchain data for each disk at guest bootup already so it is easier to remove or hot-unplug a disk if backingchain is broken from libvirt point of view. upper application know nothing about backingchain for guest disks.
I haven't explained myself correctly here. In the last paragraph I was talking about the whole 'startupPolicy for harddisk' approach, not only about removing the disk on migration. Martin
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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -580,6 +580,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); @@ -870,14 +877,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); @@ -1062,6 +1065,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
s/doesn't/don't/ in $SUBJ On 07/02/2013 11:35 AM, Guannan Ren wrote:
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
s/exist/exists/; s/the/The/
arguments don't have so much meannings for F_OK, so give 0 for them.
s/meannings/meaning/
--- 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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -580,6 +580,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; + } +
Pre-existing, but this nad other errors will be shadowed by the error in qemuDomainBlockCommit. The existence of the file is already checked by:
if (!(*canonical = canonicalize_file_name(combined))) {
this ^^ line, so no need to call virFileAccessibleAs().
virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); @@ -870,14 +877,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;
And then you report one more error here, even though the function may fail because of something else. I'd guess that's why it was a warning. No idea why it skipped the errors, though. The errors should be fixed, but this patch just adds more confusion to the error reporting. Martin
On 07/15/2013 09:31 PM, Martin Kletzander wrote:
s/doesn't/don't/ in $SUBJ
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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 s/exist/exists/; s/the/The/
arguments don't have so much meannings for F_OK, so give 0 for them. s/meannings/meaning/
--- 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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -580,6 +580,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; + } + Pre-existing, but this nad other errors will be shadowed by the error in qemuDomainBlockCommit. The existence of the file is already checked by:
if (!(*canonical = canonicalize_file_name(combined))) {
this ^^ line, so no need to call virFileAccessibleAs().
Calling virFileAccessibleAs() here is for more clear error message.
virReportSystemError(errno, _("Can't canonicalize path '%s'"), path); @@ -870,14 +877,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;
And then you report one more error here, even though the function may fail because of something else. I'd guess that's why it was a warning. No idea why it skipped the errors, though.
The errors should be fixed, but this patch just adds more confusion to the error reporting.
no, VIR_ERROR just adds one more error log item, the actual raised error is still there not being shadowed. Guannan
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c
@@ -870,14 +877,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);
This change means you won't be able to start the pool if one of the files is missing a backing file. I've forwarded a patch [1] from/for [2] that ignores missing files on pool start and there is a bug [3] requesting that we ignore other files as well. I feel like this is going in the other direction. Wouldn't it be enough to check for them on domain start-up and leave the pool running even if one of those volumes doesn't have an existing backing file? Jan [1] https://www.redhat.com/archives/libvir-list/2013-July/msg00635.html [2] https://bugzilla.redhat.com/show_bug.cgi?id=977706 [3] https://bugzilla.redhat.com/show_bug.cgi?id=710866
On 07/15/2013 05:31 PM, Ján Tomko wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c
@@ -870,14 +877,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);
This change means you won't be able to start the pool if one of the files is missing a backing file. I've forwarded a patch [1] from/for [2] that ignores missing files on pool start and there is a bug [3] requesting that we ignore other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool running even if one of those volumes doesn't have an existing backing file?
How about making it configurable for the pool? There are definitely some users who want the pool to reflect actual info after pool-refresh.
Jan
[1] https://www.redhat.com/archives/libvir-list/2013-July/msg00635.html [2] https://bugzilla.redhat.com/show_bug.cgi?id=977706 [3] https://bugzilla.redhat.com/show_bug.cgi?id=710866
On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
On 07/15/2013 05:31 PM, Ján Tomko wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c
@@ -870,14 +877,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);
This change means you won't be able to start the pool if one of the files is missing a backing file. I've forwarded a patch [1] from/for [2] that ignores missing files on pool start and there is a bug [3] requesting that we ignore other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool running even if one of those volumes doesn't have an existing backing file?
How about making it configurable for the pool? There are definitely some users who want the pool to reflect actual info after pool-refresh.
I don't think this needs to be configurable. The pool should show *every* single file, regardless of whether the file has a broken backing file. We shouldn't be trying to second guess what the user wants to do with a image with broken backing file. Just expose as much info as we have and let them deal with the problem. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/16/2013 10:40 AM, Daniel P. Berrange wrote:
On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
On 07/15/2013 05:31 PM, Ján Tomko wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c
@@ -870,14 +877,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);
This change means you won't be able to start the pool if one of the files is missing a backing file. I've forwarded a patch [1] from/for [2] that ignores missing files on pool start and there is a bug [3] requesting that we ignore other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool running even if one of those volumes doesn't have an existing backing file?
How about making it configurable for the pool? There are definitely some users who want the pool to reflect actual info after pool-refresh.
I don't think this needs to be configurable. The pool should show *every* single file, regardless of whether the file has a broken backing file. We shouldn't be trying to second guess what the user wants to do with a image with broken backing file. Just expose as much info as we have and let them deal with the problem.
I was thinking about the case that was mentioned in Jan's link to bugzilla, where they wanted to keep even deleted volumes. Since I disagree with that for normal pool, we could make it configurable for such use-cases (although it looks more like invalid usage of our pools in that BZ). Martin
On Tue, Jul 16, 2013 at 10:47:54AM +0200, Martin Kletzander wrote:
On 07/16/2013 10:40 AM, Daniel P. Berrange wrote:
On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
On 07/15/2013 05:31 PM, Ján Tomko wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c
@@ -870,14 +877,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);
This change means you won't be able to start the pool if one of the files is missing a backing file. I've forwarded a patch [1] from/for [2] that ignores missing files on pool start and there is a bug [3] requesting that we ignore other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool running even if one of those volumes doesn't have an existing backing file?
How about making it configurable for the pool? There are definitely some users who want the pool to reflect actual info after pool-refresh.
I don't think this needs to be configurable. The pool should show *every* single file, regardless of whether the file has a broken backing file. We shouldn't be trying to second guess what the user wants to do with a image with broken backing file. Just expose as much info as we have and let them deal with the problem.
I was thinking about the case that was mentioned in Jan's link to bugzilla, where they wanted to keep even deleted volumes. Since I disagree with that for normal pool, we could make it configurable for such use-cases (although it looks more like invalid usage of our pools in that BZ).
Which BZ are you referring to ? I read BZ 977706 to be about not causing errors during pool-refresh if a 2nd node has deleted the volume while the first node is in the middle of refresh. This isn't about keeping deleted volumes visible. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/16/2013 11:08 AM, Daniel P. Berrange wrote: [...]
Which BZ are you referring to ? I read BZ 977706 to be about not causing errors during pool-refresh if a 2nd node has deleted the volume while the first node is in the middle of refresh. This isn't about keeping deleted volumes visible.
Misread the BZ, the problem is the pool gets delete, not the volume, sorry. Martin
On 07/16/2013 04:40 PM, Daniel P. Berrange wrote:
On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
On 07/15/2013 05:31 PM, Ján Tomko wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -870,14 +877,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); This change means you won't be able to start the pool if one of the files is missing a backing file. I've forwarded a patch [1] from/for [2] that ignores missing files on pool start and there is a bug [3] requesting that we ignore other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool running even if one of those volumes doesn't have an existing backing file?
How about making it configurable for the pool? There are definitely some users who want the pool to reflect actual info after pool-refresh. I don't think this needs to be configurable. The pool should show *every* single file, regardless of whether the file has a broken backing file. We shouldn't be trying to second guess what the user wants to do with a image with broken backing file. Just expose as much info as we have and let them deal with the problem.
Daniel, How about for guest cold bootup in this situation where one of its disks has a broken file we should throw an error out or give a warn in the log as what it is doing currently.
On Wed, Jul 17, 2013 at 10:08:55AM +0800, Guannan Ren wrote:
On 07/16/2013 04:40 PM, Daniel P. Berrange wrote:
On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
On 07/15/2013 05:31 PM, Ján Tomko wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -870,14 +877,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); This change means you won't be able to start the pool if one of the files is missing a backing file. I've forwarded a patch [1] from/for [2] that ignores missing files on pool start and there is a bug [3] requesting that we ignore other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool running even if one of those volumes doesn't have an existing backing file?
How about making it configurable for the pool? There are definitely some users who want the pool to reflect actual info after pool-refresh. I don't think this needs to be configurable. The pool should show *every* single file, regardless of whether the file has a broken backing file. We shouldn't be trying to second guess what the user wants to do with a image with broken backing file. Just expose as much info as we have and let them deal with the problem.
Daniel,
How about for guest cold bootup in this situation where one of its disks has a broken file we should throw an error out or give a warn in the log as what it is doing currently.
If we can't access the file on guest boot, then we need to raise errors, since it wouldn't be possible to set labelling or permissions. Of course we have to make sure we don't give bogus errors in the case of root squash NFS too. We need to consider storage pool enumeration separately from guest bootup though. Both have specific semantics and we shouldn't force them to behave the same way - the API needs to serve both their needs. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/17/2013 05:41 PM, Daniel P. Berrange wrote:
On Wed, Jul 17, 2013 at 10:08:55AM +0800, Guannan Ren wrote:
On 07/16/2013 04:40 PM, Daniel P. Berrange wrote:
On Tue, Jul 16, 2013 at 08:14:54AM +0200, Martin Kletzander wrote:
On 07/15/2013 05:31 PM, Ján Tomko wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -870,14 +877,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); This change means you won't be able to start the pool if one of the files is missing a backing file. I've forwarded a patch [1] from/for [2] that ignores missing files on pool start and there is a bug [3] requesting that we ignore other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool running even if one of those volumes doesn't have an existing backing file?
How about making it configurable for the pool? There are definitely some users who want the pool to reflect actual info after pool-refresh. I don't think this needs to be configurable. The pool should show *every* single file, regardless of whether the file has a broken backing file. We shouldn't be trying to second guess what the user wants to do with a image with broken backing file. Just expose as much info as we have and let them deal with the problem.
Daniel,
How about for guest cold bootup in this situation where one of its disks has a broken file we should throw an error out or give a warn in the log as what it is doing currently. If we can't access the file on guest boot, then we need to raise errors, since it wouldn't be possible to set labelling or permissions. Of course we have to make sure we don't give bogus errors in the case of root squash NFS too. We need to consider storage pool enumeration separately from guest bootup though. Both have specific semantics and we shouldn't force them to behave the same way - the API needs to serve both their needs.
Hi Daniel, About the root squash NFS issue, currently, we use access(disk_path, F_OK) to check each disk file in a diskchain. It is okay to boot up guest for disk whose backing files are located in NFS shared folder with root_squash. The disk chain will not be reported as broken in my test. It seems that qemu needs only the read permission for disk backing files and all changes on disk in furture will write to current disk file itself. So I think in this case libvirt doesn't need to do checking on NFS shared folder. Do you think it is right? Guannan
On 07/15/2013 11:31 PM, Ján Tomko wrote:
On 07/02/2013 11:35 AM, Guannan Ren wrote:
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 27aa4fe..cb61e5b 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -870,14 +877,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); This change means you won't be able to start the pool if one of the files is missing a backing file. I've forwarded a patch [1] from/for [2] that ignores missing files on pool start and there is a bug [3] requesting that we ignore other files as well. I feel like this is going in the other direction.
Wouldn't it be enough to check for them on domain start-up and leave the pool running even if one of those volumes doesn't have an existing backing file?
Jan
[1] https://www.redhat.com/archives/libvir-list/2013-July/msg00635.html [2] https://bugzilla.redhat.com/show_bug.cgi?id=977706 [3] https://bugzilla.redhat.com/show_bug.cgi?id=710866
As what you pointed out is close to my issue, I chose to propose a patch on bug 977706 first https://www.redhat.com/archives/libvir-list/2013-July/msg01026.html. Guannan
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 b791125..5459eb4 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4699,6 +4699,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 8d79066..d54d477 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2006,75 +2006,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 ac5ffcf..4672f7f 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3543,17 +3543,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 0d6defc..577db8e 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
On 07/02/2013 05:35 PM, Guannan Ren wrote:
v3: https://www.redhat.com/archives/libvir-list/2013-June/thread.html
v3 to v4: Rebase
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.
Are we going to support this feature. I think this is a good feature for two reasons: 1, currently, we only check the disk presence for floppy and CDROM device which is not enough. For disks with backing files, if one of its backing file is missing, libvirt doesn't check anything, the qemu will throw an unclear error message as follows: A guest with a disk image /var/lib/libvirt/images/snapshot/snap2.qcow # qemu-img info --backing-chain /var/lib/libvirt/images/snapshot/snap2.qcow image: /var/lib/libvirt/images/snapshot/snap2.qcow file format: qcow2 virtual size: 10G (10737418240 bytes) disk size: 19M cluster_size: 65536 backing file: /var/lib/libvirt/images/snapshot/snap1.qcow backing file format: qcow2 image: /var/lib/libvirt/images/snapshot/snap1.qcow file format: qcow2 virtual size: 10G (10737418240 bytes) disk size: 196K cluster_size: 65536 backing file: /var/lib/libvirt/images/fedora18.img backing file format: raw if we change snap1.qcow to *snap1.qcow.back* , then bootup the guest The qemu will throw an error: qemu-system-x86_64: -drive file=/var/lib/libvirt/images/snapshot/snap2.qcow,if=none,id=drive-virtio-disk0,format=qcow2: could not open disk image /var/lib/libvirt/images/snapshot/snap2.qcow: No such file or directory Actually, snap2.qcow is there, one of its backing file snap1.qcow is missing. So we need to check the presence of all files in disk chains. With this patch, libvirt will throw an error: error: Backing file '/var/lib/libvirt/images/snapshot/snap1.qcow' does not exist: No such file or directory 2, Adding 'startupPolicy' attribute for guest disk is useful for guests which use the disk from storage-centric production environment. Guest uses the the block disk assigned by Fibre channel or FCoE or ISCSI storage. The storage administrator can adjust the disk assignment between guests, if guest disk with startupPolicy value of optional, guest still can boot up after its assigned disks are missing. So I think it is good to add this flexibility to disk configuration for guests. Any thoughts? Guannan
participants (4)
-
Daniel P. Berrange -
Guannan Ren -
Ján Tomko -
Martin Kletzander