On 12/16/20 2:27 AM, Laine Stump wrote: This line ^^ contains some trailing spaces. Michal

Laine Stump wrote: For this and 1/5: Reviewed-by: Roman Bogorodskiy <bogorodskiy(a)gmail.com&gt; Thanks for this cleanup. Roman Bogorodskiy

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(a)redhat.com&gt; --- 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 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(a)redhat.com&gt; --- 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 4:27 PM, Michal Privoznik wrote: 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?