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.agu...@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
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
