6:28 a.m.
On 11.07.2014 13:06, Michele Paolino wrote:
On 10/07/2014 18:01, Michal Privoznik wrote:
> 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>
>> ---
>> 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)).
Yes, these files are present in 3/4. I will post the v2 of this series
in a single patch.
>
>> 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.
The check has been done here because if in future we will decide to
support other chardevs in addition to the unix socket, the XML file will
be different.
Until then I find the code easier to read with checks grouped.
For example, if we want to add the support for a TCP socket, the path
attribute needs to be replaced with host, service and protocol. After a
quick look at the _virDomainChrSourceDef structure, the XML description
in this case should look like:
<source type='tcp' host='host' service='serv'
mode='mode'
protocol='prot'/>
Do we want to add these additional checks only when the support for
other chardevs will be added, or is there an alternative solution?
Yes. There's no need to introduce the checks for unsupported
combinations now.
>
>> + }
>> } 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>
>
The nix structure in the _virDomainChrSourceDef uses a boolean to define
the socket mode. My proposal is to set listen=true only if mode='server'
and have it equal to false (client) otherwise.
Correct and I understood that. But the problem is mode='blah' has the
same effect as mode='client' or any other value but 'server'. I think we
should reject other strings than 'server' or 'client'. Or even reject
'client' too (and assume client mode whenever vhostuser_mode == NULL).
I'll leave that up to you.
>
>> +
>> + }
>> +
>> + 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.
The (converted) content of vhostuser_type and vhostuser_mode can be
retrieved in "(def->data.vhostuser)->data.nix.listen" and
"(def->data.vhostuser)->type".
Correct, but you still need to free() the vhostuser_type and
vhostuser_mode after the conversion. For instance, consider following
analogical code:
{
char *c = strdup("123");
int i;
virStrToLong_ui(c, NULL, 10, &i);
return;
}
At the point of 'return' @i contains converted value, but the memory
allocated for @c is leaked. Same goes for virXMLPropString() instead of
strdup().
Do we want their content available anyway?
Yes, but only for the time of parsing. After that you can free them.
Michal