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

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(a)redhat.com&gt; > --- > 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.

> + 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.