Jan, Thank you for reviewing. I will fix the patch in accordance with your comments.
Seiji > -----Original Message----- > From: Ján Tomko [mailto:jto...@redhat.com] > Sent: Thursday, August 15, 2013 5:36 AM > To: Seiji Aguchi > Cc: libvir-list@redhat.com > Subject: Re: [libvirt] [PATCH v3] xml: introduce startupPolicy for chardev > device > > 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.agu...@hds.com> > > Signed-off-by: Eric Blake <ebl...@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
