2:51 a.m.
These patches are based on code
https://www.redhat.com/archives/libvir-list/2013-July/msg01741.html
The set of patches is trying to add 'startupPolicy' support for guest
hard disks.
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. migration is not supported yet.
Guannan Ren(2)
[PATCH v5 1/2] conf: add startupPolicy attribute for harddisk
[PATCH v5 2/2] qemu: support to drop disk with 'optional' startupPolicy
docs/formatdomain.html.in | 8 ++++++--
docs/schemas/domaincommon.rng | 6 ++++++
include/libvirt/libvirt.h.in | 1 +
src/conf/domain_conf.c | 31 +++++++++++++++++++++++--------
src/qemu/qemu_domain.c | 77
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------
5 files changed, 97 insertions(+), 26 deletions(-)
2:51 a.m.
New subject: [libvirt] [PATCH v5 1/2] conf: add startupPolicy attribute for harddisk
Add startupPolicy attribute policy for harddisk with type "file",
"block" and "dir". 'requisite' is not supported currently for
hardisk.
---
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>),
+ 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.
</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 "
+ "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'",
--
1.8.3.1
8:54 a.m.
New subject: [libvirt] [PATCH v5 1/2] conf: add startupPolicy attribute for harddisk
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
2:51 a.m.
New subject: [libvirt] [PATCH v5 2/2] qemu: support to drop disk with 'optional' startupPolicy
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,
#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();
- 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;
+
VIR_DEBUG("Checking for disk presence");
- for (i = 0; i < vm->def->ndisks; i++) {
- disk = vm->def->disks[i];
+ for (i = 0; i < count; i++) {
+ disk = vm->def->disks[nextdisk];
+
if (!disk->src)
continue;
if (qemuDomainDetermineDiskChain(driver, disk, false) >= 0 &&
- qemuDiskChainCheckBroken(disk) >= 0)
+ qemuDiskChainCheckBroken(disk) >= 0) {
+ nextdisk++;
continue;
+ }
if (disk->startupPolicy) {
if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
- cold_boot) >= 0)
+ cold_boot,
+ &nextdisk) >= 0)
continue;
}
--
1.8.3.1
9:39 a.m.
New subject: [libvirt] [PATCH v5 2/2] qemu: support to drop disk with 'optional' startupPolicy
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
1:37 a.m.
New subject: [libvirt] [PATCH v6 2/2] qemu: support to drop disk with 'optional' startupPolicy
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
@@ -2027,13 +2027,53 @@ cleanup:
}
static int
+qemuDomainCheckRemoveOptionalDisk(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ virDomainDiskDefPtr disk)
+{
+ 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);
+ VIR_FREE(disk->src);
+ } else {
+ event = virDomainEventDiskChangeNewFromObj(vm, disk->src, NULL,
+ disk->info.alias,
+
VIR_DOMAIN_EVENT_DISK_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)
{
char uuid[VIR_UUID_STRING_BUFLEN];
- virDomainEventPtr event = NULL;
int startupPolicy = disk->startupPolicy;
virUUIDFormat(vm->def->uuid, uuid);
@@ -2056,17 +2096,8 @@ qemuDomainCheckDiskStartupPolicy(virQEMUDriverPtr driver,
break;
}
- virResetLastError();
- 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) < 0)
+ goto error;
return 0;
@@ -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];
if (!disk->src)
@@ -2094,10 +2126,11 @@ qemuDomainCheckDiskPresence(virQEMUDriverPtr driver,
qemuDiskChainCheckBroken(disk) >= 0)
continue;
- if (disk->startupPolicy) {
- if (qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
- cold_boot) >= 0)
- continue;
+ if (disk->startupPolicy &&
+ qemuDomainCheckDiskStartupPolicy(driver, vm, disk,
+ cold_boot) >= 0) {
+ virResetLastError();
+ continue;
}
goto error;
--
1.8.3.1
8:40 a.m.
New subject: [libvirt] [PATCH v6 2/2] qemu: support to drop disk with 'optional' startupPolicy
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
10:03 a.m.
New subject: [libvirt] [PATCH v6 2/2] qemu: support to drop disk with 'optional' startupPolicy
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's good you noticed it in time. my local testcase doesn't include the
case of domain without disks.
I should add it.
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.
Make sense.
ACK with that fixed and this patch squashed in the previous one (also
with those mentioned things fixed).
Martin
Thanks for this review.
Guannan
4:32 a.m.
New subject: [libvirt] [PATCH v6 2/2] qemu: support to drop disk with 'optional' startupPolicy
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
4107
days inactive
4114
days old
8 comments
2 participants
participants (2)
-
Guannan Ren -
Martin Kletzander