[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

> +        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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]