[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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.
> 
> Daniel
> 
> diff -r 6812c3044043 src/Makefile.am
> --- a/src/Makefile.am	Wed Nov 12 21:11:46 2008 +0000
> +++ b/src/Makefile.am	Wed Nov 12 21:11:51 2008 +0000
> @@ -132,6 +132,10 @@
>  		storage_driver.h storage_driver.c		\
>  		storage_backend.h storage_backend.c
>  
> +# Network driver generic impl APIs

Typo

> +NODE_DEVICE_CONF_SOURCES =					\
> +		node_device_conf.c node_device_conf.h
> +
...
> diff -r 6812c3044043 src/node_device_conf.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/src/node_device_conf.c	Wed Nov 12 21:11:51 2008 +0000
...
> +char *virNodeDeviceDefFormat(virConnectPtr conn,
> +                             const virNodeDeviceDefPtr def)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +    virNodeDevCapsDefPtr caps = def->caps;
> +    char *tmp;
> +
> +    virBufferAddLit(&buf, "<device>\n");
> +    virBufferEscapeString(&buf, "  <name>%s</name>\n", def->name);
> +
> +    if (def->parent)
> +        virBufferEscapeString(&buf, "  <parent>%s</parent>\n", def->parent);
> +
> +    for (caps = def->caps; caps; caps = caps->next) {
> +        char uuidstr[VIR_UUID_STRING_BUFLEN];
> +        union _virNodeDevCapData *data = &caps->data;
> +
> +        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?

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?

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

> +            break;
> +        case VIR_NODE_DEV_CAP_SCSI_HOST:
> +            virBufferVSprintf(&buf, "    <host>%d</host>\n",
> +                              data->scsi_host.host);
> +            break;
> +        case VIR_NODE_DEV_CAP_SCSI:
> +            virBufferVSprintf(&buf, "    <host>%d</host>\n", data->scsi.host);
> +            virBufferVSprintf(&buf, "    <bus>%d</bus>\n", data->scsi.bus);
> +            virBufferVSprintf(&buf, "    <target>%d</target>\n",
> +                              data->scsi.target);
> +            virBufferVSprintf(&buf, "    <lun>%d</lun>\n", data->scsi.lun);
> +            if (data->scsi.type)
> +                virBufferVSprintf(&buf, "    <type>%s</type>\n",
> +                                  data->scsi.type);
> +            break;
> +        case VIR_NODE_DEV_CAP_STORAGE:
> +            if (data->storage.bus)
> +                virBufferVSprintf(&buf, "    <bus>%s</bus>\n",
> +                                  data->storage.bus);
> +            if (data->storage.drive_type)
> +                virBufferVSprintf(&buf, "    <drive_type>%s</drive_type>\n",
> +                                  data->storage.drive_type);
> +            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?

> +            if (data->storage.model)
> +                virBufferVSprintf(&buf, "    <model>%s</model>\n",
> +                                  data->storage.model);
> +            if (data->storage.vendor)
> +                virBufferVSprintf(&buf, "    <vendor>%s</vendor>\n",
> +                                  data->storage.vendor);
> +            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/>

> +            break;
> +        case VIR_NODE_DEV_CAP_LAST:
> +            /* ignore special LAST value */
> +            break;
> +        }
> +
> +        virBufferAddLit(&buf, "  </capability>\n");
> +    }
> +
> +    virBufferAddLit(&buf, "</device>\n");
> +
> +    if (virBufferError(&buf))
> +        goto no_memory;
> +
> +    return virBufferContentAndReset(&buf);
> +
> + no_memory:
> +    virNodeDeviceReportError(conn, VIR_ERR_NO_MEMORY, NULL);
> +    tmp = virBufferContentAndReset(&buf);
> +    VIR_FREE(tmp);
> +    return NULL;
> +}
> +
> +void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps)
> +{
> +    union _virNodeDevCapData *data = &caps->data;
> +
> +    switch (caps->type) {
> +    case VIR_NODE_DEV_CAP_SYSTEM:
> +        VIR_FREE(data->system.product_name);
> +        VIR_FREE(data->system.hardware.vendor_name);
> +        VIR_FREE(data->system.hardware.version);
> +        VIR_FREE(data->system.hardware.serial);
> +        VIR_FREE(data->system.firmware.vendor_name);
> +        VIR_FREE(data->system.firmware.version);
> +        VIR_FREE(data->system.firmware.release_date);
> +        break;
> +    case VIR_NODE_DEV_CAP_PCI_DEV:
> +        VIR_FREE(data->pci_dev.product_name);
> +        VIR_FREE(data->pci_dev.vendor_name);
> +        break;
> +    case VIR_NODE_DEV_CAP_USB_DEV:
> +        VIR_FREE(data->usb_dev.product_name);
> +        VIR_FREE(data->usb_dev.vendor_name);
> +        break;
> +    case VIR_NODE_DEV_CAP_USB_INTERFACE:
> +        VIR_FREE(data->usb_if.description);
> +        break;
> +    case VIR_NODE_DEV_CAP_NET:
> +        VIR_FREE(data->net.interface);
> +        VIR_FREE(data->net.address);
> +        break;
> +    case VIR_NODE_DEV_CAP_BLOCK:
> +        VIR_FREE(data->block.device);
> +        break;
> +    case VIR_NODE_DEV_CAP_SCSI_HOST:
> +        break;
> +    case VIR_NODE_DEV_CAP_SCSI:
> +        VIR_FREE(data->scsi.type);
> +        break;
> +    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.

> +        break;
> +    case VIR_NODE_DEV_CAP_LAST:
> +        /* This case is here to shutup the compiler */
> +        break;
> +    }
> +
> +    VIR_FREE(caps);
> +}
> +
> 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.

Cheers,
Mark.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]