[libvirt] PATCH: 8/12:Interal driver API for node devices

[David Lively]

On Fri, 2008-11-14 at 13:20 +0000, Mark McLoughlin wrote:
> On Thu, 2008-11-13 at 17:31 +0000, Daniel P. Berrange wrote:
> >
> > +        virBufferVSprintf(&buf, "  <capability type='%s'>\n",
> > +                          virNodeDevCapTypeToString(caps->type));
> > +        switch (caps->type) {
> ...
> > +        case VIR_NODE_DEV_CAP_NET:
> > +            virBufferVSprintf(&buf, "    <interface>%s</interface>\n",
> > +                              data->net.interface);
> > +            if (data->net.address)
> > +                virBufferVSprintf(&buf, "    <address>%s</address>\n",
> > +                                  data->net.address);
> > +            if (data->net.subtype != VIR_NODE_DEV_CAP_NET_LAST) {
> > +                const char *subtyp =
> > +                    virNodeDevNetCapTypeToString(data->net.subtype);
> > +                virBufferVSprintf(&buf, "    <capability type='%s'>\n", subtyp);
> > +                switch (data->net.subtype) {
> > +                case VIR_NODE_DEV_CAP_NET_80203:
> > +                    virBufferVSprintf(&buf,
> > +                                      "      <mac_address>%012llx</address>\n",
> > +                                      data->net.data.ieee80203.mac_address);
> > +                    break;
> > +                case VIR_NODE_DEV_CAP_NET_80211:
> > +                    virBufferVSprintf(&buf,
> > +                                      "      <mac_address>%012llx</address>\n",
> > +                                      data->net.data.ieee80211.mac_address);
> > +                    break;
> > +                case VIR_NODE_DEV_CAP_NET_LAST:
> > +                    /* Keep dumb compiler happy */
> > +                    break;
> > +                }
> > +                virBufferAddLit(&buf, "    </capability>\n");
> 
> This all seems a bit odd.
> 
> We've listed the mac address once already, so why do it again in a hex format?

Agreed.  It's basically just reflecting the HAL attributes.  But I have
to admit I find a lot of HAL rather non-intuitive ...  I have no
particular attachment to any of this node device description XML.  I
really liked Daniel's suggestion that this be based on a subset of HAL,
and have tried to do that as much as possible.  I don't really care what
it's based on, but it sure would be nice to avoid inventing yet another
XML device representation (hence the attraction to HAL).  

But the more I've gotten to know HAL, the less I've liked the idea.  I'm
not aware of an alternative that doesn't involve inventing our own.
Perhaps there's something suitable in CIM??  I'm a CIM-dummy, so I have
no idea ...   This is related to the device naming issue as well.  I
share all of Dan B's concerns here ...

> 
> And I wouldn't think of "802.3 vs 802.11" as a network device capability, but more like it's "type" - they're mutually exclusive, right?
> 
> Also, we have:
> 
>   <device>
>     ...
>     <capability type='net'>
>       ...
>       <capability type='80203'>
>         <mac_address>001320f74a06</address>
>       </capability>
>     </capability>
>   </device>
> 
> i.e. two different capability semantics?
> 

Again, just reflecting HAL.  But I agree it seems bizarre.

> > +            }
> > +            break;
> > +        case VIR_NODE_DEV_CAP_BLOCK:
> > +            virBufferVSprintf(&buf, "    <device>%s</device>\n",
> > +                              data->block.device);
> 
> Two nested <device> nodes:
> 
> <device>
>   ...
>   <capability type='block'>
>     <device>/dev/sda1</device>
>   </capability>
> </device>
> 
> How about <path> or <device_path> ?

This one doesn't bother me so much, though granted the two <device> tags
are talking about completely different things (clear to me from the
context, but perhaps violating some proper XML design rules?).

> > +            if (data->storage.originating_device)
> > +                virBufferVSprintf(&buf,
> > +                                  "    <originating_device>%s"
> > +                                  "</originating_device>\n",
> > +                                  data->storage.originating_device);
> 
> This originating_device thing sounds strange, and I don't think we
> implement it yet. Leave it out for now?

Fine with me.  I don't know what it is -- just sounded generally useful.
If it's not widely implemented, we probably shouldn't bother supporting
it.

> > +            if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_REMOVABLE) {
> > +                int avl = data->storage.flags &
> > +                    VIR_NODE_DEV_CAP_STORAGE_REMOVABLE_MEDIA_AVAILABLE;
> > +                virBufferAddLit(&buf, "    <capability type='removable'>\n");
> > +                virBufferVSprintf(&buf,
> > +                                  "      <media_available>%d"
> > +                                  "</media_available>\n", avl ? 1 : 0);
> > +                virBufferVSprintf(&buf, "      <media_size>%llu</media_size>\n",
> > +                                  data->storage.removable_media_size);
> > +                virBufferAddLit(&buf, "    </capability>\n");
> > +            } else {
> > +                virBufferVSprintf(&buf, "    <size>%llu</size>\n",
> > +                                  data->storage.size);
> > +            }
> > +            if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_HOTPLUGGABLE)
> > +                virBufferAddLit(&buf,
> > +                                "    <capability type='hotpluggable' />\n");
> 
> Again, the nested capabilities doesn't seem right.
> 
> How about just:
> 
>   <removable media_available="1"/>
>   <hotpluggable/>

Again, just mimicking HAL's properties: removable.media_available,
removable.media_size.

> > +    case VIR_NODE_DEV_CAP_STORAGE:
> > +        VIR_FREE(data->storage.bus);
> > +        VIR_FREE(data->storage.drive_type);
> > +        VIR_FREE(data->storage.model);
> > +        VIR_FREE(data->storage.vendor);
> 
> originating_device not freed.

Good catch.

Thanks!
Dave