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(a)hds.com>
Signed-off-by: Eric Blake <eblake(a)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