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

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?

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.