On 07/24/2013 10:05 PM, Seiji Aguchi wrote:
[Problem] Currently, guest OS's messages can be logged to a local disk of host OS by creating chadevs with options below.
s/chadevs/chardevs/
-chardev file,id=charserial0,path=<log file's path> -device isa-serial,chardev=chardevserial0,id=serial0
When a hardware failure happens in the disk, qemu-kvm can't create the chardevs. In this case, guest OS doesn't boot up.
Actually, there are users who don't desire that guest OS goes down due to a hardware failure of a log disk only. Therefore, qemu should offer some way to boot guest OS up even if the log disk is broken.
[Solution] This patch supports startupPolicy for chardev.
The starupPolicy is introduced just in cases where chardev is "file"
s/starupPolicy/startupPolicy/
because this patch aims for making guest OS boot up when a hardware failure happens.
In other cases (pty, dev, pipe and unix) it is not introduced because they don't access to hardware.
s/to//
The policy works as follows. - If the value is "optional", guestOS boots up by dropping the chardev. - If other values are specified, guestOS fails to boot up. (the default)
Description about original startupPolicy attribute: http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=e5a84d74a278
Signed-off-by: Seiji Aguchi <seiji.aguchi@hds.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- Change from v2 - To pass "make check", add followings. - Add serial source optional testing. - check if startupPolicy is NULL in virDomainChrSourceDefParseXML(). - Add xml format of startupPolicy in virDomainChrSourceDefFormat().
Patch v2 and comment from Eric Blake - https://www.redhat.com/archives/libvir-list/2013-May/msg01814.html - https://www.redhat.com/archives/libvir-list/2013-May/msg01943.html --- docs/formatdomain.html.in | 16 ++++++++- docs/schemas/domaincommon.rng | 3 ++ src/conf/domain_conf.c | 22 +++++++++++- src/conf/domain_conf.h | 1 + src/qemu/qemu_process.c | 25 +++++++++++++- .../qemuxml2argv-serial-source-optional.args | 9 +++++ .../qemuxml2argv-serial-source-optional.xml | 35 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 1 + tests/virt-aa-helper-test | 3 ++ 10 files changed, 113 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 7601aaa..5c9d4fb 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -4281,13 +4281,27 @@ qemu-kvm -net nic,model=? /dev/null <p> A file is opened and all data sent to the character device is written to the file. + <span class="since">Since 1.0.6</span>, it is possible to define + policy on what happens if the file is not accessible when + booting or migrating. This is done by + a <code>startupPolicy</code> attribute: </p>
+ <ul> + <li>If the value is "mandatory" (the default), the guest fails + to boot or migrate if the file is not found.</li> + <li>If the value is "optional", a missing file is at boot or + migration is substituted with /dev/null, so the guest still sees + the device but the host no longer tracks guest data on the device.</li>
+ <li>If the value is "requisite", the file is required for + booting, but optional on migration.</li>
This isn't mentioned in the commit message or implemented in the patch. We should either implement it or reject the "requisite" value.
+ </ul> + <pre> ... <devices> <serial type="file"> - <source path="/var/log/vm/vm-serial.log"/> + <source path="/var/log/vm/vm-serial.log" startupPolicy="optional"/> <target port="1"/> </serial> </devices> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 745b959..10b3365 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2817,6 +2817,9 @@ </optional> <optional> <attribute name="path"/> + <optional> + <ref name="startupPolicy"/> + </optional> </optional> <optional> <attribute name="host"/> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10cb7f6..279ff9e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6819,6 +6819,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, char *path = NULL; char *mode = NULL; char *protocol = NULL; + char *startupPolicy = NULL; int remaining = 0;
while (cur != NULL) { @@ -6839,6 +6840,9 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def, !(flags & VIR_DOMAIN_XML_INACTIVE))) path = virXMLPropString(cur, "path");
+ if (startupPolicy == NULL && + def->type == VIR_DOMAIN_CHR_TYPE_FILE) + startupPolicy = virXMLPropString(cur, "startupPolicy"); break;
case VIR_DOMAIN_CHR_TYPE_UDP: @@ -6911,6 +6915,13 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
def->data.file.path = path; path = NULL; + + if (startupPolicy) { + def->data.file.startupPolicy = + virDomainStartupPolicyTypeFromString(startupPolicy);
+ startupPolicy = NULL;
This line is pointless, the value of startupPolicy is not used after. We need to VIR_FREE(startupPolicy) in the cleanup section instead, since virXMLPropString allocates memory.
+ } + break;
case VIR_DOMAIN_CHR_TYPE_STDIO: @@ -15014,8 +15025,15 @@ virDomainChrSourceDefFormat(virBufferPtr buf, if (def->type != VIR_DOMAIN_CHR_TYPE_PTY || (def->data.file.path && !(flags & VIR_DOMAIN_XML_INACTIVE))) { - virBufferEscapeString(buf, " <source path='%s'/>\n", - def->data.file.path); + virBufferEscapeString(buf, " <source path='%s'", + def->data.file.path); + + if (def->data.file.path && def->data.file.startupPolicy) { + const char *policy = +virDomainStartupPolicyTypeToString(def->data.file.startupPolicy); + virBufferAsprintf(buf, " startupPolicy='%s'", policy); + } + virBufferAddLit(buf, "/>\n"); } break;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f265966..0899556 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1102,6 +1102,7 @@ struct _virDomainChrSourceDef { /* no <source> for null, vc, stdio */ struct { char *path; + int startupPolicy; /* enum virDomainStartupPolicy */ } file; /* pty, file, pipe, or device */ struct { char *host; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a46d944..35d63d5 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2511,7 +2511,30 @@ qemuProcessPrepareChardevDevice(virDomainDefPtr def ATTRIBUTE_UNUSED, virReportSystemError(errno, _("Unable to pre-create chardev file '%s'"), dev->source.data.file.path);
I think we should only report this error when startupPolicy is mandatory...
- return -1; + if (dev->source.data.file.startupPolicy != + VIR_DOMAIN_STARTUP_POLICY_OPTIONAL) { + return -1; + } + VIR_FREE(dev->source.data.file.path); + /* + * Change a destination to /dev/null to boot guest OS up + * even if a log disk is broken. + */ + VIR_WARN("Switch the destination to /dev/null");
.. and extend this warning to include the path, or even the system error, or we should call virResetLastError here.
+ dev->source.data.file.path = strdup("/dev/null"); + + if (!(dev->source.data.file.path)) { + virReportOOMError(); + return -1; + } +
This can be reduced to: if (VIR_STRDUP(dev->source.data.file.path, "/dev/null") < 0) return -1;
+ if ((fd = open(dev->source.data.file.path, + O_CREAT | O_APPEND, S_IRUSR|S_IWUSR)) < 0) { + virReportSystemError(errno, + _("Unable to pre-create chardev file '%s'"), + dev->source.data.file.path); + return -1; + } }
I think we can safely assume /dev/null to exist on any system where QEMU runs and there's no need to pre-create it. Jan