On 07/31/2013 09:51 AM, Guannan Ren wrote:

Add startupPolicy attribute policy for harddisk with type "file",

s/policy//

"block" and "dir". 'requisite' is not supported currently for hardisk.

s/hardisk/harddisk/

--- docs/formatdomain.html.in | 8 ++++++-- docs/schemas/domaincommon.rng | 6 ++++++ src/conf/domain_conf.c | 31 +++++++++++++++++++++++-------- 3 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 78e132e..cbfc773 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1620,7 +1620,8 @@ policy what to do with the disk if the source file is not accessible. (NB, <code>startupPolicy</code> is not valid for "volume" disk unless the specified storage volume is of "file" type). This is done by the - <code>startupPolicy</code> attribute, accepting these values: + <code>startupPolicy</code> attribute(<span class="since">Since 0.9.7</span>),

Add a space before the opening bracket.

+ accepting these values: <table class="top_table"> <tr> <td> mandatory </td> @@ -1636,7 +1637,10 @@ <td> drop if missing at any start attempt </td> </tr> </table> - <span class="since">Since 0.9.7</span> + <span class="since">Since 1.1.2</span> the <code>startupPolicy</code> is extended + to support hard disks besides cdrom and floppy. On guest cold bootup, if a certain disk + is not accessible or its disk chain is broken, with startupPolicy 'optional' the guest + will drop this disk. This feature doesn't support migration currently.

I'm worried that in case there is the possibility to (un)plug the disks during migration appears in the future, and we make a use of that, we'd have to stick with the same behavior for 'optional' policy which is described here in order not to be backwards compatible. It may be confusing to some users as well. What is your (and others) opinion on adding another startupPolicy called for example 'drop_on_boot' (or something more meaningful and appropriate) which would mean exactly the behavior explained here? I must say that I'm complete OK with this approach in case we're sure that we will never have the possibility to have automatically (un)pluggable disks during migration. And I personally, think, that this won't happen on qemu/libvirt level. It would be nice if others expressed their opinion on this as well.

