[libvirt] [PATCH v2 2/2] Introduce and use virDomainDiskEmptySource

[Michal Privoznik]

On 03/31/2017 04:17 PM, Peter Krempa wrote:
> On Fri, Mar 31, 2017 at 16:02:14 +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>
>> ---
>>
>> diff to v1:
>> - Free more struct entries
>> - Set disk type
>> - Call the function less frequently
>>
>>  src/conf/domain_conf.c   | 33 +++++++++++++++++++++++++++++++++
>>  src/conf/domain_conf.h   |  1 +
>>  src/libvirt_private.syms |  1 +
>>  src/qemu/qemu_domain.c   |  2 +-
>>  src/qemu/qemu_process.c  |  2 +-
>>  5 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 01553b5..f1f0b56 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -1719,6 +1719,39 @@ virDomainDiskSetSource(virDomainDiskDefPtr def, const char *src)
>>  }
>>
>>
>> +void
>> +virDomainDiskEmptySource(virDomainDiskDefPtr def)
>> +{
>> +    virStorageSourcePtr src = def->src;
>> +
>> +    switch ((virStorageType) src->type) {
>> +    case VIR_STORAGE_TYPE_DIR:
>> +    case VIR_STORAGE_TYPE_FILE:
>> +    case VIR_STORAGE_TYPE_BLOCK:
>> +        VIR_FREE(src->path);
>> +        break;
>> +    case VIR_STORAGE_TYPE_NETWORK:
>> +        VIR_FREE(src->path);
>> +        VIR_FREE(src->volume);
>> +        virStorageNetHostDefFree(src->nhosts, src->hosts);
>> +        src->nhosts = 0;
>> +        src->hosts = NULL;
>> +        src->protocol = VIR_STORAGE_NET_PROTOCOL_NONE;
>> +        break;
>> +    case VIR_STORAGE_TYPE_VOLUME:
>> +        virStorageSourcePoolDefFree(src->srcpool);
>> +        src->srcpool = NULL;
>> +        break;
>> +    case VIR_STORAGE_TYPE_NONE:
>> +    case VIR_STORAGE_TYPE_LAST:
>> +        break;
>> +    }
>> +
>> +    virStorageSourceBackingStoreClear(src);
>
> I think that instead of all the above you should just call
> virStorageSourceClear(). It also clears the format of the volume,
> seclabels, encryption data and all other stuff which is relevant only
> for the image itself.
>
>> +    src->type = VIR_STORAGE_TYPE_FILE;
>
> And then fix the type.
>
>> +}
>> +
>> +
>>  const char *
>>  virDomainDiskGetDriver(virDomainDiskDefPtr def)
>>  {
>
> ACK with the change above. I think this should be safe for freeze.

Yeah, I think too, but lets not push it. This is a very narrow use case 
so our users will not suffer. I'll push it after the release. But thank 
you anyway.

Michal