[libvirt] [PATCH 3/8] docs: Add docs and rng schema for new XML cdbfilter

[Eric Blake]

On 12/13/2012 12:05 PM, Osier Yang wrote:
> Since "rawio" and "cdbfilter" are only valid for "lun", this
> groups them together; And since both of them intend to allow
> the unprivledged user to use the SG_IO commands, they must be

s/unprivledged/unprivileged/

> exclusive.
> ---
>  docs/formatdomain.html.in     |   13 +++++++++-
>  docs/schemas/domaincommon.rng |   52 ++++++++++++++++++++++++++++------------
>  2 files changed, 48 insertions(+), 17 deletions(-)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 8e234fd..8c7c682 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1395,7 +1395,18 @@
>          rawio='yes', rawio capability will be enabled for all disks in
>          the domain (because, in the case of QEMU, this capability can
>          only be set on a per-process basis). This attribute is only
> -        valid when device is "lun".
> +        valid when device is "lun". NB, <code>rawio</code> intends to
> +        confine the capability per-device, however, current QEMU
> +        implementation gives the domain process broader capability
> +        than that (per-process basis, affects all the domain disks).
> +        To confine the capability as much as possible for QEMU driver
> +        as this stage, <code>cdbfilter</code> is recommended.
> +        The optional <code>cdbfilter</code> attribute
> +        (<span class="since">since 1.0.1</span>) indicates whether the
> +        kernel will filter unprivileged SG_IO for the disk, valid settings
> +        are "yes" or "no" (defaults to "yes"). Note that it's exclusive
> +        with attribute <code>rawio</code>; Same with <code>rawio</code>,
> +        <code>cdbfilter</code> is only valid for device 'lun'.

Hmm, this doesn't seem like the smartest of designs.  If I'm
understanding you right, we have:

rawio     cdbfilter      result
missing   missing        kernel prevents SG_IO
missing   no             kernel prevents SG_IO
missing   yes            SG_IO allowed for disk
no        missing        kernel prevents SG_IO
no        no             kernel prevents SG_IO
no        yes            SG_IO allowed for disk
yes       missing        SG_IO allowed for process
yes       no             SG_IO allowed for process
yes       yes            error

Why not simplify things, and have a single attribute rawio, with
multiple values?

rawio        result
missing      kernel prevents SG_IO
no           kernel prevents SG_IO
yes          SG_IO allowed for process
cdb          SG_IO allowed for disk

where we document that 'yes' works on more kernel versions than 'cdb',
but that 'cdb' (new to 1.0.1) is more secure.

>          The optional <code>snapshot</code> attribute indicates the default
>          behavior of the disk during disk snapshots: "internal"
>          requires a file format such as qcow2 that can store both the
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 14344e2..c8cdfd7 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -971,24 +971,44 @@
>      -->
>    <define name="disk">
>      <element name="disk">
> -      <optional>
> -        <attribute name="device">
> -          <choice>
> -            <value>floppy</value>
> -            <value>disk</value>
> -            <value>cdrom</value>
> -            <value>lun</value>
> -          </choice>
> -        </attribute>
> -      </optional>
> -      <optional>
> -        <attribute name="rawio">
> +      <choice>
> +        <group>
> +          <optional>
> +            <attribute name="device">
> +              <choice>
> +                <value>floppy</value>
> +                <value>disk</value>
> +                <value>cdrom</value>
> +              </choice>
> +            </attribute>
> +          </optional>
> +        </group>

I like that you split this up, to enforce that rawio only appears with
device='lun'.  However...

> +        <group>
> +          <optional>
> +            <attribute name="device">
> +              <value>lun</value>
> +            </attribute>
> +          </optional>

This device='lun' should not be <optional>, since the default when
device= is omitted is 'disk' (that is, unless you want the absence of
device= and the presence of rawio='...' to imply a lun, but I think we
should require an explicit use of lun before allowing rawio=).

>            <choice>
> -            <value>yes</value>
> -            <value>no</value>
> +            <optional>
> +              <attribute name="rawio">
> +                <choice>
> +                  <value>yes</value>
> +                  <value>no</value>
> +                </choice>
> +              </attribute>
> +            </optional>
> +            <optional>
> +              <attribute name="cdbfilter">
> +                <choice>
> +                  <value>yes</value>
> +                  <value>no</value>
> +                </choice>

And this part would need a major rework if you go with my idea of a
tri-state attribute value for rawio instead of introducing yet another
attribute.

> +              </attribute>
> +            </optional>
>            </choice>
> -        </attribute>
> -      </optional>
> +        </group>
> +      </choice>
>        <optional>
>          <ref name="snapshot"/>
>        </optional>
> 

-- 
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: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121214/3b40ce7b/attachment-0001.sig>