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

[Peter Krempa]

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.

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?

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170331/466af3e5/attachment-0001.sig>