[PATCH 0/5] Followup to virNetDevGenerateName() patches
A few issues came up during review of this series that were better fixed in cleanups rather than requiring yet another round of review. (A couple of things I noticed after the other patches were already pushed). Laine Stump (5): util: fix tap device name auto-generation for FreeBSD bhyve: remove redundant code that adds "template" netdev name qemu: remove redundant code that adds "template" netdev name util: simplify virNetDevMacVLanCreateWithVPortProfile() util: minor comment/formatting changes to virNetDevTapCreate() src/bhyve/bhyve_command.c | 7 ---- src/qemu/qemu_interface.c | 18 +++-------- src/util/virnetdevmacvlan.c | 64 ++++++------------------------------- src/util/virnetdevtap.c | 46 +++++++------------------- 4 files changed, 25 insertions(+), 110 deletions(-) -- 2.28.0
The Linux implementation of virNetDevCreate() no longer requires a template ifname (e.g. "vnet%d") when it is called, but just generates a new name if ifname is empty. The FreeBSD implementation requires that the caller actually fill in a template ifname, and will fail if ifname is empty. Since we want to eliminate all the special code in callers that is setting the template name, we need to make the behavior of the FreeBSD virNetDevCreate() match the behavior of the Linux virNetDevCreate(). The simplest way to do this is to use the new virNetDevGenerateName() function - if ifname is empty it generates a new name with the proper prefix, and if it's not empty, it leaves it alone. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virnetdevtap.c | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 88ad321627..cca2f614fe 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -308,7 +308,6 @@ int virNetDevTapCreate(char **ifname, int s; struct ifreq ifr; int ret = -1; - char *newifname = NULL; if (tapfdSize > 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -316,6 +315,12 @@ int virNetDevTapCreate(char **ifname, goto cleanup; } + /* auto-generate an unused name for the new device (this + * is NOP if a name has been provided) + */ + if (virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET) < 0) + return -1; + /* As FreeBSD determines interface type by name, * we have to create 'tap' interface first and * then rename it to 'vnet' @@ -329,34 +334,6 @@ int virNetDevTapCreate(char **ifname, goto cleanup; } - /* In case we were given exact interface name (e.g. 'vnetN'), - * we just rename to it. If we have format string like - * 'vnet%d', we need to find the first available name that - * matches this pattern - */ - if (strstr(*ifname, "%d") != NULL) { - size_t i; - for (i = 0; i <= IF_MAXUNIT; i++) { - g_autofree char *newname = NULL; - - newname = g_strdup_printf(*ifname, i); - - if (virNetDevExists(newname) == 0) { - newifname = g_steal_pointer(&newname); - break; - } - } - if (newifname) { - VIR_FREE(*ifname); - *ifname = newifname; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to generate new name for interface %s"), - ifr.ifr_name); - goto cleanup; - } - } - if (tapfd) { g_autofree char *dev_path = NULL; dev_path = g_strdup_printf("/dev/%s", ifr.ifr_name); -- 2.28.0
The FreeBSD version of virNetDevTapCreate() now calls virNetDevGenerateName(), and virNetDevGenerateName() understands that a blank ifname should be replaced with a generated name based on a device-type-specific template - so there is no longer any need for the higher level functions to stuff a template name ("vnet%d") into ifname. Signed-off-by: Laine Stump <laine@redhat.com> --- src/bhyve/bhyve_command.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 4cf98c0eb1..daf313c9c1 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -79,13 +79,6 @@ bhyveBuildNetArgStr(const virDomainDef *def, goto cleanup; } - if (!net->ifname || - 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"); - } - if (!dryRun) { if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, def->uuid, NULL, NULL, 0, -- 2.28.0
The lower level function virNetDevGenerateName() now understands that a blank ifname should be replaced with a generated name based on a template that it knows about itself - there is no need for the higher level functions to stuff a template name ("vnet%d") into ifname. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_interface.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index 197c0aa239..be2f53945c 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -455,14 +455,10 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, goto cleanup; } } else { - if (!net->ifname || - 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 */ + + if (!net->ifname) template_ifname = true; - } + if (virNetDevTapCreate(&net->ifname, tunpath, tapfd, tapfdSize, tap_create_flags) < 0) { goto cleanup; @@ -559,14 +555,8 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def, goto cleanup; } - if (!net->ifname || - 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 */ + if (!net->ifname) template_ifname = true; - } if (qemuInterfaceIsVnetCompatModel(net)) tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR; -- 2.28.0
Since commit 282d135ddbb the parser for <interface> has cleared out any interface name from the input XML that used the macvtap/macvlan name as a prefix. Along with that, the switch to use the new virNetDevGenerateName() function for auto-generating macvtap/macvlan device names (commit 9b5d741a9), has realized two facts: 1) virNetDevGenerateName() can be called with a name already filled in, and in that case it is an effective NOP. 2) because virNetDevGenerate() will always find an unused name, there is no need to retry device creation in a loop - if it fails the first time, it would fail any subsequent time as well. that, combined with the aforementioned parser change allow us to simplify virNetDevMacVLanCreateWithVPortProfile() - we no longer need any extra code to determine if a template "AutoName" was requested, and don't need a separate code path for creating the device in the case that a specific name was given in the XML - all we need to do is log any requested name, and then call exactly the same code as we would if no name was given. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virnetdevmacvlan.c | 64 ++++++------------------------------- 1 file changed, 10 insertions(+), 54 deletions(-) diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c index a4ad698335..2deefe6589 100644 --- a/src/util/virnetdevmacvlan.c +++ b/src/util/virnetdevmacvlan.c @@ -673,6 +673,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, uint32_t macvtapMode; int vf = -1; bool vnet_hdr = flags & VIR_NETDEV_MACVLAN_VNET_HDR; + virNetDevGenNameType type; macvtapMode = modeMap[mode]; @@ -706,66 +707,21 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, } if (ifnameRequested) { - int rc; - bool isAutoName - = (STRPREFIX(ifnameRequested, VIR_NET_GENERATED_MACVTAP_PREFIX) || - STRPREFIX(ifnameRequested, VIR_NET_GENERATED_MACVLAN_PREFIX)); - VIR_INFO("Requested macvtap device name: %s", ifnameRequested); - - if ((rc = virNetDevExists(ifnameRequested)) < 0) - return -1; - - if (rc) { - /* ifnameRequested is already being used */ - - if (!isAutoName) { - virReportSystemError(EEXIST, - _("Unable to create device '%s'"), - ifnameRequested); - return -1; - } - } else { - - /* ifnameRequested is available. try to open it */ - - virNetDevReserveName(ifnameRequested); - - if (virNetDevMacVLanCreate(ifnameRequested, macaddress, - linkdev, macvtapMode, flags) == 0) { - - /* virNetDevMacVLanCreate() was successful - use this name */ - ifname = g_strdup(ifnameRequested); - - } else if (!isAutoName) { - /* couldn't open ifnameRequested, but it wasn't an - * autogenerated named, so there is nothing else to - * try - fail and return. - */ - return -1; - } - } + ifname = g_strdup(ifnameRequested); } - if (!ifname) { - /* ifnameRequested was NULL, or it was an already in use - * autogenerated name, so now we look for an unused - * autogenerated name. - */ - virNetDevGenNameType type; - if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) - type = VIR_NET_DEV_GEN_NAME_MACVTAP; - else - type = VIR_NET_DEV_GEN_NAME_MACVLAN; + if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) + type = VIR_NET_DEV_GEN_NAME_MACVTAP; + else + type = VIR_NET_DEV_GEN_NAME_MACVLAN; - if (virNetDevGenerateName(&ifname, type) < 0 || - virNetDevMacVLanCreate(ifname, macaddress, - linkdev, macvtapMode, flags) < 0) - return -1; + if (virNetDevGenerateName(&ifname, type) < 0 || + virNetDevMacVLanCreate(ifname, macaddress, + linkdev, macvtapMode, flags) < 0) { + return -1; } - /* all done creating the device */ - if (virNetDevVPortProfileAssociate(ifname, virtPortProfile, macaddress, -- 2.28.0
The comment about auto-generating names was obsoleted by recent changes, and there was an unnecessary set of braces around a single line conditional body. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virnetdevtap.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index cca2f614fe..8649978e53 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -174,15 +174,14 @@ int virNetDevTapCreate(char **ifname, int ret = -1; int fd = -1; - /* if ifname is "vnet%d", then auto-generate a name for the new + /* if ifname is empty, then auto-generate a name for the new * device (the kernel could do this for us, but has a bad habit of * immediately re-using names that have just been released, which - * can lead to race conditions). - * if ifname is just a user-provided name, virNetDevGenerateName - * leaves it unchanged. */ - if (virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET) < 0) { + * can lead to race conditions). if ifname is just a + * user-provided name, virNetDevGenerateName leaves it + * unchanged. */ + if (virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET) < 0) return -1; - } if (!tunpath) tunpath = "/dev/net/tun"; -- 2.28.0
the lxc driver uses virNetDevGenerateName() for its veth device names since patch 2dd0fb492, so it should be using virNetDevReserveName() during daemon restart/reconnect to skip over the device names that are in use. Signed-off-by: Laine Stump <laine@redhat.com> --- I meant to mention this during review of the abovementioned patch, but forgot. (NB: a couple days ago I *removed* similar code from this same spot, but it was trying to reserve the name of macvlan devices; a macvlan device is moved into the container's namespace at startup, so it is not visible to the host anyway. This new case is for the 1/2 of a veth pair that does remain in the host's namespace (type='bridge|network|ethernet' use a veth pair) src/lxc/lxc_process.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0f7c929535..a842ac91c5 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1640,6 +1640,30 @@ virLXCProcessReconnectNotifyNets(virDomainDefPtr def) for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; + /* type='bridge|network|ethernet' interfaces may be using an + * autogenerated netdev name, so we should update the counter + * for autogenerated names to skip past this one. + */ + switch (virDomainNetGetActualType(net)) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + virNetDevReserveName(net->ifname); + break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_VDPA: + case VIR_DOMAIN_NET_TYPE_LAST: + break; + } + if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (!conn && !(conn = virGetConnectNetwork())) continue; -- 2.29.2
On 12/16/20 2:27 AM, Laine Stump wrote:
A few issues came up during review of this series that were better fixed in cleanups rather than requiring yet another round of review. (A couple of things I noticed after the other patches were already pushed).
Laine Stump (5): util: fix tap device name auto-generation for FreeBSD bhyve: remove redundant code that adds "template" netdev name qemu: remove redundant code that adds "template" netdev name util: simplify virNetDevMacVLanCreateWithVPortProfile() util: minor comment/formatting changes to virNetDevTapCreate()
src/bhyve/bhyve_command.c | 7 ---- src/qemu/qemu_interface.c | 18 +++-------- src/util/virnetdevmacvlan.c | 64 ++++++------------------------------- src/util/virnetdevtap.c | 46 +++++++------------------- 4 files changed, 25 insertions(+), 110 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On 12/16/20 9:13 PM, Laine Stump wrote:
the lxc driver uses virNetDevGenerateName() for its veth device names since patch 2dd0fb492, so it should be using virNetDevReserveName() during daemon restart/reconnect to skip over the device names that are in use.
Signed-off-by: Laine Stump <laine@redhat.com> ---
I meant to mention this during review of the abovementioned patch, but forgot.
(NB: a couple days ago I *removed* similar code from this same spot, but it was trying to reserve the name of macvlan devices; a macvlan device is moved into the container's namespace at startup, so it is not visible to the host anyway. This new case is for the 1/2 of a veth pair that does remain in the host's namespace (type='bridge|network|ethernet' use a veth pair)
src/lxc/lxc_process.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0f7c929535..a842ac91c5 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1640,6 +1640,30 @@ virLXCProcessReconnectNotifyNets(virDomainDefPtr def) for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i];
+ /* type='bridge|network|ethernet' interfaces may be using an + * autogenerated netdev name, so we should update the counter + * for autogenerated names to skip past this one. + */ + switch (virDomainNetGetActualType(net)) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + virNetDevReserveName(net->ifname); + break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_VDPA: + case VIR_DOMAIN_NET_TYPE_LAST: + break; + } +
I remember Peter being picky about switch()-es and (almost) always he wanted me to add the default case with virReportEnumRangeError() despite the variable passed to switch() being verified earlier. IIUC his reasoning was that if we had a memory being overwritten somewhere it's better to error out (I say it's better to crash), but since I don't care that much, this could have: default: virReportEnumRangeError(virDomainNetType, virDomainNetGetActualType(net)); return; At your discretion.
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (!conn && !(conn = virGetConnectNetwork())) continue;
Michal
On 12/16/20 2:27 AM, Laine Stump wrote:
The Linux implementation of virNetDevCreate() no longer requires a template ifname (e.g. "vnet%d") when it is called, but just generates a new name if ifname is empty. The FreeBSD implementation requires that the caller actually fill in a template ifname, and will fail if ifname is empty. Since we want to eliminate all the special code in callers that is setting the template name, we need to make the behavior of the FreeBSD virNetDevCreate() match the behavior of the Linux virNetDevCreate().
The simplest way to do this is to use the new virNetDevGenerateName() function - if ifname is empty it generates a new name with the proper prefix, and if it's not empty, it leaves it alone.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virnetdevtap.c | 35 ++++++----------------------------- 1 file changed, 6 insertions(+), 29 deletions(-)
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 88ad321627..cca2f614fe 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -308,7 +308,6 @@ int virNetDevTapCreate(char **ifname, int s; struct ifreq ifr; int ret = -1; - char *newifname = NULL;
if (tapfdSize > 1) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -316,6 +315,12 @@ int virNetDevTapCreate(char **ifname, goto cleanup; }
+ /* auto-generate an unused name for the new device (this + * is NOP if a name has been provided) + */ + if (virNetDevGenerateName(ifname, VIR_NET_DEV_GEN_NAME_VNET) < 0) + return -1; +
This line ^^ contains some trailing spaces.
/* As FreeBSD determines interface type by name, * we have to create 'tap' interface first and * then rename it to 'vnet'
Michal
On 12/16/20 4:27 PM, Michal Privoznik wrote:
On 12/16/20 9:13 PM, Laine Stump wrote:
the lxc driver uses virNetDevGenerateName() for its veth device names since patch 2dd0fb492, so it should be using virNetDevReserveName() during daemon restart/reconnect to skip over the device names that are in use.
Signed-off-by: Laine Stump <laine@redhat.com> ---
I meant to mention this during review of the abovementioned patch, but forgot.
(NB: a couple days ago I *removed* similar code from this same spot, but it was trying to reserve the name of macvlan devices; a macvlan device is moved into the container's namespace at startup, so it is not visible to the host anyway. This new case is for the 1/2 of a veth pair that does remain in the host's namespace (type='bridge|network|ethernet' use a veth pair)
src/lxc/lxc_process.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0f7c929535..a842ac91c5 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1640,6 +1640,30 @@ virLXCProcessReconnectNotifyNets(virDomainDefPtr def) for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; + /* type='bridge|network|ethernet' interfaces may be using an + * autogenerated netdev name, so we should update the counter + * for autogenerated names to skip past this one. + */ + switch (virDomainNetGetActualType(net)) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + virNetDevReserveName(net->ifname); + break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_VDPA: + case VIR_DOMAIN_NET_TYPE_LAST: + break; + } +
I remember Peter being picky about switch()-es and (almost) always he wanted me to add the default case with virReportEnumRangeError() despite the variable passed to switch() being verified earlier. IIUC his reasoning was that if we had a memory being overwritten somewhere it's better to error out (I say it's better to crash), but since I don't care that much, this could have:
default: virReportEnumRangeError(virDomainNetType, virDomainNetGetActualType(net)); return;
But if we add a default case to the switch, we won't get all the nice compile-time errors when we add a new value to the enum and forget to add it to every switch, will we?
At your discretion.
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { if (!conn && !(conn = virGetConnectNetwork())) continue;
Michal
On 12/17/20 1:28 AM, Laine Stump wrote:
On 12/16/20 4:27 PM, Michal Privoznik wrote:
On 12/16/20 9:13 PM, Laine Stump wrote:
the lxc driver uses virNetDevGenerateName() for its veth device names since patch 2dd0fb492, so it should be using virNetDevReserveName() during daemon restart/reconnect to skip over the device names that are in use.
Signed-off-by: Laine Stump <laine@redhat.com> ---
I meant to mention this during review of the abovementioned patch, but forgot.
(NB: a couple days ago I *removed* similar code from this same spot, but it was trying to reserve the name of macvlan devices; a macvlan device is moved into the container's namespace at startup, so it is not visible to the host anyway. This new case is for the 1/2 of a veth pair that does remain in the host's namespace (type='bridge|network|ethernet' use a veth pair)
src/lxc/lxc_process.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 0f7c929535..a842ac91c5 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1640,6 +1640,30 @@ virLXCProcessReconnectNotifyNets(virDomainDefPtr def) for (i = 0; i < def->nnets; i++) { virDomainNetDefPtr net = def->nets[i]; + /* type='bridge|network|ethernet' interfaces may be using an + * autogenerated netdev name, so we should update the counter + * for autogenerated names to skip past this one. + */ + switch (virDomainNetGetActualType(net)) { + case VIR_DOMAIN_NET_TYPE_BRIDGE: + case VIR_DOMAIN_NET_TYPE_NETWORK: + case VIR_DOMAIN_NET_TYPE_ETHERNET: + virNetDevReserveName(net->ifname); + break; + case VIR_DOMAIN_NET_TYPE_DIRECT: + case VIR_DOMAIN_NET_TYPE_USER: + case VIR_DOMAIN_NET_TYPE_VHOSTUSER: + case VIR_DOMAIN_NET_TYPE_SERVER: + case VIR_DOMAIN_NET_TYPE_CLIENT: + case VIR_DOMAIN_NET_TYPE_MCAST: + case VIR_DOMAIN_NET_TYPE_INTERNAL: + case VIR_DOMAIN_NET_TYPE_HOSTDEV: + case VIR_DOMAIN_NET_TYPE_UDP: + case VIR_DOMAIN_NET_TYPE_VDPA: + case VIR_DOMAIN_NET_TYPE_LAST: + break; + } +
I remember Peter being picky about switch()-es and (almost) always he wanted me to add the default case with virReportEnumRangeError() despite the variable passed to switch() being verified earlier. IIUC his reasoning was that if we had a memory being overwritten somewhere it's better to error out (I say it's better to crash), but since I don't care that much, this could have:
default: virReportEnumRangeError(virDomainNetType, virDomainNetGetActualType(net)); return;
But if we add a default case to the switch, we won't get all the nice compile-time errors when we add a new value to the enum and forget to add it to every switch, will we?
We will. I worried about the same, but as it turned out we will get errors if a new item is added into the enum. Michal
Laine Stump wrote:
The FreeBSD version of virNetDevTapCreate() now calls virNetDevGenerateName(), and virNetDevGenerateName() understands that a blank ifname should be replaced with a generated name based on a device-type-specific template - so there is no longer any need for the higher level functions to stuff a template name ("vnet%d") into ifname.
Signed-off-by: Laine Stump <laine@redhat.com>
For this and 1/5: Reviewed-by: Roman Bogorodskiy <bogorodskiy@gmail.com> Thanks for this cleanup.
--- src/bhyve/bhyve_command.c | 7 ------- 1 file changed, 7 deletions(-)
diff --git a/src/bhyve/bhyve_command.c b/src/bhyve/bhyve_command.c index 4cf98c0eb1..daf313c9c1 100644 --- a/src/bhyve/bhyve_command.c +++ b/src/bhyve/bhyve_command.c @@ -79,13 +79,6 @@ bhyveBuildNetArgStr(const virDomainDef *def, goto cleanup; }
- if (!net->ifname || - 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"); - } - if (!dryRun) { if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, def->uuid, NULL, NULL, 0, -- 2.28.0
Roman Bogorodskiy
participants (3)
-
Laine Stump -
Michal Privoznik -
Roman Bogorodskiy