</dd> <dt><code>mirror</code></dt> <dd> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 745b959..c6ee01c 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1124,6 +1124,9 @@ <ref name="absFilePath"/> </attribute> <optional> + <ref name="startupPolicy"/> + </optional> + <optional> <ref name='devSeclabel'/> </optional> </element> @@ -1141,6 +1144,9 @@ <attribute name="dir"> <ref name="absFilePath"/> </attribute> + <optional> + <ref name="startupPolicy"/> + </optional> <empty/> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a86be8c..e764492 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4795,7 +4795,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, switch (def->type) { case VIR_DOMAIN_DISK_TYPE_FILE: source = virXMLPropString(cur, "file"); - startupPolicy = virXMLPropString(cur, "startupPolicy"); break; case VIR_DOMAIN_DISK_TYPE_BLOCK: source = virXMLPropString(cur, "dev"); @@ -4882,7 +4881,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, case VIR_DOMAIN_DISK_TYPE_VOLUME: if (virDomainDiskSourcePoolDefParse(cur, def) < 0) goto error; - startupPolicy = virXMLPropString(cur, "startupPolicy"); break; default: virReportError(VIR_ERR_INTERNAL_ERROR, @@ -4891,6 +4889,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; }

+ startupPolicy = virXMLPropString(cur, "startupPolicy"); + /* People sometimes pass a bogus '' source path when they mean to omit the source element completely (e.g. CDROM without media). This is @@ -5463,14 +5463,22 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; }

- if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && - def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY) { - virReportError(VIR_ERR_INVALID_ARG, - _("Setting disk %s is allowed only for " - "cdrom or floppy"), + if (def->type == VIR_DOMAIN_DISK_TYPE_NETWORK) { + virReportError(VIR_ERR_XML_ERROR, + _("Setting disk %s is not allowed for " + "disk of network type"), startupPolicy); goto error; } + + if (def->device != VIR_DOMAIN_DISK_DEVICE_CDROM && + def->device != VIR_DOMAIN_DISK_DEVICE_FLOPPY && + val == VIR_DOMAIN_STARTUP_POLICY_REQUISITE) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Setting disk 'requisite' only for "

You deleted "is allowed" from the message.

+ "cdrom or floppy")); + goto error; + } def->startupPolicy = val; }

@@ -14125,6 +14133,9 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, case VIR_DOMAIN_DISK_TYPE_BLOCK: virBufferEscapeString(buf, " <source dev='%s'", def->src); + if (def->startupPolicy) + virBufferEscapeString(buf, " startupPolicy='%s'", + startupPolicy); if (def->nseclabels) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 8); @@ -14137,8 +14148,12 @@ virDomainDiskSourceDefFormat(virBufferPtr buf, } break; case VIR_DOMAIN_DISK_TYPE_DIR: - virBufferEscapeString(buf, " <source dir='%s'/>\n", + virBufferEscapeString(buf, " <source dir='%s'", def->src); + if (def->startupPolicy) + virBufferEscapeString(buf, " startupPolicy='%s'", + startupPolicy); + virBufferAddLit(buf, "/>\n"); break; case VIR_DOMAIN_DISK_TYPE_NETWORK: virBufferAsprintf(buf, " <source protocol='%s'",

Not that it would happen, but after this patch and before the folowing one, the code is inconsistent, the next patch should be squashed in this one. ACK with the nits fixed and the next one squashed in. Martin

On 07/31/2013 09:51 AM, Guannan Ren wrote:

Go through disks of guest, if one disk doesn't exist or its backing chain is broken, with 'optional' startupPolicy, for CDROM and Floppy we only discard its source path definition in xml, for disks we drop it from disk list and free it. --- include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_domain.c | 77 +++++++++++++++++++++++++++++++++++--------- 2 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index c0eb25b..3888ba5 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4728,6 +4728,7 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, */ typedef enum { VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0, /* oldSrcPath is set */ + VIR_DOMAIN_EVENT_HARDDISK_DROP_MISSING_ON_START = 1,

s/HARDDISK/DISK/ to make it consistent

#ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_DISK_CHANGE_LAST diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1ff802c..5da6e18 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2027,13 +2027,59 @@ cleanup: }

static int +qemuDomainCheckRemoveOptionalDisk(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + size_t *nextdisk) +{ + char uuid[VIR_UUID_STRING_BUFLEN]; + virDomainEventPtr event = NULL; + virDomainDiskDefPtr del_disk = NULL; + + virUUIDFormat(vm->def->uuid, uuid); + + VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " + "due to inaccessible source '%s'", + disk->dst, vm->def->name, uuid, disk->src); + + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || + disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + + event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, + disk->info.alias, + VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START); + /* For cdrom or floppy, we only remove its source definition + * So the nextdisk need to point to next disk. + */ + (*nextdisk)++; + VIR_FREE(disk->src); + } else { + event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, + disk->info.alias, + VIR_DOMAIN_EVENT_HARDDISK_DROP_MISSING_ON_START); + + if (!(del_disk = virDomainDiskRemoveByName(vm->def, disk->src))) { + virReportError(VIR_ERR_INVALID_ARG, + _("no source device %s"), disk->src); + return -1; + } + virDomainDiskDefFree(del_disk); + } + + if (event) + qemuDomainEventQueue(driver, event); + + return 0; +} + +static int qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, - bool cold_boot) + bool cold_boot, + size_t *nextdisk) { char uuid[VIR_UUID_STRING_BUFLEN]; - virDomainEventPtr event = NULL; int startupPolicy = disk->startupPolicy;

virUUIDFormat(vm->def->uuid, uuid); @@ -2057,16 +2103,8 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver, }

virResetLastError();

This should really be one function up, especially when I requested it as a condition for ACK.

- VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " - "due to inaccessible source '%s'", - disk->dst, vm->def->name, uuid, disk->src); - - event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, disk->info.alias, - VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START); - if (event) - qemuDomainEventQueue(driver, event); - - VIR_FREE(disk->src); + if (qemuDomainCheckRemoveOptionalDisk(driver, vm, disk, nextdisk) < 0) + goto error;

return 0;

@@ -2082,21 +2120,28 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, int ret = -1; size_t i; virDomainDiskDefPtr disk; + size_t count = vm->def->ndisks; + size_t nextdisk = 0; +

Definitely drop all this nextdisk nonsense, you can clean it up with: ssize_t i; for(i = ndisks - 1; i >= 0; i--) ... then you don't have to pass it to any underlying function, or you can pass 'i' and call virDomainDiskRemove() instead of RemoveByName. looking forward to v2 (or v6), Martin

On 08/02/2013 08:37 AM, Guannan Ren wrote:

Go through disks of guest, if one disk doesn't exist or its backing chain is broken, with 'optional' startupPolicy, for CDROM and Floppy we only discard its source path definition in xml, for disks we drop it from disk list and free it. --- include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_domain.c | 69 ++++++++++++++++++++++++++++++++------------ 2 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7bd3559..52ac95d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4727,6 +4727,7 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, */ typedef enum { VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0, /* oldSrcPath is set */ + VIR_DOMAIN_EVENT_DISK_DROP_MISSING_ON_START = 1,

#ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_DISK_CHANGE_LAST diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1ff802c..6794e26 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c [...] @@ -2080,11 +2111,12 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, bool cold_boot) { int ret = -1; - size_t i; + ssize_t i; virDomainDiskDefPtr disk; + size_t ndisks = vm->def->ndisks;

VIR_DEBUG("Checking for disk presence"); - for (i = 0; i < vm->def->ndisks; i++) { + for (i = ndisks - 1; i >= 0; i--) { disk = vm->def->disks[i];

Now I noticed that there should be either (1) a check for domain without any disks (if ndisks == 0; return 0) or (2) a different for loop, like this for example: for (i = vm->def->ndisks; i > 0; i--) { disk = vm->def->disks[i - 1]; I know that I proposed the faulty version, sorry for that. From those 2 versions I like the second one better because it looks cleaner and you can skip first lines of this hunk. It'd be nice to have a test for this to make sure we won't break it in future. At least xml2xml test if we can't test the disk dropping now. ACK with that fixed and this patch squashed in the previous one (also with those mentioned things fixed). Martin

On 08/06/2013 09:40 PM, Martin Kletzander wrote:

On 08/02/2013 08:37 AM, Guannan Ren wrote:

...

Go through disks of guest, if one disk doesn't exist or its backing chain is broken, with 'optional' startupPolicy, for CDROM and Floppy we only discard its source path definition in xml, for disks we drop it from disk list and free it. --- include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_domain.c | 69 ++++++++++++++++++++++++++++++++------------ 2 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7bd3559..52ac95d 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4727,6 +4727,7 @@ typedef void (*virConnectDomainEventBlockJobCallback)(virConnectPtr conn, */ typedef enum { VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START = 0, /* oldSrcPath is set */ + VIR_DOMAIN_EVENT_DISK_DROP_MISSING_ON_START = 1,

#ifdef VIR_ENUM_SENTINELS VIR_DOMAIN_EVENT_DISK_CHANGE_LAST diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1ff802c..6794e26 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c [...] @@ -2080,11 +2111,12 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, bool cold_boot) { int ret = -1; - size_t i; + ssize_t i; virDomainDiskDefPtr disk; + size_t ndisks = vm->def->ndisks;

VIR_DEBUG("Checking for disk presence"); - for (i = 0; i < vm->def->ndisks; i++) { + for (i = ndisks - 1; i >= 0; i--) { disk = vm->def->disks[i];

Now I noticed that there should be either (1) a check for domain without any disks (if ndisks == 0; return 0) or (2) a different for loop, like this for example:

for (i = vm->def->ndisks; i > 0; i--) { disk = vm->def->disks[i - 1];

I know that I proposed the faulty version, sorry for that. From those 2 versions I like the second one better because it looks cleaner and you can skip first lines of this hunk.

It'd be nice to have a test for this to make sure we won't break it in future. At least xml2xml test if we can't test the disk dropping now.

ACK with that fixed and this patch squashed in the previous one (also with those mentioned things fixed).

Martin

pushed with these mentioned fixed. Guannan