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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list