Re: [PATCH 6/5] lxc: skip the netdev autogenerated name counter past existing devices

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(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; >> +        } >> + > > 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?