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

[Michal Privoznik]

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 at 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