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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>>  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
>>
>>     
>
>   

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list





[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]