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

[Michal Privoznik]

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 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(-)
>
> [...]
>
>>>> +        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