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

Martin Kletzander <mkletzan@xxxxxxxxxx> · Fri, 31 Mar 2017 14:10:34 +0200

On Fri, Mar 31, 2017 at 01:13:51PM +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)
+{
+    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);
+        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;
+    }
+}
+
+

Won't this break on migration?  I feel like we're losing information
here.  Can't we just check the sourceOptional (or whatever the name is)
when reconnecting to such domains?

Anyway, that's a discussion we can have later on.  This patch is not
changing the behaviour, just fixing part of it.  So ACK to that, however
since I don't feel that very knowledgeable and confident in the XEN and
VMX areas.

But ...

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 31af2e9..5c833b8 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -2283,7 +2283,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
            virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);

            if (STRCASEEQ(fileName, "auto detect")) {
-                ignore_value(virDomainDiskSetSource(*def, NULL));
+                virDomainDiskZeroSource(*def);
                (*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL;
            } else if (virDomainDiskSetSource(*def, fileName) < 0) {
                goto cleanup;
@@ -2294,7 +2294,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
            virDomainDiskSetType(*def, VIR_STORAGE_TYPE_BLOCK);

            if (STRCASEEQ(fileName, "auto detect")) {
-                ignore_value(virDomainDiskSetSource(*def, NULL));
+                virDomainDiskZeroSource(*def);
                (*def)->startupPolicy = VIR_DOMAIN_STARTUP_POLICY_OPTIONAL;
            } else if (virDomainDiskSetSource(*def, fileName) < 0) {
                goto cleanup;
@@ -2326,7 +2326,7 @@ virVMXParseDisk(virVMXContext *ctx, virDomainXMLOptionPtr xmlopt, virConfPtr con
            }

            virDomainDiskSetType(*def, VIR_STORAGE_TYPE_FILE);
-            ignore_value(virDomainDiskSetSource(*def, NULL));
+            virDomainDiskZeroSource(*def);
        } else {
            virReportError(VIR_ERR_INTERNAL_ERROR,
                           _("Invalid or not yet handled value '%s' "

looking at the code, and it's even visible just from the context in
these hunks, you're only changing the behaviour for
VIR_STORAGE_TYPE_BLOCK in VMX (i.e. not changing the behaviour), ...

diff --git a/src/xenconfig/xen_xm.c b/src/xenconfig/xen_xm.c
index 8ef68bb..327cb65 100644
--- a/src/xenconfig/xen_xm.c
+++ b/src/xenconfig/xen_xm.c
@@ -140,7 +140,7 @@ xenParseXMDisk(virConfPtr conf, virDomainDefPtr def)

            if (offset == head) {
                /* No source file given, eg CDROM with no media */
-                ignore_value(virDomainDiskSetSource(disk, NULL));
+                virDomainDiskZeroSource(disk);
            } else {

... and here you clear the source of a disk object that was just created
and has no source set yet.  So this part of the condition can be removed.

I wanted to say that this could be cleaned even more, so for now just
change the 'else' to 'if (offset != head)', and that's ACK and safe for
freeze.

                if (VIR_STRNDUP(tmp, head, offset - head) < 0)
                    goto cleanup;
--
2.10.2

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

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



