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