[libvirt] [PATCH 03/23] Add support for <hostdev mode="capabilities">

[Eric Blake]

On 11/30/2012 01:26 PM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> The <hostdev> device type has long had a redundant "mode"
> attribute, which has always been "subsys". This finally
> introduces a new mode "capabilities", which will be used
> by the LXC driver for device assignment. Since container
> based virtualization uses a single kernel, the idea of
> assigning physical PCI devices doesn't make sense. It is
> still reasonable to assign USB devices, but for assinging

s/assinging/assigning/

> arbitrary nodes in /dev, the new 'capabilities' mode is
> to be used.
> 
> The first capability support is 'storage', which is for
> assignment of block devices. Functionally this is really
> pretty similar to the <disk> support. The only difference
> is the device node name is identical in both host and
> container namespaces.
> 
>     <hostdev mode='capabilities' type='storage'>
>       <source>
>         <block>/dev/sdf1</block>
>       </source>
>     </hostdev>
> 
> The second capability support is 'misc', which is for
> assignment of character devices. There is no existing
> parallel to this. Again the device node is the same
> inside & outside the container.
> 
>     <hostdev mode='capabilities' type='misc'>
>       <source>
>         <char>/dev/sdf1</char>
>       </source>
>     </hostdev>
> 
> The reason for keeping the char & storage devices
> separate in the domain XML, is to mirror the split
> in the node device XML. NB the node device XML does
> not yet report character devices, but that's another
> new patch to come
> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  docs/schemas/domaincommon.rng        | 128 +++++++++++++++++--------

Missing documentation in an appropriate docs/*.html.in.  I haven't
checked if you add it later in the series, or completely forgot.

>  src/conf/domain_audit.c              |  80 +++++++++++-----
>  src/conf/domain_conf.c               | 175 ++++++++++++++++++++++++++++++++++-
>  src/conf/domain_conf.h               |  31 +++++--
>  src/libvirt_private.syms             |   1 +
>  tests/lxcxml2xmldata/lxc-hostdev.xml |  34 +++++++
>  tests/lxcxml2xmltest.c               |   1 +
>  7 files changed, 375 insertions(+), 75 deletions(-)
>  create mode 100644 tests/lxcxml2xmldata/lxc-hostdev.xml

But at least you added tests.

> +
> +  <define name="hostdevcaps">
> +    <attribute name="mode">
> +      <value>capabilities</value>
> +    </attribute>
> +    <choice>
> +      <group>
> +        <ref name="hostdevcapsstorage"/>
> +      </group>
> +    </choice>
> +  </define>

> +  <define name="hostdevcapsstorage">
> +    <attribute name="type">
> +      <value>storage</value>
> +    </attribute>
> +    <element name="source">
> +      <element name="block">
> +        <ref name="absFilePath"/>
> +      </element>
> +    </element>

I see where you added <hostdev mode='capabilities' type='storage'> but
not where you added <hostdev mode='capabilities' type='misc'>.  Did you
mean to have a <choice>storage|misc</choice> for the type attribute in
hostdevcapsstorage?

> +
> +    case VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES:
> +        switch (hostdev->source.caps.type) {
> +        case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE:
> +            if (!(device = virAuditEncode("disk",
> +                                          VIR_AUDIT_STR(hostdev->source.caps.u.storage.block)))) {
> +                VIR_WARN("OOM while encoding audit message");
> +                goto cleanup;
> +            }
> +
> +            VIR_AUDIT(VIR_AUDIT_RECORD_RESOURCE, success,
> +                      "virt=%s resrc=hostdev reason=%s %s uuid=%s %s",
> +                      virt, reason, vmname, uuidstr, device);
> +            break;
> +
> +        default:
> +            VIR_WARN("Unexpected hostdev type while encoding audit message: %d",
> +                     hostdev->source.caps.type);

Are you also missing 'misc' handling in this section of C code?  I see
places where it appears, but it seems incomplete (maybe I missed
something, though).

> -    if (!(device = virAuditEncode("device", VIR_AUDIT_STR(address)))) {
> -        VIR_WARN("OOM while encoding audit message");
> +    default:
> +        VIR_WARN("Unexpected hostdev mode while cndoing audit message: %d",

s/cndoing/encoding/

> +++ b/src/conf/domain_conf.c
> @@ -530,12 +530,16 @@ VIR_ENUM_IMPL(virDomainGraphicsSpiceClipboardCopypaste,
>  
>  VIR_ENUM_IMPL(virDomainHostdevMode, VIR_DOMAIN_HOSTDEV_MODE_LAST,
>                "subsystem",
> -              "capabilities")
> +              "capabilities");

Why the added ';'...

>  
>  VIR_ENUM_IMPL(virDomainHostdevSubsys, VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST,
>                "usb",
>                "pci")

...especially when it's already optional?

> @@ -1440,6 +1444,17 @@ void virDomainHostdevDefClear(virDomainHostdevDefPtr def)
>       */
>      if (def->parent.type == VIR_DOMAIN_DEVICE_NONE)
>          virDomainDeviceInfoFree(def->info);
> +
> +    if (def->mode == VIR_DOMAIN_HOSTDEV_MODE_CAPABILITIES) {
> +        switch (def->source.caps.type) {
> +        case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_STORAGE:
> +            VIR_FREE(def->source.caps.u.storage.block);
> +            break;
> +        case VIR_DOMAIN_HOSTDEV_CAPS_TYPE_MISC:
> +            VIR_FREE(def->source.caps.u.misc.chardev);
> +            break;
> +        }

For example, here you do handle both types.

-- 
-- 
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/20121210/0f7bce36/attachment-0001.sig>