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

[Martin Kletzander]

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.

Martin