On Wed, Feb 17, 2010 at 05:11:00PM +0000, Matthew Booth wrote:
Add support for virtio-serial by defining a new 'virtio' channel target type and a virtio-serial controller. Allows the following to be specified in a domain:
<controller type='virtio-serial' index='0' max_ports='16' vectors='4'/> <channel type='pty'> <target type='virtio' name='org.linux-kvm.port.0'/> <address type='virtio-serial' controller='0' bus='0'/> </channel>
* docs/schemas/domain.rng: Add virtio-serial controller and virtio channel type. * src/conf/domain_conf.[ch]: Domain parsing/serialization for virtio-serial controller and virtio channel. * tests/qemuxml2xmltest.c tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.xml : add domain xml parsing test * src/libvirt_private.syms src/qemu/qemu_conf.c: virDomainDefAddDiskControllers() renamed to virDomainDefAddImplicitControllers() --- docs/schemas/domain.rng | 71 +++++- src/conf/domain_conf.c | 234 +++++++++++++++++--- src/conf/domain_conf.h | 25 ++- src/libvirt_private.syms | 2 +- src/qemu/qemu_conf.c | 2 +- .../qemuxml2argv-channel-virtio.xml | 35 +++ tests/qemuxml2xmltest.c | 1 + 7 files changed, 320 insertions(+), 50 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-channel-virtio.xml
diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index 53b82ce..85df8b8 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -523,16 +523,36 @@ </define> <define name="controller"> <element name="controller"> - <optional> - <attribute name="type"> - <choice> - <value>fdc</value> - <value>ide</value> - <value>scsi</value> - <value>sata</value> - </choice> - </attribute> - </optional> + <choice> + <group> + <optional> + <attribute name="type"> + <choice> + <value>fdc</value> + <value>ide</value> + <value>scsi</value> + <value>sata</value> + </choice> + </attribute> + </optional> + </group> + <!-- virtio-serial can have 2 additional attributes --> + <group> + <attribute name="type"> + <value>virtio-serial</value> + </attribute> + <optional> + <attribute name="max_ports"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <optional> + <attribute name="vectors"> + <ref name="unsignedInt"/> + </attribute> + </optional>
What are these two new attributes doing ? Can't we just auto-assign those values based on the configured channels later int he XML.
+ </group> + </choice> <attribute name="index"> <ref name="unsignedInt"/> </attribute> @@ -1139,12 +1159,25 @@ <attribute name="port"/> </element> </define> + <define name="virtioTarget"> + <element name="target"> + <attribute name="type"> + <value>virtio</value> + </attribute> + <optional> + <attribute name="name"/> + </optional> + </element> + </define> <define name="channel"> <element name="channel"> <ref name="qemucdevSrcType"/> <interleave> <ref name="qemucdevSrcDef"/> - <ref name="guestfwdTarget"/> + <choice> + <ref name="guestfwdTarget"/> + <ref name="virtioTarget"/> + </choice> <optional> <ref name="address"/> </optional> @@ -1269,6 +1302,16 @@ <ref name="driveUnit"/> </attribute> </define> + <define name="virtioserialaddress"> + <attribute name="controller"> + <ref name="driveController"/> + </attribute> + <optional> + <attribute name="bus"> + <ref name="driveBus"/> + </attribute> + </optional> + </define>
What is the "bus" in the content of virtio serial ?
@@ -916,7 +930,8 @@ void virDomainDefClearDeviceAliases(virDomainDefPtr def) */ static int virDomainDeviceInfoFormat(virBufferPtr buf, virDomainDeviceInfoPtr info, - int flags) + int flags, + const char *indent)
I'm not seeing why we need to pass 'indent' through here? The device info data should always be appearing at exactly the same place in all devices, specifically at /domain/devices/[device type]/, so indent level should always be the same.
@@ -1481,10 +1553,49 @@ virDomainControllerDefParseXML(xmlNodePtr node, if (virDomainDeviceInfoParseXML(node, &def->info, flags) < 0) goto error;
+ switch (def->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: + 0; /* C requires a statement immediately following a label */
Just put a curly brace after the case case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: {
+ + char *max_ports = virXMLPropString(node, "max_ports"); + if (max_ports) { + int r = virStrToLong_i(max_ports, NULL, 10, + &def->opts.vioserial.max_ports); + if (r != 0 || def->opts.vioserial.max_ports < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid max_ports: %s"), max_ports); + VIR_FREE(max_ports); + goto error; + } + } else { + def->opts.vioserial.max_ports = -1; + } + VIR_FREE(max_ports); + + char *vectors = virXMLPropString(node, "vectors"); + if (vectors) { + int r = virStrToLong_i(vectors, NULL, 10, + &def->opts.vioserial.vectors); + if (r != 0 || def->opts.vioserial.vectors < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid vectors: %s"), vectors); + VIR_FREE(vectors); + goto error; + } + } else { + def->opts.vioserial.vectors = -1; + }
I think '0' would be preferable as the not-initialized number here, since its not a valid value for either attribute AFAICT
+ VIR_FREE(vectors); + break;
And close it here } break;
+ + default: + break; + } + if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Disk controllers must use the 'pci' address type")); + _("Controllers must use the 'pci' address type")); goto error; }
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 :|