Re: [PATCH 2/2] Introduce and use virDomainDiskZeroSource

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

 



On 03/31/2017 02:18 PM, Peter Krempa wrote:
On Fri, Mar 31, 2017 at 13:13:51 +0200, Michal Privoznik wrote:
Currently, if we want to zero out disk source (e,g, due to
startupPolicy when starting up a domain) we use
virDomainDiskSetSource(disk, NULL). This works well for file
based storage (storage type file, dir, or block). But it doesn't
work at all for other types like volume and network.

So imagine that you have a domain that has a CDROM configured
which source is a volume from an inactive pool. Because it is
startupPolicy='optional', the CDROM is empty when the domain
starts. However, the source element is not cleared out in the
status XML and thus when the daemon restarts and tries to
reconnect to the domain it refreshes the disks (which fails - the
storage pool is still not running) and thus the domain is killed.

Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
 src/conf/domain_conf.c   | 23 +++++++++++++++++++++++
 src/conf/domain_conf.h   |  1 +
 src/libvirt_private.syms |  1 +
 src/qemu/qemu_domain.c   |  2 +-
 src/qemu/qemu_process.c  |  2 +-
 src/vmx/vmx.c            |  6 +++---
 src/xenconfig/xen_xm.c   |  2 +-
 7 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 01553b5..a60a456 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -1719,6 +1719,29 @@ virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src)
 }


+void
+virDomainDiskZeroSource(virDomainDiskDefPtr def)

virDomainDiskEmptySource perhaps?

+{
+    switch ((virStorageType) def->src->type) {
+    case VIR_STORAGE_TYPE_DIR:
+    case VIR_STORAGE_TYPE_FILE:
+    case VIR_STORAGE_TYPE_BLOCK:
+        VIR_FREE(def->src->path);
+        break;
+    case VIR_STORAGE_TYPE_NETWORK:
+        VIR_FREE(def->src->volume);

This certainly is not enough to clear network disks. You also need to
clear the hosts and the path component. Resetting the protocol also
would be desired.

Fair enough.


+        break;
+    case VIR_STORAGE_TYPE_VOLUME:
+        virStorageSourcePoolDefFree(def->src->srcpool);
+        def->src->srcpool = NULL;
+        break;
+    case VIR_STORAGE_TYPE_NONE:
+    case VIR_STORAGE_TYPE_LAST:
+        break;
+    }

You also should reset the disk type. I'm not sure though wether setting
it to "NONE" is a good idea. "FILE" might be safer.

This doesn't sound fair. If the original disk was type of volume, why it should all of a sudden be reported as type of file? If we are afraid of other areas of the code failing or being unprepared for, I say have them fail so that they can be fixed.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux