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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list