[libvirt] [PATCH 2/2 (v3)] util: fallback to ioctl(SIOCBRDELBR) if netlink RTM_DELLINK fails

[Laine Stump]

On 08/27/2015 06:07 AM, Martin Kletzander wrote:
> On Wed, Aug 26, 2015 at 03:05:25PM -0400, Laine Stump wrote:
>> commit 09778e09 switched from using ioctl(SIOCBRDELBR) for bridge
>> device deletion to using a netlink RTM_DELLINK message, which is the
>> more modern way to delete a bridge (and also doesn't require the
>> bridge to be ~IFF_UP to succeed). However, although older kernels
>> (e.g. 2.6.32, in RHEL6/CentOS6) support deleting *some* link types
>> with RTM_NEWLINK, they don't support deleting bridges, and there is no
>> compile-time way to figure this out.
>>
>> This patch moves the body of the SIOCBRDELBR version of
>> virNetDevBridgeDelete() into a static function, calls the new function
>> from the original, and also calls the new function from the
>> RTM_DELLINK version if the RTM_DELLINK message generates an EOPNOTSUPP
>> error. Since RTM_DELLINK is done from the subordinate function
>> virNetlinkDelLink, which is also called for other purposes (deleting a
>> macvtap interface), a function pointer called "fallback" has been
>> added to the arglist of virNetlinkDelLink() - if that arg != NULL, the
>> provided function will be called when (and only when) RTM_DELLINK
>> fails with EOPNOTSUPP.
>>
>> Resolves:  https://bugzilla.redhat.com/show_bug.cgi?id=1252780 (part 2)
>> ---
>> Another idea for doing this I came up with during review of the
>> others...
>>
>> src/util/virnetdevbridge.c  | 41
>> ++++++++++++++++++++++++++++++-----------
>> src/util/virnetdevmacvlan.c |  2 +-
>> src/util/virnetlink.c       | 13 +++++++++++--
>> src/util/virnetlink.h       |  5 ++++-
>> 4 files changed, 46 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/util/virnetdevbridge.c b/src/util/virnetdevbridge.c
>> index ae38901..3b06829 100644
>> --- a/src/util/virnetdevbridge.c
>> +++ b/src/util/virnetdevbridge.c
>> @@ -558,20 +558,15 @@ int virNetDevBridgeCreate(const char *brname)
>>  *
>>  * Returns 0 in case of success or an errno code in case of failure.
>>  */
>> -#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)
>> +#if defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRDELBR)
>> +static int
>> +virNetDevBridgeDeleteWithIoctl(const char *brname)
>> {
>>     int fd = -1;
>>     int ret = -1;
>>
>> +    ignore_value(virNetDevSetOnline(brname, false));
>> +
>>     if ((fd = virNetDevSetupControl(NULL, NULL)) < 0)
>>         return -1;
>>
>> @@ -587,8 +582,32 @@ int virNetDevBridgeDelete(const char *brname)
>>     VIR_FORCE_CLOSE(fd);
>>     return ret;
>> }
>> +#endif
>> +
>> +
>> +#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. fallback to
>> +     * using ioctl(SIOCBRDELBR) if netlink fails with EOPNOTSUPP.
>> +     */
>> +    return virNetlinkDelLink(brname, virNetDevBridgeDeleteWithIoctl);
>
> You're passing DeleteWithIoctl here, but that was defined only if
> defined(HAVE_STRUCT_IFREQ) && defined(SIOCBRDELBR), does it mean that
> if you have libnl on linux, you also have those two?  If not, then I
> suggest adding check for HAVE_STRUCT_IFREQ && SIOCBRDELBR definitions
> atop this function definition as well, just to make sure we don't run
> into similar problems later on with some weird distribution/custom
> builds/versions.  No need to resend, this is just a hint before
> pushing.

Yeah, thanks for pointing that out. I had that check in v2, but forgot
it here.