[libvirt] [PATCH 0/2] qemu: fix disk error_policy problems
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).
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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.
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Laine Stump -
Paolo Bonzini