Re: [libvirt] [PATCH 1/2] Add domain support for virtio channel

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