[libvirt] [PATCH 2/2] qemu: add separate rerror_policy for disk errors

Tue Oct 4 19:54:37 UTC 2011

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.

> -Since 0.8.0
> + how the hypervisor will behave on a disk read or write
> + error, possible values are "stop", "ignore", and
> + "enospace".Since 0.8.0 There is
> + also an optional<code>rerror_policy</code> that controls
> + behavior for read errors only.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 at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


