[PATCHv3 5/5] netdev: Prevent functions that call virNetDevTapCreate from adding 'vnet%d' into ifname

[Laine Stump]

(NB: As I looked further into the details of this, I decided it would be 
simpler for me to just post a few patches to take the place of this one 
patch. I've left in the reasoning for a couple of those patches which I 
had typed before I made the decision to just make a new series, but this 
patch can just be dropped, as the series I just sent here:

https://www.redhat.com/archives/libvir-list/2020-December/msg00744.html

takes care of everything done here, plus some other things.)

On 12/13/20 8:50 PM, Shi Lei wrote:
> Those functions that call virNetDevTapCreate don't need to adding
> 'vnet%d' into ifname when it is empty, since virNetDevGenerateName
> which is in virNetDevTapCreate can deal with it.
> 
> Signed-off-by: Shi Lei <shi_lei at massclouds.com>
> ---
>   src/bhyve/bhyve_command.c |  1 -
>   src/qemu/qemu_interface.c | 12 ------------
>   2 files changed, 13 deletions(-)
> 
> diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c
> index 4cf98c0e..92b31a6e 100644
> --- a/src/bhyve/bhyve_command.c
> +++ b/src/bhyve/bhyve_command.c
> @@ -83,7 +83,6 @@ bhyveBuildNetArgStr(const virDomainDef *def,
>           STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
>           strchr(net->ifname, '%')) {
>           VIR_FREE(net->ifname);
> -        net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
>       }


Actually

1) this entire clause can be eliminated - from the start if the "if (" 
to the "}" just before my comment - it is a remnant of code added all 
the way back in 2007  (commit a8977b62ba), but we have been doing the 
same clearing of "vnetN" generated names in the parser itself since 
commit 282d135d (2008), and this was made more complete/consistent in 
commit 77f72a861 (August 2019) which gives more details of the history 
(in case you're interested, which you probably aren't, and shouldn't be 
:-)). This is in Patch 2 of the patchset linked above.

2) Hmm. But I just noticed that in the case of the virNetDevCreateTap() 
used by FreeBSD/OpenBSD, we actually had forgotten to switch it to use 
virNetDevGenerateName() in your patches! I've remedied that in Patch 1 
of the new patchset I linked above).

>   
>       if (!dryRun) {
> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
> index 197c0aa2..87cfb8fc 100644
> --- a/src/qemu/qemu_interface.c
> +++ b/src/qemu/qemu_interface.c
> @@ -413,7 +413,6 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
>       virMacAddr tapmac;
>       int ret = -1;
>       unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
> -    bool template_ifname = false;

This bool was added by commit 2b1f67d418 from 2009, and is used to clear 
out the autogenerated ifname in case of failure. We do want to preserve 
that behavior (in order to avoid some unwanted "cleaning up what isn't 
there" during failure), so while we can get rid of everything about 
adding VIR_NET_GENERATED_VNET_PREFIX + "%d", we need to remember that we 
generated this name, and clear it during a failure exit from the 
function (see Patch 3 of my new patchset)

>       g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>       const char *tunpath = "/dev/net/tun";
>       const char *auditdev = tunpath;
> @@ -459,9 +458,6 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
>               STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
>               strchr(net->ifname, '%')) {
>               VIR_FREE(net->ifname);
> -            net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
> -            /* avoid exposing vnet%d in getXMLDesc or error outputs */
> -            template_ifname = true;
>           }
>           if (virNetDevTapCreate(&net->ifname, tunpath, tapfd, tapfdSize,
>                                  tap_create_flags) < 0) {
> @@ -512,8 +508,6 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
>           virDomainAuditNetDevice(def, net, auditdev, false);
>           for (i = 0; i < tapfdSize && tapfd[i] >= 0; i++)
>               VIR_FORCE_CLOSE(tapfd[i]);
> -        if (template_ifname)
> -            VIR_FREE(net->ifname);
>       }
>   
>       return ret;
> @@ -541,7 +535,6 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
>       const char *brname;
>       int ret = -1;
>       unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP;
> -    bool template_ifname = false;
>       g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
>       const char *tunpath = "/dev/net/tun";
>   
> @@ -563,9 +556,6 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
>           STRPREFIX(net->ifname, VIR_NET_GENERATED_VNET_PREFIX) ||
>           strchr(net->ifname, '%')) {
>           VIR_FREE(net->ifname);
> -        net->ifname = g_strdup(VIR_NET_GENERATED_VNET_PREFIX "%d");
> -        /* avoid exposing vnet%d in getXMLDesc or error outputs */
> -        template_ifname = true;
>       }
>   
>       if (qemuInterfaceIsVnetCompatModel(net))
> @@ -630,8 +620,6 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
>           size_t i;
>           for (i = 0; i < *tapfdSize && tapfd[i] >= 0; i++)
>               VIR_FORCE_CLOSE(tapfd[i]);
> -        if (template_ifname)
> -            VIR_FREE(net->ifname);
>       }
>   
>       return ret;
>