[libvirt] [PATCH 0/5]Add startupPolicy attribute support for hard disks
The set of patches is trying to add 'startupPolicy' attribute support to the source element of hard disks. Policy levels are using the mandatory, requisite, optional levels as originally documented. For the 'optional' policy, there is a little difference from CDROM and Floppy which only drop its source path, for disks, if missing, the checking function will drop their definitions, because qemu doesn't allow missing source path for hard disk. If guest is using per-device boot element for its devices, after dropping one or more bootable device, the boot order will not be contiguous, the way here I use is to reorder them to make them contiguous. In this way, I introduce two new bit-operating functions virBitmapNextLastSetBit: Search for the last set bit before certain position. virBitmapNextLastSetBit: Search for the last clear bit before certain position. Guannan Ren(5) [PATCH 1/5] conf: add startupPolicy attribute for harddisk [PATCH 2/5] util: add two functions to find last set or unset bit in [PATCH 3/5] qemu: move disk presence checking before disk chain [PATCH 4/5] qemu: drop disk definition if missing and reorder [PATCH 5/5] event: add hard disk dropping event reason enum docs/formatdomain.html.in | 9 +++++--- include/libvirt/libvirt.h.in | 1 + src/conf/domain_conf.c | 21 +++++++++++++------ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 2 ++ src/qemu/qemu_domain.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------- src/qemu/qemu_process.c | 7 ++++--- src/util/virbitmap.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virbitmap.h | 6 ++++++ tests/virbitmaptest.c | 51 +++++++++++++++++++++++++++++++++++++++++++++- 10 files changed, 276 insertions(+), 24 deletions(-)
Add startupPolicy attribute policy for harddisk with type "file", "block" and "dir". The "network" type disk is still not supported. --- docs/formatdomain.html.in | 9 ++++++--- src/conf/domain_conf.c | 19 +++++++++++++------ 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8a3c3b7..a32bdc3 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1450,8 +1450,8 @@ For a "file" disk type which represents a cdrom or floppy (the <code>device</code> attribute), it is possible to define policy what to do with the disk if the source file is not accessible. - This is done by the <code>startupPolicy</code> attribute, accepting - these values: + This is done by the <code>startupPolicy</code> attribute + (<span class="since">Since 0.9.7</span>), accepting these values: <table class="top_table"> <tr> <td> mandatory </td> @@ -1467,7 +1467,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.0.4</span>, the <code>startupPolicy</code> extends + to support hard disks besides cdrom and floppy. However, the disk of "network" + type is still not reached. For the guest which is using per-device <code>boot</code> + element, the boot devices will be reordered after dropping its bootable disks. </dd> <dt><code>mirror</code></dt> <dd> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3278e9c..177faaa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4044,7 +4044,6 @@ virDomainDiskDefParseXML(virCapsPtr caps, 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"); @@ -4137,6 +4136,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, 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 @@ -4674,11 +4675,10 @@ virDomainDiskDefParseXML(virCapsPtr caps, 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, - _("Setting disk %s is allowed only for " - "cdrom or floppy"), + _("Setting disk %s is not allowed for " + "disk of network type"), startupPolicy); goto error; } @@ -12838,6 +12838,9 @@ virDomainDiskDefFormat(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); @@ -12850,8 +12853,12 @@ virDomainDiskDefFormat(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'", -- 1.7.11.2
On 2013年03月18日 17:10, Guannan Ren wrote:
Add startupPolicy attribute policy for harddisk with type "file", "block" and "dir". The "network" type disk is still not supported. --- docs/formatdomain.html.in | 9 ++++++--- src/conf/domain_conf.c | 19 +++++++++++++------ 2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8a3c3b7..a32bdc3 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1450,8 +1450,8 @@ For a "file" disk type which represents a cdrom or floppy
[...]
(the<code>device</code> attribute), it is possible to define policy what to do with the disk if the source file is not accessible. - This is done by the<code>startupPolicy</code> attribute, accepting - these values: + This is done by the<code>startupPolicy</code> attribute + (<span class="since">Since 0.9.7</span>), accepting these values: <table class="top_table"> <tr> <td> mandatory</td> @@ -1467,7 +1467,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.0.4</span>, the<code>startupPolicy</code> extends
s/extends/is extended/,
+ to support hard disks besides cdrom and floppy. However, the disk of "network" + type is still not reached. For the guest which is using per-device<code>boot</code> + element, the boot devices will be reordered after dropping its bootable disks.
The paragraph starts with "For a 'file' disk type". But your patch extends the "startupPolicy" for all disk types except network. Which means mismatching here.
</dd> <dt><code>mirror</code></dt> <dd> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3278e9c..177faaa 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4044,7 +4044,6 @@ virDomainDiskDefParseXML(virCapsPtr caps, 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"); @@ -4137,6 +4136,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, 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 @@ -4674,11 +4675,10 @@ virDomainDiskDefParseXML(virCapsPtr caps, 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, - _("Setting disk %s is allowed only for " - "cdrom or floppy"), + _("Setting disk %s is not allowed for " + "disk of network type"), startupPolicy); goto error;
So you should change the schema, to allow the startupPolicy for disk of types except network.
} @@ -12838,6 +12838,9 @@ virDomainDiskDefFormat(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); @@ -12850,8 +12853,12 @@ virDomainDiskDefFormat(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'",
virBitmapNextLastSetBit: Search for the last set bit before certain position. virBitmapNextLastSetBit: Search for the last clear bit before certain position. --- src/libvirt_private.syms | 2 + src/util/virbitmap.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virbitmap.h | 6 +++ tests/virbitmaptest.c | 51 ++++++++++++++++++++++++- 4 files changed, 154 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5cad990..a17465a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1045,6 +1045,8 @@ virBitmapNew; virBitmapNewCopy; virBitmapNewData; virBitmapNextClearBit; +virBitmapNextLastClearBit; +virBitmapNextLastSetBit; virBitmapNextSetBit; virBitmapParse; virBitmapSetAll; diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index 21509ac..bb677db 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -50,6 +50,21 @@ struct _virBitmap { #define VIR_BITMAP_BIT_OFFSET(b) ((b) % VIR_BITMAP_BITS_PER_UNIT) #define VIR_BITMAP_BIT(b) (1UL << VIR_BITMAP_BIT_OFFSET(b)) +/* helper function to get last set bit in long integer */ +static int +flsl(long mask) +{ + int bit = VIR_BITMAP_BITS_PER_UNIT; + + if (mask == 0) + return 0; + + for (; !(mask & 1UL << (VIR_BITMAP_BITS_PER_UNIT - 1)); bit--) { + mask = (unsigned long)mask << 1; + } + + return bit; +} /** * virBitmapNew: @@ -632,6 +647,46 @@ virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos) } /** + * virBitmapNextLastSetBit + * @bitmap: the bitmap + * @pos: the position before which to search for a set bit + * + * Search for the last set bit before position @pos in bitmap @bitmap. + * @pos can be -1 to search for the last set bit. Position starts + * at bitmap->max_bit. + */ + +ssize_t +virBitmapNextLastSetBit(virBitmapPtr bitmap, ssize_t pos) +{ + ssize_t nl; + size_t nb; + unsigned long bits; + + if (pos < 0) + pos = bitmap->max_bit; + + pos--; + + if (pos < 0 || pos >= bitmap->max_bit) + return -1; + + nl = pos / VIR_BITMAP_BITS_PER_UNIT; + nb = pos % VIR_BITMAP_BITS_PER_UNIT; + + bits = bitmap->map[nl] & -1UL >> (VIR_BITMAP_BITS_PER_UNIT - nb - 1); + + while (bits == 0 && --nl >= 0) { + bits = bitmap->map[nl]; + } + + if (bits == 0) + return -1; + + return flsl(bits) - 1 + nl * VIR_BITMAP_BITS_PER_UNIT; +} + +/** * virBitmapNextClearBit: * @bitmap: the bitmap * @pos: the position after which to search for a clear bit @@ -679,6 +734,47 @@ virBitmapNextClearBit(virBitmapPtr bitmap, ssize_t pos) return ffsl(bits) - 1 + nl * VIR_BITMAP_BITS_PER_UNIT; } +/** + * virBitmapNextLastClearBit: + * @bitmap: the bitmap + * @pos: the position before which to search for a clear bit + * + * Search for the last clear bit before position @pos in bitmap @bitmap. + * @pos can be -1 to search for the last clear bit. Position starts + * at bitmap->max_bit. + * + * Returns the position of the found bit, or -1 if no bit found. + */ +ssize_t +virBitmapNextLastClearBit(virBitmapPtr bitmap, ssize_t pos) +{ + ssize_t nl; + size_t nb; + unsigned long bits; + + if (pos < 0) + pos = bitmap->max_bit; + + pos--; + + if (pos < 0 || pos >= bitmap->max_bit) + return -1; + + nl = pos / VIR_BITMAP_BITS_PER_UNIT; + nb = pos % VIR_BITMAP_BITS_PER_UNIT; + + bits = ~bitmap->map[nl] & -1UL >> (VIR_BITMAP_BITS_PER_UNIT - nb - 1); + + while (bits == 0 && --nl >= 0) { + bits = ~bitmap->map[nl]; + } + + if (bits == 0) + return -1; + + return flsl(bits) - 1 + nl * VIR_BITMAP_BITS_PER_UNIT; +} + /* Return the number of bits currently set in the map. */ size_t virBitmapCountBits(virBitmapPtr bitmap) diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 044c7a6..d650e1f 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -103,9 +103,15 @@ bool virBitmapIsAllSet(virBitmapPtr bitmap) ssize_t virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos) ATTRIBUTE_NONNULL(1); +ssize_t virBitmapNextLastSetBit(virBitmapPtr bitmap, ssize_t pos) + ATTRIBUTE_NONNULL(1); + ssize_t virBitmapNextClearBit(virBitmapPtr bitmap, ssize_t pos) ATTRIBUTE_NONNULL(1); +ssize_t virBitmapNextLastClearBit(virBitmapPtr bitmap, ssize_t pos) + ATTRIBUTE_NONNULL(1); + size_t virBitmapCountBits(virBitmapPtr bitmap) ATTRIBUTE_NONNULL(1); diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index 95d010a..f0a3086 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -161,7 +161,9 @@ error: return ret; } -/* test for virBitmapNextSetBit, virBitmapNextClearBit */ +/* test for virBitmapNextSetBit, virBitmapNextClearBit + * virBitmapNextLastSetBit, virBitmapNextLastClearBit + */ static int test4(const void *data ATTRIBUTE_UNUSED) { const char *bitsString = "0, 2-4, 6-10, 12, 14-18, 20, 22, 25"; @@ -193,9 +195,21 @@ static int test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextClearBit(bitmap, i - 1) != i) goto error; } + if (virBitmapNextClearBit(bitmap, i) != -1) goto error; + if (virBitmapNextLastSetBit(bitmap, -1) != -1) + goto error; + + for (i = size; i > 0; i--) { + if (virBitmapNextLastClearBit(bitmap, i) != i - 1) + goto error; + } + + if (virBitmapNextLastClearBit(bitmap, i) != -1) + goto error; + virBitmapFree(bitmap); bitmap = NULL; @@ -218,6 +232,18 @@ static int test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextSetBit(bitmap, i) != -1) goto error; + j = 1; + i = -1; + + while (j <= ARRAY_CARDINALITY(bitsPos)) { + i = virBitmapNextLastSetBit(bitmap, i); + if (i != bitsPos[ARRAY_CARDINALITY(bitsPos) - j++]) + goto error; + } + + if (virBitmapNextLastSetBit(bitmap, i) != -1) + goto error; + j = 0; i = -1; @@ -230,6 +256,18 @@ static int test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextClearBit(bitmap, i) != -1) goto error; + j = 1; + i = -1; + + while (j <= ARRAY_CARDINALITY(bitsPosInv)) { + i = virBitmapNextLastClearBit(bitmap, i); + if (i != bitsPosInv[ARRAY_CARDINALITY(bitsPosInv) - j++]) + goto error; + } + + if (virBitmapNextLastClearBit(bitmap, i) != -1) + goto error; + /* 3. full set */ virBitmapSetAll(bitmap); @@ -244,6 +282,17 @@ static int test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextClearBit(bitmap, -1) != -1) goto error; + for (i = size; i > 0; i--) { + if (virBitmapNextLastSetBit(bitmap, i) != i - 1) + goto error; + } + + if (virBitmapNextLastSetBit(bitmap, i) != -1) + goto error; + + if (virBitmapNextLastClearBit(bitmap, -1) != -1) + goto error; + virBitmapFree(bitmap); return 0; -- 1.7.11.2
This patch makes preparations for disk droping if they are not accessable when using startupPolicy "optional". That is to say, the patch aims to perform the disk presense checking before making disk chain. --- src/qemu/qemu_process.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1941d4e..3d2b7d6 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3621,14 +3621,15 @@ int qemuProcessStart(virConnectPtr conn, goto cleanup; VIR_DEBUG("Checking for CDROM and floppy presence"); + if (qemuDomainCheckDiskPresence(driver, vm, + flags & VIR_QEMU_PROCESS_START_COLD) < 0) + goto cleanup; + for (i = 0; i < vm->def->ndisks ; i++) { if (qemuDomainDetermineDiskChain(driver, vm->def->disks[i], false) < 0) goto cleanup; } - if (qemuDomainCheckDiskPresence(driver, vm, - flags & VIR_QEMU_PROCESS_START_COLD) < 0) - goto cleanup; /* Get the advisory nodeset from numad if 'placement' of * either <vcpu> or <numatune> is 'auto'. -- 1.7.11.2
With 'optional' startupPolicy set, when one or more disk are missing, the qemu process drops their definitions and bootups the vm. When the vm is using per-device boot elements, then we need to reorder them in order to perform migrate successfully if necessary. During the reordering, it uses virBitmapNextLastSetBit to find the last set bit. --- src/conf/domain_conf.c | 2 ++ src/conf/domain_conf.h | 1 + src/qemu/qemu_domain.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 86 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 177faaa..069e702 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10992,6 +10992,8 @@ virDomainDefParseXML(virCapsPtr caps, if (virDomainDefAddImplicitControllers(def) < 0) goto error; + /* Save the valid number of per-device boot */ + def->os.nBootPerDevs = bootMapSize; virBitmapFree(bootMap); return def; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 96f11ba..362f645 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1543,6 +1543,7 @@ struct _virDomainOSDef { char *machine; size_t nBootDevs; int bootDevs[VIR_DOMAIN_BOOT_LAST]; + unsigned long nBootPerDevs; /* enum virDomainBootMenu */ int bootmenu; char *init; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c79b05d..4594b2c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1791,30 +1791,89 @@ cleanup: virObjectUnref(cfg); } +/* Reorder devices with per-device boot, make them contiguous */ +static int +qemuDomainPerDevicesBootReorder(virDomainDefPtr def, + virBitmapPtr bitmap) +{ + size_t count; + size_t n; + int i = -1; + + if (bitmap == NULL) + return 0; + + count = virBitmapCountBits(bitmap); + + while (count && count--) { + i = virBitmapNextLastSetBit(bitmap, i); + for (n = 0 ; n < def->ndisks ; n++) { + virDomainDiskDefPtr disk = def->disks[n]; + if (disk->info.bootIndex > i + 1) + disk->info.bootIndex -= 1; + } + + for (n = 0 ; n < def->nnets ; n++) { + virDomainNetDefPtr net = def->nets[n]; + if (net->info.bootIndex > i + 1) + net->info.bootIndex -= 1; + } + + for (n = 0 ; n < def->nhostdevs ; n++) { + virDomainHostdevDefPtr hostdev = def->hostdevs[n]; + if (hostdev->info->bootIndex > i + 1) + hostdev->info->bootIndex -= 1; + } + + for (n = 0 ; n < def->nredirdevs ; n++) { + virDomainRedirdevDefPtr redirdev = def->redirdevs[n]; + if (redirdev->info.bootIndex > i + 1) + redirdev->info.bootIndex -= 1; + } + } + + return 0; +} + int qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, virDomainObjPtr vm, bool cold_boot) { int ret = -1; - int i; + size_t i; virDomainDiskDefPtr disk; char uuid[VIR_UUID_STRING_BUFLEN]; virDomainEventPtr event = NULL; + virDomainDefPtr def = vm->def; + size_t count = def->ndisks; + unsigned long bootMapSize = def->os.nBootPerDevs; + size_t nextDisk = 0; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + virBitmapPtr bitmap = NULL; + + virUUIDFormat(def->uuid, uuid); - virUUIDFormat(vm->def->uuid, uuid); + if (bootMapSize) { + if (!(bitmap = virBitmapNew(bootMapSize))) { + virReportOOMError(); + goto cleanup; + } + } - for (i = 0; i < vm->def->ndisks; i++) { - disk = vm->def->disks[i]; + for (i = 0; i < count; i++) { + disk = def->disks[nextDisk]; - if (!disk->startupPolicy || !disk->src) + if (!disk->startupPolicy || !disk->src) { + nextDisk++; continue; + } if (virFileAccessibleAs(disk->src, F_OK, cfg->user, cfg->group) >= 0) { /* disk accessible */ + nextDisk++; continue; } @@ -1846,20 +1905,37 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, VIR_DEBUG("Dropping disk '%s' on domain '%s' (UUID '%s') " "due to inaccessible source '%s'", - disk->dst, vm->def->name, uuid, disk->src); + disk->dst, 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); + /* For CDROM and Floppy disk, only drop source path. + * For Hard disk, drop its definition. + */ + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || + disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + VIR_FREE(disk->src); + nextDisk++; + } else { + if (bitmap && disk->info.bootIndex) + ignore_value(virBitmapSetBit(bitmap, disk->info.bootIndex - 1)); + + virDomainDiskDefFree(disk); + if (VIR_DELETE_ELEMENT(def->disks, nextDisk, def->ndisks) < 0) + goto cleanup; + } } + qemuDomainPerDevicesBootReorder(def, bitmap); + ret = 0; cleanup: virObjectUnref(cfg); + virBitmapFree(bitmap); return ret; } -- 1.7.11.2
VIR_DOMAIN_EVENT_DISK_CHANGE_MISSING_ON_START --- include/libvirt/libvirt.h.in | 1 + src/qemu/qemu_domain.c | 18 +++++++++++++----- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index f6a7aff..72df5a2 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -4603,6 +4603,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, #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 4594b2c..7ce8b1d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1907,22 +1907,30 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver, "due to inaccessible source '%s'", disk->dst, 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); - /* For CDROM and Floppy disk, only drop source path. * For Hard disk, drop its definition. */ 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); + if (event) + qemuDomainEventQueue(driver, event); + VIR_FREE(disk->src); nextDisk++; } else { if (bitmap && disk->info.bootIndex) ignore_value(virBitmapSetBit(bitmap, disk->info.bootIndex - 1)); + event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL, + disk->info.alias, + VIR_DOMAIN_EVENT_HARDDISK_DROP_MISSING_ON_START); + if (event) + qemuDomainEventQueue(driver, event); + virDomainDiskDefFree(disk); if (VIR_DELETE_ELEMENT(def->disks, nextDisk, def->ndisks) < 0) goto cleanup; -- 1.7.11.2
On 2013年03月18日 17:10, Guannan Ren wrote:
The set of patches is trying to add 'startupPolicy' attribute support to the source element of hard disks. Policy levels are using the mandatory, requisite, optional levels as originally documented.
For the 'optional' policy, there is a little difference from CDROM and Floppy which only drop its source path, for disks, if missing, the checking function will drop their definitions, because qemu doesn't allow missing source path for hard disk.
So, wondering why we need "startupPolicy" for disk not CD-ROM and FLoppy, a disk not CD-ROM or Floppy with empty source doesn't make sense, and it should be prohibited even when parsing (we do this indeed). Extending the "startupPolicy" to disk types like 'dir', 'block' makes sense though.
If guest is using per-device boot element for its devices, after dropping one or more bootable device, the boot order will not be contiguous, the way here I use is to reorder them to make them contiguous. In this way, I introduce two new bit-operating functions
virBitmapNextLastSetBit: Search for the last set bit before certain position.
virBitmapNextLastSetBit: Search for the last clear bit before certain position.
Guannan Ren(5) [PATCH 1/5] conf: add startupPolicy attribute for harddisk [PATCH 2/5] util: add two functions to find last set or unset bit in [PATCH 3/5] qemu: move disk presence checking before disk chain [PATCH 4/5] qemu: drop disk definition if missing and reorder [PATCH 5/5] event: add hard disk dropping event reason enum
docs/formatdomain.html.in | 9 +++++--- include/libvirt/libvirt.h.in | 1 + src/conf/domain_conf.c | 21 +++++++++++++------ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 2 ++ src/qemu/qemu_domain.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------- src/qemu/qemu_process.c | 7 ++++--- src/util/virbitmap.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virbitmap.h | 6 ++++++ tests/virbitmaptest.c | 51 +++++++++++++++++++++++++++++++++++++++++++++- 10 files changed, 276 insertions(+), 24 deletions(-)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 03/18/2013 06:40 PM, Osier Yang wrote:
On 2013年03月18日 17:10, Guannan Ren wrote:
The set of patches is trying to add 'startupPolicy' attribute support to the source element of hard disks. Policy levels are using the mandatory, requisite, optional levels as originally documented.
For the 'optional' policy, there is a little difference from CDROM and Floppy which only drop its source path, for disks, if missing, the checking function will drop their definitions, because qemu doesn't allow missing source path for hard disk.
So, wondering why we need "startupPolicy" for disk not CD-ROM and FLoppy, a disk not CD-ROM or Floppy with empty source doesn't make sense,
Yes, so if the disk is inaccessible , I chose to drop its whole definition like virDomainDiskDefFree(disk); Sometimes, the disk image file is the same inaccessible as CD-ROM image file.
and it should be prohibited even when parsing (we do this indeed). Extending the "startupPolicy" to disk types like 'dir', 'block' makes sense though.
On Mon, Mar 18, 2013 at 05:10:17PM +0800, Guannan Ren wrote:
The set of patches is trying to add 'startupPolicy' attribute support to the source element of hard disks. Policy levels are using the mandatory, requisite, optional levels as originally documented.
For the 'optional' policy, there is a little difference from CDROM and Floppy which only drop its source path, for disks, if missing, the checking function will drop their definitions, because qemu doesn't allow missing source path for hard disk.
What is the motivation for this feature ? I personally find even the existing CDROM code for this to be of rather dubious value, so would like to see a clear use case for why we need to expand this hack. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/18/2013 10:42 PM, Daniel P. Berrange wrote:
The set of patches is trying to add 'startupPolicy' attribute support to the source element of hard disks. Policy levels are using the mandatory, requisite, optional levels as originally documented.
For the 'optional' policy, there is a little difference from CDROM and Floppy which only drop its source path, for disks, if missing, the checking function will drop their definitions, because qemu doesn't allow missing source path for hard disk. What is the motivation for this feature ? I personally find even the existing CDROM code for this to be of rather dubious value, so would
On Mon, Mar 18, 2013 at 05:10:17PM +0800, Guannan Ren wrote: like to see a clear use case for why we need to expand this hack.
Daniel
There is a real case from a libvirt customer request, which needs to make disk of block type use "optional" policy. "The DR concept for our virtualization infrastructure is based upon host-based mirroring of independent SAN LUNs. Because RH does not support CLVM spanning multiple data centers we have to pass the LUNs to the KVM and mirror inside of the KVM. If there is an outage taking down a storage box or an entire DC, we will be unable to migrate, relocate or start any KVM, despite the KVM being able to run just on one side of the mirror." "Declare all devices as optional. If there aren't any mirror parts left, the KVM will fail, but that is an acceptable behaviour that will be recognized by our monitoring software. There is no need to have libvirt guard against failure of storage devices in such a setup. " https://bugzilla.redhat.com/show_bug.cgi?id=910171 Guannan
On Tue, Mar 19, 2013 at 03:56:40PM +0800, Guannan Ren wrote:
On 03/18/2013 10:42 PM, Daniel P. Berrange wrote:
The set of patches is trying to add 'startupPolicy' attribute support to the source element of hard disks. Policy levels are using the mandatory, requisite, optional levels as originally documented.
For the 'optional' policy, there is a little difference from CDROM and Floppy which only drop its source path, for disks, if missing, the checking function will drop their definitions, because qemu doesn't allow missing source path for hard disk. What is the motivation for this feature ? I personally find even the existing CDROM code for this to be of rather dubious value, so would
On Mon, Mar 18, 2013 at 05:10:17PM +0800, Guannan Ren wrote: like to see a clear use case for why we need to expand this hack.
Daniel
There is a real case from a libvirt customer request, which needs to make disk of block type use "optional" policy.
"The DR concept for our virtualization infrastructure is based upon host-based mirroring of independent SAN LUNs. Because RH does not support CLVM spanning multiple data centers we have to pass the LUNs to the KVM and mirror inside of the KVM. If there is an outage taking down a storage box or an entire DC, we will be unable to migrate, relocate or start any KVM, despite the KVM being able to run just on one side of the mirror."
"Declare all devices as optional. If there aren't any mirror parts left, the KVM will fail, but that is an acceptable behaviour that will be recognized by our monitoring software. There is no need to have libvirt guard against failure of storage devices in such a setup. "
Ok, so they are doing multi-pathing inside the guest, and so don't care if some of the LUNs on the host are not available when started. This makes more sense now as a feature. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 03/19/2013 08:21 PM, Daniel P. Berrange wrote:
On Tue, Mar 19, 2013 at 03:56:40PM +0800, Guannan Ren wrote:
On 03/18/2013 10:42 PM, Daniel P. Berrange wrote:
The set of patches is trying to add 'startupPolicy' attribute support to the source element of hard disks. Policy levels are using the mandatory, requisite, optional levels as originally documented.
For the 'optional' policy, there is a little difference from CDROM and Floppy which only drop its source path, for disks, if missing, the checking function will drop their definitions, because qemu doesn't allow missing source path for hard disk. What is the motivation for this feature ? I personally find even the existing CDROM code for this to be of rather dubious value, so would
On Mon, Mar 18, 2013 at 05:10:17PM +0800, Guannan Ren wrote: like to see a clear use case for why we need to expand this hack.
Daniel There is a real case from a libvirt customer request, which needs to make disk of block type use "optional" policy.
"The DR concept for our virtualization infrastructure is based upon host-based mirroring of independent SAN LUNs. Because RH does not support CLVM spanning multiple data centers we have to pass the LUNs to the KVM and mirror inside of the KVM. If there is an outage taking down a storage box or an entire DC, we will be unable to migrate, relocate or start any KVM, despite the KVM being able to run just on one side of the mirror."
"Declare all devices as optional. If there aren't any mirror parts left, the KVM will fail, but that is an acceptable behaviour that will be recognized by our monitoring software. There is no need to have libvirt guard against failure of storage devices in such a setup. " Ok, so they are doing multi-pathing inside the guest, and so don't care if some of the LUNs on the host are not available when started. This makes more sense now as a feature.
Regards, Daniel
Okay, thanks, I will rebase this patchset because [3/5] has been pushed by the same patch from Jiri.
participants (3)
-
Daniel P. Berrange -
Guannan Ren -
Osier Yang