Re: [libvirt] [PATCH 2/2] Introduce and use virDomainDiskZeroSource
On 03/31/2017 03:23 PM, Peter Krempa wrote:
On Fri, Mar 31, 2017 at 15:03:35 +0200, Michal Privoznik wrote:
> 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>
>>> ---
>>> 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?
Well, yes. As said it's the safest option. E.g. the following XML which
would be a result of the code above does not pass schema validation:
<disk type='network' device='cdrom'>
<driver name='qemu' type='raw'/>
<target dev='hda' bus='ide'/>
<readonly/>
<address type='drive' controller='0' bus='0'
target='0' unit='0'/>
</disk>
Is it fault of the schema being broken? Perhaps yes. I didn't really
think about it.
Yes, it's the schema which is broken. Our schema should accept that any
disk with device='cdrom' can lack <source/> element regardless of its
type. The same goes for floppy disks.
Also. Since the disk is empty the source really does not mater. The need
to keep is a historical artifact which will require a lot of
refactoring.
> 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.
I'm not sure whether this approach is correct. You are saying we should
introduce regressions?
No. If they are broken now, that is not what I call a regression.
The problem here is that while libvirt models the "type" as being part
of the disk, it in fact is part of the source itself. The qemu driver
will probably be mostly prepared for this, but I'd rather not risk it.
Certaily not during the freeze, such shenanigans are certainly NOT safe
for the freeze.
Fair enough. I don't care that much about this. I care more about fixing
the bug and getting the fix in for the release. If adding the one line
is a requisite I can live with that. Will post v2 soon then.
BTW: just to be clear - you're okay with 1/2 being pushed as is, right?
Michal