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

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(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(-)

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