[libvirt] [PATCH 0/4] util: use netlink to create bridge devices
This patch series comes out of a comment in: https://bugzilla.redhat.com/show_bug.cgi?id=1125755 that pointed out that sioctl(SIOCBRDELBR) won't delete a bridge interface if it is IFF_UP, but the netlink RTM_DELLINK message doesn't care - it will delete it anyway. In these patches we switch to using RTM_DELLINK to avoid failures caused by a bridge interface that is IFF_UP. Laine Stump (4): util: netlink function to delete any network device util: replace body of virNetDevMacVLanDelete() with virNetlinkDelLink() util: use netlink to delete bridge devices util: use netlink to create bridge devices src/libvirt_private.syms | 1 + src/util/virnetdevbridge.c | 90 +++++++++++++++++++++++++++++++++++++++++++-- src/util/virnetdevmacvlan.c | 67 +-------------------------------- src/util/virnetlink.c | 89 ++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 3 +- 5 files changed, 181 insertions(+), 69 deletions(-) -- 2.1.0
libvirt has always used the netlink RTM_DELLINK message to delete macvtap/macvlan devices, but it can actually be used to delete other types of network devices, such as bonds and bridges. This patch makes virNetDevMacVLanDelete() available as a generic function so it can intelligibly be called to delete these other types of interfaces. --- src/libvirt_private.syms | 1 + src/util/virnetlink.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 3 +- 3 files changed, 92 insertions(+), 1 deletion(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ca3520d..631edf3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1805,6 +1805,7 @@ virNetDevVPortProfileOpTypeToString; # util/virnetlink.h virNetlinkCommand; +virNetlinkDelLink; virNetlinkEventAddClient; virNetlinkEventRemoveClient; virNetlinkEventServiceIsRunning; diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index d52f66a..86c9c9c 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -277,6 +277,87 @@ int virNetlinkCommand(struct nl_msg *nl_msg, } +/** + * virNetlinkDelLink: + * + * @ifname: Name of the link + * + * delete a network "link" (aka interface aka device) with the given + * name. This works for many different types of network devices, + * including macvtap and bridges. + * + * Returns 0 on success, -1 on fatal error. + */ +int +virNetlinkDelLink(const char *ifname) +{ + int rc = -1; + struct nlmsghdr *resp = NULL; + struct nlmsgerr *err; + struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC }; + unsigned int recvbuflen; + struct nl_msg *nl_msg; + + nl_msg = nlmsg_alloc_simple(RTM_DELLINK, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); + if (!nl_msg) { + virReportOOMError(); + return -1; + } + + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) + goto buffer_too_small; + + if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) + goto buffer_too_small; + + if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, + NETLINK_ROUTE, 0) < 0) { + goto cleanup; + } + + if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) + goto malformed_resp; + + switch (resp->nlmsg_type) { + case NLMSG_ERROR: + err = (struct nlmsgerr *)NLMSG_DATA(resp); + if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) + goto malformed_resp; + + if (err->error) { + virReportSystemError(-err->error, + _("error destroying network device %s"), + ifname); + goto cleanup; + } + break; + + case NLMSG_DONE: + break; + + default: + goto malformed_resp; + } + + rc = 0; + cleanup: + nlmsg_free(nl_msg); + VIR_FREE(resp); + return rc; + + malformed_resp: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed netlink response message")); + goto cleanup; + + buffer_too_small: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + goto cleanup; +} + + int virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen) { @@ -803,6 +884,14 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, return -1; } + +int +virNetlinkDelLink(const char *ifname ATTRIBUTE_UNSUPPORTED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); + return -1; +} + /** * stopNetlinkEventServer: stop the monitor to receive netlink * messages for libvirtd diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 1a3e06d..06c3cd0 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2013, 2015 Red Hat, Inc. * Copyright (C) 2010-2012 IBM Corporation * * This library is free software; you can redistribute it and/or @@ -51,6 +51,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, struct nlmsghdr **resp, unsigned int *respbuflen, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups); +int virNetlinkDelLink(const char *ifname); int virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen); -- 2.1.0
On 03/23/2015 03:43 PM, Laine Stump wrote:
libvirt has always used the netlink RTM_DELLINK message to delete macvtap/macvlan devices, but it can actually be used to delete other types of network devices, such as bonds and bridges. This patch makes virNetDevMacVLanDelete() available as a generic function so it can intelligibly be called to delete these other types of interfaces. --- src/libvirt_private.syms | 1 + src/util/virnetlink.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virnetlink.h | 3 +- 3 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ca3520d..631edf3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1805,6 +1805,7 @@ virNetDevVPortProfileOpTypeToString;
# util/virnetlink.h virNetlinkCommand; +virNetlinkDelLink; virNetlinkEventAddClient; virNetlinkEventRemoveClient; virNetlinkEventServiceIsRunning; diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index d52f66a..86c9c9c 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -277,6 +277,87 @@ int virNetlinkCommand(struct nl_msg *nl_msg, }
+/** + * virNetlinkDelLink: + * + * @ifname: Name of the link + * + * delete a network "link" (aka interface aka device) with the given + * name. This works for many different types of network devices, + * including macvtap and bridges. + * + * Returns 0 on success, -1 on fatal error. + */ +int +virNetlinkDelLink(const char *ifname) +{ + int rc = -1; + struct nlmsghdr *resp = NULL; + struct nlmsgerr *err; + struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC }; + unsigned int recvbuflen; + struct nl_msg *nl_msg; + + nl_msg = nlmsg_alloc_simple(RTM_DELLINK, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); + if (!nl_msg) { + virReportOOMError(); + return -1; + } + + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) + goto buffer_too_small; + + if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) + goto buffer_too_small; + + if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, + NETLINK_ROUTE, 0) < 0) { + goto cleanup; + } + + if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) + goto malformed_resp; + + switch (resp->nlmsg_type) { + case NLMSG_ERROR: + err = (struct nlmsgerr *)NLMSG_DATA(resp); + if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) + goto malformed_resp; + + if (err->error) { + virReportSystemError(-err->error, + _("error destroying network device %s"), + ifname); + goto cleanup; + } + break; + + case NLMSG_DONE: + break; + + default: + goto malformed_resp; + } + + rc = 0; + cleanup: + nlmsg_free(nl_msg); + VIR_FREE(resp); + return rc; + + malformed_resp: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed netlink response message")); + goto cleanup; + + buffer_too_small: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + goto cleanup; +} + + int virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen) { @@ -803,6 +884,14 @@ int virNetlinkCommand(struct nl_msg *nl_msg ATTRIBUTE_UNUSED, return -1; }
+ +int +virNetlinkDelLink(const char *ifname ATTRIBUTE_UNSUPPORTED) +{ + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(unsupported)); + return -1; +} + /** * stopNetlinkEventServer: stop the monitor to receive netlink * messages for libvirtd diff --git a/src/util/virnetlink.h b/src/util/virnetlink.h index 1a3e06d..06c3cd0 100644 --- a/src/util/virnetlink.h +++ b/src/util/virnetlink.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2013, 2015 Red Hat, Inc. * Copyright (C) 2010-2012 IBM Corporation * * This library is free software; you can redistribute it and/or @@ -51,6 +51,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, struct nlmsghdr **resp, unsigned int *respbuflen, uint32_t src_pid, uint32_t dst_pid, unsigned int protocol, unsigned int groups); +int virNetlinkDelLink(const char *ifname);
Like the virNetDevMacVLanDelete which this is essentially "replacing" - add a : ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; ACK with the change John
int virNetlinkGetErrorCode(struct nlmsghdr *resp, unsigned int recvbuflen);
These two functions are identical, so no sense in having the duplication. I resisted the temptation to replace calls to virNetDevMacVLanDelete() with calls to virNetlinkDelLink() just in case some mythical future platform has macvtap devices that aren't managed with netlink (or in case we some day need to do more than just tell the kernel to delete the device). --- src/util/virnetdevmacvlan.c | 67 ++------------------------------------------- 1 file changed, 2 insertions(+), 65 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index ec959a9..5fd2097 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2014 Red Hat, Inc. + * Copyright (C) 2010-2015 Red Hat, Inc. * Copyright (C) 2010-2012 IBM Corporation * * This library is free software; you can redistribute it and/or @@ -220,70 +220,7 @@ virNetDevMacVLanCreate(const char *ifname, */ int virNetDevMacVLanDelete(const char *ifname) { - int rc = -1; - struct nlmsghdr *resp = NULL; - struct nlmsgerr *err; - struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC }; - unsigned int recvbuflen; - struct nl_msg *nl_msg; - - nl_msg = nlmsg_alloc_simple(RTM_DELLINK, - NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); - if (!nl_msg) { - virReportOOMError(); - return -1; - } - - if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) - goto buffer_too_small; - - if (nla_put(nl_msg, IFLA_IFNAME, strlen(ifname)+1, ifname) < 0) - goto buffer_too_small; - - if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, - NETLINK_ROUTE, 0) < 0) { - goto cleanup; - } - - if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) - goto malformed_resp; - - switch (resp->nlmsg_type) { - case NLMSG_ERROR: - err = (struct nlmsgerr *)NLMSG_DATA(resp); - if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) - goto malformed_resp; - - if (err->error) { - virReportSystemError(-err->error, - _("error destroying %s interface"), - ifname); - goto cleanup; - } - break; - - case NLMSG_DONE: - break; - - default: - goto malformed_resp; - } - - rc = 0; - cleanup: - nlmsg_free(nl_msg); - VIR_FREE(resp); - return rc; - - malformed_resp: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("malformed netlink response message")); - goto cleanup; - - buffer_too_small: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("allocated netlink buffer is too small")); - goto cleanup; + return virNetlinkDelLink(ifname); } -- 2.1.0
On 03/23/2015 03:43 PM, Laine Stump wrote:
These two functions are identical, so no sense in having the duplication. I resisted the temptation to replace calls to virNetDevMacVLanDelete() with calls to virNetlinkDelLink() just in case some mythical future platform has macvtap devices that aren't managed with netlink (or in case we some day need to do more than just tell the kernel to delete the device). --- src/util/virnetdevmacvlan.c | 67 ++------------------------------------------- 1 file changed, 2 insertions(+), 65 deletions(-)
ACK - John
On 03/23/2015 03:43 PM, Laine Stump wrote:
These two functions are identical, so no sense in having the duplication. I resisted the temptation to replace calls to virNetDevMacVLanDelete() with calls to virNetlinkDelLink() just in case some mythical future platform has macvtap devices that aren't managed with netlink (or in case we some day need to do more than just tell the kernel to delete the device). --- src/util/virnetdevmacvlan.c | 67 ++------------------------------------------- 1 file changed, 2 insertions(+), 65 deletions(-)
hmm interesting... Started reading the next patch and noticed something... Perhaps I was quick on the trigger finger for this one... This module was compiled with "if WITH_MACVTAP", but the API being replaced/called uses "#if defined(__linux__) && defined(HAVE_LIBNL)" thus this module never cared about linux & HAVE_LIBNL, but now depends on an API that does. My reading comprehension of the Makefile in order to determine whether anything matters is "limited"... Of course this module has libnl calls in it already so perhaps either WITH_MACVTAP implies __linux__ and HAVE_LIBNL or perhaps that something that needs to be adjusted. Not my specialty! John
On 03/24/2015 06:25 PM, John Ferlan wrote:
On 03/23/2015 03:43 PM, Laine Stump wrote:
These two functions are identical, so no sense in having the duplication. I resisted the temptation to replace calls to virNetDevMacVLanDelete() with calls to virNetlinkDelLink() just in case some mythical future platform has macvtap devices that aren't managed with netlink (or in case we some day need to do more than just tell the kernel to delete the device). --- src/util/virnetdevmacvlan.c | 67 ++------------------------------------------- 1 file changed, 2 insertions(+), 65 deletions(-)
hmm interesting... Started reading the next patch and noticed something... Perhaps I was quick on the trigger finger for this one...
This module was compiled with "if WITH_MACVTAP", but the API being replaced/called uses "#if defined(__linux__) && defined(HAVE_LIBNL)" thus this module never cared about linux & HAVE_LIBNL, but now depends on an API that does. My reading comprehension of the Makefile in order to determine whether anything matters is "limited"... Of course this module has libnl calls in it already so perhaps either WITH_MACVTAP implies __linux__ and HAVE_LIBNL or perhaps that something that needs to be adjusted.
configure assures that HAVE_LIBNL is true if --with-macvtap is requested, so it's not possible to have WITH_MACVTAP without HAVE_LIBNL. As for __linux__, I'm not even sure why that got in to begin with. I don't know that libnl (or netlink) is available on anything that isn't Linux. Definitely macvtap isn't on anything non-Linux though, so I'd say we're safe there as well.
Not my specialty!
but thankfully you weren't afraid to review it anyway. Thanks!
https://bugzilla.redhat.com/show_bug.cgi?id=1125755 reported that a stray bridge device was left on the system when a libvirt network failed to start due to an illegal iptables rule caused by bad config. Apparently the reason this was happening was that NetworkManager was noticing immediately when the bridge device was created and automatically setting it IFF_UP. libvirt would then try to setup the iptables rules, get an error back, and since libvirt had never IFF_UPed the bridge, it didn't expect that it needed to set it ~IFF_UP before deleting it during the cleanup process. But the ioctl(SIOCBRDELBR) ioctl will fail to delete a bridge if it is IFF_UP. Since that bug was reported, NetworkManager has gotten a bit more polite in this respect, but just in case something similar happens in the future, this patch switches to using the netlink RTM_DELLINK message to delete the bridge - unlike SIOCBRDELBR, it will delete the requested bridge no matter what the setting of IFF_UP. --- src/util/virnetdevbridge.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index d9dcc39..2bad40a 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2015 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -457,7 +457,15 @@ int virNetDevBridgeCreate(const char *brname) * * Returns 0 in case of success or an errno code in case of failure. */ -#if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRDELBR) +#if defined(__linux__) && defined(HAVE_LIBNL) +int virNetDevBridgeDelete(const char *brname) +{ + /* If netlink is available, use it, as it is successful at + * deleting a bridge even if it is currently IFF_UP. + */ + return virNetlinkDelLink(brname); +} +#elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRDELBR) int virNetDevBridgeDelete(const char *brname) { int fd = -1; -- 2.1.0
On 03/23/2015 03:43 PM, Laine Stump wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1125755
reported that a stray bridge device was left on the system when a libvirt network failed to start due to an illegal iptables rule caused by bad config. Apparently the reason this was happening was that NetworkManager was noticing immediately when the bridge device was created and automatically setting it IFF_UP. libvirt would then try to setup the iptables rules, get an error back, and since libvirt had never IFF_UPed the bridge, it didn't expect that it needed to set it ~IFF_UP before deleting it during the cleanup process. But the ioctl(SIOCBRDELBR) ioctl will fail to delete a bridge if it is IFF_UP.
Since that bug was reported, NetworkManager has gotten a bit more polite in this respect, but just in case something similar happens in the future, this patch switches to using the netlink RTM_DELLINK message to delete the bridge - unlike SIOCBRDELBR, it will delete the requested bridge no matter what the setting of IFF_UP. --- src/util/virnetdevbridge.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
ACK - John
Just as it is possible to delete a bridge device with the netlink RTM_DELLINK message, one can be created with the RTM_NEWLINK message. Because of differences in the format of the message, it's not as straightforward as with virNetlinkDelLink() to create a single utility function that can be used to create any type of interface, so the new netlink version of virNetDevBridgeCreate() does its own construction of the netlink message and calls virNetlinkCommand() itself. This doesn't provide any extra functionality, just provides symmetry with the previous commit. NB: We *could* alter the API of virNetDevBridgeCreate() to take a MAC address, and directly program that mac address into the bridge (by adding an IFLA_ADDRESS attribute, as is done in virNetDevMacVLanCreate()) rather than separately creating the "dummy tap" (e.g. virbr0-nic) to maintain a fixed mac address on the bridge, but the commit history of virnetdevbridge.c shows that the presence of this dummy tap is essential in some older versions of the kernel (between 2.6.39 and 3.1 or 3.2, possibly?) to proper operation of IPv6 DAD, and I don't want to take the chance of breaking something that I don't have the time/setup to test (my RHEL6 box is at kernel 2.6.32-544, and the next lowest kernel I have is 3.17) --- src/util/virnetdevbridge.c | 78 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-) diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c index 2bad40a..6be8aa3 100644 --- a/src/util/virnetdevbridge.c +++ b/src/util/virnetdevbridge.c @@ -395,7 +395,83 @@ virNetDevBridgePortSetUnicastFlood(const char *brname ATTRIBUTE_UNUSED, * * Returns 0 in case of success or -1 on failure */ -#if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR) +#if defined(__linux__) && defined(HAVE_LIBNL) +int virNetDevBridgeCreate(const char *brname) +{ + /* use a netlink RTM_NEWLINK message to create the bridge */ + const char *type = "bridge"; + int rc = -1; + struct nlmsghdr *resp = NULL; + struct nlmsgerr *err; + struct ifinfomsg ifinfo = { .ifi_family = AF_UNSPEC }; + unsigned int recvbuflen; + struct nl_msg *nl_msg; + struct nlattr *linkinfo; + + nl_msg = nlmsg_alloc_simple(RTM_NEWLINK, + NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL); + if (!nl_msg) { + virReportOOMError(); + return -1; + } + + if (nlmsg_append(nl_msg, &ifinfo, sizeof(ifinfo), NLMSG_ALIGNTO) < 0) + goto buffer_too_small; + if (nla_put(nl_msg, IFLA_IFNAME, strlen(brname)+1, brname) < 0) + goto buffer_too_small; + if (!(linkinfo = nla_nest_start(nl_msg, IFLA_LINKINFO))) + goto buffer_too_small; + if (nla_put(nl_msg, IFLA_INFO_KIND, strlen(type), type) < 0) + goto buffer_too_small; + nla_nest_end(nl_msg, linkinfo); + + if (virNetlinkCommand(nl_msg, &resp, &recvbuflen, 0, 0, + NETLINK_ROUTE, 0) < 0) { + goto cleanup; + } + + if (recvbuflen < NLMSG_LENGTH(0) || resp == NULL) + goto malformed_resp; + + switch (resp->nlmsg_type) { + case NLMSG_ERROR: + err = (struct nlmsgerr *)NLMSG_DATA(resp); + if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err))) + goto malformed_resp; + + switch (err->error) { + case 0: + break; + default: + virReportSystemError(-err->error, + _("error creating bridge interface %s"), + brname); + goto cleanup; + } + break; + + case NLMSG_DONE: + break; + default: + goto malformed_resp; + } + + rc = 0; + cleanup: + nlmsg_free(nl_msg); + VIR_FREE(resp); + return rc; + + malformed_resp: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("malformed netlink response message")); + goto cleanup; + buffer_too_small: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("allocated netlink buffer is too small")); + goto cleanup; +} +#elif defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRADDBR) int virNetDevBridgeCreate(const char *brname) { int fd = -1; -- 2.1.0
On 03/23/2015 03:43 PM, Laine Stump wrote:
Just as it is possible to delete a bridge device with the netlink RTM_DELLINK message, one can be created with the RTM_NEWLINK message. Because of differences in the format of the message, it's not as straightforward as with virNetlinkDelLink() to create a single utility function that can be used to create any type of interface, so the new netlink version of virNetDevBridgeCreate() does its own construction of the netlink message and calls virNetlinkCommand() itself.
This doesn't provide any extra functionality, just provides symmetry with the previous commit.
NB: We *could* alter the API of virNetDevBridgeCreate() to take a MAC address, and directly program that mac address into the bridge (by adding an IFLA_ADDRESS attribute, as is done in virNetDevMacVLanCreate()) rather than separately creating the "dummy tap" (e.g. virbr0-nic) to maintain a fixed mac address on the bridge, but the commit history of virnetdevbridge.c shows that the presence of this dummy tap is essential in some older versions of the kernel (between 2.6.39 and 3.1 or 3.2, possibly?) to proper operation of IPv6 DAD, and I don't want to take the chance of breaking something that I don't have the time/setup to test (my RHEL6 box is at kernel 2.6.32-544, and the next lowest kernel I have is 3.17) --- src/util/virnetdevbridge.c | 78 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-)
What's here seems reasonable - I don't have "history" on my side to answer the older versions of the kernel query... One other difference between this and virNetDevMacVLanCreate is the error path that handles *retry if the name is already in use... Whether that's a worthy addition here or not is your call... ACK - John
On 03/24/2015 06:39 PM, John Ferlan wrote:
On 03/23/2015 03:43 PM, Laine Stump wrote:
Just as it is possible to delete a bridge device with the netlink RTM_DELLINK message, one can be created with the RTM_NEWLINK message. Because of differences in the format of the message, it's not as straightforward as with virNetlinkDelLink() to create a single utility function that can be used to create any type of interface, so the new netlink version of virNetDevBridgeCreate() does its own construction of the netlink message and calls virNetlinkCommand() itself.
This doesn't provide any extra functionality, just provides symmetry with the previous commit.
NB: We *could* alter the API of virNetDevBridgeCreate() to take a MAC address, and directly program that mac address into the bridge (by adding an IFLA_ADDRESS attribute, as is done in virNetDevMacVLanCreate()) rather than separately creating the "dummy tap" (e.g. virbr0-nic) to maintain a fixed mac address on the bridge, but the commit history of virnetdevbridge.c shows that the presence of this dummy tap is essential in some older versions of the kernel (between 2.6.39 and 3.1 or 3.2, possibly?) to proper operation of IPv6 DAD, and I don't want to take the chance of breaking something that I don't have the time/setup to test (my RHEL6 box is at kernel 2.6.32-544, and the next lowest kernel I have is 3.17) --- src/util/virnetdevbridge.c | 78 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-)
What's here seems reasonable - I don't have "history" on my side to answer the older versions of the kernel query...
One other difference between this and virNetDevMacVLanCreate is the error path that handles *retry if the name is already in use...
Hmm. Interesting idea, but it wouldn't be useful with the current way that things work - unlike macvtap devices, which are created one per domain-interface and whose name we don't care about, by the time we get to the point that we want to create a bridge device (which is one for each network), we've already committed to the name we're going to use. Some restructuring of network startup could make that useful though. Still, I think I'll push this as it is for now (since it meets the current usage demands, and maintains the existing API for virNetDevBridgeCreate()) and think about building in EEXIST handling later (this could be especially problematic, since the same capability would need to be put into the other two implementations of virNetDevBridgeCreate(), and one of those is for a platform that I don't have setup to test on). Thanks for the reviews!
participants (2)
-
John Ferlan -
Laine Stump