Re: [PATCH 3/3] virDomainNetFindIdx: Ignore auto generated MAC addresses

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

 



On 10/04/2017 01:19 PM, Erik Skultety wrote:
> On Mon, Oct 02, 2017 at 01:01:20PM +0200, Michal Privoznik wrote:
>> When detaching an <interface/> from domain, it's MAC address is
>> parsed and if not present one is generated. If, however, no
>> corresponding interface is found in the domain, the following
>> error is reported:
>>
>> error: operation failed: no device matching mac address 52:54:00:75:32:5b found
>>
>> where the MAC address is the auto generated one. This might be
>> very confusing. Solution to this is to ignore auto generated MAC
>> address when looking up the device.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
> 
> While I think that a better fix would be to not generate the MAC during the
> parsing stage because it's semantically wrong, I also understand that changing
> that properly wouldn't be possible without a large refactor. Also, I'm not
> experienced enough in the networking code to be so sure that moving the burden
> of generating the MAC to individual drivers rather than doing it in the parser
> is a viable solution for all the drivers. Therefore I agree with this approach,
> it gets the job done, but if someone has a stronger opinion backed by solid
> arguments as for why it should be worth to refactor the whole thing, speak up.
> 
>>  src/conf/domain_conf.c | 45 +++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 35 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 87192eb2d..aab43d307 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -15634,11 +15634,17 @@ int virDomainNetInsert(virDomainDefPtr def, virDomainNetDefPtr net)
>>      return 0;
>>  }
>>
>> -/* virDomainNetFindIdx: search according to mac address and guest side
>> - *                      PCI address (if specified)
>> +/**
>> + * virDomainNetFindIdx:
>> + * @def: domain definition
>> + * @net: interface definition
>>   *
>> - * Return: index of match if unique match found
>> - *         -1 otherwise and an error is logged
>> + * Lookup domain's network interface based on passed @net
>> + * definition. If @net's MAC address was auto generated,
>> + * the MAC comparison is ignored.
>> + *
>> + * Return: index of match if unique match found,
>> + *         -1 otherwise and an error is logged.
>>   */
>>  int
>>  virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
>> @@ -15646,11 +15652,13 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
>>      size_t i;
>>      int matchidx = -1;
>>      char mac[VIR_MAC_STRING_BUFLEN];
>> +    bool MACAddrSpecified = !net->mac.generated;
>>      bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&net->info,
>>                                                            VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
>>
>>      for (i = 0; i < def->nnets; i++) {
>> -        if (virMacAddrCmp(&def->nets[i]->mac, &net->mac))
>> +        if (MACAddrSpecified &&
>> +            virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0)
>>              continue;
> 
> Code-wise, I would probably enhance this loop in the following manner:
> - move the PCI lookup code above ^this hunk because semantically, PCI match has
>   a precedence over MAC (probably because we allow multiple devices with the
>   same MAC, but there might be other historical reasons for that), so you save
>   some CPU cycles
> 
>>
>>          if ((matchidx >= 0) && !PCIAddrSpecified) {
> 
> - you could then drop the second operand ^here, because PCI would be handled
>   first (and it breaks the loop), so we get here only with MAC lookup
> 
>> @@ -15660,9 +15668,15 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
>>               * specify only vendor and product ID, and there may be
>>               * multiples of those.
>>               */
>> -            virReportError(VIR_ERR_OPERATION_FAILED,
>> -                           _("multiple devices matching mac address %s found"),
>> -                           virMacAddrFormat(&net->mac, mac));
>> +            if (MACAddrSpecified) {
>> +                virReportError(VIR_ERR_OPERATION_FAILED,
>> +                               _("multiple devices matching mac address %s found"),
>> +                               virMacAddrFormat(&net->mac, mac));
>> +            } else {
>> +                virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> +                               _("multiple matching devices found"));
>> +            }
>> +
>>              return -1;
>>          }
>>          if (PCIAddrSpecified) {
> - ^this block would be moved for the reasons described above
> 
>> @@ -15679,8 +15693,9 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
>>              matchidx = i;
> 
> - and ^this assignment could be left unguarded by any if-else statement
> 
> Eventually, a patch preceding yours could be similar to the following (I didn't
> bother rebasing and making the changes, I made them on top of this patch, I'm
> sure you'll get the picture):
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index aab43d307..db0451f45 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -15657,11 +15657,23 @@ virDomainNetFindIdx(virDomainDefPtr def, virDomainNetDefPtr net)
>                                                            VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
> 
>      for (i = 0; i < def->nnets; i++) {
> +        if (PCIAddrSpecified) {
> +            if (!virPCIDeviceAddressEqual(&def->nets[i]->info.addr.pci,
> +                                          &net->info.addr.pci))
> +                continue;
> +
> +            /* exit early if the pci address was specified and
> +             * it matches, as this guarantees no duplicates.
> +             */
> +            matchidx = i;
> +            break;
> +        }

I don't think this is right. Consider the following scenario. There are
two interfaces in a domain with $mac1, $pci1 and $mac2, $pci2. What
happens if user passes $mac1+$pci2 or vice versa? With my code, we error
out. With this suggestion we match device based on PCI address (even
though MAC address doesn't match). But I agree that we can do better
here, so I'm squashing this in:

diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c
index a9523df6a..34993de73 100644
--- i/src/conf/domain_conf.c
+++ w/src/conf/domain_conf.c
@@ -15676,7 +15676,12 @@ virDomainNetFindIdx(virDomainDefPtr def,
virDomainNetDefPtr net)
             virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0)
             continue;

-        if ((matchidx >= 0) && !PCIAddrSpecified) {
+        if (PCIAddrSpecified &&
+            !virPCIDeviceAddressEqual(&def->nets[i]->info.addr.pci,
+                                      &net->info.addr.pci))
+            continue;
+
+        if (matchidx >= 0) {
             /* there were multiple matches on mac address, and no
              * qualifying guest-side PCI address was given, so we must
              * fail (NB: a USB address isn't adequate, since it may
@@ -15694,19 +15699,8 @@ virDomainNetFindIdx(virDomainDefPtr def,
virDomainNetDefPtr net)

             return -1;
         }
-        if (PCIAddrSpecified) {
-            if (virPCIDeviceAddressEqual(&def->nets[i]->info.addr.pci,
-                                         &net->info.addr.pci)) {
-                /* exit early if the pci address was specified and
-                 * it matches, as this guarantees no duplicates.
-                 */
-                matchidx = i;
-                break;
-            }
-        } else {
-            /* no PCI address given, so there may be multiple matches */
-            matchidx = i;
-        }
+
+        matchidx = i;
     }

     if (matchidx < 0) {


> 
> Anyhow, you have my
> Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx> to this patch.
> 

Thanks.

Michal

--
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]
  Powered by Linux