[libvirt] [PATCH 1/4] vhost-user support: domain configuration
Michal Privoznik
mprivozn at redhat.com
Fri Jul 11 11:28:47 UTC 2014
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 at 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
More information about the libvir-list
mailing list