2:58 p.m.
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(a)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().
Regards,
Jim
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
>
>