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

On 10/04/2011 12:35 PM, Laine Stump wrote: 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. 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. 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"> 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 } 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