[libvirt] [PATCH] allow ip and route elements for netdev ethernet
Laine Stump
laine at laine.org
Thu Mar 31 19:52:10 UTC 2016
On 03/31/2016 05:22 AM, Vasiliy Tolstov wrote:
> Allow to use ip address and routes elements inside network ethernet.
> Also allow to configure point-to-point address for host side device.
In order to allow for possible backporting of parts of this feature
without requiring backports of the rest, I think this should be in 4
patches:
1) changes to virnetdev.[ch] to support setting a peer address
2) changes to domain_conf.[ch], domaincommon.rng (that's the RNG file
that needs modification, not network.rng), and formatdomain.html.in to
add peer address to the config.
3) change to lxc_container.c to support setting the peer address.
4) change to qemu_interface.c to support setting ip/peer/route.
This way someone could backport the lxc support without needing to
backport qemu support, or they could backport some potential future fix
to one of the virNetDev*() functions without needing to backport the new
functionality at the hypervisor level. (I know this seems pedantic, but
multiple times I've ended up needing to manaually mangle bugfix patches
during a backport in order to account for lack of some feature on the
old branch).
>
> Signed-off-by: Vasiliy Tolstov <v.tolstov at selfip.ru>
> ---
> docs/formatdomain.html.in | 12 ++++++++-
> docs/schemas/network.rng | 3 +++
As I noted above, the file that needs this addition is domaincommon.rng
(which has the RNG for domain XML) not network.rng (which has the RNG
for libvirt virtual network XML, a completely different thing from the
<interface> inside a <domain>).
> include/libvirt/libvirt-domain.h | 1 +
> src/conf/domain_conf.c | 14 ++++++++++-
> src/conf/domain_conf.h | 1 +
> src/conf/network_conf.c | 26 ++++++++++++++++++-
> src/conf/network_conf.h | 1 +
Since it seems that my understanding is correct (this feature is to
support setting a POINTOPOINT peer address on the tap device used by a
domain's <interface type='ethernet'> device), you should get rid of the
changes to network_conf.[ch] - those are for configuring a libvirt
virtual network, not for configuring a domain's <interface> element.
> src/lxc/lxc_container.c | 2 +-
> src/network/bridge_driver.c | 2 +-
The change to bridge_driver should also be eliminated (except that
you'll need to put a NULL in place of the peer address). AFAIK it's
nonsensical to set a POINTOPOINT peer address for a linux host bridge
device (that's what this change does).
> src/qemu/qemu_interface.c | 39 +++++++++++++++++++++++++++++
> src/util/virnetdev.c | 54 ++++++++++++++++++++++++++++------------
> src/util/virnetdev.h | 1 +
> 12 files changed, 135 insertions(+), 21 deletions(-)
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 71ffe750452e..1203e96a8286 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -4717,6 +4717,7 @@ qemu-kvm -net nic,model=? /dev/null
> <source network='default'/>
> <target dev='vnet0'/>
> <b><ip address='192.168.122.5' prefix='24'/></b>
> + <b><ip address='192.168.122.5' prefix='24' peer='10.0.0.10'/></b>
> <b><route family='ipv4' address='192.168.122.0' prefix='24' gateway='192.168.122.1'/></b>
> <b><route family='ipv4' address='192.168.122.8' gateway='192.168.122.1'/></b>
> </interface>
> @@ -4749,7 +4750,16 @@ qemu-kvm -net nic,model=? /dev/null
> 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.
> + This is used by the LXC driver and <span class="since">Since 1.3.3</span> by the QEMU
> + driver.
> + </p>
> +
> + <p>
> + <span class="since">Since 1.3.3</span> ip elements can hold peer attribute to assign
> + point-to-point address for the network device. The attributes of this element
"to assign *a* point-to-point address"
> + are described in the documentation for the <code>ip</code> element in
> + <a href="formatnetwork.html#elementsAddress">network definitions</a>.
The above is untrue. You haven't made (and *shouldn't* make) any changes
to formatnetwork.html.in, so it doesn't contain any info about the
"peer" attribute. Since there are differences between <ip> for a domain
and for a network, you should document them separately.
> + This is only used by the LXC and QEMU driver.
s/driver/drivers/
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 7e06796c3c73..437e87fac01c 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -3937,6 +3937,7 @@ typedef virDomainIPAddress *virDomainIPAddressPtr;
> struct _virDomainInterfaceIPAddress {
> int type; /* virIPAddrType */
> char *addr; /* IP address */
> + char *peer; /* IP peer */
> unsigned int prefix; /* IP address prefix */
> };
You've added this into the struct that is used to report IP address info
returned by virDomainInterfaceAddresses, but haven't put anything into
the code supporting that API to report a peer address. You should either
add that support, or remove peer from this struct.
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d5d9ff702f42..34855233ad15 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5725,7 +5725,7 @@ virDomainNetIpParseXML(xmlNodePtr node)
> unsigned int prefixValue = 0;
> char *familyStr = NULL;
> int family = AF_UNSPEC;
> - char *address = NULL;
> + char *address = NULL, *peer = NULL;
>
> if (!(prefixStr = virXMLPropString(node, "prefix")) ||
> (virStrToLong_ui(prefixStr, NULL, 10, &prefixValue) < 0)) {
> @@ -5739,6 +5739,9 @@ virDomainNetIpParseXML(xmlNodePtr node)
> goto cleanup;
> }
>
> + if ((peer = virXMLPropString(node, "peer")) == NULL)
> + VIR_DEBUG("Peer is empty");
> +
> familyStr = virXMLPropString(node, "family");
> if (familyStr && STREQ(familyStr, "ipv4"))
> family = AF_INET;
> @@ -5756,6 +5759,14 @@ virDomainNetIpParseXML(xmlNodePtr node)
> address);
> goto cleanup;
> }
> +
> + if ((peer != NULL) && (virSocketAddrParse(&ip->peer, peer, family) < 0)) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("Failed to parse IP address: '%s'"),
> + peer);
> + goto cleanup;
> + }
> +
> ip->prefix = prefixValue;
>
> ret = ip;
> @@ -5765,6 +5776,7 @@ virDomainNetIpParseXML(xmlNodePtr node)
> VIR_FREE(prefixStr);
> VIR_FREE(familyStr);
> VIR_FREE(address);
> + VIR_FREE(peer);
> VIR_FREE(ip);
> return ret;
> }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 83bdd67dec45..040882ec8460 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -513,6 +513,7 @@ typedef struct _virDomainNetIpDef virDomainNetIpDef;
> typedef virDomainNetIpDef *virDomainNetIpDefPtr;
> struct _virDomainNetIpDef {
> virSocketAddr address; /* ipv4 or ipv6 address */
> + virSocketAddr peer; /* ipv4 or ipv6 address of peer */
> unsigned int prefix; /* number of 1 bits in the net mask */
> };
[removed unneeded network_conf.[ch] diffs]
>
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 348bbfbc01fc..a1deb0c00d4c 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -520,7 +520,7 @@ static int lxcContainerRenameAndEnableInterfaces(virDomainDefPtr vmDef,
>
> VIR_DEBUG("Adding IP address '%s/%u' to '%s'",
> ipStr, ip->prefix, newname);
> - if (virNetDevSetIPAddress(newname, &ip->address, prefix) < 0) {
> + if (virNetDevSetIPAddress(newname, &ip->address, &ip->peer, prefix) < 0) {
> virReportError(VIR_ERR_SYSTEM_ERROR,
> _("Failed to set IP address '%s' on %s"),
> ipStr, newname);
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index a09a7e474fc5..f3ff88ff55a6 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1970,7 +1970,7 @@ networkAddAddrToBridge(virNetworkObjPtr network,
> }
>
> if (virNetDevSetIPAddress(network->def->bridge,
> - &ipdef->address, prefix) < 0)
> + &ipdef->address, &ipdef->peer, prefix) < 0)
> return -1;
>
> return 0;
> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> index 13a513152876..5729325fadb9 100644
> --- a/src/qemu/qemu_interface.c
> +++ b/src/qemu/qemu_interface.c
> @@ -474,6 +474,45 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
> if (virNetDevSetMAC(net->ifname, &tapmac) < 0)
> goto cleanup;
>
> + for (j = 0; j < net->nips; j++) {
> + virDomainNetIpDefPtr ip = net->ips[j];
> + unsigned int prefix = (ip->prefix > 0) ? ip->prefix :
> + VIR_SOCKET_ADDR_DEFAULT_PREFIX;
> + char *ipStr = virSocketAddrFormat(&ip->address);
> +
> + VIR_DEBUG("Adding IP address '%s/%u' to '%s'",
> + ipStr, ip->prefix, net->ifname);
> +
> + if (virNetDevSetIPAddress(net->ifname, &ip->address, &ip->peer, prefix) < 0) {
> + virReportError(VIR_ERR_SYSTEM_ERROR,
> + _("Failed to set IP address '%s' on %s"),
> + ipStr, net->ifname);
> + VIR_FREE(ipStr);
> + goto cleanup;
> + }
> + VIR_FREE(ipStr);
> + }
> +
> + if (net->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_UP ||
> + net->linkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT) {
> + if (virNetDevSetOnline(net->ifname, true) < 0)
> + goto cleanup;
> +
> + /* Set the routes */
> + for (j = 0; j < net->nroutes; j++) {
> + virNetworkRouteDefPtr route = net->routes[j];
> +
> + if (virNetDevAddRoute(net->ifname,
> + virNetworkRouteDefGetAddress(route),
> + virNetworkRouteDefGetPrefix(route),
> + virNetworkRouteDefGetGateway(route),
> + virNetworkRouteDefGetMetric(route)) < 0) {
> + goto cleanup;
> + }
> + }
> + }
> +
> +
Seeing this makes me think that there is nothing in the validation to
assure somebody doesn't try to set an ip address for an interface type
that doesn't support it. Of course that's not necessary for this feature
to work (it's an existing problem) but a separate patch to log an error
if there are any ip's set for types other than ethernet would be a nice
touch (currently this happens in two places for some reason -
qemuBuildInterfaceCommandLine() and qemuDomainAttachNetDevice() - there
seems to be a lot of duplicated code there...)
> if (net->script &&
> qemuExecuteEthernetScript(net->ifname, net->script) < 0)
> goto cleanup;
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index aed50f546263..6e32ebbf6cee 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1039,21 +1039,28 @@ virNetDevCreateNetlinkAddressMessage(int messageType,
> const char *ifname,
> virSocketAddr *addr,
> unsigned int prefix,
> - virSocketAddr *broadcast)
> + virSocketAddr *broadcast,
> + virSocketAddr *peer)
> {
> struct nl_msg *nlmsg = NULL;
> struct ifaddrmsg ifa;
> unsigned int ifindex;
> void *addrData = NULL;
> + void *peerData = NULL;
> void *broadcastData = NULL;
> size_t addrDataLen;
>
> if (virNetDevGetIPAddressBinary(addr, &addrData, &addrDataLen) < 0)
> return NULL;
>
> - if (broadcast && virNetDevGetIPAddressBinary(broadcast, &broadcastData,
> - &addrDataLen) < 0)
> - return NULL;
> + if (peer && VIR_SOCKET_ADDR_VALID(peer)) {
> + if (virNetDevGetIPAddressBinary(peer, &peerData, &addrDataLen) < 0)
> + return NULL;
> + } else if (broadcast) {
> + if (virNetDevGetIPAddressBinary(broadcast, &broadcastData,
> + &addrDataLen) < 0)
> + return NULL;
> + }
>
> /* Get the interface index */
> if ((ifindex = if_nametoindex(ifname)) == 0)
> @@ -1078,12 +1085,15 @@ virNetDevCreateNetlinkAddressMessage(int messageType,
> if (nla_put(nlmsg, IFA_LOCAL, addrDataLen, addrData) < 0)
> goto buffer_too_small;
>
> - if (nla_put(nlmsg, IFA_ADDRESS, addrDataLen, addrData) < 0)
> - goto buffer_too_small;
> + if (peerData) {
> + if (nla_put(nlmsg, IFA_ADDRESS, addrDataLen, peerData) < 0)
> + goto buffer_too_small;
> + }
>
> - if (broadcastData &&
> - nla_put(nlmsg, IFA_BROADCAST, addrDataLen, broadcastData) < 0)
> - goto buffer_too_small;
> + if (broadcastData) {
> + if (nla_put(nlmsg, IFA_BROADCAST, addrDataLen, broadcastData) < 0)
> + goto buffer_too_small;
> + }
>
> return nlmsg;
>
> @@ -1098,6 +1108,7 @@ virNetDevCreateNetlinkAddressMessage(int messageType,
> * virNetDevSetIPAddress:
> * @ifname: the interface name
> * @addr: the IP address (IPv4 or IPv6)
> + * @peer: The IP address of peer (IPv4 or IPv6)
> * @prefix: number of 1 bits in the netmask
> *
> * Add an IP address to an interface. This function *does not* remove
> @@ -1108,6 +1119,7 @@ virNetDevCreateNetlinkAddressMessage(int messageType,
> */
> int virNetDevSetIPAddress(const char *ifname,
> virSocketAddr *addr,
> + virSocketAddr *peer,
> unsigned int prefix)
> {
> virSocketAddr *broadcast = NULL;
> @@ -1116,9 +1128,8 @@ int virNetDevSetIPAddress(const char *ifname,
> struct nlmsghdr *resp = NULL;
> unsigned int recvbuflen;
>
> -
> /* The caller needs to provide a correct address */
> - if (VIR_SOCKET_ADDR_FAMILY(addr) == AF_INET) {
> + if (VIR_SOCKET_ADDR_FAMILY(addr) == AF_INET && !VIR_SOCKET_ADDR_VALID(peer)) {
> /* compute a broadcast address if this is IPv4 */
> if (VIR_ALLOC(broadcast) < 0)
> return -1;
> @@ -1129,7 +1140,7 @@ int virNetDevSetIPAddress(const char *ifname,
>
> if (!(nlmsg = virNetDevCreateNetlinkAddressMessage(RTM_NEWADDR, ifname,
> addr, prefix,
> - broadcast)))
> + broadcast, peer)))
> goto cleanup;
>
> if (virNetlinkCommand(nlmsg, &resp, &recvbuflen, 0, 0,
> @@ -1288,7 +1299,7 @@ int virNetDevClearIPAddress(const char *ifname,
>
> if (!(nlmsg = virNetDevCreateNetlinkAddressMessage(RTM_DELADDR, ifname,
> addr, prefix,
> - NULL)))
> + NULL, NULL)))
> goto cleanup;
>
> if (virNetlinkCommand(nlmsg, &resp, &recvbuflen, 0, 0,
> @@ -1423,21 +1434,27 @@ virNetDevWaitDadFinish(virSocketAddrPtr *addrs, size_t count)
>
> int virNetDevSetIPAddress(const char *ifname,
> virSocketAddr *addr,
> + virSocketAddr *peer,
> unsigned int prefix)
> {
> virCommandPtr cmd = NULL;
> - char *addrstr = NULL, *bcaststr = NULL;
> + char *addrstr = NULL, *bcaststr = NULL, *peerstr = NULL;
> virSocketAddr broadcast;
> int ret = -1;
>
> if (!(addrstr = virSocketAddrFormat(addr)))
> goto cleanup;
> +
> + if (VIR_SOCKET_ADDR_VALID(peer) && !(peerstr = virSocketAddrFormat(&peer)))
> + goto cleanup;
> +
> /* format up a broadcast address if this is IPv4 */
> - if ((VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET)) &&
> + if (!peerstr && ((VIR_SOCKET_ADDR_IS_FAMILY(addr, AF_INET)) &&
> ((virSocketAddrBroadcastByPrefix(addr, prefix, &broadcast) < 0) ||
> - !(bcaststr = virSocketAddrFormat(&broadcast)))) {
> + !(bcaststr = virSocketAddrFormat(&broadcast))))) {
> goto cleanup;
> }
> +
> # ifdef IFCONFIG_PATH
> cmd = virCommandNew(IFCONFIG_PATH);
> virCommandAddArg(cmd, ifname);
> @@ -1446,6 +1463,8 @@ int virNetDevSetIPAddress(const char *ifname,
> else
> virCommandAddArg(cmd, "inet");
> virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix);
> + if (peerstr)
> + virCommandAddArgList(cmd, "pointopoint", peerstr, NULL);
> if (bcaststr)
> virCommandAddArgList(cmd, "broadcast", bcaststr, NULL);
> virCommandAddArg(cmd, "alias");
> @@ -1453,6 +1472,8 @@ int virNetDevSetIPAddress(const char *ifname,
> cmd = virCommandNew(IP_PATH);
> virCommandAddArgList(cmd, "addr", "add", NULL);
> virCommandAddArgFormat(cmd, "%s/%u", addrstr, prefix);
> + if (peerstr)
> + virCommandAddArgList(cmd, "peer", peerstr, NULL);
> if (bcaststr)
> virCommandAddArgList(cmd, "broadcast", bcaststr, NULL);
> virCommandAddArgList(cmd, "dev", ifname, NULL);
> @@ -1465,6 +1486,7 @@ int virNetDevSetIPAddress(const char *ifname,
> cleanup:
> VIR_FREE(addrstr);
> VIR_FREE(bcaststr);
> + VIR_FREE(peerstr);
> virCommandFree(cmd);
> return ret;
> }
> diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
> index e7719d58a410..240fff774d30 100644
> --- a/src/util/virnetdev.h
> +++ b/src/util/virnetdev.h
> @@ -90,6 +90,7 @@ int virNetDevGetOnline(const char *ifname,
>
> int virNetDevSetIPAddress(const char *ifname,
> virSocketAddr *addr,
> + virSocketAddr *peer,
> unsigned int prefix)
> ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
> int virNetDevAddRoute(const char *ifname,
More information about the libvir-list
mailing list