[libvirt] [PATCH 07/10] libxl: prevent attaching multiple netdevs with the same MAC

[Marek Marczykowski-Górecki]

On Thu, Feb 19, 2015 at 03:58:30PM -0700, Jim Fehlig wrote:
> Marek Marczykowski-Górecki wrote:
> > On Thu, Feb 19, 2015 at 03:10:13PM -0700, Jim Fehlig wrote:
> >> Jim Fehlig wrote:
> >>> Marek Marczykowski-Górecki wrote:
> >>>> On Thu, Feb 19, 2015 at 01:58:02PM -0700, Jim Fehlig wrote:
> >>>>> Marek Marczykowski-Górecki wrote:
> >>>>>> On Thu, Feb 19, 2015 at 11:43:15AM -0700, Jim Fehlig wrote:
> >>>>>>> Marek Marczykowski-Górecki wrote:
> >>>>>>>> It will not be possible to detach such device later. Also improve
> >>>>>>>> logging in such cases.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek at invisiblethingslab.com>
> >>>>>>>> ---
> >>>>>>>>  src/libxl/libxl_driver.c | 41 +++++++++++++++++++++++++++++++++++++++--
> >>>>>>>>  1 file changed, 39 insertions(+), 2 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> >>>>>>>> index ce3a99b..005cc96 100644
> >>>>>>>> --- a/src/libxl/libxl_driver.c
> >>>>>>>> +++ b/src/libxl/libxl_driver.c
> >>>>>>>> @@ -2787,6 +2787,7 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
> >>>>>>>>      int actualType;
> >>>>>>>>      libxl_device_nic nic;
> >>>>>>>>      int ret = -1;
> >>>>>>>> +    char mac[VIR_MAC_STRING_BUFLEN];
> >>>>>>>>  
> >>>>>>>>      /* preallocate new slot for device */
> >>>>>>>>      if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0)
> >>>>>>>> @@ -2801,6 +2802,14 @@ libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
> >>>>>>>>  
> >>>>>>>>      actualType = virDomainNetGetActualType(net);
> >>>>>>>>  
> >>>>>>>> +    /* -2 means "multiple matches" so then fail also */
> >>>>>>>>   
> >>>>>>>>       
> >>>>>>>>           
> >>>>>>>>             
> >>>>>>>>                 
> >>>>>>> No longer true after commit 2fbae1b2.  I think you just want to check if
> >>>>>>> virDomainNetFindIdx() >= 0, meaning the def already contains a net
> >>>>>>> device with the same mac address.
> >>>>>>>     
> >>>>>>>         
> >>>>>>>           
> >>>>>>>               
> >>>>>> But here the error is when the device *is* found, so the opposite case
> >>>>>> than already reported as an error by virDomainNetFindIdx.
> >>>>>>       
> >>>>>>         
> >>>>>>             
> >>>>> If you find an idx >= 0, then the domain def already contains a net
> >>>>> device with the same mac address, right?  In that case, you report an
> >>>>> error and return failure from libxlDomainAttachNetDevice().
> >>>>>     
> >>>>>       
> >>>>>           
> >>>> Right, but if I do not find one (idx == -1), I will proceed with
> >>>> (possibly successful) adding the device, while the error was already
> >>>> reported by virDomainNetFindIdx.
> >>>>   
> >>>>     
> >>>>         
> >>> Ah, right :-/.
> >>>
> >>> Another option: introduce virDomainHasNet() to detect if the domain def
> >>> already contains the net device.
> >>>   
> >>>       
> >> A better option would be to fix this in libxl, for the benefit of other
> >> libxl apps.
> >>     
> >
> > Actually libxl has no problem with duplicated mac addresses, its libvirt
> > that makes problem.
> 
> Yeah, it appears duplicate mac addresses are only valid if on different
> PCI devices. 

Is that true for libvirt generally (all drivers)?

> Back to virDomainHasNet? :-)  Or checking for the
> duplicate directly in libxlDomainAttachNetDevice()?

What do you mean by "directly"? This is exactly what my patch did (until
virDomainNetFindIdx stopped reporting duplicates).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150220/cff11ef4/attachment-0001.sig>