[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3] xml: introduce startupPolicy for chardev device



Jan,

Thank you for reviewing.
I will fix the patch in accordance with your comments.

Seiji


> -----Original Message-----
> From: Ján Tomko [mailto:jtomko 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 aguchi hds com>
> > Signed-off-by: Eric Blake <eblake 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>
> >    ...
> >    &lt;devices&gt;
> >      &lt;serial type="file"&gt;
> > -      &lt;source path="/var/log/vm/vm-serial.log"/&gt;
> > +      &lt;source path="/var/log/vm/vm-serial.log" startupPolicy="optional"/&gt;
> >        &lt;target port="1"/&gt;
> >      &lt;/serial&gt;
> >    &lt;/devices&gt;
> > 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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]