On 08/06/2013 09:40 PM, Martin Kletzander wrote:
On 08/02/2013 08:37 AM, Guannan Ren wrote: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. --- include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_domain.c | 69 ++++++++++++++++++++++++++++++++------------ 2 files changed, 52 insertions(+), 18 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7bd3559..52ac95d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4727,6 +4727,7 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, */ typedef enum { VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0, /* oldSrcPath is set */ + VIR_DOMAIN_EVENT_DISK_DROP_MISSING_ON_START = 1,#ifdef VIR_ENUM_SENTINELSVIR_DOMAIN_EVENT_DISK_CHANGE_LAST diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1ff802c..6794e26 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c[...]@@ -2080,11 +2111,12 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, bool cold_boot) { int ret = -1; - size_t i; + ssize_t i; virDomainDiskDefPtr disk; + size_t ndisks = vm->def->ndisks;VIR_DEBUG("Checking for disk presence");- for (i = 0; i < vm->def->ndisks; i++) { + for (i = ndisks - 1; i >= 0; i--) { disk = vm->def->disks[i];Now I noticed that there should be either (1) a check for domain without any disks (if ndisks == 0; return 0) or (2) a different for loop, like this for example: for (i = vm->def->ndisks; i > 0; i--) { disk = vm->def->disks[i - 1]; I know that I proposed the faulty version, sorry for that. From those 2 versions I like the second one better because it looks cleaner and you can skip first lines of this hunk.
It's good you noticed it in time. my local testcase doesn't include the case of domain without disks.
I should add it.
It'd be nice to have a test for this to make sure we won't break it in future. At least xml2xml test if we can't test the disk dropping now.
Make sense.
ACK with that fixed and this patch squashed in the previous one (also with those mentioned things fixed). Martin
Thanks for this review. Guannan -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list