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
