[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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) {
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.

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]