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

[Laine Stump]

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?

> 
> 
> At your discretion.
> 
>>           if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
>>               if (!conn && !(conn = virGetConnectNetwork()))
>>                   continue;
>>
> 
> Michal