Re: [libvirt] [PATCH] vhost-user: add support reconnect for vhost-user ports

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&gt; >> --- >> 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 >> &lt;/interface&gt; >> &lt;interface type='vhostuser'&gt; >> &lt;mac address='52:54:00:3b:83:1b'/&gt; >> - &lt;source type='unix' path='/tmp/vhost2.sock' mode='client'/&gt; >> + &lt;source type='unix' path='/tmp/vhost2.sock' mode='client' reconnect='10'/&gt; >> &lt;model type='virtio'/&gt; >> &lt;driver queues='5'/&gt; >> &lt;/interface&gt; >> @@ -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>&lt;model&gt;</code> element is mandatory. >> + <code>&lt;model&gt;</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>