[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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 */
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).


Thanks for this review.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]