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

[Marek Marczykowski-Górecki]

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. Is it correct
(harmless) to call virReportError when no error occurred (function
returns >=0)? Maybe libvirt should check for duplicated devices at
higher level (-> for all drivers in the same place)?

> > +    if (virDomainNetFindIdx(vm->def, net) != -1) {
> > +        virReportError(VIR_ERR_OPERATION_FAILED,
> > +                _("device matching mac address %s already exists"),
> > +                virMacAddrFormat(&net->mac, mac));
> > +        return -1;
> > +    }
> > +
> >      if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> >          /* This is really a "smart hostdev", so it should be attached
> >           * as a hostdev (the hostdev code will reach over into the
> > @@ -2879,6 +2888,7 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
> >      virDomainHostdevDefPtr hostdev;
> >      virDomainHostdevDefPtr found;
> >      virDomainHostdevSubsysPCIPtr pcisrc;
> > +    char mac[VIR_MAC_STRING_BUFLEN];
> >  
> >      switch (dev->type) {
> >          case VIR_DOMAIN_DEVICE_DISK:
> > @@ -2896,6 +2906,12 @@ libxlDomainAttachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
> >  
> >          case VIR_DOMAIN_DEVICE_NET:
> >              net = dev->data.net;
> > +            if (virDomainNetFindIdx(vmdef, net) >= 0) {
> > +                virReportError(VIR_ERR_INVALID_ARG,
> > +                               _("network device with mac %s already exists."),
> > +                               virMacAddrFormat(&net->mac, mac));
> > +                return -1;
> > +            }
> >   
> 
> Ah, like you've done here :-).
> 
> >              if (virDomainNetInsert(vmdef, net))
> >                  return -1;
> >              dev->data.net = NULL;
> > @@ -3060,8 +3076,18 @@ libxlDomainDetachNetDevice(libxlDriverPrivatePtr driver,
> >      char mac[VIR_MAC_STRING_BUFLEN];
> >      int ret = -1;
> >  
> > -    if ((detachidx = virDomainNetFindIdx(vm->def, net)) < 0)
> > +    if ((detachidx = virDomainNetFindIdx(vm->def, net)) < 0) {
> > +        if (detachidx == -2) {
> > +            virReportError(VIR_ERR_OPERATION_FAILED,
> > +                    _("multiple devices matching mac address %s found"),
> > +                    virMacAddrFormat(&net->mac, mac));
> > +        } else {
> > +            virReportError(VIR_ERR_OPERATION_FAILED,
> > +                    _("network device %s not found"),
> > +                    virMacAddrFormat(&net->mac, mac));
> > +        }
> >   
> 
> virDomainNetFindIdx() handles the error reporting now.
> 
> >          return -1;
> > +    }
> >  
> >      detach = vm->def->nets[detachidx];
> >  
> > @@ -3136,6 +3162,7 @@ libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
> >      virDomainHostdevDefPtr hostdev, det_hostdev;
> >      virDomainNetDefPtr net;
> >      int idx;
> > +    char mac[VIR_MAC_STRING_BUFLEN];
> >  
> >      switch (dev->type) {
> >          case VIR_DOMAIN_DEVICE_DISK:
> > @@ -3150,8 +3177,18 @@ libxlDomainDetachDeviceConfig(virDomainDefPtr vmdef, virDomainDeviceDefPtr dev)
> >  
> >          case VIR_DOMAIN_DEVICE_NET:
> >              net = dev->data.net;
> > -            if ((idx = virDomainNetFindIdx(vmdef, net)) < 0)
> > +            if ((idx = virDomainNetFindIdx(vmdef, net)) < 0) {
> > +                if (idx == -2) {
> > +                    virReportError(VIR_ERR_OPERATION_FAILED,
> > +                            _("multiple devices matching mac address %s found"),
> > +                            virMacAddrFormat(&dev->data.net->mac, mac));
> > +                } else {
> > +                    virReportError(VIR_ERR_OPERATION_FAILED,
> > +                            _("network device %s not found"),
> > +                            virMacAddrFormat(&dev->data.net->mac, mac));
> > +                }
> >   
> 
> Same here.
> 
> 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/20150219/29845f70/attachment-0001.sig>