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;
+ }
+
if (MACAddrSpecified &&
virMacAddrCmp(&def->nets[i]->mac, &net->mac) != 0)
continue;
- if ((matchidx >= 0) && !PCIAddrSpecified) {
+ 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
@@ -15678,20 +15690,11 @@ 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;
- }
+
+ /* no PCI address given, so there may be multiple matches */
+ matchidx = i;
}
if (matchidx < 0) {
Anyhow, you have my
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx> to this patch.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
