Re: [libvirt] [PATCH v6 2/2] qemu: support to drop disk with 'optional' startupPolicy

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_SENTINELS > VIR_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'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. ACK with that fixed and this patch squashed in the previous one (also with those mentioned things fixed). Martin