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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 06/24/2016 03:59 PM, Laine Stump wrote:
> 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@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxx>
>>> Signed-off-by: Laine Stump <laine@xxxxxxxxx>
>>> ---
>>>   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).

Yeah - I was probably thinking more about the Free thing, but since
you've pointed out that the freeaddrinfo is done - it's a moot point.

John
> 
>> John
>>

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]