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

Re: [libvirt] [PATCH v2] support for QEMU vhost-user



On 11.07.2014 19:47, Michele Paolino wrote:
> This patch adds support for the QEMU vhost-user feature to libvirt.
> vhost-user enables the communication between a QEMU virtual machine
> and other userspace process using the Virtio transport protocol.
> It uses a char dev (e.g. Unix socket) for the control plane,
> while the data plane based on shared memory.
> 
> The XML looks like:
> 
> <interface type='vhostuser'>
>      <source type='unix' path='/tmp/vhost.sock' mode='server'/>
>      <mac address='52:54:00:3b:83:1a'/>
>      <model type='virtio'/>
> </interface>
> 
> changes from v1:
>   * addressed comments
>   * removed unnecessary checks
>   * series merged in a single patch

We tend to write the diff to previous versions into notes not in the commit message as it pollutes git log.

BTW: I didn't ask the whole patchset to be merged into a single patch, but it doesn't hurt in this specific case either (the diff stat seems reasonably big).

> 
> The previous version of this patch can be found at:
> http://www.redhat.com/archives/libvir-list/2014-July/msg00111.html
> 
> Signed-off-by: Michele Paolino <m paolino virtualopensystems com>
> ---
>   docs/formatdomain.html.in                          | 34 +++++++++
>   docs/schemas/domaincommon.rng                      | 25 +++++++
>   src/conf/domain_conf.c                             | 87 ++++++++++++++++++++++
>   src/conf/domain_conf.h                             | 10 ++-
>   src/libxl/libxl_conf.c                             |  1 +
>   src/lxc/lxc_process.c                              |  1 +
>   src/qemu/qemu_command.c                            | 63 ++++++++++++++++
>   src/uml/uml_conf.c                                 |  5 ++
>   src/xenxs/xen_sxpr.c                               |  1 +
>   .../qemuxml2argv-net-vhostuser.args                |  7 ++
>   .../qemuxml2argv-net-vhostuser.xml                 | 33 ++++++++
>   tests/qemuxml2argvtest.c                           |  1 +
>   tests/qemuxml2xmltest.c                            |  1 +
>   13 files changed, 267 insertions(+), 2 deletions(-)
>   create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args
>   create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.xml
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 3f8bbee..606b7d4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3927,6 +3927,40 @@ qemu-kvm -net nic,model=? /dev/null
>         <span class="since">Since 0.9.5</span>
>       </p>
>   
> +    <h5><a name="elementVhostuser">vhost-user interface</a></h5>
> +
> +    <p>
> +     vhost-user enables the communication between a QEMU virtual machine
> +     and other userspace process using the Virtio transport protocol.
> +     A char dev (e.g. Unix socket) is used for the control plane, while
> +     the data plane is based on shared memory.
> +    </p>
> +
> +<pre>
> +  ...
> +  &lt;devices&gt;
> +    &lt;interface type='vhostuser'&gt;
> +      &lt;source type='unix' path='/tmp/vhost.sock' mode='server'&gt;
> +      &lt;/source&gt;
> +      &lt;mac address='52:54:00:3b:83:1a'&gt;
> +      &lt;/mac&gt;
> +      &lt;model type='virtio'&gt;
> +      &lt;/model&gt;


I don't think so. Empty bodies elements are written as <elem/>. And that's how libvirt formats them too. And if I were to be really picky, <mac/> is formated before <source/>.

> +    &lt;/interface&gt;
> +  &lt;/devices&gt;
> +  ...</pre>
> +
> +    <p>
> +      The <code>&lt;source&gt;</code> element has to be specified
> +      along with the type of char device.
> +      Currently, only type='unix' is supported, where the path (the
> +      directory path of the socket) and mode attributes are required.
> +      Both <code>mode='server'</code> and <code>mode='client'</code>
> +      are supported.
> +      vhost-user requires the virtio model type, thus the
> +      <code>&lt;model&gt;</code> element is mandatory.
> +    </p>

I think it would be useful for users reading the documentation to know what version of libvirt was this introduced in.

