[libvirt] [PATCH] vhost-user: add support reconnect for vhost-user ports
Michal Privoznik
mprivozn at redhat.com
Wed Sep 20 14:29:49 UTC 2017
On 09/20/2017 03:59 PM, Pavel Hrdina wrote:
> On Wed, Sep 20, 2017 at 03:15:23PM +0200, Michal Privoznik wrote:
>> On 09/08/2017 11:12 AM, ZhiPeng Lu wrote:
>>> For vhost-user ports, Open vSwitch acts as the server and QEMU the client.
>>> When OVS crashed or restart, QEMU shoule be reconnect to OVS.
>>>
>>> Signed-off-by: ZhiPeng Lu <lu.zhipeng at zte.com.cn>
>>> ---
>>> docs/formatdomain.html.in | 6 +++--
>>> docs/schemas/domaincommon.rng | 5 ++++
>>> src/conf/domain_conf.c | 28 ++++++++++++++++++++--
>>> .../qemuxml2argv-net-vhostuser-multiq.args | 2 +-
>>> .../qemuxml2argv-net-vhostuser-multiq.xml | 2 +-
>>> 5 files changed, 37 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>> index 8ca7637..ffe45c2 100644
>>> --- a/docs/formatdomain.html.in
>>> +++ b/docs/formatdomain.html.in
>>> @@ -5660,7 +5660,7 @@ qemu-kvm -net nic,model=? /dev/null
>>> </interface>
>>> <interface type='vhostuser'>
>>> <mac address='52:54:00:3b:83:1b'/>
>>> - <source type='unix' path='/tmp/vhost2.sock' mode='client'/>
>>> + <source type='unix' path='/tmp/vhost2.sock' mode='client' reconnect='10'/>
>>> <model type='virtio'/>
>>> <driver queues='5'/>
>>> </interface>
>>> @@ -5675,7 +5675,9 @@ qemu-kvm -net nic,model=? /dev/null
>>> Both <code>mode='server'</code> and <code>mode='client'</code>
>>> are supported.
>>> vhost-user requires the virtio model type, thus the
>>> - <code><model></code> element is mandatory.
>>> + <code><model></code> element is mandatory. <code>reconnect</code>
>>> + is an optional element,which configures reconnect timeout if the
>>> + connection is lost.
>>
>> This is missing "Since 3.7.0".
>>
>>> </p>
>>>
>>> <h5><a id="elementNwfilter">Traffic filtering with NWFilter</a></h5>
>>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>>> index 06c5a91..82f30ae 100644
>>> --- a/docs/schemas/domaincommon.rng
>>> +++ b/docs/schemas/domaincommon.rng
>>> @@ -2383,6 +2383,11 @@
>>> <value>client</value>
>>> </choice>
>>> </attribute>
>>> + <optional>
>>> + <attribute name="reconnect">
>>> + <ref name='positiveInteger'/>
>>> + </attribute>
>>> + </optional>
>>> <empty/>
>>> </element>
>>> <ref name="interface-options"/>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index 2fc1fc3..f9c3b35 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -10176,6 +10176,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>>> char *vhostuser_mode = NULL;
>>> char *vhostuser_path = NULL;
>>> char *vhostuser_type = NULL;
>>> + char *vhostuser_reconnect = NULL;
>>> char *trustGuestRxFilters = NULL;
>>> char *vhost_path = NULL;
>>> virNWFilterHashTablePtr filterparams = NULL;
>>> @@ -10262,11 +10263,12 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>>> goto error;
>>> }
>>> } else if (!vhostuser_path && !vhostuser_mode && !vhostuser_type
>>> - && def->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
>>> - virXMLNodeNameEqual(cur, "source")) {
>>> + && !vhostuser_reconnect && def->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER
>>> + && virXMLNodeNameEqual(cur, "source")) {
>>> vhostuser_type = virXMLPropString(cur, "type");
>>> vhostuser_path = virXMLPropString(cur, "path");
>>> vhostuser_mode = virXMLPropString(cur, "mode");
>>> + vhostuser_reconnect = virXMLPropString(cur, "reconnect");
>>> } else if (!def->virtPortProfile
>>> && virXMLNodeNameEqual(cur, "virtualport")) {
>>> if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
>>> @@ -10478,6 +10480,11 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>>> "type='vhostuser'/>"));
>>> goto error;
>>> }
>>> + if (vhostuser_reconnect != NULL && STREQ(vhostuser_mode, "server")) {
>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> + _("'reconnect' attribute unsupported "
>>> + "'server' mode for <interface type='vhostuser'>"));
>>> + }
>>
>> This can be checked later, when we validate vhostuser_mode.
>>
>>>
>>> if (VIR_ALLOC(def->data.vhostuser) < 0)
>>> goto error;
>>> @@ -10490,6 +10497,17 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>>> def->data.vhostuser->data.nix.listen = true;
>>> } else if (STREQ(vhostuser_mode, "client")) {
>>> def->data.vhostuser->data.nix.listen = false;
>>> + if (vhostuser_reconnect != NULL) {
>>> + def->data.vhostuser->data.nix.reconnect.enabled = true;
>>> + if (virStrToLong_ui(vhostuser_reconnect, NULL, 0,
>>> + &def->data.vhostuser->data.nix.reconnect.timeout) < 0) {
>>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> + _("vhost-user reconnect attribute is invalid"));
>>> + vhostuser_reconnect = NULL;
>>> + def->data.vhostuser->data.nix.reconnect.enabled = false;
>>
>> There are two problems with this. Setting vhostuser_reconnect to NULL
>> leaks it when the control jumps to the error label. Secondly, there's no
>> need to set reconnect.enabled to false. Oh, yes and we know what base
>> the reconnect number is in. Therefore s/0/10/. I guess we don't want to
>> allow:
>>
>> reconnect="0x50"
>>
>> I've fixed all these problems, ACKed and pushed.
>
> Sigh, I was planning to reply to this patch that we should model the XML
> the same way as the reconnect element for chardev devices [1]. Now I
> see that the documentation misses an example. Anyway this should be the
> correct XML:
>
> <source type='unix' path='/tmp/vhost2.sock' mode='client'>
> <reconnect enabled='yes' timeout='10'/>
> </source>
I was thinking about this too. Problem with this is that <source/> for
<interface> has slightly different meaning than for <channel/>. For
instance, we allow:
<interface type='ethernet'>
<mac address='00:16:3e:0f:ef:8a'/>
<source>
<ip address='192.168.122.12' family='ipv4' prefix='24' peer='192.168.122.1'/>
<ip address='192.168.122.13' family='ipv4' prefix='24'/>
<route family='ipv4' address='0.0.0.0' gateway='192.168.122.1'/>
<route family='ipv4' address='192.168.124.0' prefix='24' gateway='192.168.124.1'/>
</source>
<ip address='192.168.122.1' family='ipv4' prefix='32' peer='192.168.122.12'/>
<guest dev='eth2'/>
</interface>
But now that I think about it, sure. If you think that having it as a
separate element instead of an attribute is better do provide patches.
Michal
More information about the libvir-list
mailing list