New subject: [libvirt] PATCH: 1/12: Build libtool convenience libraries

13 Nov 2008

On Fri, Nov 14, 2008 at 01:20:22PM +0000, Mark McLoughlin wrote:
> On Thu, 2008-11-13 at 17:31 +0000, Daniel P. Berrange wrote:
> > This is the basic internal driver support code for the node device APIs.
> > The actual registration of the drivers was pushed to a later patch to
> > allow this to compile on its own & thus be fully bisectable.
> > +        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?

I'd removed the 2nd hexformat one.

> 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?

The reason for the nested capabilities is that they're supposed
to represent refinements of the main capability. While the
current wifi vs ethernet refinements are mutually exclusive,
I think its worth having the option of multiple refinements
in the future.

> > +            }
> > +            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 has gone away with the merge of 'block' into 'storage'

> > +            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?

Yep, removed it

> > > +            if (data->storage.flags & VIR_NODE_DEV_CAP_STORAGE_HOTPLUGGABLE)
> > +                virBufferAddLit(&buf,
> > +                                "    <capability type='hotpluggable' />\n");
> 
> Again, the nested capabilities doesn't seem right.

This was the same rationale as the nested capability info for
network devices - one or more refinements of the master
capability.

> > +    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.

It is gone completely now

> > diff -r 6812c3044043 src/node_device_conf.h
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/src/node_device_conf.h	Wed Nov 12 21:11:51 2008 +0000
> ...
> > +char *virNodeDeviceDefFormat(virConnectPtr conn,
> > +                             const virNodeDeviceDefPtr def);
> > +
> > +// TODO: virNodeDeviceDefParseString/File/Node for virNodeDeviceCreate
> 
> Kinda superflous comment - if we implement DeviceCreate(), the need for
> those functions will be obvious.

Forgot to remove this TODO, but will do so.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

[libvirt] PATCH: 0/12: Modular build & node devices integration

Mark McLoughlin

David Lively

Mark McLoughlin

Mark McLoughlin

David Lively

Mark McLoughlin

David Lively

Mark McLoughlin

tags

participants (5)