[libvirt] [PATCH] Introduce startupPolicy for chardev to make guest OS bootable when hardware failure happens in log disk
12:26 p.m.
[Problem]
Currently, guest OS's messages can be logged to a local disk of host OS
by creating chadevs with options below.
-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 case where chardev is "file"
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.
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=e5a84d74a2789a917bf3...
Signed-off-by: Seiji Aguchi <seiji.aguchi(a)hds.com>
---
docs/formatdomain.html.in | 9 ++++++++-
docs/schemas/domaincommon.rng | 3 +++
src/conf/domain_conf.c | 8 ++++++++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_process.c | 25 ++++++++++++++++++++++++-
tests/virt-aa-helper-test | 3 +++
6 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index f325c3c..1e1bf27 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4044,13 +4044,20 @@ 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.
+ It is possible to define policy whether guestOS boots up
+ if the file is not accessible. This is done by a startupPolicy
+ attribute:
+ <ul>
+ <li>If the vaule is "optional", guestOS boots up by dropping the
file.</li>
+ <li>If other values are specified, guestOS fails to boot up. (the
default)</li>
+ </ul>
</p>
<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 10596dc..6fc0a3c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2706,6 +2706,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 a8b5dfd..6680f15 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6467,6 +6467,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
char *path = NULL;
char *mode = NULL;
char *protocol = NULL;
+ char *startupPolicy = NULL;
int remaining = 0;
while (cur != NULL) {
@@ -6487,6 +6488,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:
@@ -6559,6 +6563,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
def->data.file.path = path;
path = NULL;
+
+ def->data.file.startupPolicy =
+ virDomainStartupPolicyTypeFromString(startupPolicy);
+ startupPolicy = NULL;
break;
case VIR_DOMAIN_CHR_TYPE_STDIO:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3a0f23a..e709951 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1052,6 +1052,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 e75c8c9..6e2f78e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2442,7 +2442,30 @@ qemuProcessPrepareChardevDevice(virDomainDefPtr def
ATTRIBUTE_UNUSED,
virReportSystemError(errno,
_("Unable to pre-create chardev file
'%s'"),
dev->source.data.file.path);
- 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");
+ dev->source.data.file.path = strdup("/dev/null");
+
+ if (!(dev->source.data.file.path)) {
+ virReportOOMError();
+ 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;
+ }
}
VIR_FORCE_CLOSE(fd);
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index af91c61..7172fd6 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -255,6 +255,9 @@ testme "0" "disk (empty cdrom)" "-r -u
$valid_uuid" "$test_xml"
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='file'><source
path='$tmpdir/serial.log'/><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
testme "0" "serial" "-r -u $valid_uuid"
"$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='file'><source
path='$tmpdir/serial.log' startupPolicy='optional'/><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
+testme "0" "serial" "-r -u $valid_uuid"
"$test_xml"
+
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='pty'><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
testme "0" "serial (pty)" "-r -u $valid_uuid"
"$test_xml"
--
1.7.1
7:10 p.m.
Any comments?
-----Original Message-----
From: Seiji Aguchi
Sent: Thursday, May 02, 2013 1:26 PM
To: libvir-list(a)redhat.com
Cc: dle-develop(a)lists.sourceforge.net; Tomoki Sekiyama (tomoki.sekiyama(a)hds.com)
Subject: [PATCH] Introduce startupPolicy for chardev to make guest OS bootable when
hardware failure happens in log disk
[Problem]
Currently, guest OS's messages can be logged to a local disk of host OS
by creating chadevs with options below.
-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 case where chardev is "file"
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.
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=e5a84d74a2789a917bf3...
Signed-off-by: Seiji Aguchi <seiji.aguchi(a)hds.com>
---
docs/formatdomain.html.in | 9 ++++++++-
docs/schemas/domaincommon.rng | 3 +++
src/conf/domain_conf.c | 8 ++++++++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_process.c | 25 ++++++++++++++++++++++++-
tests/virt-aa-helper-test | 3 +++
6 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index f325c3c..1e1bf27 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4044,13 +4044,20 @@ 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.
+ It is possible to define policy whether guestOS boots up
+ if the file is not accessible. This is done by a startupPolicy
+ attribute:
+ <ul>
+ <li>If the vaule is "optional", guestOS boots up by dropping the
file.</li>
+ <li>If other values are specified, guestOS fails to boot up. (the
default)</li>
+ </ul>
</p>
<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 10596dc..6fc0a3c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2706,6 +2706,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 a8b5dfd..6680f15 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6467,6 +6467,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
char *path = NULL;
char *mode = NULL;
char *protocol = NULL;
+ char *startupPolicy = NULL;
int remaining = 0;
while (cur != NULL) {
@@ -6487,6 +6488,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:
@@ -6559,6 +6563,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
def->data.file.path = path;
path = NULL;
+
+ def->data.file.startupPolicy =
+ virDomainStartupPolicyTypeFromString(startupPolicy);
+ startupPolicy = NULL;
break;
case VIR_DOMAIN_CHR_TYPE_STDIO:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3a0f23a..e709951 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1052,6 +1052,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 e75c8c9..6e2f78e 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2442,7 +2442,30 @@ qemuProcessPrepareChardevDevice(virDomainDefPtr def
ATTRIBUTE_UNUSED,
virReportSystemError(errno,
_("Unable to pre-create chardev file
'%s'"),
dev->source.data.file.path);
- 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");
+ dev->source.data.file.path = strdup("/dev/null");
+
+ if (!(dev->source.data.file.path)) {
+ virReportOOMError();
+ 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;
+ }
}
VIR_FORCE_CLOSE(fd);
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index af91c61..7172fd6 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -255,6 +255,9 @@ testme "0" "disk (empty cdrom)" "-r -u
$valid_uuid" "$test_xml"
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='file'><source
path='$tmpdir/serial.log'/><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
testme "0" "serial" "-r -u $valid_uuid"
"$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='file'><source
path='$tmpdir/serial.log'
startupPolicy='optional'/><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
+testme "0" "serial" "-r -u $valid_uuid"
"$test_xml"
+
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='pty'><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
testme "0" "serial (pty)" "-r -u $valid_uuid"
"$test_xml"
--
1.7.1
11:07 p.m.
On 05/10/2013 06:10 PM, Seiji Aguchi wrote:
Any comments?
Apologies that no one has spoken up yet, but this is on my list of
things to review. I got waylaid by debugging two nasty races today, or
I would have had time to actually give comments; but come Monday, you
should have some better feedback.
> -----Original Message-----
> From: Seiji Aguchi
> Sent: Thursday, May 02, 2013 1:26 PM
> To: libvir-list(a)redhat.com
> Cc: dle-develop(a)lists.sourceforge.net; Tomoki Sekiyama (tomoki.sekiyama(a)hds.com)
> Subject: [PATCH] Introduce startupPolicy for chardev to make guest OS bootable when
hardware failure happens in log disk
>
> [Problem]
> Currently, guest OS's messages can be logged to a local disk of host OS
> by creating chadevs with options below.
> -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 case where chardev is "file"
> 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.
>
> 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=e5a84d74a2789a917bf3...
>
> Signed-off-by: Seiji Aguchi <seiji.aguchi(a)hds.com>
> ---
> docs/formatdomain.html.in | 9 ++++++++-
> docs/schemas/domaincommon.rng | 3 +++
> src/conf/domain_conf.c | 8 ++++++++
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_process.c | 25 ++++++++++++++++++++++++-
> tests/virt-aa-helper-test | 3 +++
> 6 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index f325c3c..1e1bf27 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4044,13 +4044,20 @@ 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.
> + It is possible to define policy whether guestOS boots up
> + if the file is not accessible. This is done by a startupPolicy
> + attribute:
> + <ul>
> + <li>If the vaule is "optional", guestOS boots up by dropping
the file.</li>
> + <li>If other values are specified, guestOS fails to boot up. (the
default)</li>
> + </ul>
> </p>
>
> <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 10596dc..6fc0a3c 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2706,6 +2706,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 a8b5dfd..6680f15 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6467,6 +6467,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> char *path = NULL;
> char *mode = NULL;
> char *protocol = NULL;
> + char *startupPolicy = NULL;
> int remaining = 0;
>
> while (cur != NULL) {
> @@ -6487,6 +6488,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:
> @@ -6559,6 +6563,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>
> def->data.file.path = path;
> path = NULL;
> +
> + def->data.file.startupPolicy =
> + virDomainStartupPolicyTypeFromString(startupPolicy);
> + startupPolicy = NULL;
> break;
>
> case VIR_DOMAIN_CHR_TYPE_STDIO:
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3a0f23a..e709951 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1052,6 +1052,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 e75c8c9..6e2f78e 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -2442,7 +2442,30 @@ qemuProcessPrepareChardevDevice(virDomainDefPtr def
ATTRIBUTE_UNUSED,
> virReportSystemError(errno,
> _("Unable to pre-create chardev file
'%s'"),
> dev->source.data.file.path);
> - 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");
> + dev->source.data.file.path = strdup("/dev/null");
> +
> + if (!(dev->source.data.file.path)) {
> + virReportOOMError();
> + 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;
> + }
> }
>
> VIR_FORCE_CLOSE(fd);
> diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
> index af91c61..7172fd6 100755
> --- a/tests/virt-aa-helper-test
> +++ b/tests/virt-aa-helper-test
> @@ -255,6 +255,9 @@ testme "0" "disk (empty cdrom)" "-r -u
$valid_uuid" "$test_xml"
> sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='file'><source
> path='$tmpdir/serial.log'/><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
> testme "0" "serial" "-r -u $valid_uuid"
"$test_xml"
>
> +sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='file'><source
path='$tmpdir/serial.log'
> startupPolicy='optional'/><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
> +testme "0" "serial" "-r -u $valid_uuid"
"$test_xml"
> +
> sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='pty'><target
> port='0'/></serial></devices>,g"
"$template_xml" > "$test_xml"
> testme "0" "serial (pty)" "-r -u $valid_uuid"
"$test_xml"
>
> --
> 1.7.1
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:38 p.m.
On 05/02/2013 11:26 AM, Seiji Aguchi wrote:
The subject line is a bit long for a patch. 'git shortlog -30' will
give you an idea of typical subject lines; I'm modifying this to:
xml: introduce startupPolicy for chardev device
[Problem]
Currently, guest OS's messages can be logged to a local disk of host OS
by creating chadevs with options below.
-chardev file,id=charserial0,path=<log file's path> -device
isa-serial,chardev=chardevserial0,id=serial0
This long line is okay (quoting a command line)...
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
...but here, wrapping below 80 characters is desirable. Also, space
after '.' ending a sentence, in English. So I rewrapped it.
disk is broken.
[Solution]
This patch supports startupPolicy for chardev.
The starupPolicy is introduced just in case where chardev is "file"
s/case/cases/
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.
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=e5a84d74a2789a917bf3...
Signed-off-by: Seiji Aguchi <seiji.aguchi(a)hds.com>
---
docs/formatdomain.html.in | 9 ++++++++-
docs/schemas/domaincommon.rng | 3 +++
Good - adding documentation alongside the feature.
src/conf/domain_conf.c | 8 ++++++++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_process.c | 25 ++++++++++++++++++++++++-
It's a bit nicer to split the XML addition and a driver's implementation
of the XML (makes backporting easier if we later implement in a
different driver, and want to backport the second implementation without
the first); but this patch is small enough that I'm not too worried
about it.
tests/virt-aa-helper-test | 3 +++
This added one test, but not quite enough. I generally also expect a
.xml and .args that shows the new XML in use, and that there is no
corresponding change to a generated qemu command line (as the automatic
dropping is handled solely by libvirt reworking the XML). I can
probably do that on your behalf.
6 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index f325c3c..1e1bf27 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4044,13 +4044,20 @@ 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.
+ It is possible to define policy whether guestOS boots up
s/guestOS/the guest/
+ if the file is not accessible. This is done by a
startupPolicy
+ attribute:
+ <ul>
Fails 'make check' with:
Validating formatdomain.html
formatdomain.html.tmp:4516: element p: validity error : Element ul is
not declared in p list of possible children
</p>
^
Solved by closing the <p> before starting the <ul>, or ditching <ul>
altogether.
+ <li>If the vaule is "optional", guestOS boots
up by dropping the file.</li>
s/vaule/value/; s/guestOS/guest/
+ <li>If other values are specified, guestOS fails to boot
up. (the default)</li>
+ </ul>
Needs a "since 1.0.6" notation.
Rather than leaving it as 'optional' and a nebulous 'anything else',
I'd
rather we match existing practice, and specifically call out the same
states as elsewhere ('optional'|'requisite'|'mandatory');
especially
since your RNG change reused that same enum.
+++ b/src/qemu/qemu_process.c
@@ -2442,7 +2442,30 @@ qemuProcessPrepareChardevDevice(virDomainDefPtr def
ATTRIBUTE_UNUSED,
virReportSystemError(errno,
_("Unable to pre-create chardev file
'%s'"),
dev->source.data.file.path);
- return -1;
+ if (dev->source.data.file.startupPolicy !=
+ VIR_DOMAIN_STARTUP_POLICY_OPTIONAL) {
+ return -1;
+ }
This issues an error even when policy said we want a replacement and
thus succeed; we only want to issue an error when we can't succeed, or
else clear the error when we try the fallback.
+ 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.
Not the typical comment style, and should come before we release the old
name.
+ */
+ VIR_WARN("Switch the destination to /dev/null");
+ dev->source.data.file.path = strdup("/dev/null");
+
+ if (!(dev->source.data.file.path)) {
+ virReportOOMError();
+ return -1;
+ }
VIR_STRDUP has gone in since you posted, simplifying this.
+
+ if ((fd = open(dev->source.data.file.path,
+ O_CREAT | O_APPEND, S_IRUSR|S_IWUSR)) < 0) {
O_CREAT and O_APPEND are pointless now that we know we are opening
/dev/null. POSIX requires that you use at least one of O_RDONLY,
O_WRONLY, or O_RDWR (yes, I know you were copying existing bad code, but
no need to repeat that mistake). Just because O_RDONLY is 0 on Linux
doesn't make it correct to omit an access mode in your code. For that
matter, we KNOW that /dev/null already exists (POSIX says it does, and
any system where it doesn't has lots more problems on its hand), so we
can exit early as soon as we've malloc'd the replacement file name.
+ virReportSystemError(errno,
+ _("Unable to pre-create chardev file
'%s'"),
Since you've already changed the file name, this error message is not as
useful as if it were the user's original file name; but changing that is
a bit harder.
Here's what I've changed so far; I won't push until I've also added a
testcase that proves xml2xml round-trip works on the new attribute (but
it's late for me on a holiday weekend, so that might not be until
Tuesday). Since I've reviewed the patch prior to code freeze for 1.0.6,
I think it will be okay pushing next week even if I don't finish the
testsuite addition before DV does the freeze.
Thanks again for a first time contribution, and apologies for the delays
in getting it reviewed (I think others will agree with me that we've had
a LOT of list traffic lately).
Interdiff to your original mail (I'll send the complete patch, as-is, as
a reply).
diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in
index 92fc496..299fa49 100644
--- i/docs/formatdomain.html.in
+++ w/docs/formatdomain.html.in
@@ -4212,14 +4212,20 @@ 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.
- It is possible to define policy whether guestOS boots up
- if the file is not accessible. This is done by a startupPolicy
- attribute:
- <ul>
- <li>If the vaule is "optional", guestOS boots up by dropping the
file.</li>
- <li>If other values are specified, guestOS fails to boot up. (the
default)</li>
- </ul>
+ <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>
+ </ul>
<pre>
...
diff --git i/src/qemu/qemu_process.c w/src/qemu/qemu_process.c
index 1bc3305..154445d 100644
--- i/src/qemu/qemu_process.c
+++ w/src/qemu/qemu_process.c
@@ -2439,38 +2439,24 @@ qemuProcessPrepareChardevDevice(virDomainDefPtr
def ATTRIBUTE_UNUSED,
return 0;
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);
+ O_CREAT | O_APPEND | O_RDONLY, S_IRUSR|S_IWUSR)) < 0) {
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");
- dev->source.data.file.path = strdup("/dev/null");
-
- if (!(dev->source.data.file.path)) {
- virReportOOMError();
- 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;
}
+ /* Change destination to /dev/null to work around missing file. */
+ VIR_WARN("chardev file '%s' not found, switching to
/dev/null",
+ dev->source.data.file.path);
+ VIR_FREE(dev->source.data.file.path);
+ if (VIR_STRDUP(dev->source.data.file.path, "/dev/null") < 0)
+ return -1;
+ } else {
+ VIR_FORCE_CLOSE(fd);
}
- VIR_FORCE_CLOSE(fd);
-
return 0;
}
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:46 p.m.
New subject: [libvirt] [PATCHv2] xml: introduce startupPolicy for chardev device
From: Seiji Aguchi <seiji.aguchi(a)hds.com>
[Problem]
Currently, guest OS's messages can be logged to a local disk of host OS
by creating chadevs with options below.
-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"
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.
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>
---
Seiji's patch plus my initial review comments; I've ack'd this much,
but want to add a round-trip xml2xml test before pushing anything
(Seiji, if you want to use this as a starting point, rather than
waiting for my long weekend, feel free to post a v3).
docs/formatdomain.html.in | 15 ++++++++++++++-
docs/schemas/domaincommon.rng | 3 +++
src/conf/domain_conf.c | 8 ++++++++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_process.c | 23 ++++++++++++++++-------
tests/virt-aa-helper-test | 3 +++
6 files changed, 45 insertions(+), 8 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 755d084..299fa49 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -4212,13 +4212,26 @@ 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>
+ </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 3cace35..e8eec6c 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -2780,6 +2780,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 a9656af..c24a9f0 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6626,6 +6626,7 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
char *path = NULL;
char *mode = NULL;
char *protocol = NULL;
+ char *startupPolicy = NULL;
int remaining = 0;
while (cur != NULL) {
@@ -6646,6 +6647,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:
@@ -6718,6 +6722,10 @@ virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
def->data.file.path = path;
path = NULL;
+
+ def->data.file.startupPolicy =
+ virDomainStartupPolicyTypeFromString(startupPolicy);
+ startupPolicy = NULL;
break;
case VIR_DOMAIN_CHR_TYPE_STDIO:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 3a71d6c..4d49dd5 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1074,6 +1074,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 d4fd4fb..154445d 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -2439,15 +2439,24 @@ qemuProcessPrepareChardevDevice(virDomainDefPtr def
ATTRIBUTE_UNUSED,
return 0;
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;
+ O_CREAT | O_APPEND | O_RDONLY, S_IRUSR|S_IWUSR)) < 0) {
+ if (dev->source.data.file.startupPolicy !=
+ VIR_DOMAIN_STARTUP_POLICY_OPTIONAL) {
+ virReportSystemError(errno,
+ _("Unable to pre-create chardev file
'%s'"),
+ dev->source.data.file.path);
+ return -1;
+ }
+ /* Change destination to /dev/null to work around missing file. */
+ VIR_WARN("chardev file '%s' not found, switching to
/dev/null",
+ dev->source.data.file.path);
+ VIR_FREE(dev->source.data.file.path);
+ if (VIR_STRDUP(dev->source.data.file.path, "/dev/null") < 0)
+ return -1;
+ } else {
+ VIR_FORCE_CLOSE(fd);
}
- VIR_FORCE_CLOSE(fd);
-
return 0;
}
diff --git a/tests/virt-aa-helper-test b/tests/virt-aa-helper-test
index af91c61..7172fd6 100755
--- a/tests/virt-aa-helper-test
+++ b/tests/virt-aa-helper-test
@@ -255,6 +255,9 @@ testme "0" "disk (empty cdrom)" "-r -u
$valid_uuid" "$test_xml"
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='file'><source
path='$tmpdir/serial.log'/><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
testme "0" "serial" "-r -u $valid_uuid"
"$test_xml"
+sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='file'><source
path='$tmpdir/serial.log' startupPolicy='optional'/><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
+testme "0" "serial" "-r -u $valid_uuid"
"$test_xml"
+
sed -e "s,###UUID###,$uuid,g" -e "s,###DISK###,$disk1,g" -e
"s,</devices>,<serial type='pty'><target
port='0'/></serial></devices>,g" "$template_xml"
> "$test_xml"
testme "0" "serial (pty)" "-r -u $valid_uuid"
"$test_xml"
--
1.8.1.4
6:07 p.m.
New subject: [libvirt] [PATCHv2] xml: introduce startupPolicy for chardev device
On 05/24/2013 04:46 PM, Eric Blake wrote:
From: Seiji Aguchi <seiji.aguchi(a)hds.com>
[Problem]
Currently, guest OS's messages can be logged to a local disk of host OS
by creating chadevs with options below.
-chardev file,id=charserial0,path=<log file's path> -device
isa-serial,chardev=chardevserial0,id=serial0
Seiji's patch plus my initial review comments; I've ack'd this much,
but want to add a round-trip xml2xml test before pushing anything
(Seiji, if you want to use this as a starting point, rather than
waiting for my long weekend, feel free to post a v3).
Sorry, but this needs a v3, and I'm going to hand it back to you to fix.
I tried adding a testsuite, but it proved that you weren't outputting
the startupPolicy during dumpxml formatting. My initial attempt to add
it backfired (I'll post it below); I'm worried that we are playing too
fast and loose with a union type, since even with my patch in place,
'make check' fails with problems like:
84) QEMU XML-2-XML serial-dev ...
Offset 913
Expect [/>
<target port='0'/>
</serial>
<console type='dev'>
<source path='/dev/ttyS2]
Actual [ startupPolicy='(null)'/>
<target port='0'/>
</serial>
<console type='dev'>
<source path='/dev/ttyS2' startupPolicy='(null)]
which is a bug, since you said startupPolicy should only be tied to
files and not other <serial> types. Do we need to repeat the
startupPolicy on the <console> mirror of the first <serial> device, or
is that unnecessary back-compat, and where we can safely declare that
startupPolicy is only emitted for the <serial> side of things?
All told, your patch is not complete until it passes 'make check' with a
new xml2xml test that proves we can round trip the new policy.
diff --git c/src/conf/domain_conf.c w/src/conf/domain_conf.c
index c24a9f0..5af5e40 100644
--- c/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -14547,8 +14547,14 @@ 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",
+ 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
c/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
new file mode 100644
index 0000000..24977f2
--- /dev/null
+++ w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
@@ -0,0 +1,8 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu
-S -M \
+pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-no-acpi -boot c -usb -hdc /tmp/idedisk.img \
+-chardev file,id=charserial0,path=/tmp/serial.log \
+-device isa-serial,chardev=charserial0,id=serial0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git
c/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
new file mode 100644
index 0000000..34e0eb9
--- /dev/null
+++ w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
@@ -0,0 +1,35 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>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='file' device='disk'>
+ <source file='/tmp/idedisk.img'/>
+ <target dev='hdc' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='2'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <serial type='file'>
+ <source path='/tmp/serial.log' startupPolicy='optional'/>
+ <target port='0'/>
+ </serial>
+ <console type='file'>
+ <source path='/tmp/serial.log'/>
+ <target type='serial' port='0'/>
+ </console>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git c/tests/qemuxml2argvtest.c w/tests/qemuxml2argvtest.c
index 2c7fd01..22f4782 100644
--- c/tests/qemuxml2argvtest.c
+++ w/tests/qemuxml2argvtest.c
@@ -727,6 +727,8 @@ mymain(void)
QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
DO_TEST("console-compat-chardev",
QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
+ DO_TEST("serial-source-optional",
+ QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
DO_TEST("channel-guestfwd",
QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
diff --git c/tests/qemuxml2xmltest.c w/tests/qemuxml2xmltest.c
index 64a271c..1147519 100644
--- c/tests/qemuxml2xmltest.c
+++ w/tests/qemuxml2xmltest.c
@@ -86,6 +86,8 @@ testCompareXMLToXMLHelper(const void *data)
ret = testCompareXMLToXMLFiles(xml_in,
info->different ? xml_out : xml_in,
false);
+ if (ret < 0)
+ goto cleanup;
}
if (info->when & WHEN_ACTIVE) {
ret = testCompareXMLToXMLFiles(xml_in,
@@ -231,6 +233,7 @@ mymain(void)
DO_TEST("console-virtio-many");
DO_TEST("channel-guestfwd");
DO_TEST("channel-virtio");
+ DO_TEST("serial-source-optional");
DO_TEST("hostdev-usb-address");
DO_TEST("hostdev-pci-address");
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:14 p.m.
New subject: [libvirt] [PATCHv2] xml: introduce startupPolicy for chardev device
Eric,
-----Original Message-----
From: libvir-list-bounces(a)redhat.com [mailto:libvir-list-bounces@redhat.com] On Behalf Of
Eric Blake
Sent: Tuesday, May 28, 2013 7:08 PM
Cc: libvir-list(a)redhat.com; dle-develop(a)lists.sourceforge.net; Tomoki Sekiyama
Subject: Re: [libvirt] [PATCHv2] xml: introduce startupPolicy for chardev device
On 05/24/2013 04:46 PM, Eric Blake wrote:
> From: Seiji Aguchi <seiji.aguchi(a)hds.com>
>
> [Problem]
> Currently, guest OS's messages can be logged to a local disk of host OS
> by creating chadevs with options below.
> -chardev file,id=charserial0,path=<log file's path> -device
isa-serial,chardev=chardevserial0,id=serial0
>
> Seiji's patch plus my initial review comments; I've ack'd this much,
> but want to add a round-trip xml2xml test before pushing anything
> (Seiji, if you want to use this as a starting point, rather than
> waiting for my long weekend, feel free to post a v3).
Sorry, but this needs a v3, and I'm going to hand it back to you to fix.
I tried adding a testsuite, but it proved that you weren't outputting
the startupPolicy during dumpxml formatting. My initial attempt to add
it backfired (I'll post it below);
Thank you for handling/testing my patch.
I'm worried that we are playing too
fast and loose with a union type, since even with my patch in place,
'make check' fails with problems like:
84) QEMU XML-2-XML serial-dev ...
Offset 913
Expect [/>
<target port='0'/>
</serial>
<console type='dev'>
<source path='/dev/ttyS2]
Actual [ startupPolicy='(null)'/>
<target port='0'/>
</serial>
<console type='dev'>
<source path='/dev/ttyS2' startupPolicy='(null)]
which is a bug, since you said startupPolicy should only be tied to
files and not other <serial> types. Do we need to repeat the
startupPolicy on the <console> mirror of the first <serial> device, or
is that unnecessary back-compat, and where we can safely declare that
startupPolicy is only emitted for the <serial> side of things?
I need to think carefully about it before deciding it in a hurry.
All told, your patch is not complete until it passes 'make
check' with a
new xml2xml test that proves we can round trip the new policy.
Anyway, I will post the v3 patch by fixing the issue.
Seiji
diff --git c/src/conf/domain_conf.c w/src/conf/domain_conf.c
index c24a9f0..5af5e40 100644
--- c/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -14547,8 +14547,14 @@ 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",
+ 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
c/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
new file mode 100644
index 0000000..24977f2
--- /dev/null
+++ w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.args
@@ -0,0 +1,8 @@
+LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu
-S -M \
+pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults \
+-chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait \
+-mon chardev=charmonitor,id=monitor,mode=readline \
+-no-acpi -boot c -usb -hdc /tmp/idedisk.img \
+-chardev file,id=charserial0,path=/tmp/serial.log \
+-device isa-serial,chardev=charserial0,id=serial0 \
+-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3
diff --git
c/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
new file mode 100644
index 0000000..34e0eb9
--- /dev/null
+++ w/tests/qemuxml2argvdata/qemuxml2argv-serial-source-optional.xml
@@ -0,0 +1,35 @@
+<domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>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='file' device='disk'>
+ <source file='/tmp/idedisk.img'/>
+ <target dev='hdc' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='2'/>
+ </disk>
+ <controller type='usb' index='0'/>
+ <controller type='ide' index='0'/>
+ <controller type='pci' index='0' model='pci-root'/>
+ <serial type='file'>
+ <source path='/tmp/serial.log' startupPolicy='optional'/>
+ <target port='0'/>
+ </serial>
+ <console type='file'>
+ <source path='/tmp/serial.log'/>
+ <target type='serial' port='0'/>
+ </console>
+ <memballoon model='virtio'/>
+ </devices>
+</domain>
diff --git c/tests/qemuxml2argvtest.c w/tests/qemuxml2argvtest.c
index 2c7fd01..22f4782 100644
--- c/tests/qemuxml2argvtest.c
+++ w/tests/qemuxml2argvtest.c
@@ -727,6 +727,8 @@ mymain(void)
QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
DO_TEST("console-compat-chardev",
QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
+ DO_TEST("serial-source-optional",
+ QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
DO_TEST("channel-guestfwd",
QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG);
diff --git c/tests/qemuxml2xmltest.c w/tests/qemuxml2xmltest.c
index 64a271c..1147519 100644
--- c/tests/qemuxml2xmltest.c
+++ w/tests/qemuxml2xmltest.c
@@ -86,6 +86,8 @@ testCompareXMLToXMLHelper(const void *data)
ret = testCompareXMLToXMLFiles(xml_in,
info->different ? xml_out : xml_in,
false);
+ if (ret < 0)
+ goto cleanup;
}
if (info->when & WHEN_ACTIVE) {
ret = testCompareXMLToXMLFiles(xml_in,
@@ -231,6 +233,7 @@ mymain(void)
DO_TEST("console-virtio-many");
DO_TEST("channel-guestfwd");
DO_TEST("channel-virtio");
+ DO_TEST("serial-source-optional");
DO_TEST("hostdev-usb-address");
DO_TEST("hostdev-pci-address");
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4195
days inactive
4222
days old
6 comments
2 participants
participants (2)
-
Eric Blake -
Seiji Aguchi