Re: [libvirt] [PATCH 0/2] Fix bridge creation/deletion on 2.6.x kernels
On 08/26/2015 05:01 AM, Martin Kletzander wrote:
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.
Right - there is too much inormation that would be lost by leaving error
reporting up to the caller, and that leads to bug reports of the type
"This is broken." rather than "This has a problem with XYZ".
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
The "s" on the end of that is the crucial part - it would take two
functions, one for the part before checking the error code, and one for
the part after.
How about this: give virNetlinkDelLink() a new argument
"fallbackDeleteFunc" which would either be NULL, or a pointer to
virNetDevBridgeDeleteWithIoctl() (in this case)? That avoids needing to
split virNetlinkDelLink into multiple parts as well as the duplication
of code, and I think it's much less obtuse than the boolean thing of V1.