On 02.07.2014 15:20, Michele Paolino wrote: > vhost-user is added as a virDomainChrSourceDefPtr variable in the > virtual network interface configuration structure. > This patch adds support to parse vhost-user element. The XML file > 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> > > Signed-off-by: Michele Paolino <m.paolino(a)virtualopensystems.com&gt; > --- > src/conf/domain_conf.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++ > src/conf/domain_conf.h | 10 +++++-- > 2 files changed, 89 insertions(+), 2 deletions(-) Introducing a new device (subtype of a device) must always go hand in hand with documentation and XML schema adjustment. Moreover, it would be nice to test the new XML (e.g. domainschematest or qemuxml2argv stub (a .xml file under tests/qemuxml2argvdata without .args counterpart which will be introduced once qemu implementation is done)).

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index ea09bdc..de52ca4 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,21 @@ 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"); > + if (!vhostuser_type || STREQ(vhostuser_type, "unix")) { > + vhostuser_path = virXMLPropString(cur, "path"); > + vhostuser_mode = virXMLPropString(cur, "mode"); > + } > + else { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("type='%s' unsupported for" > + " <interface type='vhostuser'>"), > + vhostuser_type); > + goto error; I'd move this check a few lines below to the other checks.

> + } > } else if (!def->virtPortProfile > && xmlStrEqual(cur->name, BAD_CAST "virtualport")) { > if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) { > @@ -6867,6 +6890,50 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, > actual = NULL; > break; > > + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: > + if (!model || STRNEQ(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 (VIR_ALLOC(def->data.vhostuser) < 0) > + goto error; > + > + if (STREQ(vhostuser_type, "unix")) { Ouch, in the code I've commented above, you allow vhostuser_type to be a NULL, so this needs to be STREQ_NULLABLE(). > + > + (def->data.vhostuser)->type = VIR_DOMAIN_CHR_TYPE_UNIX; I haven't seen such braces usage since DV stopped writing patches :) > + > + if (vhostuser_path == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("No <source> 'path' attribute " > + "specified with <interface " > + "type='vhostuser'/>")); > + goto error; > + } > + > + (def->data.vhostuser)->data.nix.path = vhostuser_path; > + > + if (vhostuser_mode == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("No <source> 'mode' attribute " > + "specified with <interface " > + "type='vhostuser'/>")); > + goto error; > + } > + > + if (STREQ(vhostuser_mode, "server")) > + (def->data.vhostuser)->data.nix.listen = true; Should we disallow other modes? What if user passes: <interface type='vhostuser'> <source type='unix' path='/some/dummy/path' mode='blah'/> </interface>

> + > + } > + > + vhostuser_type = NULL; > + vhostuser_path = NULL; > + vhostuser_mode = NULL; I don't think you want to set vhostuser_type or vhostuser_mode to NULL here. In case of _path it's okay as you are stealing the pointer into the CharDev struct, but the _type and _mode are not stolen anywhere so you're leaking them effectively.

