[libvirt] [PATCH] Introduce startupPolicy for chardev to make guest OS bootable when hardware failure happens in log disk
Eric Blake
eblake at redhat.com
Fri May 24 22:38:26 UTC 2013
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=e5a84d74a2789a917bf394f15de9989ec48fded0
>
> Signed-off-by: Seiji Aguchi <seiji.aguchi at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130524/ec1d3ac6/attachment-0001.sig>
More information about the libvir-list
mailing list