Re: [libvirt] [PATCH 24/28] conf: allow setting peer address in <ip> element of <interface>

On 06/24/2016 09:29 AM, John Ferlan wrote: > > On 06/22/2016 01:37 PM, Laine Stump wrote: >> From: Vasiliy Tolstov <v.tolstov(a)selfip.ru&gt; >> >> The peer attribute is used to set the property of the same name in the >> interface IP info: >> >> <interface type='ethernet'> >> ... >> <ip family='ipv4' address='192.168.122.5' >> prefix='32' peer='192.168.122.6'/> >> ... >> </interface> >> >> Note that this element is used to set the IP information on the >> *guest* side interface, not the host side interface - that will be >> supported in an upcoming patch. >> >> (This is an updated *re*-commit of commit 690969af, which was >> subsequently reverted in commit 1d14b13f). >> >> Signed-off-by: Vasiliy Tolstov <v.tolstov(a)selfip.ru&gt; >> Signed-off-by: Laine Stump <laine(a)laine.org&gt; >> --- >> docs/formatdomain.html.in | 40 >> +++++++++++++++++++++++++--------------- >> docs/schemas/domaincommon.rng | 5 +++++ >> src/conf/domain_conf.c | 16 +++++++++++++++- >> src/conf/domain_conf.h | 1 + >> src/util/virnetdevip.h | 5 +++-- >> 5 files changed, 49 insertions(+), 18 deletions(-) >> > It seems an earlier comment I made about freeaddrinfo for the > corresponding getaddrinfo becomes even more important now... So > somewhere between there and this patch, I think you need to generate a > Free API for virNetDevIPAddrPtr > > Then everywhere that current just does a VIR_FREE(ip) should call it... > Thus this change "gets it" for free - well mostly free, you'd have to > add the freeaddrinfo() for the 'peer' as well as the 'address' > > Of course I could be wrong and I'm sure you'll let me know ;-) Yep. We already talked about this in IRC. the virNetDevIPAddr doesn't contain the info returned from getaddrinfo. virSocketAddrParseInternal (that's who calls getaddrinfo) is called by two other functions, and both of those functions copy out the important stuff from getaddrinfo, then call freeaddrinfo. So everything is correct. > >> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in >> index f660aa6..2466df7 100644 >> --- a/docs/formatdomain.html.in >> +++ b/docs/formatdomain.html.in >> @@ -4967,6 +4967,7 @@ qemu-kvm -net nic,model=? /dev/null >> &lt;source network='default'/&gt; >> &lt;target dev='vnet0'/&gt; >> <b>&lt;ip address='192.168.122.5' prefix='24'/&gt;</b> >> + <b>&lt;ip address='192.168.122.5' prefix='24' >> peer='10.0.0.10'/&gt;</b> >> <b>&lt;route family='ipv4' address='192.168.122.0' >> prefix='24' gateway='192.168.122.1'/&gt;</b> >> <b>&lt;route family='ipv4' address='192.168.122.8' >> gateway='192.168.122.1'/&gt;</b> >> &lt;/interface&gt; >> @@ -4985,21 +4986,30 @@ qemu-kvm -net nic,model=? /dev/null >> </pre> >> <p> >> - <span class="since">Since 1.2.12</span> the network devices and >> host devices >> - with network capabilities can be provided zero or more IP >> addresses to set >> - on the target device. Note that some hypervisors or network >> device types >> - will simply ignore them or only use the first one. The >> <code>family</code> >> - attribute can be set to either <code>ipv4</code> or >> <code>ipv6</code>, the >> - <code>address</code> attribute holds the IP address. The >> <code>prefix</code> >> - is not mandatory since some hypervisors do not handle it. >> - </p> >> - >> - <p> >> - <span class="since">Since 1.2.12</span> route elements can also >> be added >> - to define the network routes to use for the network device. The >> attributes >> - of this element are described in the documentation for the >> <code>route</code> >> - element in <a >> href="formatnetwork.html#elementsStaticroute">network definitions</a>. >> - This is only used by the LXC driver. >> + <span class="since">Since 1.2.12</span> network devices and >> + hostdev devices with network capabilities can optionally be >> provided >> + one or more IP addresses to set on the network device in the >> + guest. Note that some hypervisors or network device types will >> + simply ignore them or only use the first one. >> + The <code>family</code> attribute can be set to >> + either <code>ipv4</code> or <code>ipv6</code>, and the >> + <code>address</code> attribute contains the IP address. The >> + optional <code>prefix</code> is the number of 1 bits in the >> + netmask, and will be automatically set if not specified - for >> + IPv4 the default prefix is determined according to the network >> + "class" (A, B, or C - see RFC870), and for IPv6 the default >> + prefix is 64. The optional <code>peer</code> attribute holds the >> + IP address of the other end of a point-to-point network >> + device <span class="since">(since 2.0.0)</span>. >> + </p> >> + >> + <p> >> + <span class="since">Since 1.2.12</span> route elements can also be >> + added to define IP routes to add in the guest. The attributes of >> + this element are described in the documentation for >> + the <code>route</code> element >> + in <a href="formatnetwork.html#elementsStaticroute">network >> + definitions</a>. This is used by the LXC driver. >> </p> >> <h5><a name="elementVhostuser">vhost-user interface</a></h5> >> diff --git a/docs/schemas/domaincommon.rng >> b/docs/schemas/domaincommon.rng >> index 563cb3c..2d12da9 100644 >> --- a/docs/schemas/domaincommon.rng >> +++ b/docs/schemas/domaincommon.rng >> @@ -2629,6 +2629,11 @@ >> <ref name="ipPrefix"/> >> </attribute> >> </optional> >> + <optional> >> + <attribute name="peer"> >> + <ref name="ipAddr"/> >> + </attribute> >> + </optional> >> <empty/> >> </element> >> </zeroOrMore> >> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c >> index df52ac9..ad2d983 100644 >> --- a/src/conf/domain_conf.c >> +++ b/src/conf/domain_conf.c >> @@ -6108,7 +6108,7 @@ virDomainNetIPParseXML(xmlNodePtr node) >> unsigned int prefixValue = 0; >> char *familyStr = NULL; >> int family = AF_UNSPEC; >> - char *address = NULL; >> + char *address = NULL, *peer = NULL; >> if (!(address = virXMLPropString(node, "address"))) { >> virReportError(VIR_ERR_XML_ERROR, "%s", >> @@ -6146,6 +6146,13 @@ virDomainNetIPParseXML(xmlNodePtr node) >> } >> ip->prefix = prefixValue; >> + if ((peer = virXMLPropString(node, "peer")) != NULL && >> + virSocketAddrParse(&ip->peer, peer, family) < 0) { >> + virReportError(VIR_ERR_INVALID_ARG, >> + _("Invalid peer '%s' in <ip>"), peer); >> + goto cleanup; >> + } >> + >> ret = ip; >> ip = NULL; >> @@ -6153,6 +6160,7 @@ virDomainNetIPParseXML(xmlNodePtr node) >> VIR_FREE(prefixStr); >> VIR_FREE(familyStr); >> VIR_FREE(address); >> + VIR_FREE(peer); >> VIR_FREE(ip); >> return ret; >> } >> @@ -20254,6 +20262,12 @@ virDomainNetIPInfoFormat(virBufferPtr buf, >> virBufferAsprintf(buf, " family='%s'", familyStr); >> if (def->ips[i]->prefix) >> virBufferAsprintf(buf, " prefix='%u'", >> def->ips[i]->prefix); >> + if (VIR_SOCKET_ADDR_VALID(&def->ips[i]->peer)) { >> + if (!(ipStr = virSocketAddrFormat(&def->ips[i]->peer))) >> + return -1; >> + virBufferAsprintf(buf, " peer='%s'", ipStr); >> + VIR_FREE(ipStr); >> + } >> virBufferAddLit(buf, "/>\n"); >> } >> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h >> index 0df5579..7ff966f 100644 >> --- a/src/conf/domain_conf.h >> +++ b/src/conf/domain_conf.h >> @@ -383,6 +383,7 @@ typedef enum { >> VIR_DOMAIN_HOSTDEV_CAPS_TYPE_LAST >> } virDomainHostdevCapsType; >> + > Spurious change that can be dropped. > > This change is ACK'able with the mentioned adjustments (whether you want > to repost stuff or not is your call). You mean the extra line in domain_conf.h? :-P (since the other comment is a non-issue).

> John >