[libvirt] [PATCH 2/2] Introduce and use virDomainDiskZeroSource

[Martin Kletzander]

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 at redhat.com>
>---
> 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 at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170331/3b128664/attachment-0001.sig>