> + break; > + > case VIR_DOMAIN_NET_TYPE_ETHERNET: > if (dev != NULL) { > def->data.ethernet.dev = dev; > @@ -7112,6 +7179,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); > VIR_FREE(ifname); > VIR_FREE(dev); > virDomainActualNetDefFree(actual); > @@ -15989,6 +16059,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, This change requires other modifications too. For instance I'm seeing this: CC xenxs/libvirt_xenxs_la-xen_sxpr.lo xenxs/xen_sxpr.c: In function 'xenFormatSxprNet': xenxs/xen_sxpr.c:1889:5: error: enumeration value 'VIR_DOMAIN_NET_TYPE_VHOSTUSER' not handled in switch [-Werror=switch] switch (def->type) { ^ cc1: all warnings being treated as errors and its repetitions in other drivers. > @@ -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 { > I think this are the bits you need to squash in (don't forget the documentation on RNG schema): diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4920a4e..1d64f68 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6650,17 +6650,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, && def->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER && xmlStrEqual(cur->name, BAD_CAST "source")) { vhostuser_type = virXMLPropString(cur, "type"); - if (!vhostuser_type || STREQ(vhostuser_type, "unix")) { - vhostuser_path = virXMLPropString(cur, "path"); - vhostuser_mode = virXMLPropString(cur, "mode"); - } - else { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("type='%s' unsupported for" - " <interface type='vhostuser'>"), - vhostuser_type); - goto error; - } + 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) { @@ -6819,43 +6810,51 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, break; case VIR_DOMAIN_NET_TYPE_VHOSTUSER: - if (!model || STRNEQ(model, "virtio")) { + 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")); + "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 type 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; - if (STREQ(vhostuser_type, "unix")) { + def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX; + def->data.vhostuser->data.nix.path = vhostuser_path; - (def->data.vhostuser)->type = VIR_DOMAIN_CHR_TYPE_UNIX; - - if (vhostuser_path == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("No <source> 'path' attribute " - "specified with <interface " - "type='vhostuser'/>")); - goto error; - } - - (def->data.vhostuser)->data.nix.path = vhostuser_path; - - if (vhostuser_mode == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("No <source> 'mode' attribute " - "specified with <interface " - "type='vhostuser'/>")); - goto error; - } - - if (STREQ(vhostuser_mode, "server")) - (def->data.vhostuser)->data.nix.listen = true; - - } + if (STREQ(vhostuser_mode, "server")) + (def->data.vhostuser)->data.nix.listen = true; vhostuser_type = NULL; vhostuser_path = NULL; diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 0b4a0b5..a193a93 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -987,6 +987,7 @@ libxlMakeNic(virDomainDefPtr def, break; } case VIR_DOMAIN_NET_TYPE_USER: + 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/uml/uml_conf.c b/src/uml/uml_conf.c index 7a206d2..1d5d689 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -248,6 +248,11 @@ umlBuildCommandLineNet(virConnectPtr conn, _("hostdev networking type not supported")); goto error; + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("vhostuser networking type not supported")); + goto error; + case VIR_DOMAIN_NET_TYPE_LAST: break; } diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 9e59804..3d46773 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -1932,6 +1932,7 @@ xenFormatSxprNet(virConnectPtr conn, break; case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: case VIR_DOMAIN_NET_TYPE_SERVER: case VIR_DOMAIN_NET_TYPE_CLIENT: case VIR_DOMAIN_NET_TYPE_MCAST: Michal

On 02.07.2014 15:20, Michele Paolino wrote: > This patch adds the test files and the documentation for vhost-user. > > Signed-off-by: Michele Paolino <m.paolino(a)virtualopensystems.com&gt; > --- > docs/formatdomain.html.in | 34 > +++++++++++++++++++ > docs/schemas/domaincommon.rng | 39 > ++++++++++++++++++++++ > .../qemuxml2argv-net-vhostuser.args | 7 ++++ > .../qemuxml2argv-net-vhostuser.xml | 33 > ++++++++++++++++++ > tests/qemuxml2argvtest.c | 1 + > tests/qemuxml2xmltest.c | 1 + > 6 files changed, 115 insertions(+) > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.args > create mode 100644 > tests/qemuxml2argvdata/qemuxml2argv-net-vhostuser.xml > Aha! You are doing here what I've just requested in 1/4. Still I rather see it in a single patch (even though it will be bigger) because it make maintainers life easier. You know, backporting a single patch versus digging through 'git log' to search for this commit ... > 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; > + &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> > + > <h4><a name="elementsInput">Input devices</a></h4> > > <p> > diff --git a/docs/schemas/domaincommon.rng > b/docs/schemas/domaincommon.rng > index af51eee..f85dd61 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -1968,6 +1968,45 @@ > </group> > <group> > <attribute name="type"> > + <value>vhostuser</value> > + </attribute> > + <interleave> > + <optional> > + <element name="source"> I wouldn't say <source/> is optional in case of <interface type='vhostnet'/>. It contains crucial information that helps us construct the correct qemu command line which would not be possible otherwise.

> + <attribute name="type"> > + <choice> > + <value>null</value> > + <value>vc</value> > + <value>pty</value> > + <value>dev</value> > + <value>file</value> > + <value>pipe</value> > + <value>stdio</value> > + <value>udp</value> > + <value>tcp</value> > + <value>unix</value> > + <value>spicevmc</value> > + <value>spiceport</value> > + <value>nmdm</value> Honestly, I'm inclined to not enumerate all of these here now. I mean, only 'unix' is supported now. On the other hand, it's acceptable to have a RNG schema that allows something more than our XML parser. > + </choice> > + </attribute> > + <attribute name="path"> > + <ref name="absFilePath"/> > + </attribute> > + <attribute name="mode"> > + <choice> > + <value>server</value> > + <value>client</value> > + </choice> > + </attribute> > + <empty/> > + </element> > + </optional> > + <ref name="interface-options"/> > + </interleave> > + </group> > + <group> > + <attribute name="type"> > <value>network</value> > </attribute> > <interleave> > 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 b4414d6..d4b082a 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 43cd022..1fd0ad1 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"); > Nice, I've forgotten about qemuxml2xmltest completely. Michal