On Tue, Aug 25, 2015 at 11:47:53PM -0400, Laine Stump wrote:
These two patches fix this bug:https://bugzilla.redhat.com/show_bug.cgi?id=1252780 which was also separate reported in libvirt-users a couple days ago. Most people running CentOS6 or RHEL6 also run the downstream version of libvirt-0.10.2 that has many patches backported from later upstream releases, but not the patch described in the commit logs of these proposed patches. And the problem being fixed here only occurs on a 2.6.x kernel, while most people running upstream libvirt are running something much more modern. That's why not many people have seen the bug that these patches fix. Still, we claim to fully support upstream on RHEL6/CentOS6, so we'd better do it right! Two notes: 1) I tried timing 20 iterations of net-start...net-delete with these patches vs. a libvirt build that simply calls the ioctls in the beginnng (i.e. it doesn't try netlink then fall back to ioctl if that fails), and the time to completion was within 0.25%, so the extra overhead of trying one then the other isn't significant even on the older machines that will always fall back to ioctl. 2) There are 2 different versions of Patch 2/2. I did the first one as an exercise to see how ugly it would be to *not* duplicate the entire virNetlinkDelLink() function inside the new virNetDevBridgeDelete(). It was pretty ugly, so I made a cleaner version that duplicates the code instead. I would be okay pushing either version, but I think I slightly prefer the one that duplicates code and remains uncomplicated.
To be honest, I dislike both approaches. Sure, the second one is not that messy, but looking at those patches I have another idea. At first I thought that since there are not that many callers of virNetlinkDelLink(), what if we did not report the error inside that function at all and just return the errno and callers would report errors themselves. But that's not good either, virNetlinkDelLink() has very nice error reporting. So what if we took the second version, but the code that's duplicated (e.g. preparing netlink messages) that can fail by itself could be moved into its own functions that would be called in both virNetlinkDelLink() and virNetDevBridgeDelete() so that the duplication would be minimal and more prone to differentiation in the future.
Laine Stump (2): util: fallback to ioctl(SIOCBRADDBR) if netlink RTM_NEWLINK fails util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails src/util/virnetdevbridge.c | 112 ++++++++++++++++++++++++++++++++------------ src/util/virnetdevmacvlan.c | 2 +- src/util/virnetlink.c | 17 +++++-- src/util/virnetlink.h | 2 +- 4 files changed, 98 insertions(+), 35 deletions(-) -- 2.1.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
