1:35 p.m.
These two patches are related to:
https://bugzilla.redhat.com/show_bug.cgi?id=730909
The first removes the obvious problem of attempting to feed qemu
invalid options. The second makes it possible to specify a different
policy for read errors and write errors. 1/2 is written such that 2/2
will apply with minimal rewriting of existing code (that's why the
qemu_command.c part may seem a bit obtuse at first).
1:35 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: correct misspelled 'enospc' option, and only use for werror
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=730909
When support for setting the qemu disk error policy to 'enospc' was
added, it was inadvertantly as "enospace". This patch corrects that on
the qemu commandline (while retaining the "enospace" spelling for
libvirt's XML.
Also, while examining the qemu source, I found that "enospc" is not
allowed for the read error policy, only for write error policy (makes
sense). Since libvirt currently only has a single error policy
setting, when "enospace" is selected, the read error policy is set to
"ignore".
---
src/qemu/qemu_command.c | 22 ++++++++++++++++++----
1 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index ff83e2d..123bcab 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1692,11 +1692,25 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
}
if (qemuCapsGet(qemuCaps, QEMU_CAPS_MONITOR_JSON)) {
- if (disk->error_policy) {
- virBufferAsprintf(&opt, ",werror=%s,rerror=%s",
-
virDomainDiskErrorPolicyTypeToString(disk->error_policy),
-
virDomainDiskErrorPolicyTypeToString(disk->error_policy));
+ const char *wpolicy = NULL, *rpolicy = NULL;
+
+ if (disk->error_policy)
+ wpolicy = virDomainDiskErrorPolicyTypeToString(disk->error_policy);
+ if (!rpolicy)
+ rpolicy = wpolicy;
+
+ if (disk->error_policy == VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE) {
+ /* in the case of enospace, the option is spelled differently in qemu,
+ * and it's only valid for werror, not for rerror.
+ */
+ wpolicy="enospc";
+ rpolicy="ignore";
}
+
+ if (wpolicy)
+ virBufferAsprintf(&opt, ",werror=%s", wpolicy);
+ if (rpolicy)
+ virBufferAsprintf(&opt, ",rerror=%s", rpolicy);
}
if (disk->iomode) {
--
1.7.3.4
2:17 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: correct misspelled 'enospc' option, and only use for werror
On 10/04/2011 12:35 PM, Laine Stump wrote:
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=730909
When support for setting the qemu disk error policy to 'enospc' was
added, it was inadvertantly as "enospace". This patch corrects that on
spelling and grammar
s/inadvertantly/inadvertently spelled/
the qemu commandline (while retaining the "enospace"
spelling for
libvirt's XML.
Also, while examining the qemu source, I found that "enospc" is not
allowed for the read error policy, only for write error policy (makes
sense). Since libvirt currently only has a single error policy
setting, when "enospace" is selected, the read error policy is set to
"ignore".
---
src/qemu/qemu_command.c | 22 ++++++++++++++++++----
1 files changed, 18 insertions(+), 4 deletions(-)
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
11:31 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: correct misspelled 'enospc' option, and only use for werror
On 10/04/2011 03:17 PM, Eric Blake wrote:
On 10/04/2011 12:35 PM, Laine Stump wrote:
> This resolves:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=730909
>
> When support for setting the qemu disk error policy to 'enospc' was
> added, it was inadvertantly as "enospace". This patch corrects that on
spelling and grammar
s/inadvertantly/inadvertently spelled/
You have to appreciate the irony, though, right? :-)
> the qemu commandline (while retaining the "enospace" spelling for
> libvirt's XML.
>
> Also, while examining the qemu source, I found that "enospc" is not
> allowed for the read error policy, only for write error policy (makes
> sense). Since libvirt currently only has a single error policy
> setting, when "enospace" is selected, the read error policy is set to
> "ignore".
> ---
> src/qemu/qemu_command.c | 22 ++++++++++++++++++----
> 1 files changed, 18 insertions(+), 4 deletions(-)
ACK.
Okay, I'm pushing this one without waiting for 2/2, though, as I haven't
decided how to deal with some of the issues you brought up in your review.
10:06 p.m.
New subject: [libvirt] [PATCH 1.5/2] qemu: leave rerror policy at default when enospace is requested
commit 12062ab set rerror=ignore when error_policy="enospace" was
selected (since the rerror option in qemu doesn't accept "enospc", as
the werror option does).
After that patch was already pushed, Paolo Bonzini noticed it and
commented that leaving rerror at the default ("report") would be a
better choice. This patch corrects the problem - if error_policy =
"enospace" is given, rerror is left off the qemu commandline,
effectively setting it to "report". For other values, rerror is still
set to match werror.
Additionally, the parsing of error_policy was changed to no longer
erroneously allow "default" as a choice - as with most other
attributes, if you want the default setting, just don't specify an
error_policy.
Finally, two ommissions in the first patch were corrected - a
long-dormant qemuxml2argv test for enospace was enabled, and fixed to
pass, and the argv2xml parser in qemu_command.c was updated to
recognize the different spelling on the qemu commandline.
---
src/conf/domain_conf.c | 2 +-
src/qemu/qemu_command.c | 15 ++++++++-------
...uxml2argv-disk-drive-error-policy-enospace.args | 2 +-
tests/qemuxml2argvtest.c | 2 ++
4 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f9007ce..55e6c34 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2554,7 +2554,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
}
if (error_policy &&
- (def->error_policy = virDomainDiskErrorPolicyTypeFromString(error_policy))
< 0) {
+ (def->error_policy = virDomainDiskErrorPolicyTypeFromString(error_policy))
<= 0) {
virDomainReportError(VIR_ERR_INTERNAL_ERROR,
_("unknown disk error policy '%s'"),
error_policy);
goto error;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 123bcab..5f729a4 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1696,15 +1696,16 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
if (disk->error_policy)
wpolicy = virDomainDiskErrorPolicyTypeToString(disk->error_policy);
- if (!rpolicy)
- rpolicy = wpolicy;
if (disk->error_policy == VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE) {
- /* in the case of enospace, the option is spelled differently in qemu,
- * and it's only valid for werror, not for rerror.
+ /* in the case of enospace, the option is spelled
+ * differently in qemu, and it's only valid for werror,
+ * not for rerror, so leave leave rerror NULL.
*/
- wpolicy="enospc";
- rpolicy="ignore";
+ wpolicy = "enospc";
+ } else if (!rpolicy) {
+ /* for other policies, rpolicy can match wpolicy */
+ rpolicy = wpolicy;
}
if (wpolicy)
@@ -5636,7 +5637,7 @@ qemuParseCommandLineDisk(virCapsPtr caps,
def->error_policy = VIR_DOMAIN_DISK_ERROR_POLICY_STOP;
else if (STREQ(values[i], "ignore"))
def->error_policy = VIR_DOMAIN_DISK_ERROR_POLICY_IGNORE;
- else if (STREQ(values[i], "enospace"))
+ else if (STREQ(values[i], "enospc"))
def->error_policy = VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE;
} else if (STREQ(keywords[i], "index")) {
if (virStrToLong_i(values[i], NULL, 10, &idx) < 0) {
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-enospace.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-enospace.args
index 267eb5f..981d410 100644
--- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-enospace.args
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-enospace.args
@@ -1,6 +1,6 @@
LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \
pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \
-no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0,\
-format=qcow2,cache=off,werror=enospace,rerror=enospace -drive \
+format=qcow2,cache=off,werror=enospc -drive \
file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,bus=1,unit=0,format=raw -net \
none -serial none -parallel none -usb
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 9e174b3..24e831c 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -332,6 +332,8 @@ mymain(void)
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-error-policy-stop", false,
QEMU_CAPS_DRIVE, QEMU_CAPS_MONITOR_JSON, QEMU_CAPS_DRIVE_FORMAT);
+ DO_TEST("disk-drive-error-policy-enospace", false,
+ QEMU_CAPS_DRIVE, QEMU_CAPS_MONITOR_JSON, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-cache-v2-wt", false,
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_CACHE_V2, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-cache-v2-wb", false,
--
1.7.3.4
11:52 a.m.
New subject: [libvirt] [PATCH 1.5/2] qemu: leave rerror policy at default when enospace is requested
On 10/05/2011 09:06 PM, Laine Stump wrote:
commit 12062ab set rerror=ignore when
error_policy="enospace" was
selected (since the rerror option in qemu doesn't accept "enospc", as
the werror option does).
After that patch was already pushed, Paolo Bonzini noticed it and
commented that leaving rerror at the default ("report") would be a
better choice. This patch corrects the problem - if error_policy =
"enospace" is given, rerror is left off the qemu commandline,
effectively setting it to "report". For other values, rerror is still
set to match werror.
Additionally, the parsing of error_policy was changed to no longer
erroneously allow "default" as a choice - as with most other
attributes, if you want the default setting, just don't specify an
error_policy.
Finally, two ommissions in the first patch were corrected - a
s/ommissions/omissions/
long-dormant qemuxml2argv test for enospace was enabled, and fixed
to
pass, and the argv2xml parser in qemu_command.c was updated to
recognize the different spelling on the qemu commandline.
---
src/conf/domain_conf.c | 2 +-
src/qemu/qemu_command.c | 15 ++++++++-------
...uxml2argv-disk-drive-error-policy-enospace.args | 2 +-
tests/qemuxml2argvtest.c | 2 ++
4 files changed, 12 insertions(+), 9 deletions(-)
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:30 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: correct misspelled 'enospc' option, and only use for werror
On 10/04/2011 08:35 PM, Laine Stump wrote:
This resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=730909
When support for setting the qemu disk error policy to 'enospc' was
added, it was inadvertantly as "enospace". This patch corrects that on
the qemu commandline (while retaining the "enospace" spelling for
libvirt's XML.
Also, while examining the qemu source, I found that "enospc" is not
allowed for the read error policy, only for write error policy (makes
sense). Since libvirt currently only has a single error policy
setting, when "enospace" is selected, the read error policy is set to
"ignore".
---
src/qemu/qemu_command.c | 22 ++++++++++++++++++----
1 files changed, 18 insertions(+), 4 deletions(-)
For errors other than ENOSPC, werror=enospc will report the errors, so I
believe rerror=report is a better choice.
Paolo
1:35 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: add separate rerror_policy for disk errors
libvirt's XML only had a single attribute, error_policy to control
both read and write error policy, but qemu has separate settings for
these. In one case (enospc) a policy is allowed for write errors but
not read errors.
This patch adds a separate attribute that sets only the read error
policy. If just error_policy is set, it will apply to both read and
write error policy (previous behavior), but if the new rerror_policy
attribute is set, it will override error_policy for read errors only.
---
docs/formatdomain.html.in | 16 +++++++++++++---
src/conf/domain_conf.c | 16 ++++++++++++++++
src/conf/domain_conf.h | 3 ++-
src/qemu/qemu_command.c | 5 ++++-
4 files changed, 35 insertions(+), 5 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 593adcb..5265b21 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1008,9 +1008,19 @@
</li>
<li>
The optional <code>error_policy</code> attribute controls
- how the hypervisor will behave on an error, possible
- values are "stop", "ignore", and "enospace".
- <span class="since">Since 0.8.0</span>
+ how the hypervisor will behave on a disk read or write
+ error, possible values are "stop", "ignore", and
+ "enospace".<span class="since">Since
0.8.0</span> There is
+ also an optional <code>rerror_policy</code> that controls
+ behavior for read errors only.<span class="since">Since
+ 0.9.7</space>. If no rerror_policy is given, error_policy
+ is used for both read and write errors. If rerror_policy
+ is given, it overrides the <code>error_policy</code> for
+ read errors. Also note that "enospace" is not a valid
+ policy for read errors, so if <code>error_policy</code> is
+ set to "enospace" and no <code>rerror_policy</code> is
+ given, the read error policy will be automatically set to
+ "ignore".
</li>
<li>
The optional <code>io</code> attribute controls specific
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f9007ce..95e1a9a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -2297,6 +2297,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
char *bus = NULL;
char *cachetag = NULL;
char *error_policy = NULL;
+ char *rerror_policy = NULL;
char *iotag = NULL;
char *ioeventfd = NULL;
char *event_idx = NULL;
@@ -2416,6 +2417,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
driverType = virXMLPropString(cur, "type");
cachetag = virXMLPropString(cur, "cache");
error_policy = virXMLPropString(cur, "error_policy");
+ rerror_policy = virXMLPropString(cur, "rerror_policy");
iotag = virXMLPropString(cur, "io");
ioeventfd = virXMLPropString(cur, "ioeventfd");
event_idx = virXMLPropString(cur, "event_idx");
@@ -2560,6 +2562,16 @@ virDomainDiskDefParseXML(virCapsPtr caps,
goto error;
}
+ if (rerror_policy &&
+ (((def->rerror_policy
+ = virDomainDiskErrorPolicyTypeFromString(error_policy)) < 0) ||
+ (def->rerror_policy == VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE))) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unknown disk read error policy '%s'"),
+ rerror_policy);
+ goto error;
+ }
+
if (iotag) {
if ((def->iomode = virDomainDiskIoTypeFromString(iotag)) < 0 ||
def->iomode == VIR_DOMAIN_DISK_IO_DEFAULT) {
@@ -2667,6 +2679,7 @@ cleanup:
VIR_FREE(driverName);
VIR_FREE(cachetag);
VIR_FREE(error_policy);
+ VIR_FREE(rerror_policy);
VIR_FREE(iotag);
VIR_FREE(ioeventfd);
VIR_FREE(event_idx);
@@ -9127,6 +9140,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
const char *bus = virDomainDiskBusTypeToString(def->bus);
const char *cachemode = virDomainDiskCacheTypeToString(def->cachemode);
const char *error_policy =
virDomainDiskErrorPolicyTypeToString(def->error_policy);
+ const char *rerror_policy =
virDomainDiskErrorPolicyTypeToString(def->rerror_policy);
const char *iomode = virDomainDiskIoTypeToString(def->iomode);
const char *ioeventfd = virDomainIoEventFdTypeToString(def->ioeventfd);
const char *event_idx = virDomainVirtioEventIdxTypeToString(def->event_idx);
@@ -9177,6 +9191,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, " cache='%s'", cachemode);
if (def->error_policy)
virBufferAsprintf(buf, " error_policy='%s'",
error_policy);
+ if (def->rerror_policy)
+ virBufferAsprintf(buf, " rerror_policy='%s'",
rerror_policy);
if (def->iomode)
virBufferAsprintf(buf, " io='%s'", iomode);
if (def->ioeventfd)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index bc41d34..35eacc7 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -284,7 +284,8 @@ struct _virDomainDiskDef {
char *driverType;
char *serial;
int cachemode;
- int error_policy;
+ int error_policy; /* virDomainDiskErrorPolicy */
+ int rerror_policy; /* virDomainDiskErrorPolicy */
int bootIndex;
int iomode;
int ioeventfd;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 123bcab..4f1bb25 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1696,6 +1696,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
if (disk->error_policy)
wpolicy = virDomainDiskErrorPolicyTypeToString(disk->error_policy);
+ if (disk->rerror_policy)
+ rpolicy = virDomainDiskErrorPolicyTypeToString(disk->rerror_policy);
if (!rpolicy)
rpolicy = wpolicy;
@@ -1704,7 +1706,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
* and it's only valid for werror, not for rerror.
*/
wpolicy="enospc";
- rpolicy="ignore";
+ if (!disk->rerror_policy)
+ rpolicy="ignore";
}
if (wpolicy)
--
1.7.3.4
2:54 p.m.
New subject: [libvirt] [PATCH 2/2] qemu: add separate rerror_policy for disk errors
On 10/04/2011 12:35 PM, Laine Stump wrote:
libvirt's XML only had a single attribute, error_policy to
control
both read and write error policy, but qemu has separate settings for
these. In one case (enospc) a policy is allowed for write errors but
not read errors.
This patch adds a separate attribute that sets only the read error
policy. If just error_policy is set, it will apply to both read and
write error policy (previous behavior), but if the new rerror_policy
attribute is set, it will override error_policy for read errors only.
---
docs/formatdomain.html.in | 16 +++++++++++++---
Missing the patch to docs/schemas/domaincommon.rng
Also, I'd like to see at least one addition to tests/qemuxml2argvdata
that exposes the new command line.
-<span class="since">Since 0.8.0</span>
+ how the hypervisor will behave on a disk read or write
+ error, possible values are "stop", "ignore", and
+ "enospace".<span class="since">Since
0.8.0</span> There is
+ also an optional<code>rerror_policy</code> that controls
+ behavior for read errors only.<span class="since">Since
+ 0.9.7</space>. If no rerror_policy is given, error_policy
+ is used for both read and write errors. If rerror_policy
+ is given, it overrides the<code>error_policy</code> for
+ read errors. Also note that "enospace" is not a valid
+ policy for read errors, so if<code>error_policy</code> is
+ set to "enospace" and no<code>rerror_policy</code>
is
+ given, the read error policy will be automatically set to
+ "ignore".
I think we also need to document that "default" can be used for either
(or both) [r]error_policy as a way to rely on the hypervisor defaults.
More on this below.
@@ -2560,6 +2562,16 @@ virDomainDiskDefParseXML(virCapsPtr caps,
goto error;
}
+ if (rerror_policy&&
+ (((def->rerror_policy
+ = virDomainDiskErrorPolicyTypeFromString(error_policy))< 0) ||
virDomainDiskErrorPolicyTypeFromString converts "default" to 0, which
means this is an undocumented value that we parse. In many existing
cases, we use vir...TypeFromString() <= 0 to reject parsing "default".
But here, you copied-and-pasted from error_policy, which also was using
< 0 (and silently accepting "default"). Now, as to _why_ it makes sense
to allow default, consider the case where I want my qemu command line to
have one, but not both, policies. How do I specify that in the XML? By
doing:
rerror_policy='stop'
I get my desired:
rerror=stop
But what about the converse, for just werror on the command line? Using
error_policy='stop'
produces
rerror=stop,werror=stop
which isn't quite what I wanted. So I need:
rerror_policy='default' error_policy='stop'
to get exactly:
werror=stop
on the command line, which argues that "default" should be part of the
documented XML for rerror_policy. And symmetry argues that we might as
well then document "default" for error_policy.
Hence, I think the following rng change covers things:
diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng
index cffaac2..fffee9a 100644
--- i/docs/schemas/domaincommon.rng
+++ w/docs/schemas/domaincommon.rng
@@ -853,13 +853,25 @@
</attribute>
</define>
<define name="driverErrorPolicy">
- <attribute name="error_policy">
- <choice>
- <value>stop</value>
- <value>ignore</value>
- <value>enospace</value>
- </choice>
- </attribute>
+ <optional>
+ <attribute name="error_policy">
+ <choice>
+ <value>default</value>
+ <value>stop</value>
+ <value>ignore</value>
+ <value>enospace</value>
+ </choice>
+ </attribute>
+ </optional>
+ <optional>
+ <attribute name="rerror_policy">
+ <choice>
+ <value>default</value>
+ <value>stop</value>
+ <value>ignore</value>
+ </choice>
+ </attribute>
+ </optional>
</define>
<define name="driverIO">
<attribute name="io">
@@ -9177,6 +9191,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, " cache='%s'", cachemode);
if (def->error_policy)
virBufferAsprintf(buf, " error_policy='%s'",
error_policy);
+ if (def->rerror_policy)
+ virBufferAsprintf(buf, " rerror_policy='%s'",
rerror_policy);
Hmm, given my earlier arguments that documenting "default" makes sense,
we would have to change the logic here to be that we always print both
attributes, even if one of them is "default", if at least one of them is
something besides default:
if (def->error_policy || def->rerror_policy) {
virBufferAsprintf(buf, " error_policy='%s'
rerror_policy='%s'",
error_policy, rerror_policy
}
+++ b/src/qemu/qemu_command.c
@@ -1696,6 +1696,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
if (disk->error_policy)
wpolicy = virDomainDiskErrorPolicyTypeToString(disk->error_policy);
+ if (disk->rerror_policy)
+ rpolicy = virDomainDiskErrorPolicyTypeToString(disk->rerror_policy);
if (!rpolicy)
rpolicy = wpolicy;
@@ -1704,7 +1706,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
* and it's only valid for werror, not for rerror.
*/
wpolicy="enospc";
- rpolicy="ignore";
+ if (!disk->rerror_policy)
+ rpolicy="ignore";
And if you take my suggestion for an explicit "default", then we can't
quite key off of !disk->rerror_policy. Hmm, maybe I'm making this all
more complicated than it has to be. ;(
An alternative would be adding "none" as a valid (non-zero) policy for
both [r]error_policy, fix the parser to use <= 0 to reject "default",
and fix the qemu_command to convert the enum value of NONE to omitting
the string.
At any rate, I'll let you think about it a bit more for a v2 - we
definitely need the rng improved and tests added, before we commit to
anything, even if what we end up committing to doesn't expose "default"
as an option to the user.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:06 p.m.
New subject: [libvirt] [PATCHv2 2/2] qemu: add separate rerror_policy for disk errors
Previously libvirt's disk device XML only had a single attribute,
error_policy, to control both read and write error policy, but qemu
has separate options for controlling read and write. In one case
(enospc) a policy is allowed for write errors but not read errors.
This patch adds a separate attribute that sets only the read error
policy. If just error_policy is set, it will apply to both read and
write error policy (previous behavior), but if the new rerror_policy
attribute is set, it will override error_policy for read errors only.
Possible values for rerror_policy are "stop", "report", and
"ignore"
("report" is the qemu-controlled default for rerror_policy when
error_policy isn't specified).
For consistency, the value "report" has been added to the possible
values for error_policy as well.
---
Changes in V2:
* added rerror_policy to domaincommon.rng
* added "report" to the list of possible policies and to docs
* fix qemuParseCommandLineDisk to set rerror_policy and error_policy
separately.
* add a new xml2argv & argv2xml test that sets error_policy='report'
rerror_policy='ignore'.
docs/formatdomain.html.in | 18 +++++++++--
docs/schemas/domaincommon.rng | 13 ++++++++
src/conf/domain_conf.c | 17 ++++++++++
src/conf/domain_conf.h | 4 ++-
src/qemu/qemu_command.c | 17 +++++++++-
tests/qemuargv2xmltest.c | 1 +
...gv-disk-drive-error-policy-wreport-rignore.args | 6 +++
...rgv-disk-drive-error-policy-wreport-rignore.xml | 33 ++++++++++++++++++++
tests/qemuxml2argvtest.c | 2 +
9 files changed, 105 insertions(+), 6 deletions(-)
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-wreport-rignore.args
create mode 100644
tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-wreport-rignore.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 593adcb..b15f9e2 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1008,9 +1008,21 @@
</li>
<li>
The optional <code>error_policy</code> attribute controls
- how the hypervisor will behave on an error, possible
- values are "stop", "ignore", and "enospace".
- <span class="since">Since 0.8.0</span>
+ how the hypervisor will behave on a disk read or write
+ error, possible values are "stop", "report",
"ignore", and
+ "enospace".<span class="since">Since 0.8.0,
"report" since
+ 0.9.7</span> The default setting of error_policy is
"report".
+ There is also an
+ optional <code>rerror_policy</code> that controls behavior
+ for read errors only. <span class="since">Since
+ 0.9.7</space>. If no rerror_policy is given, error_policy
+ is used for both read and write errors. If rerror_policy
+ is given, it overrides the <code>error_policy</code> for
+ read errors. Also note that "enospace" is not a valid
+ policy for read errors, so if <code>error_policy</code> is
+ set to "enospace" and no <code>rerror_policy</code> is
+ given, the read error policy will be left at its default,
+ which is "report".
</li>
<li>
The optional <code>io</code> attribute controls specific
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index cffaac2..492a41d 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -820,6 +820,9 @@
<ref name="driverErrorPolicy"/>
</optional>
<optional>
+ <ref name="driverRerrorPolicy"/>
+ </optional>
+ <optional>
<ref name="driverIO"/>
</optional>
<optional>
@@ -856,11 +859,21 @@
<attribute name="error_policy">
<choice>
<value>stop</value>
+ <value>report</value>
<value>ignore</value>
<value>enospace</value>
</choice>
</attribute>
</define>
+ <define name="driverRerrorPolicy">
+ <attribute name="rerror_policy">
+ <choice>
+ <value>stop</value>
+ <value>report</value>
+ <value>ignore</value>
+ </choice>
+ </attribute>
+ </define>
<define name="driverIO">
<attribute name="io">
<choice>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 55e6c34..a537251 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -176,6 +176,7 @@ VIR_ENUM_IMPL(virDomainDiskCache, VIR_DOMAIN_DISK_CACHE_LAST,
VIR_ENUM_IMPL(virDomainDiskErrorPolicy, VIR_DOMAIN_DISK_ERROR_POLICY_LAST,
"default",
"stop",
+ "report",
"ignore",
"enospace")
@@ -2297,6 +2298,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
char *bus = NULL;
char *cachetag = NULL;
char *error_policy = NULL;
+ char *rerror_policy = NULL;
char *iotag = NULL;
char *ioeventfd = NULL;
char *event_idx = NULL;
@@ -2416,6 +2418,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
driverType = virXMLPropString(cur, "type");
cachetag = virXMLPropString(cur, "cache");
error_policy = virXMLPropString(cur, "error_policy");
+ rerror_policy = virXMLPropString(cur, "rerror_policy");
iotag = virXMLPropString(cur, "io");
ioeventfd = virXMLPropString(cur, "ioeventfd");
event_idx = virXMLPropString(cur, "event_idx");
@@ -2560,6 +2563,16 @@ virDomainDiskDefParseXML(virCapsPtr caps,
goto error;
}
+ if (rerror_policy &&
+ (((def->rerror_policy
+ = virDomainDiskErrorPolicyTypeFromString(rerror_policy)) <= 0) ||
+ (def->rerror_policy == VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE))) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unknown disk read error policy '%s'"),
+ rerror_policy);
+ goto error;
+ }
+
if (iotag) {
if ((def->iomode = virDomainDiskIoTypeFromString(iotag)) < 0 ||
def->iomode == VIR_DOMAIN_DISK_IO_DEFAULT) {
@@ -2667,6 +2680,7 @@ cleanup:
VIR_FREE(driverName);
VIR_FREE(cachetag);
VIR_FREE(error_policy);
+ VIR_FREE(rerror_policy);
VIR_FREE(iotag);
VIR_FREE(ioeventfd);
VIR_FREE(event_idx);
@@ -9127,6 +9141,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
const char *bus = virDomainDiskBusTypeToString(def->bus);
const char *cachemode = virDomainDiskCacheTypeToString(def->cachemode);
const char *error_policy =
virDomainDiskErrorPolicyTypeToString(def->error_policy);
+ const char *rerror_policy =
virDomainDiskErrorPolicyTypeToString(def->rerror_policy);
const char *iomode = virDomainDiskIoTypeToString(def->iomode);
const char *ioeventfd = virDomainIoEventFdTypeToString(def->ioeventfd);
const char *event_idx = virDomainVirtioEventIdxTypeToString(def->event_idx);
@@ -9177,6 +9192,8 @@ virDomainDiskDefFormat(virBufferPtr buf,
virBufferAsprintf(buf, " cache='%s'", cachemode);
if (def->error_policy)
virBufferAsprintf(buf, " error_policy='%s'",
error_policy);
+ if (def->rerror_policy)
+ virBufferAsprintf(buf, " rerror_policy='%s'",
rerror_policy);
if (def->iomode)
virBufferAsprintf(buf, " io='%s'", iomode);
if (def->ioeventfd)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index bc41d34..56c9777 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -209,6 +209,7 @@ enum virDomainDiskCache {
enum virDomainDiskErrorPolicy {
VIR_DOMAIN_DISK_ERROR_POLICY_DEFAULT,
VIR_DOMAIN_DISK_ERROR_POLICY_STOP,
+ VIR_DOMAIN_DISK_ERROR_POLICY_REPORT,
VIR_DOMAIN_DISK_ERROR_POLICY_IGNORE,
VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE,
@@ -284,7 +285,8 @@ struct _virDomainDiskDef {
char *driverType;
char *serial;
int cachemode;
- int error_policy;
+ int error_policy; /* virDomainDiskErrorPolicy */
+ int rerror_policy; /* virDomainDiskErrorPolicy */
int bootIndex;
int iomode;
int ioeventfd;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 5f729a4..cf99f89 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1696,6 +1696,8 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
if (disk->error_policy)
wpolicy = virDomainDiskErrorPolicyTypeToString(disk->error_policy);
+ if (disk->rerror_policy)
+ rpolicy = virDomainDiskErrorPolicyTypeToString(disk->rerror_policy);
if (disk->error_policy == VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE) {
/* in the case of enospace, the option is spelled
@@ -5631,14 +5633,22 @@ qemuParseCommandLineDisk(virCapsPtr caps,
def->cachemode = VIR_DOMAIN_DISK_CACHE_DIRECTSYNC;
else if (STREQ(values[i], "unsafe"))
def->cachemode = VIR_DOMAIN_DISK_CACHE_UNSAFE;
- } else if (STREQ(keywords[i], "werror") ||
- STREQ(keywords[i], "rerror")) {
+ } else if (STREQ(keywords[i], "werror")) {
if (STREQ(values[i], "stop"))
def->error_policy = VIR_DOMAIN_DISK_ERROR_POLICY_STOP;
+ else if (STREQ(values[i], "report"))
+ def->error_policy = VIR_DOMAIN_DISK_ERROR_POLICY_REPORT;
else if (STREQ(values[i], "ignore"))
def->error_policy = VIR_DOMAIN_DISK_ERROR_POLICY_IGNORE;
else if (STREQ(values[i], "enospc"))
def->error_policy = VIR_DOMAIN_DISK_ERROR_POLICY_ENOSPACE;
+ } else if (STREQ(keywords[i], "rerror")) {
+ if (STREQ(values[i], "stop"))
+ def->rerror_policy = VIR_DOMAIN_DISK_ERROR_POLICY_STOP;
+ else if (STREQ(values[i], "report"))
+ def->rerror_policy = VIR_DOMAIN_DISK_ERROR_POLICY_REPORT;
+ else if (STREQ(values[i], "ignore"))
+ def->rerror_policy = VIR_DOMAIN_DISK_ERROR_POLICY_IGNORE;
} else if (STREQ(keywords[i], "index")) {
if (virStrToLong_i(values[i], NULL, 10, &idx) < 0) {
virDomainDiskDefFree(def);
@@ -5674,6 +5684,9 @@ qemuParseCommandLineDisk(virCapsPtr caps,
}
}
+ if (def->rerror_policy == def->error_policy)
+ def->rerror_policy = 0;
+
if (!def->src &&
def->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
def->type != VIR_DOMAIN_DISK_TYPE_NETWORK) {
diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c
index 6a79630..807a771 100644
--- a/tests/qemuargv2xmltest.c
+++ b/tests/qemuargv2xmltest.c
@@ -165,6 +165,7 @@ mymain(void)
DO_TEST("disk-drive-cache-v1-none");
DO_TEST("disk-drive-error-policy-stop");
DO_TEST("disk-drive-error-policy-enospace");
+ DO_TEST("disk-drive-error-policy-wreport-rignore");
DO_TEST("disk-drive-cache-v2-wt");
DO_TEST("disk-drive-cache-v2-wb");
DO_TEST("disk-drive-cache-v2-none");
diff --git
a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-wreport-rignore.args
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-wreport-rignore.args
new file mode 100644
index 0000000..4879576
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-wreport-rignore.args
@@ -0,0 +1,6 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \
+pc -m 214 -smp 1 -nographic -monitor unix:/tmp/test-monitor,server,nowait \
+-no-acpi -boot c -drive file=/dev/HostVG/QEMUGuest1,if=ide,bus=0,unit=0,\
+format=qcow2,cache=off,werror=report,rerror=ignore -drive \
+file=/dev/HostVG/QEMUGuest2,if=ide,media=cdrom,bus=1,unit=0,format=raw -net \
+none -serial none -parallel none -usb
diff --git
a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-wreport-rignore.xml
b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-wreport-rignore.xml
new file mode 100644
index 0000000..70068aa
--- /dev/null
+++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-error-policy-wreport-rignore.xml
@@ -0,0 +1,33 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory>219136</memory>
+ <currentMemory>219136</currentMemory>
+ <vcpu>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu</emulator>
+ <disk type='block' device='disk'>
+ <driver name='qemu' type='qcow2' cache='none'
error_policy='report' rerror_policy='ignore'/>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
unit='0'/>
+ </disk>
+ <disk type='block' device='cdrom'>
+ <driver name='qemu' type='raw'/>
+ <source dev='/dev/HostVG/QEMUGuest2'/>
+ <target dev='hdc' bus='ide'/>
+ <readonly/>
+ <address type='drive' controller='0' bus='1'
unit='0'/>
+ </disk>
+ <controller type='ide' index='0'/>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index 24e831c..3d65d5f 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -334,6 +334,8 @@ mymain(void)
QEMU_CAPS_DRIVE, QEMU_CAPS_MONITOR_JSON, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-error-policy-enospace", false,
QEMU_CAPS_DRIVE, QEMU_CAPS_MONITOR_JSON, QEMU_CAPS_DRIVE_FORMAT);
+ DO_TEST("disk-drive-error-policy-wreport-rignore", false,
+ QEMU_CAPS_DRIVE, QEMU_CAPS_MONITOR_JSON, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-cache-v2-wt", false,
QEMU_CAPS_DRIVE, QEMU_CAPS_DRIVE_CACHE_V2, QEMU_CAPS_DRIVE_FORMAT);
DO_TEST("disk-drive-cache-v2-wb", false,
--
1.7.3.4
noon
New subject: [libvirt] [PATCHv2 2/2] qemu: add separate rerror_policy for disk errors
On 10/05/2011 09:06 PM, Laine Stump wrote:
Previously libvirt's disk device XML only had a single
attribute,
error_policy, to control both read and write error policy, but qemu
has separate options for controlling read and write. In one case
(enospc) a policy is allowed for write errors but not read errors.
This patch adds a separate attribute that sets only the read error
policy. If just error_policy is set, it will apply to both read and
write error policy (previous behavior), but if the new rerror_policy
attribute is set, it will override error_policy for read errors only.
Possible values for rerror_policy are "stop", "report", and
"ignore"
("report" is the qemu-controlled default for rerror_policy when
error_policy isn't specified).
For consistency, the value "report" has been added to the possible
values for error_policy as well.
ACK. This turned out a lot nicer compared to v1, now that we had time
to think about the issues.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4796
days inactive
4798
days old
10 comments
3 participants
participants (3)
-
Eric Blake -
Laine Stump -
Paolo Bonzini