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

[Marek Marczykowski-Górecki]

On Thu, Feb 19, 2015 at 06:13:01PM -0700, Jim Fehlig wrote:
> Marek Marczykowski-Górecki wrote:
> > 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).
> >   
> 
> I mean coding up the search for an existing mac directly in
> libxlDomainAttachNetDevice().  E.g.

I see, IMO its better with (reusable...) virDomainHasNet.

> static int
> libxlDomainAttachNetDevice(libxlDriverPrivatePtr driver,
>                            libxlDomainObjPrivatePtr priv,
>                            virDomainObjPtr vm,
>                            virDomainNetDefPtr net)
> {
>     int actualType;
>     libxl_device_nic nic;
>     int ret = -1;
>     size_t i;
>     virDomainDefPtr def = vm->def;
> 
>     for (i = 0; i < def->nnets; i++) {
>         if (virMacAddrCmp(&def->nets[i]->mac, &net->mac) == 0)
>             error;
>      }
>     ...
> }
> 
> Regards,
> Jim

-- 
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/067f2f69/attachment-0001.sig>