Re: [libvirt] [PATCH v4 1/4] conf: add startupPolicy attribute for harddisk

On 07/15/2013 09:31 PM, Martin Kletzander wrote: > On 07/02/2013 11:35 AM, Guannan Ren wrote: >> Add startupPolicy attribute policy for harddisk with type "file", >> "block" and "dir". The "network" type disk is still not supported. >> --- >> docs/schemas/domaincommon.rng | 6 ++++++ >> src/conf/domain_conf.c | 20 +++++++++++++------- >> 2 files changed, 19 insertions(+), 7 deletions(-) >> > [...] >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index 011de71..1040b40 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c > [...] >> @@ -5410,11 +5410,10 @@ >> virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, >> goto error; >> } >> - if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && >> - def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { >> + if (def->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { >> virReportError(VIR_ERR_INVALID_ARG, > Pre-existing, but shouldn't this be VIR_ERR_XML_ERROR ? > > Otherwise the patch looks ok, but is missing documentation. And in this > case it is important to have it there, I think. > > The possibility of specifying startupPolicy should be also documented > and it should be mentioned there that the whole disk will be dropped in > case this attribute is specified and the disk (or any other in it's > backing chain) is not found. I'm trying to stress this out because for > cdrom and floppy disks we can safely drop the source only, it can be > plugged in even with old QEMU, OSes are used to that. But with disks it > changes what is seen from the guest. I believe that's also why > startupPolicy can be specified only for cdrom, floppy and USB hostdevs > for now. This particular functionality calls for potentional problems > IMHO in case it is not properly documented. > > Also, 'requisite' for disks would mean that you have to hot-unplug the > disk if it needs to be removed due to migration because otherwise qemu > wouldn't start on the destination, or would it? Yeah, the hot-unplugging disk is necessary in the case of 'requisite' if diskchains checking failed during migration. > > The more I'm staring into the code, the more I feel like this is not a > good idea and should be managed from upper application if this behavior > is required for disks. I am not sure it is upper application's work. libvirt collected diskchain data for each disk at guest bootup already so it is easier to remove or hot-unplug a disk if backingchain is broken from libvirt point of view. upper application know nothing about backingchain for guest disks.