> +
>       <h4><a name="elementsInput">Input devices</a></h4>
>   
>       <p>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index af51eee..c9c02b6 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1968,6 +1968,31 @@
>           </group>
>           <group>
>             <attribute name="type">
> +            <value>vhostuser</value>
> +          </attribute>
> +          <interleave>
> +              <element name="source">
> +                <attribute name="type">
> +                  <choice>
> +                    <value>unix</value>
> +                  </choice>
> +                </attribute>
> +                <attribute name="path">
> +                  <ref name="absFilePath"/>
> +                </attribute>
> +                <attribute name="mode">
> +                  <choice>
> +                    <value>server</value>
> +                    <value>client</value>
> +                  </choice>
> +                </attribute>
> +                <empty/>
> +              </element>
> +            <ref name="interface-options"/>
> +          </interleave>
> +        </group>
> +        <group>
> +          <attribute name="type">
>               <value>network</value>
>             </attribute>
>             <interleave>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 8df43b7..fb286c6 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -349,6 +349,7 @@ VIR_ENUM_IMPL(virDomainFSWrpolicy, VIR_DOMAIN_FS_WRPOLICY_LAST,
>   VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST,
>                 "user",
>                 "ethernet",
> +              "vhostuser",
>                 "server",
>                 "client",
>                 "mcast",
> @@ -1361,6 +1362,10 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
>           VIR_FREE(def->data.ethernet.ipaddr);
>           break;
>   
> +    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> +        virDomainChrSourceDefFree(def->data.vhostuser);
> +        break;
> +
>       case VIR_DOMAIN_NET_TYPE_SERVER:
>       case VIR_DOMAIN_NET_TYPE_CLIENT:
>       case VIR_DOMAIN_NET_TYPE_MCAST:
> @@ -6665,6 +6670,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>       char *mode = NULL;
>       char *linkstate = NULL;
>       char *addrtype = NULL;
> +    char *vhostuser_mode = NULL;
> +    char *vhostuser_path = NULL;
> +    char *vhostuser_type = NULL;
>       virNWFilterHashTablePtr filterparams = NULL;
>       virDomainActualNetDefPtr actual = NULL;
>       xmlNodePtr oldnode = ctxt->node;
> @@ -6710,6 +6718,12 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>                          xmlStrEqual(cur->name, BAD_CAST "source")) {
>                   dev  = virXMLPropString(cur, "dev");
>                   mode = virXMLPropString(cur, "mode");
> +            } else if (!vhostuser_path && !vhostuser_mode && !vhostuser_type
> +                       && def->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> +                       xmlStrEqual(cur->name, BAD_CAST "source")) {
> +                vhostuser_type = virXMLPropString(cur, "type");
> +                vhostuser_path = virXMLPropString(cur, "path");
> +                vhostuser_mode = virXMLPropString(cur, "mode");
>               } else if (!def->virtPortProfile
>                          && xmlStrEqual(cur->name, BAD_CAST "virtualport")) {
>                   if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> @@ -6867,6 +6881,65 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>           actual = NULL;
>           break;
>   
> +    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> +        if (STRNEQ_NULLABLE(model, "virtio")) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("Wrong or no <model> 'type' attribute "
> +                             "specified with <interface type='vhostuser'/>. "
> +                             "vhostuser requires the virtio-net* frontend"));
> +            goto error;
> +        }
> +
> +        if (STRNEQ_NULLABLE(vhostuser_type, "unix")) {
> +            if (vhostuser_type)
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("Type='%s' unsupported for"
> +                                 " <interface type='vhostuser'>"),
> +                               vhostuser_type);
> +            else
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("No <source> 'type' attribute "
> +                                 "specified for <interface "
> +                                 "type='vhostuser'>"));
> +            goto error;
> +        }
> +
> +        if (vhostuser_path == NULL) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("No <source> 'path' attribute "
> +                             "specified with <interface "
> +                             "type='vhostuser'/>"));
> +            goto error;
> +        }
> +
> +        if (vhostuser_mode == NULL) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("No <source> 'mode' attribute "
> +                             "specified with <interface "
> +                             "type='vhostuser'/>"));
> +            goto error;
> +        }
> +
> +        if (VIR_ALLOC(def->data.vhostuser) < 0)
> +            goto error;
> +
> +        def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;
> +        def->data.vhostuser->data.nix.path = vhostuser_path;

