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

[Michal Privoznik]

On 10/04/2017 03:28 PM, John Ferlan wrote:
> 
> 
> On 10/02/2017 07:01 AM, Michal Privoznik wrote:
>> When detaching an <interface/> from domain, it's MAC address is
> 
> s/from domain/from a domain/
> s/it's/the
> 
>> parsed and if not present one is generated. If, however, no
> 
> s/, however,//
> 
>> 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 at redhat.com>
>> ---
>>  src/conf/domain_conf.c | 45 +++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 35 insertions(+), 10 deletions(-)
>>
> 
> I had looked at this series too, but forgot to send this when something
> else caused me to reboot.... Patch 1 and 2 I had no comments...
> 
> My primary comment here is should the new message generated in some way
> indicate that it's the generated mac... It's no big deal, but just a
> thought and may help compared to having to look for the generic message.
> 
> 
>> 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;
>>  
>>          if ((matchidx >= 0) && !PCIAddrSpecified) {
>> @@ -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"));
> 
> multiple devices matching generated mac address found ?

I think this is misleading. The whole point of this patch is that if the
MAC was generated whilst parsing user's XML, it is not taken into
consideration. Therefore, other unique attributes are consulted - so far
it's just PCI address. Yes, the MAC address is still generated, but not
taken into consideration when matching. That's why I'd like to avoid
printing it out (it's confusing anyway), and hence this piece of code.

Michal