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

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(a)redhat.com&gt; > --- 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; + }

Anyhow, you have my Reviewed-by: Erik Skultety <eskultet(a)redhat.com&gt; to this patch.