The @vhostuser_path clear should be right here, otherwise ...

> +
> +        if (STREQ(vhostuser_mode, "server"))
> +            def->data.vhostuser->data.nix.listen = true;
> +        else if (STREQ(vhostuser_mode, "client"))
> +            def->data.vhostuser->data.nix.listen = false;
> +        else {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("Wrong <source> 'mode' attribute "
> +                             "specified with <interface "
> +                             "type='vhostuser'/>"));
> +            goto error;

... if we get here (e.g. user specified mode='blah') ...

> +        }
> +        vhostuser_path = NULL;
> +        break;
> +
>       case VIR_DOMAIN_NET_TYPE_ETHERNET:
>           if (dev != NULL) {
>               def->data.ethernet.dev = dev;
> @@ -7112,6 +7185,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>       VIR_FREE(portgroup);
>       VIR_FREE(address);
>       VIR_FREE(port);
> +    VIR_FREE(vhostuser_type);
> +    VIR_FREE(vhostuser_path);
> +    VIR_FREE(vhostuser_mode);

... then we free the @vhostuser_path. However, the memory was already freed by virDomainNetDefFree() under 'error' label (not visible in this diff though). So we have a double free:


==30765== Thread 5:
==30765== Invalid free() / delete / delete[] / realloc()
==30765==    at 0x4C2B5AC: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==30765==    by 0x52ADCE9: virFree (viralloc.c:582)
==30765==    by 0x533F74C: virDomainNetDefParseXML (domain_conf.c:7142)
==30765==    by 0x534ECF4: virDomainDefParseXML (domain_conf.c:12418)
==30765==    by 0x5351A11: virDomainDefParseNode (domain_conf.c:13222)
==30765==    by 0x5351866: virDomainDefParse (domain_conf.c:13164)
==30765==    by 0x53518C6: virDomainDefParseString (domain_conf.c:13180)
==30765==    by 0x14354207: qemuDomainDefineXML (qemu_driver.c:6239)
==30765==    by 0x53DD9EC: virDomainDefineXML (libvirt.c:8682)
==30765==    by 0x128934: remoteDispatchDomainDefineXML (remote_dispatch.h:3309)
==30765==    by 0x12887F: remoteDispatchDomainDefineXMLHelper (remote_dispatch.h:3289)
==30765==    by 0x544FCC1: virNetServerProgramDispatchCall (virnetserverprogram.c:437)
==30765==  Address 0x17481650 is 0 bytes inside a block of size 16 free'd
==30765==    at 0x4C2B5AC: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==30765==    by 0x52ADCE9: virFree (viralloc.c:582)
==30765==    by 0x532EF4D: virDomainChrSourceDefClear (domain_conf.c:1432)
==30765==    by 0x532F2FB: virDomainChrSourceDefFree (domain_conf.c:1507)
==30765==    by 0x532ED1E: virDomainNetDefFree (domain_conf.c:1351)
==30765==    by 0x533F878: virDomainNetDefParseXML (domain_conf.c:7167)
==30765==    by 0x534ECF4: virDomainDefParseXML (domain_conf.c:12418)
==30765==    by 0x5351A11: virDomainDefParseNode (domain_conf.c:13222)
==30765==    by 0x5351866: virDomainDefParse (domain_conf.c:13164)
==30765==    by 0x53518C6: virDomainDefParseString (domain_conf.c:13180)
==30765==    by 0x14354207: qemuDomainDefineXML (qemu_driver.c:6239)
==30765==    by 0x53DD9EC: virDomainDefineXML (libvirt.c:8682)


>       VIR_FREE(ifname);
>       VIR_FREE(dev);
>       virDomainActualNetDefFree(actual);
> @@ -15987,6 +16063,17 @@ virDomainNetDefFormat(virBufferPtr buf,
>                                     def->data.ethernet.ipaddr);
>               break;
>   
> +        case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> +            if (def->data.vhostuser->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
> +                virBufferAddLit(buf, "<source type='unix'");
> +                virBufferEscapeString(buf, " path='%s'",
> +                                      def->data.vhostuser->data.nix.path);
> +                if (def->data.vhostuser->data.nix.listen)
> +                    virBufferAddLit(buf, " mode='server'");
> +                virBufferAddLit(buf, "/>\n");
> +            }
> +            break;
> +
>           case VIR_DOMAIN_NET_TYPE_BRIDGE:
>               virBufferEscapeString(buf, "<source bridge='%s'/>\n",
>                                     def->data.bridge.brname);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index eb5bad7..91f7524 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -133,6 +133,12 @@ typedef virDomainIdMapDef *virDomainIdMapDefPtr;
>   typedef struct _virDomainPanicDef virDomainPanicDef;
>   typedef virDomainPanicDef *virDomainPanicDefPtr;
>   
> +/* forward declarations virDomainChrSourceDef, required by
> + * virDomainNetDef
> + */
> +typedef struct _virDomainChrSourceDef virDomainChrSourceDef;
> +typedef virDomainChrSourceDef *virDomainChrSourceDefPtr;
> +
>   /* Flags for the 'type' field in virDomainDeviceDef */
>   typedef enum {
>       VIR_DOMAIN_DEVICE_NONE = 0,
> @@ -795,6 +801,7 @@ struct _virDomainFSDef {
>   typedef enum {
>       VIR_DOMAIN_NET_TYPE_USER,
>       VIR_DOMAIN_NET_TYPE_ETHERNET,
> +    VIR_DOMAIN_NET_TYPE_VHOSTUSER,
>       VIR_DOMAIN_NET_TYPE_SERVER,
>       VIR_DOMAIN_NET_TYPE_CLIENT,
>       VIR_DOMAIN_NET_TYPE_MCAST,
> @@ -880,6 +887,7 @@ struct _virDomainNetDef {
>               char *dev;
>               char *ipaddr;
>           } ethernet;
> +        virDomainChrSourceDefPtr vhostuser;
>           struct {
>               char *address;
>               int port;
> @@ -1006,8 +1014,6 @@ typedef enum {
>   } virDomainChrSpicevmcName;
>   
>   /* The host side information for a character device.  */
> -typedef struct _virDomainChrSourceDef virDomainChrSourceDef;
> -typedef virDomainChrSourceDef *virDomainChrSourceDefPtr;
>   struct _virDomainChrSourceDef {
>       int type; /* virDomainChrType */
>       union {
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 4b6b5c0..c196136 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -955,6 +955,7 @@ libxlMakeNic(virDomainDefPtr def,
>                   return -1;
>               break;
>           }
> +        case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
>           case VIR_DOMAIN_NET_TYPE_USER:
>           case VIR_DOMAIN_NET_TYPE_SERVER:
>           case VIR_DOMAIN_NET_TYPE_CLIENT:
> diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
> index 0aef13a..854f65d 100644
> --- a/src/lxc/lxc_process.c
> +++ b/src/lxc/lxc_process.c
> @@ -437,6 +437,7 @@ static int virLXCProcessSetupInterfaces(virConnectPtr conn,
>   
>           case VIR_DOMAIN_NET_TYPE_USER:
>           case VIR_DOMAIN_NET_TYPE_ETHERNET:
> +        case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
>           case VIR_DOMAIN_NET_TYPE_SERVER:
>           case VIR_DOMAIN_NET_TYPE_CLIENT:
>           case VIR_DOMAIN_NET_TYPE_MCAST:
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index af4d009..e2d4645 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -6923,6 +6923,66 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
>   }
>   
>   static int
> +qemuBuildVhostuserCommandLine(virCommandPtr cmd,
> +                              virDomainDefPtr def,
> +                              virDomainNetDefPtr net,
> +                              virQEMUCapsPtr qemuCaps)
> +{
> +    virBuffer chardev_buf = VIR_BUFFER_INITIALIZER;
> +    virBuffer netdev_buf = VIR_BUFFER_INITIALIZER;
> +
> +    char* nic = NULL;
> +    char* str = NULL;
> +
> +    if (!qemuDomainSupportsNetdev(def, qemuCaps, net)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("Netdev support unavailable"));
> +        goto error;
> +    }
> +
> +    if (net->data.vhostuser->type == VIR_DOMAIN_CHR_TYPE_UNIX) {

I'd suggest using switch ((virDomainChrType) net->data.vhostuser->type) even though there's gonna be only one item implemented as it makes it eaier for future implementations.

> +        virBufferAsprintf(&chardev_buf, "socket,id=char%s,path=%s%s",
> +                          net->info.alias, net->data.vhostuser->data.nix.path,
> +                          net->data.vhostuser->data.nix.listen ? ",server" : "");
> +    }
> +
> +    virBufferAsprintf(&netdev_buf, "type=vhost-user,id=host%s,chardev=char%s",
> +                      net->info.alias, net->info.alias);
> +
> +    if (virBufferError(&chardev_buf) || virBufferError(&netdev_buf)) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +
> +    str = virBufferContentAndReset(&chardev_buf);

We already have virCommandAddArgBuffer() which does exatcly this.

> +    virCommandAddArgList(cmd, "-chardev", str, NULL);
> +    VIR_FREE(str);
> +
> +    str = virBufferContentAndReset(&netdev_buf);
> +    virCommandAddArgList(cmd, "-netdev", str, NULL);
> +    VIR_FREE(str);
> +
> +    if (!(nic = qemuBuildNicDevStr(def, net, -1, 0, 0, qemuCaps))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       "%s", _("Error generating NIC -device string"));
> +        goto error;
> +    }
> +
> +    virCommandAddArgList(cmd, "-device", nic, NULL);
> +    VIR_FREE(nic);
> +
> +    return 0;
> +
> + error:
> +    virBufferFreeAndReset(&chardev_buf);
> +    virBufferFreeAndReset(&netdev_buf);
> +    VIR_FREE(nic);
> +    VIR_FREE(str);
> +
> +    return -1;
> +}
> +
> +static int
>   qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>                                 virQEMUDriverPtr driver,
>                                 virConnectPtr conn,
> @@ -6945,6 +7005,9 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd,
>       int actualType = virDomainNetGetActualType(net);
>       size_t i;
>   
> +    if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER)
> +        return qemuBuildVhostuserCommandLine(cmd, def, net, qemuCaps);
> +

There are couple of checks at the begining of the function. Placing this up front them means disabling them. I don't think we want to do that.

>       if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
>           /* NET_TYPE_HOSTDEV devices are really hostdev devices, so
>            * their commandlines are constructed with other hostdevs.
> diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
> index 464d56d..d33d9b4 100644
> --- a/src/uml/uml_conf.c
> +++ b/src/uml/uml_conf.c
> @@ -182,6 +182,11 @@ umlBuildCommandLineNet(virConnectPtr conn,
>           }
>           break;
>   
> +    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("vhostuser networking type not supported"));
> +        goto error;
> +
>       case VIR_DOMAIN_NET_TYPE_SERVER:
>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                          _("TCP server networking type not supported"));
> diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c
> index aacf74c..fe49c42 100644
> --- a/src/xenxs/xen_sxpr.c
> +++ b/src/xenxs/xen_sxpr.c
> @@ -1933,6 +1933,7 @@ xenFormatSxprNet(virConnectPtr conn,
>               virBufferEscapeSexpr(buf, "(ip '%s')", def->data.ethernet.ipaddr);
>           break;
>   
> +    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
>       case VIR_DOMAIN_NET_TYPE_USER:
>       case VIR_DOMAIN_NET_TYPE_SERVER:
>       case VIR_DOMAIN_NET_TYPE_CLIENT:
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args
> new file mode 100644
> index 0000000..cc66ec3
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args
> @@ -0,0 +1,7 @@
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu -S -M \
> +pc -m 214 -smp 1 -nographic -nodefaults -monitor unix:/tmp/test-monitor,server,nowait \
> +-no-acpi -boot c -usb -hda /dev/HostVG/QEMUGuest1 \
> +-chardev socket,id=charnet0,path=/tmp/vhost.sock,server \
> +-netdev type=vhost-user,id=hostnet0,chardev=charnet0 \
> +-device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:ee:96:6b,bus=pci.0,addr=0x3
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.xml
> new file mode 100644
> index 0000000..b49d48e
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.xml
> @@ -0,0 +1,33 @@
> +<domain type='qemu'>
> +  <name>QEMUGuest1</name>
> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> +  <memory unit='KiB'>219136</memory>
> +  <currentMemory unit='KiB'>219136</currentMemory>
> +  <vcpu placement='static'>1</vcpu>
> +  <os>
> +    <type arch='i686' machine='pc'>hvm</type>
> +    <boot dev='hd'/>
> +  </os>
> +  <clock offset='utc'/>
> +  <on_poweroff>destroy</on_poweroff>
> +  <on_reboot>restart</on_reboot>
> +  <on_crash>destroy</on_crash>
> +  <devices>
> +    <emulator>/usr/bin/qemu</emulator>
> +    <disk type='block' device='disk'>
> +      <driver name='qemu' type='raw'/>
> +      <source dev='/dev/HostVG/QEMUGuest1'/>
> +      <target dev='hda' bus='ide'/>
> +      <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> +    </disk>
> +    <controller type='usb' index='0'/>
> +    <controller type='ide' index='0'/>
> +    <controller type='pci' index='0' model='pci-root'/>
> +    <interface type='vhostuser'>
> +      <mac address='52:54:00:ee:96:6b'/>
> +      <source type='unix' path='/tmp/vhost.sock' mode='server'/>
> +      <model type='virtio'/>
> +    </interface>
> +    <memballoon model='none'/>
> +  </devices>
> +</domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index d5066d2..2099624 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -927,6 +927,7 @@ mymain(void)
>       DO_TEST_FAILURE("misc-enable-s4", NONE);
>       DO_TEST("misc-no-reboot", NONE);
>       DO_TEST("misc-uuid", QEMU_CAPS_NAME, QEMU_CAPS_UUID);
> +    DO_TEST("net-vhostuser", QEMU_CAPS_DEVICE, QEMU_CAPS_NETDEV);
>       DO_TEST("net-user", NONE);
>       DO_TEST("net-virtio", NONE);
>       DO_TEST("net-virtio-device",
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 200d50f..359125a 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -245,6 +245,7 @@ mymain(void)
>       DO_TEST("misc-disable-suspends");
>       DO_TEST("misc-enable-s4");
>       DO_TEST("misc-no-reboot");
> +    DO_TEST("net-vhostuser");
>       DO_TEST("net-user");
>       DO_TEST("net-virtio");
>       DO_TEST("net-virtio-device");
> 

Otherwise looking good. ACK.

I'm squashing this in and pushing:

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 057465a..27465bb 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -3947,22 +3947,19 @@ qemu-kvm -net nic,model=? /dev/null
     <h5><a name="elementVhostuser">vhost-user interface</a></h5>
 
     <p>
-     vhost-user enables the communication between a QEMU virtual machine
-     and other userspace process using the Virtio transport protocol.
-     A char dev (e.g. Unix socket) is used for the control plane, while
-     the data plane is based on shared memory.
+    <span class="since">Since 1.2.7</span> the vhost-user enables the
+    communication between a QEMU virtual machine and other userspace process
+    using the Virtio transport protocol.  A char dev (e.g. Unix socket) is used
+    for the control plane, while the data plane is based on shared memory.
     </p>
 
 <pre>
   ...
   &lt;devices&gt;
     &lt;interface type='vhostuser'&gt;
-      &lt;source type='unix' path='/tmp/vhost.sock' mode='server'&gt;
-      &lt;/source&gt;
-      &lt;mac address='52:54:00:3b:83:1a'&gt;
-      &lt;/mac&gt;
-      &lt;model type='virtio'&gt;
-      &lt;/model&gt;
+      &lt;mac address='52:54:00:3b:83:1a'/&gt;
+      &lt;source type='unix' path='/tmp/vhost.sock' mode='server'/&gt;
+      &lt;model type='virtio'/&gt;
     &lt;/interface&gt;
   &lt;/devices&gt;
   ...</pre>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f8cd5d4..ad4549c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -6878,6 +6878,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
 
         def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;
         def->data.vhostuser->data.nix.path = vhostuser_path;
+        vhostuser_path = NULL;
 
         if (STREQ(vhostuser_mode, "server"))
             def->data.vhostuser->data.nix.listen = true;
@@ -6890,7 +6891,6 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
                              "type='vhostuser'/>"));
             goto error;
         }
-        vhostuser_path = NULL;
         break;
 
     case VIR_DOMAIN_NET_TYPE_ETHERNET:
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 2ac83eb..c0a5a13 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6856,9 +6856,7 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
 {
     virBuffer chardev_buf = VIR_BUFFER_INITIALIZER;
     virBuffer netdev_buf = VIR_BUFFER_INITIALIZER;
-
-    char* nic = NULL;
-    char* str = NULL;
+    char *nic = NULL;
 
     if (!qemuDomainSupportsNetdev(def, qemuCaps, net)) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -6866,27 +6864,40 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
         goto error;
     }
 
-    if (net->data.vhostuser->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
+    switch ((virDomainChrType) net->data.vhostuser->type) {
+    case VIR_DOMAIN_CHR_TYPE_UNIX:
         virBufferAsprintf(&chardev_buf, "socket,id=char%s,path=%s%s",
                           net->info.alias, net->data.vhostuser->data.nix.path,
                           net->data.vhostuser->data.nix.listen ? ",server" : "");
+        break;
+
+    case VIR_DOMAIN_CHR_TYPE_NULL:
+    case VIR_DOMAIN_CHR_TYPE_VC:
+    case VIR_DOMAIN_CHR_TYPE_PTY:
+    case VIR_DOMAIN_CHR_TYPE_DEV:
+    case VIR_DOMAIN_CHR_TYPE_FILE:
+    case VIR_DOMAIN_CHR_TYPE_PIPE:
+    case VIR_DOMAIN_CHR_TYPE_STDIO:
+    case VIR_DOMAIN_CHR_TYPE_UDP:
+    case VIR_DOMAIN_CHR_TYPE_TCP:
+    case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
+    case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
+    case VIR_DOMAIN_CHR_TYPE_NMDM:
+    case VIR_DOMAIN_CHR_TYPE_LAST:
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("vhost-user type '%s' not supported"),
+                        virDomainChrTypeToString(net->data.vhostuser->type));
+        goto error;
     }
 
     virBufferAsprintf(&netdev_buf, "type=vhost-user,id=host%s,chardev=char%s",
                       net->info.alias, net->info.alias);
 
-    if (virBufferError(&chardev_buf) || virBufferError(&netdev_buf)) {
-        virReportOOMError();
-        goto error;
-    }
+    virCommandAddArg(cmd, "-chardev");
+    virCommandAddArgBuffer(cmd, &chardev_buf);
 
-    str = virBufferContentAndReset(&chardev_buf);
-    virCommandAddArgList(cmd, "-chardev", str, NULL);
-    VIR_FREE(str);
-
-    str = virBufferContentAndReset(&netdev_buf);
-    virCommandAddArgList(cmd, "-netdev", str, NULL);
-    VIR_FREE(str);
+    virCommandAddArg(cmd, "-netdev");
+    virCommandAddArgBuffer(cmd, &netdev_buf);

     if (!(nic = qemuBuildNicDevStr(def, net, -1, 0, 0, qemuCaps))) {
         virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -6903,7 +6914,6 @@ qemuBuildVhostuserCommandLine(virCommandPtr cmd,
     virBufferFreeAndReset(&chardev_buf);
     virBufferFreeAndReset(&netdev_buf);
     VIR_FREE(nic);
-    VIR_FREE(str);
 
     return -1;
 }


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