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

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(a)invisiblethingslab.com&gt; > --- > 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.

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