Re: [PATCH v2 0/2] virnetdevtap.c: Disallow pre-existing TAP devices
On 12/8/22 11:59 AM, Michal Privoznik wrote:
v2 of:
https://listman.redhat.com/archives/libvir-list/2022-December/236197.html
diff to v1:
- Check for existing device iff virNetDevGenerateName() returned early
(per Laine's suggestion).
Michal Prívozník (2):
virnetdev: Make virNetDevGenerateName() return 1 if no name was
generated
virnetdevtap.c: Disallow pre-existing TAP devices
src/qemu/qemu_interface.c | 2 ++
src/util/virnetdev.c | 6 ++++--
src/util/virnetdevtap.c | 23 +++++++++++++++++++++--
src/util/virnetdevtap.h | 2 ++
4 files changed, 29 insertions(+), 4 deletions(-)
Reviewed-by: Laine Stump <laine(a)redhat.com>
(As I was reviewing, I realized I only got half of the story about the
FreeeBSD virNetDevTapCreate(), and there is actually a 2nd pre-existing
bug - since FreeBSD creates the tap device by creating a device and then
renaming it to the desired name, not only is it not necessary to
separately check for a pre-existing tap in order to avoid the
"unreported error" that your patch fixes for tap devices on Linux, but
*also* it means that on FreeBSD there is no way that we can use a
pre-existing tap device when we actually want to (i.e. when managed='no'
is set in <target>).
So instead of just telling you that the call to virNetDevExists() isn't
needed in order to prevent silently opening an existing device when we
don't want to allow it, I should have also said that there should be a
check for virNetDevExists(), but it should instead be used to alter the
logic in the case that your newly added ALLOW_EXISTING flag is set.
Since it's fixing a totally separate problem, it should be a separate
patch anyway though. I can send an RFC patch for that, but haven't had a
working FreeBSD system since 1998 or so (although I did spin up a VM
maybe 5 years ago) so I would have nothing to test it on other than
verifying it could compile with libvirt CI.)