Re: [PATCH 4/8] Support startupPolicy for 'volume' disk

[Osier Yang <jyang@xxxxxxxxxx>]

On 06/04/13 07:41, John Ferlan wrote:
> On 04/04/2013 03:37 PM, Osier Yang wrote:
> > "startupPolicy" is only valid for file type storage volume, otherwise
> > it fails on starting the domain.
> > ---
> >   docs/formatdomain.html.in                                | 7 ++++---
> >   docs/schemas/domaincommon.rng                            | 3 +++
> >   src/conf/domain_conf.c                                   | 9 +++++++--
> >   src/qemu/qemu_command.c                                  | 7 +++++++
> >   tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml | 2 +-
> >   5 files changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index bdc815f..ce185a9 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -1467,11 +1467,12 @@
> >           0.7.5; <code>type='network'</code> since
> >           0.8.7; <code>protocol='iscsi'</code> since 1.0.4;
> >           <code>type='volume'</code> since 1.0.5;</span><br/>
> > -        For a "file" disk type which represents a cdrom or floppy
> > +        For a "file" or "volume" disk type which represents a cdrom or floppy
> >           (the <code>device</code> attribute), it is possible to define
> >           policy what to do with the disk if the source file is not accessible.
> > -        This is done by the <code>startupPolicy</code> attribute, accepting
> > -        these values:
> > +        (NB, <code>startupPolicy</code> is not valid for "volume" disk unless
> > +         the specified storage volume is of "file" type). This is done by the
> > +        <code>startupPolicy</code> attribute, accepting these values:
> >           <table class="top_table">
> >             <tr>
> >               <td> mandatory </td>
> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> > index 46cccc4..4e7e712 100644
> > --- a/docs/schemas/domaincommon.rng
> > +++ b/docs/schemas/domaincommon.rng
> > @@ -1103,6 +1103,9 @@
> >                   <attribute name="volume">
> >                     <ref name="volName"/>
> >                   </attribute>
> > +                <optional>
> > +                  <ref name="startupPolicy"/>
> > +                </optional>
> >                 </element>
> >               </optional>
> >               <ref name="diskspec"/>
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 8538d5f..c1d2cbb 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -4180,6 +4180,7 @@ virDomainDiskDefParseXML(virCapsPtr caps,
> >                   case VIR_DOMAIN_DISK_TYPE_VOLUME:
> >                       if (virDomainDiskSourcePoolDefParse(cur, def) < 0)
> >                           goto error;
> > +                    startupPolicy = virXMLPropString(cur, "startupPolicy");
> Is there no way later on in this module to validate that the
> startupPolicy for a DISK_TYPE_VOLUME is for a file only?  Seems a shame
> to wait for building the qemu command to tell someone the bad news.

That's because we translate the source in the driver. Translating when
parsing doesn't make sense, as inactive domain even might don't want
to have the disk source existing.

> John
> >                       break;
> >                   default:
> >                       virReportError(VIR_ERR_INTERNAL_ERROR,
> > @@ -12890,9 +12891,13 @@ virDomainDiskSourceDefFormat(virBufferPtr buf,
> >               /* Parsing guarantees the def->srcpool->volume cannot be NULL
> >                * if def->srcpool->pool is not NULL.
> >                */
> > -            if (def->srcpool->pool)
> > -                virBufferAsprintf(buf, "      <source pool='%s' volume='%s'/>\n",
> > +            if (def->srcpool)
> > +                virBufferAsprintf(buf, "      <source pool='%s' volume='%s'",
> >                                     def->srcpool->pool, def->srcpool->volume);
> > +            if (def->startupPolicy)
> > +                virBufferEscapeString(buf, " startupPolicy='%s'/>\n", startupPolicy);
> > +            else
> > +                virBufferAddLit(buf, "/>\n");
> >               break;
> >           default:
> >               virReportError(VIR_ERR_INTERNAL_ERROR,
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 03c7195..cdfe801 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -2703,6 +2703,13 @@ qemuTranslateDiskSourcePool(virConnectPtr conn,
> >       if (virStorageVolGetInfo(vol, &info) < 0)
> >           goto cleanup;
> >   
> > +    if (def->startupPolicy &&
> > +        info.type != VIR_STORAGE_VOL_FILE) {
> > +        virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                       _("'startupPolicy' is only valid for 'file' type volume"));
> > +        goto cleanup;
> > +    }
> > +
> >       switch (info.type) {
> >       case VIR_STORAGE_VOL_FILE:
> >       case VIR_STORAGE_VOL_BLOCK:
> > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
> > index 876eebe..a218e78 100644
> > --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
> > +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-source-pool.xml
> > @@ -15,7 +15,7 @@
> >     <devices>
> >       <emulator>/usr/bin/qemu</emulator>
> >       <disk type='volume' device='cdrom'>
> > -      <source pool='blk-pool0' volume='blk-pool0-vol0'/>
> > +      <source pool='blk-pool0' volume='blk-pool0-vol0' startupPolicy='optional'/>
> >         <target dev='hda' bus='ide'/>
> >         <readonly/>
> >         <address type='drive' controller='0' bus='0' target='0' unit='1'/>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list