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.