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(a)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