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