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 <[email protected]> > > --- > > 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?
pgpPifxazHf3f.pgp
Description: PGP signature
-- libvir-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/libvir-list
