[libvirt] [PATCH] qemu: cleanup tap devices on FreeBSD
We have to explicitly destroy TAP devices on FreeBSD because they're not freed after being closed, otherwise we end up with orphaned TAP devices after destroying a domain. --- src/qemu/qemu_process.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ffa939a..9181423 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -61,6 +61,7 @@ #include "viruuid.h" #include "virprocess.h" #include "virtime.h" +#include "virnetdevbridge.h" #include "virnetdevtap.h" #include "virbitmap.h" #include "viratomic.h" @@ -4381,6 +4382,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainNetGetActualVirtPortProfile(net), cfg->stateDir)); VIR_FREE(net->ifname); + } else if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_BRIDGE) { + /* On some OSes, e.g. FreeBSD, tap devices are not cleaned up when released, + * so we have to handle that manually */ + ignore_value(virNetDevBridgeRemovePort(virDomainNetGetActualBridgeName(net), + net->ifname)); + ignore_value(virNetDevTapDelete(net->ifname)); } /* release the physical device (or any other resources used by * this interface in the network driver -- 1.8.4.3
On Mon, Feb 24, 2014 at 11:12:33PM +0400, Roman Bogorodskiy wrote:
We have to explicitly destroy TAP devices on FreeBSD because they're not freed after being closed, otherwise we end up with orphaned TAP devices after destroying a domain. --- src/qemu/qemu_process.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ffa939a..9181423 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -61,6 +61,7 @@ #include "viruuid.h" #include "virprocess.h" #include "virtime.h" +#include "virnetdevbridge.h" #include "virnetdevtap.h" #include "virbitmap.h" #include "viratomic.h" @@ -4381,6 +4382,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainNetGetActualVirtPortProfile(net), cfg->stateDir)); VIR_FREE(net->ifname); + } else if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_BRIDGE) { + /* On some OSes, e.g. FreeBSD, tap devices are not cleaned up when released, + * so we have to handle that manually */ + ignore_value(virNetDevBridgeRemovePort(virDomainNetGetActualBridgeName(net), + net->ifname)); + ignore_value(virNetDevTapDelete(net->ifname)); } /* release the physical device (or any other resources used by * this interface in the network driver
Conceptually ok, but I fear this impl will result in the logs getting polluted with "cannot delete tap device" or similar log messages on OS where TAP device deletion is automatic. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Daniel P. Berrange wrote:
On Mon, Feb 24, 2014 at 11:12:33PM +0400, Roman Bogorodskiy wrote:
We have to explicitly destroy TAP devices on FreeBSD because they're not freed after being closed, otherwise we end up with orphaned TAP devices after destroying a domain. --- src/qemu/qemu_process.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ffa939a..9181423 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -61,6 +61,7 @@ #include "viruuid.h" #include "virprocess.h" #include "virtime.h" +#include "virnetdevbridge.h" #include "virnetdevtap.h" #include "virbitmap.h" #include "viratomic.h" @@ -4381,6 +4382,12 @@ void qemuProcessStop(virQEMUDriverPtr driver, virDomainNetGetActualVirtPortProfile(net), cfg->stateDir)); VIR_FREE(net->ifname); + } else if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_BRIDGE) { + /* On some OSes, e.g. FreeBSD, tap devices are not cleaned up when released, + * so we have to handle that manually */ + ignore_value(virNetDevBridgeRemovePort(virDomainNetGetActualBridgeName(net), + net->ifname)); + ignore_value(virNetDevTapDelete(net->ifname)); } /* release the physical device (or any other resources used by * this interface in the network driver
Conceptually ok, but I fear this impl will result in the logs getting polluted with "cannot delete tap device" or similar log messages on OS where TAP device deletion is automatic.
Do you have an idea about better way to do that? Maybe we could create something opposite to virNetDevTapCreateInBridgePort(), e.g. virNetDevTapReleaseInBridgePort() which would destroy tap device on FreeBSD and do nothing on Linux? But that will still require platform specific #ifdef's because checking tap releasing feels like a little out of scope of configure checks. Roman Bogorodskiy
On 02/26/2014 08:57 AM, Roman Bogorodskiy wrote:
Conceptually ok, but I fear this impl will result in the logs getting polluted with "cannot delete tap device" or similar log messages on OS where TAP device deletion is automatic.
Do you have an idea about better way to do that?
Maybe add a boolean parameter that says whether the function should be quiet to avoid log noise, where the default is noisy but this cleanup path is quiet. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Feb 26, 2014 at 09:12:11AM -0700, Eric Blake wrote:
On 02/26/2014 08:57 AM, Roman Bogorodskiy wrote:
Conceptually ok, but I fear this impl will result in the logs getting polluted with "cannot delete tap device" or similar log messages on OS where TAP device deletion is automatic.
Do you have an idea about better way to do that?
Maybe add a boolean parameter that says whether the function should be quiet to avoid log noise, where the default is noisy but this cleanup path is quiet.
Or just make it #ifdef BSD, or call virNetDevExists() to check it ? Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Daniel P. Berrange wrote:
On Wed, Feb 26, 2014 at 09:12:11AM -0700, Eric Blake wrote:
On 02/26/2014 08:57 AM, Roman Bogorodskiy wrote:
Conceptually ok, but I fear this impl will result in the logs getting polluted with "cannot delete tap device" or similar log messages on OS where TAP device deletion is automatic.
Do you have an idea about better way to do that?
Maybe add a boolean parameter that says whether the function should be quiet to avoid log noise, where the default is noisy but this cleanup path is quiet.
Or just make it #ifdef BSD, or call virNetDevExists() to check it ?
I think I came to an #idef based idea which looks more or less easy to follow: - in virnetdevtap.h, define something like VIR_TAP_NEEDS_MANUAL_CLEANUP on certain OSes (i.e. FreeBSD only at this point) - Use it in qemu_process.c (that should be a little more readable than #ifdef __FreeBSD__) Roman Bogorodskiy
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Roman Bogorodskiy